diff --git a/nori-rs/acp/src/broker/docs.md b/nori-rs/acp/src/broker/docs.md index 285086ddf..e2f953446 100644 --- a/nori-rs/acp/src/broker/docs.md +++ b/nori-rs/acp/src/broker/docs.md @@ -47,7 +47,7 @@ BrokerClient ----HTTP POST /api/sessions/{id}/release---->| - `CloudConnectionInfo` is a plain struct with `ws_url: String` and `auth_token: String`. It is the branch condition throughout the system -- `Option` being `Some` means cloud mode, `None` means local subprocess mode - `BrokerClient::new()` loads persisted credentials from `{nori_home}/cloud-auth.json` and filters them to match the current `broker_url`. If the stored credentials are for a different broker, they are ignored - `BrokerClient::has_valid_token()` checks whether the stored JWT is present and not expired, using `is_token_expired()` which decodes the base64url JWT payload and compares the `exp` claim against the current system time -- `BrokerClient::authenticate()` runs an OAuth browser flow: it binds a local HTTP server on an ephemeral port, opens the broker's `/api/auth/cli?redirect_uri=...` URL in the default browser, waits up to 2 minutes for a callback with a `?token=` query parameter, and persists the credentials via `save_credentials()`. The 2-minute timeout prevents the CLI from hanging indefinitely if the user abandons the browser flow; on timeout, `server.unblock()` is called to shut down the callback listener thread cleanly +- `BrokerClient::authenticate()` runs an OAuth browser flow: it binds a local HTTP server on an ephemeral port, prints the broker's `/api/auth/cli?redirect_uri=...` URL to stderr, opens that URL in the default browser, waits up to 2 minutes for a callback with a `?token=` query parameter, and persists the credentials via `save_credentials()`. Printing the URL before opening it keeps `nori cloud` usable from SSH or other headless shells. The 2-minute timeout prevents the CLI from hanging indefinitely if the user abandons the browser flow; on timeout, `server.unblock()` is called to shut down the callback listener thread cleanly - `BrokerClient::acquire_session()` POSTs to `{broker_url}/api/sessions/acquire` with a Bearer token and a JSON body `{"source": "cli"}`, returning a `SessionInfo` containing `session_id` and `ws_url`. The `source` field identifies this as a CLI client so the broker uses the correct claim identity format. HTTP 401 responses map to `BrokerError::TokenExpired` - `BrokerClient::release_session()` POSTs to `{broker_url}/api/sessions/{session_id}/release` with a Bearer token to explicitly release a cloud session. Called by the CLI as a best-effort cleanup after the TUI exits (wrapped in a 5-second timeout). HTTP 401 maps to `TokenExpired`; non-success responses map to `BrokerError::ReleaseFailed` - `CloudCredentials` is the serialized form persisted to `cloud-auth.json`; it pairs `broker_url` with `auth_token` so credentials for different brokers do not collide diff --git a/nori-rs/acp/src/broker/mod.rs b/nori-rs/acp/src/broker/mod.rs index e5cf0d696..562fc381d 100644 --- a/nori-rs/acp/src/broker/mod.rs +++ b/nori-rs/acp/src/broker/mod.rs @@ -1,6 +1,7 @@ use anyhow::Result; use serde::Deserialize; use serde::Serialize; +use std::io::Write; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -163,6 +164,20 @@ impl BrokerClient { } pub async fn authenticate(&mut self) -> Result<()> { + let mut stderr = std::io::stderr(); + self.authenticate_with(&mut stderr, |auth_url| webbrowser::open(auth_url).is_ok()) + .await + } + + async fn authenticate_with( + &mut self, + output: &mut W, + open_browser: OpenBrowser, + ) -> Result<()> + where + W: Write, + OpenBrowser: FnOnce(&str) -> bool, + { let server = Arc::new( tiny_http::Server::http("127.0.0.1:0") .map_err(|e| anyhow::anyhow!("failed to bind local server: {e}"))?, @@ -175,8 +190,12 @@ impl BrokerClient { let auth_url = build_cli_auth_url(&self.broker_url, port); - if webbrowser::open(&auth_url).is_err() { - eprintln!("Could not open browser. Please visit:\n{auth_url}"); + writeln!(output, "Opening browser for authentication:\n{auth_url}")?; + if !open_browser(&auth_url) { + writeln!( + output, + "Could not open browser automatically. Please visit the URL above." + )?; } let (tx, rx) = tokio::sync::oneshot::channel::(); diff --git a/nori-rs/acp/src/broker/tests.rs b/nori-rs/acp/src/broker/tests.rs index 41876499f..662838591 100644 --- a/nori-rs/acp/src/broker/tests.rs +++ b/nori-rs/acp/src/broker/tests.rs @@ -152,6 +152,71 @@ fn builds_cli_auth_url_under_api_prefix() { ); } +#[tokio::test] +async fn authenticate_prints_url_during_browser_login() { + let dir = tempdir().unwrap(); + let broker_url = "https://broker.test".to_string(); + let mut client = BrokerClient::new(broker_url.clone(), dir.path().to_path_buf()); + let token = future_jwt(); + let callback_token = token.clone(); + let mut output = Vec::new(); + + tokio::time::timeout( + std::time::Duration::from_secs(5), + client.authenticate_with(&mut output, move |auth_url| { + let auth_url = auth_url.to_string(); + let callback_token = callback_token; + std::thread::spawn(move || complete_auth_callback(&auth_url, &callback_token)); + true + }), + ) + .await + .unwrap() + .unwrap(); + + let output = String::from_utf8(output).unwrap(); + let expected_url_prefix = "https://broker.test/api/auth/cli?redirect_uri=http://localhost:"; + assert!( + output.contains(expected_url_prefix), + "expected auth output to include URL prefix {expected_url_prefix}, got {output:?}" + ); + + assert_eq!( + load_credentials(dir.path()), + Some(CloudCredentials { + broker_url, + auth_token: token, + }) + ); +} + +fn complete_auth_callback(auth_url: &str, token: &str) { + use std::io::Read as _; + use std::io::Write as _; + + let auth_url = url::Url::parse(auth_url).unwrap(); + let redirect_uri = auth_url + .query_pairs() + .find(|(key, _)| key == "redirect_uri") + .map(|(_, value)| value.into_owned()) + .unwrap(); + let redirect_uri = url::Url::parse(&redirect_uri).unwrap(); + let port = redirect_uri.port().unwrap(); + let request_path = format!("{}?token={token}", redirect_uri.path()); + let mut stream = std::net::TcpStream::connect(("127.0.0.1", port)).unwrap(); + write!( + stream, + "GET {request_path} HTTP/1.1\r\nHost: localhost:{port}\r\nConnection: close\r\n\r\n" + ) + .unwrap(); + let mut response = String::new(); + stream.read_to_string(&mut response).unwrap(); + assert!( + response.contains("Authentication successful"), + "expected success response, got {response:?}" + ); +} + // ── BrokerClient construction ────────────────────────────────────── #[test] diff --git a/nori-rs/cli/docs.md b/nori-rs/cli/docs.md index 0e95f3931..42275e0cd 100644 --- a/nori-rs/cli/docs.md +++ b/nori-rs/cli/docs.md @@ -51,7 +51,7 @@ match subcommand { - `nori cloud --broker-url https://broker.example.com` - Overrides the broker URL - TUI flags such as `--agent`, `--profile`, `--sandbox` can be passed after `cloud` - Broker URL resolution follows a three-step priority chain: `--broker-url` flag > `[cloud] broker_url` in `config.toml` > interactive stdin prompt. The interactive prompt only activates when stdin is a terminal (`std::io::IsTerminal`); non-interactive invocations without a configured URL receive an error with setup instructions. The prompt validates that the URL starts with `http://` or `https://` and persists the value to `config.toml` via `save_cloud_broker_url()` from `@/nori-rs/acp/src/config/loader.rs`, so subsequent runs use the saved URL automatically -- The dispatch flow: resolves broker URL (see above), creates `BrokerClient` (see `@/nori-rs/acp/src/broker/docs.md`), authenticates via browser OAuth if needed, acquires a session (with automatic re-authentication retry on `BrokerError::TokenExpired`), then sets `TuiCli.cloud_connection` and calls `nori_tui::run_main()`. The retry handles the edge case where `has_valid_token()` passes at startup but the broker rejects the token with HTTP 401 during `acquire_session()` -- the CLI re-triggers `broker.authenticate()` and retries once +- The dispatch flow: resolves broker URL (see above), creates `BrokerClient` (see `@/nori-rs/acp/src/broker/docs.md`), authenticates via browser OAuth if needed, acquires a session (with automatic re-authentication retry on `BrokerError::TokenExpired`), then sets `TuiCli.cloud_connection` and calls `nori_tui::run_main()`. Browser OAuth prints the exact URL being opened so headless SSH sessions can copy it into another browser. The retry handles the edge case where `has_valid_token()` passes at startup but the broker rejects the token with HTTP 401 during `acquire_session()` -- the CLI re-triggers `broker.authenticate()` and retries once - After `run_main()` returns, the CLI calls `broker.release_session()` with a 5-second timeout as best-effort cleanup. Release failures or timeouts are logged but do not affect the exit code - The `CloudConnectionInfo` flows through the TUI unchanged until it reaches `AcpBackend::spawn()`, which uses `SacpConnection::connect_remote()` instead of spawning a local subprocess diff --git a/nori-rs/cli/src/main.rs b/nori-rs/cli/src/main.rs index 6e5e802e6..06dccd269 100644 --- a/nori-rs/cli/src/main.rs +++ b/nori-rs/cli/src/main.rs @@ -553,7 +553,6 @@ async fn cli_main(codex_linux_sandbox_exe: Option) -> anyhow::Result<() let mut broker = nori_acp::broker::BrokerClient::new(broker_url, nori_home); if !broker.has_valid_token() { - eprintln!("Opening browser for authentication..."); broker.authenticate().await?; eprintln!("Authenticated."); }