From 6bc75c0ac68b59cb10cee70574a689f83e4de768 Mon Sep 17 00:00:00 2001 From: Ralph Khreish <35776126+Crunchyman-ralph@users.noreply.github.com> Date: Wed, 15 Oct 2025 17:32:15 +0200 Subject: [PATCH] fix: auth refresh (#1314) --- .changeset/dirty-hairs-know.md | 5 + apps/cli/src/commands/auth.command.ts | 10 +- apps/cli/src/commands/context.command.ts | 30 +-- .../tm-core/src/auth/auth-manager.spec.ts | 2 +- packages/tm-core/src/auth/auth-manager.ts | 77 ++------ .../tm-core/src/auth/credential-store.spec.ts | 47 +++-- .../tm-core/src/auth/credential-store.test.ts | 47 +++-- packages/tm-core/src/auth/credential-store.ts | 15 +- packages/tm-core/src/auth/oauth-service.ts | 35 +++- .../src/auth/supabase-session-storage.ts | 9 +- .../tm-core/src/clients/supabase-client.ts | 5 +- .../tm-core/src/storage/storage-factory.ts | 4 +- .../tm-core/tests/auth/auth-refresh.test.ts | 87 +++------ .../integration/auth-token-refresh.test.ts | 174 ++++++------------ 14 files changed, 239 insertions(+), 308 deletions(-) create mode 100644 .changeset/dirty-hairs-know.md diff --git a/.changeset/dirty-hairs-know.md b/.changeset/dirty-hairs-know.md new file mode 100644 index 00000000..c222152b --- /dev/null +++ b/.changeset/dirty-hairs-know.md @@ -0,0 +1,5 @@ +--- +"task-master-ai": patch +--- + +Improve auth token refresh flow diff --git a/apps/cli/src/commands/auth.command.ts b/apps/cli/src/commands/auth.command.ts index 39992beb..97fd9683 100644 --- a/apps/cli/src/commands/auth.command.ts +++ b/apps/cli/src/commands/auth.command.ts @@ -143,7 +143,7 @@ export class AuthCommand extends Command { */ private async executeStatus(): Promise { try { - const result = await this.displayStatus(); + const result = this.displayStatus(); this.setLastResult(result); } catch (error: any) { this.handleError(error); @@ -171,8 +171,8 @@ export class AuthCommand extends Command { /** * Display authentication status */ - private async displayStatus(): Promise { - const credentials = await this.authManager.getCredentials(); + private displayStatus(): AuthResult { + const credentials = this.authManager.getCredentials(); console.log(chalk.cyan('\nšŸ” Authentication Status\n')); @@ -325,7 +325,7 @@ export class AuthCommand extends Command { ]); if (!continueAuth) { - const credentials = await this.authManager.getCredentials(); + const credentials = this.authManager.getCredentials(); ui.displaySuccess('Using existing authentication'); if (credentials) { @@ -490,7 +490,7 @@ export class AuthCommand extends Command { /** * Get current credentials (for programmatic usage) */ - getCredentials(): Promise { + getCredentials(): AuthCredentials | null { return this.authManager.getCredentials(); } diff --git a/apps/cli/src/commands/context.command.ts b/apps/cli/src/commands/context.command.ts index b18b06dd..2bd175d5 100644 --- a/apps/cli/src/commands/context.command.ts +++ b/apps/cli/src/commands/context.command.ts @@ -115,7 +115,7 @@ export class ContextCommand extends Command { */ private async executeShow(): Promise { try { - const result = await this.displayContext(); + const result = this.displayContext(); this.setLastResult(result); } catch (error: any) { this.handleError(error); @@ -126,7 +126,7 @@ export class ContextCommand extends Command { /** * Display current context */ - private async displayContext(): Promise { + private displayContext(): ContextResult { // Check authentication first if (!this.authManager.isAuthenticated()) { console.log(chalk.yellow('āœ— Not authenticated')); @@ -139,7 +139,7 @@ export class ContextCommand extends Command { }; } - const context = await this.authManager.getContext(); + const context = this.authManager.getContext(); console.log(chalk.cyan('\nšŸŒ Workspace Context\n')); @@ -250,7 +250,7 @@ export class ContextCommand extends Command { ]); // Update context - await this.authManager.updateContext({ + this.authManager.updateContext({ orgId: selectedOrg.id, orgName: selectedOrg.name, // Clear brief when changing org @@ -263,7 +263,7 @@ export class ContextCommand extends Command { return { success: true, action: 'select-org', - context: (await this.authManager.getContext()) || undefined, + context: this.authManager.getContext() || undefined, message: `Selected organization: ${selectedOrg.name}` }; } catch (error) { @@ -284,7 +284,7 @@ export class ContextCommand extends Command { } // Check if org is selected - const context = await this.authManager.getContext(); + const context = this.authManager.getContext(); if (!context?.orgId) { ui.displayError( 'No organization selected. Run "tm context org" first.' @@ -343,7 +343,7 @@ export class ContextCommand extends Command { if (selectedBrief) { // Update context with brief const briefName = `Brief ${selectedBrief.id.slice(0, 8)}`; - await this.authManager.updateContext({ + this.authManager.updateContext({ briefId: selectedBrief.id, briefName: briefName }); @@ -353,12 +353,12 @@ export class ContextCommand extends Command { return { success: true, action: 'select-brief', - context: (await this.authManager.getContext()) || undefined, + context: this.authManager.getContext() || undefined, message: `Selected brief: ${selectedBrief.name}` }; } else { // Clear brief selection - await this.authManager.updateContext({ + this.authManager.updateContext({ briefId: undefined, briefName: undefined }); @@ -368,7 +368,7 @@ export class ContextCommand extends Command { return { success: true, action: 'select-brief', - context: (await this.authManager.getContext()) || undefined, + context: this.authManager.getContext() || undefined, message: 'Cleared brief selection' }; } @@ -491,7 +491,7 @@ export class ContextCommand extends Command { // Update context: set org and brief const briefName = `Brief ${brief.id.slice(0, 8)}`; - await this.authManager.updateContext({ + this.authManager.updateContext({ orgId: brief.accountId, orgName, briefId: brief.id, @@ -508,7 +508,7 @@ export class ContextCommand extends Command { this.setLastResult({ success: true, action: 'set', - context: (await this.authManager.getContext()) || undefined, + context: this.authManager.getContext() || undefined, message: 'Context set from brief' }); } catch (error: any) { @@ -613,7 +613,7 @@ export class ContextCommand extends Command { }; } - await this.authManager.updateContext(context); + this.authManager.updateContext(context); ui.displaySuccess('Context updated'); // Display what was set @@ -631,7 +631,7 @@ export class ContextCommand extends Command { return { success: true, action: 'set', - context: (await this.authManager.getContext()) || undefined, + context: this.authManager.getContext() || undefined, message: 'Context updated' }; } catch (error) { @@ -682,7 +682,7 @@ export class ContextCommand extends Command { /** * Get current context (for programmatic usage) */ - getContext(): Promise { + getContext(): UserContext | null { return this.authManager.getContext(); } diff --git a/packages/tm-core/src/auth/auth-manager.spec.ts b/packages/tm-core/src/auth/auth-manager.spec.ts index b78ce8f3..d18b2d86 100644 --- a/packages/tm-core/src/auth/auth-manager.spec.ts +++ b/packages/tm-core/src/auth/auth-manager.spec.ts @@ -35,7 +35,7 @@ vi.mock('./credential-store.js', () => { } saveCredentials() {} clearCredentials() {} - hasValidCredentials() { + hasCredentials() { return false; } } diff --git a/packages/tm-core/src/auth/auth-manager.ts b/packages/tm-core/src/auth/auth-manager.ts index 2acd301b..b5842160 100644 --- a/packages/tm-core/src/auth/auth-manager.ts +++ b/packages/tm-core/src/auth/auth-manager.ts @@ -29,8 +29,6 @@ export class AuthManager { private oauthService: OAuthService; private supabaseClient: SupabaseAuthClient; private organizationService?: OrganizationService; - private logger = getLogger('AuthManager'); - private refreshPromise: Promise | null = null; private constructor(config?: Partial) { this.credentialStore = CredentialStore.getInstance(config); @@ -83,60 +81,10 @@ export class AuthManager { /** * Get stored authentication credentials - * Automatically refreshes the token if expired + * Returns credentials as-is (even if expired). Refresh must be triggered explicitly + * via refreshToken() or will occur automatically when using the Supabase client for API calls. */ - async getCredentials(): Promise { - const credentials = this.credentialStore.getCredentials({ - allowExpired: true - }); - - if (!credentials) { - 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; - } - } - - try { - this.logger.info('Token expired, attempting automatic refresh...'); - 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; - } - - /** - * Get stored authentication credentials (synchronous version) - * Does not attempt automatic refresh - */ - getCredentialsSync(): AuthCredentials | null { + getCredentials(): AuthCredentials | null { return this.credentialStore.getCredentials(); } @@ -219,25 +167,26 @@ export class AuthManager { } /** - * Check if authenticated + * Check if authenticated (credentials exist, regardless of expiration) + * @returns true if credentials are stored, including expired credentials */ isAuthenticated(): boolean { - return this.credentialStore.hasValidCredentials(); + return this.credentialStore.hasCredentials(); } /** * Get the current user context (org/brief selection) */ - async getContext(): Promise { - const credentials = await this.getCredentials(); + getContext(): UserContext | null { + const credentials = this.getCredentials(); return credentials?.selectedContext || null; } /** * Update the user context (org/brief selection) */ - async updateContext(context: Partial): Promise { - const credentials = await this.getCredentials(); + updateContext(context: Partial): void { + const credentials = this.getCredentials(); if (!credentials) { throw new AuthenticationError('Not authenticated', 'NOT_AUTHENTICATED'); } @@ -262,8 +211,8 @@ export class AuthManager { /** * Clear the user context */ - async clearContext(): Promise { - const credentials = await this.getCredentials(); + clearContext(): void { + const credentials = this.getCredentials(); if (!credentials) { throw new AuthenticationError('Not authenticated', 'NOT_AUTHENTICATED'); } @@ -280,7 +229,7 @@ export class AuthManager { private async getOrganizationService(): Promise { if (!this.organizationService) { // First check if we have credentials with a token - const credentials = await this.getCredentials(); + const credentials = this.getCredentials(); if (!credentials || !credentials.token) { throw new AuthenticationError('Not authenticated', 'NOT_AUTHENTICATED'); } diff --git a/packages/tm-core/src/auth/credential-store.spec.ts b/packages/tm-core/src/auth/credential-store.spec.ts index 05bb813c..9ac43d0d 100644 --- a/packages/tm-core/src/auth/credential-store.spec.ts +++ b/packages/tm-core/src/auth/credential-store.spec.ts @@ -52,7 +52,7 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(expiredCredentials); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).toBeNull(); }); @@ -69,7 +69,7 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(validCredentials); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).not.toBeNull(); expect(retrieved?.token).toBe('valid-token'); @@ -92,6 +92,25 @@ describe('CredentialStore - Token Expiration', () => { expect(retrieved).not.toBeNull(); expect(retrieved?.token).toBe('expired-token'); }); + + it('should return expired token by default (allowExpired defaults to true)', () => { + const expiredCredentials: AuthCredentials = { + token: 'expired-token-default', + refreshToken: 'refresh-token', + userId: 'test-user', + email: 'test@example.com', + expiresAt: new Date(Date.now() - 60000).toISOString(), + savedAt: new Date().toISOString() + }; + + credentialStore.saveCredentials(expiredCredentials); + + // Call without options - should default to allowExpired: true + const retrieved = credentialStore.getCredentials(); + + expect(retrieved).not.toBeNull(); + expect(retrieved?.token).toBe('expired-token-default'); + }); }); describe('Clock Skew Tolerance', () => { @@ -108,7 +127,7 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(almostExpiredCredentials); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).toBeNull(); }); @@ -126,7 +145,7 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(validCredentials); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).not.toBeNull(); expect(retrieved?.token).toBe('valid-token'); @@ -146,7 +165,7 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(credentials); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).not.toBeNull(); expect(typeof retrieved?.expiresAt).toBe('number'); // Normalized to number @@ -164,7 +183,7 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(credentials); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).not.toBeNull(); expect(typeof retrieved?.expiresAt).toBe('number'); @@ -185,7 +204,7 @@ describe('CredentialStore - Token Expiration', () => { mode: 0o600 }); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).toBeNull(); }); @@ -203,7 +222,7 @@ describe('CredentialStore - Token Expiration', () => { mode: 0o600 }); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); expect(retrieved).toBeNull(); }); @@ -244,15 +263,15 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(credentials); - const retrieved = credentialStore.getCredentials(); + const retrieved = credentialStore.getCredentials({ allowExpired: false }); // Should be normalized to number for runtime use expect(typeof retrieved?.expiresAt).toBe('number'); }); }); - describe('hasValidCredentials', () => { - it('should return false for expired credentials', () => { + describe('hasCredentials', () => { + it('should return true for expired credentials', () => { const expiredCredentials: AuthCredentials = { token: 'expired-token', refreshToken: 'refresh-token', @@ -264,7 +283,7 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(expiredCredentials); - expect(credentialStore.hasValidCredentials()).toBe(false); + expect(credentialStore.hasCredentials()).toBe(true); }); it('should return true for valid credentials', () => { @@ -279,11 +298,11 @@ describe('CredentialStore - Token Expiration', () => { credentialStore.saveCredentials(validCredentials); - expect(credentialStore.hasValidCredentials()).toBe(true); + expect(credentialStore.hasCredentials()).toBe(true); }); it('should return false when no credentials exist', () => { - expect(credentialStore.hasValidCredentials()).toBe(false); + expect(credentialStore.hasCredentials()).toBe(false); }); }); }); diff --git a/packages/tm-core/src/auth/credential-store.test.ts b/packages/tm-core/src/auth/credential-store.test.ts index 62d2eb71..ce35b36b 100644 --- a/packages/tm-core/src/auth/credential-store.test.ts +++ b/packages/tm-core/src/auth/credential-store.test.ts @@ -197,7 +197,7 @@ describe('CredentialStore', () => { JSON.stringify(mockCredentials) ); - const result = store.getCredentials(); + const result = store.getCredentials({ allowExpired: false }); expect(result).toBeNull(); expect(mockLogger.warn).toHaveBeenCalledWith( @@ -226,6 +226,31 @@ describe('CredentialStore', () => { expect(result).not.toBeNull(); expect(result?.token).toBe('expired-token'); }); + + it('should return expired tokens by default (allowExpired defaults to true)', () => { + const expiredTimestamp = Date.now() - 3600000; // 1 hour ago + const mockCredentials = { + token: 'expired-token-default', + userId: 'user-expired', + expiresAt: expiredTimestamp, + tokenType: 'standard', + savedAt: new Date().toISOString() + }; + + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockReturnValue( + JSON.stringify(mockCredentials) + ); + + // Call without options - should default to allowExpired: true + const result = store.getCredentials(); + + expect(result).not.toBeNull(); + expect(result?.token).toBe('expired-token-default'); + expect(mockLogger.warn).not.toHaveBeenCalledWith( + expect.stringContaining('Authentication token has expired') + ); + }); }); describe('saveCredentials with timestamp normalization', () => { @@ -451,7 +476,7 @@ describe('CredentialStore', () => { }); }); - describe('hasValidCredentials', () => { + describe('hasCredentials', () => { it('should return true when valid unexpired credentials exist', () => { const futureDate = new Date(Date.now() + 3600000); // 1 hour from now const credentials = { @@ -465,10 +490,10 @@ describe('CredentialStore', () => { vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(credentials)); - expect(store.hasValidCredentials()).toBe(true); + expect(store.hasCredentials()).toBe(true); }); - it('should return false when credentials are expired', () => { + it('should return true when credentials are expired', () => { const pastDate = new Date(Date.now() - 3600000); // 1 hour ago const credentials = { token: 'expired-token', @@ -481,13 +506,13 @@ describe('CredentialStore', () => { vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(credentials)); - expect(store.hasValidCredentials()).toBe(false); + expect(store.hasCredentials()).toBe(true); }); it('should return false when no credentials exist', () => { vi.mocked(fs.existsSync).mockReturnValue(false); - expect(store.hasValidCredentials()).toBe(false); + expect(store.hasCredentials()).toBe(false); }); it('should return false when file contains invalid JSON', () => { @@ -495,7 +520,7 @@ describe('CredentialStore', () => { vi.mocked(fs.readFileSync).mockReturnValue('invalid json {'); vi.mocked(fs.renameSync).mockImplementation(() => undefined); - expect(store.hasValidCredentials()).toBe(false); + expect(store.hasCredentials()).toBe(false); }); it('should return false for credentials without expiry', () => { @@ -510,7 +535,7 @@ describe('CredentialStore', () => { vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(credentials)); // Credentials without expiry are considered invalid - expect(store.hasValidCredentials()).toBe(false); + expect(store.hasCredentials()).toBe(false); // Should log warning about missing expiration expect(mockLogger.warn).toHaveBeenCalledWith( @@ -518,14 +543,14 @@ describe('CredentialStore', () => { ); }); - it('should use allowExpired=false by default', () => { + it('should use allowExpired=true', () => { // Spy on getCredentials to verify it's called with correct params const getCredentialsSpy = vi.spyOn(store, 'getCredentials'); vi.mocked(fs.existsSync).mockReturnValue(false); - store.hasValidCredentials(); + store.hasCredentials(); - expect(getCredentialsSpy).toHaveBeenCalledWith({ allowExpired: false }); + expect(getCredentialsSpy).toHaveBeenCalledWith({ allowExpired: true }); }); }); diff --git a/packages/tm-core/src/auth/credential-store.ts b/packages/tm-core/src/auth/credential-store.ts index 10b27346..1d4b2776 100644 --- a/packages/tm-core/src/auth/credential-store.ts +++ b/packages/tm-core/src/auth/credential-store.ts @@ -54,9 +54,12 @@ export class CredentialStore { /** * Get stored authentication credentials + * @param options.allowExpired - Whether to return expired credentials (default: true) * @returns AuthCredentials with expiresAt as number (milliseconds) for runtime use */ - getCredentials(options?: { allowExpired?: boolean }): AuthCredentials | null { + getCredentials({ + allowExpired = true + }: { allowExpired?: boolean } = {}): AuthCredentials | null { try { if (!fs.existsSync(this.config.configFile)) { return null; @@ -90,7 +93,6 @@ export class CredentialStore { // Check if the token has expired (with clock skew tolerance) const now = Date.now(); - const allowExpired = options?.allowExpired ?? false; if (now >= expiresAtMs - this.CLOCK_SKEW_MS && !allowExpired) { this.logger.warn( 'Authentication token has expired or is about to expire', @@ -103,7 +105,7 @@ export class CredentialStore { return null; } - // Return valid token + // Return credentials (even if expired) to enable refresh flows return authData; } catch (error) { this.logger.error( @@ -199,10 +201,11 @@ export class CredentialStore { } /** - * Check if credentials exist and are valid + * Check if credentials exist (regardless of expiration status) + * @returns true if credentials are stored, including expired credentials */ - hasValidCredentials(): boolean { - const credentials = this.getCredentials({ allowExpired: false }); + hasCredentials(): boolean { + const credentials = this.getCredentials({ allowExpired: true }); return credentials !== null; } diff --git a/packages/tm-core/src/auth/oauth-service.ts b/packages/tm-core/src/auth/oauth-service.ts index 00c3fdec..3977b5db 100644 --- a/packages/tm-core/src/auth/oauth-service.ts +++ b/packages/tm-core/src/auth/oauth-service.ts @@ -281,15 +281,26 @@ export class OAuthService { // Exchange code for session using PKCE const session = await this.supabaseClient.exchangeCodeForSession(code); + // Calculate expiration - can be overridden with TM_TOKEN_EXPIRY_MINUTES + let expiresAt: string | undefined; + const tokenExpiryMinutes = process.env.TM_TOKEN_EXPIRY_MINUTES; + if (tokenExpiryMinutes) { + const minutes = parseInt(tokenExpiryMinutes); + expiresAt = new Date(Date.now() + minutes * 60 * 1000).toISOString(); + this.logger.warn(`Token expiry overridden to ${minutes} minute(s)`); + } else { + expiresAt = session.expires_at + ? new Date(session.expires_at * 1000).toISOString() + : undefined; + } + // Save authentication data const authData: AuthCredentials = { token: session.access_token, refreshToken: session.refresh_token, userId: session.user.id, email: session.user.email, - expiresAt: session.expires_at - ? new Date(session.expires_at * 1000).toISOString() - : undefined, + expiresAt, tokenType: 'standard', savedAt: new Date().toISOString() }; @@ -340,10 +351,18 @@ export class OAuthService { // Get user info from the session const user = await this.supabaseClient.getUser(); - // Calculate expiration time - const expiresAt = expiresIn - ? new Date(Date.now() + parseInt(expiresIn) * 1000).toISOString() - : undefined; + // Calculate expiration time - can be overridden with TM_TOKEN_EXPIRY_MINUTES + let expiresAt: string | undefined; + const tokenExpiryMinutes = process.env.TM_TOKEN_EXPIRY_MINUTES; + if (tokenExpiryMinutes) { + const minutes = parseInt(tokenExpiryMinutes); + expiresAt = new Date(Date.now() + minutes * 60 * 1000).toISOString(); + this.logger.warn(`Token expiry overridden to ${minutes} minute(s)`); + } else { + expiresAt = expiresIn + ? new Date(Date.now() + parseInt(expiresIn) * 1000).toISOString() + : undefined; + } // Save authentication data const authData: AuthCredentials = { @@ -351,7 +370,7 @@ export class OAuthService { refreshToken: refreshToken || undefined, userId: user?.id || 'unknown', email: user?.email, - expiresAt: expiresAt, + expiresAt, tokenType: 'standard', savedAt: new Date().toISOString() }; diff --git a/packages/tm-core/src/auth/supabase-session-storage.ts b/packages/tm-core/src/auth/supabase-session-storage.ts index e431a2c1..fe82e6ed 100644 --- a/packages/tm-core/src/auth/supabase-session-storage.ts +++ b/packages/tm-core/src/auth/supabase-session-storage.ts @@ -98,11 +98,11 @@ export class SupabaseSessionStorage implements SupportedStorage { // Only handle Supabase session keys if (key === STORAGE_KEY || key.includes('auth-token')) { try { + this.logger.info('Supabase called setItem - storing refreshed session'); + // Parse the session and update our credentials const sessionUpdates = this.parseSessionToCredentials(value); - const existingCredentials = this.store.getCredentials({ - allowExpired: true - }); + const existingCredentials = this.store.getCredentials(); if (sessionUpdates.token) { const updatedCredentials: AuthCredentials = { @@ -113,6 +113,9 @@ export class SupabaseSessionStorage implements SupportedStorage { } as AuthCredentials; this.store.saveCredentials(updatedCredentials); + this.logger.info( + 'Successfully saved refreshed credentials from Supabase' + ); } } catch (error) { this.logger.error('Error setting session:', error); diff --git a/packages/tm-core/src/clients/supabase-client.ts b/packages/tm-core/src/clients/supabase-client.ts index d12ce7d1..fbf9d4c0 100644 --- a/packages/tm-core/src/clients/supabase-client.ts +++ b/packages/tm-core/src/clients/supabase-client.ts @@ -17,10 +17,11 @@ export class SupabaseAuthClient { private client: SupabaseJSClient | null = null; private sessionStorage: SupabaseSessionStorage; private logger = getLogger('SupabaseAuthClient'); + private credentialStore: CredentialStore; constructor() { - const credentialStore = CredentialStore.getInstance(); - this.sessionStorage = new SupabaseSessionStorage(credentialStore); + this.credentialStore = CredentialStore.getInstance(); + this.sessionStorage = new SupabaseSessionStorage(this.credentialStore); } /** diff --git a/packages/tm-core/src/storage/storage-factory.ts b/packages/tm-core/src/storage/storage-factory.ts index 18b5a8fb..5c65ba54 100644 --- a/packages/tm-core/src/storage/storage-factory.ts +++ b/packages/tm-core/src/storage/storage-factory.ts @@ -73,7 +73,7 @@ export class StorageFactory { ); } // Use auth token from AuthManager (synchronous - no auto-refresh here) - const credentials = authManager.getCredentialsSync(); + const credentials = authManager.getCredentials(); if (credentials) { // Merge with existing storage config, ensuring required fields const nextStorage: StorageSettings = { @@ -103,7 +103,7 @@ export class StorageFactory { // Then check if authenticated via AuthManager if (authManager.isAuthenticated()) { - const credentials = authManager.getCredentialsSync(); + const credentials = authManager.getCredentials(); if (credentials) { // Configure API storage with auth credentials const nextStorage: StorageSettings = { diff --git a/packages/tm-core/tests/auth/auth-refresh.test.ts b/packages/tm-core/tests/auth/auth-refresh.test.ts index 1ae50aaf..0ae4f357 100644 --- a/packages/tm-core/tests/auth/auth-refresh.test.ts +++ b/packages/tm-core/tests/auth/auth-refresh.test.ts @@ -50,7 +50,7 @@ describe('AuthManager Token Refresh', () => { } }); - it('should not make concurrent refresh requests', async () => { + it('should return expired credentials to enable refresh flows', () => { // Set up expired credentials with refresh token const expiredCredentials: AuthCredentials = { token: 'expired_access_token', @@ -63,50 +63,16 @@ describe('AuthManager Token Refresh', () => { 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() - } - }; + // Get credentials should return them even if expired + // Refresh will be handled by explicit calls or client operations + const credentials = authManager.getCredentials(); - 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); + expect(credentials).not.toBeNull(); + expect(credentials?.token).toBe('expired_access_token'); + expect(credentials?.refreshToken).toBe('valid_refresh_token'); }); - it('should return valid credentials without attempting refresh', async () => { + it('should return valid credentials', () => { // Set up valid (non-expired) credentials const validCredentials: AuthCredentials = { token: 'valid_access_token', @@ -119,17 +85,14 @@ describe('AuthManager Token Refresh', () => { 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(); + const credentials = 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 () => { + it('should return expired credentials even without refresh token', () => { // Set up expired credentials WITHOUT refresh token + // We still return them - it's up to the caller to handle const expiredCredentials: AuthCredentials = { token: 'expired_access_token', refreshToken: undefined, @@ -141,17 +104,19 @@ describe('AuthManager Token Refresh', () => { credentialStore.saveCredentials(expiredCredentials); - const credentials = await authManager.getCredentials(); + const credentials = authManager.getCredentials(); + // Returns credentials even if expired + expect(credentials).not.toBeNull(); + expect(credentials?.token).toBe('expired_access_token'); + }); + + it('should return null if no credentials exist', () => { + const credentials = 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 () => { + it('should return credentials regardless of refresh token validity', () => { // Set up expired credentials with refresh token const expiredCredentials: AuthCredentials = { token: 'expired_access_token', @@ -164,13 +129,11 @@ describe('AuthManager Token Refresh', () => { 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 = authManager.getCredentials(); - const credentials = await authManager.getCredentials(); - - expect(credentials).toBeNull(); - expect(refreshTokenSpy).toHaveBeenCalledTimes(1); + // Returns credentials - refresh will be attempted by the client which will handle failure + expect(credentials).not.toBeNull(); + expect(credentials?.token).toBe('expired_access_token'); + expect(credentials?.refreshToken).toBe('invalid_refresh_token'); }); }); diff --git a/packages/tm-core/tests/integration/auth-token-refresh.test.ts b/packages/tm-core/tests/integration/auth-token-refresh.test.ts index b6103de5..869a0754 100644 --- a/packages/tm-core/tests/integration/auth-token-refresh.test.ts +++ b/packages/tm-core/tests/integration/auth-token-refresh.test.ts @@ -76,7 +76,7 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { }); describe('Expired Token Detection', () => { - it('should detect expired token', async () => { + it('should return expired token for Supabase to refresh', () => { // Set up expired credentials const expiredCredentials: AuthCredentials = { token: 'expired-token', @@ -91,24 +91,15 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - // Mock the Supabase refreshSession to return new tokens - const mockRefreshSession = vi - .fn() - .mockResolvedValue(mockRefreshedSession); - vi.spyOn( - authManager['supabaseClient'], - 'refreshSession' - ).mockImplementation(mockRefreshSession); + // Get credentials returns them even if expired + const credentials = authManager.getCredentials(); - // Get credentials should trigger refresh - const credentials = await authManager.getCredentials(); - - expect(mockRefreshSession).toHaveBeenCalledTimes(1); expect(credentials).not.toBeNull(); - expect(credentials?.token).toBe('new-access-token-xyz'); + expect(credentials?.token).toBe('expired-token'); + expect(credentials?.refreshToken).toBe('valid-refresh-token'); }); - it('should not refresh valid token', async () => { + it('should return valid token', () => { // Set up valid credentials const validCredentials: AuthCredentials = { token: 'valid-token', @@ -123,22 +114,14 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - // Mock refresh to ensure it's not called - const mockRefreshSession = vi.fn(); - vi.spyOn( - authManager['supabaseClient'], - 'refreshSession' - ).mockImplementation(mockRefreshSession); + const credentials = authManager.getCredentials(); - const credentials = await authManager.getCredentials(); - - expect(mockRefreshSession).not.toHaveBeenCalled(); expect(credentials?.token).toBe('valid-token'); }); }); describe('Token Refresh Flow', () => { - it('should refresh expired token and save new credentials', async () => { + it('should manually refresh expired token and save new credentials', async () => { const expiredCredentials: AuthCredentials = { token: 'old-token', refreshToken: 'old-refresh-token', @@ -162,23 +145,24 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { 'refreshSession' ).mockResolvedValue(mockRefreshedSession); - const refreshedCredentials = await authManager.getCredentials(); + // Explicitly call refreshToken() method + const refreshedCredentials = await authManager.refreshToken(); expect(refreshedCredentials).not.toBeNull(); - expect(refreshedCredentials?.token).toBe('new-access-token-xyz'); - expect(refreshedCredentials?.refreshToken).toBe('new-refresh-token-xyz'); + expect(refreshedCredentials.token).toBe('new-access-token-xyz'); + expect(refreshedCredentials.refreshToken).toBe('new-refresh-token-xyz'); // Verify context was preserved - expect(refreshedCredentials?.selectedContext?.orgId).toBe('test-org'); - expect(refreshedCredentials?.selectedContext?.briefId).toBe('test-brief'); + expect(refreshedCredentials.selectedContext?.orgId).toBe('test-org'); + expect(refreshedCredentials.selectedContext?.briefId).toBe('test-brief'); // Verify new expiration is in the future - const newExpiry = new Date(refreshedCredentials!.expiresAt!).getTime(); + const newExpiry = new Date(refreshedCredentials.expiresAt!).getTime(); const now = Date.now(); expect(newExpiry).toBeGreaterThan(now); }); - it('should return null if refresh fails', async () => { + it('should throw error if manual refresh fails', async () => { const expiredCredentials: AuthCredentials = { token: 'expired-token', refreshToken: 'invalid-refresh-token', @@ -198,12 +182,11 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { 'refreshSession' ).mockRejectedValue(new Error('Refresh token expired')); - const credentials = await authManager.getCredentials(); - - expect(credentials).toBeNull(); + // Explicit refreshToken() call should throw + await expect(authManager.refreshToken()).rejects.toThrow(); }); - it('should return null if no refresh token available', async () => { + it('should return expired credentials even without refresh token', () => { const expiredCredentials: AuthCredentials = { token: 'expired-token', // No refresh token @@ -217,18 +200,21 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - const credentials = await authManager.getCredentials(); + const credentials = authManager.getCredentials(); - expect(credentials).toBeNull(); + // Credentials are returned even without refresh token + expect(credentials).not.toBeNull(); + expect(credentials?.token).toBe('expired-token'); + expect(credentials?.refreshToken).toBeUndefined(); }); - it('should return null if credentials missing expiresAt', async () => { + it('should return null if credentials missing expiresAt', () => { const credentialsWithoutExpiry: AuthCredentials = { token: 'test-token', refreshToken: 'refresh-token', userId: 'test-user-id', email: 'test@example.com', - // Missing expiresAt + // Missing expiresAt - invalid token savedAt: new Date().toISOString() } as any; @@ -236,16 +222,17 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - const credentials = await authManager.getCredentials(); + const credentials = authManager.getCredentials(); - // Should return null because no valid expiration + // Tokens without valid expiration are considered invalid expect(credentials).toBeNull(); }); }); describe('Clock Skew Tolerance', () => { - it('should refresh token within 30-second expiry window', async () => { + it('should return credentials within 30-second expiry window', () => { // Token expires in 15 seconds (within 30-second buffer) + // Supabase will handle refresh automatically const almostExpiredCredentials: AuthCredentials = { token: 'almost-expired-token', refreshToken: 'valid-refresh-token', @@ -259,23 +246,16 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - const mockRefreshSession = vi - .fn() - .mockResolvedValue(mockRefreshedSession); - vi.spyOn( - authManager['supabaseClient'], - 'refreshSession' - ).mockImplementation(mockRefreshSession); + const credentials = authManager.getCredentials(); - const credentials = await authManager.getCredentials(); - - // Should trigger refresh due to 30-second buffer - expect(mockRefreshSession).toHaveBeenCalledTimes(1); - expect(credentials?.token).toBe('new-access-token-xyz'); + // Credentials are returned (Supabase handles auto-refresh in background) + expect(credentials).not.toBeNull(); + expect(credentials?.token).toBe('almost-expired-token'); + expect(credentials?.refreshToken).toBe('valid-refresh-token'); }); - it('should not refresh token well before expiry', async () => { - // Token expires in 5 minutes (well outside 30-second buffer) + it('should return valid token well before expiry', () => { + // Token expires in 5 minutes const validCredentials: AuthCredentials = { token: 'valid-token', refreshToken: 'valid-refresh-token', @@ -289,21 +269,17 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - const mockRefreshSession = vi.fn(); - vi.spyOn( - authManager['supabaseClient'], - 'refreshSession' - ).mockImplementation(mockRefreshSession); + const credentials = authManager.getCredentials(); - const credentials = await authManager.getCredentials(); - - expect(mockRefreshSession).not.toHaveBeenCalled(); + // Valid credentials are returned as-is + expect(credentials).not.toBeNull(); expect(credentials?.token).toBe('valid-token'); + expect(credentials?.refreshToken).toBe('valid-refresh-token'); }); }); describe('Synchronous vs Async Methods', () => { - it('getCredentialsSync should not trigger refresh', () => { + it('getCredentials should return expired credentials', () => { const expiredCredentials: AuthCredentials = { token: 'expired-token', refreshToken: 'valid-refresh-token', @@ -317,40 +293,17 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - // Synchronous call should return null without refresh - const credentials = authManager.getCredentialsSync(); - - expect(credentials).toBeNull(); - }); - - it('getCredentials async should trigger refresh', async () => { - const expiredCredentials: AuthCredentials = { - token: 'expired-token', - refreshToken: 'valid-refresh-token', - userId: 'test-user-id', - email: 'test@example.com', - expiresAt: new Date(Date.now() - 60000).toISOString(), - savedAt: new Date().toISOString() - }; - - credentialStore.saveCredentials(expiredCredentials); - - authManager = AuthManager.getInstance(); - - vi.spyOn( - authManager['supabaseClient'], - 'refreshSession' - ).mockResolvedValue(mockRefreshedSession); - - const credentials = await authManager.getCredentials(); + // Returns credentials even if expired - Supabase will handle refresh + const credentials = authManager.getCredentials(); expect(credentials).not.toBeNull(); - expect(credentials?.token).toBe('new-access-token-xyz'); + expect(credentials?.token).toBe('expired-token'); + expect(credentials?.refreshToken).toBe('valid-refresh-token'); }); }); describe('Multiple Concurrent Calls', () => { - it('should handle concurrent getCredentials calls gracefully', async () => { + it('should handle concurrent getCredentials calls gracefully', () => { const expiredCredentials: AuthCredentials = { token: 'expired-token', refreshToken: 'valid-refresh-token', @@ -364,29 +317,20 @@ describe('AuthManager - Token Auto-Refresh Integration', () => { authManager = AuthManager.getInstance(); - const mockRefreshSession = vi - .fn() - .mockResolvedValue(mockRefreshedSession); - vi.spyOn( - authManager['supabaseClient'], - 'refreshSession' - ).mockImplementation(mockRefreshSession); + // Make multiple concurrent calls (synchronous now) + const creds1 = authManager.getCredentials(); + const creds2 = authManager.getCredentials(); + const creds3 = authManager.getCredentials(); - // Make multiple concurrent calls - const [creds1, creds2, creds3] = await Promise.all([ - authManager.getCredentials(), - authManager.getCredentials(), - authManager.getCredentials() - ]); + // All should get the same credentials (even if expired) + expect(creds1?.token).toBe('expired-token'); + expect(creds2?.token).toBe('expired-token'); + expect(creds3?.token).toBe('expired-token'); - // All should get the refreshed token - expect(creds1?.token).toBe('new-access-token-xyz'); - expect(creds2?.token).toBe('new-access-token-xyz'); - expect(creds3?.token).toBe('new-access-token-xyz'); - - // Refresh might be called multiple times, but that's okay - // (ideally we'd debounce, but this is acceptable behavior) - expect(mockRefreshSession).toHaveBeenCalled(); + // All include refresh token for Supabase to use + expect(creds1?.refreshToken).toBe('valid-refresh-token'); + expect(creds2?.refreshToken).toBe('valid-refresh-token'); + expect(creds3?.refreshToken).toBe('valid-refresh-token'); }); }); });