Skip to content

feat: add OAuth2 login/logout with PKCE#15

Draft
kwent wants to merge 16 commits intomasterfrom
features/oauth2-login
Draft

feat: add OAuth2 login/logout with PKCE#15
kwent wants to merge 16 commits intomasterfrom
features/oauth2-login

Conversation

@kwent
Copy link
Copy Markdown
Member

@kwent kwent commented Mar 28, 2026

Summary

  • Adds rootly login and rootly logout commands for browser-based OAuth2 authentication using Authorization Code + PKCE flow
  • Uses magic client_id=rootly-cli so no configuration is needed — just run rootly login
  • Tokens stored in ~/.rootly-cli/tokens.yaml (mode 0600) with auto-refresh via golang.org/x/oauth2 transport
  • API client automatically uses OAuth Bearer tokens when no API key is set, falling back gracefully
  • New internal/oauth/ package wraps golang.org/x/oauth2 with persisting token source, PKCE helpers, and User-Agent transport

Test plan

  • go test ./... — 447 tests pass (31 new across internal/oauth/ and internal/cmd/auth/)
  • Manual: rootly login --api-host=localhost:22166 completes browser flow and stores tokens
  • Manual: rootly incidents list --api-host=localhost:22166/api works with OAuth tokens
  • Manual: rootly logout clears tokens
  • Manual: token auto-refresh works when access_token expires

kwent and others added 13 commits March 27, 2026 17:39
Add browser-based OAuth2 Authorization Code + PKCE flow for CLI authentication.
Uses magic client_id="rootly-cli" so no configuration is needed — just `rootly login`.

- `rootly login` opens browser, runs local callback server on :19797, exchanges code for tokens
- `rootly logout` clears stored tokens
- Tokens stored in ~/.rootly-cli/tokens.yaml (mode 0600) with auto-refresh via oauth2.Transport
- API client falls back to OAuth tokens when no API key is set
- Supports localhost dev servers (http) and production (https with api.->app. rewrite)
- Uses golang.org/x/oauth2 built-in PKCE (GenerateVerifier, S256ChallengeOption, VerifierOption)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Downgrade golang.org/x/oauth2 to v0.28.0 (Go 1.24 compatible)
- Pin go.mod to go 1.24.2 (CI runs Go 1.24)
- Fix errcheck: add _, _ to fmt.Fprint/Fprintf return values
- Fix noctx: use net.ListenConfig.Listen and exec.CommandContext
- Fix staticcheck ST1023: use type inference for transport
- Fix goimports: align config.go struct fields
- Fix noctx in tests: use http.NewRequestWithContext instead of client.Get

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
- errcheck: check fmt.Fprintf return in logout.go
- gocritic/httpNoBody: use http.NoBody instead of nil in test requests
- unconvert: split transport declaration and assignment to satisfy both staticcheck and unconvert

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Remove duplicate deriveAuthBaseURL from client.go and login.go
- Export as oauth.DeriveAuthBaseURL for shared use
- Remove dead `_ = t` assignment in client.go
- Move DeriveAuthBaseURL tests to oauth package

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Remove 24 redundant ensureScheme() calls — endpoint already normalized in NewClient()
- Add oauth.HasTokens() for cheap file-exists check, avoid double LoadTokens() reads
- Replace fragile command-name string matching with Annotations["skipAuth"] pattern
- Update getAPIClient() in all 6 resource packages to use HasTokens()

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Store OAuth tokens under an `oauth` key in ~/.rootly-cli/config.yaml
instead of a separate tokens.yaml file. One config file to rule them all.

- Add OAuthData struct to config.Config with `yaml:"oauth,omitempty"`
- Rewrite oauth/tokens.go to read/write from config.yaml, preserving existing fields
- SaveTokens/ClearTokens now do read-modify-write to preserve api_key, api_host, etc.
- Remove separate tokens.yaml file concept entirely
- Update all tests for new storage format

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Windows uses USERPROFILE for os.UserHomeDir(), not HOME. Add
USERPROFILE to all test setups that override HOME. Also skip
file permission checks on Windows and fix double blank line.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
When --api-host is localhost or 127.0.0.1 without an explicit path,
automatically append /api since the Rails monolith serves under /api/v1.

Before: rootly incidents list --api-host=localhost:22166/api
After:  rootly incidents list --api-host=localhost:22166

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
When the refresh token is expired or revoked, surface a user-friendly
error message instead of a cryptic oauth2 error. Also update CHANGELOG
with all unreleased changes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@kwent
Copy link
Copy Markdown
Member Author

kwent commented Mar 28, 2026

