diff --git a/src/commands/issue.ts b/src/commands/issue.ts index 3ee8955..6851e02 100644 --- a/src/commands/issue.ts +++ b/src/commands/issue.ts @@ -16,7 +16,7 @@ import { } from '../lib/issues-api.js'; import type { IssueData } from '../lib/issues-api.js'; import { buildRepoAtUri } from '../utils/at-uri.js'; -import { requireAuth } from '../utils/auth-helpers.js'; +import { ensureAuthenticated, requireAuth } from '../utils/auth-helpers.js'; import { readBodyInput } from '../utils/body-input.js'; import { formatDate, formatIssueState, outputJson } from '../utils/formatting.js'; import { validateIssueBody, validateIssueTitle } from '../utils/validation.js'; @@ -112,10 +112,7 @@ function createViewCommand(): Command { try { // 1. Validate auth const client = createApiClient(); - if (!(await client.resumeSession())) { - console.error('✗ Not authenticated. Run "tangled auth login" first.'); - process.exit(1); - } + await ensureAuthenticated(client); // 2. Get repo context const context = await getCurrentRepoContext(); @@ -188,10 +185,7 @@ function createEditCommand(): Command { // 2. Validate auth const client = createApiClient(); - if (!(await client.resumeSession())) { - console.error('✗ Not authenticated. Run "tangled auth login" first.'); - process.exit(1); - } + await ensureAuthenticated(client); // 3. Get repo context const context = await getCurrentRepoContext(); @@ -271,10 +265,7 @@ function createCloseCommand(): Command { try { // 1. Validate auth const client = createApiClient(); - if (!(await client.resumeSession())) { - console.error('✗ Not authenticated. Run "tangled auth login" first.'); - process.exit(1); - } + await ensureAuthenticated(client); // 2. Get repo context const context = await getCurrentRepoContext(); @@ -331,10 +322,7 @@ function createReopenCommand(): Command { try { // 1. Validate auth const client = createApiClient(); - if (!(await client.resumeSession())) { - console.error('✗ Not authenticated. Run "tangled auth login" first.'); - process.exit(1); - } + await ensureAuthenticated(client); // 2. Get repo context const context = await getCurrentRepoContext(); @@ -391,10 +379,7 @@ function createDeleteCommand(): Command { .action(async (issueId: string, options: { force?: boolean; json?: string | true }) => { // 1. Validate auth const client = createApiClient(); - if (!(await client.resumeSession())) { - console.error('✗ Not authenticated. Run "tangled auth login" first.'); - process.exit(1); - } + await ensureAuthenticated(client); // 2. Get repo context const context = await getCurrentRepoContext(); @@ -487,10 +472,7 @@ function createCreateCommand(): Command { try { // 1. Validate auth const client = createApiClient(); - if (!(await client.resumeSession())) { - console.error('✗ Not authenticated. Run "tangled auth login" first.'); - process.exit(1); - } + await ensureAuthenticated(client); // 2. Get repo context const context = await getCurrentRepoContext(); @@ -574,10 +556,7 @@ function createListCommand(): Command { try { // 1. Validate auth const client = createApiClient(); - if (!(await client.resumeSession())) { - console.error('✗ Not authenticated. Run "tangled auth login" first.'); - process.exit(1); - } + await ensureAuthenticated(client); // 2. Get repo context const context = await getCurrentRepoContext(); diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index 9f83b73..7843797 100644 --- a/src/lib/api-client.ts +++ b/src/lib/api-client.ts @@ -1,6 +1,7 @@ import { AtpAgent } from '@atproto/api'; import type { AtpSessionData } from '@atproto/api'; import { + KeychainAccessError, clearCurrentSessionMetadata, deleteSession, getCurrentSessionMetadata, @@ -106,7 +107,11 @@ export class TangledApiClient { return true; } catch (error) { - // If resume fails, clear invalid session + if (error instanceof KeychainAccessError) { + // Don't clear credentials — keychain may just be temporarily locked + throw error; + } + // Session data invalid or agent resume failed — clear stale state await clearCurrentSessionMetadata(); return false; } diff --git a/src/lib/session.ts b/src/lib/session.ts index a1dadbf..f1b799b 100644 --- a/src/lib/session.ts +++ b/src/lib/session.ts @@ -3,6 +3,13 @@ import { AsyncEntry } from '@napi-rs/keyring'; const SERVICE_NAME = 'tangled-cli'; +export class KeychainAccessError extends Error { + constructor(message: string) { + super(message); + this.name = 'KeychainAccessError'; + } +} + export interface SessionMetadata { handle: string; did: string; @@ -44,8 +51,8 @@ export async function loadSession(accountId: string): Promise { - const entry = new AsyncEntry(SERVICE_NAME, 'current-session-metadata'); - const serialized = await entry.getPassword(); - if (!serialized) { - return null; + try { + const entry = new AsyncEntry(SERVICE_NAME, 'current-session-metadata'); + const serialized = await entry.getPassword(); + if (!serialized) { + return null; + } + return JSON.parse(serialized) as SessionMetadata; + } catch (error) { + throw new KeychainAccessError( + `Cannot access keychain: ${error instanceof Error ? error.message : 'Unknown error'}` + ); } - return JSON.parse(serialized) as SessionMetadata; } /** diff --git a/src/utils/auth-helpers.ts b/src/utils/auth-helpers.ts index 2d548c7..2eca2bd 100644 --- a/src/utils/auth-helpers.ts +++ b/src/utils/auth-helpers.ts @@ -1,4 +1,6 @@ +import { execSync } from 'node:child_process'; import type { TangledApiClient } from '../lib/api-client.js'; +import { KeychainAccessError } from '../lib/session.js'; /** * Validate that the client is authenticated and has an active session @@ -9,7 +11,7 @@ export async function requireAuth(client: TangledApiClient): Promise<{ did: string; handle: string; }> { - if (!(await client.isAuthenticated())) { + if (!client.isAuthenticated()) { throw new Error('Must be authenticated. Run "tangled auth login" first.'); } @@ -20,3 +22,45 @@ export async function requireAuth(client: TangledApiClient): Promise<{ return session; } + +function tryUnlockKeychain(): boolean { + if (process.platform !== 'darwin') return false; + try { + execSync('security unlock-keychain', { stdio: 'inherit' }); + return true; + } catch { + return false; + } +} + +/** + * Resume session and ensure the client is authenticated. + * On macOS, if the keychain is locked, attempts to unlock it interactively + * via `security unlock-keychain` before falling back to an error message. + * Exits the process with a clear error message if authentication fails. + */ +export async function ensureAuthenticated(client: TangledApiClient): Promise { + try { + const authenticated = await client.resumeSession(); + if (!authenticated) { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + } + } catch (error) { + if (error instanceof KeychainAccessError) { + const unlocked = tryUnlockKeychain(); + if (unlocked) { + try { + const retried = await client.resumeSession(); + if (retried) return; + } catch { + // fall through to error message + } + } + console.error('✗ Cannot access keychain. Please unlock your Mac keychain and try again.'); + console.error(' You can unlock it manually with: security unlock-keychain'); + process.exit(1); + } + throw error; + } +} diff --git a/tests/commands/issue.test.ts b/tests/commands/issue.test.ts index 21695c9..68c2183 100644 --- a/tests/commands/issue.test.ts +++ b/tests/commands/issue.test.ts @@ -163,7 +163,10 @@ describe('issue create command', () => { describe('authentication required', () => { it('should fail when not authenticated', async () => { - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + }); const command = createIssueCommand(); @@ -419,7 +422,10 @@ describe('issue list command', () => { describe('authentication required', () => { it('should fail when not authenticated', async () => { - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + }); const command = createIssueCommand(); @@ -689,7 +695,10 @@ describe('issue view command', () => { }); it('should fail when not authenticated', async () => { - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + }); const command = createIssueCommand(); await expect(command.parseAsync(['node', 'test', 'view', '1'])).rejects.toThrow( @@ -886,7 +895,10 @@ describe('issue edit command', () => { }); it('should fail when not authenticated', async () => { - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + }); const command = createIssueCommand(); await expect( @@ -1028,7 +1040,10 @@ describe('issue close command', () => { }); it('should fail when not authenticated', async () => { - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + }); const command = createIssueCommand(); await expect(command.parseAsync(['node', 'test', 'close', '1'])).rejects.toThrow( @@ -1142,7 +1157,10 @@ describe('issue reopen command', () => { }); it('should fail when not authenticated', async () => { - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + }); const command = createIssueCommand(); await expect(command.parseAsync(['node', 'test', 'reopen', '1'])).rejects.toThrow( @@ -1294,7 +1312,10 @@ describe('issue delete command', () => { }); it('should fail when not authenticated', async () => { - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { + console.error('✗ Not authenticated. Run "tangled auth login" first.'); + process.exit(1); + }); const command = createIssueCommand(); await expect(command.parseAsync(['node', 'test', 'delete', '1', '--force'])).rejects.toThrow( diff --git a/tests/lib/api-client.test.ts b/tests/lib/api-client.test.ts index 1d5c57e..877d463 100644 --- a/tests/lib/api-client.test.ts +++ b/tests/lib/api-client.test.ts @@ -1,6 +1,7 @@ import type { AtpSessionData } from '@atproto/api'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { TangledApiClient } from '../../src/lib/api-client.js'; +import { KeychainAccessError } from '../../src/lib/session.js'; import * as sessionModule from '../../src/lib/session.js'; import { mockSessionData, mockSessionMetadata } from '../helpers/mock-data.js'; @@ -30,15 +31,19 @@ vi.mock('@atproto/api', () => { }; }); -// Mock session management -vi.mock('../../src/lib/session.js', () => ({ - saveSession: vi.fn(), - loadSession: vi.fn(), - deleteSession: vi.fn(), - saveCurrentSessionMetadata: vi.fn(), - getCurrentSessionMetadata: vi.fn(), - clearCurrentSessionMetadata: vi.fn(), -})); +// Mock session management (use importOriginal to preserve KeychainAccessError class) +vi.mock('../../src/lib/session.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + saveSession: vi.fn(), + loadSession: vi.fn(), + deleteSession: vi.fn(), + saveCurrentSessionMetadata: vi.fn(), + getCurrentSessionMetadata: vi.fn(), + clearCurrentSessionMetadata: vi.fn(), + }; +}); describe('TangledApiClient', () => { let client: TangledApiClient; @@ -150,6 +155,15 @@ describe('TangledApiClient', () => { expect(resumed).toBe(false); expect(vi.mocked(sessionModule.clearCurrentSessionMetadata)).toHaveBeenCalled(); }); + + it('should rethrow KeychainAccessError without clearing metadata', async () => { + vi.mocked(sessionModule.getCurrentSessionMetadata).mockRejectedValueOnce( + new KeychainAccessError('Cannot access keychain: locked') + ); + + await expect(client.resumeSession()).rejects.toThrow(KeychainAccessError); + expect(vi.mocked(sessionModule.clearCurrentSessionMetadata)).not.toHaveBeenCalled(); + }); }); describe('isAuthenticated', () => { diff --git a/tests/lib/issues-api.test.ts b/tests/lib/issues-api.test.ts index f052e6c..aa37af9 100644 --- a/tests/lib/issues-api.test.ts +++ b/tests/lib/issues-api.test.ts @@ -30,7 +30,7 @@ const createMockClient = (authenticated = true): TangledApiClient => { }; return { - isAuthenticated: vi.fn(async () => authenticated), + isAuthenticated: vi.fn(() => authenticated), getSession: vi.fn(() => authenticated ? { did: 'did:plc:test123', handle: 'test.bsky.social' } : null ), diff --git a/tests/utils/auth-helpers.test.ts b/tests/utils/auth-helpers.test.ts index f6ab7d8..9370b49 100644 --- a/tests/utils/auth-helpers.test.ts +++ b/tests/utils/auth-helpers.test.ts @@ -1,6 +1,12 @@ -import { describe, expect, it, vi } from 'vitest'; +import { execSync } from 'node:child_process'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { TangledApiClient } from '../../src/lib/api-client.js'; -import { requireAuth } from '../../src/utils/auth-helpers.js'; +import { KeychainAccessError } from '../../src/lib/session.js'; +import { ensureAuthenticated, requireAuth } from '../../src/utils/auth-helpers.js'; + +vi.mock('node:child_process', () => ({ + execSync: vi.fn(), +})); // Mock API client factory const createMockClient = ( @@ -8,7 +14,7 @@ const createMockClient = ( session: { did: string; handle: string } | null ): TangledApiClient => { return { - isAuthenticated: vi.fn(async () => authenticated), + isAuthenticated: vi.fn(() => authenticated), getSession: vi.fn(() => session), } as unknown as TangledApiClient; }; @@ -37,3 +43,101 @@ describe('requireAuth', () => { await expect(requireAuth(mockClient)).rejects.toThrow('No active session found'); }); }); + +describe('ensureAuthenticated', () => { + // biome-ignore lint/suspicious/noExplicitAny: spy instance types vary by platform signature + let mockExit: any; + // biome-ignore lint/suspicious/noExplicitAny: spy instance types vary by platform signature + let mockConsoleError: any; + + beforeEach(() => { + mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + mockConsoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.mocked(execSync).mockReset(); + }); + + afterEach(() => { + mockExit.mockRestore(); + mockConsoleError.mockRestore(); + }); + + it('should return normally when resumeSession succeeds', async () => { + const mockClient = { + resumeSession: vi.fn().mockResolvedValue(true), + } as unknown as TangledApiClient; + + await expect(ensureAuthenticated(mockClient)).resolves.toBeUndefined(); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it('should exit with error when not authenticated', async () => { + const mockClient = { + resumeSession: vi.fn().mockResolvedValue(false), + } as unknown as TangledApiClient; + + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); + expect(mockConsoleError).toHaveBeenCalledWith( + '✗ Not authenticated. Run "tangled auth login" first.' + ); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it('should unlock keychain and retry when KeychainAccessError is thrown', async () => { + const mockClient = { + resumeSession: vi + .fn() + .mockRejectedValueOnce(new KeychainAccessError('locked')) + .mockResolvedValueOnce(true), + } as unknown as TangledApiClient; + + vi.mocked(execSync).mockReturnValue(Buffer.from('')); + + await expect(ensureAuthenticated(mockClient)).resolves.toBeUndefined(); + expect(execSync).toHaveBeenCalledWith('security unlock-keychain', { stdio: 'inherit' }); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it('should exit with keychain error when unlock fails', async () => { + const mockClient = { + resumeSession: vi.fn().mockRejectedValue(new KeychainAccessError('locked')), + } as unknown as TangledApiClient; + + vi.mocked(execSync).mockImplementation(() => { + throw new Error('unlock failed'); + }); + + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); + expect(mockConsoleError).toHaveBeenCalledWith( + '✗ Cannot access keychain. Please unlock your Mac keychain and try again.' + ); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it('should exit with keychain error when unlock succeeds but retry fails', async () => { + const mockClient = { + resumeSession: vi + .fn() + .mockRejectedValueOnce(new KeychainAccessError('locked')) + .mockRejectedValueOnce(new KeychainAccessError('still locked')), + } as unknown as TangledApiClient; + + vi.mocked(execSync).mockReturnValue(Buffer.from('')); + + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); + expect(mockConsoleError).toHaveBeenCalledWith( + '✗ Cannot access keychain. Please unlock your Mac keychain and try again.' + ); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it('should rethrow unexpected errors', async () => { + const mockClient = { + resumeSession: vi.fn().mockRejectedValue(new Error('unexpected network error')), + } as unknown as TangledApiClient; + + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('unexpected network error'); + expect(mockExit).not.toHaveBeenCalled(); + }); +});