fix(cli-generator): support Basic Auth keyring login - #17550
fix(cli-generator): support Basic Auth keyring login#17550devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Prompt for both halves on `auth login --with-token` for HTTP Basic schemes, store them in per-part keyring slots the Basic binding resolves at request time, report Basic as two required slots in `auth status`, clear both on logout, and reject undeclared --scheme names. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
The Basic-auth login/status/logout split is coherent and the two derived keyring slots keep AuthCredentialSource untouched. A few issues: Vec::dedup() on the unsorted declared list won't remove non-adjacent duplicates, the Basic password is read with terminal echo on, the logout loop can leave one half behind on error, and run_basic_paste skips the env-shadowing warning that ADR-0008 promises for login flows.
- 🟡 2 warning(s)
- 🔵 3 suggestion(s)
To request another review, comment /ai-review on this pull request.
| let mut valid = declared.clone(); | ||
| valid.dedup(); |
There was a problem hiding this comment.
🟡 warning
dedup() only collapses consecutive duplicates. declared is bindings-then-flows, so a scheme that has both a binding and a login flow (e.g. bindings ["a","b"], flow "a") yields ["a","b","a"] and the error message lists a, b, a. Dedup order-preserving instead:
| let mut valid = declared.clone(); | |
| valid.dedup(); | |
| let mut valid: Vec<&str> = Vec::new(); | |
| for name in &declared { | |
| if !valid.contains(name) { | |
| valid.push(name); | |
| } | |
| } |
| let _ = write!(err, "Password: "); | ||
| let _ = err.flush(); | ||
| let password = read_line_from_stdin("password")?; |
There was a problem hiding this comment.
🟡 warning
The password is read via plain read_line, so it is echoed to the terminal in interactive use (and lands in scrollback). For the token paste path that was arguably tolerable; for an explicit Password: prompt it's a visible regression in hygiene. Consider a no-echo read when stdin is a TTY (falling back to read_line_from_stdin when piped, which the e2e test relies on).
| for part in [BASIC_USERNAME_PART, BASIC_PASSWORD_PART] { | ||
| store.delete(cli_name, &basic_keyring_account(&scheme, part))?; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔵 suggestion
? inside the loop means a failure deleting :username leaves :password in the keyring — a half-logged-out state that is worse than either endpoint. Attempt both deletes and return the first error afterwards:
| for part in [BASIC_USERNAME_PART, BASIC_PASSWORD_PART] { | |
| store.delete(cli_name, &basic_keyring_account(&scheme, part))?; | |
| } | |
| } | |
| let mut first_err = None; | |
| for part in [BASIC_USERNAME_PART, BASIC_PASSWORD_PART] { | |
| if let Err(e) = store.delete(cli_name, &basic_keyring_account(&scheme, part)) { | |
| first_err = first_err.or(Some(e)); | |
| } | |
| } | |
| if let Some(e) = first_err { | |
| return Err(e); | |
| } | |
| } |
| fn run_basic_paste(cli_name: &str, scheme_name: &str) -> Result<(), CliError> { | ||
| let stderr = std::io::stderr(); | ||
| let mut err = stderr.lock(); | ||
|
|
||
| let _ = writeln!( | ||
| err, | ||
| "Scheme `{scheme_name}` uses HTTP Basic auth, which needs two values." | ||
| ); | ||
| let _ = write!(err, "Username: "); | ||
| let _ = err.flush(); | ||
| let username = read_line_from_stdin("username")?; | ||
| let _ = write!(err, "Password: "); | ||
| let _ = err.flush(); | ||
| let password = read_line_from_stdin("password")?; | ||
|
|
||
| let store = active_store(); | ||
| store.set( | ||
| cli_name, | ||
| &basic_keyring_account(scheme_name, BASIC_USERNAME_PART), | ||
| &username, | ||
| )?; | ||
| store.set( | ||
| cli_name, | ||
| &basic_keyring_account(scheme_name, BASIC_PASSWORD_PART), | ||
| &password, | ||
| )?; | ||
|
|
||
| let _ = writeln!( | ||
| err, | ||
| "{}", | ||
| green(&format!( | ||
| "✓ Stored username and password for {cli_name}:{scheme_name} in {}", | ||
| store.backend_label() | ||
| )) | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🔵 suggestion
run_basic_paste skips two things the token path provides: the env-shadowing warning ADR-0008 documents for auth login ("warns at flow start when an env var would shadow the keyring entry") and the token_paste_url hint. Since env keeps precedence over these new keyring slots, a user with TWILIO_PASSWORD set will store a password that never takes effect and get no hint why. Worth emitting the same shadow warning for each half before storing.
| }) | ||
| .collect(); | ||
| "source": describe_source(src), | ||
| "part": slot.part, |
There was a problem hiding this comment.
🔵 suggestion
"part": slot.part emits "part": null for every token/OAuth entry, which changes the existing --json shape for all non-Basic schemes. If any consumers do strict schema validation, consider omitting the key when part is None (build the object with serde_json::Map and insert conditionally).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 participant
Description
Linear ticket: Refs
Follow-up to #17546 (Twilio CLI feedback). On a CLI whose auth scheme is HTTP Basic,
auth login --with-tokenwas a silent no-op: it stored one pasted token under(cli_name, scheme_name), butinject_keyring_sourcesdeliberately skippedSchemeBinding::Basic, so no provider ever read that entry —auth statusstill reportedlogged_in: falseimmediately after✓ Stored credential. Two adjacent papercuts from the same report are fixed here too: Basicauth statusflattened username+password into one fallback chain (password-only environment ⇒logged_in: true, username labelledshadowed), and an unknown--schemename was accepted silently, producing the misleading "No login flow declared for schemeBasicAuth" instead of naming the schemes the binary declares.HTTP Basic needs two independent values, so it cannot share the single keyring slot the token schemes use. It now writes two derived slots —
<scheme>:username/<scheme>:password— which keepsAuthCredentialSource::Keyringand every storage backend behind it untouched. ADR-0008 is updated to document the two-entry shape.Token and OAuth paths are unchanged: the Basic branch is only reached when the resolved scheme's binding is
SchemeBinding::Basic, and env sources keep precedence over the keyring (the keyring source is appended to the existing chain, exactly as for tokens).Changes Made
run_basic_paste:auth login --with-tokenon a Basic scheme prompts for username and password (labelled stdin reads, clear error naming the missing half) and stores each in its own keyring slot.inject_keyring_sources:SchemeBinding::Basicnow appends a per-part keyring source to both halves instead of being a no-op, so the Basic provider resolves the stored values at request time.auth status:expand_sources→expand_slots, returning oneCredentialSlotper required value. Basic reportslogged_inonly when both slots resolve; JSON entries carry a"part"field, human output prefixesusername:/password:. No secret values are printed.auth logout: also deletes both Basic-derived slots.resolve_scheme: an explicit--schemethat isn't declared errors withUnknown auth scheme \X`. This CLI declares: …` (unchanged escape hatch for CLIs that declare no schemes).generators/cli/changes/unreleased/fix-basic-auth-login-and-status.yml.Testing
Unit tests (
cargo test --lib auth::: 292 passed; fullcargo test: 1922 passed): Basic slot splitting, both-halves-required status, keyring injection covering both halves, logout clearing both slots, undeclared-scheme validation, plus the existing token/OAuth cases.End-to-end on a real Twilio CLI built from this branch (scheme
accountSid_authToken, no env vars set), against a local HTTP server:auth login --scheme BasicAuth→Unknown auth scheme \BasicAuth`. This CLI declares: accountSid_authToken.`printf 'ACfakesid\nfaketoken\n' | twilio auth login --with-token --scheme accountSid_authToken→ stores both;auth status --jsonflips to"logged_in": truewithpart: username/part: passwordeachactive.authorization: Basic QUNmYWtlc2lkOmZha2V0b2tlbg==(decodes toACfakesid:faketoken).TWILIO_USERNAME/TWILIO_PASSWORDalso set, the header decodes to the env values — keyring stays the lower-precedence fallback.auth logout→ all four sourcesmissing,"logged_in": false.Link to Devin session: https://app.devin.ai/sessions/f08b0a3d82234745a76bf2369d295c29
Open in Devin Desktop: https://app.devin.ai/desktop/session/f08b0a3d82234745a76bf2369d295c29?variant=devin