From 4322f9a076d0f75aed798fd0133b64e75e2914cd Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 23:32:58 -0700 Subject: [PATCH 1/3] auth: ready the BC5 device login for staff go-live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- README.md | 6 +- e2e/auth.bats | 2 + internal/auth/auth.go | 80 ++++++++++++++++++-- internal/auth/device_test.go | 107 ++++++++++++++++++++++++++- internal/cli/root.go | 29 ++++++++ internal/cli/root_test.go | 32 ++++++++ internal/commands/auth.go | 2 +- internal/commands/profile.go | 2 +- internal/tui/resolve/account.go | 16 +++- internal/tui/resolve/account_test.go | 54 ++++++++++++++ skills/basecamp/SKILL.md | 3 +- 11 files changed, 313 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index f6768afdb..ac6999763 100644 --- a/README.md +++ b/README.md @@ -121,9 +121,9 @@ no modern OAuth issuer is advertised for the server; once a modern issuer is selected, login failures surface loudly rather than silently falling back. ```bash -basecamp auth login # Authenticate with Basecamp -basecamp auth login --scope read # Read-only access (default; ignored by Launchpad) -basecamp auth login --scope full # Full read+write access (ignored by Launchpad) +basecamp auth login # Authenticate with Basecamp (full access) +basecamp auth login --scope read # Read-only access (ignored by Launchpad) +basecamp auth login --scope full # Full read+write access (default; ignored by Launchpad) basecamp auth token # Print token for scripts ``` diff --git a/e2e/auth.bats b/e2e/auth.bats index 16f36f8c0..8c879154b 100644 --- a/e2e/auth.bats +++ b/e2e/auth.bats @@ -79,6 +79,7 @@ load test_helper assert_success assert_output_contains "Headless authentication with manual browser instructions" assert_output_contains "ignored by Launchpad" + assert_output_contains "default full" } @test "basecamp profile create --help describes flags provider-neutrally" { @@ -86,6 +87,7 @@ load test_helper assert_success assert_output_contains "Headless authentication with manual browser instructions" assert_output_contains "ignored by Launchpad" + assert_output_contains "default full" } @test "basecamp auth login rejects --device-code --local" { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index ca0399c54..6d782559f 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -48,6 +48,13 @@ const ( oauthTypeLaunchpad = "launchpad" ) +// OAuth scopes the BC5 client is registered for. Launchpad ignores scope +// entirely; its tokens are read-write. +const ( + scopeRead = "read" + scopeFull = "full" +) + // Default OAuth callback address and redirect URI. const ( defaultCallbackAddr = "127.0.0.1:8976" @@ -393,7 +400,7 @@ func (m *Manager) Login(ctx context.Context, opts LoginOptions) (*LoginResult, e } // Validate scope early (single source of truth) - if opts.Scope != "" && opts.Scope != "read" && opts.Scope != "full" { + if opts.Scope != "" && opts.Scope != scopeRead && opts.Scope != scopeFull { return nil, output.ErrUsage("Invalid scope. Use 'read' or 'full'") } @@ -551,10 +558,21 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau } } - devOpts := []oauth.DeviceOption{oauth.WithDeviceHTTPClient(m.httpClient)} - if opts.Scope != "" { - devOpts = append(devOpts, oauth.WithDeviceScope(opts.Scope)) + // 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 } + + devOpts := make([]oauth.DeviceOption, 0, 2+len(opts.deviceOptions)) + devOpts = append(devOpts, + oauth.WithDeviceHTTPClient(m.httpClient), + oauth.WithDeviceScope(requestedScope), + ) devOpts = append(devOpts, opts.deviceOptions...) // The SDK display hook can't return an error, and the SDK proceeds into @@ -620,12 +638,11 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau return nil, err } + // The granted scope is whatever the server says it granted; fall back to + // what was asked for only when the token response omits it. effectiveScope := token.Scope if effectiveScope == "" { - effectiveScope = opts.Scope - } - if effectiveScope == "" { - effectiveScope = "read" + effectiveScope = requestedScope } creds := &Credentials{ @@ -1085,6 +1102,53 @@ func (m *Manager) GetOAuthType() string { return creds.OAuthType } +// IsReadOnly reports that the stored credentials grant read access only, so +// any write is refused before the server resolves the resource. Only BC5 +// tokens carry a scope; Launchpad tokens are read-write, so a 403 there is a +// genuine permission failure rather than a missing scope. +func (m *Manager) IsReadOnly() bool { + creds, err := m.store.Load(m.credentialKey()) + if err != nil { + return false + } + return creds.OAuthType == oauthTypeBC5 && creds.Scope == scopeRead +} + +// accountResourceURNPrefix is the RFC 8707 resource indicator BC5 binds an +// account-scoped token to. The trailing segment is the account's public ID — +// the same one that appears in Basecamp URLs. +const accountResourceURNPrefix = "urn:bc:account:" + +// AccountID returns the account a BC5 token is bound to, derived from its +// stored RFC 8707 resource indicator, or "" when the credentials carry no +// account binding (Launchpad tokens, or a resource naming the service +// origin rather than one account). +// +// The binding is authoritative: the token grants access to exactly this +// account, so there is nothing to discover over the network and no picker to +// show. It also works where account discovery cannot — /authorization.json +// is served only on the API host, which beta deployments don't route. +func (m *Manager) AccountID() string { + creds, err := m.store.Load(m.credentialKey()) + if err != nil { + return "" + } + + id := strings.TrimPrefix(creds.Resource, accountResourceURNPrefix) + if id == creds.Resource || id == "" { + return "" + } + + // Digits only: this feeds URL construction, and a resource indicator is + // server-controlled data. + for _, r := range id { + if r < '0' || r > '9' { + return "" + } + } + return id +} + // GetUserEmail returns the stored user email for the current credential key. func (m *Manager) GetUserEmail() string { credKey := m.credentialKey() diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 93be99128..ae7c218b0 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -404,7 +404,7 @@ func TestLoginDevice_ScopeWiring(t *testing.T) { assert.Equal(t, "full", result.Scope) }) - t.Run("unset scope omitted and defaults to read", func(t *testing.T) { + t.Run("unset scope requests full explicitly", func(t *testing.T) { as := startDeviceAS(t) as.token = func(int) (int, string) { return http.StatusOK, `{"access_token":"tok","refresh_token":"ref","token_type":"bearer"}` @@ -419,10 +419,34 @@ func TestLoginDevice_ScopeWiring(t *testing.T) { }) require.NoError(t, err) + // Omitting scope would let the server pick its least-privilege + // default (read), silently making every write fail — Launchpad + // logins have always been read-write. calls := as.deviceCalls() require.Len(t, calls, 1) - _, hasScope := calls[0]["scope"] - assert.False(t, hasScope, "no scope requested means no scope parameter") + assert.Equal(t, "full", calls[0].Get("scope"), "an unqualified login asks for full access") + assert.Equal(t, "full", result.Scope) + }) + + t.Run("explicit read is honored", func(t *testing.T) { + as := startDeviceAS(t) + as.token = func(int) (int, string) { + return http.StatusOK, `{"access_token":"tok","refresh_token":"ref","token_type":"bearer"}` + } + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + + result, err := m.Login(context.Background(), LoginOptions{ + Scope: "read", + Remote: true, + Logger: func(string) {}, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + + calls := as.deviceCalls() + require.Len(t, calls, 1) + assert.Equal(t, "read", calls[0].Get("scope"), "--scope read is how a caller asks for less") assert.Equal(t, "read", result.Scope) }) @@ -881,3 +905,80 @@ func TestLoginDevice_ResourceEchoEndToEnd(t *testing.T) { assert.Equal(t, "dev-ref-2", rotated.RefreshToken) assert.Equal(t, "urn:bc:account:42", rotated.Resource, "an omitted resource must preserve the stored binding") } + +// TestAccountID covers the account a BC5 token is bound to, read back from its +// RFC 8707 resource indicator. This is what lets a device login address its +// account without /authorization.json, which BC3 serves only on the API host. +func TestAccountID(t *testing.T) { + tests := []struct { + name string + creds Credentials + expected string + }{ + { + name: "account URN yields the account ID", + creds: Credentials{OAuthType: oauthTypeBC5, Resource: "urn:bc:account:2914079"}, + expected: "2914079", + }, + { + name: "no resource indicator", + creds: Credentials{OAuthType: oauthTypeBC5}, + expected: "", + }, + { + name: "a service origin names no account", + // RFC 9728 metadata publishes the serving origin; BC3 treats that + // form as "no account restriction", not as an account. + creds: Credentials{OAuthType: oauthTypeBC5, Resource: "https://3.basecampapi.com"}, + expected: "", + }, + { + name: "empty account segment", + creds: Credentials{OAuthType: oauthTypeBC5, Resource: "urn:bc:account:"}, + expected: "", + }, + { + name: "non-numeric segment is refused", + // The indicator is server-controlled and feeds URL construction. + creds: Credentials{OAuthType: oauthTypeBC5, Resource: "urn:bc:account:../../evil"}, + expected: "", + }, + { + name: "launchpad credentials carry no binding", + creds: Credentials{OAuthType: oauthTypeLaunchpad}, + expected: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + m := newDeviceTestManager(t, "https://example.com") + require.NoError(t, m.store.Save(m.credentialKey(), &tc.creds)) + assert.Equal(t, tc.expected, m.AccountID()) + }) + } +} + +// TestIsReadOnly gates the insufficient-scope explanation: only a read-scoped +// BC5 token turns a bare 403 into "re-login with --scope full". Launchpad +// tokens carry no scope, so their 403 is a real permission failure. +func TestIsReadOnly(t *testing.T) { + tests := []struct { + name string + creds Credentials + expected bool + }{ + {"read-scoped BC5", Credentials{OAuthType: oauthTypeBC5, Scope: "read"}, true}, + {"full-scoped BC5", Credentials{OAuthType: oauthTypeBC5, Scope: "full"}, false}, + {"launchpad", Credentials{OAuthType: oauthTypeLaunchpad}, false}, + {"launchpad with a stale scope", Credentials{OAuthType: oauthTypeLaunchpad, Scope: "read"}, false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + m := newDeviceTestManager(t, "https://example.com") + require.NoError(t, m.store.Save(m.credentialKey(), &tc.creds)) + assert.Equal(t, tc.expected, m.IsReadOnly()) + }) + } +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 6f974f02d..4f3564d98 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -374,6 +374,9 @@ func Execute() { // Transform Cobra errors to match Bash CLI error format err = transformCobraError(err) + // A read-scoped token's 403 carries no body to explain itself. + err = explainInsufficientScope(err, appctx.FromContext(executedCmd.Context())) + // Convert error to structured output apiErr := output.AsError(err) @@ -610,6 +613,32 @@ func isMachineConsumer(root *cobra.Command) bool { return false } +// explainInsufficientScope replaces a bare 403 with scope-specific guidance +// when the stored token is read-only. +// +// BC5 checks scope before it resolves the resource and answers with an empty +// body — the only signal is a WWW-Authenticate challenge the SDK does not +// surface — so an unqualified "access denied" is all a user would otherwise +// see for a write on a read-scoped login. A 403 that already carries a +// server hint is left alone, as is any credential that could genuinely lack +// permission (Launchpad tokens have no scope at all). +func explainInsufficientScope(err error, app *appctx.App) error { + if app == nil || app.Auth == nil || !app.Auth.IsReadOnly() { + return err + } + return scopeErrorFor(err) +} + +// scopeErrorFor rewrites an unexplained 403 as an insufficient-scope error. +// A 403 that already carries a server hint explains itself and is preserved. +func scopeErrorFor(err error) error { + apiErr := output.AsError(err) + if apiErr == nil || apiErr.HTTPStatus != 403 || apiErr.Hint != "" { + return err + } + return output.ErrForbiddenScope() +} + // transformCobraError transforms Cobra's default error messages to match the // Bash CLI format for consistency with existing tests and user expectations. func transformCobraError(err error) error { diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index 636d03c78..82d378bf5 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -12,6 +12,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/commands" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -285,3 +286,34 @@ func TestVersionWithJQReturnsUsageError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "--jq is not supported by the version command") } + +// TestScopeErrorFor covers the rewrite that turns BC5's unexplained 403 into +// actionable guidance. BC5 checks scope before it resolves the resource and +// answers with an empty body, so "access denied" is all a read-scoped write +// would otherwise report. +func TestScopeErrorFor(t *testing.T) { + t.Run("bare 403 becomes an insufficient-scope error", func(t *testing.T) { + rewritten := scopeErrorFor(output.ErrForbidden("access denied")) + + apiErr := output.AsError(rewritten) + require.NotNil(t, apiErr) + assert.Equal(t, 403, apiErr.HTTPStatus) + assert.Contains(t, apiErr.Hint, "--scope full", "the user needs the remedy, not just the refusal") + }) + + t.Run("a 403 that explains itself is preserved", func(t *testing.T) { + explained := &output.Error{ + Code: "forbidden", + Message: "Project is archived", + Hint: "Unarchive the project first", + HTTPStatus: 403, + } + + assert.Same(t, explained, scopeErrorFor(explained), "a server-supplied hint must not be replaced") + }) + + t.Run("other statuses are untouched", func(t *testing.T) { + notFound := output.ErrNotFound("project", "12345") + assert.Same(t, notFound, scopeErrorFor(notFound)) + }) +} diff --git a/internal/commands/auth.go b/internal/commands/auth.go index f2de24c6b..7656d0c04 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -297,7 +297,7 @@ func buildLoginCmd(use string) *cobra.Command { }, } - cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (ignored by Launchpad)") + cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (default full; ignored by Launchpad)") cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Don't open browser automatically") cmd.Flags().BoolVar(&remote, "remote", false, "Force remote/headless mode (paste callback URL instead of local listener)") cmd.Flags().BoolVar(&local, "local", false, "Force local mode (override SSH auto-detection)") diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 7e899b4e5..81b658652 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -337,7 +337,7 @@ Examples: } cmd.Flags().StringVar(&baseURL, "base-url", "", "Basecamp API base URL (default: https://3.basecampapi.com)") - cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (ignored by Launchpad)") + cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (default full; ignored by Launchpad)") cmd.Flags().StringVar(&accountID, "account", "", "Account ID") cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Don't open browser automatically") cmd.Flags().BoolVar(&remote, "remote", false, "Force remote/headless mode (paste callback URL instead of local listener)") diff --git a/internal/tui/resolve/account.go b/internal/tui/resolve/account.go index 92fc4da0d..330b558c5 100644 --- a/internal/tui/resolve/account.go +++ b/internal/tui/resolve/account.go @@ -13,8 +13,9 @@ import ( // Account resolves the account ID using the following precedence: // 1. CLI flag (--account) // 2. Config file (account_id) -// 3. Interactive prompt (if terminal is interactive) -// 4. Error (if no account can be determined) +// 3. The account a BC5 token is bound to (RFC 8707 resource indicator) +// 4. Interactive prompt (if terminal is interactive) +// 5. Error (if no account can be determined) // // Returns the resolved account ID and the source it came from. func (r *Resolver) Account(ctx context.Context) (*ResolvedValue, error) { @@ -34,7 +35,16 @@ func (r *Resolver) Account(ctx context.Context) (*ResolvedValue, error) { }, nil } - // 3. Try interactive prompt if available + // 3. A BC5 token is bound to one account by its RFC 8707 resource + // indicator. That binding is the answer — no fetch, no picker. + if accountID := r.auth.AccountID(); accountID != "" { + return &ResolvedValue{ + Value: accountID, + Source: SourceDefault, + }, nil + } + + // 4. Try interactive prompt if available if !r.IsInteractive() { return nil, output.ErrUsage("--account is required (or set account_id in config)") } diff --git a/internal/tui/resolve/account_test.go b/internal/tui/resolve/account_test.go index b91660363..e690e9979 100644 --- a/internal/tui/resolve/account_test.go +++ b/internal/tui/resolve/account_test.go @@ -111,3 +111,57 @@ func TestFetchAccounts_LaunchpadToken(t *testing.T) { require.Len(t, accounts, 1) assert.Equal(t, int64(200), accounts[0].ID) } + +// TestAccount_UsesResourceIndicatorBinding proves a BC5 device login can +// address its own account with no network call and no picker: the token is +// bound to exactly one account by its RFC 8707 resource indicator. This is +// also the only path that works against beta deployments, where BC3 serves +// /authorization.json only on an API host that beta does not route. +func TestAccount_UsesResourceIndicatorBinding(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "basecamp"), 0700)) + + cfg := &config.Config{BaseURL: "https://3.basecampapi.com"} + authMgr := auth.NewManager(cfg, nil) + require.NoError(t, authMgr.GetStore().Save(authMgr.CredentialKey(), &auth.Credentials{ + AccessToken: "bc_at_test", + OAuthType: "bc5", + Scope: "full", + Resource: "urn:bc:account:2914079", + })) + + // A nil SDK client is deliberate: reaching the accounts fetch would panic, + // so this also proves the binding short-circuits the network. + r := New(nil, authMgr, cfg, WithFlags(&Flags{Agent: true})) + + resolved, err := r.Account(context.Background()) + require.NoError(t, err) + assert.Equal(t, "2914079", resolved.Value) + assert.Equal(t, SourceDefault, resolved.Source) +} + +// TestAccount_FlagBeatsResourceIndicator keeps the binding subordinate to an +// explicit choice. +func TestAccount_FlagBeatsResourceIndicator(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "basecamp"), 0700)) + + cfg := &config.Config{BaseURL: "https://3.basecampapi.com"} + authMgr := auth.NewManager(cfg, nil) + require.NoError(t, authMgr.GetStore().Save(authMgr.CredentialKey(), &auth.Credentials{ + AccessToken: "bc_at_test", + OAuthType: "bc5", + Resource: "urn:bc:account:2914079", + })) + + r := New(nil, authMgr, cfg, WithFlags(&Flags{Account: "999", Agent: true})) + + resolved, err := r.Account(context.Background()) + require.NoError(t, err) + assert.Equal(t, "999", resolved.Value) + assert.Equal(t, SourceFlag, resolved.Source) +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index d7f4e93b0..44f79ac25 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1241,7 +1241,8 @@ leave the skill only and surface the per-agent `basecamp setup ` commands. ```bash basecamp auth status # Check auth basecamp auth login # Re-authenticate -basecamp auth login --scope full # Full access (ignored by Launchpad) +basecamp auth login --scope full # Full access (the default; ignored by Launchpad) +basecamp auth login --scope read # Read-only access (ignored by Launchpad) basecamp auth login --device-code # Headless authentication with manual browser instructions ``` From 9c18ee003e802f7dfe54ad1f9992299a355d56cb Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 23:49:50 -0700 Subject: [PATCH 2/3] auth: honor BASECAMP_TOKEN in the credential-derived answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/auth/auth.go | 18 ++++++++++++++++ internal/auth/device_test.go | 40 ++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 6d782559f..35057be5d 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -1106,7 +1106,16 @@ func (m *Manager) GetOAuthType() string { // any write is refused before the server resolves the resource. Only BC5 // tokens carry a scope; Launchpad tokens are read-write, so a 403 there is a // genuine permission failure rather than a missing scope. +// +// BASECAMP_TOKEN wins — match AccessToken() precedence. The request carried +// the environment token, whose scope is unknown and unrelated to whatever +// credentials happen to sit in the store, so its 403 must be reported as it +// arrived rather than blamed on a stale stored scope. func (m *Manager) IsReadOnly() bool { + if os.Getenv("BASECAMP_TOKEN") != "" { + return false + } + creds, err := m.store.Load(m.credentialKey()) if err != nil { return false @@ -1128,7 +1137,16 @@ const accountResourceURNPrefix = "urn:bc:account:" // account, so there is nothing to discover over the network and no picker to // show. It also works where account discovery cannot — /authorization.json // is served only on the API host, which beta deployments don't route. +// +// BASECAMP_TOKEN wins — match AccessToken() precedence. Requests carry the +// environment token, which is bound to whatever the operator issued it for; +// answering with a stored token's account would silently address the wrong +// one. Fall through to discovery, which asks using the token in play. func (m *Manager) AccountID() string { + if os.Getenv("BASECAMP_TOKEN") != "" { + return "" + } + creds, err := m.store.Load(m.credentialKey()) if err != nil { return "" diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index ae7c218b0..15f86cc6d 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -982,3 +982,43 @@ func TestIsReadOnly(t *testing.T) { }) } } + +// TestEnvTokenOverridesStoredCredentialAnswers pins BASECAMP_TOKEN precedence +// for the two accessors that read the credential store. Requests carry the +// environment token (AccessToken short-circuits on it), so answering from +// stored credentials would describe a token that is not being used: a stale +// BC5 binding would address the wrong account, and a stale read scope would +// relabel that token's genuine 403 as a missing scope. +func TestEnvTokenOverridesStoredCredentialAnswers(t *testing.T) { + stale := &Credentials{ + AccessToken: "stored-tok", + OAuthType: oauthTypeBC5, + Scope: scopeRead, + Resource: "urn:bc:account:2914079", + } + + t.Run("stored answers stand without an env token", func(t *testing.T) { + m := newDeviceTestManager(t, "https://example.com") + require.NoError(t, m.store.Save(m.credentialKey(), stale)) + t.Setenv("BASECAMP_TOKEN", "") // don't inherit an ambient token + + assert.Equal(t, "2914079", m.AccountID()) + assert.True(t, m.IsReadOnly()) + }) + + t.Run("env token suppresses the stored account binding", func(t *testing.T) { + m := newDeviceTestManager(t, "https://example.com") + require.NoError(t, m.store.Save(m.credentialKey(), stale)) + t.Setenv("BASECAMP_TOKEN", "bc_at_from_environment") + + assert.Empty(t, m.AccountID(), "the env token's account is not the stored one") + }) + + t.Run("env token suppresses the stored read scope", func(t *testing.T) { + m := newDeviceTestManager(t, "https://example.com") + require.NoError(t, m.store.Save(m.credentialKey(), stale)) + t.Setenv("BASECAMP_TOKEN", "bc_at_from_environment") + + assert.False(t, m.IsReadOnly(), "a 403 from the env token must not be blamed on a stored scope") + }) +} From bb050b6af045a440aeed6d0c97b145b77a2a8409 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 4 Aug 2026 00:05:29 -0700 Subject: [PATCH 3/3] auth: drop the insufficient-scope rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/auth/auth.go | 21 ---------------- internal/auth/device_test.go | 49 ++++++------------------------------ internal/cli/root.go | 29 --------------------- internal/cli/root_test.go | 32 ----------------------- 4 files changed, 7 insertions(+), 124 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 35057be5d..5d5195212 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -1102,27 +1102,6 @@ func (m *Manager) GetOAuthType() string { return creds.OAuthType } -// IsReadOnly reports that the stored credentials grant read access only, so -// any write is refused before the server resolves the resource. Only BC5 -// tokens carry a scope; Launchpad tokens are read-write, so a 403 there is a -// genuine permission failure rather than a missing scope. -// -// BASECAMP_TOKEN wins — match AccessToken() precedence. The request carried -// the environment token, whose scope is unknown and unrelated to whatever -// credentials happen to sit in the store, so its 403 must be reported as it -// arrived rather than blamed on a stale stored scope. -func (m *Manager) IsReadOnly() bool { - if os.Getenv("BASECAMP_TOKEN") != "" { - return false - } - - creds, err := m.store.Load(m.credentialKey()) - if err != nil { - return false - } - return creds.OAuthType == oauthTypeBC5 && creds.Scope == scopeRead -} - // accountResourceURNPrefix is the RFC 8707 resource indicator BC5 binds an // account-scoped token to. The trailing segment is the account's public ID — // the same one that appears in Basecamp URLs. diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 15f86cc6d..45fdbf27d 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -959,37 +959,11 @@ func TestAccountID(t *testing.T) { } } -// TestIsReadOnly gates the insufficient-scope explanation: only a read-scoped -// BC5 token turns a bare 403 into "re-login with --scope full". Launchpad -// tokens carry no scope, so their 403 is a real permission failure. -func TestIsReadOnly(t *testing.T) { - tests := []struct { - name string - creds Credentials - expected bool - }{ - {"read-scoped BC5", Credentials{OAuthType: oauthTypeBC5, Scope: "read"}, true}, - {"full-scoped BC5", Credentials{OAuthType: oauthTypeBC5, Scope: "full"}, false}, - {"launchpad", Credentials{OAuthType: oauthTypeLaunchpad}, false}, - {"launchpad with a stale scope", Credentials{OAuthType: oauthTypeLaunchpad, Scope: "read"}, false}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - m := newDeviceTestManager(t, "https://example.com") - require.NoError(t, m.store.Save(m.credentialKey(), &tc.creds)) - assert.Equal(t, tc.expected, m.IsReadOnly()) - }) - } -} - -// TestEnvTokenOverridesStoredCredentialAnswers pins BASECAMP_TOKEN precedence -// for the two accessors that read the credential store. Requests carry the -// environment token (AccessToken short-circuits on it), so answering from -// stored credentials would describe a token that is not being used: a stale -// BC5 binding would address the wrong account, and a stale read scope would -// relabel that token's genuine 403 as a missing scope. -func TestEnvTokenOverridesStoredCredentialAnswers(t *testing.T) { +// TestEnvTokenOverridesStoredAccountBinding pins BASECAMP_TOKEN precedence for +// AccountID. Requests carry the environment token (AccessToken short-circuits +// on it), so answering from a stored BC5 binding would silently address an +// account that token may have nothing to do with. +func TestEnvTokenOverridesStoredAccountBinding(t *testing.T) { stale := &Credentials{ AccessToken: "stored-tok", OAuthType: oauthTypeBC5, @@ -997,28 +971,19 @@ func TestEnvTokenOverridesStoredCredentialAnswers(t *testing.T) { Resource: "urn:bc:account:2914079", } - t.Run("stored answers stand without an env token", func(t *testing.T) { + t.Run("stored binding stands without an env token", func(t *testing.T) { m := newDeviceTestManager(t, "https://example.com") require.NoError(t, m.store.Save(m.credentialKey(), stale)) t.Setenv("BASECAMP_TOKEN", "") // don't inherit an ambient token assert.Equal(t, "2914079", m.AccountID()) - assert.True(t, m.IsReadOnly()) }) - t.Run("env token suppresses the stored account binding", func(t *testing.T) { + t.Run("env token suppresses the stored binding", func(t *testing.T) { m := newDeviceTestManager(t, "https://example.com") require.NoError(t, m.store.Save(m.credentialKey(), stale)) t.Setenv("BASECAMP_TOKEN", "bc_at_from_environment") assert.Empty(t, m.AccountID(), "the env token's account is not the stored one") }) - - t.Run("env token suppresses the stored read scope", func(t *testing.T) { - m := newDeviceTestManager(t, "https://example.com") - require.NoError(t, m.store.Save(m.credentialKey(), stale)) - t.Setenv("BASECAMP_TOKEN", "bc_at_from_environment") - - assert.False(t, m.IsReadOnly(), "a 403 from the env token must not be blamed on a stored scope") - }) } diff --git a/internal/cli/root.go b/internal/cli/root.go index 4f3564d98..6f974f02d 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -374,9 +374,6 @@ func Execute() { // Transform Cobra errors to match Bash CLI error format err = transformCobraError(err) - // A read-scoped token's 403 carries no body to explain itself. - err = explainInsufficientScope(err, appctx.FromContext(executedCmd.Context())) - // Convert error to structured output apiErr := output.AsError(err) @@ -613,32 +610,6 @@ func isMachineConsumer(root *cobra.Command) bool { return false } -// explainInsufficientScope replaces a bare 403 with scope-specific guidance -// when the stored token is read-only. -// -// BC5 checks scope before it resolves the resource and answers with an empty -// body — the only signal is a WWW-Authenticate challenge the SDK does not -// surface — so an unqualified "access denied" is all a user would otherwise -// see for a write on a read-scoped login. A 403 that already carries a -// server hint is left alone, as is any credential that could genuinely lack -// permission (Launchpad tokens have no scope at all). -func explainInsufficientScope(err error, app *appctx.App) error { - if app == nil || app.Auth == nil || !app.Auth.IsReadOnly() { - return err - } - return scopeErrorFor(err) -} - -// scopeErrorFor rewrites an unexplained 403 as an insufficient-scope error. -// A 403 that already carries a server hint explains itself and is preserved. -func scopeErrorFor(err error) error { - apiErr := output.AsError(err) - if apiErr == nil || apiErr.HTTPStatus != 403 || apiErr.Hint != "" { - return err - } - return output.ErrForbiddenScope() -} - // transformCobraError transforms Cobra's default error messages to match the // Bash CLI format for consistency with existing tests and user expectations. func transformCobraError(err error) error { diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index 82d378bf5..636d03c78 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -12,7 +12,6 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/commands" "github.com/basecamp/basecamp-cli/internal/config" - "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -286,34 +285,3 @@ func TestVersionWithJQReturnsUsageError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "--jq is not supported by the version command") } - -// TestScopeErrorFor covers the rewrite that turns BC5's unexplained 403 into -// actionable guidance. BC5 checks scope before it resolves the resource and -// answers with an empty body, so "access denied" is all a read-scoped write -// would otherwise report. -func TestScopeErrorFor(t *testing.T) { - t.Run("bare 403 becomes an insufficient-scope error", func(t *testing.T) { - rewritten := scopeErrorFor(output.ErrForbidden("access denied")) - - apiErr := output.AsError(rewritten) - require.NotNil(t, apiErr) - assert.Equal(t, 403, apiErr.HTTPStatus) - assert.Contains(t, apiErr.Hint, "--scope full", "the user needs the remedy, not just the refusal") - }) - - t.Run("a 403 that explains itself is preserved", func(t *testing.T) { - explained := &output.Error{ - Code: "forbidden", - Message: "Project is archived", - Hint: "Unarchive the project first", - HTTPStatus: 403, - } - - assert.Same(t, explained, scopeErrorFor(explained), "a server-supplied hint must not be replaced") - }) - - t.Run("other statuses are untouched", func(t *testing.T) { - notFound := output.ErrNotFound("project", "12345") - assert.Same(t, notFound, scopeErrorFor(notFound)) - }) -}