diff --git a/package.json b/package.json index 519ba06..8e15ad5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@stephendolan/ynab-cli", - "version": "2.8.2", + "version": "2.8.3", "description": "A command-line interface for You Need a Budget (YNAB)", "type": "module", "main": "./dist/cli.js", diff --git a/src/commands/auth.test.ts b/src/commands/auth.test.ts new file mode 100644 index 0000000..2e2da4d --- /dev/null +++ b/src/commands/auth.test.ts @@ -0,0 +1,73 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../lib/api-client.js', () => ({ + client: { checkAuthentication: vi.fn() }, +})); + +vi.mock('../lib/output.js', () => ({ + outputJson: vi.fn(), +})); + +import { client } from '../lib/api-client.js'; +import { outputJson } from '../lib/output.js'; +import { createAuthCommand } from './auth.js'; + +const mockCheckAuthentication = client.checkAuthentication as ReturnType; +const mockOutputJson = outputJson as ReturnType; + +describe('ynab auth status', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + async function runStatus() { + await createAuthCommand().parseAsync(['node', 'auth', 'status']); + } + + it('renders only the authenticated user ID for a valid credential', async () => { + mockCheckAuthentication.mockResolvedValue({ + authenticated: true, + credentialPresent: true, + user: { id: 'user-id', name: 'Jane Doe' }, + token: 'valid-test-token', + }); + + await runStatus(); + + expect(mockOutputJson).toHaveBeenCalledWith({ + authenticated: true, + user: { id: 'user-id' }, + }); + expect(JSON.stringify(mockOutputJson.mock.calls)).not.toContain('valid-test-token'); + }); + + it('renders the invalid-token message without exposing the token', async () => { + mockCheckAuthentication.mockResolvedValue({ + authenticated: false, + credentialPresent: true, + token: 'invalid-test-token', + }); + + await runStatus(); + + expect(mockOutputJson).toHaveBeenCalledWith({ + authenticated: false, + message: 'Token exists but is invalid', + }); + expect(JSON.stringify(mockOutputJson.mock.calls)).not.toContain('invalid-test-token'); + }); + + it('renders the missing-credential message', async () => { + mockCheckAuthentication.mockResolvedValue({ + authenticated: false, + credentialPresent: false, + }); + + await runStatus(); + + expect(mockOutputJson).toHaveBeenCalledWith({ + authenticated: false, + message: 'Not authenticated', + }); + }); +}); diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 6fadfb9..de8a701 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -74,19 +74,17 @@ export function createAuthCommand(): Command { .description('Check authentication status') .action( withErrorHandling(async () => { - const isAuthenticated = await auth.isAuthenticated(); + const status = await client.checkAuthentication(); - if (!isAuthenticated) { - outputJson({ authenticated: false, message: 'Not authenticated' }); + if (!status.authenticated) { + outputJson({ + authenticated: false, + message: status.credentialPresent ? 'Token exists but is invalid' : 'Not authenticated', + }); return; } - try { - const user = await client.getUser(); - outputJson({ authenticated: true, user: { id: user?.id } }); - } catch { - outputJson({ authenticated: false, message: 'Token exists but is invalid' }); - } + outputJson({ authenticated: true, user: { id: status.user?.id } }); }) ); diff --git a/src/lib/api-client.test.ts b/src/lib/api-client.test.ts new file mode 100644 index 0000000..9b42e38 --- /dev/null +++ b/src/lib/api-client.test.ts @@ -0,0 +1,116 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('ynab', () => ({ API: vi.fn() })); + +vi.mock('./auth.js', () => ({ + auth: { resolveCredential: vi.fn() }, +})); + +import * as ynab from 'ynab'; +import { auth } from './auth.js'; +import { YnabClient } from './api-client.js'; + +const mockApiConstructor = ynab.API as unknown as ReturnType; +const mockGetUser = vi.fn(); +const mockResolveCredential = auth.resolveCredential as ReturnType; + +describe('YnabClient authentication status', () => { + const validToken = 'valid-test-token'; + + beforeEach(() => { + vi.clearAllMocks(); + mockGetUser.mockResolvedValue({ data: { user: { id: 'user-id' } } }); + mockApiConstructor.mockImplementation(function () { + return { user: { getUser: mockGetUser } }; + }); + }); + + it('returns authenticated for a valid keychain token', async () => { + mockResolveCredential.mockResolvedValue({ token: validToken, source: 'keychain' }); + + const status = await new YnabClient().checkAuthentication(); + + expect(status).toEqual({ + authenticated: true, + credentialPresent: true, + user: { id: 'user-id' }, + }); + expect(mockApiConstructor).toHaveBeenCalledWith(validToken); + }); + + it('returns authenticated for a valid YNAB_API_KEY', async () => { + mockResolveCredential.mockResolvedValue({ token: validToken, source: 'environment' }); + + const status = await new YnabClient().checkAuthentication(); + + expect(status.authenticated).toBe(true); + expect(mockApiConstructor).toHaveBeenCalledWith(validToken); + }); + + it('returns unauthenticated for an invalid YNAB_API_KEY', async () => { + mockResolveCredential.mockResolvedValue({ token: 'invalid-test-token', source: 'environment' }); + mockGetUser.mockRejectedValue({ + error: { id: '401', name: 'unauthorized', detail: 'Unauthorized' }, + }); + + const status = await new YnabClient().checkAuthentication(); + + expect(status).toEqual({ authenticated: false, credentialPresent: true }); + }); + + it('propagates network failures', async () => { + mockResolveCredential.mockResolvedValue({ token: validToken, source: 'keychain' }); + mockGetUser.mockRejectedValue(new TypeError('fetch failed')); + + await expect(new YnabClient().checkAuthentication()).rejects.toThrow('fetch failed'); + }); + + it('propagates rate limit failures', async () => { + mockResolveCredential.mockResolvedValue({ token: validToken, source: 'keychain' }); + const rateLimitError = { + error: { id: '429', name: 'too_many_requests', detail: 'Too many requests' }, + }; + mockGetUser.mockRejectedValue(rateLimitError); + + await expect(new YnabClient().checkAuthentication()).rejects.toEqual(rateLimitError); + }); + + it('uses the validated API instance after credential rotation', async () => { + const client = new YnabClient(); + mockResolveCredential.mockResolvedValue({ token: 'first-token', source: 'keychain' }); + + await client.checkAuthentication(); + + mockResolveCredential.mockResolvedValue({ token: 'second-token', source: 'keychain' }); + await client.checkAuthentication(); + await client.getUser(); + + expect(mockApiConstructor).toHaveBeenCalledTimes(2); + expect(mockApiConstructor).toHaveBeenLastCalledWith('second-token'); + }); + + it('returns unauthenticated without making a request when no credential exists', async () => { + mockResolveCredential.mockResolvedValue(null); + + const status = await new YnabClient().checkAuthentication(); + + expect(status).toEqual({ authenticated: false, credentialPresent: false }); + expect(mockApiConstructor).not.toHaveBeenCalled(); + expect(mockGetUser).not.toHaveBeenCalled(); + }); + + it('clears the cached API when no credential exists', async () => { + const client = new YnabClient(); + mockResolveCredential.mockResolvedValue({ token: validToken, source: 'keychain' }); + + await client.checkAuthentication(); + + mockResolveCredential.mockResolvedValue(null); + await expect(client.getApi()).rejects.toMatchObject({ statusCode: 401 }); + + mockResolveCredential.mockResolvedValue({ token: validToken, source: 'keychain' }); + await client.getUser(); + + expect(mockApiConstructor).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index 03f2ac6..028aba0 100644 --- a/src/lib/api-client.ts +++ b/src/lib/api-client.ts @@ -1,35 +1,41 @@ import * as ynab from 'ynab'; import { config } from './config.js'; import { YnabCliError, sanitizeApiError } from './errors.js'; -import { auth } from './auth.js'; +import { auth, type ResolvedCredential } from './auth.js'; type TransactionTypeFilter = 'uncategorized' | 'unapproved' | undefined; +function isUnauthorizedError(error: unknown): boolean { + if (typeof error !== 'object' || error === null) { + return false; + } + + const apiError = (error as { error?: unknown }).error; + if (typeof apiError !== 'object' || apiError === null) { + return false; + } + + const { id, name } = apiError as { id?: unknown; name?: unknown }; + return id === '401' && name === 'unauthorized'; +} + export class YnabClient { private api: ynab.API | null = null; + private apiToken: string | null = null; private envVarWarningShown = false; clearApi(): void { this.api = null; + this.apiToken = null; this.envVarWarningShown = false; } - async getApi(): Promise { - if (this.api) { + private getApiForCredential(credential: ResolvedCredential): ynab.API { + if (this.api && this.apiToken === credential.token) { return this.api; } - const keychainToken = await auth.getAccessToken(); - const accessToken = keychainToken || process.env.YNAB_API_KEY || null; - - if (!accessToken) { - throw new YnabCliError( - 'Not authenticated. Please run: ynab auth login or set YNAB_API_KEY environment variable', - 401 - ); - } - - if (!keychainToken && process.env.YNAB_API_KEY && !this.envVarWarningShown) { + if (credential.source === 'environment' && !this.envVarWarningShown) { console.warn( '\x1b[33m⚠️ WARNING: Using YNAB_API_KEY environment variable.\n' + 'Environment variables may be visible to other processes.\n' + @@ -38,10 +44,28 @@ export class YnabClient { this.envVarWarningShown = true; } - this.api = new ynab.API(accessToken); + this.api = new ynab.API(credential.token); + this.apiToken = credential.token; return this.api; } + private async resolveApi(): Promise<{ api: ynab.API; credential: ResolvedCredential }> { + const credential = await auth.resolveCredential(); + if (!credential) { + this.clearApi(); + throw new YnabCliError( + 'Not authenticated. Please run: ynab auth login or set YNAB_API_KEY environment variable', + 401 + ); + } + + return { api: this.getApiForCredential(credential), credential }; + } + + async getApi(): Promise { + return (await this.resolveApi()).api; + } + async getBudgetId(budgetIdOrDefault?: string): Promise { const budgetId = (budgetIdOrDefault && budgetIdOrDefault !== 'default' ? budgetIdOrDefault : undefined) || config.getDefaultBudget() || process.env.YNAB_BUDGET_ID; @@ -61,6 +85,29 @@ export class YnabClient { return response.data.user; } + async checkAuthentication() { + const credential = await auth.resolveCredential(); + if (!credential) { + this.clearApi(); + return { authenticated: false, credentialPresent: false } as const; + } + + try { + const api = this.getApiForCredential(credential); + const response = await api.user.getUser(); + return { + authenticated: true, + credentialPresent: true, + user: response.data.user, + } as const; + } catch (error) { + if (isUnauthorizedError(error)) { + return { authenticated: false, credentialPresent: true } as const; + } + throw error; + } + } + async getBudgets(includeAccounts = false) { const api = await this.getApi(); const response = await api.plans.getPlans(includeAccounts); @@ -367,7 +414,7 @@ export class YnabClient { } async rawApiCall(method: string, path: string, data?: unknown, budgetId?: string) { - await this.getApi(); + const { credential } = await this.resolveApi(); let fullPath = path; if (path.includes('{budget_id}') || path.includes('{plan_id}')) { @@ -376,9 +423,8 @@ export class YnabClient { } const url = `https://api.ynab.com/v1${fullPath}`; - const accessToken = (await auth.getAccessToken()) || process.env.YNAB_API_KEY; const headers = { - Authorization: `Bearer ${accessToken}`, + Authorization: `Bearer ${credential.token}`, 'Content-Type': 'application/json', }; diff --git a/src/lib/auth.test.ts b/src/lib/auth.test.ts index 470ab2d..108c379 100644 --- a/src/lib/auth.test.ts +++ b/src/lib/auth.test.ts @@ -17,6 +17,7 @@ import { config } from './config.js'; describe('AuthManager', () => { let auth: AuthManager; + const originalApiKey = process.env.YNAB_API_KEY; const testToken = 'test-token-abc123'; const updatedToken = 'test-token-xyz789'; @@ -25,10 +26,16 @@ describe('AuthManager', () => { resetKeyringForTesting(); mockEntry.getPassword.mockReturnValue(null); mockEntry.deletePassword.mockReturnValue(false); + delete process.env.YNAB_API_KEY; auth = new AuthManager(); }); afterEach(() => { + if (originalApiKey === undefined) { + delete process.env.YNAB_API_KEY; + } else { + process.env.YNAB_API_KEY = originalApiKey; + } config.clearDefaultBudget(); }); @@ -81,24 +88,45 @@ describe('AuthManager', () => { }); }); - describe('authentication status', () => { - it('should return true when authenticated', async () => { + describe('credential resolution', () => { + it('should resolve a keychain token', async () => { mockEntry.getPassword.mockReturnValue(testToken); - const isAuth = await auth.isAuthenticated(); - expect(isAuth).toBe(true); + + const credential = await auth.resolveCredential(); + + expect(credential).toEqual({ token: testToken, source: 'keychain' }); }); - it('should return false when not authenticated', async () => { - const isAuth = await auth.isAuthenticated(); - expect(isAuth).toBe(false); + it('should resolve YNAB_API_KEY', async () => { + process.env.YNAB_API_KEY = testToken; + + const credential = await auth.resolveCredential(); + + expect(credential).toEqual({ token: testToken, source: 'environment' }); }); - it('should return false after token deletion', async () => { + it('should prefer a keychain token over YNAB_API_KEY', async () => { + mockEntry.getPassword.mockReturnValue(testToken); + process.env.YNAB_API_KEY = updatedToken; + + const credential = await auth.resolveCredential(); + + expect(credential).toEqual({ token: testToken, source: 'keychain' }); + }); + + it('should return null when no credential is configured', async () => { + const credential = await auth.resolveCredential(); + + expect(credential).toBe(null); + }); + + it('should return null after token deletion', async () => { mockEntry.deletePassword.mockReturnValue(true); await auth.deleteAccessToken(); - const isAuth = await auth.isAuthenticated(); - expect(isAuth).toBe(false); + const credential = await auth.resolveCredential(); + + expect(credential).toBe(null); }); }); diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 522dbb2..a3d5c67 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -12,6 +12,11 @@ const KEYRING_UNAVAILABLE_ERROR = let keyring: Entry | null | undefined = undefined; +export interface ResolvedCredential { + token: string; + source: 'keychain' | 'environment'; +} + function getKeyring(): Entry | null { if (keyring !== undefined) { return keyring; @@ -41,6 +46,20 @@ export class AuthManager { return null; } + async resolveCredential(): Promise { + const keychainToken = await this.getAccessToken(); + if (keychainToken) { + return { token: keychainToken, source: 'keychain' }; + } + + const environmentToken = process.env.YNAB_API_KEY; + if (environmentToken) { + return { token: environmentToken, source: 'environment' }; + } + + return null; + } + async setAccessToken(token: string): Promise { const entry = getKeyring(); if (!entry) { @@ -66,10 +85,6 @@ export class AuthManager { return false; } - async isAuthenticated(): Promise { - return (await this.getAccessToken()) !== null; - } - async logout(): Promise { await this.deleteAccessToken(); config.clearDefaultBudget(); diff --git a/src/mcp/server.test.ts b/src/mcp/server.test.ts new file mode 100644 index 0000000..514b3e2 --- /dev/null +++ b/src/mcp/server.test.ts @@ -0,0 +1,45 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../lib/api-client.js', () => ({ + client: { checkAuthentication: vi.fn() }, +})); + +import { client } from '../lib/api-client.js'; +import { checkAuth } from './server.js'; + +const mockCheckAuthentication = client.checkAuthentication as ReturnType; + +describe('MCP check_auth', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('serializes a validated authenticated status without exposing a token', async () => { + mockCheckAuthentication.mockResolvedValue({ + authenticated: true, + credentialPresent: true, + user: { id: 'user-id' }, + token: 'valid-test-token', + }); + + const result = await checkAuth(); + + expect(mockCheckAuthentication).toHaveBeenCalledOnce(); + expect(JSON.parse(result.content[0].text)).toEqual({ authenticated: true }); + expect(result.content[0].text).not.toContain('valid-test-token'); + }); + + it('serializes an unauthenticated status without exposing a token', async () => { + mockCheckAuthentication.mockResolvedValue({ + authenticated: false, + credentialPresent: true, + token: 'invalid-test-token', + }); + + const result = await checkAuth(); + + expect(mockCheckAuthentication).toHaveBeenCalledOnce(); + expect(JSON.parse(result.content[0].text)).toEqual({ authenticated: false }); + expect(result.content[0].text).not.toContain('invalid-test-token'); + }); +}); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 2bc7449..f9aa413 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -2,7 +2,6 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; import { z } from 'zod/v3'; import { client } from '../lib/api-client.js'; -import { auth } from '../lib/auth.js'; import { YnabCliError, sanitizeApiError, sanitizeErrorMessage } from '../lib/errors.js'; import { amountToMilliunits, applyFieldSelection, applyTransactionFilters, convertMilliunitsToAmounts, summarizeTransactions, findTransferCandidates, type SummaryTransaction, type TransactionLike } from '../lib/utils.js'; @@ -38,7 +37,7 @@ const toolRegistry = [ { name: 'delete_scheduled_transaction', description: 'Delete a scheduled transaction' }, { name: 'raw_api_call', description: 'Make a direct YNAB API call' }, { name: 'get_user', description: 'Get information about the authenticated user' }, - { name: 'check_auth', description: 'Check if YNAB authentication is configured' }, + { name: 'check_auth', description: 'Check if YNAB authentication is configured and valid' }, ]; const server = new McpServer({ @@ -100,6 +99,11 @@ function currencyResponse(data: unknown) { return jsonResponse(convertMilliunitsToAmounts(data)); } +export async function checkAuth() { + const status = await client.checkAuthentication(); + return jsonResponse({ authenticated: status.authenticated }); +} + tool( 'list_budgets', 'List all budgets in the YNAB account', @@ -541,9 +545,9 @@ tool( tool( 'check_auth', - 'Check if YNAB authentication is configured', + 'Check if YNAB authentication is configured and valid', {}, - async () => jsonResponse({ authenticated: await auth.isAuthenticated() }) + checkAuth ); tool(