feat(cli): name the machine on the approval page

crates/mergify-auth/src/device.rs

@@ -193,9 +193,34 @@ pub fn client(api_url: Url) -> Result<HttpClient, CliError> {
 
 /// Open a grant: ask the server for the code pair the user is about
 /// to approve.
-pub async fn authorize(client: &HttpClient) -> Result<Authorization, CliError> {
+///
+/// `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 `<client name> on <device name>` 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<Authorization, CliError> {
+    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::<Authorization, OAuthError>(DEVICE_CODE_PATH, &[("client_id", CLIENT_ID)])
+        .post_form::<Authorization, OAuthError>(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);
     }

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)]

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]

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 <hostname>` 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<String> {
+    // 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<String> {
+    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<String> {
+    // `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<String> {
+    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);
+    }
+}