diff --git a/.surface b/.surface index cacb7f11..74489d61 100644 --- a/.surface +++ b/.surface @@ -1929,6 +1929,7 @@ FLAG basecamp auth login --agent type=bool FLAG basecamp auth login --cache-dir type=string FLAG basecamp auth login --count type=bool FLAG basecamp auth login --device-code type=bool +FLAG basecamp auth login --expect-identity type=string FLAG basecamp auth login --help type=bool FLAG basecamp auth login --hints type=bool FLAG basecamp auth login --ids-only type=bool @@ -1936,6 +1937,7 @@ FLAG basecamp auth login --in type=string FLAG basecamp auth login --jq type=string FLAG basecamp auth login --json type=bool FLAG basecamp auth login --local type=bool +FLAG basecamp auth login --login-hint type=string FLAG basecamp auth login --markdown type=bool FLAG basecamp auth login --md type=bool FLAG basecamp auth login --no-browser type=bool @@ -1950,6 +1952,7 @@ FLAG basecamp auth login --stats type=bool FLAG basecamp auth login --styled type=bool FLAG basecamp auth login --todolist type=string FLAG basecamp auth login --verbose type=count +FLAG basecamp auth login --with-token type=bool FLAG basecamp auth logout --account type=string FLAG basecamp auth logout --agent type=bool FLAG basecamp auth logout --cache-dir type=string @@ -10660,6 +10663,7 @@ FLAG basecamp login --agent type=bool FLAG basecamp login --cache-dir type=string FLAG basecamp login --count type=bool FLAG basecamp login --device-code type=bool +FLAG basecamp login --expect-identity type=string FLAG basecamp login --help type=bool FLAG basecamp login --hints type=bool FLAG basecamp login --ids-only type=bool @@ -10667,6 +10671,7 @@ FLAG basecamp login --in type=string FLAG basecamp login --jq type=string FLAG basecamp login --json type=bool FLAG basecamp login --local type=bool +FLAG basecamp login --login-hint type=string FLAG basecamp login --markdown type=bool FLAG basecamp login --md type=bool FLAG basecamp login --no-browser type=bool @@ -10681,6 +10686,7 @@ FLAG basecamp login --stats type=bool FLAG basecamp login --styled type=bool FLAG basecamp login --todolist type=string FLAG basecamp login --verbose type=count +FLAG basecamp login --with-token type=bool FLAG basecamp logout --account type=string FLAG basecamp logout --agent type=bool FLAG basecamp logout --cache-dir type=string diff --git a/README.md b/README.md index 29cb2643..7567959d 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,33 @@ basecamp auth login --scope full # Full read+write access (default; ignored by L basecamp auth token # Print token for scripts ``` +`--expect-identity ` makes any login assert who it authenticated as: the +new credential is checked before it is stored, and on a mismatch nothing is +written (a profile's previous credential is untouched) and the command exits +non-zero. `--login-hint ` names the account to sign in as on the +device-flow approval page; this build tells you the hint rather than sending +it to the server, so the approval page is not preselected. + +### Personal access tokens + +A [personal access token](https://app.basecamp.com/my/access_tokens) can be +imported instead of running OAuth — the shape for bots, CI, and any machine +that should never sign in interactively. The token is read from stdin (never +an argument), verified against the server — who it authenticates as, and that +it can reach the profile's account — and only then stored under a named +profile, with whatever expiry the server reports for it: + +```bash +op read "op://Vault/Item/credential" | basecamp auth login --with-token -P bot --account 999 +op read "op://Vault/Item/credential" | basecamp auth login --with-token -P bot --account 999 --expect-identity 12345 --json +``` + +`--account` is required when the profile does not exist yet. `--json` returns +an envelope with the profile, account, identity and person, `oauth_type`, +`scope`, and `expires_at` (the expiry the server reports for the token, or +`null` when it reports none). A token has no refresh token, so near a reported +expiry the CLI refuses it and asks for a fresh import. + ### Multiple Identities Use named profiles when the same machine or agent gateway needs more than one Basecamp identity. Each profile has its own stored OAuth credentials and can be selected per command: @@ -218,6 +245,12 @@ To use your own OAuth app (e.g., a custom Launchpad integration): Both `BASECAMP_OAUTH_CLIENT_ID` and `BASECAMP_OAUTH_CLIENT_SECRET` must be set together. +`BASECAMP_OAUTH_ISSUER=https://app.basecamp.com` pins the OAuth authorization +server and skips discovery, so `basecamp auth login` reaches a server that is +serving piloted clients but not yet advertising itself (discovery still 404s). +It is a temporary escape hatch for that dark pilot, not a configuration +surface, and will be removed once the server advertises its metadata. + ## AI Agent Integration `basecamp` works with any AI agent that can run shell commands. diff --git a/e2e/auth.bats b/e2e/auth.bats index 8c879154..69abd686 100644 --- a/e2e/auth.bats +++ b/e2e/auth.bats @@ -90,6 +90,35 @@ load test_helper assert_output_contains "default full" } +@test "basecamp auth login --help shows --with-token, --expect-identity, and --login-hint" { + run basecamp auth login --help + assert_success + assert_output_contains "--with-token" + assert_output_contains "--expect-identity" + assert_output_contains "--login-hint" + assert_output_contains "op read" +} + +@test "basecamp auth login --with-token requires a profile" { + run env -u BASECAMP_PROFILE basecamp auth login --with-token 0 and is within the expiry window. - if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-300 { + if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-int64(RefreshWindow.Seconds()) { if err := m.refreshLocked(ctx, credKey, creds); err != nil { return "", err } @@ -168,8 +179,8 @@ func (m *Manager) StoredAccessToken(ctx context.Context) (string, error) { return "", output.ErrAuth(fmt.Sprintf("No stored credentials for %s: %v", credKey, err)) } - // Check if token is expired (with 5 minute buffer) - if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-300 { + // Check if token is expired (with the refresh-window buffer) + if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-int64(RefreshWindow.Seconds()) { if err := m.refreshLocked(ctx, credKey, creds); err != nil { // Preserve the original error type (API, network, etc.) return "", err @@ -345,6 +356,19 @@ type LoginOptions struct { // Mutually exclusive with Remote. Local bool + // LoginHint names the account (email address) the user should sign in + // as. It is meant to steer the device-flow sign-in page and never + // authenticates on its own; this build announces it to the user rather + // than sending it (see announceLoginHint), and Launchpad's + // authorization-code flow ignores it. + LoginHint string + + // Verify, when set, is called with the freshly issued access token and + // its provider type ("bc5" or "launchpad") before anything is stored. A + // non-nil error aborts the login and nothing is written: the credential + // is proven first and persisted second. + Verify func(ctx context.Context, accessToken, oauthType string) error + // InputReader is the source for pasted callback URLs in remote mode. // If nil, os.Stdin is used. InputReader io.Reader @@ -478,6 +502,9 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * if opts.Scope != "" { opts.log("Launchpad does not support OAuth scopes; --scope ignored") } + if opts.LoginHint != "" { + opts.log("Launchpad does not support login hints; --login-hint ignored") + } clientCreds, err := launchpadClientCredentials(opts.log) if err != nil { @@ -572,6 +599,11 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * creds.TokenEndpoint = oauthCfg.TokenEndpoint creds.Scope = "" + if opts.Verify != nil { + if err := opts.Verify(ctx, creds.AccessToken, oauthTypeLaunchpad); err != nil { + return nil, err + } + } if err := m.store.Save(credKey, creds); err != nil { return nil, err } @@ -622,6 +654,7 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau oauth.WithDeviceScope(requestedScope), ) devOpts = append(devOpts, opts.deviceOptions...) + announceLoginHint(opts) // The SDK display hook can't return an error, and the SDK proceeds into // polling regardless. On malformed display data the callback records @@ -709,6 +742,11 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau creds.ExpiresAt = token.ExpiresAt.Unix() } + if opts.Verify != nil { + if err := opts.Verify(ctx, creds.AccessToken, oauthTypeBC5); err != nil { + return nil, err + } + } if err := m.store.Save(credKey, creds); err != nil { return nil, err } @@ -716,6 +754,20 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau return &LoginResult{OAuthType: oauthTypeBC5, Scope: effectiveScope}, nil } +// announceLoginHint tells the user which account LoginOptions.LoginHint +// named. The hint belongs on the wire as Basecamp's login_hint extension to +// the device authorization request, but the pinned basecamp-sdk has no +// option for it yet (basecamp/basecamp-sdk#841 adds WithDeviceLoginHint); +// once that bump lands this becomes an oauth.WithDeviceLoginHint entry in +// devOpts. The hint is flag input headed for a terminal, so it is reduced +// to one line first. +func announceLoginHint(opts *LoginOptions) { + if opts.LoginHint == "" { + return + } + opts.log("Sign in as " + richtext.SanitizeSingleLine(opts.LoginHint) + " when the browser asks (this build cannot pass --login-hint to the server yet).") +} + // validVerificationURL validates a server-supplied verification URI with the // same policy as other OAuth browser URLs (https, or http on loopback, no // userinfo). Returns the raw URL when valid, "" otherwise. @@ -730,6 +782,35 @@ func validVerificationURL(raw string) string { return raw } +// ImportToken stores an externally issued BC5 access token (a personal +// access token) as the current credential, in one write, together with the +// identity it was verified to authenticate as and the expiry the server +// reported for it (zero when it reported none). The token has no refresh +// token and no token endpoint: a zero expiry is the non-expiring path +// AccessToken already takes, and a reported one makes AccessToken refuse +// the token near expiry with "No refresh token available" rather than +// letting requests start failing — either way the remedy is to import +// again. Scope is what the token was verified or declared to carry. +func (m *Manager) ImportToken(token, scope, userID, userEmail string, expiresAt time.Time) error { + if scope != scopeRead && scope != scopeFull { + return output.ErrUsage("Invalid scope. Use 'read' or 'full'") + } + creds := &Credentials{ + AccessToken: token, + OAuthType: oauthTypeBC5, + Scope: scope, + UserID: userID, + UserEmail: userEmail, + Source: CredentialSourceToken, + } + if !expiresAt.IsZero() { + creds.ExpiresAt = expiresAt.Unix() + } + m.mu.Lock() + defer m.mu.Unlock() + return m.store.Save(m.credentialKey(), creds) +} + // Logout removes stored credentials. func (m *Manager) Logout() error { credKey := m.credentialKey() @@ -749,6 +830,10 @@ type discovery struct { // outcomes fall back to Launchpad; once a BC5 issuer is selected, every // failure is returned loudly — never converted into a Launchpad attempt. func (m *Manager) discoverOAuth(ctx context.Context, log func(string)) (*discovery, error) { + if issuer := os.Getenv(oauthIssuerEnv); issuer != "" { + return pinnedIssuerDiscovery(issuer, log) + } + origin, err := resourceOrigin(m.cfg.BaseURL) if err != nil { return nil, err @@ -795,6 +880,39 @@ func (m *Manager) discoverOAuth(ctx context.Context, log func(string)) (*discove return &discovery{config: res.Config, oauthType: oauthTypeBC5, issuer: res.Issuer}, nil } +// oauthIssuerEnv pins the BC5 authorization server, bypassing discovery. +// +// It exists for the production dark pilot: a server whose OAuth surface is +// dark answers discovery with 404 while still serving piloted clients, so the +// only way to reach it is to name it. It is not a configuration surface and +// is removed once the server advertises itself. +const oauthIssuerEnv = "BASECAMP_OAUTH_ISSUER" + +// pinnedIssuerDiscovery builds the BC5 device-flow config from a pinned +// issuer origin, deriving the endpoints Basecamp mounts under /oauth. The +// issuer is operator-supplied environment, so it passes the same endpoint +// checks a discovery document would, and the derived endpoints are checked +// again by loginDevice before any POST. A rejected value is never echoed +// (like a base URL, it can carry userinfo or a query string); the accepted +// one is announced, reduced to one line, so the operator sees what was +// pinned — as the discovered issuer is. +func pinnedIssuerDiscovery(issuer string, log func(string)) (*discovery, error) { + issuer = strings.TrimRight(strings.TrimSpace(issuer), "/") + u, err := url.Parse(issuer) + if err != nil || u.Opaque != "" || !isSecureEndpointURL(u) || u.Path != "" || u.RawQuery != "" || u.ForceQuery || u.Fragment != "" { + return nil, output.ErrAuth("invalid " + oauthIssuerEnv + ": must be an origin — an absolute https URL (or http on loopback) with a hostname and no path, userinfo, query, or fragment") + } + deviceEndpoint := issuer + "/oauth/device_authorizations" + cfg := &oauth.Config{ + Issuer: issuer, + TokenEndpoint: issuer + "/oauth/tokens", + DeviceAuthorizationEndpoint: &deviceEndpoint, + GrantTypesSupported: []string{oauth.DeviceCodeGrantType, "refresh_token"}, + } + log(fmt.Sprintf("Authenticating via %s (device flow, pinned by %s)", richtext.SanitizeSingleLine(issuer), oauthIssuerEnv)) + return &discovery{config: cfg, oauthType: oauthTypeBC5, issuer: issuer}, nil +} + // resourceOrigin reduces the configured base URL to a bare scheme://host[:port] // origin for RFC 9728 protected-resource discovery, CANONICALIZED: lowercase // scheme and host, explicit default ports stripped. The SDK binds the @@ -1131,7 +1249,12 @@ func (m *Manager) AuthorizationEndpoint(ctx context.Context) (string, error) { return strings.TrimSuffix(lpURL, "/") + "/authorization.json", nil } - oauthType := m.GetOAuthType() + return m.AuthorizationEndpointFor(m.GetOAuthType()) +} + +// AuthorizationEndpointFor returns the authorization info endpoint for a +// credential of the given OAuth type, whether or not it is stored yet. +func (m *Manager) AuthorizationEndpointFor(oauthType string) (string, error) { switch oauthType { case "bc3", oauthTypeBC5: // resourceOrigin, not NormalizeBaseURL: the latter only trims a diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index fb894c8f..6dce39a6 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1715,3 +1715,99 @@ func TestAuthorizationEndpoint_LaunchpadTokenOverridesStoredBC3(t *testing.T) { assert.Equal(t, "https://launchpad.37signals.com/authorization.json", ep, "non-bc_at_ env token must route to launchpad, not stored bc3") } + +// TestLoginLaunchpadIgnoresLoginHint: the authorization-code flow has no +// login_hint; the option is acknowledged as ignored, not silently dropped. +func TestLoginLaunchpadIgnoresLoginHint(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.NotFound(w, r) + })) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + t.Setenv("BASECAMP_LAUNCHPAD_URL", srv.URL) + t.Setenv("BASECAMP_OAUTH_ISSUER", "") + + cfg := &config.Config{BaseURL: srv.URL} + m := NewManager(cfg, srv.Client()) + m.store = newTestStore(t, tmpDir) + + var logs []string + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + LoginHint: "bot@example.com", + Logger: func(msg string) { logs = append(logs, msg) }, + InputReader: strings.NewReader(""), + }) + require.Error(t, err, "EOF on the paste prompt aborts the login") + assert.Contains(t, strings.Join(logs, "\n"), "--login-hint ignored") +} + +// TestLoginLaunchpadVerifyRunsBeforeStore: the authorization-code flow +// honors the same pre-store hook as the device flow. +func TestLoginLaunchpadVerifyRunsBeforeStore(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/authorization/token": + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"remote-tok","token_type":"bearer","refresh_token":"remote-refresh"}`) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + t.Setenv("BASECAMP_LAUNCHPAD_URL", srv.URL) + t.Setenv("BASECAMP_OAUTH_ISSUER", "") + + cfg := &config.Config{BaseURL: srv.URL} + m := NewManager(cfg, srv.Client()) + m.store = newTestStore(t, tmpDir) + credKey := config.NormalizeBaseURL(srv.URL) + + sl := newSyncLogger() + pr, pw := io.Pipe() + defer pr.Close() + + var seenToken, seenType string + errCh := make(chan error, 1) + go func() { + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + Logger: sl.log, + InputReader: pr, + Verify: func(_ context.Context, token, oauthType string) error { + seenToken, seenType = token, oauthType + return output.ErrAuth("not you") + }, + }) + errCh <- err + }() + + var authURL string + select { + case authURL = <-sl.authReady: + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for auth URL to be logged") + } + u, err := url.Parse(authURL) + require.NoError(t, err) + _, err = fmt.Fprintf(pw, "http://127.0.0.1:8976/callback?code=test-code&state=%s\n", u.Query().Get("state")) + require.NoError(t, err) + pw.Close() + + select { + case err := <-errCh: + require.Error(t, err) + assert.Contains(t, err.Error(), "not you") + case <-time.After(5 * time.Second): + t.Fatal("Login timed out") + } + assert.Equal(t, "remote-tok", seenToken) + assert.Equal(t, "launchpad", seenType) + _, loadErr := m.store.Load(credKey) + assert.Error(t, loadErr, "a rejected token is never stored") +} diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 45fdbf27..7f9e7915 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -48,7 +48,7 @@ func startDeviceAS(t *testing.T) *deviceAS { w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, as.metadata()) }) - mux.HandleFunc("/oauth/device", func(w http.ResponseWriter, r *http.Request) { + deviceHandler := func(w http.ResponseWriter, r *http.Request) { require.NoError(t, r.ParseForm()) as.mu.Lock() as.deviceForms = append(as.deviceForms, r.PostForm) @@ -57,8 +57,8 @@ func startDeviceAS(t *testing.T) *deviceAS { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) fmt.Fprint(w, body) - }) - mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) { + } + tokenHandler := func(w http.ResponseWriter, r *http.Request) { require.NoError(t, r.ParseForm()) as.mu.Lock() call := len(as.tokenForms) @@ -68,7 +68,13 @@ func startDeviceAS(t *testing.T) *deviceAS { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) fmt.Fprint(w, body) - }) + } + mux.HandleFunc("/oauth/device", deviceHandler) + mux.HandleFunc("/oauth/token", tokenHandler) + // The paths Basecamp mounts, which a pinned BASECAMP_OAUTH_ISSUER derives + // without reading metadata. + mux.HandleFunc("/oauth/device_authorizations", deviceHandler) + mux.HandleFunc("/oauth/tokens", tokenHandler) as.srv = httptest.NewServer(mux) t.Cleanup(as.srv.Close) @@ -144,6 +150,9 @@ func newDeviceTestManager(t *testing.T, baseURL string) *Manager { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + // A pinned issuer in the developer's environment would bypass the + // discovery every test here exercises. + t.Setenv("BASECAMP_OAUTH_ISSUER", "") cfg := &config.Config{BaseURL: baseURL} m := NewManager(cfg, http.DefaultClient) m.store = newTestStore(t, tmpDir) @@ -987,3 +996,208 @@ func TestEnvTokenOverridesStoredAccountBinding(t *testing.T) { assert.Empty(t, m.AccountID(), "the env token's account is not the stored one") }) } + +// TestDiscoverOAuth_PinnedIssuerSkipsDiscovery: BASECAMP_OAUTH_ISSUER names +// the authorization server outright. No discovery request is made — the +// resource here 404s everything and counts — and the device flow runs +// against the endpoints Basecamp mounts under the issuer. +func TestDiscoverOAuth_PinnedIssuerSkipsDiscovery(t *testing.T) { + as := startDeviceAS(t) + resource, discoveryHits := countingServer(t) + m := newDeviceTestManager(t, resource.URL) + t.Setenv("BASECAMP_OAUTH_ISSUER", as.srv.URL+"/") + + cl := &collectLogger{} + result, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + Logger: cl.log, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Equal(t, &LoginResult{OAuthType: "bc5", Scope: "read"}, result) + + assert.Zero(t, *discoveryHits, "a pinned issuer must not consult resource metadata") + require.Len(t, as.deviceCalls(), 1) + assert.Equal(t, "basecamp-cli", as.deviceCalls()[0].Get("client_id")) + assert.NotEmpty(t, as.tokenCalls()) + + logs := cl.joined() + assert.Contains(t, logs, "pinned by BASECAMP_OAUTH_ISSUER") + assert.Contains(t, logs, "Authenticating via "+as.srv.URL+" (device flow") + + // The trailing slash is trimmed: the stored token endpoint is the exact + // mount, which refresh will POST to later. + creds, err := m.store.Load(config.NormalizeBaseURL(resource.URL)) + require.NoError(t, err) + assert.Equal(t, as.srv.URL+"/oauth/tokens", creds.TokenEndpoint) + assert.Equal(t, "bc5", creds.OAuthType) +} + +// TestDiscoverOAuth_PinnedIssuerIsCheckedLikeAnEndpoint: the override is +// operator environment, but it names where credentials get POSTed, so it +// passes the same checks a discovery document would — and never falls back +// to Launchpad. +func TestDiscoverOAuth_PinnedIssuerIsCheckedLikeAnEndpoint(t *testing.T) { + for name, issuer := range map[string]string{ + "plain http off loopback": "http://as.example", + "userinfo": "https://user@as.example", + "no host": "https://", + "bad port": "https://as.example:70000", + "file scheme": "file:///etc/passwd", + "query string": "https://as.example/?token=hunter2", + "fragment": "https://as.example/#frag", + "path": "https://as.example/some/path", + "opaque": "https:as.example", + } { + t.Run(name, func(t *testing.T) { + resource, discoveryHits := countingServer(t) + m := newDeviceTestManager(t, resource.URL) + t.Setenv("BASECAMP_OAUTH_ISSUER", issuer) + + _, err := m.Login(context.Background(), LoginOptions{Remote: true, Logger: func(string) {}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_OAUTH_ISSUER") + assert.NotContains(t, err.Error(), "as.example", "the rejected value is never echoed") + assert.NotContains(t, err.Error(), "hunter2") + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) + assert.Zero(t, *discoveryHits, "a rejected override must not fall back to discovery or Launchpad") + }) + } +} + +// TestLoginDevice_LoginHintAnnounced: until the SDK bump that carries +// login_hint on the wire, the hint is told to the user rather than sent. +func TestLoginDevice_LoginHintAnnounced(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + LoginHint: "bot@example.com", + Logger: cl.log, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Contains(t, cl.joined(), "Sign in as bot@example.com") + require.Len(t, as.deviceCalls(), 1) +} + +func TestImportToken(t *testing.T) { + m := newDeviceTestManager(t, "https://3.basecampapi.com") + m.cfg.ActiveProfile = "bot" + + require.NoError(t, m.ImportToken("bc_at_secret", "full", "51177542", "bot@example.com", time.Time{})) + + creds, err := m.store.Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, &Credentials{AccessToken: "bc_at_secret", OAuthType: "bc5", Scope: "full", UserID: "51177542", UserEmail: "bot@example.com", Source: "token"}, creds) + assert.Zero(t, creds.ExpiresAt) + + // Non-expiring: served as-is, with no refresh attempted (there is no + // refresh token or endpoint to attempt one with, and no HTTP client that + // could reach one). + tok, err := m.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "bc_at_secret", tok) + assert.True(t, m.IsAuthenticated()) + assert.Equal(t, "bc5", m.GetOAuthType()) + + err = m.Refresh(context.Background()) + require.Error(t, err, "an explicit refresh has nothing to refresh with") + assert.Contains(t, err.Error(), "No refresh token") + + err = m.ImportToken("bc_at_secret", "admin", "", "", time.Time{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "Invalid scope") + + // A reported expiry is kept, and near it the token is refused rather + // than served: there is no refresh token to renew it with. + require.NoError(t, m.ImportToken("bc_at_short", "full", "", "", time.Now().Add(time.Minute))) + creds, err = m.store.Load("profile:bot") + require.NoError(t, err) + assert.Positive(t, creds.ExpiresAt) + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "No refresh token") +} + +// TestLoginDevice_VerifyRunsBeforeStore: a Verify hook sees the freshly +// issued token before anything is written, and its error aborts the login +// with nothing stored. +func TestLoginDevice_VerifyRunsBeforeStore(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + credKey := config.NormalizeBaseURL(resource.URL) + + var seenToken, seenType string + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + Logger: func(string) {}, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + Verify: func(_ context.Context, token, oauthType string) error { + seenToken, seenType = token, oauthType + _, loadErr := m.store.Load(credKey) + assert.Error(t, loadErr, "the store must still be empty while verifying") + return output.ErrAuth("not you") + }, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "not you") + assert.Equal(t, "dev-tok", seenToken) + assert.Equal(t, "bc5", seenType) + _, loadErr := m.store.Load(credKey) + assert.Error(t, loadErr, "a rejected token is never stored") + + result, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + Logger: func(string) {}, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + Verify: func(context.Context, string, string) error { return nil }, + }) + require.NoError(t, err) + assert.Equal(t, "bc5", result.OAuthType) + creds, err := m.store.Load(credKey) + require.NoError(t, err) + assert.Equal(t, "dev-tok", creds.AccessToken) +} + +func TestLoginDevice_LoginHintIsSanitizedForTheTerminal(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + LoginHint: "bot@example.com\x1b]8;;https://evil.example\x07\r\nEvil", + Logger: cl.log, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + logs := cl.joined() + assert.Contains(t, logs, "Sign in as bot@example.com Evil") + assert.NotContains(t, logs, "\x1b") + assert.NotContains(t, logs, "evil.example") +} + +func TestDiscoverOAuth_PinnedIssuerIsSanitizedForTheTerminal(t *testing.T) { + resource, _ := countingServer(t) + m := newDeviceTestManager(t, resource.URL) + // Only an origin is accepted now, so a control character can only ride + // in the host — where url.Parse or the endpoint check refuses it. Either + // way the terminal never sees it: refused values are not echoed, and an + // accepted one is announced through the sanitizer. + t.Setenv("BASECAMP_OAUTH_ISSUER", "http://127.0.0.1\u0085:3001") + + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{Remote: true, Logger: cl.log, deviceOptions: []oauth.DeviceOption{instantSleep()}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_OAUTH_ISSUER") + assert.NotContains(t, err.Error(), "\u0085") + assert.NotContains(t, cl.joined(), "\u0085") +} diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 8fb20fcc..19c88a4f 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -22,6 +22,11 @@ type Credentials struct { UserID string `json:"user_id,omitempty"` UserEmail string `json:"user_email,omitempty"` + // Source records how the credential was obtained when that is not the + // OAuth flow: "token" for an imported personal access token, which has + // no refresh material. Empty means an OAuth login. + Source string `json:"source,omitempty"` + // Resource is the RFC 8707 resource indicator the tokens are bound to // (BC5: urn:bc:account:). BC5 device logins as the trusted // basecamp-cli client mint MULTI-ACCOUNT refresh tokens, and the refresh diff --git a/internal/cli/root.go b/internal/cli/root.go index fb95735c..8badce0f 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -112,7 +112,8 @@ func NewRootCmd() *cobra.Command { } // Resolve profile - profileName, err := resolveProfile(cfg, flags) + mayCreate := cmd.Annotations[commands.AnnotationProfileMayCreate] != "" + profileName, err := resolveProfile(cfg, flags, mayCreate) if err != nil { if bareRoot { initBareRootApp(cfg) @@ -120,7 +121,14 @@ func NewRootCmd() *cobra.Command { } return err } - if profileName != "" { + _, profileKnown := cfg.Profiles[profileName] + if profileName != "" && !profileKnown { + // A may-create command named a profile that does not exist + // yet: it becomes the active credential key with the + // top-level configuration, and the command registers it. + cfg.ActiveProfile = profileName + } + if profileKnown { if err := cfg.ApplyProfile(profileName); err != nil { return err } @@ -138,10 +146,12 @@ func NewRootCmd() *cobra.Command { Todolist: flags.Todolist, CacheDir: flags.CacheDir, }) - // Profile-scoped cache (only if cache dir was not explicitly set via flag or env) - if flags.CacheDir == "" && os.Getenv("BASECAMP_CACHE_DIR") == "" { - cfg.CacheDir = filepath.Join(cfg.CacheDir, "profiles", profileName) - } + } + // Profile-scoped cache (only if cache dir was not explicitly set + // via flag or env). A not-yet-registered name is unvalidated + // input, so it does not become a path component. + if profileKnown && flags.CacheDir == "" && os.Getenv("BASECAMP_CACHE_DIR") == "" { + cfg.CacheDir = filepath.Join(cfg.CacheDir, "profiles", profileName) } // Enforce HTTPS for non-localhost base_url. @@ -545,27 +555,31 @@ func jqRenderErrorDiagnostic(err error) string { // 4. Single profile → auto-use // 5. Multiple profiles → interactive picker (if TTY) // 6. No profiles → empty string (use top-level config values) -func resolveProfile(cfg *config.Config, flags appctx.GlobalFlags) (string, error) { +// +// An explicitly named profile (1, 2) must exist unless mayCreate: a command +// annotated AnnotationProfileMayCreate registers the profile itself, so the +// name passes through for it to act on. +func resolveProfile(cfg *config.Config, flags appctx.GlobalFlags, mayCreate bool) (string, error) { // 1. --profile flag if flags.Profile != "" { + if _, ok := cfg.Profiles[flags.Profile]; ok || mayCreate { + return flags.Profile, nil + } if len(cfg.Profiles) == 0 { return "", fmt.Errorf("profile %q specified via --profile but no profiles are configured; create one with: basecamp profile create", flags.Profile) } - if _, ok := cfg.Profiles[flags.Profile]; !ok { - return "", fmt.Errorf("unknown profile %q (available: %s)", flags.Profile, profileNames(cfg)) - } - return flags.Profile, nil + return "", fmt.Errorf("unknown profile %q (available: %s)", flags.Profile, profileNames(cfg)) } // 2. BASECAMP_PROFILE env var if profile := os.Getenv("BASECAMP_PROFILE"); profile != "" { + if _, ok := cfg.Profiles[profile]; ok || mayCreate { + return profile, nil + } if len(cfg.Profiles) == 0 { return "", fmt.Errorf("profile %q specified via BASECAMP_PROFILE but no profiles are configured; create one with: basecamp profile create", profile) } - if _, ok := cfg.Profiles[profile]; !ok { - return "", fmt.Errorf("unknown profile %q from BASECAMP_PROFILE (available: %s)", profile, profileNames(cfg)) - } - return profile, nil + return "", fmt.Errorf("unknown profile %q from BASECAMP_PROFILE (available: %s)", profile, profileNames(cfg)) } // No profiles configured - use top-level config diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index a0d34379..48304f57 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -408,3 +408,67 @@ func TestReportWireError(t *testing.T) { reportWireError(&buf, output.ErrAPI(502, "bad\x1b[31mgateway\r\ninjected")) assert.Equal(t, "Error: badgateway injected\n", buf.String()) } + +// TestResolveProfileMayCreatePassesUnknownName: a command that registers the +// named profile itself (auth login --with-token) receives the name; every +// other command is still refused an unknown --profile / BASECAMP_PROFILE. +func TestResolveProfileMayCreatePassesUnknownName(t *testing.T) { + t.Setenv("BASECAMP_PROFILE", "") + cfg := &config.Config{Profiles: map[string]*config.ProfileConfig{"human": {}}} + + name, err := resolveProfile(cfg, appctx.GlobalFlags{Profile: "bot"}, true) + require.NoError(t, err) + assert.Equal(t, "bot", name) + + _, err = resolveProfile(cfg, appctx.GlobalFlags{Profile: "bot"}, false) + require.Error(t, err) + assert.Contains(t, err.Error(), `unknown profile "bot"`) + + name, err = resolveProfile(cfg, appctx.GlobalFlags{Profile: "human"}, false) + require.NoError(t, err) + assert.Equal(t, "human", name) + + // With no profiles configured at all, the may-create name still passes. + empty := &config.Config{} + name, err = resolveProfile(empty, appctx.GlobalFlags{Profile: "bot"}, true) + require.NoError(t, err) + assert.Equal(t, "bot", name) + _, err = resolveProfile(empty, appctx.GlobalFlags{Profile: "bot"}, false) + require.Error(t, err) + + t.Setenv("BASECAMP_PROFILE", "bot") + name, err = resolveProfile(cfg, appctx.GlobalFlags{}, true) + require.NoError(t, err) + assert.Equal(t, "bot", name) + _, err = resolveProfile(cfg, appctx.GlobalFlags{}, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_PROFILE") +} + +// TestUnknownProfileNameIsNotACachePath: a may-create command can name a +// profile that does not exist yet; that unvalidated name must not become a +// cache directory component before the command has vetted it. +func TestUnknownProfileNameIsNotACachePath(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_PROFILE", "") + t.Setenv("BASECAMP_CACHE_DIR", "") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("HOME", t.TempDir()) + + var cacheDir string + root := NewRootCmd() + root.AddCommand(&cobra.Command{ + Use: "probe", + Annotations: map[string]string{commands.AnnotationProfileMayCreate: "true"}, + RunE: func(cmd *cobra.Command, args []string) error { + cacheDir = appctx.FromContext(cmd.Context()).Config.CacheDir + return nil + }, + }) + root.SetArgs([]string{"probe", "--profile", "../../outside"}) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + require.NoError(t, root.Execute()) + assert.NotContains(t, cacheDir, "outside") +} diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 7656d0c0..2c9c84ae 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -2,19 +2,26 @@ package commands import ( + "context" "fmt" "io" "os" "sort" + "strconv" "strings" "time" + "unicode" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/spf13/cobra" "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/harness" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" ) @@ -93,9 +100,13 @@ func newAuthStatusCmd() *cobra.Command { effectiveScope = "" } + source := "oauth" + if creds.Source != "" { + source = creds.Source + } status := map[string]any{ "authenticated": true, - "source": "oauth", + "source": source, "oauth_type": creds.OAuthType, } if effectiveScope != "" { @@ -223,6 +234,16 @@ func NewLogoutCmd() *cobra.Command { return buildLogoutCmd("logout") } +// AnnotationProfileMayCreate marks a command that registers the profile +// named by --profile / BASECAMP_PROFILE itself. The root pre-run lets an +// unknown name through for such a command instead of rejecting it, leaving +// the top-level configuration in place under that credential key. +const AnnotationProfileMayCreate = "profile_may_create" + +// maxTokenBytes bounds what --with-token reads from stdin: an access token +// is a few hundred bytes, so anything larger is not one. +const maxTokenBytes = 4096 + // buildLoginCmd constructs a login command with the given Use name. // Shared by newAuthLoginCmd ("login" under auth) and NewLoginCmd (top-level). func buildLoginCmd(use string) *cobra.Command { @@ -231,20 +252,67 @@ func buildLoginCmd(use string) *cobra.Command { var remote bool var local bool var deviceCode bool + var withToken bool + var expectIdentity string + var loginHint string cmd := &cobra.Command{ Use: use, Short: "Authenticate with Basecamp", - Long: "Start the OAuth flow to authenticate with Basecamp.", + Long: `Start the OAuth flow to authenticate with Basecamp, or import a personal access token. + +Examples: + basecamp auth login # Browser (or device) flow + basecamp auth login --device-code # Headless: approve the printed code elsewhere + basecamp auth login --expect-identity 12345 # Refuse the login unless it is this identity + +Import a personal access token from stdin (never pass it as an argument): + op read "op:////credential" | basecamp auth login --with-token -P bot --account 999 + ... | basecamp auth login --with-token -P bot --account 999 --expect-identity 12345 --json + +--with-token verifies the token against the server — who it authenticates as, +and that it can reach the profile's account — before storing it under the +named profile, creating the profile when --account is given. + +--login-hint names the account to sign in as on the device-flow approval page. +This build tells you the hint instead of sending it to the server.`, + Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, + // No positional arguments, so a token pasted after --with-token is + // refused rather than silently ignored while it sits in shell history. + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if app == nil { return fmt.Errorf("app not initialized") } + expect, err := parseExpectIdentity(expectIdentity) + if err != nil { + return err + } + + if withToken { + return runLoginWithToken(cmd, app, scope, expect) + } + if app.Flags.JQFilter != "" { return output.ErrJQNotSupported("the login command") } + if machineOutputFlagSet(app) { + return output.ErrUsageHint("Interactive login cannot run under a machine output mode", + "Browser and device logins print instructions and wait for approval, which no envelope can carry. "+ + "Check credentials with `basecamp auth status`, or import a token headlessly: "+ + "`... | basecamp auth login --with-token -P --account --json`.") + } + if expect != 0 && os.Getenv("BASECAMP_TOKEN") != "" { + return errEnvTokenShadows("--expect-identity cannot be checked while BASECAMP_TOKEN is set") + } + if name := app.Config.ActiveProfile; name != "" { + if _, ok := app.Config.Profiles[name]; !ok { + return output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), + fmt.Sprintf("Create it with `basecamp profile create %s`, or import a token: `... | basecamp auth login --with-token -P %s --account `.", name, name)) + } + } if deviceCode { remote = true @@ -259,12 +327,18 @@ func buildLoginCmd(use string) *cobra.Command { fmt.Fprintln(w, r.Summary.Render("Starting Basecamp authentication...")) } + // With an expectation the login is assertive: the token is + // checked before it is stored, and a mismatch stores nothing. + // Without one the identity line stays informational. + verifier := &loginVerifier{app: app, expectIdentity: expect, account: app.Config.AccountID, strict: expect != 0} result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, Remote: remote, Local: local, + LoginHint: loginHint, Logger: func(msg string) { fmt.Fprintln(w, msg) }, + Verify: verifier.verify, }) if err != nil { return err @@ -277,18 +351,11 @@ func buildLoginCmd(use string) *cobra.Command { fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Access: %s", result.Scope))) } - resp, profileErr := app.SDK.Get(cmd.Context(), "/my/profile.json") - if profileErr == nil { - var profile struct { - ID int `json:"id"` - Name string `json:"name"` - Email string `json:"email_address"` - } - if err := resp.UnmarshalData(&profile); err == nil { - if err := app.Auth.SetUserIdentity(fmt.Sprintf("%d", profile.ID), profile.Email); err == nil { - fmt.Fprintln(w, r.Data.Render(fmt.Sprintf("Logged in as: %s", profile.Name))) - } + if who := verifier.who; who != nil { + if who.PersonID != 0 { + _ = app.Auth.SetUserIdentity(strconv.FormatInt(who.PersonID, 10), who.Email) } + fmt.Fprintln(w, r.Data.Render("Logged in as: "+who.label())) } printAgentNudge(w, r) @@ -302,12 +369,413 @@ func buildLoginCmd(use string) *cobra.Command { 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)") cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Headless authentication with manual browser instructions") + cmd.Flags().BoolVar(&withToken, "with-token", false, "Read a personal access token from stdin instead of running OAuth (requires --profile)") + cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise store nothing") + cmd.Flags().StringVar(&loginHint, "login-hint", "", "Email address to sign in as (device flow only; announced, not yet sent; ignored by Launchpad)") cmd.MarkFlagsMutuallyExclusive("remote", "local") cmd.MarkFlagsMutuallyExclusive("device-code", "local") + for _, flag := range []string{"device-code", "remote", "local", "no-browser", "login-hint"} { + cmd.MarkFlagsMutuallyExclusive("with-token", flag) + } return cmd } +// runLoginWithToken imports a personal access token from stdin as the +// active profile's credential. Everything that can be checked without the +// token is checked before stdin is read; the token is then verified through +// a client of its own — identity, and access to the profile's account — +// and only a token that passes is stored and its profile registered. +func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect int64) error { + if scope == "" { + scope = "full" + } + if scope != "read" && scope != "full" { + return output.ErrUsage("Invalid scope. Use 'read' or 'full'") + } + if os.Getenv("BASECAMP_TOKEN") != "" { + return errEnvTokenShadows("BASECAMP_TOKEN is set") + } + + name := app.Config.ActiveProfile + if name == "" { + return output.ErrUsageHint("--with-token stores the token under a named profile", + "Pass -P/--profile ; add --account when the profile does not exist yet.") + } + if !isValidProfileName(name) { + return output.ErrUsage(fmt.Sprintf("Invalid profile name %q: use only letters, numbers, hyphens, and underscores", name)) + } + + // The effective account and base URL are what every later command will + // address under this profile, so they are what the token must be + // verified for — and they must be the profile's own. A --account / + // BASECAMP_ACCOUNT_ID / BASECAMP_BASE_URL override that disagrees with + // the binding would verify the token for one place and store it for + // another. + account := app.Config.AccountID + existing := app.Config.Profiles[name] + globalUnbound := false + if existing != nil && existing.AccountID == "" { + unbound, err := globalProfileIsUnbound(name) + if err != nil { + return err + } + globalUnbound = unbound + } + var created *config.ProfileConfig + bindAccount := false + switch { + case existing == nil && !accountGivenExplicitly(app): + return output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), + "Pass --account to create it alongside the imported token.") + case existing == nil: + created = &config.ProfileConfig{BaseURL: app.Config.BaseURL, AccountID: account} + case existing.BaseURL != "" && config.NormalizeBaseURL(existing.BaseURL) != config.NormalizeBaseURL(app.Config.BaseURL): + return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to %s, not %s", name, existing.BaseURL, app.Config.BaseURL), + "Import into a different profile, or drop the BASECAMP_BASE_URL override.") + case existing.AccountID == "" && !accountGivenExplicitly(app): + return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account", name), + "Pass --account to bind it alongside the imported token.") + case existing.AccountID == "" && !globalUnbound: + // Binding rewrites the global config file. The effective profile + // is accountless, so if the global entry is missing or already + // carries an account, the accountless one came from a system, repo + // or local config and would keep shadowing whatever is written. + return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account and is not the global config's entry", name), + "Add account_id to the config file that defines it, then rerun the import.") + case existing.AccountID == "": + bindAccount = true + case !accountIDsEqual(account, existing.AccountID): + return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to account %s, not %s", name, existing.AccountID, account), + "Import into a different profile, or drop the --account / BASECAMP_ACCOUNT_ID override.") + } + if err := requireNumericAccount(account); err != nil { + return err + } + + token, err := readTokenFromStdin(cmd) + if err != nil { + return err + } + + verifier := &loginVerifier{app: app, expectIdentity: expect, account: account, strict: true, noRefresh: true} + if err := verifier.verify(cmd.Context(), token, "bc5"); err != nil { + return err + } + who := verifier.who + // The server's word on the token's scope beats the caller's declaration. + if who.Scope != "" { + scope = who.Scope + } + + // The profile entry goes first: an entry without a credential is a + // visible, harmless state (profile list shows it unauthenticated), where + // a stored secret without an entry would be an orphan. + isDefault := app.Config.DefaultProfile == name + switch { + case created != nil: + created.Scope = scope + if isDefault, err = registerProfile(name, created); err != nil { + return err + } + if app.Config.Profiles == nil { + app.Config.Profiles = make(map[string]*config.ProfileConfig) + } + app.Config.Profiles[name] = created + case bindAccount: + if err := bindProfileAccount(name, account); err != nil { + return err + } + existing.AccountID = account + } + + if err := app.Auth.ImportToken(token, scope, strconv.FormatInt(who.PersonID, 10), who.Email, who.ExpiresAt); err != nil { + return fmt.Errorf("profile %q is registered but the token could not be stored (rerun the import): %w", name, err) + } + + var expiresAt any + tokenLine := "personal access token (does not expire)" + if !who.ExpiresAt.IsZero() { + expiresAt = who.ExpiresAt.UTC().Format(time.RFC3339) + tokenLine = "expires " + who.ExpiresAt.Local().Format("2006-01-02") + } + + data := map[string]any{ + "profile": name, + "account_id": account, + "base_url": app.Config.BaseURL, + "source": "token", + "oauth_type": "bc5", + "scope": scope, + "expires_at": expiresAt, + "identity": map[string]any{"id": who.IdentityID, "email": who.IdentityEmail}, + "person": map[string]any{"id": who.PersonID, "name": who.Name, "email": who.Email}, + "profile_created": created != nil, + } + if isDefault { + data["default"] = true + } + + if app.IsMachineOutput() { + return app.OK(data, output.WithSummary("Logged in as "+who.label())) + } + + w := cmd.OutOrStdout() + r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) + fmt.Fprintln(w, r.Success.Render("Logged in as "+who.label())) + fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: %s", name, account, scope, tokenLine))) + if created != nil { + line := fmt.Sprintf("Created profile %q for account %s", name, account) + if isDefault { + line += " (default)" + } + fmt.Fprintln(w, r.Muted.Render(line)) + } + return nil +} + +// accountGivenExplicitly reports whether the effective account came from +// this invocation (--account or BASECAMP_ACCOUNT_ID) rather than a config +// file: a new bot profile must not inherit whatever account the operator's +// own configuration happens to name. +func accountGivenExplicitly(app *appctx.App) bool { + src := config.Source(app.Config.Sources["account_id"]) + return app.Config.AccountID != "" && (src == config.SourceFlag || src == config.SourceEnv) +} + +// requireNumericAccount mirrors App.RequireAccount for an account the login +// is about to bind a profile to: anything but ASCII digits would leave a +// profile every account-scoped command rejects. +func requireNumericAccount(account string) error { + if account == "" { + return output.ErrUsage("Account ID required. Set via --account flag or BASECAMP_ACCOUNT_ID env.") + } + for _, c := range account { + if c < '0' || c > '9' { + return output.ErrUsage(fmt.Sprintf("Invalid account ID %q: must contain only digits", account)) + } + } + return nil +} + +// readTokenFromStdin reads one access token from the command's stdin. A +// terminal is refused outright — a secret typed at a prompt lands in shell +// and terminal history, and the command exists to be piped from a secret +// store. The token is never echoed, logged, or included in an error. +func readTokenFromStdin(cmd *cobra.Command) (string, error) { + in := cmd.InOrStdin() + if stdinarg.IsTerminal(in) { + return "", output.ErrUsageHint("--with-token reads the token from stdin, and stdin is a terminal", + "Pipe it in from a secret store: `op read \"op:////credential\" | basecamp auth login --with-token -P --account `.") + } + + data, err := io.ReadAll(io.LimitReader(in, maxTokenBytes+1)) + if err != nil { + return "", fmt.Errorf("reading token from stdin: %w", err) + } + if len(data) > maxTokenBytes { + return "", output.ErrUsage(fmt.Sprintf("Token on stdin is longer than %d bytes; expected a single access token", maxTokenBytes)) + } + + // A pipe delivers the token with one line ending (`op read`, `echo`, a + // CRLF file on Windows); that one is stripped and nothing else is — + // any other whitespace is not a token, and quietly trimming it would + // hide a malformed secret rather than the secret store's exact value. + token := strings.TrimSuffix(string(data), "\n") + token = strings.TrimSuffix(token, "\r") + if token == "" { + return "", output.ErrUsageHint("No token on stdin", + "Pipe it in from a secret store: `op read \"op:////credential\" | basecamp auth login --with-token -P --account `.") + } + if strings.IndexFunc(token, func(c rune) bool { return unicode.IsSpace(c) || unicode.IsControl(c) }) >= 0 { + return "", output.ErrUsage("Token on stdin must be a single line with no whitespace or control characters (one trailing line ending is allowed)") + } + return token, nil +} + +// machineOutputFlagSet reports whether an explicit output flag asked for a +// machine format. The config-driven formats are deliberately excluded: a +// configured format=json must not lock a person out of an interactive login. +func machineOutputFlagSet(app *appctx.App) bool { + return app.Flags.Agent || app.Flags.JSON || app.Flags.Quiet || app.Flags.IDsOnly || app.Flags.Count +} + +// parseExpectIdentity parses --expect-identity: 0 when absent, the identity +// ID otherwise. Identity IDs are positive, so 0 is free to mean "none". +func parseExpectIdentity(raw string) (int64, error) { + if raw == "" { + return 0, nil + } + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil || id <= 0 { + return 0, output.ErrUsage(fmt.Sprintf("Invalid --expect-identity %q: expected a numeric identity ID", raw)) + } + return id, nil +} + +// errEnvTokenShadows names the reason BASECAMP_TOKEN blocks a verified +// login: every request, the verification included, would carry the +// environment token instead of the credential being checked. +func errEnvTokenShadows(msg string) error { + return output.ErrUsageHint(msg, + "Every request, including the identity check, would use the environment token instead of the new credential. Unset it and retry.") +} + +// loginIdentity is who a credential authenticates as: the account-independent +// identity from the authorization endpoint — what --expect-identity compares +// against — and the person within the verified account. +type loginIdentity struct { + IdentityID int64 + IdentityEmail string + PersonID int64 + Name string + Email string + Scope string + // ExpiresAt is the expiry the authorization document reported for the + // credential; zero when it reported none. + ExpiresAt time.Time +} + +// label renders the identity for a one-line terminal sink. Name and email +// are server-supplied, so they are reduced to single lines first. +func (l *loginIdentity) label() string { + label := richtext.SanitizeSingleLine(l.Name) + if email := richtext.SanitizeSingleLine(l.Email); email != "" { + label += " <" + email + ">" + } + parts := []string{} + if l.IdentityID != 0 { + parts = append(parts, fmt.Sprintf("identity %d", l.IdentityID)) + } + if l.PersonID != 0 { + parts = append(parts, fmt.Sprintf("person %d", l.PersonID)) + } + if len(parts) > 0 { + label += " (" + strings.Join(parts, ", ") + ")" + } + return strings.TrimSpace(label) +} + +// loginVerifier proves a credential before it is stored. verify runs as +// LoginOptions.Verify (and directly for token imports) with a client bound +// to the candidate token, so nothing it learns comes from a stored +// credential or from BASECAMP_TOKEN. +// +// Strict mode is the assertive login: the authorization endpoint must +// answer with an identity, the effective account (when there is one) must +// be among the accounts the token can reach and its person record must +// resolve, and the identity must match any expectation. Non-strict mode +// keeps the informational "Logged in as" line best-effort: a lookup failure +// leaves who nil or without a person, and the login proceeds. A scope the +// CLI cannot store and a stated expectation are refused in either mode. +type loginVerifier struct { + app *appctx.App + expectIdentity int64 + account string + strict bool + // noRefresh marks a credential with nothing to refresh with (an + // imported token): one already inside the refresh window is refused, + // where an OAuth login's refresh token would simply renew it. + noRefresh bool + + who *loginIdentity +} + +func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType string) error { + client := v.app.SDKClientFor(&basecamp.StaticTokenProvider{Token: accessToken}) + + endpoint, err := v.app.Auth.AuthorizationEndpointFor(oauthType) + if err != nil { + return err + } + info, err := client.Authorization().GetInfo(ctx, &basecamp.GetInfoOptions{Endpoint: endpoint, FilterProduct: "bc3"}) + if err != nil { + if !v.strict { + return nil + } + return output.ErrAuth(fmt.Sprintf("Could not verify the new credential: %v", err)) + } + + who := &loginIdentity{ + IdentityID: info.Identity.ID, + IdentityEmail: info.Identity.EmailAddress, + Email: info.Identity.EmailAddress, + Name: strings.TrimSpace(info.Identity.FirstName + " " + info.Identity.LastName), + Scope: info.Scope, + } + if expiry, ok := info.Expiry(); ok { + who.ExpiresAt = expiry + } + if who.Scope != "" && who.Scope != "read" && who.Scope != "full" { + return output.ErrAuth(fmt.Sprintf("The server reports scope %q for this credential; only read or full can be stored", richtext.SanitizeSingleLine(who.Scope))) + } + if v.strict && who.IdentityID <= 0 { + return output.ErrAuth("The authorization endpoint did not report an identity for the new credential; nothing was stored") + } + // An imported token has nothing to refresh with, so one already inside + // the refresh window could not serve a single command once stored. + if v.noRefresh && !who.ExpiresAt.IsZero() && time.Until(who.ExpiresAt) <= auth.RefreshWindow { + return output.ErrAuth(fmt.Sprintf("The credential expires at %s, within the %s the CLI keeps clear of expiry; nothing was stored — mint a fresh token", who.ExpiresAt.UTC().Format(time.RFC3339), auth.RefreshWindow)) + } + if v.expectIdentity != 0 && who.IdentityID != v.expectIdentity { + return output.ErrAuth(fmt.Sprintf("Authenticated as %s, not identity %d; nothing was stored", who.label(), v.expectIdentity)) + } + + // The person record is account-scoped (/{account}/my/profile.json), so + // it doubles as the proof the token reaches the account it is about to + // be used for. The authorization document is checked first: a missing + // account there is a clearer answer than a 404 from the person lookup. + if v.account != "" { + if !authorizesAccount(info, v.account) { + if !v.strict { + v.who = who + return nil + } + return output.ErrAuth(fmt.Sprintf("%s cannot access account %s (authorized: %s); nothing was stored", who.label(), v.account, authorizedAccountIDs(info))) + } + person, err := client.ForAccount(v.account).People().Me(ctx) + if err != nil { + if !v.strict { + v.who = who + return nil + } + return output.ErrAuth(fmt.Sprintf("Could not verify %s on account %s: %v", who.label(), v.account, err)) + } + who.PersonID = person.ID + who.Name = person.Name + if person.EmailAddress != "" { + who.Email = person.EmailAddress + } + } + + v.who = who + return nil +} + +// authorizesAccount reports whether the authorization document names the +// account as reachable (and not expired) by the credential. +func authorizesAccount(info *basecamp.AuthorizationInfo, account string) bool { + for _, acct := range info.Accounts { + if !acct.Expired && accountIDsEqual(strconv.FormatInt(acct.ID, 10), account) { + return true + } + } + return false +} + +func authorizedAccountIDs(info *basecamp.AuthorizationInfo) string { + ids := make([]string, 0, len(info.Accounts)) + for _, acct := range info.Accounts { + if !acct.Expired { + ids = append(ids, strconv.FormatInt(acct.ID, 10)) + } + } + if len(ids) == 0 { + return "none" + } + return strings.Join(ids, ", ") +} + // buildLogoutCmd constructs a logout command with the given Use name. // Shared by newAuthLogoutCmd ("logout" under auth) and NewLogoutCmd (top-level). func buildLogoutCmd(use string) *cobra.Command { diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 1c6fee13..26a5f69d 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -3,18 +3,28 @@ package commands import ( "bytes" "context" + "encoding/json" + "fmt" + "io" "net/http" "net/http/httptest" "os" + "path/filepath" + "runtime" + "strings" + "sync" "testing" "time" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/stdinarg" ) // TestAuthLoginDeviceCodeForcesRemoteMode is the regression test for the @@ -78,3 +88,1025 @@ func TestAuthLoginDeviceCodeForcesRemoteMode(t *testing.T) { assert.NotContains(t, output, "Opening browser", "remote mode must not attempt a browser launch") } + +// loginIdentityServer is a mock resource server for the pre-store identity +// check: /authorization.json and the account-scoped /{account}/my/profile.json +// answer for exactly one bearer token and record what they were sent. +type loginIdentityServer struct { + srv *httptest.Server + + mu sync.Mutex + bearers []string + paths []string + // identityID is what /authorization.json reports; scope is optional; + // accounts are the authorized account IDs (999 by default). + identityID int64 + scope string + accounts []int64 + // authorizationStatus overrides the /authorization.json status when non-zero. + authorizationStatus int + // personName lets a test plant hostile content in the person record. + personName string + // expiresAt is what /authorization.json reports; empty omits the field. + expiresAt string +} + +func startLoginIdentityServer(t *testing.T, wantToken string) *loginIdentityServer { + t.Helper() + s := &loginIdentityServer{identityID: 28142355, accounts: []int64{999}, personName: "Clawdito", expiresAt: "2036-01-01T00:00:00Z"} + mux := http.NewServeMux() + record := func(r *http.Request) bool { + bearer := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + s.mu.Lock() + s.bearers = append(s.bearers, bearer) + s.paths = append(s.paths, r.URL.Path) + s.mu.Unlock() + return bearer == wantToken + } + mux.HandleFunc("/999/my/profile.json", func(w http.ResponseWriter, r *http.Request) { + if !record(r) { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "id": 51177542, "name": s.personName, "email_address": "clawdito@example.com", + })) + }) + mux.HandleFunc("/authorization.json", func(w http.ResponseWriter, r *http.Request) { + if !record(r) { + w.WriteHeader(http.StatusUnauthorized) + return + } + if s.authorizationStatus != 0 { + w.WriteHeader(s.authorizationStatus) + return + } + accounts := make([]map[string]any, 0, len(s.accounts)) + for _, id := range s.accounts { + accounts = append(accounts, map[string]any{"id": id, "name": "Acme", "href": fmt.Sprintf("%s/%d", s.srv.URL, id), "product": "bc3"}) + } + body := map[string]any{ + "identity": map[string]any{"id": s.identityID, "first_name": "Claw", "last_name": "Dito", "email_address": "identity@example.com"}, + "accounts": accounts, + } + if s.expiresAt != "" { + body["expires_at"] = s.expiresAt + } + if s.scope != "" { + body["scope"] = s.scope + } + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(body)) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + record(r) + http.NotFound(w, r) + }) + s.srv = httptest.NewServer(mux) + t.Cleanup(s.srv.Close) + return s +} + +func (s *loginIdentityServer) seenBearers() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.bearers...) +} + +func (s *loginIdentityServer) seenPaths() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.paths...) +} + +// managerTokenProvider feeds the app's SDK client from the auth manager, as +// the production authAdapter does. The login never verifies through it — +// verification rides a client bound to the candidate token — so a test that +// sees it used has found a regression. +type managerTokenProvider struct{ mgr *auth.Manager } + +func (p *managerTokenProvider) AccessToken(ctx context.Context) (string, error) { + return p.mgr.AccessToken(ctx) +} + +// loginTestApp is an App wired for --with-token tests: file credential +// store under a temp XDG_CONFIG_HOME, SDK client (and SDKClientFor) pointed +// at the identity server, JSON output captured in buf. +func loginTestApp(t *testing.T, srv *loginIdentityServer, cfg *config.Config) (*appctx.App, *bytes.Buffer) { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("BASECAMP_OAUTH_ISSUER", "") + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + cfg.BaseURL = srv.srv.URL + if cfg.Sources == nil { + cfg.Sources = map[string]string{} + } + authMgr := auth.NewManager(cfg, srv.srv.Client()) + authMgr.SetStore(auth.NewStore(config.GlobalConfigDir())) + sdkOptions := []basecamp.ClientOption{ + basecamp.WithTransport(srv.srv.Client().Transport), + basecamp.WithMaxRetries(1), + } + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: srv.srv.URL}, &managerTokenProvider{mgr: authMgr}, sdkOptions...) + buf := &bytes.Buffer{} + app := &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + SDKOptions: sdkOptions, + Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), + } + return app, buf +} + +// withAccount sets the effective account the way the root pre-run would +// from the given source ("flag", "env", "profile", "global"). +func withAccount(app *appctx.App, id, source string) { + app.Config.AccountID = id + app.Config.Sources["account_id"] = source + if source == "flag" { + app.Flags.Account = id + } +} + +// runLogin executes `auth login` with stdin set to in and returns everything +// the command wrote to stdout/stderr, and the error. +func runLogin(t *testing.T, app *appctx.App, in io.Reader, args ...string) (string, error) { + t.Helper() + cmd := NewAuthCmd() + cmd.SetArgs(append([]string{"login"}, args...)) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetIn(in) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + err := cmd.Execute() + return out.String(), err +} + +func readGlobalConfig(t *testing.T) map[string]any { + t.Helper() + data, err := os.ReadFile(filepath.Join(config.GlobalConfigDir(), "config.json")) + if os.IsNotExist(err) { + return map[string]any{} + } + require.NoError(t, err) + var cfg map[string]any + require.NoError(t, json.Unmarshal(data, &cfg)) + return cfg +} + +func assertNothingStored(t *testing.T, app *appctx.App, name string) { + t.Helper() + _, loadErr := app.Auth.GetStore().Load("profile:" + name) + assert.Error(t, loadErr, "no credential may be stored for a rejected token") + assert.NotContains(t, readGlobalConfig(t), "profiles", "no profile entry may be registered for a rejected token") + assert.NotContains(t, app.Config.Profiles, name) +} + +func TestAuthLoginWithTokenCreatesProfileAndVerifiesIdentity(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + // A trailing newline is what `op read` and `echo` deliver; it must not + // reach the Authorization header. + out, err := runLogin(t, app, strings.NewReader("bc_at_secret\n"), "--with-token") + require.NoError(t, err, out) + + assert.Contains(t, out, "Logged in as Clawdito (identity 28142355, person 51177542)") + assert.Contains(t, out, `Created profile "bot" for account 999 (default)`) + assert.NotContains(t, out, "bc_at_secret", "the token must never be echoed") + for _, bearer := range srv.seenBearers() { + assert.Equal(t, "bc_at_secret", bearer) + } + assert.Equal(t, []string{"/authorization.json", "/999/my/profile.json"}, srv.seenPaths(), + "the person lookup is account-scoped, after the authorization document") + + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "bc_at_secret", creds.AccessToken) + assert.Equal(t, time.Date(2036, 1, 1, 0, 0, 0, 0, time.UTC).Unix(), creds.ExpiresAt, "the expiry the server reported is kept") + assert.Empty(t, creds.RefreshToken) + assert.Equal(t, "bc5", creds.OAuthType) + assert.Equal(t, "full", creds.Scope) + assert.Equal(t, "51177542", creds.UserID) + assert.Equal(t, "clawdito@example.com", creds.UserEmail) + assert.Equal(t, "token", creds.Source) + + // Far from expiry: AccessToken serves it without attempting a refresh. + tok, err := app.Auth.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "bc_at_secret", tok) + + cfgFile := readGlobalConfig(t) + profiles := cfgFile["profiles"].(map[string]any) + bot := profiles["bot"].(map[string]any) + assert.Equal(t, "999", bot["account_id"]) + assert.Equal(t, srv.srv.URL, bot["base_url"]) + assert.Equal(t, "full", bot["scope"]) + assert.Equal(t, "bot", cfgFile["default_profile"], "the first profile becomes the default") + assert.Equal(t, "999", app.Config.Profiles["bot"].AccountID, "the in-memory config learns the profile too") +} + +func TestAuthLoginWithTokenJSONEnvelope(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + app.Flags.JSON = true + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", "28142355") + require.NoError(t, err, out) + assert.Empty(t, out, "machine mode writes the envelope only") + + var envelope struct { + OK bool `json:"ok"` + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.True(t, envelope.OK) + data := envelope.Data + assert.Equal(t, "bot", data["profile"]) + assert.Equal(t, "999", data["account_id"]) + assert.Equal(t, "token", data["source"]) + assert.Equal(t, "bc5", data["oauth_type"]) + assert.Equal(t, "full", data["scope"]) + assert.Equal(t, "2036-01-01T00:00:00Z", data["expires_at"]) + assert.Equal(t, true, data["profile_created"]) + assert.Equal(t, true, data["default"]) + assert.Equal(t, float64(28142355), data["identity"].(map[string]any)["id"]) + assert.Equal(t, "identity@example.com", data["identity"].(map[string]any)["email"], "the identity's own email, not the person's") + assert.Equal(t, float64(51177542), data["person"].(map[string]any)["id"]) + assert.Equal(t, "Clawdito", data["person"].(map[string]any)["name"]) + assert.Equal(t, "clawdito@example.com", data["person"].(map[string]any)["email"]) + assert.NotContains(t, buf.String(), "bc_at_secret") +} + +func TestAuthLoginWithTokenExpectIdentityMismatchStoresNothing(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret\n"), "--with-token", "--expect-identity", "1") + require.Error(t, err) + assert.Contains(t, err.Error(), "Authenticated as Claw Dito (identity 28142355), not identity 1") + assert.NotContains(t, err.Error()+out, "bc_at_secret") + + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) + + assertNothingStored(t, app, "bot") + assert.Equal(t, []string{"/authorization.json"}, srv.seenPaths(), "a rejected identity is not looked up any further") +} + +func TestAuthLoginWithTokenExpectIdentityAcceptsNumericSpellings(t *testing.T) { + for _, spelling := range []string{"28142355", "028142355", "+28142355"} { + t.Run(spelling, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", spelling) + require.NoError(t, err, out) + }) + } +} + +func TestAuthLoginWithTokenMismatchLeavesPreviousCredentialUntouched(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_new") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}, + } + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "profile") + prev := &auth.Credentials{AccessToken: "bc_at_old", RefreshToken: "old-refresh", OAuthType: "bc5", Scope: "full", ExpiresAt: 4102444800} + require.NoError(t, app.Auth.GetStore().Save("profile:bot", prev)) + + _, err := runLogin(t, app, strings.NewReader("bc_at_new"), "--with-token", "--expect-identity", "1") + require.Error(t, err) + + creds, loadErr := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, loadErr) + assert.Equal(t, prev, creds, "the previous credential is never touched by a rejected import") + for _, bearer := range srv.seenBearers() { + assert.Equal(t, "bc_at_new", bearer, "verification must never run as the stored credential") + } +} + +func TestAuthLoginWithTokenFailsClosedWhenUnverifiable(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.authorizationStatus = http.StatusUnauthorized + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "Could not verify the new credential") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenRejectedTokenIsNotStored(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_wrong"), "--with-token") + require.Error(t, err) + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenRequiresAccessToTheAccount(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.accounts = []int64{111, 222} + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot access account 999 (authorized: 111, 222)") + assertNothingStored(t, app, "bot") + assert.NotContains(t, srv.seenPaths(), "/999/my/profile.json") +} + +func TestAuthLoginWithTokenRejectsPersonLookupFailure(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.accounts = []int64{999, 1000} + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "1000", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err, "the authorization document lists 1000 but the account-scoped lookup 404s") + assert.Contains(t, err.Error(), "on account 1000") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenScopeReportedByServerWins(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.scope = "read" + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "read", creds.Scope) + assert.Equal(t, "51177542", creds.UserID) + assert.Equal(t, "read", app.Config.Profiles["bot"].Scope) +} + +func TestAuthLoginWithTokenRejectsUnknownReportedScope(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.scope = "admin\x1b[31m" + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "only read or full can be stored") + assert.NotContains(t, err.Error(), "\x1b") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenRejectsInvalidScopeBeforeReadingStdin(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + in := strings.NewReader("bc_at_secret") + _, err := runLogin(t, app, in, "--with-token", "--scope", "admin") + require.Error(t, err) + assert.Contains(t, err.Error(), "Invalid scope") + assert.Equal(t, 12, in.Len(), "stdin must not be consumed by a usage error") + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenRequiresAccountToCreateProfile(t *testing.T) { + t.Run("no account anywhere", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" does not exist`) + assert.Contains(t, err.Error(), "--account") + assert.Empty(t, srv.seenBearers(), "nothing may be sent before the profile question is settled") + assertNothingStored(t, app, "bot") + }) + + t.Run("a config-file account does not count", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "global") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "--account") + assert.Empty(t, srv.seenBearers()) + }) + + t.Run("BASECAMP_ACCOUNT_ID counts", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "env") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + assert.Equal(t, "999", app.Config.Profiles["bot"].AccountID) + }) +} + +func TestAuthLoginWithTokenRejectsNonNumericAccount(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "abc", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Invalid account ID "abc"`) + assert.Empty(t, srv.seenBearers()) + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenRequiresProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "named profile") + assert.Contains(t, err.Error(), "--profile") + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenRejectsAccountMismatchOnExistingProfile(t *testing.T) { + for _, source := range []string{"flag", "env"} { + t.Run(source, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "111"}}, + } + app, _ := loginTestApp(t, srv, cfg) + // The root pre-run re-applies flag and env over the profile + // binding, so the effective account is the override. + withAccount(app, "222", source) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" is bound to account 111, not 222`) + assert.Empty(t, srv.seenBearers()) + }) + } +} + +func TestAuthLoginWithTokenExistingProfileKeepsItsEntry(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + DefaultProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999", ProjectID: "42"}}, + } + app, buf := loginTestApp(t, srv, cfg) + withAccount(app, "999", "profile") + app.Flags.JSON = true + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), + []byte(`{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"42"}},"default_profile":"bot"}`), 0o600)) + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + + cfgFile := readGlobalConfig(t) + bot := cfgFile["profiles"].(map[string]any)["bot"].(map[string]any) + assert.Equal(t, "42", bot["project_id"], "importing into an existing profile must not rewrite its entry") + assert.Equal(t, "bot", cfgFile["default_profile"]) + + var envelope struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.Equal(t, false, envelope.Data["profile_created"]) + assert.Equal(t, true, envelope.Data["default"], "an existing default profile reports default") +} + +func TestAuthLoginWithTokenRejectsBadStdin(t *testing.T) { + for name, tc := range map[string]struct { + in string + want string + }{ + "empty": {"", "No token on stdin"}, + "bare newline": {"\n", "No token on stdin"}, + "whitespace": {" \n\t", "single line"}, + "leading space": {" bc_at_secret", "single line"}, + "trailing space": {"bc_at_secret ", "single line"}, + "two trailing newlines": {"bc_at_secret\n\n", "single line"}, + "two lines": {"bc_at_one\nbc_at_two\n", "single line"}, + "inner space": {"bc_at one", "single line"}, + "control char": {"bc_at\x1bone", "single line"}, + "oversized": {strings.Repeat("x", maxTokenBytes+1), "longer than"}, + } { + t.Run(name, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader(tc.in), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), tc.want) + assert.NotContains(t, err.Error(), "bc_at_", "the rejected input must not be echoed") + assert.Empty(t, srv.seenBearers()) + }) + } +} + +func TestAuthLoginWithTokenRefusesTerminalStdin(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + t.Skipf("open /dev/ptmx: %v", err) + } + defer pty.Close() + if !stdinarg.IsTerminal(pty) { + t.Skip("this environment's pty is not a terminal") + } + + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err = runLogin(t, app, pty, "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "stdin is a terminal") + assert.Contains(t, err.Error(), `op read "op:////credential" | basecamp auth login --with-token -P --account `) + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenRefusesEnvToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + t.Setenv("BASECAMP_TOKEN", "env-token") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_TOKEN is set") + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginRejectsNonNumericExpectIdentity(t *testing.T) { + for _, bad := range []string{"clawdito", "0", "-5", ""} { + if bad == "" { + continue + } + t.Run(bad, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", bad) + require.Error(t, err) + assert.Contains(t, err.Error(), "numeric identity ID") + assert.Empty(t, srv.seenBearers()) + }) + } +} + +func TestAuthLoginWithTokenIsExclusiveWithInteractiveFlags(t *testing.T) { + for _, flag := range []string{"--device-code", "--remote", "--local", "--no-browser", "--login-hint=x@y"} { + t.Run(flag, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", flag) + require.Error(t, err) + assert.Contains(t, err.Error(), "with-token") + }) + } +} + +// TestAuthLoginSanitizesServerSuppliedName: the person's name is server +// data headed for a one-line terminal sink; the JSON field keeps it verbatim. +func TestAuthLoginSanitizesServerSuppliedName(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.personName = "Claw\x1b]8;;https://evil.example\x07dito\r\nEvil" + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + assert.Contains(t, out, "Logged in as Clawdito Evil ") + assert.NotContains(t, out, "\x1b") + assert.NotContains(t, out, "evil.example") + + app2, buf2 := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app2, "999", "flag") + app2.Flags.JSON = true + _, err = runLogin(t, app2, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err) + _ = buf + assert.Contains(t, buf2.String(), `evil.example`, "structured output carries the field verbatim") +} + +// TestAuthLoginRefusesMachineOutputForInteractiveFlows covers the machine-mode +// half of #669: a browser or device login under --json/--agent used to print +// prose and block on approval. It now refuses before touching the network. +func TestAuthLoginRefusesMachineOutputForInteractiveFlows(t *testing.T) { + for name, set := range map[string]func(*appctx.App){ + "json": func(a *appctx.App) { a.Flags.JSON = true }, + "agent": func(a *appctx.App) { a.Flags.Agent = true }, + "quiet": func(a *appctx.App) { a.Flags.Quiet = true }, + "ids-only": func(a *appctx.App) { a.Flags.IDsOnly = true }, + "count": func(a *appctx.App) { a.Flags.Count = true }, + } { + t.Run(name, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + set(app) + _, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), "machine output mode") + assert.Contains(t, err.Error(), "--with-token") + assert.Empty(t, srv.seenBearers()) + }) + } +} + +func TestAuthLoginUnknownProfileNeedsCreateOrToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "ghost"}) + + _, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "ghost" does not exist`) + assert.Contains(t, err.Error(), "basecamp profile create ghost") + assert.Contains(t, err.Error(), "--with-token -P ghost") +} + +func TestAuthLoginExpectIdentityRefusesEnvToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + t.Setenv("BASECAMP_TOKEN", "env-token") + + _, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "1") + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_TOKEN is set") +} + +// TestAuthLoginDeviceFlowExpectIdentityStoresNothingOnMismatch: the OAuth +// flows verify through the same pre-store hook. A device login whose token +// belongs to someone else never reaches the credential store, and the +// success line is never printed. +func TestAuthLoginDeviceFlowExpectIdentityStoresNothingOnMismatch(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + // A pinned issuer served by the same mux: the device grant hands out + // dev-tok, which the identity server then answers for. + srv.srv.Config.Handler = deviceGrantThen(t, srv.srv.Config.Handler) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot", Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}}) + withAccount(app, "999", "profile") + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + + out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "1") + require.Error(t, err) + assert.Contains(t, err.Error(), "not identity 1") + assert.NotContains(t, out, "Authentication successful", "no success line before the check") + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "a device login that fails --expect-identity stores nothing") + + // The same grant with the right expectation is stored and labeled. + out, err = runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "28142355") + require.NoError(t, err, out) + assert.Contains(t, out, "Authentication successful") + assert.Contains(t, out, "Logged in as: Clawdito (identity 28142355, person 51177542)") + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "dev-tok", creds.AccessToken) + assert.Equal(t, "51177542", creds.UserID) +} + +// deviceGrantThen wraps a handler with a BC5 device grant that issues +// dev-tok immediately, at the paths a pinned issuer derives. +func deviceGrantThen(t *testing.T, next http.Handler) http.Handler { + t.Helper() + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/device_authorizations": + fmt.Fprintf(w, `{"device_code":"dc","user_code":"ABCD-EFGH","verification_uri":%q,"expires_in":600,"interval":1}`, "http://"+r.Host+"/verify") + case "/oauth/tokens": + fmt.Fprint(w, `{"access_token":"dev-tok","refresh_token":"dev-ref","token_type":"bearer","expires_in":3600,"scope":"full"}`) + default: + next.ServeHTTP(w, r) + } + }) +} + +func TestAuthLoginWithTokenAcceptsOneTrailingLineEnding(t *testing.T) { + for name, in := range map[string]string{"LF": "bc_at_secret\n", "CRLF": "bc_at_secret\r\n", "none": "bc_at_secret"} { + t.Run(name, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + out, err := runLogin(t, app, strings.NewReader(in), "--with-token") + require.NoError(t, err, out) + for _, bearer := range srv.seenBearers() { + assert.Equal(t, "bc_at_secret", bearer) + } + }) + } +} + +func TestAuthLoginWithTokenFailsClosedWithoutAnIdentity(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.identityID = 0 + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "did not report an identity") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenBindsAnUnboundExistingProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {ProjectID: "42"}}, + } + app, _ := loginTestApp(t, srv, cfg) + app.Config.Sources["profiles"] = "global" + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), + []byte(`{"profiles":{"bot":{"base_url":"`+srv.srv.URL+`","project_id":"42"}}}`), 0o600)) + + t.Run("without an explicit account the import is refused", func(t *testing.T) { + withAccount(app, "999", "global") + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" has no account`) + assert.Empty(t, srv.seenBearers()) + }) + + t.Run("an explicit account is bound alongside the token", func(t *testing.T) { + withAccount(app, "999", "flag") + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + bot := readGlobalConfig(t)["profiles"].(map[string]any)["bot"].(map[string]any) + assert.Equal(t, "999", bot["account_id"], "the verified account is persisted on the profile") + assert.Equal(t, "42", bot["project_id"], "the rest of the entry is preserved") + assert.Equal(t, "999", app.Config.Profiles["bot"].AccountID) + }) +} + +func TestAuthLoginWithTokenRejectsBaseURLOverrideOnExistingProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {BaseURL: "https://staging.example/", AccountID: "999"}}, + } + // loginTestApp points the effective base URL at the identity server, + // which is what a BASECAMP_BASE_URL override would do. + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "profile") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" is bound to https://staging.example/, not `+srv.srv.URL) + assert.Empty(t, srv.seenBearers()) + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr) +} + +func TestAuthLoginWithTokenComparesAccountsNumerically(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "0999"}}, + } + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "flag") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) +} + +// TestAuthLoginDeviceFlowWithoutExpectationKeepsBestEffortIdentity: an +// ordinary login must not be lost to the informational lookups. When the +// configured account is not one the token reaches, the credential is still +// stored, the identity line falls back to the authorization document, and +// no person id is fabricated. +func TestAuthLoginDeviceFlowWithoutExpectationKeepsBestEffortIdentity(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + srv.accounts = []int64{111} + srv.srv.Config.Handler = deviceGrantThen(t, srv.srv.Config.Handler) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot", Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}}) + withAccount(app, "999", "profile") + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + + out, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.NoError(t, err, out) + assert.Contains(t, out, "Authentication successful") + assert.Contains(t, out, "Logged in as: Claw Dito (identity 28142355)") + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "dev-tok", creds.AccessToken) + assert.Empty(t, creds.UserID, "no person was resolved, so none is recorded") +} + +func TestAuthLoginWithTokenWithoutReportedExpiryIsNonExpiring(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.expiresAt = "" + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + app.Flags.JSON = true + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Zero(t, creds.ExpiresAt) + + var envelope struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.Contains(t, envelope.Data, "expires_at") + assert.Nil(t, envelope.Data["expires_at"]) +} + +// TestAuthLoginWithTokenBindsOnlyTheGlobalConfigsOwnEntry: config layers +// merge per profile name, so cfg.Sources["profiles"] is aggregate and says +// nothing about where this profile came from. The global file's entry is +// the evidence: bind when it exists and is unbound, refuse otherwise — +// before stdin is read. +func TestAuthLoginWithTokenBindsOnlyTheGlobalConfigsOwnEntry(t *testing.T) { + cases := map[string]struct { + global string + sources string + wantErr string + }{ + "global file has no such profile": { + global: `{"profiles":{"other":{"base_url":"https://3.basecampapi.com"}}}`, + sources: "repo", + wantErr: "not the global config's entry", + }, + "global entry is already bound (shadowed by an accountless layer)": { + global: `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"111"}}}`, + sources: "local", + wantErr: "not the global config's entry", + }, + "global entry is unbound even though another layer contributed profiles": { + global: `{"profiles":{"bot":{"base_url":"BASE","project_id":"42"}}}`, + sources: "repo", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {}}, + Sources: map[string]string{"profiles": tc.sources}, + } + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "flag") + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), + []byte(strings.ReplaceAll(tc.global, "BASE", srv.srv.URL)), 0o600)) + + in := strings.NewReader("bc_at_secret") + out, err := runLogin(t, app, in, "--with-token") + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + assert.Equal(t, 12, in.Len(), "refused before stdin is read") + assert.Empty(t, srv.seenBearers()) + return + } + require.NoError(t, err, out) + bot := readGlobalConfig(t)["profiles"].(map[string]any)["bot"].(map[string]any) + assert.Equal(t, "999", bot["account_id"]) + assert.Equal(t, "42", bot["project_id"]) + }) + } +} + +func TestAuthLoginWithTokenRefusesATokenInsideTheRefreshWindow(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.expiresAt = time.Now().Add(2 * time.Minute).UTC().Format(time.RFC3339) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "within the 5m0s the CLI keeps clear of expiry") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginRejectsPositionalArguments(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + in := strings.NewReader("bc_at_secret") + _, err := runLogin(t, app, in, "--with-token", "bc_at_pasted") + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown command") + assert.Equal(t, 12, in.Len()) + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthStatusReportsAnImportedToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err) + + cmd := NewAuthCmd() + cmd.SetArgs([]string{"status"}) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + require.NoError(t, cmd.Execute()) + var envelope struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Equal(t, "token", envelope.Data["source"], "a machine consumer can tell an imported token from an OAuth login") + assert.Equal(t, "bc5", envelope.Data["oauth_type"]) +} + +func TestAuthLoginWithTokenRefusesToRewriteMalformedConfig(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + require.NoError(t, os.WriteFile(configPath, []byte("{ not json"), 0o600)) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "not valid JSON") + data, readErr := os.ReadFile(configPath) + require.NoError(t, readErr) + assert.Equal(t, "{ not json", string(data), "the operator's file is left exactly as it was") + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "the entry comes before the credential, so nothing is stored") +} + +// TestAuthLoginDeviceFlowExpectIdentityKeepsAShortLivedAccessToken: the +// near-expiry refusal is for imports, which cannot refresh. An asserted +// OAuth login whose access token is about to expire has a refresh token +// and is stored. +func TestAuthLoginDeviceFlowExpectIdentityKeepsAShortLivedAccessToken(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + srv.expiresAt = time.Now().Add(2 * time.Minute).UTC().Format(time.RFC3339) + srv.srv.Config.Handler = deviceGrantThen(t, srv.srv.Config.Handler) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot", Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}}) + withAccount(app, "999", "profile") + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + + out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "28142355") + require.NoError(t, err, out) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "dev-ref", creds.RefreshToken) + assert.Empty(t, creds.Source) +} + +func TestAuthLoginWithTokenRefusesToRewriteANonObjectProfilesValue(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"profiles":[],"format":"json"}`), 0o600)) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `"profiles" value that is not an object`) + data, readErr := os.ReadFile(configPath) + require.NoError(t, readErr) + assert.Equal(t, `{"profiles":[],"format":"json"}`, string(data)) + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr) +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 81b65865..9c4fc0e0 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -7,6 +7,7 @@ import ( "path/filepath" "regexp" "sort" + "strconv" "github.com/spf13/cobra" @@ -156,6 +157,9 @@ func newProfileShowCmd() *cobra.Command { if err == nil && creds.AccessToken != "" { result["authenticated"] = true result["oauth_type"] = creds.OAuthType + if creds.Source != "" { + result["source"] = creds.Source + } isLaunchpad = creds.OAuthType == "launchpad" // Suppress credential scope for Launchpad (scopes not supported) @@ -232,6 +236,13 @@ Examples: profileCfg.AccountID = accountID } + // The entry is written only after the login succeeds, so prove + // the config file can take it before a credential exists to + // orphan: a malformed file is refused here, not after OAuth. + if _, err := writableGlobalProfiles(); err != nil { + return err + } + // Snapshot in-memory config before mutation prevActiveProfile := app.Config.ActiveProfile prevBaseURL := app.Config.BaseURL @@ -269,43 +280,8 @@ Examples: profileCfg.Scope = loginResult.Scope } - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") - if err := os.MkdirAll(config.GlobalConfigDir(), 0700); err != nil { - return fmt.Errorf("failed to create config directory: %w", err) - } - - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) - } - - // Get or create profiles map - profilesMap, _ := configData["profiles"].(map[string]any) - if profilesMap == nil { - profilesMap = make(map[string]any) - } - - // Add profile with effective scope - profileEntry := map[string]any{ - "base_url": profileCfg.BaseURL, - } - if profileCfg.AccountID != "" { - profileEntry["account_id"] = profileCfg.AccountID - } - if profileCfg.Scope != "" { - profileEntry["scope"] = profileCfg.Scope - } - profilesMap[name] = profileEntry - configData["profiles"] = profilesMap - - // If this is the first profile, set it as default - isDefault := len(profilesMap) == 1 - if isDefault { - configData["default_profile"] = name - } - - // Write config atomically - if err := atomicWriteJSON(configPath, configData); err != nil { + isDefault, err := registerProfile(name, profileCfg) + if err != nil { return err } @@ -378,27 +354,7 @@ func newProfileDeleteCmd() *cobra.Command { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: could not delete credentials for profile %q: %v\n", name, err) } - // Update config file - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) - } - - if profilesMap, ok := configData["profiles"].(map[string]any); ok { - delete(profilesMap, name) - if len(profilesMap) == 0 { - delete(configData, "profiles") - } - } - - // Clear default_profile if it was this profile - if dp, ok := configData["default_profile"].(string); ok && dp == name { - delete(configData, "default_profile") - } - - // Write config back atomically - if err := atomicWriteJSON(configPath, configData); err != nil { + if err := unregisterProfile(name); err != nil { return err } @@ -432,13 +388,10 @@ func newProfileSetDefaultCmd() *cobra.Command { return output.ErrUsage(fmt.Sprintf("Profile %q not found", name)) } - // Update config file - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return err } - configData["default_profile"] = name if err := atomicWriteJSON(configPath, configData); err != nil { @@ -453,6 +406,170 @@ func newProfileSetDefaultCmd() *cobra.Command { } } +// registerProfile adds a profile entry to the global config file. The first +// profile registered becomes the default; isDefault reports whether this one +// did. The in-memory config is the caller's to update. +func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err error) { + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return false, err + } + profilesMap, err := globalProfilesMap(configData, configPath) + if err != nil { + return false, err + } + + entry := map[string]any{ + "base_url": p.BaseURL, + } + if p.AccountID != "" { + entry["account_id"] = p.AccountID + } + if p.Scope != "" { + entry["scope"] = p.Scope + } + profilesMap[name] = entry + + isDefault = len(profilesMap) == 1 + if isDefault { + configData["default_profile"] = name + } + + return isDefault, atomicWriteJSON(configPath, configData) +} + +// loadGlobalConfigFile returns the global config file's contents and path, +// with the config directory in place for the write every caller is about +// to make. A missing file is an empty config; a file that cannot be read or +// parsed is an error, since writing the map back would otherwise replace +// whatever the operator had with a partial decode. +func loadGlobalConfigFile() (map[string]any, string, error) { + if err := os.MkdirAll(config.GlobalConfigDir(), 0700); err != nil { + return nil, "", fmt.Errorf("failed to create config directory: %w", err) + } + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + configData := make(map[string]any) + data, err := os.ReadFile(configPath) //nolint:gosec // G304: Path is from trusted config location + if os.IsNotExist(err) { + return configData, configPath, nil + } + if err != nil { + return nil, configPath, fmt.Errorf("failed to read config file %s: %w", configPath, err) + } + if err := json.Unmarshal(data, &configData); err != nil { + return nil, configPath, fmt.Errorf("config file %s is not valid JSON, refusing to rewrite it: %w", configPath, err) + } + return configData, configPath, nil +} + +// globalProfilesMap returns the config's "profiles" object, creating it in +// the map when absent. A present value of any other shape is refused for +// the same reason a parse failure is: the caller is about to write the map +// back, and replacing an unexpected value would destroy operator config. +func globalProfilesMap(configData map[string]any, configPath string) (map[string]any, error) { + raw, present := configData["profiles"] + if !present { + profilesMap := make(map[string]any) + configData["profiles"] = profilesMap + return profilesMap, nil + } + profilesMap, ok := raw.(map[string]any) + if !ok { + return nil, fmt.Errorf("config file %s has a \"profiles\" value that is not an object, refusing to rewrite it", configPath) + } + return profilesMap, nil +} + +// writableGlobalProfiles reports whether the global config file can take a +// profile entry — readable, parseable, with an object (or absent) profiles +// value — without writing anything. Callers that obtain a credential before +// registering its profile run this first. +func writableGlobalProfiles() (map[string]any, error) { + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return nil, err + } + return globalProfilesMap(configData, configPath) +} + +// globalProfileEntry returns the named profile's entry in the global config +// file, or nil when the file has none (or its profiles value is not an +// object — which the writers refuse separately). +func globalProfileEntry(configData map[string]any, name string) map[string]any { + profilesMap, _ := configData["profiles"].(map[string]any) + entry, _ := profilesMap[name].(map[string]any) + return entry +} + +// globalProfileIsUnbound reports whether the global config file defines the +// profile without an account — the one shape bindProfileAccount can act on. +// Config layers merge per profile name, so the effective profile being +// accountless says nothing about which file it came from; the global entry +// itself is the evidence. +func globalProfileIsUnbound(name string) (bool, error) { + configData, _, err := loadGlobalConfigFile() + if err != nil { + return false, err + } + entry := globalProfileEntry(configData, name) + return entry != nil && getStringOrNumber(entry, "account_id") == "", nil +} + +// bindProfileAccount sets the account on an existing profile entry in the +// global config file, leaving every other field of the entry as it is. +func bindProfileAccount(name, account string) error { + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return err + } + if _, err := globalProfilesMap(configData, configPath); err != nil { + return err + } + entry := globalProfileEntry(configData, name) + if entry == nil { + return output.ErrUsage(fmt.Sprintf("Profile %q not found in %s", name, configPath)) + } + entry["account_id"] = account + return atomicWriteJSON(configPath, configData) +} + +// getStringOrNumber reads a config value that may be stored as a string or +// a JSON number, as config.loadFromFile accepts for IDs. +func getStringOrNumber(m map[string]any, key string) string { + switch v := m[key].(type) { + case string: + return v + case float64: + return strconv.FormatFloat(v, 'f', -1, 64) + default: + return "" + } +} + +// unregisterProfile removes a profile entry from the global config file, +// clearing default_profile when it named this profile. Credentials are the +// caller's to remove. +func unregisterProfile(name string) error { + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return err + } + profilesMap, err := globalProfilesMap(configData, configPath) + if err != nil { + return err + } + delete(profilesMap, name) + if len(profilesMap) == 0 { + delete(configData, "profiles") + } + + if dp, ok := configData["default_profile"].(string); ok && dp == name { + delete(configData, "default_profile") + } + + return atomicWriteJSON(configPath, configData) +} + // validProfileName matches letters, numbers, hyphens, and underscores. var validProfileName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index 917e9929..eee79b32 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -1134,3 +1134,41 @@ func TestProfileCreateWithNilProfilesMap(t *testing.T) { require.True(t, ok, "expected profiles map in config") assert.Contains(t, profiles, "new-profile", "new profile should be created") } + +// TestProfileSetDefaultCreatesTheConfigDir: the global config directory may +// not exist yet when the profiles came from another config layer. +func TestProfileSetDefaultCreatesTheConfigDir(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", filepath.Join(t.TempDir(), "fresh")) + + app, _ := setupTestApp(t) + app.Config.Profiles = map[string]*config.ProfileConfig{"bot": {BaseURL: "https://3.basecampapi.com"}} + + require.NoError(t, executeCommand(NewProfileCmd(), app, "set-default", "bot")) + data, err := os.ReadFile(filepath.Join(config.GlobalConfigDir(), "config.json")) + require.NoError(t, err) + assert.Contains(t, string(data), `"default_profile": "bot"`) +} + +// TestProfileCreateRefusesAMalformedConfigBeforeLogin: registration happens +// after OAuth, so a config file that cannot take the entry must be refused +// before a credential exists to orphan. +func TestProfileCreateRefusesAMalformedConfigBeforeLogin(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"profiles":[]}`), 0o600)) + + // Any network use would be a login attempt; the no-network transport + // fails instantly, and the assertion below is on the refusal wording. + app, _ := setupTestApp(t) + err := executeCommand(NewProfileCmd(), app, "create", "bot", "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), `"profiles" value that is not an object`) + data, readErr := os.ReadFile(configPath) + require.NoError(t, readErr) + assert.Equal(t, `{"profiles":[]}`, string(data)) + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "no login may have run") +} diff --git a/internal/stdinarg/stdinarg.go b/internal/stdinarg/stdinarg.go index 2666f131..66fecfd2 100644 --- a/internal/stdinarg/stdinarg.go +++ b/internal/stdinarg/stdinarg.go @@ -94,6 +94,16 @@ func IsPiped(r io.Reader) bool { return fi.Mode()&os.ModeCharDevice == 0 } +// IsTerminal reports whether the reader is an interactive terminal. A +// non-*os.File reader — the cmd.SetIn test seam — never is. Unlike IsPiped +// this asks term.IsTerminal, so /dev/null (a character device that delivers +// nothing) is not a terminal: a secret redirected from it should read as +// empty, not be refused as typed. +func IsTerminal(r io.Reader) bool { + f, ok := r.(*os.File) + return ok && isTerminal(f) +} + // InteractiveStdio reports whether both stdout and stdin are terminals — the // floor for launching anything that draws to the terminal and reads // keystrokes. A TUI (picker, wizard) reads key events from stdin, so a pipe or diff --git a/internal/stdinarg/stdinarg_test.go b/internal/stdinarg/stdinarg_test.go index 78d935a7..c6abf5be 100644 --- a/internal/stdinarg/stdinarg_test.go +++ b/internal/stdinarg/stdinarg_test.go @@ -161,3 +161,25 @@ func TestPredicatesDisagreeOnTheStreamTheyAskAbout(t *testing.T) { assert.False(t, InteractiveStdio(), "a piped stdout is no place for a picker") assert.True(t, InteractivePrompt(), "but a form draws to stderr, which is still a terminal") } + +// TestIsTerminal: only a real terminal counts. A buffer (the cmd.SetIn +// seam), a pipe, and /dev/null — a character device that delivers nothing — +// are all "not a terminal", so a secret redirected from any of them is read +// rather than refused as typed. +func TestIsTerminal(t *testing.T) { + assert.False(t, IsTerminal(strings.NewReader("token"))) + + devnull, err := os.Open(os.DevNull) + require.NoError(t, err) + defer devnull.Close() + assert.False(t, IsTerminal(devnull)) + + pipeR, pipeW, err := os.Pipe() + require.NoError(t, err) + defer pipeR.Close() + defer pipeW.Close() + assert.False(t, IsTerminal(pipeR)) + + pty := openPTY(t) + assert.True(t, IsTerminal(pty)) +} diff --git a/skills/basecamp-doctor/SKILL.md b/skills/basecamp-doctor/SKILL.md index 913399fd..eafda447 100644 --- a/skills/basecamp-doctor/SKILL.md +++ b/skills/basecamp-doctor/SKILL.md @@ -20,7 +20,11 @@ Interpret every check by status: Report failures and warnings with their `hint` fields. Also inspect the top-level `breadcrumbs` array and preserve its structured `cmd` next steps, because a breadcrumb can provide a more specific action than a check hint. Use these common remediations when relevant: -- Basecamp authentication: `basecamp auth login` +- Basecamp authentication: `basecamp auth login`. Guard a replacement login + with `--expect-identity ` so a browser signed in as someone else cannot + become the profile. A bot or CI profile that must never sign in interactively + imports a personal access token instead: + `op read "op:////credential" | basecamp auth login --with-token -P --account --expect-identity ` - Agent plugin installation or version: `basecamp setup agents` (honors `BASECAMP_SETUP_AGENT`) - Codex plugin specifically: `basecamp setup codex` - Claude Code plugin specifically: `basecamp setup claude` diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index ca867626..c1fe7a95 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1317,6 +1317,8 @@ basecamp auth login # Re-authenticate 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 +basecamp auth login --with-token -P bot --account # Import a personal access token from stdin (pipe it in) +basecamp auth login --expect-identity # Discard the login unless it authenticated as this identity ``` **Network errors / localhost URLs:**