From 05904f7c0d5ad6463bd703abd39340fc15247dda Mon Sep 17 00:00:00 2001 From: Ken-ichi Ueda Date: Wed, 3 Jun 2026 11:48:17 -0700 Subject: [PATCH] fix: report auth state based on session validation Instead of trusting cached metadata, it actually ensures the session is valid, and distinguishes between lack of auth and an expired session. Co-authored-by: Claude (claude-opus-4-8) --- src/commands/auth.ts | 25 ++++++++++++++++--- src/utils/auth-helpers.ts | 13 ++++++++-- tests/commands/auth.test.ts | 42 +++++++++++++++++++++++++++++--- tests/utils/auth-helpers.test.ts | 31 ++++++++++++++++++++++- 4 files changed, 101 insertions(+), 10 deletions(-) diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 4d2da00..511b334 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -1,6 +1,6 @@ import { Command } from 'commander'; import { createApiClient } from '../lib/api-client.js'; -import { getCurrentSessionMetadata } from '../lib/session.js'; +import { getCurrentSessionMetadata, KeychainAccessError } from '../lib/session.js'; import { promptForLogin } from '../utils/prompts.js'; /** @@ -78,17 +78,34 @@ export function createAuthCommand(): Command { try { const session = await getCurrentSessionMetadata(); - if (session) { + if (!session) { + console.log('✗ Not authenticated'); + console.log('Run "tangled auth login" to authenticate'); + return; + } + + // Metadata only proves a login happened at some point. Validate the + // stored session actually works (resumeSession refreshes the token) so + // we don't report "Authenticated" for an expired session that every + // other command would reject. + const client = createApiClient(); + const valid = await client.resumeSession(); + + if (valid) { console.log('✓ Authenticated'); console.log(` Handle: @${session.handle}`); console.log(` DID: ${session.did}`); console.log(` PDS: ${session.pds}`); console.log(` Last used: ${new Date(session.lastUsed).toLocaleString()}`); } else { - console.log('✗ Not authenticated'); - console.log('Run "tangled auth login" to authenticate'); + console.log('✗ Session expired'); + console.log('Run "tangled auth login" to re-authenticate'); } } catch (error) { + if (error instanceof KeychainAccessError) { + console.error('✗ Cannot access keychain. Please unlock your keychain and try again.'); + process.exit(1); + } console.error( `✗ Failed to check status: ${error instanceof Error ? error.message : 'Unknown error'}` ); diff --git a/src/utils/auth-helpers.ts b/src/utils/auth-helpers.ts index 2eca2bd..f60a4c2 100644 --- a/src/utils/auth-helpers.ts +++ b/src/utils/auth-helpers.ts @@ -1,6 +1,6 @@ import { execSync } from 'node:child_process'; import type { TangledApiClient } from '../lib/api-client.js'; -import { KeychainAccessError } from '../lib/session.js'; +import { getCurrentSessionMetadata, KeychainAccessError } from '../lib/session.js'; /** * Validate that the client is authenticated and has an active session @@ -43,7 +43,16 @@ export async function ensureAuthenticated(client: TangledApiClient): Promise { + const actual = await importOriginal(); + return { + ...actual, + getCurrentSessionMetadata: vi.fn(), + }; +}); vi.mock('../../src/utils/prompts.js'); describe('Auth Commands', () => { let mockClient: { login: ReturnType; logout: ReturnType; + resumeSession: ReturnType; }; let consoleLogSpy: ReturnType; let consoleErrorSpy: ReturnType; @@ -26,6 +34,7 @@ describe('Auth Commands', () => { mockClient = { login: vi.fn(), logout: vi.fn(), + resumeSession: vi.fn(), }; vi.mocked(apiClientModule.createApiClient).mockReturnValue(mockClient as never); @@ -126,12 +135,14 @@ describe('Auth Commands', () => { }); describe('status command', () => { - it('should show authenticated status with session details', async () => { + it('should show authenticated status when the session validates', async () => { vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(mockSessionMetadata); + mockClient.resumeSession.mockResolvedValue(true); const auth = createAuthCommand(); await auth.parseAsync(['node', 'test', 'status']); + expect(mockClient.resumeSession).toHaveBeenCalled(); expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('Authenticated')); expect(consoleLogSpy).toHaveBeenCalledWith( expect.stringContaining(`@${mockSessionMetadata.handle}`) @@ -139,16 +150,41 @@ describe('Auth Commands', () => { expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining(mockSessionMetadata.did)); }); - it('should show not authenticated status', async () => { + it('should show session expired when metadata exists but resume fails', async () => { + vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(mockSessionMetadata); + mockClient.resumeSession.mockResolvedValue(false); + + const auth = createAuthCommand(); + await auth.parseAsync(['node', 'test', 'status']); + + expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('Session expired')); + expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('tangled auth login')); + // Must not falsely claim the user is authenticated. + expect(consoleLogSpy).not.toHaveBeenCalledWith(expect.stringContaining('✓ Authenticated')); + }); + + it('should show not authenticated status when no metadata exists', async () => { vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(null); const auth = createAuthCommand(); await auth.parseAsync(['node', 'test', 'status']); + expect(mockClient.resumeSession).not.toHaveBeenCalled(); expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('Not authenticated')); expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('tangled auth login')); }); + it('should report a locked keychain distinctly', async () => { + vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(mockSessionMetadata); + mockClient.resumeSession.mockRejectedValue(new KeychainAccessError('locked')); + + const auth = createAuthCommand(); + await expect(auth.parseAsync(['node', 'test', 'status'])).rejects.toThrow('process.exit(1)'); + + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('keychain')); + expect(processExitSpy).toHaveBeenCalledWith(1); + }); + it('should handle status check errors gracefully', async () => { vi.mocked(sessionModule.getCurrentSessionMetadata).mockRejectedValue( new Error('Failed to read session') diff --git a/tests/utils/auth-helpers.test.ts b/tests/utils/auth-helpers.test.ts index 28718b5..00d376f 100644 --- a/tests/utils/auth-helpers.test.ts +++ b/tests/utils/auth-helpers.test.ts @@ -1,6 +1,7 @@ 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 * as sessionModule from '../../src/lib/session.js'; import { KeychainAccessError } from '../../src/lib/session.js'; import { ensureAuthenticated, requireAuth } from '../../src/utils/auth-helpers.js'; @@ -8,6 +9,14 @@ vi.mock('node:child_process', () => ({ execSync: vi.fn(), })); +vi.mock('../../src/lib/session.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getCurrentSessionMetadata: vi.fn(), + }; +}); + // Mock API client factory const createMockClient = ( authenticated: boolean, @@ -56,6 +65,7 @@ describe('ensureAuthenticated', () => { }); mockConsoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); vi.mocked(execSync).mockReset(); + vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(null); }); afterEach(() => { @@ -72,10 +82,11 @@ describe('ensureAuthenticated', () => { expect(mockExit).not.toHaveBeenCalled(); }); - it('should exit with error when not authenticated', async () => { + it('should exit with "not authenticated" when resume fails and no metadata exists', async () => { const mockClient = { resumeSession: vi.fn().mockResolvedValue(false), } as unknown as TangledApiClient; + vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(null); await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); expect(mockConsoleError).toHaveBeenCalledWith( @@ -84,6 +95,24 @@ describe('ensureAuthenticated', () => { expect(mockExit).toHaveBeenCalledWith(1); }); + it('should exit with "session expired" when resume fails but metadata exists', async () => { + const mockClient = { + resumeSession: vi.fn().mockResolvedValue(false), + } as unknown as TangledApiClient; + vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue({ + handle: 'user.bsky.social', + did: 'did:plc:test123', + pds: 'https://bsky.social', + lastUsed: '2024-01-01T00:00:00.000Z', + }); + + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); + expect(mockConsoleError).toHaveBeenCalledWith( + '✗ Session expired. Run "tangled auth login" to re-authenticate.' + ); + expect(mockExit).toHaveBeenCalledWith(1); + }); + it.skipIf(process.platform !== 'darwin')( 'should unlock keychain and retry when KeychainAccessError is thrown', async () => { -- 2.51.2