Fix auth token refresh by passing client credentials#254
Merged
Conversation
refreshLocked was sending RefreshRequest without ClientID/ClientSecret, causing "Missing client_id" errors from the token endpoint. - Resolve client credentials per OAuthType: BC3 loads from client.json, Launchpad uses env vars or built-in defaults - Migrate old credentials: empty OAuthType defaults to "launchpad", empty TokenEndpoint is derived for launchpad (hard error for bc3) - Clear ExpiresAt to 0 when server omits expires_in, preserving the existing contract (ExpiresAt==0 means non-expiring) - Surface the BC3 custom-redirect limitation: DCR credentials from custom-redirect logins are session-only and not persisted, so refresh after process restart fails with a clear diagnostic
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes OAuth token refresh failures by ensuring refresh requests include client credentials, with provider-aware credential resolution and migrations for older stored credential formats.
Changes:
- Update
refreshLockedto migrate missingOAuthType/TokenEndpointand includeClientID/ClientSecretin refresh requests. - Improve handling of missing
expires_inby clearingExpiresAtto0(non-expiring contract). - Add unit tests covering Launchpad/BC3 refresh credential inclusion, migrations, and expiry behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/auth/auth.go | Adds credential migrations and provider-specific client credential resolution for refresh requests; adjusts expiry handling. |
| internal/auth/auth_test.go | Adds new TestRefreshLocked_* cases validating refresh request params and expiry/migration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nching - Guard capturedBody with sync.Mutex in 3 refresh tests (race detector) - Clear BASECAMP_OAUTH_CLIENT_ID/SECRET env vars in launchpad tests - Branch BC3 client load error on os.IsNotExist for custom-redirect explanation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
refreshLockedwas sendingRefreshRequestwithoutClientID/ClientSecret, causing "Missing client_id" errors from the token endpointclient.json, Launchpad uses env vars or built-in defaultsOAuthTypedefaults to"launchpad", emptyTokenEndpointis derived for launchpad (hard error for bc3)ExpiresAtto 0 when server omitsexpires_in, preserving the existing contract (ExpiresAt==0means non-expiring atauth.go:93)auth.go:659), so refresh after restart fails with a clear diagnostic rather than a generic errorScope limitation
Default-redirect BC3 refresh is fixed. Custom-redirect BC3 refresh after process restart remains unsupported because DCR credentials are intentionally not persisted (they'd become stale without the redirect override). The error message now explains this clearly.
Test plan
TestRefreshLocked_LaunchpadSendsClientIDTestRefreshLocked_BC3SendsClientIDTestRefreshLocked_BC3WithoutClientJSONTestRefreshLocked_EmptyOAuthTypeDefaultsToLaunchpadTestRefreshLocked_ClearsExpiresAtWhenServerOmitsbin/cipassesTestRefreshLocked_*(5 tests, all pass with-race)