feat(auth): rewrite auth and session commands as native Go#43
feat(auth): rewrite auth and session commands as native Go#43miguelsanchez-upsun wants to merge 57 commits intomainfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Rewrites the CLI’s authentication and session-related commands from PHP delegation to native Go implementations, introducing a Go-backed session store, OAuth2/PKCE browser flow, and session-backed token sourcing (while keeping PHP parity where required).
Changes:
- Added a session Manager/Store abstraction with filesystem and in-memory implementations.
- Implemented OAuth2 URL resolution, PKCE helpers, browser login flow, and session-backed token source/transport retry support.
- Added/updated integration and unit tests plus mock API/auth server endpoints to cover the new Go commands.
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/mockapi/auth_server.go | Extends mock auth server with OAuth2 authorize/code + PKCE validation and token revocation tracking. |
| pkg/mockapi/api_server.go | Adds mock phone verification endpoints for parity with real API paths. |
| internal/session/store.go | Adds session Store interface with filesystem and in-memory implementations. |
| internal/session/resolver.go | Adds session ID resolution and PHP-compatible ID sanitization helpers. |
| internal/session/resolver_test.go | Unit tests for session ID sanitization and directory naming. |
| internal/session/manager.go | Introduces session Manager for session/token persistence, listing, and deletion. |
| internal/session/manager_test.go | Unit tests for file/memory store behavior and Manager operations. |
| internal/legacy/legacy.go | Allows injecting additional env vars into PHP subprocess executions. |
| internal/auth/urls.go | Adds env/config-based OAuth2 endpoint resolution helpers. |
| internal/auth/urls_test.go | Unit tests for OAuth2 URL precedence and env-prefix isolation. |
| internal/auth/transport.go | Updates transport to retry once on 401 and restore POST bodies for the retry. |
| internal/auth/token_source.go | Adds a session-backed oauth2.TokenSource with refresh-token support and caching. |
| internal/auth/token_source_test.go | Unit tests for session token loading, refreshing, and error cases. |
| internal/auth/pkce.go | Adds PKCE verifier/challenge helpers. |
| internal/auth/pkce_test.go | Unit tests for PKCE verifier generation and RFC vector challenge derivation. |
| internal/auth/legacy.go | Removes legacy PHP-token-source implementation (now replaced by session token source). |
| internal/auth/flow.go | Implements local callback server + PKCE browser login flow orchestration. |
| internal/auth/flow_unix.go | Adds Unix browser opener and tty guard behavior. |
| internal/auth/flow_windows.go | Adds Windows browser opener behavior. |
| internal/auth/client.go | Replaces legacy-authenticated client with session-authenticated client and transport. |
| internal/api/users.go | Adds user and phone verification API helpers used by new auth commands. |
| commands/cobrahelp/help.go | Adds PHP-style help rendering for Go commands. |
| commands/session/switch.go | Implements session:switch command to persist active session ID. |
| commands/auth/helpers.go | Adds shared helpers for session injection into PHP, API client creation, and rendering. |
| commands/auth/helpers_test.go | Unit tests for helpers (format/table/session hints/env injection). |
| commands/auth/token.go | Implements auth:token in Go, including api_token exchange and header output. |
| commands/auth/api_token_login.go | Implements auth:api-token-login in Go with retries and session persistence. |
| commands/auth/browser_login.go | Implements auth:browser-login (PKCE) in Go with prompts and persistence. |
| commands/auth/info.go | Implements auth:info in Go with auto-login logic and property output. |
| commands/auth/logout.go | Implements auth:logout in Go including revoke + per-session deletion modes. |
| commands/auth/verify_phone.go | Implements auth:verify-phone-number interactive flow in Go. |
| commands/auth/ssh.go | Delegates post-login SSH cert finalization to legacy PHP as best-effort. |
| commands/root.go | Registers new Go auth/session commands and injects session creds into PHP delegation. |
| commands/init.go | Switches init to use new Go auth client instead of legacy PHP auth client. |
| integration-tests/tests.go | Updates integration test harness env var names and per-test HOME isolation + stdin wiring. |
| integration-tests/session_switch_test.go | Adds integration tests for session:switch behaviors. |
| integration-tests/auth_token_test.go | Adds integration tests for auth:token output modes and default warnings. |
| integration-tests/auth_logout_test.go | Adds integration tests for logout modes and multi-session hint messaging. |
| integration-tests/auth_api_token_login_test.go | Adds integration tests for API token login + PHP command auth after Go login. |
| integration-tests/auth_browser_login_test.go | Adds integration tests for PKCE browser login flow and already-logged-in behavior. |
| integration-tests/auth_info_test.go | Expands integration tests for auth:info prompting, auto-login, and aliases. |
| integration-tests/auth_verify_phone_test.go | Adds integration tests for verify-phone retries and attempt limits. |
| go.mod | Adds new dependencies used by browser flow/help and phone validation; updates protobuf version. |
| go.sum | Updates checksums for new/updated Go module dependencies. |
Comments suppressed due to low confidence (3)
internal/session/manager.go:1
- Manager’s API token operations bypass the injected Store abstraction (direct os.ReadFile/os.WriteFile/os.Remove). This breaks NewWithStore testability and can leave API-token files behind when using MemStore (e.g., Delete/DeleteAll won’t clean them up because they call Store.Delete). Consider extending Store to cover token file read/write/remove (or storing the api-token as a Store-backed artifact) and route Get/Set/DeleteAPIToken through the Store implementation.
internal/auth/token_source.go:1 - unsafeRefreshToken always sets BasicAuth even when OAuth2ClientID is empty. That can unintentionally send an Authorization header and cause refresh failures against stricter OAuth servers. Mirror the guard used elsewhere (only set BasicAuth if the client ID is non-empty).
internal/auth/transport.go:1 - wrapRequest unconditionally buffers the full request body into memory and ignores io.ReadAll/Close errors. This can cause large memory spikes for big uploads and can silently turn read failures into empty/partial bodies on retry. Consider (1) returning an error from wrapRequest and handling it in RoundTrip, (2) not overriding an existing req.GetBody if already set, and (3) optionally limiting buffering (or only enabling buffering for known small request types) to reduce memory risk.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const maxAttempts = 5 | ||
| scanner := bufio.NewScanner(cmd.InOrStdin()) | ||
| for attempt := 1; attempt <= maxAttempts; attempt++ { | ||
| fmt.Fprint(cmd.ErrOrStderr(), "Enter your API token: ") | ||
| if !scanner.Scan() { | ||
| return fmt.Errorf("read API token: %w", scanner.Err()) | ||
| } | ||
| apiToken = strings.TrimSpace(scanner.Text()) | ||
| if apiToken == "" { | ||
| fmt.Fprintln(cmd.ErrOrStderr(), "The token cannot be empty") | ||
| continue | ||
| } | ||
| var err error | ||
| s, err = exchangeAPIToken(cmd.Context(), cfg, apiToken) | ||
| if err == nil { | ||
| break | ||
| } | ||
| if errors.Is(err, ErrInvalidAPIToken) { | ||
| fmt.Fprintln(cmd.ErrOrStderr(), ErrInvalidAPIToken.Error()) | ||
| if attempt == maxAttempts { | ||
| return fmt.Errorf("login failed after %d attempts", maxAttempts) | ||
| } | ||
| continue | ||
| } | ||
| return fmt.Errorf("login failed: %w", err) | ||
| } | ||
| } | ||
| fmt.Fprintln(cmd.ErrOrStderr(), "The API token is valid.") |
There was a problem hiding this comment.
In the interactive path, empty input does not fail after maxAttempts; the loop can exit with apiToken == "" and s == nil, but the code still prints “The API token is valid.” and proceeds to persist credentials. Track whether a successful exchange occurred and return an error if attempts are exhausted due to empty input (and also handle the case where the loop completes without setting s).
internal/api/users.go
Outdated
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| resp, err := c.HTTPClient.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
| var result struct { | ||
| Type string `json:"type"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | ||
| return err | ||
| } | ||
| if result.Type == "phone" { | ||
| return fmt.Errorf("phone verification status is still pending") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
CheckVerificationStatus does not validate resp.StatusCode before decoding/using the response. A non-200 response could be decoded as an empty struct and incorrectly treated as “verified”. Add an explicit status-code check (similar to SendPhoneVerification/VerifyPhone) and return a clear error when the server is not OK.
| func (c *Client) EnsureAuthenticated(_ context.Context) error { | ||
| _, err := c.tokenSource.Token() |
There was a problem hiding this comment.
EnsureAuthenticated ignores the provided context and calls Token(), which refreshes using context.Background() (per sessionTokenSource.Token). This prevents cancellation/timeouts from propagating during auth checks. Prefer using the passed context (e.g., call TokenContext(ctx)) so callers can bound auth latency.
| func (c *Client) EnsureAuthenticated(_ context.Context) error { | |
| _, err := c.tokenSource.Token() | |
| func (c *Client) EnsureAuthenticated(ctx context.Context) error { | |
| _, err := c.tokenSource.TokenContext(ctx) |
…on.Manager Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> # Conflicts: # commands/init.go
# Conflicts: # commands/init.go
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… not-logged-in error
…urls.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…IToken function Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n method Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… in NewWithStore Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract resolveAuthBase() to eliminate 3x URL resolution duplication - Extract InjectSessionCredentials() shared by root.go and ssh.go - Extract printSessionID() shared by browser_login and info - Fix variable shadowing httpClient in newAPIClient - Simplify info.go double login-state check into single pass - Eliminate redundant session.New() in auth:token command - Avoid N+1 session load in token refresh (pass loaded session) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…token source, and auth helpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…HP parity Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… parity) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…and verify-phone retry Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Attempts(5) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
59814b6 to
191b06c
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Route API token I/O through the Store interface (ReadFile/WriteFile/RemoveFile) so NewWithStore+MemStore is fully hermetic in tests - Guard SetBasicAuth in token_source.go when OAuth2ClientID is empty, matching the pattern already used in flow.go and token.go - Propagate io.ReadAll error from wrapRequest and skip buffering when GetBody is already set Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
auth:info,auth:token,auth:logout,auth:api-token-login,auth:browser-login,auth:verify-phone-number, andsession:switchas native Go commands, replacing PHP delegationTest plan
auth:browser-loginPKCE flow works end-to-endauth:tokeninjects token into PHP delegation)--yesflag auto-accepts browser login prompt onauth:info🤖 Generated with Claude Code