Add browser-based device authorization#16
Open
IliaMManolov wants to merge 1 commit into
Open
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Collaborator
Author
|
Companion backend/dashboard PR: https://github.com/Edison-Watch/edison-watch/pull/1077 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/edison-stdiod/src/config_extra_tests.rs">
<violation number="1" location="crates/edison-stdiod/src/config_extra_tests.rs:49">
P2: Replacing an invalid saved backend must not leave its stale scoped token usable, but this regression test never checks `client_access_token`. As written, credential-isolation regressions can pass unnoticed; asserting that `merged.client_access_token` is `None` would cover the seeded case.</violation>
</file>
<file name="crates/edison-stdiod/src/secure_file.rs">
<violation number="1" location="crates/edison-stdiod/src/secure_file.rs:86">
P2: On Windows, saving credentials can permanently remove the current file if the process stops or the follow-up `rename` fails after `remove_file` succeeds; readers can also observe the file missing during normal replacement. An atomic Windows replacement API, or a failure-safe replacement/rollback strategy, would preserve the existing file until the new one is installed.</violation>
</file>
<file name="crates/edison-stdiod/src/tunnel.rs">
<violation number="1" location="crates/edison-stdiod/src/tunnel.rs:237">
P2: A close carrying a reason returns before `send_task.abort()` runs, so the old sink task can outlive the session and retain its receiver/socket while the supervisor handles the close error. Cleaning up the task before returning the classified error keeps revoked or otherwise rejected sessions from leaking connection resources.</violation>
</file>
<file name="crates/edison-stdiod/src/cli/login.rs">
<violation number="1" location="crates/edison-stdiod/src/cli/login.rs:78">
P1: Reauthorizing a reused installation for a different user or organization is not recognized as an account change because this condition omits `authenticated_user_id` and `authenticated_org_id`. Comparing both returned account IDs here would clear the previous per-user secret and revoke the old client credential instead of retaining it.</violation>
</file>
<file name="crates/edison-stdiod/src/auth.rs">
<violation number="1" location="crates/edison-stdiod/src/auth.rs:144">
P2: The token exchange can leak the PKCE verifier to a redirect target because this client follows redirects by default and a 307/308 response can resend the POST body. Disabling redirects for this auth client (or rejecting every cross-origin redirect) keeps authorization material bound to the configured backend.</violation>
<violation number="2" location="crates/edison-stdiod/src/auth.rs:334">
P3: A valid bearer token response using another casing (for example `bearer`) is rejected as a protocol error. Compare the token type case-insensitively before accepting the response.</violation>
</file>
<file name="crates/edison-stdiod/src/config.rs">
<violation number="1" location="crates/edison-stdiod/src/config.rs:247">
P2: A saved backend URL without any saved credential now blocks a different `--backend` override, so first-run/partial configs and logged-out users cannot switch servers without editing the config. Restrict this mismatch error to configs that actually contain issuer-bound authentication state (or otherwise allow the explicit backend override).</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Comment on lines
+78
to
+79
| let account_changed = !same_issuer | ||
| || cfg.client_installation_id.as_deref() != Some(token.client_installation_id.as_str()); |
There was a problem hiding this comment.
P1: Reauthorizing a reused installation for a different user or organization is not recognized as an account change because this condition omits authenticated_user_id and authenticated_org_id. Comparing both returned account IDs here would clear the previous per-user secret and revoke the old client credential instead of retaining it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/cli/login.rs, line 78:
<comment>Reauthorizing a reused installation for a different user or organization is not recognized as an account change because this condition omits `authenticated_user_id` and `authenticated_org_id`. Comparing both returned account IDs here would clear the previous per-user secret and revoke the old client credential instead of retaining it.</comment>
<file context>
@@ -1,122 +1,328 @@
+ println!("Waiting for authorization...");
+
+ let token = auth.poll(&code, pkce.verifier()).await?;
+ let account_changed = !same_issuer
+ || cfg.client_installation_id.as_deref() != Some(token.client_installation_id.as_str());
+
</file context>
Suggested change
| let account_changed = !same_issuer | |
| || cfg.client_installation_id.as_deref() != Some(token.client_installation_id.as_str()); | |
| let account_changed = !same_issuer | |
| || cfg.client_installation_id.as_deref() != Some(token.client_installation_id.as_str()) | |
| || cfg.authenticated_user_id.as_deref() != Some(token.user_id.as_str()) | |
| || cfg.authenticated_org_id.as_deref() != Some(token.org_id.as_str()); |
| }, | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!( |
There was a problem hiding this comment.
P2: Replacing an invalid saved backend must not leave its stale scoped token usable, but this regression test never checks client_access_token. As written, credential-isolation regressions can pass unnoticed; asserting that merged.client_access_token is None would cover the seeded case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/config_extra_tests.rs, line 49:
<comment>Replacing an invalid saved backend must not leave its stale scoped token usable, but this regression test never checks `client_access_token`. As written, credential-isolation regressions can pass unnoticed; asserting that `merged.client_access_token` is `None` would cover the seeded case.</comment>
<file context>
@@ -0,0 +1,53 @@
+ },
+ )
+ .unwrap();
+ assert_eq!(
+ merged.backend_url.as_deref(),
+ Some("https://replacement.test")
</file context>
| // std has no Windows equivalent of POSIX rename-over-existing. | ||
| // This remove/rename fallback makes repeated saves work, but there | ||
| // is a brief non-atomic window where the destination is absent. | ||
| match std::fs::remove_file(path) { |
There was a problem hiding this comment.
P2: On Windows, saving credentials can permanently remove the current file if the process stops or the follow-up rename fails after remove_file succeeds; readers can also observe the file missing during normal replacement. An atomic Windows replacement API, or a failure-safe replacement/rollback strategy, would preserve the existing file until the new one is installed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/secure_file.rs, line 86:
<comment>On Windows, saving credentials can permanently remove the current file if the process stops or the follow-up `rename` fails after `remove_file` succeeds; readers can also observe the file missing during normal replacement. An atomic Windows replacement API, or a failure-safe replacement/rollback strategy, would preserve the existing file until the new one is installed.</comment>
<file context>
@@ -0,0 +1,147 @@
+ // std has no Windows equivalent of POSIX rename-over-existing.
+ // This remove/rename fallback makes repeated saves work, but there
+ // is a brief non-atomic window where the destination is absent.
+ match std::fs::remove_file(path) {
+ Ok(()) => {}
+ Err(error) if error.kind() == io::ErrorKind::NotFound => {}
</file context>
| if let Some(frame) = frame { | ||
| let reason = frame.reason.to_string(); | ||
| if !reason.is_empty() { | ||
| return Err(session_close_error(frame.code, reason).into()); |
There was a problem hiding this comment.
P2: A close carrying a reason returns before send_task.abort() runs, so the old sink task can outlive the session and retain its receiver/socket while the supervisor handles the close error. Cleaning up the task before returning the classified error keeps revoked or otherwise rejected sessions from leaking connection resources.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/tunnel.rs, line 237:
<comment>A close carrying a reason returns before `send_task.abort()` runs, so the old sink task can outlive the session and retain its receiver/socket while the supervisor handles the close error. Cleaning up the task before returning the classified error keeps revoked or otherwise rejected sessions from leaking connection resources.</comment>
<file context>
@@ -187,8 +230,16 @@ pub async fn run_frame_loop(
+ if let Some(frame) = frame {
+ let reason = frame.reason.to_string();
+ if !reason.is_empty() {
+ return Err(session_close_error(frame.code, reason).into());
+ }
+ info!(code = %frame.code, "backend closed WS");
</file context>
Suggested change
| return Err(session_close_error(frame.code, reason).into()); | |
| let error = session_close_error(frame.code, reason); | |
| send_task.abort(); | |
| let _ = send_task.await; | |
| return Err(error.into()); |
| pub fn new(base: impl Into<String>) -> Result<Self, AuthError> { | ||
| let base = config::normalize_backend_url(&base.into()) | ||
| .map_err(|_| AuthError::Protocol("backend URL was invalid".into()))?; | ||
| let http = Client::builder() |
There was a problem hiding this comment.
P2: The token exchange can leak the PKCE verifier to a redirect target because this client follows redirects by default and a 307/308 response can resend the POST body. Disabling redirects for this auth client (or rejecting every cross-origin redirect) keeps authorization material bound to the configured backend.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/auth.rs, line 144:
<comment>The token exchange can leak the PKCE verifier to a redirect target because this client follows redirects by default and a 307/308 response can resend the POST body. Disabling redirects for this auth client (or rejecting every cross-origin redirect) keeps authorization material bound to the configured backend.</comment>
<file context>
@@ -0,0 +1,693 @@
+ pub fn new(base: impl Into<String>) -> Result<Self, AuthError> {
+ let base = config::normalize_backend_url(&base.into())
+ .map_err(|_| AuthError::Protocol("backend URL was invalid".into()))?;
+ let http = Client::builder()
+ .timeout(Duration::from_secs(20))
+ .build()
</file context>
|
|
||
| if !has_legacy_override { | ||
| if let (Some(requested), Some(saved)) = (&override_backend, &persisted_backend) { | ||
| if requested != saved { |
There was a problem hiding this comment.
P2: A saved backend URL without any saved credential now blocks a different --backend override, so first-run/partial configs and logged-out users cannot switch servers without editing the config. Restrict this mismatch error to configs that actually contain issuer-bound authentication state (or otherwise allow the explicit backend override).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/config.rs, line 247:
<comment>A saved backend URL without any saved credential now blocks a different `--backend` override, so first-run/partial configs and logged-out users cannot switch servers without editing the config. Restrict this mismatch error to configs that actually contain issuer-bound authentication state (or otherwise allow the explicit backend override).</comment>
<file context>
@@ -101,14 +213,99 @@ pub struct Resolved {
+
+ if !has_legacy_override {
+ if let (Some(requested), Some(saved)) = (&override_backend, &persisted_backend) {
+ if requested != saved {
+ return Err(anyhow!(
+ "--backend does not match the backend bound to the saved credential; pass an explicit legacy --api-key or run `edison-stdiod login`"
</file context>
| } | ||
|
|
||
| fn validate_token(token: &DeviceTokenResponse) -> Result<(), AuthError> { | ||
| if token.token_type != "Bearer" { |
There was a problem hiding this comment.
P3: A valid bearer token response using another casing (for example bearer) is rejected as a protocol error. Compare the token type case-insensitively before accepting the response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/auth.rs, line 334:
<comment>A valid bearer token response using another casing (for example `bearer`) is rejected as a protocol error. Compare the token type case-insensitively before accepting the response.</comment>
<file context>
@@ -0,0 +1,693 @@
+}
+
+fn validate_token(token: &DeviceTokenResponse) -> Result<(), AuthError> {
+ if token.token_type != "Bearer" {
+ return Err(AuthError::Protocol("token_type was not Bearer".into()));
+ }
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
needs_reauthandneeds_upgradestatesValidation
cargo test --workspace(95 tests)cargo clippy --workspace --all-targets -- -D warningscargo package -p edison-stdiod --allow-dirtyprekhooksSummary by cubic
Adds OAuth-style device authorization with PKCE for
edison-stdiod, enabling browser login, logout, and scoped client credentials. The daemon now reloads rotated credentials at runtime and handles auth states without restarts.New Features
loginopens the browser;logoutrevokes and clears local credentials./api/v1/client/...surface; legacy API-key flows remain compatible for add/list/remove where valid.needs_reauth/needs_upgradestates, and clean child process tree termination on auth changes or shutdown.Migration
edison-stdiod login --backend <URL>to issue and persist a client token; uselogoutto revoke/remove.installrequires a persisted credential (config.toml); set the backend and authenticate before installing the service.Written for commit 888e2f0. Summary will update on new commits.