fix: oidc cache busting issues fixed (#1656)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## 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.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

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>(OidcClientConfigService);
+        validationService = module.get<OidcValidationService>(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');
+        });
+    });
+});

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],
 })

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],
 })

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>(OidcConfigPersistence);
+        clientConfigService = module.get<OidcClientConfigService>(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();
+        });
+    });
+});

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<OidcConfig> {
     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<OidcConfig> {
         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<OidcConfig> {
         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<OidcConfig> {
                 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,