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..5d5195212 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,50 @@ func (m *Manager) GetOAuthType() string { return creds.OAuthType } +// 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. +// +// 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 "" + } + + 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..45fdbf27d 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,85 @@ 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()) + }) + } +} + +// 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, + Scope: scopeRead, + Resource: "urn:bc:account:2914079", + } + + 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()) + }) + + 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") + }) +} 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 ```