BC5 device logins request full access by default - #616
Conversation
Smoke-testing the device flow against a live BC5 authorization server (bc3 beta14) surfaced three gaps between "the flow completes" and "the CLI is usable". All three are invisible while BC5 is dark, and all three become everyone's first impression the day it isn't. Request full access by default. BC5 defaults an omitted scope to its least-privilege registered entry — read — so an unqualified `auth login` produced a token that 403s every write. Launchpad logins have always been read-write, so matching that is what keeps go-live from silently demoting everyone; `--scope read` is how a caller asks for less. Resolve the account from the token's own RFC 8707 resource indicator. A BC5 token is bound to exactly one account, so there is nothing to discover and no picker to show. It also works where discovery cannot: BC3 serves /authorization.json only on the API host, which beta deployments do not route, leaving `--account` mandatory there for no reason a user could see. Explain an insufficient-scope refusal. BC5 checks scope before it resolves the resource and answers with an empty body, so a read-scoped write reported a bare "access denied" with no remedy. ErrForbiddenScope already carried the right words and had no caller. It stays keyed to read-scoped BC5 credentials — Launchpad tokens carry no scope, so their 403 is a real permission failure and is left alone.
github-actions
Bot
added
commands
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4322f9a076
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR prepares the BC5 (RFC 8628 device flow) OAuth path for staff go-live by closing three usability gaps that only surface once a live BC5 authorization server starts advertising resource metadata. It touches the auth manager, account resolution, and top-level error handling, plus documentation/help text, while leaving Launchpad and current production behavior unchanged.
Changes:
- Device logins now request
fullscope explicitly by default (previously an omitted scope let BC5 pick its least-privilegereaddefault), with--scope readas the opt-down; help text and docs updated accordingly. - Account resolution gains a step that reads the account a BC5 token is bound to via its RFC 8707 resource indicator (
urn:bc:account:<id>), avoiding a mandatory--accountand a network fetch that fails on beta hosts. - A read-scoped BC5 token's otherwise unexplained 403 is rewritten into an actionable "insufficient scope" error (
explainInsufficientScope/scopeErrorFor), while server-hinted 403s and Launchpad tokens are left untouched.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/auth/auth.go | Defaults device scope to full; adds IsReadOnly() and AccountID() (RFC 8707 binding with digit-only validation); simplifies effective-scope fallback |
| internal/cli/root.go | Adds explainInsufficientScope/scopeErrorFor to rewrite bare read-scoped 403s into scope guidance |
| internal/tui/resolve/account.go | Inserts BC5 token-binding as resolution step 3 before the interactive prompt; updates precedence doc comment |
| internal/tui/resolve/account_test.go | Tests binding-based resolution and flag precedence over the binding |
| internal/auth/device_test.go | Tests full-by-default/explicit-read scope wiring, AccountID cases, and IsReadOnly |
| internal/cli/root_test.go | Tests the 403 rewrite (bare vs. server-hinted vs. non-403) |
| internal/commands/auth.go | Updates --scope flag help to note "default full" |
| internal/commands/profile.go | Updates --scope flag help to note "default full" |
| README.md | Updates login examples to reflect full as the default |
| skills/basecamp/SKILL.md | Adds --scope read example and clarifies full is the default |
| e2e/auth.bats | Asserts help output contains "default full" for login and profile create |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AccessToken() short-circuits on BASECAMP_TOKEN, so a request carrying an environment token never touches the credential store. AccountID() and IsReadOnly() read it anyway, and both then describe a token that is not the one in play. With stale BC5 credentials on disk and BASECAMP_TOKEN set, the resolver selected the stored binding's account — silently addressing an account the environment token may have nothing to do with — and a genuine 403 from that token was relabeled "insufficient scope", pointing the user at a re-login that would not have helped. Both now defer to the environment token: no account binding to offer, and no stored scope to blame a refusal on.
jeremy
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c18ee003e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
It relabeled every unexplained 403 on a read-scoped token as a missing scope, including refusals where read scope was already sufficient: `hillcharts show` documents legitimate 403s and falls them through (internal/commands/hillcharts.go:101), and `gauges list` can 403 when the feature is disabled. Both are reads, and both would have told the user to re-authenticate with --scope full — advice that could not have helped. Telling read from write needs either the WWW-Authenticate challenge, which the SDK drops, or per-command mutation context the catalog does not carry. A verb-name allowlist would fail exactly the way this did, silently and on whichever command it forgot. Requesting full access by default already removes the cliff this was meant to soften, so the honest move is to report the server's 403 as it arrived until the signal exists. ErrForbiddenScope keeps its wording for that day; a communiqué asks the SDK to surface the challenge code.
|
Heads up for anyone re-reading: the third change described in the earlier review summaries — the read-scoped 403 → "insufficient scope" rewrite ( It fired on genuine read-side refusals ( The PR is now two changes: full scope by default and account resolution from the RFC 8707 token binding. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/auth/auth.go:569
- The PR description's "fix 3" states that an insufficient-scope 403 is now rewritten to the actionable
output.ErrForbiddenScope()hint ("Run: basecamp auth login --scope full"), and the Verification/coverage notes claim a test for "the 403 rewrite including a server-hinted 403 it must not touch." However, this branch contains no code wiring that behavior:output.ErrForbiddenScope()still has zero callers (only defined ininternal/output/errors.goand exercised byinternal/output/output_test.go), and no test covers a scope-based 403 rewrite. Fixes 1 (scope default) and 2 (account binding) are present, but fix 3 appears to be missing from the diff. Please either include the fix-3 implementation and its test, or update the PR description so it doesn't claim behavior that isn't shipped here.
// Request a scope explicitly rather than letting the server pick. BC5
// defaults an omitted scope to its least-privilege entry (read), which
// would silently hand every write command a 403 — Launchpad logins have
// always been read-write, so an unqualified `auth login` keeps meaning
// that here. --scope read is how a caller asks for less.
requestedScope := opts.Scope
if requestedScope == "" {
requestedScope = scopeFull
}
Resource-first discovery and the device flow shipped in v0.8.0/v0.8.1, but
nothing had exercised them against a live BC5 authorization server — production
still 404s its resource metadata, so every login falls back to Launchpad and the
BC5 path never runs.
bc3's
beta14now serves a complete authorization server, so I smoke-tested thereleased code against it. The flow works: discovery selects the BC5 issuer, the
device grant completes, the token is account-bound, and refresh rotates it. Two
gaps sat between "the flow completes" and "the CLI is usable" — both invisible
while BC5 is dark, both of them everyone's first impression the day it isn't.
1. An unqualified login was read-only
BC5 defaults an omitted
scopeto the client's least-privilege registered entry(
read). The CLI omitted it, sobasecamp auth loginproduced a token that 403severy write. Launchpad logins have always been read-write, so shipping this as-is
would silently demote every user at go-live.
Device logins now request
fullexplicitly.--scope readis how a caller asksfor less.
This is a behavior change for anyone already on v0.8.0/v0.8.1 who logs in
after BC5 goes live: they get
fullwhere the current code would have given themread. That is the intent — it restores Launchpad parity — but it belongs in therelease notes rather than shipping quietly.
2.
--accountwas mandatory against beta, for no visible reasonA BC5 token is bound to exactly one account by its RFC 8707 resource indicator
(
urn:bc:account:<id>), so there is nothing to discover and no picker to show.Account resolution now reads that binding, ahead of the network fetch.
It also works where discovery cannot. BC3 gates
/authorization.jsononapi_request?, which is true only on the API host — and beta's API host maps toproduction's, which does not route to the beta deployment:
So account discovery 401s on every beta host. Before this change that made
BASECAMP_ACCOUNT_IDmandatory for anyone smoke-testing there; the failure readas "invalid or expired token", which is not what happened. Production is
unaffected either way — its API host is the API host.
Both accessors that read stored credentials (
AccountID, andIsReadOnlywhileit existed) defer to
BASECAMP_TOKEN, matching the precedenceAccessTokenandAuthorizationEndpointalready establish. Otherwise a stale BC5 credential couldaim resolution at an account the environment token has nothing to do with.
What this PR deliberately does not do
An earlier revision also rewrote a bare 403 into "insufficient scope — run
auth login --scope full". Review caught that it fires on genuine read-siderefusals too:
hillcharts showdocuments legitimate 403s and falls them through(
internal/commands/hillcharts.go:101), andgauges listcan 403 when thefeature is disabled. Both are reads under a read token, where the advice could
not have helped.
Separating those needs either the
WWW-Authenticatechallenge — which the SDKdrops — or per-command mutation context the catalog does not carry. A verb-name
allowlist would fail the same way, silently, on whichever command it forgot.
Since requesting
fullby default removes the cliff that rewrite was meant tosoften, the 403 is now reported as the server sent it.
output.ErrForbiddenScopekeeps its wording for when the signal exists; a communiqué asks the SDK to
surface the challenge code.
Verification
Against
beta14.3.bc4-beta.com, an actual BC5 authorization server:advertises both BC5 and Launchpad; the SDK filters Launchpad out, leaving
exactly one candidate. Worth knowing that the ambiguity guard is load-bearing
in real config — a second non-Launchpad issuer would hard-fail rather than fall
back.
auth loginreportsAccess: full;credentials store
oauth_type: bc5withresource: urn:bc:account:2914079.auth refreshrotates the token and echoes the resource indicator.projects listreturns real data with no account configured (fix 2).read-scoped one — server-side, before the resource is resolved: a write to a
nonexistent bucket returns 403, not 404. The one todo created during testing
was trashed.
device code expired before authorization completed, exit 3) and leaves existing credentials intact.bin/cigreen on macOS and on Linux/x86 (32-core). New coverage: scope defaultand explicit
readat the device-authorization form; the account-binding tableincluding the service-origin form and traversal-shaped input;
BASECAMP_TOKENprecedence over the binding, with the no-env-token case pinned so it cannot pass
off ambient state; flag precedence over the binding.
Not broken
Production behavior is unchanged.
3.basecampapi.comstill 404s resourcemetadata, so logins classify as
resource_discovery_failedand fall back toLaunchpad with today's warning line. Stored Launchpad credentials keep refreshing
through the Launchpad path; none of this touches them. BC5 activates per host as
each host starts advertising, with no further CLI change.
The first production BC5 login remains a separate gate — beta14 derisks it, but
does not close it.