diff --git a/crates/mergify-auth/src/device.rs b/crates/mergify-auth/src/device.rs index e3f62dc2..4bff2b9c 100644 --- a/crates/mergify-auth/src/device.rs +++ b/crates/mergify-auth/src/device.rs @@ -193,9 +193,34 @@ pub fn client(api_url: Url) -> Result { /// Open a grant: ask the server for the code pair the user is about /// to approve. -pub async fn authorize(client: &HttpClient) -> Result { +/// +/// `device_name` names the machine asking, so the approval page can +/// default the token's name to this one rather than to the client +/// name every machine shares. **Send the bare hostname**: the server +/// composes ` on ` itself, so a client +/// that sent "Mergify CLI on host" would produce "Mergify CLI on +/// Mergify CLI on host". It also sanitizes the value and caps the +/// composed label, which is why nothing is trimmed on this side. +/// +/// Optional on the wire: a grant without the field is what every +/// client sent until it existed, and the server keeps its own +/// default for one. It is additive against a deployment that +/// predates it, too — that endpoint reads its form fields by name +/// rather than as a strict model, so an unknown one is ignored and +/// an older on-prem still logs in. +pub async fn authorize( + client: &HttpClient, + device_name: Option<&str>, +) -> Result { + let mut form = vec![("client_id", CLIENT_ID)]; + // Omitted rather than sent empty when this machine cannot name + // itself: a grant without the field is the case every client had + // until now, and the server still has its own default for it. + if let Some(device_name) = device_name { + form.push(("device_name", device_name)); + } match client - .post_form::(DEVICE_CODE_PATH, &[("client_id", CLIENT_ID)]) + .post_form::(DEVICE_CODE_PATH, &form) .await? { ApiOutcome::Ok(authorization) => checked(authorization), @@ -423,7 +448,7 @@ mod tests { .mount(&server) .await; - let authorization = authorize(&test_client(&server)).await.unwrap(); + let authorization = authorize(&test_client(&server), None).await.unwrap(); assert_eq!(authorization.device_code, "dev-secret"); assert_eq!(authorization.user_code, "BCDF-GHJK"); assert_eq!( @@ -435,6 +460,55 @@ mod tests { assert_eq!(schedule.expires_in, Duration::from_secs(600)); } + // The machine's own name, so the approval page can default the + // token to "Mergify CLI on work-laptop" instead of the client + // name every machine shares. + #[tokio::test] + async fn authorize_sends_the_device_name() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/oauth/device/code")) + .and(body_string_contains("client_id=mergify-cli")) + .and(body_string_contains("device_name=work-laptop")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "device_code": "dev-secret", + "user_code": "BCDF-GHJK", + "verification_uri": "https://dashboard.mergify.com/device", + }))) + .expect(1) + .mount(&server) + .await; + + authorize(&test_client(&server), Some("work-laptop")) + .await + .unwrap(); + } + + // A machine that cannot name itself sends no field at all. An + // empty one is not the same thing: the server would sanitize it + // back to nothing, and a deployment that predates the field has + // to see exactly the request it has always seen. + #[tokio::test] + async fn authorize_omits_the_device_name_when_there_is_none() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/oauth/device/code")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "device_code": "dev-secret", + "user_code": "BCDF-GHJK", + "verification_uri": "https://dashboard.mergify.com/device", + }))) + .expect(1) + .mount(&server) + .await; + + authorize(&test_client(&server), None).await.unwrap(); + + let requests = server.received_requests().await.unwrap(); + let body = String::from_utf8_lossy(&requests[0].body); + assert_eq!(body, "client_id=mergify-cli", "got {body:?}"); + } + // A deployment that omits the two optional numbers still has to // produce a usable schedule, at the RFC's defaults. #[tokio::test] @@ -451,7 +525,7 @@ mod tests { .mount(&server) .await; - let authorization = authorize(&test_client(&server)).await.unwrap(); + let authorization = authorize(&test_client(&server), None).await.unwrap(); assert_eq!(authorization.verification_uri_complete, None); let schedule = PollSchedule::from_authorization(&authorization); assert_eq!(schedule.interval, DEFAULT_INTERVAL); @@ -532,7 +606,7 @@ mod tests { .mount(&server) .await; - let err = authorize(&test_client(&server)).await.unwrap_err(); + let err = authorize(&test_client(&server), None).await.unwrap_err(); assert!(err.to_string().contains("only http(s)"), "got {err}"); } @@ -556,7 +630,7 @@ mod tests { .mount(&server) .await; - let authorization = authorize(&test_client(&server)).await.unwrap(); + let authorization = authorize(&test_client(&server), None).await.unwrap(); assert_eq!( authorization.user_code, "BCDF[2Kgo to https://attacker.test", @@ -631,7 +705,7 @@ mod tests { .mount(&server) .await; - let err = authorize(&test_client(&server)).await.unwrap_err(); + let err = authorize(&test_client(&server), None).await.unwrap_err(); assert!(err.to_string().contains("Unknown client_id."), "got {err}"); assert_eq!(err.exit_code(), mergify_core::ExitCode::MergifyApiError); } diff --git a/crates/mergify-auth/src/lib.rs b/crates/mergify-auth/src/lib.rs index ec13d973..dc4e4eb7 100644 --- a/crates/mergify-auth/src/lib.rs +++ b/crates/mergify-auth/src/lib.rs @@ -9,6 +9,8 @@ //! - [`identity`] — `GET /v1/user`, the only way to turn a //! credential into an account name and the only way to tell a //! live one from a revoked one. +//! - [`machine`] — the hostname a login sends so the approval page +//! can name the machine asking. //! - [`login`] / [`logout`] / [`status`] — the three commands. //! //! The credential itself is stored by @@ -20,6 +22,7 @@ pub mod device; pub mod identity; pub mod login; pub mod logout; +pub mod machine; pub mod status; #[cfg(test)] diff --git a/crates/mergify-auth/src/login.rs b/crates/mergify-auth/src/login.rs index 36aa7ab6..781b68e5 100644 --- a/crates/mergify-auth/src/login.rs +++ b/crates/mergify-auth/src/login.rs @@ -16,6 +16,7 @@ use url::Url; use crate::browser::Browser; use crate::device; use crate::identity; +use crate::machine; pub struct LoginOptions<'a> { pub api_url: Option<&'a str>, @@ -54,7 +55,14 @@ pub async fn run(opts: LoginOptions<'_>, output: &mut dyn Output) -> Result<(), // store, so this costs nothing. let previous = opts.store.get(&api_url)?; - let authorization = device::authorize(&client).await?; + // The machine's own name, so the approval page's Token name + // field defaults to this machine instead of to the client name + // 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; + 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))?; @@ -808,6 +816,44 @@ mod tests { }); } + // 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 + // this machine actually calls itself, so the test says the same + // thing on a laptop and in a container with no `hostname`. + #[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); + 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); + }); + } + // `--no-browser`: nothing is spawned, and the instructions go // back to telling the user to open the URL themselves. #[test] diff --git a/crates/mergify-auth/src/machine.rs b/crates/mergify-auth/src/machine.rs new file mode 100644 index 00000000..ec730038 --- /dev/null +++ b/crates/mergify-auth/src/machine.rs @@ -0,0 +1,135 @@ +//! The name this machine calls itself. +//! +//! One caller: the `device_name` `auth login` sends when it opens a +//! grant, so the approval page's Token name field defaults to +//! `Mergify CLI on ` rather than to the static client name +//! every machine shares. The field is optional on the wire — a +//! machine that cannot name itself sends nothing and the server +//! keeps its own default — so nothing here is worth failing a login +//! over, and every failure is a `None`. + +use std::process::Command; +use std::time::Duration; + +/// How long `hostname` gets. It reads a name the kernel already +/// holds, so this is not a budget — it is the ceiling on how long a +/// wedged shim on `PATH` can hold up a login. `login` runs this +/// before anything is on screen, and a blank terminal is a worse +/// failure than a token named "Mergify CLI". +const LOOKUP_TIMEOUT: Duration = Duration::from_secs(2); + +/// The machine's hostname, as the machine reports it. +/// +/// `hostname(1)` first: it ships on all three platforms and is the +/// only answer that is still right after the name changes under a +/// running shell. The variables are the fallback for a container +/// with no `hostname` on `PATH`. +/// +/// Verbatim, on purpose. Not lower-cased, not stripped of a +/// `.local` suffix, not cut to length. The server sanitizes the +/// value (printable ASCII, collapsed whitespace, 60 characters) and +/// composes the label itself, so a client that trimmed first would +/// only disagree with it. +pub async fn name() -> Option { + // Off the runtime thread and on a deadline. The value decorates + // the approval page and nothing waits on it, so a `hostname` + // that does not come back is dropped rather than waited for. + let from_command = + tokio::time::timeout(LOOKUP_TIMEOUT, tokio::task::spawn_blocking(from_command)) + .await + .ok() + .and_then(Result::ok) + .flatten(); + from_command.or_else(from_env) +} + +fn from_command() -> Option { + let output = Command::new("hostname").output().ok()?; + if !output.status.success() { + return None; + } + // Lossy because a name is worth more than the byte that did not + // decode: the server maps anything outside printable ASCII to a + // space anyway. + as_name(&String::from_utf8_lossy(&output.stdout)) +} + +fn from_env() -> Option { + // `COMPUTERNAME` is Windows' own; `HOSTNAME` is exported by some + // shells and images and is worth asking for once the command is + // gone. + mergify_core::env::var_non_empty("COMPUTERNAME") + .or_else(|| mergify_core::env::var_non_empty("HOSTNAME")) + .as_deref() + .and_then(as_name) +} + +/// What a `hostname` process printed, as a name. +/// +/// Dropping the line's own newline is reading the command's output, +/// not sanitizing the value — what is inside the line goes to the +/// server as it stands. A name that is only whitespace is no name. +fn as_name(raw: &str) -> Option { + let trimmed = raw.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + // Whatever this machine is called, what comes back has to be + // sendable: the newline `hostname` prints must not reach the + // form body. + #[tokio::test] + async fn the_name_of_the_machine_running_the_suite_is_sendable() { + if let Some(name) = name().await { + assert!(!name.is_empty()); + assert_eq!(name.trim(), name, "a name must carry no surrounding space"); + } + } + + #[test] + fn a_printed_name_loses_its_line_and_nothing_else() { + assert_eq!(as_name("work-laptop\n").as_deref(), Some("work-laptop")); + // The `.local` stays: the server composes and caps the + // label, and a client that trimmed here would disagree with + // what the approval page shows. + assert_eq!( + as_name("some-macbook.local\n").as_deref(), + Some("some-macbook.local"), + ); + } + + #[test] + fn a_machine_that_names_itself_nothing_has_no_name() { + assert_eq!(as_name(" \n"), None); + assert_eq!(as_name(""), None); + } + + // The container case: no `hostname` on PATH, and the image sets + // the variable instead. + #[test] + fn the_variables_are_the_fallback() { + let from_windows = temp_env::with_vars( + [ + ("COMPUTERNAME", Some("WIN-BOX")), + ("HOSTNAME", Some("ignored")), + ], + from_env, + ); + assert_eq!(from_windows.as_deref(), Some("WIN-BOX")); + + let from_shell = temp_env::with_vars( + [("COMPUTERNAME", None), ("HOSTNAME", Some("build-42"))], + from_env, + ); + assert_eq!(from_shell.as_deref(), Some("build-42")); + + let from_nothing = temp_env::with_vars( + [("COMPUTERNAME", None::<&str>), ("HOSTNAME", None)], + from_env, + ); + assert_eq!(from_nothing, None); + } +}