From 8ee37d68127a127ef5783547c18c64c0575ac89f Mon Sep 17 00:00:00 2001 From: Ralph Khreish <35776126+Crunchyman-ralph@users.noreply.github.com> Date: Mon, 13 Oct 2025 15:59:22 +0200 Subject: [PATCH] chore: apply requested coderabbit changes --- ...h-manager.test.ts => auth-manager.spec.ts} | 13 +- packages/tm-core/src/auth/auth-manager.ts | 48 +++-- .../tm-core/tests/auth/auth-refresh.test.ts | 176 ++++++++++++++++++ 3 files changed, 214 insertions(+), 23 deletions(-) rename packages/tm-core/src/auth/{auth-manager.test.ts => auth-manager.spec.ts} (92%) create mode 100644 packages/tm-core/tests/auth/auth-refresh.test.ts diff --git a/packages/tm-core/src/auth/auth-manager.test.ts b/packages/tm-core/src/auth/auth-manager.spec.ts similarity index 92% rename from packages/tm-core/src/auth/auth-manager.test.ts rename to packages/tm-core/src/auth/auth-manager.spec.ts index 54ec05ee..b78ce8f3 100644 --- a/packages/tm-core/src/auth/auth-manager.test.ts +++ b/packages/tm-core/src/auth/auth-manager.spec.ts @@ -21,11 +21,16 @@ const CredentialStoreSpy = vi.fn(); vi.mock('./credential-store.js', () => { return { CredentialStore: class { + static getInstance(config?: any) { + return new (this as any)(config); + } + static resetInstance() { + // Mock reset instance method + } constructor(config: any) { CredentialStoreSpy(config); - this.getCredentials = vi.fn(() => null); } - getCredentials() { + getCredentials(_options?: any) { return null; } saveCredentials() {} @@ -85,7 +90,7 @@ describe('AuthManager Singleton', () => { expect(instance1).toBe(instance2); }); - it('should use config on first call', () => { + it('should use config on first call', async () => { const config = { baseUrl: 'https://test.auth.com', configDir: '/test/config', @@ -101,7 +106,7 @@ describe('AuthManager Singleton', () => { // Verify the config is passed to internal components through observable behavior // getCredentials would look in the configured file path - const credentials = instance.getCredentials(); + const credentials = await instance.getCredentials(); expect(credentials).toBeNull(); // File doesn't exist, but config was propagated correctly }); diff --git a/packages/tm-core/src/auth/auth-manager.ts b/packages/tm-core/src/auth/auth-manager.ts index 95f33a6d..b310afb2 100644 --- a/packages/tm-core/src/auth/auth-manager.ts +++ b/packages/tm-core/src/auth/auth-manager.ts @@ -30,6 +30,7 @@ export class AuthManager { private supabaseClient: SupabaseAuthClient; private organizationService?: OrganizationService; private logger = getLogger('AuthManager'); + private refreshPromise: Promise | null = null; private constructor(config?: Partial) { this.credentialStore = CredentialStore.getInstance(config); @@ -82,38 +83,47 @@ export class AuthManager { * Automatically refreshes the token if expired */ async getCredentials(): Promise { - const credentials = this.credentialStore.getCredentials(); + const credentials = this.credentialStore.getCredentials({ allowExpired: true }); - // If credentials exist but are expired, try to refresh if (!credentials) { - const expiredCredentials = this.credentialStore.getCredentials({ - allowExpired: true - }); + return null; + } - // Check if we have any credentials at all - if (!expiredCredentials) { - // No credentials found - return null; + // Check if credentials are expired (with 30-second clock skew buffer) + const CLOCK_SKEW_MS = 30_000; + const isExpired = credentials.expiresAt + ? new Date(credentials.expiresAt).getTime() <= Date.now() + CLOCK_SKEW_MS + : false; + + // If expired and we have a refresh token, attempt refresh + if (isExpired && credentials.refreshToken) { + // Return existing refresh promise if one is in progress + if (this.refreshPromise) { + try { + return await this.refreshPromise; + } catch { + return null; + } } - // Check if refresh token is available - if (!expiredCredentials.refreshToken) { - this.logger.warn( - 'Token expired but no refresh token available. Please re-authenticate.' - ); - return null; - } - - // Attempt refresh try { this.logger.info('Token expired, attempting automatic refresh...'); - return await this.refreshToken(); + this.refreshPromise = this.refreshToken(); + const result = await this.refreshPromise; + return result; } catch (error) { this.logger.warn('Automatic token refresh failed:', error); return null; + } finally { + this.refreshPromise = null; } } + // Return null if expired and no refresh token + if (isExpired) { + return null; + } + return credentials; } diff --git a/packages/tm-core/tests/auth/auth-refresh.test.ts b/packages/tm-core/tests/auth/auth-refresh.test.ts new file mode 100644 index 00000000..1ae50aaf --- /dev/null +++ b/packages/tm-core/tests/auth/auth-refresh.test.ts @@ -0,0 +1,176 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import type { Session } from '@supabase/supabase-js'; +import { AuthManager } from '../../src/auth/auth-manager'; +import { CredentialStore } from '../../src/auth/credential-store'; +import type { AuthCredentials } from '../../src/auth/types'; + +describe('AuthManager Token Refresh', () => { + let authManager: AuthManager; + let credentialStore: CredentialStore; + let tmpDir: string; + let authFile: string; + + beforeEach(() => { + // Reset singletons + AuthManager.resetInstance(); + CredentialStore.resetInstance(); + + // Create temporary directory for test isolation + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'tm-auth-refresh-')); + authFile = path.join(tmpDir, 'auth.json'); + + // Initialize AuthManager with test config (this will create CredentialStore internally) + authManager = AuthManager.getInstance({ + configDir: tmpDir, + configFile: authFile + }); + + // Get the CredentialStore instance that AuthManager created + credentialStore = CredentialStore.getInstance(); + credentialStore.clearCredentials(); + }); + + afterEach(() => { + // Clean up + try { + credentialStore.clearCredentials(); + } catch { + // Ignore cleanup errors + } + AuthManager.resetInstance(); + CredentialStore.resetInstance(); + vi.restoreAllMocks(); + + // Remove temporary directory + if (tmpDir && fs.existsSync(tmpDir)) { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('should not make concurrent refresh requests', async () => { + // Set up expired credentials with refresh token + const expiredCredentials: AuthCredentials = { + token: 'expired_access_token', + refreshToken: 'valid_refresh_token', + userId: 'test-user-id', + email: 'test@example.com', + expiresAt: new Date(Date.now() - 1000).toISOString(), // Expired 1 second ago + savedAt: new Date().toISOString() + }; + + credentialStore.saveCredentials(expiredCredentials); + + // Mock the refreshToken method to track calls + const refreshTokenSpy = vi.spyOn(authManager as any, 'refreshToken'); + const mockSession: Session = { + access_token: 'new_access_token', + refresh_token: 'new_refresh_token', + expires_at: Math.floor(Date.now() / 1000) + 3600, + user: { + id: 'test-user-id', + email: 'test@example.com', + app_metadata: {}, + user_metadata: {}, + aud: 'authenticated', + created_at: new Date().toISOString() + } + }; + + refreshTokenSpy.mockResolvedValue({ + token: mockSession.access_token, + refreshToken: mockSession.refresh_token, + userId: mockSession.user.id, + email: mockSession.user.email, + expiresAt: new Date(mockSession.expires_at! * 1000).toISOString(), + savedAt: new Date().toISOString() + }); + + // Make multiple concurrent calls to getCredentials + const promises = [ + authManager.getCredentials(), + authManager.getCredentials(), + authManager.getCredentials() + ]; + + const results = await Promise.all(promises); + + // Verify all calls returned the same new credentials + expect(results[0]?.token).toBe('new_access_token'); + expect(results[1]?.token).toBe('new_access_token'); + expect(results[2]?.token).toBe('new_access_token'); + + // Verify refreshToken was only called once, not three times + expect(refreshTokenSpy).toHaveBeenCalledTimes(1); + }); + + it('should return valid credentials without attempting refresh', async () => { + // Set up valid (non-expired) credentials + const validCredentials: AuthCredentials = { + token: 'valid_access_token', + refreshToken: 'valid_refresh_token', + userId: 'test-user-id', + email: 'test@example.com', + expiresAt: new Date(Date.now() + 3600000).toISOString(), // Expires in 1 hour + savedAt: new Date().toISOString() + }; + + credentialStore.saveCredentials(validCredentials); + + // Spy on refreshToken to ensure it's not called + const refreshTokenSpy = vi.spyOn(authManager as any, 'refreshToken'); + + const credentials = await authManager.getCredentials(); + + expect(credentials?.token).toBe('valid_access_token'); + expect(refreshTokenSpy).not.toHaveBeenCalled(); + }); + + it('should return null if credentials are expired with no refresh token', async () => { + // Set up expired credentials WITHOUT refresh token + const expiredCredentials: AuthCredentials = { + token: 'expired_access_token', + refreshToken: undefined, + userId: 'test-user-id', + email: 'test@example.com', + expiresAt: new Date(Date.now() - 1000).toISOString(), // Expired 1 second ago + savedAt: new Date().toISOString() + }; + + credentialStore.saveCredentials(expiredCredentials); + + const credentials = await authManager.getCredentials(); + + expect(credentials).toBeNull(); + }); + + it('should return null if no credentials exist', async () => { + const credentials = await authManager.getCredentials(); + expect(credentials).toBeNull(); + }); + + it('should handle refresh failures gracefully', async () => { + // Set up expired credentials with refresh token + const expiredCredentials: AuthCredentials = { + token: 'expired_access_token', + refreshToken: 'invalid_refresh_token', + userId: 'test-user-id', + email: 'test@example.com', + expiresAt: new Date(Date.now() - 1000).toISOString(), + savedAt: new Date().toISOString() + }; + + credentialStore.saveCredentials(expiredCredentials); + + // Mock refreshToken to throw an error + const refreshTokenSpy = vi.spyOn(authManager as any, 'refreshToken'); + refreshTokenSpy.mockRejectedValue(new Error('Refresh failed')); + + const credentials = await authManager.getCredentials(); + + expect(credentials).toBeNull(); + expect(refreshTokenSpy).toHaveBeenCalledTimes(1); + }); +});