From 9fecd006741c80f14bdcb63da35db0095769f904 Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Mon, 4 May 2026 14:23:47 +0800 Subject: [PATCH 1/2] feat(logout): mirrorstack logout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revokes the current session via DELETE /v1/auth/sessions/current (refresh token in body, CLI flow) and wipes the local credentials file at ~/.config/mirrorstack/cli/credentials.json. Behaviour: - Already signed out (creds file missing) prints "✓ already signed out" and exits 0. No server call. - Server revoke succeeds → "✓ signed out". - Server returns 401 → treat as already-revoked (the platform's Revoke handler is idempotent), wipe local creds, success. - Server fails (network, 5xx) → still wipe local creds, print a yellow warning naming the error and noting the refresh token may remain valid until expiry. Exit 0 — the user's intent is "log me out" and refusing to clear local state because the platform is unreachable would leave them stuck. Implementation reuses the existing seams: - \`api::revoke_session\` follows the &Client signature shape from the recent api.rs cleanup, runs through \`unexpected_body_error\` for non-401 failures. - \`credentials::delete\` is idempotent (NotFound → Ok) so the command body doesn't need to special-case missing files. ## Tests - 27/27 (was 25). New: - api::tests::revoke_session_204_is_ok — happy path with body match - api::tests::revoke_session_401_is_unauthenticated — 401 mapping - credentials_lifecycle now exercises delete() success + idempotent second call. Single test by design — env-var mutation in with_temp_config_dir would race if split. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 1 + src/api.rs | 65 ++++++++++++++++++++++++++++++++++++++++ src/commands/logout.rs | 65 ++++++++++++++++++++++++++++++++++++++++ src/commands/mod.rs | 4 +++ src/credentials/mod.rs | 11 +++++++ src/credentials/tests.rs | 7 +++++ 6 files changed, 153 insertions(+) create mode 100644 src/commands/logout.rs diff --git a/README.md b/README.md index e6727bc..f8bb3b4 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,7 @@ Pre-built releases (brew, scoop, deb) coming with the next milestone. ```bash mirrorstack login # Sign in via OAuth (PKCE) +mirrorstack logout # Revoke the current session and wipe local credentials mirrorstack whoami # Print the currently signed-in user mirrorstack module init # Register a new module on the platform mirrorstack --help # Show help diff --git a/src/api.rs b/src/api.rs index 5c13e41..373733a 100644 --- a/src/api.rs +++ b/src/api.rs @@ -179,6 +179,44 @@ pub fn create_module( }) } +/// DELETE /v1/auth/sessions/current — revoke the supplied refresh token +/// (CLI flow: token in body, not cookie). The platform treats a missing +/// or already-revoked session as success, so callers can call this +/// idempotently. A 401 means the access token is gone but does NOT +/// necessarily mean the refresh token is — surface as Unauthenticated +/// and let the caller decide whether to still wipe local creds. +pub fn revoke_session( + http: &Client, + api_base: &str, + access_token: &str, + refresh_token: &str, +) -> Result<(), ApiError> { + #[derive(Serialize)] + struct Body<'a> { + refresh_token: &'a str, + } + let endpoint = format!( + "{}/v1/auth/sessions/current", + api_base.trim_end_matches('/') + ); + + let resp = http + .delete(&endpoint) + .bearer_auth(access_token) + .header("Accept", "application/json") + .json(&Body { refresh_token }) + .send()?; + + let status = resp.status(); + if status.is_success() { + return Ok(()); + } + if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::FORBIDDEN { + return Err(ApiError::Unauthenticated); + } + Err(unexpected_body_error(resp)) +} + /// Common tail for unexpected (non-success, non-typed) responses: read /// the body with [`http::read_capped`] and wrap as `ApiError::Unexpected`. /// If reading fails, the io error is folded into the body for diagnosis. @@ -380,6 +418,33 @@ mod tests { } } + #[test] + fn revoke_session_204_is_ok() { + let mut server = Server::new(); + let _m = server + .mock("DELETE", "/v1/auth/sessions/current") + .match_header("authorization", "Bearer AT") + .match_body(mockito::Matcher::JsonString( + r#"{"refresh_token":"RT"}"#.into(), + )) + .with_status(204) + .create(); + + revoke_session(&test_client(), &server.url(), "AT", "RT").expect("ok"); + } + + #[test] + fn revoke_session_401_is_unauthenticated() { + let mut server = Server::new(); + let _m = server + .mock("DELETE", "/v1/auth/sessions/current") + .with_status(401) + .create(); + + let err = revoke_session(&test_client(), &server.url(), "expired", "RT").unwrap_err(); + assert!(matches!(err, ApiError::Unauthenticated), "got {err:?}"); + } + #[test] fn create_module_4xx_without_envelope_is_unexpected() { let mut server = Server::new(); diff --git a/src/commands/logout.rs b/src/commands/logout.rs new file mode 100644 index 0000000..f5b555a --- /dev/null +++ b/src/commands/logout.rs @@ -0,0 +1,65 @@ +//! `mirrorstack logout` — revoke the current session and wipe local +//! credentials. Best-effort: a failed server revoke (network down, +//! 5xx) still wipes the local credentials and exits 0 with a warning. +//! The user's intent is "log me out"; refusing to clear local state +//! because the platform is unreachable would leave them stuck. + +use std::time::Duration; + +use anyhow::Result; +use clap::Args; +use console::style; + +use crate::api::{self, ApiError}; +use crate::credentials::{self, LoadError}; +use crate::http; + +use super::{DEFAULT_API_BASE, ENV_API_URL, resolve_base}; + +#[derive(Args)] +pub struct LogoutArgs {} + +pub fn run(_args: LogoutArgs) -> Result<()> { + let creds = match credentials::load() { + Ok(c) => c, + Err(LoadError::NotFound) => { + eprintln!("{} already signed out.", style("✓").green().bold()); + return Ok(()); + } + Err(e) => return Err(e.into()), + }; + + let api_base = resolve_base(ENV_API_URL, DEFAULT_API_BASE); + let client = http::client(Duration::from_secs(15))?; + + // Server revoke is best-effort: we always wipe local creds afterwards. + // A 401 means the access token was already gone; the platform's + // Revoke handler is idempotent on the refresh token, so any non-401 + // failure (network, 5xx) just means we couldn't tell the server. + let server_warn = match api::revoke_session( + &client, + &api_base, + &creds.access_token, + &creds.refresh_token, + ) { + Ok(()) => None, + Err(ApiError::Unauthenticated) => None, + Err(e) => Some(format!("{e}")), + }; + + credentials::delete()?; + + if let Some(reason) = server_warn { + eprintln!( + "{} server revoke failed ({reason}); local credentials wiped.", + style("warning:").yellow().bold(), + ); + eprintln!( + " {}", + style("the refresh token may still be valid until it expires.").dim(), + ); + } else { + eprintln!("{} signed out.", style("✓").green().bold()); + } + Ok(()) +} diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 62ffe55..6a9f0d6 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -5,6 +5,7 @@ use anyhow::Result; use clap::{Parser, Subcommand}; mod login; +mod logout; mod module; mod whoami; @@ -43,6 +44,8 @@ pub struct Cli { enum Command { /// Sign in to MirrorStack via OAuth. Login(login::LoginArgs), + /// Sign out: revoke the current session and remove local credentials. + Logout(logout::LogoutArgs), /// Print the currently signed-in user. Whoami(whoami::WhoamiArgs), /// Manage developer modules (the per-developer reusable units installed @@ -54,6 +57,7 @@ impl Cli { pub fn run(self) -> Result<()> { match self.command { Command::Login(args) => login::run(args), + Command::Logout(args) => logout::run(args), Command::Whoami(args) => whoami::run(args), Command::Module(args) => module::run(args), } diff --git a/src/credentials/mod.rs b/src/credentials/mod.rs index 9205e65..2d19b73 100644 --- a/src/credentials/mod.rs +++ b/src/credentials/mod.rs @@ -90,6 +90,17 @@ pub fn load_or_login_hint() -> Result { } } +/// Wipe the credentials file. Idempotent — a missing file is treated as +/// success so callers don't need to special-case "already logged out". +pub fn delete() -> Result<()> { + let p = path().context("credentials: path")?; + match fs::remove_file(&p) { + Ok(()) => Ok(()), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(anyhow::anyhow!("credentials: remove: {e}")), + } +} + /// Encode `SystemTime` as RFC 3339 in JSON. Avoids pulling chrono just /// for this; serde_json's default for SystemTime is a tagged struct /// which is ugly on disk. diff --git a/src/credentials/tests.rs b/src/credentials/tests.rs index 95cd581..67f238f 100644 --- a/src/credentials/tests.rs +++ b/src/credentials/tests.rs @@ -69,4 +69,11 @@ fn credentials_lifecycle() { let mode = std::fs::metadata(&p).expect("stat").permissions().mode() & 0o777; assert_eq!(mode, 0o600, "got {mode:o}, want 0600"); } + + // delete() wipes the file; subsequent load() returns NotFound. + delete().expect("delete"); + assert!(matches!(load(), Err(LoadError::NotFound))); + + // delete() is idempotent — no error when called against missing file. + delete().expect("delete idempotent"); } From e40d367379f8a0fc6fc07f6fde46e381081ef894 Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Mon, 4 May 2026 14:27:30 +0800 Subject: [PATCH 2/2] refactor(logout): /simplify pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract \`commands::ok_mark()\` and \`commands::warn_prefix()\` for the green-✓ and yellow-warning prefixes — same \`style(...).bold()\` chain was hand-rolled at 5 call sites across logout.rs and module.rs. - \`credentials::delete\` error includes the file path so the user can manually remove it on permission errors instead of seeing a bare "permission denied". - Drop the stale \`#[allow(dead_code)]\` on \`LoadError\` — \`NotFound\` is now matched in load(), load_or_login_hint(), and logout::run(). Confirmed unnecessary by clippy passing without it. - New test \`revoke_session_5xx_is_unexpected\` covers the unexpected body path that logout's \`server_warn\` branch exercises in practice. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/api.rs | 19 +++++++++++++++++++ src/commands/logout.rs | 8 ++++---- src/commands/mod.rs | 11 +++++++++++ src/commands/module.rs | 6 +++--- src/credentials/mod.rs | 5 +++-- 5 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/api.rs b/src/api.rs index 373733a..9b0561d 100644 --- a/src/api.rs +++ b/src/api.rs @@ -445,6 +445,25 @@ mod tests { assert!(matches!(err, ApiError::Unauthenticated), "got {err:?}"); } + #[test] + fn revoke_session_5xx_is_unexpected() { + let mut server = Server::new(); + let _m = server + .mock("DELETE", "/v1/auth/sessions/current") + .with_status(503) + .with_body("upstream timeout") + .create(); + + let err = revoke_session(&test_client(), &server.url(), "AT", "RT").unwrap_err(); + match err { + ApiError::Unexpected { status, body } => { + assert_eq!(status, 503); + assert!(body.contains("upstream timeout"), "got body {body:?}"); + } + other => panic!("expected Unexpected, got {other:?}"), + } + } + #[test] fn create_module_4xx_without_envelope_is_unexpected() { let mut server = Server::new(); diff --git a/src/commands/logout.rs b/src/commands/logout.rs index f5b555a..b57e3a9 100644 --- a/src/commands/logout.rs +++ b/src/commands/logout.rs @@ -14,7 +14,7 @@ use crate::api::{self, ApiError}; use crate::credentials::{self, LoadError}; use crate::http; -use super::{DEFAULT_API_BASE, ENV_API_URL, resolve_base}; +use super::{DEFAULT_API_BASE, ENV_API_URL, ok_mark, resolve_base, warn_prefix}; #[derive(Args)] pub struct LogoutArgs {} @@ -23,7 +23,7 @@ pub fn run(_args: LogoutArgs) -> Result<()> { let creds = match credentials::load() { Ok(c) => c, Err(LoadError::NotFound) => { - eprintln!("{} already signed out.", style("✓").green().bold()); + eprintln!("{} already signed out.", ok_mark()); return Ok(()); } Err(e) => return Err(e.into()), @@ -52,14 +52,14 @@ pub fn run(_args: LogoutArgs) -> Result<()> { if let Some(reason) = server_warn { eprintln!( "{} server revoke failed ({reason}); local credentials wiped.", - style("warning:").yellow().bold(), + warn_prefix(), ); eprintln!( " {}", style("the refresh token may still be valid until it expires.").dim(), ); } else { - eprintln!("{} signed out.", style("✓").green().bold()); + eprintln!("{} signed out.", ok_mark()); } Ok(()) } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 6a9f0d6..41fcf51 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -3,6 +3,7 @@ use anyhow::Result; use clap::{Parser, Subcommand}; +use console::{StyledObject, style}; mod login; mod logout; @@ -32,6 +33,16 @@ pub(crate) fn resolve_base(env_var: &str, default: &str) -> String { std::env::var(env_var).unwrap_or_else(|_| default.into()) } +/// Green bold "✓" — shared status prefix for success lines across commands. +pub(crate) fn ok_mark() -> StyledObject<&'static str> { + style("✓").green().bold() +} + +/// Yellow bold "warning:" — shared prefix for non-fatal advisory lines. +pub(crate) fn warn_prefix() -> StyledObject<&'static str> { + style("warning:").yellow().bold() +} + /// Official command-line tool for the MirrorStack platform. #[derive(Parser)] #[command(name = "mirrorstack", version, about, long_about = None)] diff --git a/src/commands/module.rs b/src/commands/module.rs index 5c45ce2..0bdaa7d 100644 --- a/src/commands/module.rs +++ b/src/commands/module.rs @@ -21,7 +21,7 @@ use crate::http; use super::{ DEFAULT_API_BASE, DEFAULT_APPS_API_BASE, DEFAULT_WEB_BASE, ENV_API_URL, ENV_APPS_API_URL, - ENV_WEB_URL, resolve_base, + ENV_WEB_URL, ok_mark, resolve_base, }; #[derive(Args)] @@ -239,7 +239,7 @@ where fn print_created(username: &str, slug: &str, id: &str) { eprintln!( "{} created {}", - style("✓").green().bold(), + ok_mark(), style(format!("@{username}/{slug}")).cyan().bold(), ); eprintln!(" {} {}", style("id:").dim(), id); @@ -248,7 +248,7 @@ fn print_created(username: &str, slug: &str, id: &str) { fn print_already_exists(username: &str, slug: &str, id: Option<&str>) { eprintln!( "{} {} already exists; {} continuing.", - style("✓").green().bold(), + ok_mark(), style(format!("@{username}/{slug}")).cyan().bold(), style("--used set,").dim(), ); diff --git a/src/credentials/mod.rs b/src/credentials/mod.rs index 2d19b73..fc422bc 100644 --- a/src/credentials/mod.rs +++ b/src/credentials/mod.rs @@ -23,7 +23,6 @@ pub struct Credentials { } #[derive(Debug, Error)] -#[allow(dead_code)] // NotFound is matched in load(); load() is API for future commands pub enum LoadError { #[error("credentials: not found (run `mirrorstack login`)")] NotFound, @@ -92,12 +91,14 @@ pub fn load_or_login_hint() -> Result { /// Wipe the credentials file. Idempotent — a missing file is treated as /// success so callers don't need to special-case "already logged out". +/// On other errors (typically permissions) the path is included in the +/// message so the user can act manually. pub fn delete() -> Result<()> { let p = path().context("credentials: path")?; match fs::remove_file(&p) { Ok(()) => Ok(()), Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(anyhow::anyhow!("credentials: remove: {e}")), + Err(e) => Err(anyhow::anyhow!("credentials: remove {}: {e}", p.display())), } }