Implement OAuth client in VS Code #633

EhabY · 2025-10-27T13:18:54Z

Closes #586

code-asher

I am I think maybe halfway through I need to log off but thought I would submit what I have so far.

code-asher · 2025-12-04T01:51:03Z

src/api/coderApi.ts

+		const hostChanged = currentHost !== host;
+		const tokenChanged = currentToken !== token;


Do we need to handle the case of comparing an empty string to undefined? That could trigger a reconnect or suspend but maybe going from falsey to falsey should be considered as no change.

I would consider this a change, since we can go from undefined to "" due to mTLS authentication

code-asher · 2025-12-04T01:57:13Z

src/api/coderApi.ts

-			const token = this.getAxiosInstance().defaults.headers.common[
-				coderSessionTokenHeader
-			] as string | undefined;
+			return this.createOneWayWebSocket<TData>(socketConfigs);


We have a check for the base URL just above this line but createOneWayWebSocket does this check so it seems like we no longer need the one above here.

Which would make socketFactory little more than a call to createOneWayWebSocket, perhaps that means something can be simplified here.

code-asher · 2025-12-04T02:03:27Z

src/api/coderApi.ts

+		const currentHost = defaults.baseURL;
+		const currentToken = defaults.headers.common[coderSessionTokenHeader];


We grab these often enough that I wonder if we want some getters for the host and token?

code-asher · 2025-12-04T02:04:09Z

src/api/coderApi.ts

 	setHost = (host: string | undefined): void => {
-		const defaults = this.getAxiosInstance().defaults;
-		const currentHost = defaults.baseURL;
-		defaults.baseURL = host;
+		const currentToken = this.getAxiosInstance().defaults.headers.common[
+			coderSessionTokenHeader
+		] as string | undefined;
+		this.setCredentials(host, currentToken);
+	};


setHost appears to be unused except in tests. Can we remove it?

Part of me wonders if we should remove setSessionToken too and only expose setCredentials just to avoid the possibility of someone setting the host and then the token or vice versa, causing two reconnections with the first failing since it temporarily has the wrong token or url.

Yeah that could be better actually, currently we only use setSessionToken for convenience since there are a few cases where host does not change but the token does

code-asher · 2025-12-04T02:11:17Z

src/api/coderApi.ts

