From 7e3ded6858b34c15bf6870fd15d4bd4fb9b3b276 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Mon, 5 Oct 2026 17:59:01 +0200 Subject: [PATCH] feat(auth): add `auth login --device-name` to override the hostname `mergify auth login` sends the machine's hostname as `device_name` so the approval page can default the token name to this machine. Hostnames often carry a person's name, and `POST /v1/oauth/device/code` is unauthenticated: the value sits on the pending grant server-side before anyone approves, so editing the field on the page does not un-send it. It is also a poor label for someone with several machines on one account. `--device-name NAME` replaces the detected hostname and is sent as typed; the server caps and sanitizes it. A blank value (`--device-name=`, which also survives Windows PowerShell 5.1 dropping a `""` argument) sends no name at all, the same request a machine that cannot name itself already makes, and the server keeps its own default label. Without the flag nothing changes. Fixes MRGFY-9268 Co-Authored-By: Claude Opus 5.5 Change-Id: If4f977008ded4d2acd0cb1ce5492c60c3f4df4e3 --- README.md | 4 + crates/mergify-auth/src/login.rs | 105 +++++++++++++----- crates/mergify-cli/src/main.rs | 49 +++++++- .../mergify__tests__cli_schema_golden.snap | 18 +++ 4 files changed, 147 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index c88b9c97..88865b9a 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,10 @@ restricted file under your configuration directory. `mergify auth status` says which account you are signed in as, and `mergify auth logout` asks the Mergify API to revoke the credential rather than only deleting the local copy. +The approval page labels the new token `Mergify CLI on `. Pass +`--device-name "work laptop"` to name the machine yourself, or `--device-name=` +to keep the hostname from leaving the machine at all. + The commands that talk to the **Mergify API** resolve a credential in this order: diff --git a/crates/mergify-auth/src/login.rs b/crates/mergify-auth/src/login.rs index 781b68e5..36e93f1c 100644 --- a/crates/mergify-auth/src/login.rs +++ b/crates/mergify-auth/src/login.rs @@ -26,6 +26,13 @@ pub struct LoginOptions<'a> { /// printed either way, and a browser that will not open does /// not fail the login. pub browser: Option<&'a dyn Browser>, + /// `--device-name`: what to call this machine on the approval + /// page instead of its hostname. `None` detects the hostname, + /// and a blank name sends none at all, for a user who does not + /// want the hostname to leave the machine: the grant request is + /// unauthenticated and the server holds the value before anyone + /// approves, so editing the field on the page is too late. + pub device_name: Option<&'a str>, } /// What `auth login` produced, for the JSON rendering `Output` @@ -60,8 +67,13 @@ pub async fn run(opts: LoginOptions<'_>, output: &mut dyn Output) -> Result<(), // every machine shares — the page's own helper text already asks // the user to name it after the machine. Optional: a machine // that cannot name itself, or a deployment that predates the - // field, falls back to that shared default. - let device_name = machine::name().await; + // field, falls back to that shared default. `--device-name` + // replaces the detected name, and a blank one is the opt-out: + // the same request a machine that cannot name itself makes. + let device_name = match opts.device_name { + Some(name) => (!name.trim().is_empty()).then(|| name.to_string()), + None => machine::name().await, + }; let authorization = device::authorize(&client, device_name.as_deref()).await?; let opened = open_verification_page(opts.browser, verification_url(&authorization)); output.status(&instructions(&authorization, opened))?; @@ -377,6 +389,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -417,6 +430,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -449,6 +463,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -480,6 +495,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -529,6 +545,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -564,6 +581,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -611,6 +629,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -661,6 +680,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -709,6 +729,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) @@ -753,6 +774,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: Some(&browser), + device_name: None, }, &mut captured.output, ) @@ -792,6 +814,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: Some(&browser), + device_name: None, }, &mut captured.output, ) @@ -816,6 +839,34 @@ mod tests { }); } + /// The form body `run` sent on the grant request. + async fn grant_body(device_name: Option<&str>) -> String { + let server = MockServer::start().await; + mount_flow(&server, true).await; + let (dir, store) = file_store(); + let mut captured = Captured::human(); + + run( + LoginOptions { + api_url: Some(&server.uri()), + store: &store, + browser: None, + device_name, + }, + &mut captured.output, + ) + .await + .unwrap(); + + let requests = server.received_requests().await.unwrap(); + let grant = requests + .iter() + .find(|r| r.url.path() == "/v1/oauth/device/code") + .expect("the grant request"); + drop(dir); + String::from_utf8_lossy(&grant.body).into_owned() + } + // The wiring, not the renderer: `device_name` has to leave this // command on the grant request, or the approval page goes back // to naming every machine "Mergify CLI". Asserted against what @@ -824,33 +875,36 @@ mod tests { #[test] fn login_tells_the_server_which_machine_asked() { with_mergify_token(None, async { - let server = MockServer::start().await; - mount_flow(&server, true).await; - let (dir, store) = file_store(); - let mut captured = Captured::human(); - - run( - LoginOptions { - api_url: Some(&server.uri()), - store: &store, - browser: None, - }, - &mut captured.output, - ) - .await - .unwrap(); - - let requests = server.received_requests().await.unwrap(); - let grant = requests - .iter() - .find(|r| r.url.path() == "/v1/oauth/device/code") - .expect("the grant request"); - let body = String::from_utf8_lossy(&grant.body); + let body = grant_body(None).await; match crate::machine::name().await { Some(_) => assert!(body.contains("device_name="), "got {body:?}"), None => assert_eq!(body, "client_id=mergify-cli", "got {body:?}"), } - drop(dir); + }); + } + + // `--device-name` replaces the hostname rather than joining it, + // and goes out as typed: the server sanitizes, not the CLI. + #[test] + fn device_name_overrides_the_hostname() { + with_mergify_token(None, async { + let body = grant_body(Some("work laptop")).await; + assert_eq!( + body, "client_id=mergify-cli&device_name=work+laptop", + "got {body:?}" + ); + }); + } + + // `--device-name ""` is the opt-out: no hostname leaves the + // machine, and the server keeps its own default label. + #[test] + fn empty_device_name_sends_none() { + with_mergify_token(None, async { + for blank in ["", " "] { + let body = grant_body(Some(blank)).await; + assert_eq!(body, "client_id=mergify-cli", "for {blank:?}, got {body:?}"); + } }); } @@ -869,6 +923,7 @@ mod tests { api_url: Some(&server.uri()), store: &store, browser: None, + device_name: None, }, &mut captured.output, ) diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index 2af94785..3176ee33 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -165,6 +165,7 @@ enum NativeCommand { AuthLogin { opts: AuthOpts, no_browser: bool, + device_name: Option, }, /// `mergify auth logout [--api-url URL]` — revoke the stored /// credential and forget it. @@ -936,9 +937,14 @@ fn dispatch_from_parsed(parsed: CliRoot) -> Dispatch { Subcommands::Auth(AuthArgs { api_url, command }) => { let opts = AuthOpts { api_url }; Dispatch::Native(match command { - AuthSubcommand::Login(AuthLoginArgs { no_browser }) => { - NativeCommand::AuthLogin { opts, no_browser } - } + AuthSubcommand::Login(AuthLoginArgs { + no_browser, + device_name, + }) => NativeCommand::AuthLogin { + opts, + no_browser, + device_name, + }, AuthSubcommand::Logout => NativeCommand::AuthLogout(opts), AuthSubcommand::Status => NativeCommand::AuthStatus(opts), }) @@ -1567,7 +1573,11 @@ fn run_native(cmd: NativeCommand) -> ExitCode { | NativeCommand::InternalManPage => { unreachable!("introspection commands are handled before the runtime starts") } - NativeCommand::AuthLogin { opts, no_browser } => { + NativeCommand::AuthLogin { + opts, + no_browser, + device_name, + } => { let store = mergify_core::CredentialStore::discover(); let system_browser = mergify_auth::browser::SystemBrowser; let browser: Option<&dyn mergify_auth::browser::Browser> = @@ -1577,6 +1587,7 @@ fn run_native(cmd: NativeCommand) -> ExitCode { api_url: opts.api_url.as_deref(), store: &store, browser, + device_name: device_name.as_deref(), }, &mut output, ) @@ -4566,6 +4577,11 @@ struct AuthLoginArgs { /// Do not open a browser; only print the URL to open. #[arg(long = "no-browser")] no_browser: bool, + + /// Name this machine on the approval page instead of sending its + /// hostname. `--device-name=` sends no name at all. + #[arg(long = "device-name", value_name = "NAME")] + device_name: Option, } #[derive(Subcommand)] @@ -5076,6 +5092,31 @@ mod tests { assert!(!no_browser, "a browser is the default"); } + // `--device-name ""` is the opt-out, so clap must hand the empty + // string through rather than reject it or read it as absent: + // absent means "send the hostname", the opposite of what was asked. + #[test] + fn auth_login_carries_device_name_through_dispatch() { + for (args, expected) in [ + ( + &["auth", "login", "--device-name", "work laptop"][..], + Some("work laptop"), + ), + (&["auth", "login", "--device-name", ""][..], Some("")), + // The portable spelling: Windows PowerShell 5.1 drops a + // `""` argument before it reaches the binary. + (&["auth", "login", "--device-name="][..], Some("")), + (&["auth", "login"][..], None), + ] { + let Dispatch::Native(NativeCommand::AuthLogin { device_name, .. }) = + dispatch_from_parsed(parse(args)) + else { + panic!("auth login must dispatch to the native AuthLogin variant"); + }; + assert_eq!(device_name.as_deref(), expected, "for {args:?}"); + } + } + #[test] fn events_dispatches_natively_with_every_flag() { let parsed = parse(&[ diff --git a/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap b/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap index 0aad77ba..1f00cf4f 100644 --- a/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap +++ b/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap @@ -120,6 +120,24 @@ expression: schema "short": null, "valueHint": null, "valueNames": [] + }, + { + "default": null, + "env": null, + "global": false, + "help": "Name this machine on the approval page instead of sending its hostname. `--device-name=` sends no name at all", + "id": "device_name", + "kind": "option", + "long": "device-name", + "longHelp": "Name this machine on the approval page instead of sending its hostname. `--device-name=` sends no name at all", + "numArgs": "1", + "possibleValues": [], + "required": false, + "short": null, + "valueHint": null, + "valueNames": [ + "NAME" + ] } ], "commands": [],