-
Notifications
You must be signed in to change notification settings - Fork 35
Add OAuth 2.1 authentication support #693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
#633 includes some open threads that I addressed here but unsure about whether to revert or refine so please check open threads out, mainly: |
24ebec5 to
aa0e6b0
Compare
code-asher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed the test code yet but I tried it out and it looks good!
| @@ -0,0 +1,163 @@ | |||
| // OAuth 2.1 Grant Types | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we try to pull these types from coder/coder? Some of these look slightly different, which to me implies these types are wrong (or maybe coder/coder's types are wrong).
For example ClientRegistrationRequest has required redirect_uris while Coder's is optional. Also we have an application_type field that does not seem to exist in either coder/coder nor in RFC 7591 nor 7592? Where does that come from? There may be other mismatches.
coder/coder does lack all the string enum types though unfortunately, maybe we can look into adding those on the coder/coder side at some point.
| // Extract relevant fields before serializing | ||
| const state: SessionAuth = { | ||
| url: auth.url, | ||
| token: auth.token, | ||
| ...(auth.oauth && { oauth: auth.oauth }), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do this? At first I thought maybe SessionAuth was a superset of what we store or a different type, but we are converting it to another SessionAuth and it seems to have no extra fields so could we not serialize it directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I used to pass a super set before and realized that JSON.stringify would serialize extra fields and thus when reading this we get the superset which caused strange hidden issues (I think we should always constrict this to the exact type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose if we removed it now someone could change this to a superset later and forget they should constrict it.
But, if we are going that route we should probably do it for all the properties including nested ones rather than just spread auth.oauth right?
| if (!state) { | ||
| logger.warn("Received OAuth callback with no state parameter"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this happens should we emit so we can abort the auth? Not sure if this is a recoverable error. I suppose the user could try opening the auth page again, but if the state was missing presumably it will still be missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, but we could have multiple windows that potentially each login to a different (or the same) deployment so that's why I'm hesitant about this. Maybe we can show something in the UI though and the user can cancel the flow if they wish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof and there is no way for us to tell which login this callback was for. Bummer.
My intuition is that initiating multiple logins at a time will probably be uncommon, so if we do get a bad state most likely it does refer to whatever login is currently ongoing, and so it might make sense to surface this.
But yeah it does feel really bad that if someone is logging into multiple things at a time and we show some error or warning when really everything is fine.
If there was a way we could only error if the URI was unhandled, so we knew all logins ignored it because of the mismatched state, that could do the trick, because then we could show an error like "got a callback but none of the active logins matched it". But not sure how we would figure that out.
I feel like we leave this for now with a warning and see how it goes.
| this.logger.info("Revoking refresh token"); | ||
|
|
||
| const params: TokenRevocationRequest = { | ||
| token: tokenToRevoke, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are not exposing this type in coder/coder either if we want to do that eventually.
Implements OAuth 2.1 with PKCE as an alternative authentication method to session tokens. When connecting to a Coder deployment that supports OAuth, users can choose between OAuth and legacy token authentication. Key changes: OAuth Flow: - Add OAuthSessionManager to handle the complete OAuth lifecycle: dynamic client registration, PKCE authorization flow, token exchange, automatic refresh, and revocation - Add OAuthMetadataClient to discover and validate OAuth server metadata from the well-known endpoint, ensuring server meets OAuth 2.1 requirements - Handle OAuth callbacks via vscode:// URI handler with cross-window support for when callback arrives in a different VS Code window Token Management: - Store OAuth tokens (access, refresh, expiry) per-deployment in secrets - Store dynamic client registrations per-deployment in secrets - Proactive token refresh when approaching expiry (via response interceptor) - Reactive token refresh on 401 responses with automatic request retry - Handle OAuth errors (invalid_grant, invalid_client) by prompting for re-authentication Integration: - Add auth method selection prompt when server supports OAuth - Attach OAuth interceptors to CoderApi for automatic token refresh - Clear OAuth state when user explicitly chooses token auth - DeploymentManager coordinates OAuth session state with deployment changes Error Handling: - Typed OAuth error classes (InvalidGrantError, InvalidClientError, etc.) - Parse OAuth error responses from token endpoint - Show re-authentication modal for errors requiring user action
- Fix tests after rebase - Add proper OAuth error handling with re-authentication prompts - Remove in-memory token storage, rely on SecretStorage - Attach/detach OAuth interceptor based on auth method - Replace refreshIfAlmostExpired with smarter timer-based refresh - Combine OAuth tokens with session auth storage
- Split OAuthAuthorizer (login flow) from OAuthSessionManager (token lifecycle) - Add axios interceptor for automatic token refresh on 401 - Add comprehensive tests for session manager and interceptor - Rename oauthInterceptor to axiosInterceptor for clarity
- Fix critical issues from self-review - Fix URI handler for OAuth callback across windows - Add more tests for edge cases - Fix rebase conflicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I think we may want to consider an always-attached 401 interceptor, and I wonder if we should remove the clearing from getStoredTokens() to avoid accidentally deleting tokens due to races (we could just return undefined without clearing I think).
Implements OAuth 2.1 with PKCE as an alternative authentication method to session tokens. When connecting to a Coder deployment that supports OAuth, users can choose between OAuth and legacy token authentication.
Key changes:
OAuth Flow:
Token Management:
response interceptortimers)Integration:
Error Handling:
Closes #586