From e204eb80a00ab9242e3dca4ccfc3e1b55a7694b7 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Thu, 4 Sep 2025 12:29:13 -0400 Subject: [PATCH] fix: oidc cache busting issues fixed (#1656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary by CodeRabbit - Bug Fixes - SSO/OIDC provider changes now take effect immediately by clearing caches on updates, deletes, and settings changes. - Updating a provider’s issuer no longer requires an API restart. - Tests - Added extensive test coverage for OIDC config caching, including per‑provider and global invalidation and manual/automatic configuration paths. - Chores - Updated internal module wiring to resolve circular dependencies; no user-facing changes. --- .../oidc-client-config.service.cache.test.ts | 216 ++++++++++++++ .../sso/client/oidc-client.module.ts | 4 +- .../resolvers/sso/core/oidc-base.module.ts | 5 +- .../sso/core/oidc-config.cache-fix.test.ts | 276 ++++++++++++++++++ .../resolvers/sso/core/oidc-config.service.ts | 30 +- 5 files changed, 525 insertions(+), 6 deletions(-) create mode 100644 api/src/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.cache.test.ts create mode 100644 api/src/unraid-api/graph/resolvers/sso/core/oidc-config.cache-fix.test.ts diff --git a/api/src/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.cache.test.ts b/api/src/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.cache.test.ts new file mode 100644 index 000000000..41486bec7 --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.cache.test.ts @@ -0,0 +1,216 @@ +import { ConfigService } from '@nestjs/config'; +import { Test } from '@nestjs/testing'; + +import * as client from 'openid-client'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { OidcClientConfigService } from '@app/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.js'; +import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/core/oidc-validation.service.js'; +import { OidcProvider } from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; + +vi.mock('openid-client'); + +describe('OidcClientConfigService - Cache Behavior', () => { + let service: OidcClientConfigService; + let validationService: OidcValidationService; + + const createMockProvider = (port: number): OidcProvider => ({ + id: 'test-provider', + name: 'Test Provider', + clientId: 'test-client-id', + clientSecret: 'test-secret', + issuer: `http://localhost:${port}`, + scopes: ['openid', 'profile', 'email'], + authorizationRules: [], + }); + + const createMockConfiguration = (port: number) => { + const mockConfig = { + serverMetadata: vi.fn(() => ({ + issuer: `http://localhost:${port}`, + authorization_endpoint: `http://localhost:${port}/auth`, + token_endpoint: `http://localhost:${port}/token`, + jwks_uri: `http://localhost:${port}/jwks`, + userinfo_endpoint: `http://localhost:${port}/userinfo`, + })), + }; + return mockConfig as unknown as client.Configuration; + }; + + beforeEach(async () => { + vi.clearAllMocks(); + + const mockConfigService = { + get: vi.fn(), + set: vi.fn(), + }; + + const module = await Test.createTestingModule({ + providers: [ + OidcClientConfigService, + OidcValidationService, + { + provide: ConfigService, + useValue: mockConfigService, + }, + ], + }).compile(); + + service = module.get(OidcClientConfigService); + validationService = module.get(OidcValidationService); + }); + + describe('Configuration Caching', () => { + it('should cache configuration on first call', async () => { + const provider = createMockProvider(1029); + const mockConfig = createMockConfiguration(1029); + + vi.spyOn(validationService, 'performDiscovery').mockResolvedValueOnce(mockConfig); + + // First call + const config1 = await service.getOrCreateConfig(provider); + expect(validationService.performDiscovery).toHaveBeenCalledTimes(1); + expect(config1.serverMetadata().issuer).toBe('http://localhost:1029'); + + // Second call with same provider ID should use cache + const config2 = await service.getOrCreateConfig(provider); + expect(validationService.performDiscovery).toHaveBeenCalledTimes(1); + expect(config2).toBe(config1); + }); + + it('should return stale cached configuration when issuer changes without cache clear', async () => { + const provider1029 = createMockProvider(1029); + const provider1030 = createMockProvider(1030); + const mockConfig1029 = createMockConfiguration(1029); + const mockConfig1030 = createMockConfiguration(1030); + + vi.spyOn(validationService, 'performDiscovery') + .mockResolvedValueOnce(mockConfig1029) + .mockResolvedValueOnce(mockConfig1030); + + // Initial configuration on port 1029 + const config1 = await service.getOrCreateConfig(provider1029); + expect(config1.serverMetadata().issuer).toBe('http://localhost:1029'); + expect(config1.serverMetadata().authorization_endpoint).toBe('http://localhost:1029/auth'); + + // Update provider to port 1030 (simulating UI change) + // Without clearing cache, it should still return the old cached config + const config2 = await service.getOrCreateConfig(provider1030); + + // THIS IS THE BUG: The service returns cached config for port 1029 + // even though the provider now has issuer on port 1030 + expect(config2.serverMetadata().issuer).toBe('http://localhost:1029'); + expect(config2.serverMetadata().authorization_endpoint).toBe('http://localhost:1029/auth'); + + // performDiscovery should only be called once because cache is used + expect(validationService.performDiscovery).toHaveBeenCalledTimes(1); + }); + + it('should return fresh configuration after cache is cleared', async () => { + const provider1029 = createMockProvider(1029); + const provider1030 = createMockProvider(1030); + const mockConfig1029 = createMockConfiguration(1029); + const mockConfig1030 = createMockConfiguration(1030); + + vi.spyOn(validationService, 'performDiscovery') + .mockResolvedValueOnce(mockConfig1029) + .mockResolvedValueOnce(mockConfig1030); + + // Initial configuration on port 1029 + const config1 = await service.getOrCreateConfig(provider1029); + expect(config1.serverMetadata().issuer).toBe('http://localhost:1029'); + + // Clear cache for the provider + service.clearCache(provider1030.id); + + // Now it should fetch fresh config for port 1030 + const config2 = await service.getOrCreateConfig(provider1030); + expect(config2.serverMetadata().issuer).toBe('http://localhost:1030'); + expect(config2.serverMetadata().authorization_endpoint).toBe('http://localhost:1030/auth'); + + // performDiscovery should be called twice (once for each port) + expect(validationService.performDiscovery).toHaveBeenCalledTimes(2); + }); + + it('should clear all provider caches when clearCache is called without providerId', async () => { + const provider1 = { ...createMockProvider(1029), id: 'provider1' }; + const provider2 = { ...createMockProvider(1030), id: 'provider2' }; + const mockConfig1 = createMockConfiguration(1029); + const mockConfig2 = createMockConfiguration(1030); + + vi.spyOn(validationService, 'performDiscovery') + .mockResolvedValueOnce(mockConfig1) + .mockResolvedValueOnce(mockConfig2) + .mockResolvedValueOnce(mockConfig1) + .mockResolvedValueOnce(mockConfig2); + + // Cache both providers + await service.getOrCreateConfig(provider1); + await service.getOrCreateConfig(provider2); + expect(service.getCacheSize()).toBe(2); + + // Clear all caches + service.clearCache(); + expect(service.getCacheSize()).toBe(0); + + // Both should fetch fresh configs + await service.getOrCreateConfig(provider1); + await service.getOrCreateConfig(provider2); + + // performDiscovery should be called 4 times total + expect(validationService.performDiscovery).toHaveBeenCalledTimes(4); + }); + }); + + describe('Manual Configuration Caching', () => { + it('should cache manual configuration and exhibit same stale cache issue', async () => { + const provider1029: OidcProvider = { + id: 'manual-provider', + name: 'Manual Provider', + clientId: 'client-id', + clientSecret: 'secret', + issuer: '', + authorizationEndpoint: 'http://localhost:1029/auth', + tokenEndpoint: 'http://localhost:1029/token', + scopes: ['openid'], + authorizationRules: [], + }; + + const provider1030: OidcProvider = { + ...provider1029, + authorizationEndpoint: 'http://localhost:1030/auth', + tokenEndpoint: 'http://localhost:1030/token', + }; + + // Mock the client.Configuration constructor for manual configs + const mockManualConfig1029 = createMockConfiguration(1029); + const mockManualConfig1030 = createMockConfiguration(1030); + + let configCallCount = 0; + vi.mocked(client.Configuration).mockImplementation(() => { + configCallCount++; + return configCallCount === 1 ? mockManualConfig1029 : mockManualConfig1030; + }); + + vi.mocked(client.ClientSecretPost).mockReturnValue({} as any); + vi.mocked(client.allowInsecureRequests).mockImplementation(() => {}); + + // First call with port 1029 + const config1 = await service.getOrCreateConfig(provider1029); + expect(config1.serverMetadata().authorization_endpoint).toBe('http://localhost:1029/auth'); + + // Update to port 1030 without clearing cache + const config2 = await service.getOrCreateConfig(provider1030); + + // BUG: Still returns cached config with port 1029 + expect(config2.serverMetadata().authorization_endpoint).toBe('http://localhost:1029/auth'); + + // Clear cache and try again + service.clearCache(provider1030.id); + const config3 = await service.getOrCreateConfig(provider1030); + + // Now it should return the updated config + expect(config3.serverMetadata().authorization_endpoint).toBe('http://localhost:1030/auth'); + }); + }); +}); diff --git a/api/src/unraid-api/graph/resolvers/sso/client/oidc-client.module.ts b/api/src/unraid-api/graph/resolvers/sso/client/oidc-client.module.ts index ff091b887..c6fbeaa1d 100644 --- a/api/src/unraid-api/graph/resolvers/sso/client/oidc-client.module.ts +++ b/api/src/unraid-api/graph/resolvers/sso/client/oidc-client.module.ts @@ -1,11 +1,11 @@ -import { Module } from '@nestjs/common'; +import { forwardRef, Module } from '@nestjs/common'; import { OidcClientConfigService } from '@app/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.js'; import { OidcRedirectUriService } from '@app/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.js'; import { OidcBaseModule } from '@app/unraid-api/graph/resolvers/sso/core/oidc-base.module.js'; @Module({ - imports: [OidcBaseModule], + imports: [forwardRef(() => OidcBaseModule)], providers: [OidcClientConfigService, OidcRedirectUriService], exports: [OidcClientConfigService, OidcRedirectUriService], }) diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc-base.module.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc-base.module.ts index 1ec1020f9..522119c8a 100644 --- a/api/src/unraid-api/graph/resolvers/sso/core/oidc-base.module.ts +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc-base.module.ts @@ -1,12 +1,13 @@ -import { Module } from '@nestjs/common'; +import { forwardRef, Module } from '@nestjs/common'; import { UserSettingsModule } from '@unraid/shared/services/user-settings.js'; +import { OidcClientModule } from '@app/unraid-api/graph/resolvers/sso/client/oidc-client.module.js'; import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js'; import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/core/oidc-validation.service.js'; @Module({ - imports: [UserSettingsModule], + imports: [UserSettingsModule, forwardRef(() => OidcClientModule)], providers: [OidcConfigPersistence, OidcValidationService], exports: [OidcConfigPersistence, OidcValidationService], }) diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.cache-fix.test.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.cache-fix.test.ts new file mode 100644 index 000000000..523f506d2 --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.cache-fix.test.ts @@ -0,0 +1,276 @@ +import { ConfigService } from '@nestjs/config'; +import { Test } from '@nestjs/testing'; +import * as fs from 'fs/promises'; + +import { UserSettingsService } from '@unraid/shared/services/user-settings.js'; +import * as client from 'openid-client'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { OidcClientConfigService } from '@app/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.js'; +import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js'; +import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/core/oidc-validation.service.js'; +import { OidcProvider } from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; + +vi.mock('openid-client'); +vi.mock('fs/promises', () => ({ + writeFile: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + stat: vi.fn().mockRejectedValue(new Error('File not found')), +})); + +describe('OIDC Config Cache Fix - Integration Test', () => { + let configPersistence: OidcConfigPersistence; + let clientConfigService: OidcClientConfigService; + let mockConfigService: any; + + afterEach(() => { + delete process.env.PATHS_CONFIG; + }); + + const createMockProvider = (port: number): OidcProvider => ({ + id: 'test-provider', + name: 'Test Provider', + clientId: 'test-client-id', + clientSecret: 'test-secret', + issuer: `http://localhost:${port}`, + scopes: ['openid', 'profile', 'email'], + authorizationRules: [ + { + claim: 'email', + operator: 'endsWith' as any, + value: ['@example.com'], + }, + ], + }); + + const createMockConfiguration = (port: number) => { + const mockConfig = { + serverMetadata: vi.fn(() => ({ + issuer: `http://localhost:${port}`, + authorization_endpoint: `http://localhost:${port}/auth`, + token_endpoint: `http://localhost:${port}/token`, + jwks_uri: `http://localhost:${port}/jwks`, + userinfo_endpoint: `http://localhost:${port}/userinfo`, + })), + }; + return mockConfig as unknown as client.Configuration; + }; + + beforeEach(async () => { + vi.clearAllMocks(); + + // Set environment variable for config path + process.env.PATHS_CONFIG = '/tmp/test-config'; + + mockConfigService = { + get: vi.fn((key: string) => { + if (key === 'oidc') { + return { + providers: [createMockProvider(1029)], + defaultAllowedOrigins: [], + }; + } + if (key === 'paths.config') { + return '/tmp/test-config'; + } + return undefined; + }), + set: vi.fn(), + getOrThrow: vi.fn((key: string) => { + if (key === 'paths.config' || key === 'paths') { + return '/tmp/test-config'; + } + return '/tmp/test-config'; + }), + }; + + const mockUserSettingsService = { + register: vi.fn(), + getAllSettings: vi.fn(), + getAllValues: vi.fn(), + updateNamespacedValues: vi.fn(), + }; + + const module = await Test.createTestingModule({ + providers: [ + OidcConfigPersistence, + OidcClientConfigService, + OidcValidationService, + { + provide: ConfigService, + useValue: mockConfigService, + }, + { + provide: UserSettingsService, + useValue: mockUserSettingsService, + }, + ], + }).compile(); + + configPersistence = module.get(OidcConfigPersistence); + clientConfigService = module.get(OidcClientConfigService); + + // Mock the persist method since we don't want to write to disk in tests + vi.spyOn(configPersistence as any, 'persist').mockResolvedValue(undefined); + }); + + describe('Cache clearing on provider update', () => { + it('should clear cache when provider is updated via upsertProvider', async () => { + const provider1029 = createMockProvider(1029); + const provider1030 = createMockProvider(1030); + const mockConfig1029 = createMockConfiguration(1029); + const mockConfig1030 = createMockConfiguration(1030); + + // Mock validation service to return configs + const validationService = (configPersistence as any).validationService; + vi.spyOn(validationService, 'performDiscovery') + .mockResolvedValueOnce(mockConfig1029) + .mockResolvedValueOnce(mockConfig1030); + + // First, get config for port 1029 - this caches it + const config1 = await clientConfigService.getOrCreateConfig(provider1029); + expect(config1.serverMetadata().issuer).toBe('http://localhost:1029'); + + // Spy on clearCache method + const clearCacheSpy = vi.spyOn(clientConfigService, 'clearCache'); + + // Update the provider to port 1030 via upsertProvider + await configPersistence.upsertProvider(provider1030); + + // Verify cache was cleared for this specific provider + expect(clearCacheSpy).toHaveBeenCalledWith(provider1030.id); + + // Now get config again - should fetch fresh config for port 1030 + const config2 = await clientConfigService.getOrCreateConfig(provider1030); + expect(config2.serverMetadata().issuer).toBe('http://localhost:1030'); + expect(config2.serverMetadata().authorization_endpoint).toBe('http://localhost:1030/auth'); + + // Verify discovery was called twice (not using cache) + expect(validationService.performDiscovery).toHaveBeenCalledTimes(2); + }); + + it('should clear cache when provider is deleted', async () => { + const provider = createMockProvider(1029); + const mockConfig = createMockConfiguration(1029); + + // Setup initial provider in config + mockConfigService.get.mockReturnValue({ + providers: [provider, { ...provider, id: 'other-provider' }], + defaultAllowedOrigins: [], + }); + + // Mock validation service + const validationService = (configPersistence as any).validationService; + vi.spyOn(validationService, 'performDiscovery').mockResolvedValue(mockConfig); + + // First, cache the provider config + await clientConfigService.getOrCreateConfig(provider); + + // Spy on clearCache + const clearCacheSpy = vi.spyOn(clientConfigService, 'clearCache'); + + // Delete the provider + const deleted = await configPersistence.deleteProvider(provider.id); + expect(deleted).toBe(true); + + // Verify cache was cleared for the deleted provider + expect(clearCacheSpy).toHaveBeenCalledWith(provider.id); + }); + + it('should clear all provider caches when updated via settings updateValues', async () => { + // This simulates what happens when settings are saved through the UI + const settingsCallback = (configPersistence as any).userSettings.register.mock.calls[0][1]; + + const newConfig = { + providers: [ + { + ...createMockProvider(1030), + authorizationMode: 'simple', + simpleAuthorization: { + allowedDomains: ['example.com'], + allowedEmails: [], + allowedUserIds: [], + }, + }, + ], + defaultAllowedOrigins: [], + }; + + // Spy on clearCache + const clearCacheSpy = vi.spyOn(clientConfigService, 'clearCache'); + + // Mock validation + const validationService = (configPersistence as any).validationService; + vi.spyOn(validationService, 'validateProvider').mockResolvedValue({ + isValid: true, + }); + + // Call the updateValues function (simulating saving settings from UI) + await settingsCallback.updateValues(newConfig); + + // Verify cache was cleared (called without arguments to clear all) + expect(clearCacheSpy).toHaveBeenCalledWith(); + }); + + it('should NOT require API restart after updating provider issuer', async () => { + // This test confirms that the fix eliminates the need for API restart + const settingsCallback = (configPersistence as any).userSettings.register.mock.calls[0][1]; + + const newConfig = { + providers: [createMockProvider(1030)], + defaultAllowedOrigins: [], + }; + + // Mock validation + const validationService = (configPersistence as any).validationService; + vi.spyOn(validationService, 'validateProvider').mockResolvedValue({ + isValid: true, + }); + + // Update settings + const result = await settingsCallback.updateValues(newConfig); + + // Verify that restartRequired is false + expect(result.restartRequired).toBe(false); + }); + }); + + describe('Provider validation on save', () => { + it('should validate providers and include warnings but still save', async () => { + const settingsCallback = (configPersistence as any).userSettings.register.mock.calls[0][1]; + + const newConfig = { + providers: [ + createMockProvider(1030), + { ...createMockProvider(1031), id: 'invalid-provider', name: 'Invalid Provider' }, + ], + defaultAllowedOrigins: [], + }; + + // Mock validation - first provider valid, second invalid + const validationService = (configPersistence as any).validationService; + vi.spyOn(validationService, 'validateProvider') + .mockResolvedValueOnce({ isValid: true }) + .mockResolvedValueOnce({ + isValid: false, + error: 'Discovery failed: Unable to reach issuer', + }); + + // Update settings + const result = await settingsCallback.updateValues(newConfig); + + // Should save successfully but include warnings + expect(result.restartRequired).toBe(false); + expect(result.warnings).toBeDefined(); + expect(result.warnings).toContain( + '❌ Invalid Provider: Discovery failed: Unable to reach issuer' + ); + expect(result.values.providers).toHaveLength(2); + + // Cache should still be cleared even with validation warnings + const clearCacheSpy = vi.spyOn(clientConfigService, 'clearCache'); + await settingsCallback.updateValues(newConfig); + expect(clearCacheSpy).toHaveBeenCalled(); + }); + }); +}); diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts index fc4ee4ed5..cafa0d119 100644 --- a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts @@ -1,4 +1,4 @@ -import { Injectable } from '@nestjs/common'; +import { forwardRef, Inject, Injectable, Optional } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { RuleEffect } from '@jsonforms/core'; @@ -6,6 +6,7 @@ import { mergeSettingSlices } from '@unraid/shared/jsonforms/settings.js'; import { ConfigFilePersister } from '@unraid/shared/services/config-file.js'; import { UserSettingsService } from '@unraid/shared/services/user-settings.js'; +import { OidcClientConfigService } from '@app/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.js'; import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/core/oidc-validation.service.js'; import { AuthorizationOperator, @@ -30,7 +31,10 @@ export class OidcConfigPersistence extends ConfigFilePersister { constructor( configService: ConfigService, private readonly userSettings: UserSettingsService, - private readonly validationService: OidcValidationService + private readonly validationService: OidcValidationService, + @Optional() + @Inject(forwardRef(() => OidcClientConfigService)) + private readonly clientConfigService?: OidcClientConfigService ) { super(configService); this.registerSettings(); @@ -252,6 +256,15 @@ export class OidcConfigPersistence extends ConfigFilePersister { this.configService.set(this.configKey(), newConfig); await this.persist(newConfig); + // Clear the OIDC client configuration cache when a provider is updated + // This ensures the new issuer/endpoints are used immediately + if (this.clientConfigService) { + this.clientConfigService.clearCache(cleanedProvider.id); + this.logger.debug( + `Cleared OIDC client configuration cache for provider ${cleanedProvider.id}` + ); + } + return cleanedProvider; } @@ -328,6 +341,12 @@ export class OidcConfigPersistence extends ConfigFilePersister { this.configService.set(this.configKey(), newConfig); await this.persist(newConfig); + // Clear the cache for the deleted provider + if (this.clientConfigService) { + this.clientConfigService.clearCache(id); + this.logger.debug(`Cleared OIDC client configuration cache for deleted provider ${id}`); + } + return true; } @@ -440,6 +459,13 @@ export class OidcConfigPersistence extends ConfigFilePersister { this.configService.set(this.configKey(), processedConfig); await this.persist(processedConfig); + // Clear the OIDC client configuration cache to ensure fresh discovery + // This fixes the issue where changing issuer URLs requires API restart + if (this.clientConfigService) { + this.clientConfigService.clearCache(); + this.logger.debug('Cleared OIDC client configuration cache after provider update'); + } + // Include validation results in response const response: { restartRequired: boolean; values: OidcConfig; warnings?: string[] } = { restartRequired: false,