diff --git a/src/commands/login.ts b/src/commands/login.ts index 404852b1..5b73276b 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -3,6 +3,7 @@ import clack from '../utils/clack.js'; import { saveCredentials, getCredentials, getAccessToken, isTokenExpired, updateTokens } from '../lib/credentials.js'; import { getCliAuthClientId, getAuthkitDomain } from '../lib/settings.js'; import { refreshAccessToken } from '../lib/token-refresh-client.js'; +import { logInfo } from '../utils/debug.js'; /** * Parse JWT payload @@ -88,8 +89,8 @@ export async function runLogin(): Promise { const result = await refreshAccessToken(authkitDomain, clientId); if (result.accessToken && result.expiresAt) { updateTokens(result.accessToken, result.expiresAt, result.refreshToken); + logInfo('[login] Session refreshed via refresh token'); clack.log.info(`Already logged in as ${existingCreds.email ?? 'unknown'}`); - clack.log.info('(Session refreshed)'); clack.log.info('Run `workos logout` to log out'); return; } diff --git a/src/lib/config-store.spec.ts b/src/lib/config-store.spec.ts index 734dac86..f7f434e6 100644 --- a/src/lib/config-store.spec.ts +++ b/src/lib/config-store.spec.ts @@ -1,5 +1,14 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { existsSync, readFileSync, unlinkSync, mkdtempSync, rmdirSync, statSync, writeFileSync } from 'node:fs'; +import { + existsSync, + readFileSync, + unlinkSync, + mkdtempSync, + rmdirSync, + statSync, + writeFileSync, + mkdirSync, +} from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; @@ -13,6 +22,44 @@ let testDir: string; let workosDir: string; let configFile: string; +// Mock keyring storage +const mockKeyring = new Map(); +let keyringAvailable = true; + +vi.mock('@napi-rs/keyring', () => ({ + Entry: class MockEntry { + private key: string; + + constructor( + service: string, + private account: string, + ) { + this.key = `${service}:${account}`; + } + + getPassword(): string | null { + if (!keyringAvailable) { + throw new Error('Keyring not available'); + } + return mockKeyring.get(this.key) ?? null; + } + + setPassword(password: string): void { + if (!keyringAvailable) { + throw new Error('Keyring not available'); + } + mockKeyring.set(this.key, password); + } + + deletePassword(): void { + if (!keyringAvailable && mockKeyring.has(this.key)) { + throw new Error('Keyring not available'); + } + mockKeyring.delete(this.key); + } + }, +})); + // Mock os.homedir BEFORE importing config-store module vi.mock('node:os', async (importOriginal) => { const original = await importOriginal(); @@ -36,7 +83,11 @@ describe('config-store', () => { testDir = mkdtempSync(join(tmpdir(), 'config-store-test-')); workosDir = join(testDir, '.workos'); configFile = join(workosDir, 'config.json'); - // Force file-based storage for tests + + // Reset state + mockKeyring.clear(); + keyringAvailable = true; + // Force file-based storage for most tests setInsecureConfigStorage(true); }); @@ -199,4 +250,97 @@ describe('config-store', () => { expect(env?.endpoint).toBe('http://localhost:8001'); }); }); + + describe('keyring storage (default)', () => { + beforeEach(() => { + setInsecureConfigStorage(false); + }); + + it('saves config to keyring', () => { + saveConfig(sampleConfig); + expect(mockKeyring.has('workos-cli:config')).toBe(true); + }); + + it('retrieves config from keyring', () => { + saveConfig(sampleConfig); + const config = getConfig(); + expect(config?.activeEnvironment).toBe('production'); + expect(config?.environments.production.apiKey).toBe('sk_test_abc123'); + }); + + it('does not write file when keyring succeeds', () => { + saveConfig(sampleConfig); + expect(mockKeyring.has('workos-cli:config')).toBe(true); + expect(existsSync(configFile)).toBe(false); + }); + + it('does not delete existing file on keyring success', () => { + // Create a pre-existing file (from a prior fallback write) + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + saveConfig({ ...sampleConfig, activeEnvironment: 'staging' }); + + expect(mockKeyring.has('workos-cli:config')).toBe(true); + expect(existsSync(configFile)).toBe(true); + }); + + it('clears from both keyring and file', () => { + saveConfig(sampleConfig); + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + clearConfig(); + + expect(mockKeyring.has('workos-cli:config')).toBe(false); + expect(existsSync(configFile)).toBe(false); + }); + }); + + describe('file fallback (keyring unavailable)', () => { + beforeEach(() => { + setInsecureConfigStorage(false); + keyringAvailable = false; + }); + + it('falls back to file when keyring unavailable', () => { + saveConfig(sampleConfig); + expect(existsSync(configFile)).toBe(true); + expect(mockKeyring.has('workos-cli:config')).toBe(false); + }); + + it('reads from file when keyring unavailable', () => { + saveConfig(sampleConfig); + const config = getConfig(); + expect(config?.activeEnvironment).toBe('production'); + }); + }); + + describe('migration (file to keyring)', () => { + beforeEach(() => { + setInsecureConfigStorage(false); + }); + + it('migrates file config to keyring on read but keeps file', () => { + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + const config = getConfig(); + + expect(config?.activeEnvironment).toBe('production'); + expect(mockKeyring.has('workos-cli:config')).toBe(true); + expect(existsSync(configFile)).toBe(true); + }); + + it('keeps file if keyring unavailable during migration', () => { + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + keyringAvailable = false; + const config = getConfig(); + + expect(config?.activeEnvironment).toBe('production'); + expect(existsSync(configFile)).toBe(true); + }); + }); }); diff --git a/src/lib/config-store.ts b/src/lib/config-store.ts index f3589082..4c35f881 100644 --- a/src/lib/config-store.ts +++ b/src/lib/config-store.ts @@ -33,9 +33,11 @@ const ACCOUNT_NAME = 'config'; let fallbackWarningShown = false; let forceInsecureStorage = false; +let migrationAttempted = false; export function setInsecureConfigStorage(value: boolean): void { forceInsecureStorage = value; + migrationAttempted = false; } function getConfigDir(): string { @@ -134,8 +136,10 @@ export function getConfig(): CliConfig | null { const fileConfig = readFromFile(); if (fileConfig) { - // Migrate file config to keyring if possible - if (writeToKeyring(fileConfig)) deleteFile(); + if (!migrationAttempted) { + migrationAttempted = true; + writeToKeyring(fileConfig); + } return fileConfig; } @@ -145,9 +149,7 @@ export function getConfig(): CliConfig | null { export function saveConfig(config: CliConfig): void { if (forceInsecureStorage) return writeToFile(config); - if (writeToKeyring(config)) { - deleteFile(); - } else { + if (!writeToKeyring(config)) { showFallbackWarning(); writeToFile(config); } @@ -156,6 +158,7 @@ export function saveConfig(config: CliConfig): void { export function clearConfig(): void { deleteFromKeyring(); deleteFile(); + migrationAttempted = false; } export function getActiveEnvironment(): EnvironmentConfig | null { diff --git a/src/lib/credential-store.spec.ts b/src/lib/credential-store.spec.ts index 141201a5..cff20764 100644 --- a/src/lib/credential-store.spec.ts +++ b/src/lib/credential-store.spec.ts @@ -124,10 +124,22 @@ describe('credential-store', () => { expect(creds?.userId).toBe(validCreds.userId); }); - it('keeps file as durable fallback alongside keyring', () => { + it('does not write file when keyring succeeds', () => { saveCredentials(validCreds); - // Both keyring and file should have credentials + // Keyring has credentials, file should NOT be written + expect(mockKeyring.has('workos-cli:credentials')).toBe(true); + expect(existsSync(credentialsFile)).toBe(false); + }); + + it('does not delete existing file on keyring success', () => { + // Create a pre-existing file (from a prior fallback write) + mkdirSync(installerDir, { recursive: true }); + writeFileSync(credentialsFile, JSON.stringify(validCreds)); + + // Save with keyring available — file should remain untouched + saveCredentials({ ...validCreds, accessToken: 'new_token' }); + expect(mockKeyring.has('workos-cli:credentials')).toBe(true); expect(existsSync(credentialsFile)).toBe(true); }); @@ -236,11 +248,10 @@ describe('credential-store', () => { }); it('hasCredentials only checks file when flag is set', () => { - // Save with default mode (writes to both keyring + file) + // Save with insecure mode (writes to file) + setInsecureStorage(true); saveCredentials(validCreds); - // Enable insecure storage — should still find the file - setInsecureStorage(true); expect(hasCredentials()).toBe(true); }); }); diff --git a/src/lib/credential-store.ts b/src/lib/credential-store.ts index 100ce16e..169c4484 100644 --- a/src/lib/credential-store.ts +++ b/src/lib/credential-store.ts @@ -32,9 +32,11 @@ const ACCOUNT_NAME = 'credentials'; let fallbackWarningShown = false; let forceInsecureStorage = false; +let migrationAttempted = false; export function setInsecureStorage(value: boolean): void { forceInsecureStorage = value; + migrationAttempted = false; } function getCredentialsDir(): string { @@ -145,7 +147,10 @@ export function getCredentials(): Credentials | null { const fileCreds = readFromFile(); if (fileCreds) { - writeToKeyring(fileCreds); + if (!migrationAttempted) { + migrationAttempted = true; + writeToKeyring(fileCreds); + } return fileCreds; } @@ -155,15 +160,16 @@ export function getCredentials(): Credentials | null { export function saveCredentials(creds: Credentials): void { if (forceInsecureStorage) return writeToFile(creds); - writeToFile(creds); if (!writeToKeyring(creds)) { showFallbackWarning(); + writeToFile(creds); } } export function clearCredentials(): void { deleteFromKeyring(); deleteFile(); + migrationAttempted = false; } export function updateTokens(accessToken: string, expiresAt: number, refreshToken?: string): void { diff --git a/src/lib/ensure-auth.spec.ts b/src/lib/ensure-auth.spec.ts index 76a5da24..38f7a9b0 100644 --- a/src/lib/ensure-auth.spec.ts +++ b/src/lib/ensure-auth.spec.ts @@ -304,7 +304,7 @@ describe('ensure-auth', () => { expect(mockRunLogin).toHaveBeenCalledOnce(); }); - it('clears credentials when refresh fails with network error', async () => { + it('preserves credentials when refresh fails with network error', async () => { saveCredentials(expiredAccessCreds); mockRefreshAccessToken.mockResolvedValue({ @@ -313,13 +313,12 @@ describe('ensure-auth', () => { error: 'Network error', }); - // Don't save new creds in login — verify old ones were cleared mockRunLogin.mockImplementation(() => {}); await ensureAuthenticated(); - // Old stale credentials should be gone - expect(hasCredentials()).toBe(false); + // Credentials should be preserved — transient errors shouldn't nuke the session + expect(hasCredentials()).toBe(true); }); it('clears credentials when no refresh token available', async () => { diff --git a/src/lib/ensure-auth.ts b/src/lib/ensure-auth.ts index f54ced4a..8e0be91d 100644 --- a/src/lib/ensure-auth.ts +++ b/src/lib/ensure-auth.ts @@ -2,7 +2,7 @@ * Startup auth guard - ensures valid authentication before command execution. */ -import { getCredentials, updateTokens, hasCredentials, isTokenExpired, clearCredentials } from './credentials.js'; +import { getCredentials, updateTokens, isTokenExpired, clearCredentials } from './credentials.js'; import { refreshAccessToken } from './token-refresh-client.js'; import { getCliAuthClientId, getAuthkitDomain } from './settings.js'; import { runLogin } from '../commands/login.js'; @@ -36,29 +36,17 @@ export async function ensureAuthenticated(): Promise { tokenRefreshed: false, }; - // Case 1: No credentials at all - if (!hasCredentials()) { - if (isNonInteractiveEnvironment()) { - exitWithAuthRequired(); - } - logInfo('[ensure-auth] No credentials found, triggering login'); - await runLogin(); - result.loginTriggered = true; - result.authenticated = hasCredentials(); - return result; - } - + // Case 1: No credentials or invalid credentials const creds = getCredentials(); if (!creds) { - // Credentials file exists but is invalid/empty — clear stale data - clearCredentials(); + clearCredentials(); // Clean up any corrupt/empty files if (isNonInteractiveEnvironment()) { exitWithAuthRequired(); } - logInfo('[ensure-auth] Invalid credentials file, triggering login'); + logInfo('[ensure-auth] No valid credentials found, triggering login'); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; } @@ -94,12 +82,11 @@ export async function ensureAuthenticated(): Promise { logInfo('[ensure-auth] Refresh token expired, triggering login'); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; } - // Network or server error - clear stale creds and try login as fallback - clearCredentials(); + // Network or server error - keep credentials intact for retry if (isNonInteractiveEnvironment()) { exitWithAuthRequired( `Authentication refresh failed (${refreshResult.errorType}). Run \`workos login\` in an interactive terminal.`, @@ -108,7 +95,7 @@ export async function ensureAuthenticated(): Promise { logInfo(`[ensure-auth] Refresh failed (${refreshResult.errorType}), triggering login`); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; } } @@ -121,6 +108,6 @@ export async function ensureAuthenticated(): Promise { logInfo('[ensure-auth] No refresh token, triggering login'); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; }