@greptileai

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 28, 2026

Greptile Summary

This PR adds browser-based OAuth2 login/logout (rootly login / rootly logout) using the Authorization Code + PKCE flow, with token auto-refresh via golang.org/x/oauth2. Tokens are persisted in ~/.rootly-cli/config.yaml (mode 0600) alongside existing config. The new internal/oauth/ package wraps the standard library with a persisting token source and user-agent transport. All existing getAPIClient helpers are updated to accept OAuth tokens as a fallback when no API key is set, and the auth-skip logic is cleanly refactored from a hardcoded command-name list to Annotations[\"skipAuth\"].

Key findings:

  • P1 — Token refresh hits the wrong endpoint in production: NewClient in internal/api/client.go passes the already-scheme-prefixed endpoint (e.g. \"https://api.rootly.com\") to oauth.DeriveAuthBaseURL. Because the first branch of that function returns scheme-prefixed URLs unchanged, the api.app. mapping never fires, and oauthCfg.Endpoint.TokenURL becomes \"https://api.rootly.com/oauth/token\" — a different server from the one used during login (\"https://app.rootly.com/oauth/token\"). The access token will work until it expires; after that, auto-refresh silently fails.
  • P2 — Callback port duplicated: callbackPort = \"19797\" in login.go and the port in RedirectURI in oauth/oauth.go are separate constants that must stay in sync.
  • P2 — Misleading comment in persistingTokenSource: The comment claims SaveOAuth2Token is only called on refresh, but it is called on every Token() invocation (every outgoing HTTP request). For a CLI this is harmless but the comment should be corrected."

Confidence Score: 4/5

Safe to merge for initial access-token usage; token auto-refresh will fail in production after expiry due to mismatched OAuth endpoint derivation.

One P1 issue exists: the token refresh URL is derived incorrectly for the standard production workflow, causing silent failures after the access token expires. All other findings are P2 (style/maintenance). The P1 should be fixed before the feature is shipped to users who will rely on long-lived sessions.

internal/api/client.go (DeriveAuthBaseURL call with scheme-prefixed endpoint) and internal/oauth/oauth.go (DeriveAuthBaseURL function — needs to handle scheme-prefixed api.* URLs).

Important Files Changed

Filename Overview
internal/api/client.go OAuth client wiring added; DeriveAuthBaseURL is called with a scheme-prefixed endpoint, bypassing the api.→app. mapping and causing token refresh to target the wrong server in production.
internal/oauth/oauth.go New package: PKCE helpers, state generation, OAuth config builder, and DeriveAuthBaseURL — the api.→app. mapping only applies when the input has no scheme, creating an inconsistency with how the API client calls it.
internal/cmd/auth/login.go New login command: correct PKCE + state flow, browser opener, 5-minute timeout — callback port is hardcoded separately from the RedirectURI constant in oauth.go.
internal/oauth/transport.go Persisting token source and user-agent transport look correct; SaveOAuth2Token is called on every request (not just refresh) contrary to the inline comment.
internal/oauth/tokens.go Token persistence into config.yaml: read-modify-write preserves existing fields, 0600 permissions on write, clear and has-tokens helpers all look correct.
internal/cmd/auth/logout.go Simple logout command that delegates to ClearTokens; skipAuth annotation correctly set.
internal/cmd/root.go Auth-skip logic cleanly moved from hardcoded name list to Annotations["skipAuth"]; inherited flag binding and explicit hyphen→underscore mapping added.
internal/config/config.go OAuthData struct added to Config with omitempty; integrates cleanly with existing YAML persistence.

Sequence Diagram

sequenceDiagram
    participant User
    participant LoginCmd as rootly login
    participant Browser
    participant AuthServer as app.rootly.com
    participant CallbackServer as localhost:19797
    participant TokenFile as ~/.rootly-cli/config.yaml

    User->>LoginCmd: rootly login
    LoginCmd->>LoginCmd: GenerateState() + GenerateVerifier()
    LoginCmd->>CallbackServer: Start HTTP listener
    LoginCmd->>Browser: Open authURL (S256 challenge)
    Browser->>AuthServer: GET /oauth/authorize?code_challenge=...
    AuthServer->>Browser: Redirect to localhost:19797/callback?code=...
    Browser->>CallbackServer: GET /callback?code=X&state=Y
    CallbackServer->>CallbackServer: Validate state
    CallbackServer->>LoginCmd: Send code via channel
    LoginCmd->>AuthServer: POST /oauth/token (code + verifier)
    AuthServer-->>LoginCmd: access_token + refresh_token
    LoginCmd->>TokenFile: SaveOAuth2Token (0600)
    LoginCmd->>User: Login successful!

    Note over User,TokenFile: Subsequent API calls

    participant APIClient as API Client
    participant PTS as persistingTokenSource
    participant RTS as ReuseTokenSource
    participant APIServer as api.rootly.com

    User->>APIClient: rootly incidents list
    APIClient->>PTS: Token()
    PTS->>RTS: Token() [cached if valid]
    RTS-->>PTS: access_token
    PTS->>TokenFile: SaveOAuth2Token (every call)
    PTS-->>APIClient: Bearer token
    APIClient->>APIServer: GET /v1/incidents (Authorization: Bearer ...)
    APIServer-->>APIClient: incidents JSON
    APIClient-->>User: table output
Loading

Comments Outside Diff (2)

  1. internal/api/client.go, line 114-115 (link)

    P1 Token refresh endpoint mismatch for production

    oauth.DeriveAuthBaseURL(cfg.Endpoint) is called with the processed endpoint (e.g. "https://api.rootly.com" — the default in every getAPIClient helper). Because this value already carries an https:// prefix, DeriveAuthBaseURL returns it unchanged (first if branch), so oauthCfg.Endpoint.TokenURL becomes "https://api.rootly.com/oauth/token".

    During rootly login, however, the same helper is called with the bare hostname "api.rootly.com" (no scheme), which triggers the api.app. mapping and produces "https://app.rootly.com/oauth/token".

    The two token endpoints are different servers. Auto-refresh will silently hit the wrong endpoint and fail after the access token expires — the user will get an authentication error with no clear cause.

    Fix option — strip the scheme before deriving the auth base URL:

    // Strip scheme so DeriveAuthBaseURL can apply the api.→app. mapping
    rawHost := cfg.Endpoint
    rawHost = strings.TrimPrefix(rawHost, "https://")
    rawHost = strings.TrimPrefix(rawHost, "http://")
    authBaseURL := oauth.DeriveAuthBaseURL(rawHost)

    Alternatively, teach DeriveAuthBaseURL to recognise https://api.* URLs directly.

  2. internal/oauth/transport.go, line 77-79 (link)

    P2 Misleading comment — SaveOAuth2Token is called on every HTTP request

    The inline comment says:

    // Save on every call — oauth2.ReuseTokenSource ensures this only happens on refresh

    This is inaccurate. oauth2.Transport calls ts.Token() (i.e. persistingTokenSource.Token()) for every outgoing request; the underlying ReuseTokenSource avoids a network round-trip when the cached token is still valid, but it does return the token to persistingTokenSource, which then unconditionally calls SaveOAuth2Token. The result is one config-file write per API call, not just on refresh.

    For a CLI this is harmless, but the comment is misleading. A simple guard would make the intent explicit and avoid the redundant I/O:

    prev := tok.AccessToken
    tok, err = p.base.Token()
    ...
    if tok.AccessToken != prev {  // token was refreshed
        _ = SaveOAuth2Token(tok)
    }

Reviews (1): Last reviewed commit: "🐛 fix: close response body in refresh f..." | Re-trigger Greptile

"golang.org/x/oauth2"

xoauth "github.com/rootlyhq/rootly-cli/internal/oauth"
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Callback port duplicated across two files

callbackPort = "19797" in this file and the port embedded in RedirectURI = "http://localhost:19797/callback" in internal/oauth/oauth.go must always match. If one is updated without the other, the OAuth callback will silently break (the redirect URI registered with the auth server won't match the listener port).

Consider exporting a constant from the oauth package:

// internal/oauth/oauth.go
const (
    CallbackPort = "19797"
    RedirectURI  = "http://localhost:" + CallbackPort + "/callback"
)
// internal/cmd/auth/login.go
listener, err := lc.Listen(ctx, "tcp", "localhost:"+xoauth.CallbackPort)

kwent and others added 3 commits March 28, 2026 09:45
- P1: DeriveAuthBaseURL now strips scheme before applying api.→app.
  mapping, fixing token refresh endpoint mismatch in production
- P2: Deduplicate callback port — single CallbackPort constant in
  oauth package, used by both oauth.go and login.go
- P2: Only persist tokens on actual refresh (compare access_token),
  fix misleading comment about save frequency

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
The Rootly monolith no longer supports magic client IDs. On first login,
POST /oauth/register to obtain a client_id dynamically, cache it in
~/.rootly-cli/config.yaml, and re-register automatically if the authorize
endpoint returns 404 (deleted app).
The monolith no longer uses app.rootly.com — strip the "api." prefix
without prepending "app." for production and staging hosts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant