From 15ef21b6551b2244c5adad8ae10d6283d9af04e3 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Fri, 11 Sep 2026 17:23:57 +0200 Subject: [PATCH] feat(cli): open the browser on `auth login` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `auth login` printed a URL and left the user to click it. It now opens the approval page itself, which is the last bit of ceremony between running the command and approving the grant. Opening is a convenience and never a step in the flow: - The URL is still printed, always. A CI runner, a container and an SSH session have no browser to open, and a spawned opener is not a window on screen either — the terminal is the only place the user can be sure to find the address again. The instructions say "Opening your browser […] If nothing opens, go to:" only when an opener actually started, so the line never claims something that did not happen. - `--no-browser` skips it. - Every failure is a `debug!` line. The login proceeds either way. No crate for it: each platform is one spawn (`open`, `xdg-open`, `cmd /C start`), which is not worth a dependency under `deny.toml`. Three things that are not obvious in those twenty lines: - Nothing is spawned when the screen the opener would use is not the one the user is looking at. On Linux that is an unset `DISPLAY` and `WAYLAND_DISPLAY`, where `xdg-open` falls through to a terminal browser (`www-browser`, `w3m`, `lynx`) and takes over the very terminal the code is being read from; on macOS, which has no `DISPLAY` to consult, it is `SSH_CONNECTION` / `SSH_TTY`, where `open` reaches the screen of whoever is sitting at the machine and hands them a page carrying somebody else's user code. The SSH session is one of the reasons the device grant exists, so it gets the printed URL and nothing else. - On Windows, `start` is a `cmd` builtin, so the URL goes through a shell that re-parses it, where `&` separates commands and `%NAME%` is expanded — after any check we run, inside quotes as well as out, with no escape available on a command line. The URL is quoted, and one holding a `"` or a `%` is refused rather than opened: the quote because it would close the quoting, the `%` because the value spliced in is not held to the rule the check just applied, and because a substituted URL would open a different page than the one printed. The host that writes that URL is whatever `--api-url` names, the same threat `device::checked_uri` already guards. - The opener's child is reaped on a thread. `login` then sits in the poll loop for minutes, and a child nobody waits on is a zombie for all of it. That thread is asked for fallibly, because `thread::spawn` panics when the OS refuses one and a panic is the one way this convenience could still take the login down. A zombie until the process exits is the cheaper of the two. The browser is a trait so the suite can watch the URL go past without opening a window on whoever ran `cargo test`. Refs MRGFY-9262 Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I90e2ca4c0ce90afa95b3700a3c0acd35c1b0a135 --- README.md | 4 +- crates/mergify-auth/src/browser.rs | 290 ++++++++++++++++++ crates/mergify-auth/src/lib.rs | 3 + crates/mergify-auth/src/login.rs | 217 ++++++++++++- crates/mergify-cli/src/main.rs | 53 +++- .../mergify__tests__cli_schema_golden.snap | 21 +- 6 files changed, 570 insertions(+), 18 deletions(-) create mode 100644 crates/mergify-auth/src/browser.rs diff --git a/README.md b/README.md index 29167e5e..bab1769e 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,9 @@ Sign in once: mergify auth login ``` -It prints a URL and a code, you approve them in your browser, and the +It opens the approval page in your browser, and prints the URL and the code +as well: pass `--no-browser`, or run it where there is no browser to open, +and the printed pair is all you need. Once you approve, the credential lands in your OS keychain — or, on a machine with none (a container, an unattended agent, a headless box with no D-Bus session), in a restricted file under your configuration directory. `mergify auth status` says diff --git a/crates/mergify-auth/src/browser.rs b/crates/mergify-auth/src/browser.rs new file mode 100644 index 00000000..827328d1 --- /dev/null +++ b/crates/mergify-auth/src/browser.rs @@ -0,0 +1,290 @@ +//! Putting the verification page in front of the user. +//! +//! A convenience, never a step in the grant. There is no browser on +//! a CI runner, in a container, or at the far end of an SSH session, +//! and `auth login` has to work on all three — so every failure here +//! is a debug line, and the URL is printed whether this succeeds or +//! not. +//! +//! No crate for this: each platform is one process spawn, and the +//! workspace's dependency policy is not worth spending on thirty +//! lines of [`Command`]. + +use std::io; +use std::process::Command; +use std::process::Stdio; + +/// How [`crate::login`] reaches a browser. +/// +/// A trait rather than a free function so the suite can watch the +/// URL go past without opening a window on whoever ran `cargo test`. +pub trait Browser { + /// Show `url`, or say why not. An `Err` is never fatal. + fn open(&self, url: &str) -> io::Result<()>; +} + +/// The user's own browser, through the platform's URL opener. +pub struct SystemBrowser; + +impl Browser for SystemBrowser { + fn open(&self, url: &str) -> io::Result<()> { + launch(command_for(url)?) + } +} + +#[cfg(target_os = "macos")] +fn command_for(url: &str) -> io::Result { + // macOS has no `DISPLAY` to consult, so the SSH variables are + // the only signal that the screen `open` would use is not the + // one the user is looking at. It belongs to whoever is sitting + // at the machine, who did not ask to approve anything and would + // be handed a page with somebody else's user code on it. + // + // The Linux arm deliberately asks a different question rather + // than this one: an SSH session there can have a forwarded + // display, which is the case where opening *is* right, and + // `DISPLAY` is what tells the two apart. macOS has nothing + // equivalent to forward. + if mergify_core::env::var_non_empty("SSH_CONNECTION").is_some() + || mergify_core::env::var_non_empty("SSH_TTY").is_some() + { + return Err(io::Error::other( + "an SSH session: the browser would open on the machine's own screen", + )); + } + let mut command = Command::new("open"); + command.arg(url); + Ok(command) +} + +#[cfg(all(unix, not(target_os = "macos")))] +fn command_for(url: &str) -> io::Result { + // With no graphical session `xdg-open` falls through to a + // terminal browser (`www-browser`, `w3m`, `lynx`), which takes + // over the very terminal the user is reading the code from. A + // headless box and an SSH session are two of the three cases + // this command exists to keep working, so they get the printed + // URL and nothing else. + // + // The display rather than the SSH variables, on purpose: an SSH + // session with X11 forwarding has a display, and it is the + // user's own — refusing it would withhold the browser from the + // one remote case that can show one. The converse, an rc file + // that exports `DISPLAY=:0` unconditionally, then opens on the + // remote machine's monitor; that configuration sends every + // GUI-launching command there and is not ours to second-guess. + if mergify_core::env::var_non_empty("DISPLAY").is_none() + && mergify_core::env::var_non_empty("WAYLAND_DISPLAY").is_none() + { + return Err(io::Error::other( + "no graphical session: neither DISPLAY nor WAYLAND_DISPLAY is set", + )); + } + let mut command = Command::new("xdg-open"); + command.arg(url); + Ok(command) +} + +#[cfg(windows)] +fn command_for(url: &str) -> io::Result { + use std::os::windows::process::CommandExt; + + let mut command = Command::new("cmd"); + // `raw_arg`, because the whole point is the quoting: the + // standard escaping would leave the URL unquoted and `cmd` + // would act on what is in it. + command.arg("/C").raw_arg(windows_start_argument(url)?); + Ok(command) +} + +/// The verbatim `cmd` command line that opens `url`. +/// +/// `start` is a `cmd` builtin, so there is no reaching it except +/// through a shell that re-parses its own command line — and an `&` +/// or a `|` in a URL is a command separator to that shell. +/// `--api-url` decides which host writes that URL, which makes this +/// the same threat `device::checked_uri` exists for. Quoting the URL +/// makes those characters literal, and that is only sound because +/// the URL cannot carry a quote of its own: it came through +/// `Url::parse`, which percent-encodes `"` in every component it can +/// appear in. Anything that proves otherwise is refused rather than +/// handed to `cmd`. +/// +/// A `%` is refused for the same reason, and it is the subtler one: +/// `cmd` expands `%NAME%` on its command line *after* this check has +/// run, inside the quotes as well as outside, and a command line has +/// no escape for it (`%%` only works in a batch file). So a URL +/// whose percent-escapes happen to bracket a variable name opens a +/// different page than the one printed — the drift `verification_url` +/// exists to prevent — and a variable whose value holds a `"` closes +/// the quote this function just proved could not be closed. Refusing +/// costs a browser that would have opened; the URL is printed either +/// way. +/// +/// The empty `""` is `start`'s window-title argument. Without it +/// `start` reads the quoted URL as the title and opens nothing. +/// +/// Compiled on every platform so the suite can pin it anywhere; only +/// Windows calls it. +#[cfg(any(windows, test))] +fn windows_start_argument(url: &str) -> io::Result { + if url.contains(['"', '%']) || url.chars().any(char::is_control) { + return Err(io::Error::other( + "the verification URL holds a character this client will not hand to cmd", + )); + } + Ok(format!("start \"\" \"{url}\"")) +} + +fn launch(mut command: Command) -> io::Result<()> { + // Whatever the opener has to say is not the user's problem: the + // browser opens or it does not, and the URL is printed either + // way. Its output would land in the middle of the code the user + // is trying to read. + let mut child = command + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn()?; + // The opener exits as soon as the browser has the URL, but + // `login` then sits in the poll loop for minutes. A child nobody + // waits on is a zombie for all of it. + // + // `Builder::spawn` rather than `thread::spawn`, which panics + // when the OS refuses a thread. This module promises the login + // proceeds whatever happens here, and a panic would take the + // login down with it — a zombie until the process exits is the + // cheaper of the two failures. + let reaper = std::thread::Builder::new().spawn(move || match child.wait() { + // What the opener made of the URL is its own business, but a + // non-zero exit is the only sign the page never opened, and + // `open` reports "no application knows this URL" that way + // and nowhere else — its stderr went to /dev/null above. + Ok(status) if !status.success() => { + tracing::debug!(%status, "the browser opener exited with an error"); + } + Ok(_) => {} + Err(e) => tracing::debug!(error = %e, "could not wait for the browser opener"), + }); + if let Err(e) = reaper { + tracing::debug!(error = %e, "could not reap the browser opener"); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + // A `&` in a query string is ordinary, and to `cmd` it is a + // command separator. It has to end up inside the quotes. + #[test] + fn the_windows_command_line_quotes_the_url() { + assert_eq!( + windows_start_argument("https://dashboard.mergify.com/device?user_code=BCDF-GHJK&x=1") + .unwrap(), + "start \"\" \"https://dashboard.mergify.com/device?user_code=BCDF-GHJK&x=1\"", + ); + } + + // The quoting above is the only thing between a hostile + // `--api-url` and `cmd`, so a URL that could close the quote is + // not opened at all. + #[test] + fn a_url_carrying_a_quote_is_not_handed_to_cmd() { + assert!(windows_start_argument("https://evil.example/\"&calc").is_err()); + assert!(windows_start_argument("https://evil.example/\r\nhi").is_err()); + } + + // `cmd` expands `%NAME%` after this check, and the value it + // splices in is not held to the rule the check just applied. + #[test] + fn a_url_carrying_a_percent_is_not_handed_to_cmd() { + assert!(windows_start_argument("https://evil.example/?a=%SOMEVAR%").is_err()); + } + + // The whole "never fail the login" promise rests on this + // returning rather than panicking when the opener is not there + // — the CI runner and the container both reach it that way. + #[test] + fn an_opener_that_is_not_installed_is_an_error_not_a_panic() { + assert!(launch(Command::new("mergify-no-such-url-opener")).is_err()); + } + + #[cfg(unix)] + #[test] + fn launching_an_opener_that_exists_succeeds() { + launch(Command::new("true")).unwrap(); + } + + // An opener that starts and then refuses the URL is the one + // failure the caller cannot see: `launch` has already returned + // `Ok` by the time the child exits, and the reaper only has a + // `debug!` to say so with. What this pins is that the reaper + // handles that status instead of unwinding on it. + #[cfg(unix)] + #[test] + fn an_opener_that_exits_non_zero_still_lets_the_login_proceed() { + launch(Command::new("false")).unwrap(); + } + + #[cfg(target_os = "macos")] + #[test] + fn macos_opens_the_url_with_open() { + let command = temp_env::with_vars( + [("SSH_CONNECTION", None::<&str>), ("SSH_TTY", None::<&str>)], + || command_for("https://dashboard.mergify.com/device"), + ) + .unwrap(); + assert_eq!(command.get_program(), "open"); + assert_eq!( + command.get_args().collect::>(), + ["https://dashboard.mergify.com/device"], + ); + } + + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn a_graphical_session_gets_xdg_open() { + let command = temp_env::with_vars( + [("DISPLAY", Some(":0")), ("WAYLAND_DISPLAY", None::<&str>)], + || command_for("https://dashboard.mergify.com/device"), + ) + .unwrap(); + assert_eq!(command.get_program(), "xdg-open"); + assert_eq!( + command.get_args().collect::>(), + ["https://dashboard.mergify.com/device"], + ); + } + + // Over SSH to a Mac, `open` reaches the screen of whoever is + // sitting at that machine, not the person who ran the command. + #[cfg(target_os = "macos")] + #[test] + fn an_ssh_session_to_a_mac_opens_nothing() { + let opened = temp_env::with_vars( + [ + ("SSH_CONNECTION", Some("10.0.0.1 52000 10.0.0.2 22")), + ("SSH_TTY", None), + ], + || command_for("https://dashboard.mergify.com/device").is_ok(), + ); + assert!( + !opened, + "an SSH session must not open a browser on the host" + ); + } + + // The SSH session the device grant exists for: `xdg-open` here + // would hand the URL to a terminal browser and eat the terminal. + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn a_headless_session_opens_nothing() { + let opened = temp_env::with_vars( + [("DISPLAY", None::<&str>), ("WAYLAND_DISPLAY", None::<&str>)], + || command_for("https://dashboard.mergify.com/device").is_ok(), + ); + assert!(!opened, "a session with no display must not run xdg-open"); + } +} diff --git a/crates/mergify-auth/src/lib.rs b/crates/mergify-auth/src/lib.rs index 46aa8122..ec13d973 100644 --- a/crates/mergify-auth/src/lib.rs +++ b/crates/mergify-auth/src/lib.rs @@ -1,5 +1,7 @@ //! `mergify auth` — the Mergify-issued, per-user credential. //! +//! - [`browser`] — putting the approval page in front of the user, +//! which is a convenience the login never depends on. //! - [`device`] — the OAuth 2.0 device authorization grant //! (RFC 8628) against the Mergify API, which is how the CLI gets //! a credential without ever handling a password or a GitHub @@ -13,6 +15,7 @@ //! [`mergify_core::CredentialStore`]; this crate obtains it, //! revokes it, and reports on it. +pub mod browser; pub mod device; pub mod identity; pub mod login; diff --git a/crates/mergify-auth/src/login.rs b/crates/mergify-auth/src/login.rs index 1a871294..36aa7ab6 100644 --- a/crates/mergify-auth/src/login.rs +++ b/crates/mergify-auth/src/login.rs @@ -13,12 +13,18 @@ use mergify_core::credentials::Location; use serde::Serialize; use url::Url; +use crate::browser::Browser; use crate::device; use crate::identity; pub struct LoginOptions<'a> { pub api_url: Option<&'a str>, pub store: &'a CredentialStore, + /// Where to open the approval page, or `None` for + /// `--no-browser`. Opening it is a convenience: the URL is + /// printed either way, and a browser that will not open does + /// not fail the login. + pub browser: Option<&'a dyn Browser>, } /// What `auth login` produced, for the JSON rendering `Output` @@ -49,7 +55,8 @@ pub async fn run(opts: LoginOptions<'_>, output: &mut dyn Output) -> Result<(), let previous = opts.store.get(&api_url)?; let authorization = device::authorize(&client).await?; - output.status(&instructions(&authorization))?; + let opened = open_verification_page(opts.browser, verification_url(&authorization)); + output.status(&instructions(&authorization, opened))?; let token = device::poll( &client, @@ -140,15 +147,51 @@ async fn revoke_quietly(client: &mergify_core::HttpClient, token: &str) { } } -/// What the user has to do, on stderr, while the poll loop waits. -fn instructions(authorization: &device::Authorization) -> String { - let theme = mergify_tui::Theme::detect(); - let url = authorization +/// The page the user approves on: the one with the code already +/// filled in when the server offers it (RFC 8628 §3.3.1), the plain +/// one otherwise. Both the browser and the printed instructions go +/// through here, so what opens and what is on screen cannot drift +/// apart. +fn verification_url(authorization: &device::Authorization) -> &str { + authorization .verification_uri_complete .as_deref() - .unwrap_or(&authorization.verification_uri); + .unwrap_or(&authorization.verification_uri) +} + +/// Try to put the approval page in front of the user, and report +/// whether it worked so the instructions can say something true. +/// +/// Returns rather than fails, always. A CI runner, a container and +/// an SSH session have no browser to open, and none of that is a +/// reason to refuse a login whose whole design is that the approval +/// happens somewhere else. +fn open_verification_page(browser: Option<&dyn Browser>, url: &str) -> bool { + let Some(browser) = browser else { return false }; + match browser.open(url) { + Ok(()) => true, + Err(e) => { + tracing::debug!(error = %e, "could not open the verification page"); + false + } + } +} + +/// What the user has to do, on stderr, while the poll loop waits. +/// +/// The URL is printed whether or not a browser took it: a spawned +/// opener is not a window on screen, and the terminal is the only +/// place the user can be sure to find the address again. +fn instructions(authorization: &device::Authorization, opened: bool) -> String { + let theme = mergify_tui::Theme::detect(); + let url = verification_url(authorization); + let open = if opened { + "Opening your browser to authorize the Mergify CLI. If nothing opens, go to:" + } else { + "Open this URL to authorize the Mergify CLI:" + }; format!( - "Open this URL to authorize the Mergify CLI:\n\n {url}\n\n\ + "{open}\n\n {url}\n\n\ and confirm this code:\n\n {bold}{code}{reset}\n\n\ Waiting for approval…", bold = theme.bold.render(), @@ -234,6 +277,44 @@ mod tests { } } + /// A browser that records instead of opening one, so the suite + /// never puts a window on the screen of whoever ran it. + struct RecordingBrowser { + opened: std::sync::Mutex>, + works: bool, + } + + impl RecordingBrowser { + fn working() -> Self { + Self { + opened: std::sync::Mutex::new(Vec::new()), + works: true, + } + } + + fn broken() -> Self { + Self { + opened: std::sync::Mutex::new(Vec::new()), + works: false, + } + } + + fn opened(&self) -> Vec { + self.opened.lock().unwrap().clone() + } + } + + impl crate::browser::Browser for RecordingBrowser { + fn open(&self, url: &str) -> std::io::Result<()> { + self.opened.lock().unwrap().push(url.to_string()); + if self.works { + Ok(()) + } else { + Err(std::io::Error::other("no browser here")) + } + } + } + async fn mount_flow(server: &MockServer, with_identity: bool) { Mock::given(method("POST")) .and(path("/v1/oauth/device/code")) @@ -287,6 +368,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -326,6 +408,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -357,6 +440,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -387,6 +471,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -435,6 +520,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -469,6 +555,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -515,6 +602,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -564,6 +652,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -611,6 +700,7 @@ mod tests { LoginOptions { api_url: Some(&server.uri()), store: &store, + browser: None, }, &mut captured.output, ) @@ -630,7 +720,7 @@ mod tests { fn the_instructions_fall_back_to_the_plain_verification_url() { let mut authorization = authorization(); authorization.verification_uri_complete = None; - let rendered = instructions(&authorization); + let rendered = instructions(&authorization, false); assert!( rendered.contains("https://dashboard.mergify.com/device\n"), "got {rendered:?}", @@ -638,6 +728,117 @@ mod tests { assert!(rendered.contains("BCDF-GHJK"), "got {rendered:?}"); } + // The same page the user is told to open is the one that opens: + // the pre-filled one, so the code they are comparing is already + // in the field. + #[test] + fn login_opens_the_verification_page() { + with_mergify_token(None, async { + let server = MockServer::start().await; + mount_flow(&server, true).await; + let (dir, store) = file_store(); + let browser = RecordingBrowser::working(); + let mut captured = Captured::human(); + + run( + LoginOptions { + api_url: Some(&server.uri()), + store: &store, + browser: Some(&browser), + }, + &mut captured.output, + ) + .await + .unwrap(); + + assert_eq!( + browser.opened(), + ["https://dashboard.mergify.com/device?user_code=BCDF-GHJK"], + ); + let stderr = captured.stderr(); + assert!(stderr.contains("Opening your browser"), "got {stderr:?}"); + // Still printed, always: a spawned opener is not a + // window on screen, and this is where the user looks. + assert!( + stderr.contains("https://dashboard.mergify.com/device?user_code=BCDF-GHJK"), + "got {stderr:?}", + ); + drop(dir); + }); + } + + // Opening is a convenience, and the machines that cannot do it + // — a CI runner, a container, an SSH session — are the ones the + // device grant exists for. + #[test] + fn a_browser_that_will_not_open_does_not_fail_the_login() { + with_mergify_token(None, async { + let server = MockServer::start().await; + mount_flow(&server, true).await; + let (dir, store) = file_store(); + let browser = RecordingBrowser::broken(); + let mut captured = Captured::human(); + + run( + LoginOptions { + api_url: Some(&server.uri()), + store: &store, + browser: Some(&browser), + }, + &mut captured.output, + ) + .await + .unwrap(); + + let api_url = Url::parse(&server.uri()).unwrap(); + assert_eq!( + store.get(&api_url).unwrap().unwrap().credential.token, + "mut_secret", + ); + let stderr = captured.stderr(); + assert!( + stderr.contains("Open this URL to authorize"), + "a browser that did not open must not be claimed to have opened, got {stderr:?}", + ); + assert!( + stderr.contains("https://dashboard.mergify.com/device?user_code=BCDF-GHJK"), + "got {stderr:?}", + ); + drop(dir); + }); + } + + // `--no-browser`: nothing is spawned, and the instructions go + // back to telling the user to open the URL themselves. + #[test] + fn no_browser_tells_the_user_to_open_the_url() { + 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 stderr = captured.stderr(); + assert!( + stderr.contains("Open this URL to authorize"), + "got {stderr:?}", + ); + assert!(!stderr.contains("Opening your browser"), "got {stderr:?}"); + drop(dir); + }); + } + #[test] fn expires_at_turns_the_servers_lifetime_into_a_moment() { let now = DateTime::parse_from_rfc3339("2026-09-04T00:00:00Z") diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index ed9486f8..d073381a 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -162,7 +162,10 @@ const NATIVE_COMMANDS: &[(&str, &str)] = &[ enum NativeCommand { /// `mergify auth login [--api-url URL]` — run the device grant /// and store the credential it mints. - AuthLogin(AuthOpts), + AuthLogin { + opts: AuthOpts, + no_browser: bool, + }, /// `mergify auth logout [--api-url URL]` — revoke the stored /// credential and forget it. AuthLogout(AuthOpts), @@ -924,7 +927,9 @@ fn dispatch_from_parsed(parsed: CliRoot) -> Dispatch { Subcommands::Auth(AuthArgs { api_url, command }) => { let opts = AuthOpts { api_url }; Dispatch::Native(match command { - AuthSubcommand::Login => NativeCommand::AuthLogin(opts), + AuthSubcommand::Login(AuthLoginArgs { no_browser }) => { + NativeCommand::AuthLogin { opts, no_browser } + } AuthSubcommand::Logout => NativeCommand::AuthLogout(opts), AuthSubcommand::Status => NativeCommand::AuthStatus(opts), }) @@ -1553,12 +1558,16 @@ fn run_native(cmd: NativeCommand) -> ExitCode { | NativeCommand::InternalManPage => { unreachable!("introspection commands are handled before the runtime starts") } - NativeCommand::AuthLogin(opts) => { + NativeCommand::AuthLogin { opts, no_browser } => { let store = mergify_core::CredentialStore::discover(); + let system_browser = mergify_auth::browser::SystemBrowser; + let browser: Option<&dyn mergify_auth::browser::Browser> = + if no_browser { None } else { Some(&system_browser) }; mergify_auth::login::run( mergify_auth::login::LoginOptions { api_url: opts.api_url.as_deref(), store: &store, + browser, }, &mut output, ) @@ -4466,14 +4475,23 @@ struct AuthArgs { command: AuthSubcommand, } +#[derive(clap::Args)] +struct AuthLoginArgs { + /// Do not open a browser; only print the URL to open. + #[arg(long = "no-browser")] + no_browser: bool, +} + #[derive(Subcommand)] enum AuthSubcommand { /// Sign in to Mergify and store the credential. /// - /// Prints a URL and a code: open the one, enter the other, and - /// approve. The credential lands in your OS keychain, or in a - /// `0600` file when the machine has no keychain to offer. - Login, + /// Opens the approval page in your browser and prints the URL + /// and the code as well, so a machine with no browser can sign + /// in from the same command. The credential lands in your OS + /// keychain, or in a `0600` file when the machine has no + /// keychain to offer. + Login(AuthLoginArgs), /// Revoke the stored credential and forget it. /// /// Tells the Mergify API to revoke the token as well as deleting @@ -4903,6 +4921,27 @@ mod tests { assert_eq!(opts.files, vec!["report.xml"]); } + // The flag has to reach `LoginOptions`, not merely parse: an + // inverted `if no_browser` in `run_native` opens a browser for + // the user who asked for none, and every other test in the + // suite passes `browser: None` directly and would stay green. + #[test] + fn auth_login_carries_no_browser_through_dispatch() { + let Dispatch::Native(NativeCommand::AuthLogin { no_browser, .. }) = + dispatch_from_parsed(parse(&["auth", "login", "--no-browser"])) + else { + panic!("auth login must dispatch to the native AuthLogin variant"); + }; + assert!(no_browser); + + let Dispatch::Native(NativeCommand::AuthLogin { no_browser, .. }) = + dispatch_from_parsed(parse(&["auth", "login"])) + else { + panic!("auth login must dispatch to the native AuthLogin variant"); + }; + assert!(!no_browser, "a browser is the default"); + } + #[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 5a1a64f9..9a69b860 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 @@ -104,9 +104,26 @@ expression: schema { "about": "Sign in to Mergify and store the credential", "aliases": [], - "args": [], + "args": [ + { + "default": "false", + "env": null, + "global": false, + "help": "Do not open a browser; only print the URL to open", + "id": "no_browser", + "kind": "flag", + "long": "no-browser", + "longHelp": "Do not open a browser; only print the URL to open", + "numArgs": "0", + "possibleValues": [], + "required": false, + "short": null, + "valueHint": null, + "valueNames": [] + } + ], "commands": [], - "longAbout": "Sign in to Mergify and store the credential.\n\nPrints a URL and a code: open the one, enter the other, and approve. The credential lands in your OS keychain, or in a `0600` file when the machine has no keychain to offer.", + "longAbout": "Sign in to Mergify and store the credential.\n\nOpens the approval page in your browser and prints the URL and the code as well, so a machine with no browser can sign in from the same command. The credential lands in your OS keychain, or in a `0600` file when the machine has no keychain to offer.", "name": "login", "path": [ "mergify",