-			const httpAgent = await createHttpAgent(
-				vscode.workspace.getConfiguration(),
-			);
+	private async createOneWayWebSocket<TData>(


The existence of createOneWayWebSocket to me implies that createWebSocket is two-way socket, but it actually also creates a one-way web socket, except potentially reconnecting. Maybe we can have just createOneWayWebSocket and it handles enableRetry?

Or, we could rename createWebSocket to createReconnectingOneWayWebSocket or something like that.

code-asher · 2025-12-04T03:35:13Z

src/core/secretsManager.ts

+	}
+
+	public async getSessionAuth(label: string): Promise<SessionAuth | undefined> {
+		if (!label) {


Would this be blank if it was using the old format? But if so always returning undefined seems like it would break old connections (well, it would just require re-authenticating I think, so no big deal).

Although, IMO, it is fine to break those at this point. But then we should remove the concept of a label altogether and just go off the URL.

Agreed, I'll remove all label, url pairs and just rely on url (we can derive the label when need be) - let's just hope URIs are not weird where sometimes they have a trailing slash (/) and sometimes they do not or sometimes they use http and sometimes https

Actually we might need to keep label for the secrets at least (or we can deduce it from the URL), because otherwise our secrets look something like coder.session.https://dev.coder.com compared to coder.session.dev.coder.com. I guess technically there's no limitation on the key format (I think) 🤔

I forgot that the remote authority does not include the protocol so I guess we will need to look it up based on the "label" after all anyway... 😢

code-asher · 2025-12-04T03:37:00Z

src/core/secretsManager.ts

-	INVALID,
+import type { Deployment } from "./deployment";
+
+const SESSION_KEY_PREFIX = "coder.session.";


Thinking out loud, right now we have $key_deployment for each type of secret but would it make sense to have one secret for all the keys?

So instead of:

coder.session.dev.coder.com = "foo" coder.oauth.tokens.dev.coder.com = "foo"

we have something like:

coder.deployments.dev.coder.com = JSON.stringify({ session: "foo", oauth: { tokens: "foo" }, })

We could maybe have a function like update(url: string, secrets: Partial<Secrets>) or something like that, or we could still have the separate update functions like we have now.

I did this at first but it was a headache to ensure concurrent writes and having reliable onDidChange* events. Because there is no atomic way of updating this so we have to do: read -> modify -> write which means if we have two windows that are updating two different settings we might drop some info (also events would need compare old and new values)

Ooh yeah good point, if two instances are updating different parts of the settings. Yeah what you have makes sense. We might want to leave a comment explaining the architecture decision here.

code-asher · 2025-12-04T03:43:26Z

src/core/secretsManager.ts

+		await this.secrets.delete(`${OAUTH_TOKENS_PREFIX}${label}`);
+	}
+
+	public async getOAuthClientRegistration(


Definitely a nit, but I feel like we have a lot of these set/get/delete patterns doing the same thing with different keys that could maybe be squashed into something generic.

code-asher · 2025-12-04T03:45:51Z

src/core/secretsManager.ts

+
+		const existing = await this.getSessionAuth(label);
+		if (existing) {
+			return undefined;


Should we delete the legacy url and token if we have existing auth?

code-asher · 2025-12-04T03:46:32Z

src/core/secretsManager.ts

+		}
+
+		await this.setSessionAuth(label, { url: legacyUrl, token: oldToken });
+		await this.secrets.delete(LEGACY_SESSION_TOKEN_KEY);


Should we also delete the legacy url?

code-asher

I have not tested if this was an existing issue, but when I log in with a token my workspaces are not populated until I manually refresh. Are you seeing the same thing?

I have not tested logging in with oauth yet because xdg-open opens the wrong thing, but that is an issue with my system configuration. 😛 But, could be cool if there was an option in the flow to manually paste in the callback URI as a fallback.

code-asher · 2025-12-05T00:52:23Z

src/oauth/sessionManager.ts

+const AUTH_GRANT_TYPE = "authorization_code" as const;
+const REFRESH_GRANT_TYPE = "refresh_token" as const;
+const RESPONSE_TYPE = "code" as const;
+const PKCE_CHALLENGE_METHOD = "S256" as const;


Why do we need the const cast? TypeScript being dumb in some way I imagine?

code-asher · 2025-12-05T00:53:33Z

src/oauth/sessionManager.ts

+
+		this.refreshTimer = setTimeout(async () => {
+			try {
+				await this.refreshIfAlmostExpired();


If we only refresh when almost expired, why not set a timer directly for that time rather than checking in a loop?

code-asher · 2025-12-05T00:55:09Z

src/oauth/sessionManager.ts

+	 */
+	private hasRequiredScopes(grantedScope: string | undefined): boolean {
+		if (!grantedScope) {
+			// TODO server always returns empty scopes


Yeah I think this was never implemented or at least not merged.

code-asher · 2025-12-05T00:57:12Z

src/oauth/sessionManager.ts

+const DEFAULT_OAUTH_SCOPES = [
+	"workspace:read",
+	"workspace:update",
+	"workspace:start",
+	"workspace:ssh",
+	"workspace:application_connect",
+	"template:read",
+	"user:read_personal",
+].join(" ");


Did you have a way to test if these scopes were sufficient? Not sure if there is a way right now, maybe if we try to generate a key manually with these scopes (I think this can be done on the account page) and paste it in to make sure everything works.

code-asher · 2025-12-05T01:00:10Z

src/oauth/sessionManager.ts

+		registration: ClientRegistrationResponse;
+	}> {
+		const deployment = this.requireDeployment();
+		const client = CoderApi.create(deployment.url, token, this.logger);


Oh hm so some oauth requests are not using the oauth interceptor that handles oauth errors? Looks like exchange uses the plugin client which has the interceptor, and refresh and revoke use this one without the interceptor. But it looks neither of those have their own error handling.

code-asher · 2025-12-05T01:04:25Z

src/oauth/types.ts

@@ -0,0 +1,163 @@
+// OAuth 2.1 Grant Types
+export type GrantType =


Is pulling these types from typesGenerated.ts an option?

code-asher · 2025-12-05T01:09:31Z

src/oauth/sessionManager.ts

@@ -0,0 +1,808 @@
+import { type AxiosInstance } from "axios";


Have we considered using an oauth client library? I could see needing to add more parts of the spec in the future, or oidc or something, and a library could possibly handle it all for us.

code-asher · 2025-12-05T01:16:43Z

src/oauth/sessionManager.ts

+		await this.secretsManager.setOAuthTokens(deployment.label, tokens);
+		await this.secretsManager.setSessionAuth(deployment.label, {


From the outside looking in, it feels weird to me that we set oauth tokens and session auth separately. I think I would expect a single session auth object, and that object would have oauth information as well if it came from oauth.

Or to approach this another way, is there a use case for setting only oauth tokens but not the session auth?

code-asher · 2025-12-05T01:29:36Z

src/oauth/sessionManager.ts

+		progress.report({ message: "registering client...", increment: 10 });
+		const registration = await this.registerClient(axiosInstance, metadata);
+
+		progress.report({ message: "waiting for authorization...", increment: 30 });


Is it possible to add a cancel button? If I close the browser window without authorizing it looks like I am stuck without a way to change my authorization method.

Also if I click login and enter the URL again I wonder if it should kick off the authorization flow again, right now it does nothing so I have no way to reopen the window I closed.

And, if I close the window then try to log into a different deployment, the notification for the previous one still shows but we should probably cancel it when that happens (or disable the login button while auth is ongoing).

EhabY force-pushed the oauth-vs-code branch 2 times, most recently from 78db0ad to b93e027 Compare October 30, 2025 12:46

EhabY force-pushed the oauth-vs-code branch from 639df18 to 10e1593 Compare November 11, 2025 15:24

EhabY force-pushed the oauth-vs-code branch 6 times, most recently from 8e5ad5d to 423742c Compare November 26, 2025 09:34

EhabY force-pushed the oauth-vs-code branch from ea7b658 to 59fecba Compare November 28, 2025 10:17

EhabY requested a review from code-asher December 2, 2025 21:31

EhabY added 19 commits December 3, 2025 13:02

Add simple OAuth command

e2e4d2d

Refactoring into seperate files

f314ce1

Update types

8f1d4c4

Smol refactorings

b348931

Add token refresh and revocation

7139b1c

Add proper scopes

2094026

Hook up into the login/logout logic of the extension

1784b8a

WIP refactoring

ce87ae1

Simplify the OAuth flow

bd8b38c

Add error handling

6e30b1f

Allow callback handling in multiple VS Code windows

eea2957

Improve UX

29feead

Better error handling on 400/401/403 due to auth

d2f5e43

Handle per deployment login and centralized login logic

185f671

Rely on the secrets API + Refactorings

875bb26

Improve login experience

5d49d45

Add WebSocket suspension

9932135

Prune old login records + Add debug flag for deployments

467b2bd

More login synchronization issues

b82de68

Improve login synchronization + Add background oauth refresh

0d0f22a

EhabY force-pushed the oauth-vs-code branch from 03e01fc to 0d0f22a Compare December 3, 2025 10:02

code-asher reviewed Dec 4, 2025

View reviewed changes

code-asher reviewed Dec 5, 2025

View reviewed changes

		const hostChanged = currentHost !== host;
		const tokenChanged = currentToken !== token;

		const currentHost = defaults.baseURL;
		const currentToken = defaults.headers.common[coderSessionTokenHeader];

		@@ -0,0 +1,163 @@
		// OAuth 2.1 Grant Types
		export type GrantType =

		@@ -0,0 +1,808 @@
		import { type AxiosInstance } from "axios";

		await this.secretsManager.setOAuthTokens(deployment.label, tokens);
		await this.secretsManager.setSessionAuth(deployment.label, {

Implement OAuth client in VS Code #633

Are you sure you want to change the base?

Implement OAuth client in VS Code #633

Conversation

EhabY commented Oct 27, 2025

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EhabY Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

code-asher Dec 4, 2025 •

edited

Loading

EhabY Dec 4, 2025 •

edited

Loading

code-asher Dec 4, 2025 •

edited

Loading