-
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?
Changes from all commits
08c8258
c83f676
fd62ff4
e5c5c9a
f690169
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { type Logger } from "../logging/logger"; | ||
| import { type ClientRegistrationResponse } from "../oauth/types"; | ||
| import { toSafeHost } from "../util"; | ||
|
|
||
| import type { Memento, SecretStorage, Disposable } from "vscode"; | ||
|
|
@@ -8,6 +9,11 @@ import type { Deployment } from "../deployment/types"; | |
| // Each deployment has its own key to ensure atomic operations (multiple windows | ||
| // writing to a shared key could drop data) and to receive proper VS Code events. | ||
| const SESSION_KEY_PREFIX = "coder.session."; | ||
| const OAUTH_CLIENT_PREFIX = "coder.oauth.client."; | ||
|
|
||
| type SecretKeyPrefix = typeof SESSION_KEY_PREFIX | typeof OAUTH_CLIENT_PREFIX; | ||
|
|
||
| const OAUTH_CALLBACK_KEY = "coder.oauthCallback"; | ||
|
|
||
| const CURRENT_DEPLOYMENT_KEY = "coder.currentDeployment"; | ||
|
|
||
|
|
@@ -20,9 +26,22 @@ export interface CurrentDeploymentState { | |
| deployment: Deployment | null; | ||
| } | ||
|
|
||
| /** | ||
| * OAuth token data stored alongside session auth. | ||
| * When present, indicates the session is authenticated via OAuth. | ||
| */ | ||
| export interface OAuthTokenData { | ||
| token_type: "Bearer"; | ||
| refresh_token?: string; | ||
| scope?: string; | ||
| expiry_timestamp: number; | ||
| } | ||
|
|
||
| export interface SessionAuth { | ||
| url: string; | ||
| token: string; | ||
| /** If present, this session uses OAuth authentication */ | ||
| oauth?: OAuthTokenData; | ||
| } | ||
|
|
||
| // Tracks when a deployment was last accessed for LRU pruning. | ||
|
|
@@ -31,13 +50,57 @@ interface DeploymentUsage { | |
| lastAccessedAt: string; | ||
| } | ||
|
|
||
| interface OAuthCallbackData { | ||
| state: string; | ||
| code: string | null; | ||
| error: string | null; | ||
| } | ||
|
|
||
| export class SecretsManager { | ||
| constructor( | ||
| private readonly secrets: SecretStorage, | ||
| private readonly memento: Memento, | ||
| private readonly logger: Logger, | ||
| ) {} | ||
|
|
||
| private buildKey(prefix: SecretKeyPrefix, safeHostname: string): string { | ||
| return `${prefix}${safeHostname || "<legacy>"}`; | ||
| } | ||
|
|
||
| private async getSecret<T>( | ||
| prefix: SecretKeyPrefix, | ||
| safeHostname: string, | ||
| ): Promise<T | undefined> { | ||
| try { | ||
| const data = await this.secrets.get(this.buildKey(prefix, safeHostname)); | ||
| if (!data) { | ||
| return undefined; | ||
| } | ||
| return JSON.parse(data) as T; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } | ||
|
|
||
| private async setSecret<T>( | ||
| prefix: SecretKeyPrefix, | ||
| safeHostname: string, | ||
| value: T, | ||
| ): Promise<void> { | ||
| await this.secrets.store( | ||
| this.buildKey(prefix, safeHostname), | ||
| JSON.stringify(value), | ||
| ); | ||
| await this.recordDeploymentAccess(safeHostname); | ||
| } | ||
|
|
||
| private async clearSecret( | ||
| prefix: SecretKeyPrefix, | ||
| safeHostname: string, | ||
| ): Promise<void> { | ||
| await this.secrets.delete(this.buildKey(prefix, safeHostname)); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the current deployment and triggers a cross-window sync event. | ||
| */ | ||
|
|
@@ -104,7 +167,7 @@ export class SecretsManager { | |
| safeHostname: string, | ||
| listener: (auth: SessionAuth | undefined) => void | Promise<void>, | ||
| ): Disposable { | ||
| const sessionKey = this.getSessionKey(safeHostname); | ||
| const sessionKey = this.buildKey(SESSION_KEY_PREFIX, safeHostname); | ||
| return this.secrets.onDidChange(async (e) => { | ||
| if (e.key !== sessionKey) { | ||
| return; | ||
|
|
@@ -118,39 +181,27 @@ export class SecretsManager { | |
| }); | ||
| } | ||
|
|
||
| public async getSessionAuth( | ||
| public getSessionAuth( | ||
| safeHostname: string, | ||
| ): Promise<SessionAuth | undefined> { | ||
| const sessionKey = this.getSessionKey(safeHostname); | ||
| try { | ||
| const data = await this.secrets.get(sessionKey); | ||
| if (!data) { | ||
| return undefined; | ||
| } | ||
| return JSON.parse(data) as SessionAuth; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| return this.getSecret<SessionAuth>(SESSION_KEY_PREFIX, safeHostname); | ||
| } | ||
|
|
||
| public async setSessionAuth( | ||
| safeHostname: string, | ||
| auth: SessionAuth, | ||
| ): Promise<void> { | ||
| const sessionKey = this.getSessionKey(safeHostname); | ||
| // Extract only url and token before serializing | ||
| const state: SessionAuth = { url: auth.url, token: auth.token }; | ||
| await this.secrets.store(sessionKey, JSON.stringify(state)); | ||
| await this.recordDeploymentAccess(safeHostname); | ||
| } | ||
|
|
||
| private async clearSessionAuth(safeHostname: string): Promise<void> { | ||
| const sessionKey = this.getSessionKey(safeHostname); | ||
| await this.secrets.delete(sessionKey); | ||
| // Extract relevant fields before serializing | ||
| const state: SessionAuth = { | ||
| url: auth.url, | ||
| token: auth.token, | ||
| ...(auth.oauth && { oauth: auth.oauth }), | ||
| }; | ||
| await this.setSecret(SESSION_KEY_PREFIX, safeHostname, state); | ||
| } | ||
|
|
||
| private getSessionKey(safeHostname: string): string { | ||
| return `${SESSION_KEY_PREFIX}${safeHostname || "<legacy>"}`; | ||
| private clearSessionAuth(safeHostname: string): Promise<void> { | ||
| return this.clearSecret(SESSION_KEY_PREFIX, safeHostname); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -181,7 +232,10 @@ export class SecretsManager { | |
| * Clear all auth data for a deployment and remove it from the usage list. | ||
| */ | ||
| public async clearAllAuthData(safeHostname: string): Promise<void> { | ||
| await this.clearSessionAuth(safeHostname); | ||
| await Promise.all([ | ||
| this.clearSessionAuth(safeHostname), | ||
| this.clearOAuthClientRegistration(safeHostname), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have not gone through the whole code yet, but my first impression here is that it feels racy to store the auth/oauth-related details in two separate keys. For example using the refresh token in the auth key against the wrong client in the client key. Could/should they be stored together so they can be updated atomically?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There will always be some racy stuff here because we have two kinds of synchronization: inter-process, and intra-process. The latter we can sync easily but the former is synced due to the secrets API. If we combine those two keys into the same key, then it would mean if we want to update only one field we have to do the following:
|
||
| ]); | ||
| const usage = this.getDeploymentUsage().filter( | ||
| (u) => u.safeHostname !== safeHostname, | ||
| ); | ||
|
|
@@ -234,4 +288,64 @@ export class SecretsManager { | |
|
|
||
| return safeHostname; | ||
| } | ||
|
|
||
| /** | ||
| * Write an OAuth callback result to secrets storage. | ||
| * Used for cross-window communication when OAuth callback arrives in a different window. | ||
| */ | ||
| public async setOAuthCallback(data: OAuthCallbackData): Promise<void> { | ||
| await this.secrets.store(OAUTH_CALLBACK_KEY, JSON.stringify(data)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, we are not actually needing to store the callback response, we are just using the secret store for the event handling, to pass the data from the uri handler to the waiting authorizer right? Wondering if we should emit directly to registered handlers and skip the extra step of storing the state, if that is right. Also could be potentially weird if two instances are trying to log in and both write this key. Maybe it should even go on some uri handler class, like
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, this is because when you have a |
||
| } | ||
|
|
||
| /** | ||
| * Listen for OAuth callback results from any VS Code window. | ||
| * The listener receives the state parameter, code (if success), and error (if failed). | ||
| */ | ||
| public onDidChangeOAuthCallback( | ||
| listener: (data: OAuthCallbackData) => void, | ||
| ): Disposable { | ||
| return this.secrets.onDidChange(async (e) => { | ||
| if (e.key !== OAUTH_CALLBACK_KEY) { | ||
| return; | ||
| } | ||
|
|
||
| let parsed: OAuthCallbackData; | ||
| try { | ||
| const data = await this.secrets.get(OAUTH_CALLBACK_KEY); | ||
| if (!data) { | ||
| return; | ||
| } | ||
| parsed = JSON.parse(data) as OAuthCallbackData; | ||
| } catch (err) { | ||
| this.logger.error("Failed to parse OAuth callback data", err); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| listener(parsed); | ||
| } catch (err) { | ||
| this.logger.error("Error in onDidChangeOAuthCallback listener", err); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| public getOAuthClientRegistration( | ||
| safeHostname: string, | ||
| ): Promise<ClientRegistrationResponse | undefined> { | ||
| return this.getSecret<ClientRegistrationResponse>( | ||
| OAUTH_CLIENT_PREFIX, | ||
| safeHostname, | ||
| ); | ||
| } | ||
|
|
||
| public setOAuthClientRegistration( | ||
| safeHostname: string, | ||
| registration: ClientRegistrationResponse, | ||
| ): Promise<void> { | ||
| return this.setSecret(OAUTH_CLIENT_PREFIX, safeHostname, registration); | ||
| } | ||
|
|
||
| public clearOAuthClientRegistration(safeHostname: string): Promise<void> { | ||
| return this.clearSecret(OAUTH_CLIENT_PREFIX, safeHostname); | ||
| } | ||
| } | ||
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
SessionAuthwas a superset of what we store or a different type, but we are converting it to anotherSessionAuthand it seems to have no extra fields so could we not serialize it directly?Uh oh!
There was an error while loading. Please reload this page.
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.stringifywould 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)