Skip to content

Commit b2cb10e

Browse files
EhabYEhabY
committed
Self-review
1 parent 697351f commit b2cb10e

File tree

8 files changed

+160
-103
lines changed

8 files changed

+160
-103
lines changed

‎scripts/vendor-keyring.mjs‎

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,61 @@
11
/**
22
* Vendor @napi-rs/keyring into dist/node_modules/ for VSIX packaging.
33
*
4-
* pnpm uses symlinks that vsce can't follow. This script resolves them and
4+
* pnpm uses symlinks that vsce can't follow. This script resolves them and
55
* copies the JS wrapper plus macOS/Windows .node binaries into dist/, where
66
* Node's require() resolution finds them from dist/extension.js.
77
*/
8-
import { cpSync, existsSync, mkdirSync, realpathSync, rmSync } from "node:fs";
9-
import { join, resolve, basename } from "node:path";
8+
import {
9+
cpSync,
10+
existsSync,
11+
mkdirSync,
12+
readdirSync,
13+
realpathSync,
14+
rmSync,
15+
} from "node:fs";
16+
import { join, resolve } from "node:path";
1017

11-
const outputDir = resolve("dist/node_modules/@napi-rs/keyring");
1218
const keyringPkg = resolve("node_modules/@napi-rs/keyring");
19+
const outputDir = resolve("dist/node_modules/@napi-rs/keyring");
1320

1421
if (!existsSync(keyringPkg)) {
15-
console.log("@napi-rs/keyring not found, skipping");
16-
process.exit(0);
22+
console.error("@napi-rs/keyring not found — run pnpm install first");
23+
process.exit(1);
1724
}
1825

26+
// Copy the JS wrapper package (resolving pnpm symlinks)
1927
const resolvedPkg = realpathSync(keyringPkg);
20-
2128
rmSync(outputDir, { recursive: true, force: true });
2229
mkdirSync(outputDir, { recursive: true });
2330
cpSync(resolvedPkg, outputDir, { recursive: true });
2431

