Skip to content

Commit f734007

Browse files
committed
feat: make keyring opt-in and store token at login
- Default `coder.useKeyring` to false (opt-in) due to shared logout side effects between CLI and IDE - Update setting description to document sync behavior and version requirements - Store token to OS keyring at login time so the CLI picks it up immediately - Show cancellable progress notification for all CLI keyring invocations during login - Migrate CliCredentialManager public API to options bag pattern with signal and keyringOnly support
1 parent 55e8486 commit f734007

File tree

12 files changed

+364
-59
lines changed

12 files changed

+364
-59
lines changed

CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,13 @@
22

33
## Unreleased
44

5+
### Changed
6+
7+
- `coder.useKeyring` is now opt-in (default: false). Keyring storage requires CLI >= 2.29.0 for
8+
storage and logout sync, and >= 2.31.0 for syncing login from CLI to VS Code.
9+
- Session tokens are now saved to the OS keyring at login time (when enabled and CLI >= 2.29.0),
10+
not only when connecting to a workspace.
11+
512
### Added
613

714
- Proxy log directory now defaults to the extension's global storage when `coder.proxyLogDirectory`

package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,9 +157,9 @@
157157
}
158158
},
159159
"coder.useKeyring": {
160-
"markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of plaintext files. Requires CLI >= 2.29.0. Has no effect on Linux.",
160+
"markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of plaintext files. Requires CLI >= 2.29.0 (>= 2.31.0 to sync login from CLI to VS Code). This will attempt to sync between the CLI and VS Code since they share the same keyring entry. It will log you out of the CLI if you log out of the IDE, and vice versa. Has no effect on Linux.",
161161
"type": "boolean",
162-
"default": true
162+
"default": false
163163
},
164164
"coder.httpClientLogLevel": {
165165
"markdownDescription": "Controls the verbosity of HTTP client logging. This affects what details are logged for each HTTP request and response.",

src/cliConfig.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,9 @@ function isFlag(item: string, name: string): boolean {
6868
export function isKeyringEnabled(
6969
configs: Pick<WorkspaceConfiguration, "get">,
7070
): boolean {
71-
return isKeyringSupported() && configs.get<boolean>("coder.useKeyring", true);
71+
return (
72+
isKeyringSupported() && configs.get<boolean>("coder.useKeyring", false)
73+
);
7274
}
7375

7476
/**

src/core/cliCredentialManager.ts

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { execFile } from "node:child_process";
22
import fs from "node:fs/promises";
3+
import os from "node:os";
34
import path from "node:path";
45
import { promisify } from "node:util";
56
import * as semver from "semver";
@@ -34,7 +35,8 @@ export type BinaryResolver = (deploymentUrl: string) => Promise<string>;
3435
* Returns true on platforms where the OS keyring is supported (macOS, Windows).
3536
*/
3637
export function isKeyringSupported(): boolean {
37-
return process.platform === "darwin" || process.platform === "win32";
38+
const platform = os.platform();
39+
return platform === "darwin" || platform === "win32";
3840
}
3941

4042
/**
@@ -54,19 +56,25 @@ export class CliCredentialManager {
5456
* files under --global-config.
5557
*
5658
* Keyring and files are mutually exclusive — never both.
59+
*
60+
* When `keyringOnly` is set, silently returns if the keyring is unavailable
61+
* instead of falling back to file storage.
5762
*/
5863
public async storeToken(
5964
url: string,
6065
token: string,
6166
configs: Pick<WorkspaceConfiguration, "get">,
62-
signal?: AbortSignal,
67+
options?: { signal?: AbortSignal; keyringOnly?: boolean },
6368
): Promise<void> {
6469
const binPath = await this.resolveKeyringBinary(
6570
url,
6671
configs,
6772
"keyringAuth",
6873
);
6974
if (!binPath) {
75+
if (options?.keyringOnly) {
76+
return;
77+
}
7078
await this.writeCredentialFiles(url, token);
7179
return;
7280
}
@@ -80,7 +88,7 @@ export class CliCredentialManager {
8088
try {
8189
await this.execWithTimeout(binPath, args, {
8290
env: { ...process.env, CODER_SESSION_TOKEN: token },
83-
signal,
91+
signal: options?.signal,
8492
});
8593
this.logger.info("Stored token via CLI for", url);
8694
} catch (error) {
@@ -96,6 +104,7 @@ export class CliCredentialManager {
96104
public async readToken(
97105
url: string,
98106
configs: Pick<WorkspaceConfiguration, "get">,
107+
options?: { signal?: AbortSignal },
99108
): Promise<string | undefined> {
100109
let binPath: string | undefined;
101110
try {
@@ -114,10 +123,15 @@ export class CliCredentialManager {
114123

115124
const args = [...getHeaderArgs(configs), "login", "token", "--url", url];
116125
try {
117-
const { stdout } = await this.execWithTimeout(binPath, args);
126+
const { stdout } = await this.execWithTimeout(binPath, args, {
127+
signal: options?.signal,
128+
});
118129
const token = stdout.trim();
119130
return token || undefined;
120131
} catch (error) {
132+
if ((error as Error).name === "AbortError") {
133+
throw error;
134+
}
121135
this.logger.warn("Failed to read token via CLI:", error);
122136
return undefined;
123137
}
@@ -130,11 +144,11 @@ export class CliCredentialManager {
130144
public async deleteToken(
131145
url: string,
132146
configs: Pick<WorkspaceConfiguration, "get">,
133-
signal?: AbortSignal,
147+
options?: { signal?: AbortSignal },
134148
): Promise<void> {
135149
await Promise.all([
136150
this.deleteCredentialFiles(url),
137-
this.deleteKeyringToken(url, configs, signal),
151+
this.deleteKeyringToken(url, configs, options?.signal),
138152
]);
139153
}
140154

src/core/cliManager.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@ import * as semver from "semver";
1010
import * as vscode from "vscode";
1111

1212
import { errToStr } from "../api/api-helper";
13+
import { isKeyringEnabled } from "../cliConfig";
1314
import * as pgp from "../pgp";
14-
import { withCancellableProgress } from "../progress";
15+
import { withCancellableProgress, withOptionalProgress } from "../progress";
1516
import { tempFilePath, toSafeHost } from "../util";
1617
import { vscodeProposed } from "../vscodeProposed";
1718

@@ -759,13 +760,13 @@ export class CliManager {
759760
}
760761

761762
const result = await withCancellableProgress(
763+
({ signal }) =>
764+
this.cliCredentialManager.storeToken(url, token, configs, { signal }),
762765
{
763766
location: vscode.ProgressLocation.Notification,
764767
title: `Storing credentials for ${url}`,
765768
cancellable: true,
766769
},
767-
({ signal }) =>
768-
this.cliCredentialManager.storeToken(url, token, configs, signal),
769770
);
770771
if (result.ok) {
771772
return;
@@ -783,14 +784,15 @@ export class CliManager {
783784
*/
784785
public async clearCredentials(url: string): Promise<void> {
785786
const configs = vscode.workspace.getConfiguration();
786-
const result = await withCancellableProgress(
787+
const result = await withOptionalProgress(
788+
({ signal }) =>
789+
this.cliCredentialManager.deleteToken(url, configs, { signal }),
787790
{
791+
enabled: isKeyringEnabled(configs),
788792
location: vscode.ProgressLocation.Notification,
789793
title: `Removing credentials for ${url}`,
790794
cancellable: true,
791795
},
792-
({ signal }) =>
793-
this.cliCredentialManager.deleteToken(url, configs, signal),
794796
);
795797
if (result.ok) {
796798
return;

src/login/loginCoordinator.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@ import * as vscode from "vscode";
44

55
import { CoderApi } from "../api/coderApi";
66
import { needToken } from "../api/utils";
7+
import { isKeyringEnabled } from "../cliConfig";
78
import { CertificateError } from "../error/certificateError";
89
import { OAuthAuthorizer } from "../oauth/authorizer";
910
import { buildOAuthTokenData } from "../oauth/utils";
11+
import { withOptionalProgress } from "../progress";
1012
import { maybeAskAuthMethod, maybeAskUrl } from "../promptUtils";
1113
import { vscodeProposed } from "../vscodeProposed";
1214

@@ -147,6 +149,19 @@ export class LoginCoordinator implements vscode.Disposable {
147149
oauth: result.oauth, // undefined for non-OAuth logins
148150
});
149151
await this.mementoManager.addToUrlHistory(url);
152+
153+
if (result.token) {
154+
this.cliCredentialManager
155+
.storeToken(url, result.token, vscode.workspace.getConfiguration(), {
156+
keyringOnly: true,
157+
})
158+
.catch((error) => {
159+
this.logger.warn(
160+
"Failed to store token in keyring at login:",
161+
error,
162+
);
163+
});
164+
}
150165
}
151166
}
152167

@@ -243,10 +258,20 @@ export class LoginCoordinator implements vscode.Disposable {
243258
}
244259

245260
// Try keyring token (picks up tokens written by `coder login` in the terminal)
246-
const keyringToken = await this.cliCredentialManager.readToken(
247-
deployment.url,
248-
vscode.workspace.getConfiguration(),
261+
const configs = vscode.workspace.getConfiguration();
262+
const keyringResult = await withOptionalProgress(
263+
({ signal }) =>
264+
this.cliCredentialManager.readToken(deployment.url, configs, {
265+
signal,
266+
}),
267+
{
268+
enabled: isKeyringEnabled(configs),
269+
location: vscode.ProgressLocation.Notification,
270+
title: "Reading token from OS keyring...",
271+
cancellable: true,
272+
},
249273
);
274+
const keyringToken = keyringResult.ok ? keyringResult.value : undefined;
250275
if (
251276
keyringToken &&
252277
keyringToken !== providedToken &&

src/progress.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ export interface ProgressContext {
1717
* `{ cancelled: true }`.
1818
*/
1919
export function withCancellableProgress<T>(
20-
options: vscode.ProgressOptions & { cancellable: true },
2120
fn: (ctx: ProgressContext) => Promise<T>,
21+
options: vscode.ProgressOptions & { cancellable: true },
2222
): Thenable<ProgressResult<T>> {
2323
return vscode.window.withProgress(
2424
options,
@@ -40,6 +40,32 @@ export function withCancellableProgress<T>(
4040
);
4141
}
4242

43+
/**
44+
* Like withCancellableProgress, but only shows the progress notification when
45+
* `enabled` is true. When false, runs the function directly without UI.
46+
* Returns ProgressResult<T> in both cases for uniform call-site handling.
47+
*/
48+
export async function withOptionalProgress<T>(
49+
fn: (ctx: ProgressContext) => Promise<T>,
50+
options: vscode.ProgressOptions & { cancellable: true; enabled: boolean },
51+
): Promise<ProgressResult<T>> {
52+
if (options.enabled) {
53+
return withCancellableProgress(fn, options);
54+
}
55+
try {
56+
const noop = () => {
57+
// No-op: progress reporting is disabled.
58+
};
59+
const value = await fn({
60+
progress: { report: noop },
61+
signal: new AbortController().signal,
62+
});
63+
return { ok: true, value };
64+
} catch (error) {
65+
return { ok: false, cancelled: false, error };
66+
}
67+
}
68+
4369
/**
4470
* Run a task inside a VS Code progress notification (no cancellation).
4571
* A thin wrapper over `vscode.window.withProgress` that passes only the

test/unit/cliConfig.test.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1+
import * as os from "node:os";
12
import * as semver from "semver";
2-
import { afterEach, it, expect, describe, vi } from "vitest";
3+
import { it, expect, describe, vi } from "vitest";
34

45
import {
56
type CliAuth,
@@ -14,16 +15,14 @@ import { featureSetForVersion } from "@/featureSet";
1415
import { MockConfigurationProvider } from "../mocks/testHelpers";
1516
import { isWindows } from "../utils/platform";
1617

18+
vi.mock("node:os");
19+
1720
const globalConfigAuth: CliAuth = {
1821
mode: "global-config",
1922
configDir: "/config/dir",
2023
};
2124

2225
describe("cliConfig", () => {
23-
afterEach(() => {
24-
vi.unstubAllGlobals();
25-
});
26-
2726
describe("getGlobalFlags", () => {
2827
const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" };
2928

@@ -224,6 +223,12 @@ describe("cliConfig", () => {
224223
useKeyring: boolean;
225224
expected: boolean;
226225
}
226+
it("returns false on darwin when setting is unset (default)", () => {
227+
vi.mocked(os.platform).mockReturnValue("darwin");
228+
const config = new MockConfigurationProvider();
229+
expect(isKeyringEnabled(config)).toBe(false);
230+
});
231+
227232
it.each<KeyringEnabledCase>([
228233
{ platform: "darwin", useKeyring: true, expected: true },
229234
{ platform: "win32", useKeyring: true, expected: true },
@@ -232,7 +237,7 @@ describe("cliConfig", () => {
232237
])(
233238
"returns $expected on $platform with useKeyring=$useKeyring",
234239
({ platform, useKeyring, expected }) => {
235-
vi.stubGlobal("process", { ...process, platform });
240+
vi.mocked(os.platform).mockReturnValue(platform);
236241
const config = new MockConfigurationProvider();
237242
config.set("coder.useKeyring", useKeyring);
238243
expect(isKeyringEnabled(config)).toBe(expected);
@@ -242,7 +247,7 @@ describe("cliConfig", () => {
242247

243248
describe("resolveCliAuth", () => {
244249
it("returns url mode when keyring should be used", () => {
245-
vi.stubGlobal("process", { ...process, platform: "darwin" });
250+
vi.mocked(os.platform).mockReturnValue("darwin");
246251
const config = new MockConfigurationProvider();
247252
config.set("coder.useKeyring", true);
248253
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
@@ -259,7 +264,7 @@ describe("cliConfig", () => {
259264
});
260265

261266
it("returns global-config mode when keyring should not be used", () => {
262-
vi.stubGlobal("process", { ...process, platform: "linux" });
267+
vi.mocked(os.platform).mockReturnValue("linux");
263268
const config = new MockConfigurationProvider();
264269
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
265270
const auth = resolveCliAuth(

0 commit comments

Comments
 (0)