feat: add OAuth 2.0 authentication via bl auth login oauth#23
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds OAuth 2.0 authorization-code flow (with refresh), integrates OAuth into the API client via an AuthMethod enum, persists OAuth tokens in keyring + file fallback, adds CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (login_oauth)
participant Browser
participant Local as Local Callback Server
participant Backlog as Backlog OAuth Endpoint
participant Storage as Keyring/File Storage
User->>CLI: trigger login_oauth(space, client_id, client_secret)
CLI->>Local: start callback listener (port)
CLI->>Backlog: build authorization URL
CLI->>Browser: open authorization URL
Browser->>Backlog: user authenticates & authorizes
Backlog->>Local: redirect to /callback with code & state
Local->>CLI: deliver authorization code
CLI->>Backlog: POST exchange code for tokens
Backlog->>CLI: return access_token & refresh_token
CLI->>Storage: persist OAuth tokens
Storage->>CLI: confirm stored
CLI->>User: display success
sequenceDiagram
participant App
participant Client as BacklogClient
participant API as Backlog API
participant Storage as Token Storage
App->>Client: make API request
Client->>Client: inspect AuthMethod (Bearer or ApiKey)
alt Bearer
Client->>API: add Authorization: Bearer <token>
API-->>Client: 401 Unauthorized
Client->>Storage: read refresh_token
Client->>API: POST token refresh
API->>Client: return new access_token
Client->>Storage: update tokens
Client->>API: retry request with new Bearer
else ApiKey
Client->>API: add apiKey query parameter
end
API->>Client: response
Client->>App: parsed result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds browser-based OAuth 2.0 authentication to the Backlog CLI, including token storage and automatic refresh, as an alternative to long-lived API keys.
Changes:
- Introduces an OAuth 2.0 authorization-code flow with a localhost callback listener and token refresh logic.
- Extends credential storage to persist OAuth tokens (keyring-first, file fallback).
- Updates the API client to support API key or Bearer auth and retry once on 401 after refresh; updates auth commands/status output and user guides.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/oauth.rs |
Implements OAuth flow (browser open, local callback server, token exchange/refresh, encode/decode helpers). |
src/secret.rs |
Adds keyring + oauth_tokens.toml fallback storage for OAuth tokens. |
src/api/mod.rs |
Adds AuthMethod (API key vs Bearer), applies auth to requests, retries once on 401 with refresh. |
src/cmd/auth.rs |
Adds login_oauth, updates logout to delete OAuth tokens, and extends auth status to show OAuth. |
src/main.rs |
Registers a new auth subcommand and wires it to cmd::auth::login_oauth. |
docs/user-guide.md |
Documents OAuth login and token storage locations; updates command table. |
docs/user-guide.ja.md |
Japanese documentation for OAuth login and storage locations. |
Cargo.toml |
Adds dependencies for browser launch and RNG (open, getrandom). |
Cargo.lock |
Locks new dependencies and transitive crates. |
.cspell/dicts/project.txt |
Adds new terms for spell-check. |
Comments suppressed due to low confidence (1)
src/cmd/auth.rs:104
logout_all()removesconfig.tomlandcredentials.toml, but it does not remove the OAuth token fallback file (oauth_tokens.toml). Since this PR introduces that file andlogout_allclaims “Config files removed.”, consider also deletingoauth_tokens.toml(e.g., add asecret::remove_oauth_tokens_file()helper and call it here) so logout-all fully cleans up stored auth material.
pub fn logout_all() -> Result<()> {
let cfg = config::load()?;
for key in &cfg.spaces {
secret::delete(key)?;
secret::delete_oauth_tokens(key)?;
}
config::remove_config_file()?;
secret::remove_credentials_file()?;
println!(
"{}",
"Logged out from all spaces. Config files removed.".yellow()
);
Ok(())
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cmd/auth.rs (1)
89-99:⚠️ Potential issue | 🟠 Major
logout --allstill leaves the OAuth fallback file behind.After the per-space deletes, this only removes
config.tomlandcredentials.toml. If OAuth tokens were stored inoauth_tokens.toml,bl auth logout --all/ uninstall--purgeleaves that file on disk.🧹 Minimal fix
config::remove_config_file()?; secret::remove_credentials_file()?; + secret::remove_oauth_tokens_file()?;Add the matching helper in
src/secret.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/auth.rs` around lines 89 - 99, The logout_all flow currently deletes per-space secrets via secret::delete and secret::delete_oauth_tokens but never removes the global oauth_tokens file; add a helper in secret.rs (e.g., remove_oauth_tokens_file or remove_oauth_tokens) that deletes the oauth_tokens.toml file and call it from logout_all alongside secret::remove_credentials_file (after the per-space loop) so the global OAuth fallback file is removed during `bl auth logout --all` / `--purge`.
🧹 Nitpick comments (1)
src/oauth.rs (1)
262-279: Add a UTF-8 regression test for the custom decoder.The current tests only cover ASCII inputs, but Lines 275-279 contain a separate multibyte path. A single non-ASCII round-trip case would pin that behavior down and guard against regressions in
percent_decode.Also applies to: 298-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/oauth.rs` around lines 262 - 279, Add a UTF-8 regression test that exercises the multibyte branch of percent_decode by percent-encoding a non-ASCII character (e.g., a multi-byte UTF-8 glyph), feeding it into percent_decode and asserting the decoded result equals the original Unicode string; do the same for the other decoder path referenced in the diff (the multibyte branch around the later decoder code) to ensure round-trip decoding of multibyte UTF-8 is preserved. Ensure the test uses a literal non-ASCII character (not ASCII-only) and covers both '%' hex-decoding and the fallback multibyte path in percent_decode so future changes cannot regress UTF-8 handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/mod.rs`:
- Around line 297-304: The match arm swallowing refresh errors should propagate
them and also surface failures when persisting tokens: when calling
crate::oauth::refresh_access_token(...) do not convert Err(_) into Ok(false) —
return or map the actual error so callers can handle it; after obtaining
new_tokens, check the Result from crate::secret::set_oauth_tokens(...) and
propagate or return that error if it fails (do not silently ignore it), while
still updating access_token.borrow_mut() and refresh_token.borrow_mut(); in
short, replace the Err(_) => Ok(false) behavior with returning the real refresh
error, and handle/publish errors from set_oauth_tokens().
In `@src/cmd/auth.rs`:
- Around line 42-48: Update the user prompt in auth.rs so it directs users to
register a space-level app instead of a personal app: change the printed path
from "Personal settings → OAuth 2.0 Applications → Add" to "Space settings →
Apps → Add (create a Confidential Client)" and keep the existing Redirect URI
print (the format!("http://localhost:{port}/callback").bold()) intact so the
correct localhost callback is shown.
In `@src/main.rs`:
- Around line 532-540: Refactor the flat AuthCommands::LoginOauth enum variant
into a nested subcommand structure so the CLI parses as "bl auth login oauth":
create a LoginArgs struct and change the AuthCommands enum to have a
Login(LoginArgs) variant; inside LoginArgs add a field like
#[command(subcommand)] pub cmd: LoginCmd; define a LoginCmd enum with an
Oauth(OauthArgs) variant (moving the existing LoginOauth fields into OauthArgs).
Update any code that matched on AuthCommands::LoginOauth to instead match
AuthCommands::Login(cmd) -> LoginCmd::Oauth(args) and preserve the existing
fields (no_banner, port) in the new OauthArgs type.
In `@src/oauth.rs`:
- Around line 51-56: The code opens the auth URL (open::that) before starting
the local callback listener, causing late discovery of port-in-use and a race;
change the flow so the callback listener is bound/started first (invoke or spawn
the listener created by wait_for_callback or its underlying bind function and
confirm bind success) and only after successful binding call
open::that(&auth_url); also apply the same ordering where the bind occurs around
the block referenced at lines 145-150 so any bind errors are returned before
launching the browser, and ensure proper error propagation from
wait_for_callback or the listener-start function.
- Line 36: The redirect URI is built with "localhost" in the redirect_uri
variable but the HTTP listener is bound to "127.0.0.1", causing a mismatch;
change the code so both use the same host string: either build redirect_uri with
"127.0.0.1" (update the redirect_uri =
format!("http://127.0.0.1:{port}/callback")) or change the listener bind to
"localhost" (update the TcpListener/Server bind call) — ensure the same host
constant/string is reused in both the redirect_uri construction and the listener
bind to keep them identical.
- Around line 243-246: The helper generate_state currently panics on random
generation; change its signature to return anyhow::Result<String> (or
Result<String, anyhow::Error>), replace expect("...") with the ? operator (or
.context("Failed to generate random state")?) to propagate the getrandom::Error,
and keep the hex-formatting logic returning Ok(string). Then update
run_oauth_flow to call generate_state()? and propagate its error (or handle it)
so the whole module uses anyhow-based error propagation instead of aborting.
- Around line 10-15: The derived Debug for OAuthTokens and TokenResponse exposes
sensitive fields; replace the automatic derive by implementing std::fmt::Debug
for both OAuthTokens and TokenResponse so that non-sensitive fields (e.g.,
client_id) are shown but client_secret, access_token, refresh_token (and any
other secret fields) are printed as "<redacted>" (use
f.debug_struct(...).field(...).finish() pattern) to avoid leaking credentials in
logs or error messages.
In `@src/secret.rs`:
- Around line 208-213: The current delete_oauth_tokens function swallows all
keyring delete errors; change it to call keyring::Entry::new and then match the
result of entry.delete_credential(), ignoring only keyring::Error::NoEntry and
otherwise returning an Err (propagate the failure) before proceeding to
oauth_file_delete(space_key); keep calling oauth_file_delete only when the
keyring delete either succeeded or failed with NoEntry. Use the concrete symbols
delete_oauth_tokens, keyring::Entry::new, entry.delete_credential,
keyring::Error::NoEntry, and oauth_file_delete to locate and update the logic so
keyring failures are surfaced.
- Around line 264-267: The current oauth_file_set uses
oauth_file_load().unwrap_or_default(), which masks parse/permission errors and
can overwrite oauth_tokens.toml; change oauth_file_set to propagate the load
error instead of defaulting—call oauth_file_load() with ? (or otherwise return
the Err) to bail on load failures, then insert into file.tokens and call
oauth_file_save(&file) as before so we never overwrite the file after a failed
load; reference functions oauth_file_set, oauth_file_load, and oauth_file_save.
---
Outside diff comments:
In `@src/cmd/auth.rs`:
- Around line 89-99: The logout_all flow currently deletes per-space secrets via
secret::delete and secret::delete_oauth_tokens but never removes the global
oauth_tokens file; add a helper in secret.rs (e.g., remove_oauth_tokens_file or
remove_oauth_tokens) that deletes the oauth_tokens.toml file and call it from
logout_all alongside secret::remove_credentials_file (after the per-space loop)
so the global OAuth fallback file is removed during `bl auth logout --all` /
`--purge`.
---
Nitpick comments:
In `@src/oauth.rs`:
- Around line 262-279: Add a UTF-8 regression test that exercises the multibyte
branch of percent_decode by percent-encoding a non-ASCII character (e.g., a
multi-byte UTF-8 glyph), feeding it into percent_decode and asserting the
decoded result equals the original Unicode string; do the same for the other
decoder path referenced in the diff (the multibyte branch around the later
decoder code) to ensure round-trip decoding of multibyte UTF-8 is preserved.
Ensure the test uses a literal non-ASCII character (not ASCII-only) and covers
both '%' hex-decoding and the fallback multibyte path in percent_decode so
future changes cannot regress UTF-8 handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cd1cc32-d7dd-468b-a8bb-802d4c91c46f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.cspell/dicts/project.txtCargo.tomldocs/user-guide.ja.mddocs/user-guide.mdsrc/api/mod.rssrc/cmd/auth.rssrc/main.rssrc/oauth.rssrc/secret.rs
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user-guide.ja.md`:
- Around line 234-235: Update the docs entry for the `bl auth login-oauth`
command to document the `--no-banner` option as well as `--port`; mention that
the command maps to AuthCommands::LoginOauth and the CLI handler function
login_oauth(no_banner, port) so users know `--no-banner` is accepted (e.g.,
"ブラウザベースの OAuth 2.0 で認証。`--port <port>`（デフォルト: 54321）および `--no-banner` を使用可能").
- Around line 126-134: The user guide's OAuth registration path conflicts with
the CLI behavior; update either the documentation or the CLI so both show the
same Backlog UI path. Locate the doc section that currently says "スペース設定 → アプリ"
and the login_oauth() function in src/cmd/auth.rs (which points users to
"Personal settings → OAuth 2.0 Applications → Add") and make them consistent:
either change the doc text to match login_oauth() or modify login_oauth() to
point to "スペース設定 → アプリ" (and update any UI labels/messages accordingly), then
run a quick manual check to confirm the displayed path in the CLI and the doc
now match.
- Around line 848-850: The troubleshooting section "キーリングが利用できない" currently only
documents restoring credentials from `~/.config/bl/credentials.toml`; add
equivalent instructions for OAuth users describing
`~/.config/bl/oauth_tokens.toml` as the OAuth fallback (mode 0600), how to
create/populate it with tokens, set permissions (e.g., chmod 600), and explain
that the system keyring is preferred but the oauth_tokens file will be used if
the keyring is unavailable; apply the same edits to the corresponding secondary
location noted (also applies to lines 857-859) so both places consistently
document the OAuth fallback file and recovery steps.
In `@docs/user-guide.md`:
- Line 238: The docs are missing the --no-banner option for the LoginOauth
command; update the `bl auth login-oauth` row in the command table to mention
`--no-banner` (in addition to `--port <port>`) since the command is implemented
as AuthCommands::LoginOauth { no_banner, port }, i.e., document that you can
pass `--no-banner` to suppress the banner (alongside the existing note about
overriding the default callback port 54321).
- Around line 853-855: Update the "Keyring is unavailable" troubleshooting
paragraph in the "Keyring is unavailable" section to mention the OAuth fallback
file alongside the credentials fallback: include
`~/.config/bl/oauth_tokens.toml` (mode 0600) in the recovery steps and examples
where `~/.config/bl/credentials.toml` is currently referenced so both API key
and OAuth token fallbacks are documented; ensure the wording matches the table
entries for `~/.config/bl/credentials.toml` and
`~/.config/bl/oauth_tokens.toml`.
- Around line 130-138: The docs' Step 1 currently points to "Space settings →
Apps" but the CLI command bl auth login-oauth prints "Personal settings → OAuth
2.0 Applications → Add"; update the user-guide.md Step 1 text to match the CLI
output (use "Personal settings → OAuth 2.0 Applications → Add") so the
instructions align with the bl auth login-oauth flow, or alternatively change
the literal help/output in the login-oauth implementation to match the
docs—locate the CLI message in the auth command (the string printed by bl auth
login-oauth) and make the two messages identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95909ef5-c93e-4ee0-8bf4-7091497edf1d
📒 Files selected for processing (2)
docs/user-guide.ja.mddocs/user-guide.md
Addresses review comment: "generate_state() uses expect(...), which will panic the entire CLI"
Addresses review comment: "directs users to Personal settings instead of Space settings → Apps"
Addresses review comment: "match arm swallowing refresh errors should propagate them"
…auth_file_set Addresses review comments: keyring errors swallowed on logout; unwrap_or_default can erase other spaces' tokens
…xpand keyring fallback Addresses review comments: localhost/127.0.0.1 mismatch in docs, missing --no-banner for login-oauth, missing OAuth fallback in troubleshooting
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/oauth.rs (1)
55-64:⚠️ Potential issue | 🟠 MajorUnify the registered callback host with the actual redirect URI.
run_oauth_flownow sendshttp://127.0.0.1:{port}/callback, butsrc/cmd/auth.rs:45-49still tells users to registerhttp://localhost:{port}/callback. The PR notes that Backlog requires an exact Redirect URI, so a user who follows the CLI prompt will register the wrong URI and this auth request will be rejected. Please drive both strings from one shared constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/oauth.rs` around lines 55 - 64, The redirect URI used in run_oauth_flow is hardcoded to "http://127.0.0.1:{port}/callback" while the CLI help text tells users to register "http://localhost:{port}/callback"; create a single shared constant (e.g., CALLBACK_HOST or CALLBACK_URI_TEMPLATE) and use it in run_oauth_flow (where redirect_uri is formed and percent_encoded) and in the CLI prompt/registration message so both the generated auth_url and the text shown to users come from the same source; update references in the functions run_oauth_flow and the auth command’s help/print routine to construct the final URI by interpolating the port into the shared template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/oauth.rs`:
- Around line 192-193: parse_callback_params currently ignores the OAuth error
query param so wait_for_callback later fails with a misleading "code parameter
missing" and no browser response; update parse_callback_params to parse and
return an error variant (or include Option<error>) when the query contains error
and state, and update wait_for_callback (and the callback HTTP handler logic
referenced in the same area) to detect that error case, write an appropriate
HTML response to the browser (e.g., "consent denied" or the provider error) and
return a clear Err with the provider error/state instead of treating it as a
missing code; reference the functions parse_callback_params and
wait_for_callback to locate where to add error parsing, response rendering, and
error propagation.
- Around line 90-92: The reqwest blocking Client builders use infinite timeouts;
update each client construction to set finite timeouts by adding
.timeout(Duration::from_secs(30)) and .connect_timeout(Duration::from_secs(10))
on the Client::builder() before build(), importing std::time::Duration as
needed; apply this change where you create the clients for OAuth token exchange
and refresh (the functions handling token exchange/refresh in oauth.rs) and in
the BacklogClient builders (BacklogClient::from_config and
BacklogClient::new_with) so all four client builders use these timeouts.
---
Duplicate comments:
In `@src/oauth.rs`:
- Around line 55-64: The redirect URI used in run_oauth_flow is hardcoded to
"http://127.0.0.1:{port}/callback" while the CLI help text tells users to
register "http://localhost:{port}/callback"; create a single shared constant
(e.g., CALLBACK_HOST or CALLBACK_URI_TEMPLATE) and use it in run_oauth_flow
(where redirect_uri is formed and percent_encoded) and in the CLI
prompt/registration message so both the generated auth_url and the text shown to
users come from the same source; update references in the functions
run_oauth_flow and the auth command’s help/print routine to construct the final
URI by interpolating the port into the shared template.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/cmd/auth.rs (1)
190-212: Same silent error masking pattern asfrom_config.This mirrors the
if let Ok()pattern insrc/api/mod.rs:237. For consistency, if you address the suggestion there, apply the same approach here to surface OAuth load errors vs "not found".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/auth.rs` around lines 190 - 212, The current if let Ok(tokens) = secret::get_oauth_tokens(&space_key) silently ignores errors when loading OAuth tokens; change this to match on secret::get_oauth_tokens(&space_key) so you can distinguish a "not found" case from real load errors and log/return the latter (similar to how you will update src/api/mod.rs). In the match arm where you get Err(e), surface the error (use json output or colored message like the BacklogClient::from_config() error handling), and only proceed to construct AuthDisplay::OAuth, create BacklogClient via BacklogClient::from_config(), and call status_with(...) when tokens were loaded successfully. Ensure you reference get_oauth_tokens, AuthDisplay::OAuth, BacklogClient::from_config, and status_with when making the changes.src/oauth.rs (1)
316-332:percent_decodedoes not decode+as space.RFC 3986 percent-encoding doesn't treat
+specially, but form-encoded data (RFC 1866 /application/x-www-form-urlencoded) uses+for spaces. OAuth error descriptions may arrive form-encoded. The test at Line 380 asserts"User+denied"rather than"User denied", which suggests this is intentional — but users will see the raw+in error messages.Consider decoding
+to space for better UX in error messages:♻️ Optional fix
fn percent_decode(s: &str) -> String { let mut bytes: Vec<u8> = Vec::with_capacity(s.len()); let mut chars = s.chars(); while let Some(c) = chars.next() { if c == '%' { let h1 = chars.next().unwrap_or('0'); let h2 = chars.next().unwrap_or('0'); if let Ok(byte) = u8::from_str_radix(&format!("{h1}{h2}"), 16) { bytes.push(byte); } + } else if c == '+' { + bytes.push(b' '); } else { let mut buf = [0u8; 4]; bytes.extend_from_slice(c.encode_utf8(&mut buf).as_bytes()); } } String::from_utf8_lossy(&bytes).into_owned() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/oauth.rs` around lines 316 - 332, The percent_decode function currently ignores '+' and leaves it as literal plus; update percent_decode to treat '+' as a space by handling the '+' case in the main loop (e.g., push ASCII 0x20 or extend bytes with b' '), while keeping existing percent-decoding logic for '%' and UTF-8 encoding for other chars; modify any tests expecting "User+denied" if they should expect "User denied". Ensure the change is made inside the percent_decode function so all callers (OAuth error handling) receive decoded spaces.src/api/mod.rs (1)
236-249: OAuth configuration errors are silently masked by fallback to API key authentication.The
if let Ok()pattern on line 237 swallows all errors fromget_oauth_tokens, including parse errors ("Failed to parse oauth_tokens.toml") and read errors ("Failed to read {path}"). This means ifoauth_tokens.tomlis corrupted or inaccessible, the client will silently fall back to API key auth without the user's knowledge. Only "tokens not found for this space" errors are intentionally caught.To distinguish "no tokens configured" from "tokens exist but failed to load", check the error message:
♻️ Suggested approach
- if let Ok(tokens) = crate::secret::get_oauth_tokens(&space_key) { + match crate::secret::get_oauth_tokens(&space_key) { + Ok(tokens) => { + return Ok(Self { + client, + base_url, + auth: AuthMethod::Bearer { + access_token: RefCell::new(tokens.access_token), + refresh_token: RefCell::new(tokens.refresh_token), + client_id: tokens.client_id, + client_secret: tokens.client_secret, + space_key, + }, + }); + } + Err(e) if e.to_string().contains("not found") => { + // No OAuth tokens configured for this space, fall through to API key + } + Err(e) => { + // OAuth tokens exist but failed to load - surface the error + return Err(e); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/mod.rs` around lines 236 - 249, The current if-let Ok(get_oauth_tokens(&space_key)) swallows all errors and silently falls back to API key; instead match on crate::secret::get_oauth_tokens(&space_key) and only fall back when the error indicates "tokens not found for this space" (or the specific NotFound variant/message your secret module returns); for other errors (parse/read/corruption) propagate or return the error (or wrap with context) so the failure is visible. Update the match branch that constructs AuthMethod::Bearer on Ok(tokens), handle Err(e) by checking e.to_string() or pattern-matching the error type from get_oauth_tokens, and return Err(e) (or log+return) for non-"not found" errors while only proceeding to API-key logic when the error indicates missing tokens for space_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api/mod.rs`:
- Around line 236-249: The current if-let Ok(get_oauth_tokens(&space_key))
swallows all errors and silently falls back to API key; instead match on
crate::secret::get_oauth_tokens(&space_key) and only fall back when the error
indicates "tokens not found for this space" (or the specific NotFound
variant/message your secret module returns); for other errors
(parse/read/corruption) propagate or return the error (or wrap with context) so
the failure is visible. Update the match branch that constructs
AuthMethod::Bearer on Ok(tokens), handle Err(e) by checking e.to_string() or
pattern-matching the error type from get_oauth_tokens, and return Err(e) (or
log+return) for non-"not found" errors while only proceeding to API-key logic
when the error indicates missing tokens for space_key.
In `@src/cmd/auth.rs`:
- Around line 190-212: The current if let Ok(tokens) =
secret::get_oauth_tokens(&space_key) silently ignores errors when loading OAuth
tokens; change this to match on secret::get_oauth_tokens(&space_key) so you can
distinguish a "not found" case from real load errors and log/return the latter
(similar to how you will update src/api/mod.rs). In the match arm where you get
Err(e), surface the error (use json output or colored message like the
BacklogClient::from_config() error handling), and only proceed to construct
AuthDisplay::OAuth, create BacklogClient via BacklogClient::from_config(), and
call status_with(...) when tokens were loaded successfully. Ensure you reference
get_oauth_tokens, AuthDisplay::OAuth, BacklogClient::from_config, and
status_with when making the changes.
In `@src/oauth.rs`:
- Around line 316-332: The percent_decode function currently ignores '+' and
leaves it as literal plus; update percent_decode to treat '+' as a space by
handling the '+' case in the main loop (e.g., push ASCII 0x20 or extend bytes
with b' '), while keeping existing percent-decoding logic for '%' and UTF-8
encoding for other chars; modify any tests expecting "User+denied" if they
should expect "User denied". Ensure the change is made inside the percent_decode
function so all callers (OAuth error handling) receive decoded spaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9889d56f-9567-4859-985b-de13e24582c6
📒 Files selected for processing (6)
docs/user-guide.ja.mddocs/user-guide.mdsrc/api/mod.rssrc/cmd/auth.rssrc/oauth.rssrc/secret.rs
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
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.
Checklist
mainSummary
bl auth login oauthcommand for browser-based OAuth 2.0 authentication--port) to match Backlog's exact Redirect URI requirementReason for change
Backlog API keys are static and long-lived. OAuth 2.0 provides time-limited tokens with automatic refresh, which is safer for interactive use.
Changes
New files
src/oauth.rs— OAuth 2.0 authorization code flow, local TcpListener callback server, token refresh, percent encode/decode helpersModified files
Cargo.toml/Cargo.lock— addopen = "5"(browser launch),getrandom = "0.3"(CSPRNG state)src/secret.rs—get_oauth_tokens/set_oauth_tokens/delete_oauth_tokenswith keyring-first and file fallback (oauth_tokens.toml, mode 0600)src/api/mod.rs—AuthMethodenum (ApiKey/Bearer),RefCell-based token refresh,execute()closure retry pattern on 401src/cmd/auth.rs—login_oauth(),AuthDisplayenum, updatedstatus/status_with/build_status_json/logout/logout_allsrc/main.rs—mod oauth,AuthCommands::LoginOauth { no_banner, port }docs/user-guide.md/docs/user-guide.ja.md— document the new command and OAuth token storage locationsNotes
bl auth login oauthrequires users to register their own OAuth application in Backlog space settings (Backlog only supports space-level registration, so credentials cannot be bundled)