Skip to content

Commit 37266b8

Browse files
committed
feat: add cancellable progress monitors for credential operations
Show VS Code progress notifications with Cancel support during storeToken/deleteToken CLI calls. Thread AbortSignal through the credential manager and add a reusable withCancellableProgress util.
1 parent ac64e3e commit 37266b8

File tree

10 files changed

+691
-351
lines changed

10 files changed

+691
-351
lines changed

src/cliConfig.ts

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,27 @@ export function getGlobalFlags(
3333
: ["--global-config", escapeCommandArg(auth.configDir)];
3434

3535
const raw = getGlobalFlagsRaw(configs);
36+
const filtered = stripManagedFlags(raw);
37+
38+
return [...filtered, ...authFlags, ...getHeaderArgs(configs)];
39+
}
40+
41+
function stripManagedFlags(rawFlags: string[]): string[] {
3642
const filtered: string[] = [];
37-
for (let i = 0; i < raw.length; i++) {
38-
if (isFlag(raw[i], "--use-keyring")) {
43+
for (let i = 0; i < rawFlags.length; i++) {
44+
if (isFlag(rawFlags[i], "--use-keyring")) {
3945
continue;
4046
}
41-
if (isFlag(raw[i], "--global-config")) {
47+
if (isFlag(rawFlags[i], "--global-config")) {
4248
// Skip the next item too when the value is a separate entry.
43-
if (raw[i] === "--global-config") {
49+
if (rawFlags[i] === "--global-config") {
4450
i++;
4551
}
4652
continue;
4753
}
48-
filtered.push(raw[i]);
54+
filtered.push(rawFlags[i]);
4955
}
50-
51-
return [...filtered, ...authFlags, ...getHeaderArgs(configs)];
56+
return filtered;
5257
}
5358

5459
function isFlag(item: string, name: string): boolean {
@@ -66,17 +71,6 @@ export function isKeyringEnabled(
6671
return isKeyringSupported() && configs.get<boolean>("coder.useKeyring", true);
6772
}
6873

69-
/**
70-
* Single source of truth: should the extension use the OS keyring for this session?
71-
* Requires CLI >= 2.29.0, macOS or Windows, and the coder.useKeyring setting enabled.
72-
*/
73-
export function shouldUseKeyring(
74-
configs: Pick<WorkspaceConfiguration, "get">,
75-
featureSet: FeatureSet,
76-
): boolean {
77-
return isKeyringEnabled(configs) && featureSet.keyringAuth;
78-
}
79-
8074
/**
8175
* Resolves how the CLI should authenticate: via the keyring (`--url`) or via
8276
* the global config directory (`--global-config`).
@@ -87,7 +81,7 @@ export function resolveCliAuth(
8781
deploymentUrl: string,
8882
configDir: string,
8983
): CliAuth {
90-
if (shouldUseKeyring(configs, featureSet)) {
84+
if (isKeyringEnabled(configs) && featureSet.keyringAuth) {
9185
return { mode: "url", url: deploymentUrl };
9286
}
9387
return { mode: "global-config", configDir };

src/core/cliCredentialManager.ts

Lines changed: 153 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,32 @@
11
import { execFile } from "node:child_process";
2+
import fs from "node:fs/promises";
3+
import path from "node:path";
24
import { promisify } from "node:util";
5+
import * as semver from "semver";
36

47
import { isKeyringEnabled } from "../cliConfig";
8+
import { featureSetForVersion } from "../featureSet";
59
import { getHeaderArgs } from "../headers";
10+
import { toSafeHost } from "../util";
11+
12+
import * as cliUtils from "./cliUtils";
613

714
import type { WorkspaceConfiguration } from "vscode";
815

916
import type { Logger } from "../logging/logger";
1017

18+
import type { PathResolver } from "./pathResolver";
19+
1120
const execFileAsync = promisify(execFile);
1221

22+
const EXEC_TIMEOUT_MS = 60_000;
23+
const EXEC_LOG_INTERVAL_MS = 5_000;
24+
1325
/**
1426
* Resolves a CLI binary path for a given deployment URL, fetching/downloading
1527
* if needed. Returns the path or throws if unavailable.
1628
*/
17-
export type BinaryResolver = (url: string) => Promise<string>;
29+
export type BinaryResolver = (deploymentUrl: string) => Promise<string>;
1830

1931
/**
2032
* Returns true on platforms where the OS keyring is supported (macOS, Windows).
@@ -24,30 +36,33 @@ export function isKeyringSupported(): boolean {
2436
}
2537

2638
/**
27-
* Delegates credential storage to the Coder CLI. All operations resolve the
28-
* binary via the injected BinaryResolver before invoking it.
39+
* Delegates credential storage to the Coder CLI. Owns all credential
40+
* persistence: keyring-backed (via CLI) and file-based (plaintext).
2941
*/
3042
export class CliCredentialManager {
3143
constructor(
3244
private readonly logger: Logger,
3345
private readonly resolveBinary: BinaryResolver,
46+
private readonly pathResolver: PathResolver,
3447
) {}
3548

3649
/**
37-
* Store a token via `coder login --use-token-as-session`.
38-
* Token is passed via CODER_SESSION_TOKEN env var, never in args.
50+
* Store credentials for a deployment URL. Uses the OS keyring when the
51+
* setting is enabled and the CLI supports it; otherwise writes plaintext
52+
* files under --global-config.
53+
*
54+
* Keyring and files are mutually exclusive — never both.
3955
*/
4056
public async storeToken(
4157
url: string,
4258
token: string,
4359
configs: Pick<WorkspaceConfiguration, "get">,
60+
signal?: AbortSignal,
4461
): Promise<void> {
45-
let binPath: string;
46-
try {
47-
binPath = await this.resolveBinary(url);
48-
} catch (error) {
49-
this.logger.debug("Could not resolve CLI binary for token store:", error);
50-
throw error;
62+
const binPath = await this.resolveKeyringBinary(url, configs);
63+
if (!binPath) {
64+
await this.writeCredentialFiles(url, token);
65+
return;
5166
}
5267

5368
const args = [
@@ -57,12 +72,13 @@ export class CliCredentialManager {
5772
url,
5873
];
5974
try {
60-
await execFileAsync(binPath, args, {
75+
await this.execWithTimeout(binPath, args, {
6176
env: { ...process.env, CODER_SESSION_TOKEN: token },
77+
signal,
6278
});
6379
this.logger.info("Stored token via CLI for", url);
6480
} catch (error) {
65-
this.logger.debug("Failed to store token via CLI:", error);
81+
this.logger.warn("Failed to store token via CLI:", error);
6682
throw error;
6783
}
6884
}
@@ -83,49 +99,160 @@ export class CliCredentialManager {
8399
try {
84100
binPath = await this.resolveBinary(url);
85101
} catch (error) {
86-
this.logger.debug("Could not resolve CLI binary for token read:", error);
102+
this.logger.warn("Could not resolve CLI binary for token read:", error);
87103
return undefined;
88104
}
89105

90106
const args = [...getHeaderArgs(configs), "login", "token", "--url", url];
91107
try {
92-
const { stdout } = await execFileAsync(binPath, args);
108+
const { stdout } = await this.execWithTimeout(binPath, args);
93109
const token = stdout.trim();
94110
return token || undefined;
95111
} catch (error) {
96-
this.logger.debug("Failed to read token via CLI:", error);
112+
this.logger.warn("Failed to read token via CLI:", error);
97113
return undefined;
98114
}
99115
}
100116

101117
/**
102-
* Delete a token via `coder logout --url`. Best-effort: never throws.
118+
* Delete credentials for a deployment. Runs file deletion and keyring
119+
* deletion in parallel, both best-effort (never throws).
103120
*/
104121
public async deleteToken(
105122
url: string,
106123
configs: Pick<WorkspaceConfiguration, "get">,
124+
signal?: AbortSignal,
107125
): Promise<void> {
126+
await Promise.all([
127+
this.deleteCredentialFiles(url),
128+
this.deleteKeyringToken(url, configs, signal),
129+
]);
130+
}
131+
132+
/**
133+
* Resolve a CLI binary for keyring operations. Returns the binary path
134+
* when keyring is enabled in settings and the CLI version supports it,
135+
* or undefined to fall back to file-based storage.
136+
*
137+
* Throws on binary resolution or version-check failure (caller decides
138+
* whether to catch or propagate).
139+
*/
140+
private async resolveKeyringBinary(
141+
url: string,
142+
configs: Pick<WorkspaceConfiguration, "get">,
143+
): Promise<string | undefined> {
108144
if (!isKeyringEnabled(configs)) {
109-
return;
145+
return undefined;
110146
}
147+
const binPath = await this.resolveBinary(url);
148+
const version = semver.parse(await cliUtils.version(binPath));
149+
return featureSetForVersion(version).keyringAuth ? binPath : undefined;
150+
}
111151

112-
let binPath: string;
152+
/**
153+
* Wrap execFileAsync with a 60s timeout and periodic debug logging.
154+
*/
155+
private async execWithTimeout(
156+
binPath: string,
157+
args: string[],
158+
options: { env?: NodeJS.ProcessEnv; signal?: AbortSignal } = {},
159+
): Promise<{ stdout: string; stderr: string }> {
160+
const { signal, ...execOptions } = options;
161+
const timer = setInterval(() => {
162+
this.logger.debug(`CLI command still running: coder ${args[0]} ...`);
163+
}, EXEC_LOG_INTERVAL_MS);
113164
try {
114-
binPath = await this.resolveBinary(url);
165+
return await execFileAsync(binPath, args, {
166+
...execOptions,
167+
timeout: EXEC_TIMEOUT_MS,
168+
signal,
169+
});
170+
} finally {
171+
clearInterval(timer);
172+
}
173+
}
174+
175+
/**
176+
* Write URL and token files under --global-config.
177+
*/
178+
private async writeCredentialFiles(
179+
url: string,
180+
token: string,
181+
): Promise<void> {
182+
const safeHostname = toSafeHost(url);
183+
await Promise.all([
184+
this.atomicWriteFile(this.pathResolver.getUrlPath(safeHostname), url),
185+
this.atomicWriteFile(
186+
this.pathResolver.getSessionTokenPath(safeHostname),
187+
token,
188+
),
189+
]);
190+
}
191+
192+
/**
193+
* Delete URL and token files. Best-effort: never throws.
194+
*/
195+
private async deleteCredentialFiles(url: string): Promise<void> {
196+
const safeHostname = toSafeHost(url);
197+
const paths = [
198+
this.pathResolver.getSessionTokenPath(safeHostname),
199+
this.pathResolver.getUrlPath(safeHostname),
200+
];
201+
await Promise.all(
202+
paths.map((p) =>
203+
fs.rm(p, { force: true }).catch((error) => {
204+
this.logger.warn("Failed to remove credential file", p, error);
205+
}),
206+
),
207+
);
208+
}
209+
210+
/**
211+
* Delete keyring token via `coder logout`. Best-effort: never throws.
212+
*/
213+
private async deleteKeyringToken(
214+
url: string,
215+
configs: Pick<WorkspaceConfiguration, "get">,
216+
signal?: AbortSignal,
217+
): Promise<void> {
218+
let binPath: string | undefined;
219+
try {
220+
binPath = await this.resolveKeyringBinary(url, configs);
115221
} catch (error) {
116-
this.logger.debug(
117-
"Could not resolve CLI binary for token delete:",
118-
error,
119-
);
222+
this.logger.warn("Could not resolve keyring binary for delete:", error);
223+
return;
224+
}
225+
if (!binPath) {
120226
return;
121227
}
122228

123229
const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"];
124230
try {
125-
await execFileAsync(binPath, args);
231+
await this.execWithTimeout(binPath, args, { signal });
126232
this.logger.info("Deleted token via CLI for", url);
127233
} catch (error) {
128-
this.logger.debug("Failed to delete token via CLI:", error);
234+
this.logger.warn("Failed to delete token via CLI:", error);
235+
}
236+
}
237+
238+
/**
239+
* Atomically write content to a file via temp-file + rename.
240+
*/
241+
private async atomicWriteFile(
242+
filePath: string,
243+
content: string,
244+
): Promise<void> {
245+
await fs.mkdir(path.dirname(filePath), { recursive: true });
246+
const tempPath =
247+
filePath + ".temp-" + Math.random().toString(36).substring(8);
248+
try {
249+
await fs.writeFile(tempPath, content, { mode: 0o600 });
250+
await fs.rename(tempPath, filePath);
251+
} catch (err) {
252+
await fs.rm(tempPath, { force: true }).catch((rmErr) => {
253+
this.logger.warn("Failed to delete temp file", tempPath, rmErr);
254+
});
255+
throw err;
129256
}
130257
}
131258
}

0 commit comments

Comments
 (0)