25-
// Platform packages are siblings of the resolved keyring package in pnpm's layout.
26-
// Exact file names so the build fails loudly if the native module renames anything.
32+
// Native binary packages live as siblings of the resolved keyring package in
33+
// pnpm's content-addressable store (they aren't hoisted to node_modules).
2734
const siblingsDir = resolve(resolvedPkg, "..");
28-
const binaries = [
29-
"keyring-darwin-arm64/keyring.darwin-arm64.node",
30-
"keyring-darwin-x64/keyring.darwin-x64.node",
31-
"keyring-win32-arm64-msvc/keyring.win32-arm64-msvc.node",
32-
"keyring-win32-x64-msvc/keyring.win32-x64-msvc.node",
35+
const nativePackages = [
36+
"keyring-darwin-arm64",
37+
"keyring-darwin-x64",
38+
"keyring-win32-arm64-msvc",
39+
"keyring-win32-x64-msvc",
3340
];
3441

35-
for (const binary of binaries) {
36-
const symlink = join(siblingsDir, binary);
37-
if (!existsSync(symlink)) {
42+
for (const pkg of nativePackages) {
43+
const pkgDir = join(siblingsDir, pkg);
44+
if (!existsSync(pkgDir)) {
3845
console.error(
39-
`Missing native binary: ${binary}\n` +
40-
"Ensure .npmrc includes supportedArchitectures for all target OS/CPU combinations.",
46+
`Missing native package: ${pkg}\n` +
47+
"Ensure supportedArchitectures in pnpm-workspace.yaml includes all target platforms.",
4148
);
4249
process.exit(1);
4350
}
44-
const src = realpathSync(symlink);
45-
const filename = basename(binary);
46-
const dest = join(outputDir, filename);
47-
cpSync(src, dest);
51+
const nodeFile = readdirSync(pkgDir).find((f) => f.endsWith(".node"));
52+
if (!nodeFile) {
53+
console.error(`No .node binary found in ${pkg}`);
54+
process.exit(1);
55+
}
56+
cpSync(join(pkgDir, nodeFile), join(outputDir, nodeFile));
4857
}
4958

5059
console.log(
51-
`Vendored @napi-rs/keyring with ${binaries.length} platform binaries into dist/`,
60+
`Vendored @napi-rs/keyring with ${nativePackages.length} platform binaries into dist/`,
5261
);

‎src/core/cliManager.ts‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,27 +2,30 @@ import globalAxios, {
22
type AxiosInstance,
33
type AxiosRequestConfig,
44
} from "axios";
5-
import { type Api } from "coder/site/src/api/api";
65
import { createWriteStream, type WriteStream } from "node:fs";
76
import fs from "node:fs/promises";
8-
import { type IncomingMessage } from "node:http";
97
import path from "node:path";
108
import prettyBytes from "pretty-bytes";
119
import * as semver from "semver";
1210
import * as vscode from "vscode";
1311

1412
import { errToStr } from "../api/api-helper";
1513
import { shouldUseKeyring } from "../cliConfig";
16-
import { type FeatureSet } from "../featureSet";
17-
import { type KeyringStore } from "../keyringStore";
18-
import { type Logger } from "../logging/logger";
14+
import { isKeyringSupported, type KeyringStore } from "../keyringStore";
1915
import * as pgp from "../pgp";
2016
import { vscodeProposed } from "../vscodeProposed";
2117

2218
import { BinaryLock } from "./binaryLock";
2319
import * as cliUtils from "./cliUtils";
2420
import * as downloadProgress from "./downloadProgress";
25-
import { type PathResolver } from "./pathResolver";
21+
22+
import type { Api } from "coder/site/src/api/api";
23+
import type { IncomingMessage } from "node:http";
24+
25+
import type { FeatureSet } from "../featureSet";
26+
import type { Logger } from "../logging/logger";
27+
28+
import type { PathResolver } from "./pathResolver";
2629

2730
export class CliManager {
2831
private readonly binaryLock: BinaryLock;
@@ -712,9 +715,9 @@ export class CliManager {
712715
safeHostname: string,
713716
url: string | undefined,
714717
token: string | null,
715-
featureSet?: FeatureSet,
718+
featureSet: FeatureSet,
716719
) {
717-
if (featureSet && shouldUseKeyring(featureSet) && url && token !== null) {
720+
if (shouldUseKeyring(featureSet) && url && token !== null) {
718721
try {
719722
this.keyringStore.setToken(url, token);
720723
this.output.info("Stored token in OS keyring for", url);
@@ -736,11 +739,13 @@ export class CliManager {
736739
* Remove credentials for a deployment from both keyring and file storage.
737740
*/
738741
public async clearCredentials(safeHostname: string): Promise<void> {
739-
try {
740-
this.keyringStore.deleteToken(safeHostname);
741-
this.output.info("Removed keyring token for", safeHostname);
742-
} catch (error) {
743-
this.output.warn("Failed to remove keyring token", error);
742+
if (isKeyringSupported()) {
743+
try {
744+
this.keyringStore.deleteToken(safeHostname);
745+
this.output.info("Removed keyring token for", safeHostname);
746+
} catch (error) {
747+
this.output.warn("Failed to remove keyring token", error);
748+
}
744749
}
745750

746751
const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname);

‎src/keyringStore.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,10 @@ function toHost(deploymentUrl: string): string {
5555
}
5656

5757
/**
58-
* Finds the map key matching a safeHostname (ports stripped). Map keys use
59-
* `new URL().host` which preserves ports, so the fallback strips ports to match.
58+
* Finds the map key matching a safeHostname. VS Code identifies deployments by
59+
* safeHostname (port stripped), while the CLI stores map keys via `toHost`
60+
* which preserves ports. The fallback strips ports from map keys so VS Code's
61+
* port-less hostname still matches a CLI-written entry with a port.
6062
*/
6163
function findMapKey(
6264
map: CredentialMap,
@@ -90,12 +92,12 @@ export function isKeyringSupported(): boolean {
9092
* Windows: raw UTF-8 JSON bytes via setSecret/getSecret
9193
*/
9294
export class KeyringStore {
93-
constructor(
95+
public constructor(
9496
private readonly logger: Logger,
9597
private readonly entryFactory: () => KeyringEntry = createDefaultEntry,
9698
) {}
9799

98-
setToken(deploymentUrl: string, token: string): void {
100+
public setToken(deploymentUrl: string, token: string): void {
99101
this.assertSupported();
100102
const entry = this.entryFactory();
101103
const map = this.readMap(entry);
@@ -104,15 +106,15 @@ export class KeyringStore {
104106
this.writeMap(entry, map);
105107
}
106108

107-
getToken(safeHostname: string): string | undefined {
109+
public getToken(safeHostname: string): string | undefined {
108110
this.assertSupported();
109111
const entry = this.entryFactory();
110112
const map = this.readMap(entry);
111113
const key = findMapKey(map, safeHostname);
112114
return key !== undefined ? map[key].api_token : undefined;
113115
}
114116

115-
deleteToken(safeHostname: string): void {
117+
public deleteToken(safeHostname: string): void {
116118
this.assertSupported();
117119
const entry = this.entryFactory();
118120
const map = this.readMap(entry);

‎src/login/loginCoordinator.ts‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -213,11 +213,13 @@ export class LoginCoordinator implements vscode.Disposable {
213213

214214
// mTLS authentication (no token needed)
215215
if (!needToken(vscode.workspace.getConfiguration())) {
216+
this.logger.debug("Attempting mTLS authentication (no token required)");
216217
return this.tryMtlsAuth(client, isAutoLogin);
217218
}
218219

219220
// Try provided token first
220221
if (providedToken) {
222+
this.logger.debug("Trying provided token");
221223
const result = await this.tryTokenAuth(
222224
client,
223225
providedToken,
@@ -233,32 +235,31 @@ export class LoginCoordinator implements vscode.Disposable {
233235
deployment.safeHostname,
234236
);
235237
if (auth?.token && auth.token !== providedToken) {
238+
this.logger.debug("Trying stored session token");
236239
const result = await this.tryTokenAuth(client, auth.token, isAutoLogin);
237240
if (result !== "unauthorized") {
238241
return result;
239242
}
240243
}
241244

242245
// Try keyring token (picks up tokens written by `coder login` in the terminal)
246+
let keyringToken: string | undefined;
243247
try {
244-
const keyringToken = this.keyringStore.getToken(deployment.safeHostname);
245-
if (
246-
keyringToken &&
247-
keyringToken !== providedToken &&
248-
keyringToken !== auth?.token
249-
) {
250-
const result = await this.tryTokenAuth(
251-
client,
252-
keyringToken,
253-
isAutoLogin,
254-
);
255-
if (result !== "unauthorized") {
256-
return result;
257-
}
258-
}
248+
keyringToken = this.keyringStore.getToken(deployment.safeHostname);
259249
} catch (error) {
260250
this.logger.warn("Failed to read token from keyring", error);
261251
}
252+
if (
253+
keyringToken &&
254+
keyringToken !== providedToken &&
255+
keyringToken !== auth?.token
256+
) {
257+
this.logger.debug("Trying token from OS keyring");
258+
const result = await this.tryTokenAuth(client, keyringToken, isAutoLogin);
259+
if (result !== "unauthorized") {
260+
return result;
261+
}
262+
}
262263

263264
// Prompt user for token
264265
const authMethod = await maybeAskAuthMethod(client);

‎src/remote/remote.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ export class Remote {
176176
}
177177
};
178178

179-
// It could be that the cli config was deleted. If so, ask for the url.
179+
// It could be that the cli config was deleted. If so, ask for the url.
180180
if (
181181
!baseUrlRaw ||
182182
(!token && needToken(vscode.workspace.getConfiguration()))

‎test/unit/cliConfig.test.ts‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -175,48 +175,48 @@ describe("cliConfig", () => {
175175

176176
it("returns true when all conditions are met (macOS, keyringAuth, setting enabled)", () => {
177177
vi.stubGlobal("process", { ...process, platform: "darwin" });
178-
const _config = new MockConfigurationProvider();
179-
_config.set("coder.useKeyring", true);
178+
const config = new MockConfigurationProvider();
179+
config.set("coder.useKeyring", true);
180180
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
181181
expect(shouldUseKeyring(featureSet)).toBe(true);
182182
});
183183

184184
it("returns true when all conditions are met (Windows)", () => {
185185
vi.stubGlobal("process", { ...process, platform: "win32" });
186-
const _config = new MockConfigurationProvider();
187-
_config.set("coder.useKeyring", true);
186+
const config = new MockConfigurationProvider();
187+
config.set("coder.useKeyring", true);
188188
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
189189
expect(shouldUseKeyring(featureSet)).toBe(true);
190190
});
191191

192192
it("returns false on Linux", () => {
193193
vi.stubGlobal("process", { ...process, platform: "linux" });
194-
const _config = new MockConfigurationProvider();
195-
_config.set("coder.useKeyring", true);
194+
const config = new MockConfigurationProvider();
195+
config.set("coder.useKeyring", true);
196196
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
197197
expect(shouldUseKeyring(featureSet)).toBe(false);
198198
});
199199

200200
it("returns false when CLI version is too old", () => {
201201
vi.stubGlobal("process", { ...process, platform: "darwin" });
202-
const _config = new MockConfigurationProvider();
203-
_config.set("coder.useKeyring", true);
202+
const config = new MockConfigurationProvider();
203+
config.set("coder.useKeyring", true);
204204
const featureSet = featureSetForVersion(semver.parse("2.28.0"));
205205
expect(shouldUseKeyring(featureSet)).toBe(false);
206206
});
207207

208208
it("returns false when setting is disabled", () => {
209209
vi.stubGlobal("process", { ...process, platform: "darwin" });
210-
const _config = new MockConfigurationProvider();
211-
_config.set("coder.useKeyring", false);
210+
const config = new MockConfigurationProvider();
211+
config.set("coder.useKeyring", false);
212212
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
213213
expect(shouldUseKeyring(featureSet)).toBe(false);
214214
});
215215

216216
it("returns true for devel prerelease on macOS", () => {
217217
vi.stubGlobal("process", { ...process, platform: "darwin" });
218-
const _config = new MockConfigurationProvider();
219-
_config.set("coder.useKeyring", true);
218+
const config = new MockConfigurationProvider();
219+
config.set("coder.useKeyring", true);
220220
const featureSet = featureSetForVersion(
221221
semver.parse("0.0.0-devel+abc123"),
222222
);
@@ -238,8 +238,8 @@ describe("cliConfig", () => {
238238

239239
it("returns url mode when keyring should be used", () => {
240240
vi.stubGlobal("process", { ...process, platform: "darwin" });
241-
const _config = new MockConfigurationProvider();
242-
_config.set("coder.useKeyring", true);
241+
const config = new MockConfigurationProvider();
242+
config.set("coder.useKeyring", true);
243243
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
244244
const auth = resolveCliAuth(
245245
featureSet,
@@ -254,7 +254,7 @@ describe("cliConfig", () => {
254254

255255
it("returns global-config mode when keyring should not be used", () => {
256256
vi.stubGlobal("process", { ...process, platform: "linux" });
257-
const _config = new MockConfigurationProvider();
257+
new MockConfigurationProvider();
258258
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
259259
const auth = resolveCliAuth(
260260
featureSet,
@@ -269,8 +269,8 @@ describe("cliConfig", () => {
269269

270270
it("returns global-config mode when url is undefined", () => {
271271
vi.stubGlobal("process", { ...process, platform: "darwin" });
272-
const _config = new MockConfigurationProvider();
273-
_config.set("coder.useKeyring", true);
272+
const config = new MockConfigurationProvider();
273+
config.set("coder.useKeyring", true);
274274
const featureSet = featureSetForVersion(semver.parse("2.29.0"));
275275
const auth = resolveCliAuth(featureSet, undefined, "/config/dir");
276276
expect(auth).toEqual({

0 commit comments

Comments
 (0)