feat(auth): support OAuth Bearer login alongside PAT#158
Conversation
aprimakina
force-pushed
the
feat/oauth-bearer
branch
from
a2d5415 to
74fdcb8
Compare
aprimakina
force-pushed
the
feat/oauth-bearer
branch
from
74fdcb8 to
04e3662
Compare
aprimakina
force-pushed
the
feat/oauth-bearer
branch
3 times, most recently
from
e36be09 to
493a621
Compare
aprimakina
force-pushed
the
feat/oauth-bearer
branch
from
493a621 to
caba91e
Compare
Askir
left a comment
Askir
left a comment
There was a problem hiding this comment.
I think it looks good generally but maybe fix the analytics values before we merge :D
| analytics.Property("userId", apiKey.IssuingUser.Id), | ||
| analytics.Property("email", string(apiKey.IssuingUser.Email)), | ||
| analytics.Property("planType", apiKey.Project.PlanType), |
There was a problem hiding this comment.
Do we ever set these in the new oAuth based flow? Otherwise our analytics events will be lacking these details for oauth users, which would be a bit sad and break nice dashboards.
There was a problem hiding this comment.
Great catch, we didn't. Identify was only being called from ValidateAPIKey, which is the PAT-only path. Should be fixed with the next commit, except for plan_type. Do you think we need it? I haven't noticed any plan-segmented dashboards under https://us.posthog.com/project/104050/dashboard/636540
| // SetTokenExpiry populates Token.Expiry from the gateway's non-standard | ||
| // `expires_at` (Unix seconds) field. The Go oauth2 library only reads the | ||
| // standard `expires_in`, so without this the stored token's Expiry is zero | ||
| // and TokenSource.Valid() always returns true. | ||
| func SetTokenExpiry(token *oauth2.Token) { | ||
| switch v := token.Extra("expires_at").(type) { | ||
| case float64: | ||
| if v > 0 { | ||
| token.Expiry = time.Unix(int64(v), 0) | ||
| } | ||
| case int64: | ||
| if v > 0 { | ||
| token.Expiry = time.Unix(v, 0) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Any thoughts about changing this (or atleast adding the expires_in field) on the backend instead if this is the oauth standard? :D
There was a problem hiding this comment.
| func defaultGetStoredCredentials() (*config.Credentials, error) { | ||
| // Honor a PAT-shaped test override when present; production leaves it nil. | ||
| if GetCredentials != nil { | ||
| if apiKey, projectID, err := GetCredentials(); err == nil && apiKey != "" { | ||
| return &config.Credentials{APIKey: apiKey, ProjectID: projectID}, nil | ||
| } | ||
| } | ||
| return config.GetStoredCredentials() | ||
| } | ||
|
|
There was a problem hiding this comment.
It'd be nice if we could clean this up at least in a follow up and change all the tests to override the correct method with maybe even an oauth token.
| return projects[0].ID, nil | ||
| return projects[0].Id, nil | ||
| default: | ||
| return selectProjectInteractively(projects, l.out) |
There was a problem hiding this comment.
This is the only way we allow setting a new projectId right? In theory with oauth you could change projects without having to go through the login command again. Not sure if it's worth allowing this via an extra command, a cli ENV var or a new flag on each command e.g. --projectId? Fine for now but maybe a convenience thing especially as we think about multi project orgs. @nathanjcochran any opinion here?
There was a problem hiding this comment.
Yeah, imo, we should probably have a command for switching projects (e.g. tiger project switch?), which shows this same interactive menu by default (or takes a project ID as an argument), and stores the user's choice in the keyring, like it does now when you log in. That way, users could switch projects without logging in again (assuming they authenticated with OAuth - the command should probably show an error/warning if you try to run it with API key auth). Could also support other tiger project sub-commands in the future, like tiger project list.
I think we could also bring back the global --project-id flag (and a corresponding TIGER_PROJECT_ID env var), which could be used to override the saved project ID on a per-command basis without changing what's saved in the keyring. Note that the --project-id flag and TIGER_PROJECT_ID env var were part of the original CLI design, but were removed in this PR because we couldn't support switching projects while using API keys/client credentials for auth (which are inherently project-scoped).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aprimakina
force-pushed
the
feat/oauth-bearer
branch
from
ace5dda to
d3d1430
Compare
- Identify OAuth users for analytics on login, mirroring the PAT path's ValidateAPIKey. planType is omitted until the gateway returns it on the oauth /auth/info branch. - Build the token-authenticated client once in loginWithOAuth and reuse it for project selection and analytics identification (was constructed twice). - Route OAuth project-list errors through common.ExitWithErrorFromStatusCode so backend messages and exit codes surface in the CLI. - Reuse the pooled getHTTPClient in the OAuth client so Bearer requests and token refreshes share its connection limits. - Export analytics.Enabled() to skip the extra /auth/info round-trip when analytics is disabled. - Drop the deprecated credential test seam: GetStoredCredentials is now the single accessor. Migrate test sites to mockTestPAT/mockNotLoggedIn helpers, remove config.GetCredentials, and add OAuth-credential coverage asserting the client sends a Bearer token. - Restore doc comments dropped during the initial refactor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aprimakina
force-pushed
the
feat/oauth-bearer
branch
from
d3d1430 to
030d9f0
Compare
3 participants
Add OAuth (PKCE) authentication to the CLI and consolidate how auth info is
fetched, while keeping PAT / API-key auth fully supported.
Changes
tiger auth loginnow defaults to an interactive browser flow. PAT auth still works via--public-key/--secret-keyflags orTIGER_PUBLIC_KEY/TIGER_SECRET_KEY.auth logout: revokes the refresh token server-side for OAuth sessions.