From 6436f258bd7c43769dabcaf6bc5104c5ca71dee9 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 24 Aug 2025 01:05:11 -0400 Subject: [PATCH 01/74] feat(api): enhance OIDC redirect URI handling in service and tests - Updated `getRedirectUri` method in `OidcAuthService` to handle various edge cases for redirect URIs, including full URIs, malformed URLs, and default ports. - Added comprehensive tests for `OidcAuthService` to validate redirect URI construction and error handling. - Modified `RestController` to utilize `redirect_uri` query parameter for authorization requests. - Updated frontend components to include `redirect_uri` in authorization URLs, ensuring correct handling of different protocols and ports. --- .../resolvers/sso/oidc-auth.service.test.ts | 123 +++++++++++++++++- .../graph/resolvers/sso/oidc-auth.service.ts | 67 ++++++---- api/src/unraid-api/rest/rest.controller.ts | 15 ++- web/__test__/components/SsoButton.test.ts | 56 +++++++- web/components/sso/useSsoAuth.ts | 7 +- 5 files changed, 229 insertions(+), 39 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts index 3beba699cb..507009e7f7 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts @@ -2,7 +2,6 @@ import { UnauthorizedException } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { Test, TestingModule } from '@nestjs/testing'; -import * as client from 'openid-client'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { OidcAuthService } from '@app/unraid-api/graph/resolvers/sso/oidc-auth.service.js'; @@ -20,9 +19,7 @@ import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/oidc- describe('OidcAuthService', () => { let service: OidcAuthService; let oidcConfig: any; - let sessionService: any; let configService: any; - let stateService: any; let validationService: any; let module: TestingModule; @@ -61,9 +58,7 @@ describe('OidcAuthService', () => { service = module.get(OidcAuthService); oidcConfig = module.get(OidcConfigPersistence); - sessionService = module.get(OidcSessionService); configService = module.get(ConfigService); - stateService = module.get(OidcStateService); validationService = module.get(OidcValidationService); }); @@ -1651,5 +1646,123 @@ describe('OidcAuthService', () => { expect(redirectUri).toBe('http://tower.local/graphql/api/auth/oidc/callback'); }); + + it('should accept and use full redirect URI when provided', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + const redirectUri = getRedirectUri( + 'https://unraid.mytailnet.ts.net:1443/graphql/api/auth/oidc/callback' + ); + + expect(redirectUri).toBe( + 'https://unraid.mytailnet.ts.net:1443/graphql/api/auth/oidc/callback' + ); + }); + + it('should handle HTTPS with non-standard port correctly', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + const redirectUri = getRedirectUri('https://example.com:1443'); + + expect(redirectUri).toBe('https://example.com:1443/graphql/api/auth/oidc/callback'); + }); + + it('should reject invalid full redirect URIs with wrong path', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + + // This should fall back to parsing as origin since path is wrong + const redirectUri = getRedirectUri('https://example.com/wrong/path'); + + expect(redirectUri).toBe('https://example.com/graphql/api/auth/oidc/callback'); + }); + + it('should handle malformed URLs gracefully', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + configService.get.mockReturnValue('http://tower.local'); + + // Invalid URL should fall back to default + const redirectUri = getRedirectUri('not-a-valid-url'); + + expect(redirectUri).toBe('http://tower.local/graphql/api/auth/oidc/callback'); + }); + + it('should handle URL with default HTTPS port (443)', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + const redirectUri = getRedirectUri('https://example.com:443'); + + // Should not include :443 for HTTPS + expect(redirectUri).toBe('https://example.com/graphql/api/auth/oidc/callback'); + }); + + it('should handle URL with default HTTP port (80)', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + const redirectUri = getRedirectUri('http://example.com:80'); + + // Should not include :80 for HTTP + expect(redirectUri).toBe('http://example.com/graphql/api/auth/oidc/callback'); + }); + + it('should handle URL with trailing slash', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + const redirectUri = getRedirectUri('https://example.com/'); + + expect(redirectUri).toBe('https://example.com/graphql/api/auth/oidc/callback'); + }); + + it('should handle URL with query parameters in origin', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + const redirectUri = getRedirectUri('https://example.com?foo=bar'); + + // Query params should be ignored when constructing redirect URI + expect(redirectUri).toBe('https://example.com/graphql/api/auth/oidc/callback'); + }); + + it('should accept valid full redirect URI even with uppercase pathname', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + const redirectUri = getRedirectUri('https://example.com/GRAPHQL/api/auth/oidc/callback'); + + // Should reject due to case mismatch in path + expect(redirectUri).toBe('https://example.com/graphql/api/auth/oidc/callback'); + }); + + it('should handle Tailscale HTTPS with port 1443 correctly (user reported scenario)', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + + // Test with just origin (as sent by frontend) + const redirectUri1 = getRedirectUri('https://unraid.mytailnet.ts.net:1443'); + expect(redirectUri1).toBe( + 'https://unraid.mytailnet.ts.net:1443/graphql/api/auth/oidc/callback' + ); + + // Test with full redirect URI (as sent by frontend with redirect_uri param) + const redirectUri2 = getRedirectUri( + 'https://unraid.mytailnet.ts.net:1443/graphql/api/auth/oidc/callback' + ); + expect(redirectUri2).toBe( + 'https://unraid.mytailnet.ts.net:1443/graphql/api/auth/oidc/callback' + ); + }); + + it('should preserve ports in full redirect URIs', () => { + const getRedirectUri = (service as any).getRedirectUri.bind(service); + + // Should preserve explicit port 443 in full redirect URI + const httpsDefault = getRedirectUri( + 'https://example.com:443/graphql/api/auth/oidc/callback' + ); + expect(httpsDefault).toBe('https://example.com:443/graphql/api/auth/oidc/callback'); + + // Should preserve explicit port 80 in full redirect URI + const httpDefault = getRedirectUri('http://example.com:80/graphql/api/auth/oidc/callback'); + expect(httpDefault).toBe('http://example.com:80/graphql/api/auth/oidc/callback'); + + // HTTPS with non-standard port should include it + const httpsCustom = getRedirectUri( + 'https://example.com:8443/graphql/api/auth/oidc/callback' + ); + expect(httpsCustom).toBe('https://example.com:8443/graphql/api/auth/oidc/callback'); + + // HTTP with non-standard port should include it + const httpCustom = getRedirectUri('http://example.com:8080/graphql/api/auth/oidc/callback'); + expect(httpCustom).toBe('http://example.com:8080/graphql/api/auth/oidc/callback'); + }); }); }); diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts index e4e468b7be..1818e1754e 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts @@ -664,38 +664,51 @@ export class OidcAuthService { } private getRedirectUri(requestOrigin?: string): string { - // If we have the full origin (protocol://host), use it directly - if (requestOrigin) { - // Parse the origin to extract protocol and host - try { - const url = new URL(requestOrigin); - const { protocol, hostname, port } = url; - - // Reconstruct the URL, removing default ports - let cleanOrigin = `${protocol}//${hostname}`; + const CALLBACK_PATH = '/graphql/api/auth/oidc/callback'; - // Add port if it's not the default for the protocol - if ( - port && - !(protocol === 'https:' && port === '443') && - !(protocol === 'http:' && port === '80') - ) { - cleanOrigin += `:${port}`; - } + if (!requestOrigin) { + // No origin provided, use fallback + const baseUrl = this.configService.get('BASE_URL', 'http://tower.local'); + this.logger.debug(`Using fallback redirect URI: ${baseUrl}${CALLBACK_PATH}`); + return `${baseUrl}${CALLBACK_PATH}`; + } - // Special handling for localhost development with Nuxt proxy - if (hostname === 'localhost' && port === '3000') { - return `${cleanOrigin}/graphql/api/auth/oidc/callback`; - } + try { + const url = new URL(requestOrigin); - return `${cleanOrigin}/graphql/api/auth/oidc/callback`; - } catch (e) { - this.logger.warn(`Failed to parse request origin: ${requestOrigin}, error: ${e}`); + // Check if this is already a full redirect URI + if (url.pathname.endsWith(CALLBACK_PATH)) { + // Use the full redirect URI as-is (preserving any ports) + this.logger.debug(`Using full redirect URI from client: ${requestOrigin}`); + return requestOrigin; } + + // Build redirect URI from origin + const origin = this.buildOriginWithPort(url); + const redirectUri = `${origin}${CALLBACK_PATH}`; + + this.logger.debug(`Constructed redirect URI: ${redirectUri}`); + return redirectUri; + } catch (e) { + this.logger.warn(`Failed to parse request origin: ${requestOrigin}, error: ${e}`); + + // Fall back to configured BASE_URL + const baseUrl = this.configService.get('BASE_URL', 'http://tower.local'); + this.logger.debug(`Using fallback redirect URI: ${baseUrl}${CALLBACK_PATH}`); + return `${baseUrl}${CALLBACK_PATH}`; } + } + + private buildOriginWithPort(url: URL): string { + const { protocol, hostname, port } = url; + + // Check if port is empty, or is default for the protocol + const isDefaultPort = + !port || + (protocol === 'https:' && port === '443') || + (protocol === 'http:' && port === '80'); - // Fall back to configured BASE_URL or default - const baseUrl = this.configService.get('BASE_URL', 'http://tower.local'); - return `${baseUrl}/graphql/api/auth/oidc/callback`; + // Build origin with port only if non-default + return isDefaultPort ? `${protocol}//${hostname}` : `${protocol}//${hostname}:${port}`; } } diff --git a/api/src/unraid-api/rest/rest.controller.ts b/api/src/unraid-api/rest/rest.controller.ts index 3a60855ae5..b264d515fe 100644 --- a/api/src/unraid-api/rest/rest.controller.ts +++ b/api/src/unraid-api/rest/rest.controller.ts @@ -65,6 +65,7 @@ export class RestController { async oidcAuthorize( @Param('providerId') providerId: string, @Query('state') state: string, + @Query('redirect_uri') redirectUri: string, @Req() req: FastifyRequest, @Res() res: FastifyReply ) { @@ -73,10 +74,15 @@ export class RestController { return res.status(400).send('State parameter is required'); } - // Get the host and protocol from the request headers - const protocol = (req.headers['x-forwarded-proto'] as string) || req.protocol || 'http'; - const host = (req.headers['x-forwarded-host'] as string) || req.headers.host || undefined; - const requestInfo = host ? `${protocol}://${host}` : undefined; + // Use the redirect_uri from the client if provided, otherwise fall back to headers + let requestInfo = redirectUri; + if (!requestInfo) { + // Fall back to extracting from headers if redirect_uri not provided + const protocol = (req.headers['x-forwarded-proto'] as string) || req.protocol || 'http'; + const host = + (req.headers['x-forwarded-host'] as string) || req.headers.host || undefined; + requestInfo = host ? `${protocol}://${host}` : undefined; + } const authUrl = await this.oidcAuthService.getAuthorizationUrl( providerId, @@ -129,6 +135,7 @@ export class RestController { const host = (req.headers['x-forwarded-host'] as string) || req.headers.host || 'localhost:3000'; const fullUrl = `${protocol}://${host}${req.url}`; + // Extract the base URL (protocol://host:port) from the callback URL const requestInfo = `${protocol}://${host}`; this.logger.debug(`Full callback URL from request: ${fullUrl}`); diff --git a/web/__test__/components/SsoButton.test.ts b/web/__test__/components/SsoButton.test.ts index b7a0af3409..64bcb3bc61 100644 --- a/web/__test__/components/SsoButton.test.ts +++ b/web/__test__/components/SsoButton.test.ts @@ -59,6 +59,8 @@ const mockLocation = { hash: '', origin: 'http://mock-origin.com', pathname: '/login', + protocol: 'http:', + host: 'mock-origin.com', get href() { return mockLocationHref; }, @@ -253,7 +255,8 @@ describe('SsoButtons', () => { expect(sessionStorage.setItem).toHaveBeenCalledWith('sso_provider', 'unraid-net'); const generatedState = (sessionStorage.setItem as Mock).mock.calls[0][1]; - const expectedUrl = `/graphql/api/auth/oidc/authorize/unraid-net?state=${encodeURIComponent(generatedState)}`; + const redirectUri = `${mockLocation.origin}/graphql/api/auth/oidc/callback`; + const expectedUrl = `/graphql/api/auth/oidc/authorize/unraid-net?state=${encodeURIComponent(generatedState)}&redirect_uri=${encodeURIComponent(redirectUri)}`; expect(mockLocation.href).toBe(expectedUrl); }); @@ -377,6 +380,57 @@ describe('SsoButtons', () => { expect(mockLocation.href).toBe(expectedUrl); }); + it('handles HTTPS with non-standard port correctly', async () => { + const mockProviders = [ + { + id: 'tsidp', + name: 'Tailscale IDP', + buttonText: 'Sign in with Tailscale', + buttonIcon: null, + buttonVariant: 'secondary', + buttonStyle: null + } + ]; + + // Set up location with HTTPS and non-standard port + mockLocation.protocol = 'https:'; + mockLocation.host = 'unraid.mytailnet.ts.net:1443'; + mockLocation.origin = 'https://unraid.mytailnet.ts.net:1443'; + + mockUseQuery.mockReturnValue({ + result: { value: { publicOidcProviders: mockProviders } }, + refetch: vi.fn().mockResolvedValue({ data: { publicOidcProviders: mockProviders } }), + }); + + const wrapper = mount(SsoButtons, { + global: { + stubs: { + SsoProviderButton: SsoProviderButtonStub, + Button: { template: '' } + }, + }, + }); + + await flushPromises(); + vi.runAllTimers(); + await flushPromises(); + + const button = wrapper.find('button'); + await button.trigger('click'); + + // Should include the correct redirect URI with HTTPS and port 1443 + const generatedState = (sessionStorage.setItem as Mock).mock.calls[0][1]; + const redirectUri = 'https://unraid.mytailnet.ts.net:1443/graphql/api/auth/oidc/callback'; + const expectedUrl = `/graphql/api/auth/oidc/authorize/tsidp?state=${encodeURIComponent(generatedState)}&redirect_uri=${encodeURIComponent(redirectUri)}`; + + expect(mockLocation.href).toBe(expectedUrl); + + // Reset location mock for other tests + mockLocation.protocol = 'http:'; + mockLocation.host = 'mock-origin.com'; + mockLocation.origin = 'http://mock-origin.com'; + }); + it('handles multiple OIDC providers', async () => { const mockProviders = [ { diff --git a/web/components/sso/useSsoAuth.ts b/web/components/sso/useSsoAuth.ts index e68c01f08f..e0ffc359e2 100644 --- a/web/components/sso/useSsoAuth.ts +++ b/web/components/sso/useSsoAuth.ts @@ -70,8 +70,11 @@ export function useSsoAuth() { sessionStorage.setItem('sso_state', state); sessionStorage.setItem('sso_provider', providerId); - // Redirect to OIDC authorization endpoint with just the state token - const authUrl = `/graphql/api/auth/oidc/authorize/${encodeURIComponent(providerId)}?state=${encodeURIComponent(state)}`; + // Build the redirect URI based on current window location + const redirectUri = `${window.location.protocol}//${window.location.host}/graphql/api/auth/oidc/callback`; + + // Redirect to OIDC authorization endpoint with state token and redirect URI + const authUrl = `/graphql/api/auth/oidc/authorize/${encodeURIComponent(providerId)}?state=${encodeURIComponent(state)}&redirect_uri=${encodeURIComponent(redirectUri)}`; window.location.href = authUrl; }; From f7587de2f0fb971853b856b5457d040f1b085470 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 24 Aug 2025 01:17:47 -0400 Subject: [PATCH 02/74] fix(api): ensure requestInfo is correctly typed in RestController - Updated the `RestController` to explicitly define `requestInfo` as a string or undefined, improving type safety and clarity in handling the redirect URI. --- api/src/unraid-api/rest/rest.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/unraid-api/rest/rest.controller.ts b/api/src/unraid-api/rest/rest.controller.ts index b264d515fe..476121ddc7 100644 --- a/api/src/unraid-api/rest/rest.controller.ts +++ b/api/src/unraid-api/rest/rest.controller.ts @@ -75,7 +75,7 @@ export class RestController { } // Use the redirect_uri from the client if provided, otherwise fall back to headers - let requestInfo = redirectUri; + let requestInfo: string | undefined = redirectUri; if (!requestInfo) { // Fall back to extracting from headers if redirect_uri not provided const protocol = (req.headers['x-forwarded-proto'] as string) || req.protocol || 'http'; From 661768dd5b98b2cdfc89eaab57e0f677c491be9a Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 24 Aug 2025 10:46:56 -0400 Subject: [PATCH 03/74] test(api): add integration tests for OidcAuthService with enhanced logging - Introduced a new test file for `OidcAuthService` to validate its functionality and error handling during OIDC token exchanges and discovery processes. - Implemented detailed logging for various scenarios, including token exchange failures, discovery failures, and JWT claim validation issues. - Enhanced test coverage for logging request/response details and error properties from the OIDC provider, ensuring comprehensive validation of the service's behavior. --- .../sso/oidc-auth.service.integration.test.ts | 409 ++++++++++++++++++ 1 file changed, 409 insertions(+) create mode 100644 api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts new file mode 100644 index 0000000000..e3bc923903 --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts @@ -0,0 +1,409 @@ +import { Logger } from '@nestjs/common'; +import { ConfigModule, ConfigService } from '@nestjs/config'; +import { Test, TestingModule } from '@nestjs/testing'; + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { OidcAuthService } from '@app/unraid-api/graph/resolvers/sso/oidc-auth.service.js'; +import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/oidc-config.service.js'; +import { OidcProvider } from '@app/unraid-api/graph/resolvers/sso/oidc-provider.model.js'; +import { OidcSessionService } from '@app/unraid-api/graph/resolvers/sso/oidc-session.service.js'; +import { OidcStateService } from '@app/unraid-api/graph/resolvers/sso/oidc-state.service.js'; +import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/oidc-validation.service.js'; + +describe('OidcAuthService Integration Tests - Enhanced Logging', () => { + let service: OidcAuthService; + let configPersistence: OidcConfigPersistence; + let loggerSpy: any; + let debugLogs: string[] = []; + let errorLogs: string[] = []; + let warnLogs: string[] = []; + + beforeEach(async () => { + // Clear log arrays + debugLogs = []; + errorLogs = []; + warnLogs = []; + + const module: TestingModule = await Test.createTestingModule({ + imports: [ + ConfigModule.forRoot({ + isGlobal: true, + load: [() => ({ BASE_URL: 'http://test.local' })], + }), + ], + providers: [ + OidcAuthService, + OidcValidationService, + { + provide: OidcConfigPersistence, + useValue: { + getProvider: vi.fn(), + saveProvider: vi.fn(), + }, + }, + { + provide: OidcSessionService, + useValue: { + createSession: vi.fn().mockResolvedValue('mock-token'), + validateSession: vi.fn(), + }, + }, + { + provide: OidcStateService, + useValue: { + generateSecureState: vi.fn().mockReturnValue('secure-state'), + validateSecureState: vi + .fn() + .mockReturnValue({ isValid: true, clientState: 'test-state' }), + extractProviderFromState: vi.fn().mockReturnValue('test-provider'), + }, + }, + ], + }).compile(); + + service = module.get(OidcAuthService); + configPersistence = module.get(OidcConfigPersistence); + + // Spy on logger methods to capture logs + loggerSpy = { + debug: vi.spyOn(Logger.prototype, 'debug').mockImplementation((message: string) => { + debugLogs.push(message); + }), + error: vi.spyOn(Logger.prototype, 'error').mockImplementation((message: string) => { + errorLogs.push(message); + }), + warn: vi.spyOn(Logger.prototype, 'warn').mockImplementation((message: string) => { + warnLogs.push(message); + }), + log: vi.spyOn(Logger.prototype, 'log').mockImplementation(() => {}), + verbose: vi.spyOn(Logger.prototype, 'verbose').mockImplementation(() => {}), + }; + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('Token Exchange Error Logging', () => { + it('should log detailed error information when token exchange fails with Google (trailing slash issue)', async () => { + // This simulates the issue from #1616 where a trailing slash causes failure + const provider: OidcProvider = { + id: 'google-test', + name: 'Google Test', + issuer: 'https://accounts.google.com/', // Trailing slash will cause issue + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid', 'email', 'profile'], + authorizationRules: [ + { + claim: 'email', + operator: 'ENDS_WITH' as any, + value: ['@example.com'], + }, + ], + }; + + vi.mocked(configPersistence.getProvider).mockResolvedValue(provider); + + try { + await service.handleCallback( + 'google-test', + 'test-code', + 'test-state', + 'http://test.local' + ); + } catch (error) { + // We expect this to fail + } + + // Verify enhanced error logging + expect(debugLogs.some((log) => log.includes('Full token endpoint URL:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Authorization code:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Redirect URI in token request:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Client ID:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Client secret configured:'))).toBe(true); + expect(errorLogs.some((log) => log.includes('Token exchange failed:'))).toBe(true); + }); + + it('should log discovery failure details with invalid issuer URL', async () => { + const provider: OidcProvider = { + id: 'invalid-issuer', + name: 'Invalid Issuer Test', + issuer: 'https://invalid-oidc-provider.example.com', // Non-existent domain + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid', 'email'], + authorizationRules: [], + }; + + const validationService = new OidcValidationService(new ConfigService()); + const result = await validationService.validateProvider(provider); + + expect(result.isValid).toBe(false); + // Should now have more specific error message + expect(result.error).toBeDefined(); + // The error should mention the domain cannot be resolved or connection failed + expect(result.error).toMatch( + /Cannot resolve domain name|Failed to connect to OIDC provider/ + ); + expect(result.details).toBeDefined(); + expect(result.details).toHaveProperty('type'); + // Should be either DNS_ERROR or FETCH_ERROR depending on the cause + expect(['DNS_ERROR', 'FETCH_ERROR']).toContain((result.details as any).type); + }); + + it('should log detailed HTTP error responses from discovery', async () => { + const provider: OidcProvider = { + id: 'http-error-test', + name: 'HTTP Error Test', + issuer: 'https://httpstat.us/500', // Returns 500 error + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid'], + authorizationRules: [], + }; + + vi.mocked(configPersistence.getProvider).mockResolvedValue(provider); + + try { + await service.validateProvider(provider); + } catch (error) { + // Expected to fail + } + + // Check that HTTP status details are logged + expect(debugLogs.some((log) => log.includes('Discovery URL:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Client ID:'))).toBe(true); + }); + + it('should log authorization URL building details', async () => { + const provider: OidcProvider = { + id: 'auth-url-test', + name: 'Auth URL Test', + issuer: 'https://accounts.google.com', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid', 'email', 'profile'], + authorizationRules: [], + }; + + vi.mocked(configPersistence.getProvider).mockResolvedValue(provider); + + try { + const authUrl = await service.getAuthorizationUrl( + 'auth-url-test', + 'test-state', + 'http://test.local' + ); + + // Verify URL building logs + expect(debugLogs.some((log) => log.includes('Built authorization URL'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Authorization parameters:'))).toBe(true); + } catch (error) { + // May fail due to real discovery, but we're interested in the logs + } + }); + + it('should log detailed information for manual endpoint configuration', async () => { + const provider: OidcProvider = { + id: 'manual-endpoints', + name: 'Manual Endpoints Test', + issuer: undefined, + authorizationEndpoint: 'https://auth.example.com/authorize', + tokenEndpoint: 'https://auth.example.com/token', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid'], + authorizationRules: [], + }; + + vi.mocked(configPersistence.getProvider).mockResolvedValue(provider); + + const authUrl = await service.getAuthorizationUrl( + 'manual-endpoints', + 'test-state', + 'http://test.local' + ); + + // Verify manual endpoint logs + expect(debugLogs.some((log) => log.includes('Built authorization URL:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('client_id=test-client-id'))).toBe(true); + expect(authUrl).toContain('https://auth.example.com/authorize'); + }); + + it('should log JWT claim validation failures with detailed context', async () => { + const provider: OidcProvider = { + id: 'jwt-validation-test', + name: 'JWT Validation Test', + issuer: 'https://accounts.google.com', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid', 'email'], + authorizationRules: [ + { + claim: 'email', + operator: 'ENDS_WITH' as any, + value: ['@restricted.com'], + }, + ], + }; + + vi.mocked(configPersistence.getProvider).mockResolvedValue(provider); + + // Mock a scenario where JWT validation fails + try { + await service.handleCallback( + 'jwt-validation-test', + 'test-code', + 'test-state', + 'http://test.local' + ); + } catch (error) { + // Expected to fail + } + + // Check for JWT-related error logging + const hasJwtError = errorLogs.some( + (log) => log.includes('unexpected JWT claim') || log.includes('Token exchange failed') + ); + expect(hasJwtError).toBe(true); + }); + }); + + describe('Discovery Endpoint Logging', () => { + it('should log all discovery metadata when successful', async () => { + // Use a real OIDC provider that works + const provider: OidcProvider = { + id: 'microsoft', + name: 'Microsoft', + issuer: 'https://login.microsoftonline.com/common/v2.0', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid', 'email', 'profile'], + authorizationRules: [], + }; + + const validationService = new OidcValidationService(new ConfigService()); + + try { + await validationService.performDiscovery(provider); + } catch (error) { + // May fail due to network, but we're checking logs + } + + // Verify discovery logging + expect(debugLogs.some((log) => log.includes('Starting OIDC discovery'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Discovery URL:'))).toBe(true); + }); + + it('should log discovery failures with malformed JSON response', async () => { + const provider: OidcProvider = { + id: 'malformed-json', + name: 'Malformed JSON Test', + issuer: 'https://httpbin.org/html', // Returns HTML, not JSON + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid'], + authorizationRules: [], + }; + + const validationService = new OidcValidationService(new ConfigService()); + const result = await validationService.validateProvider(provider); + + expect(result.isValid).toBe(false); + // The error message should indicate JSON parsing issue + expect(result.error).toBeDefined(); + }); + + it('should handle and log HTTP vs HTTPS protocol differences', async () => { + const httpProvider: OidcProvider = { + id: 'http-local', + name: 'HTTP Local Test', + issuer: 'http://localhost:8080', // HTTP endpoint + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid'], + authorizationRules: [], + }; + + // Create a validation service and spy on its logger + const validationService = new OidcValidationService(new ConfigService()); + + try { + await validationService.validateProvider(httpProvider); + } catch (error) { + // Expected to fail if localhost:8080 isn't running + } + + // The HTTP logging happens in the validation service + // We should check that HTTP issuers are detected + expect(httpProvider.issuer).toMatch(/^http:/); + // Verify that we're testing an HTTP endpoint + expect(httpProvider.issuer).toBe('http://localhost:8080'); + }); + }); + + describe('Request/Response Detail Logging', () => { + it('should log complete request parameters for token exchange', async () => { + const provider: OidcProvider = { + id: 'token-params-test', + name: 'Token Params Test', + issuer: 'https://accounts.google.com', + clientId: 'detailed-client-id', + clientSecret: 'detailed-client-secret', + scopes: ['openid', 'email', 'profile', 'offline_access'], + authorizationRules: [], + }; + + vi.mocked(configPersistence.getProvider).mockResolvedValue(provider); + + try { + await service.handleCallback( + 'token-params-test', + 'authorization-code-12345', + 'state-with-signature', + 'https://myapp.example.com', + 'https://myapp.example.com/graphql/api/auth/oidc/callback?code=authorization-code-12345&state=state-with-signature&scope=openid+email+profile' + ); + } catch (error) { + // Expected to fail + } + + // Verify detailed parameter logging + expect(debugLogs.some((log) => log.includes('Authorization code: authorizat...'))).toBe( + true + ); + expect(debugLogs.some((log) => log.includes('Redirect URI in token request:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Expected state value:'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Client ID: detailed-client-id'))).toBe(true); + expect(debugLogs.some((log) => log.includes('Client secret configured: Yes'))).toBe(true); + }); + + it('should capture and log all error properties from openid-client', async () => { + const provider: OidcProvider = { + id: 'error-properties-test', + name: 'Error Properties Test', + issuer: 'https://expired-cert.badssl.com/', // SSL cert error + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scopes: ['openid'], + authorizationRules: [], + }; + + const validationService = new OidcValidationService(new ConfigService()); + const result = await validationService.validateProvider(provider); + + expect(result.isValid).toBe(false); + expect(result.error).toBeDefined(); + // Should detect SSL/certificate issues or connection failure + expect(result.error).toMatch( + /SSL\/TLS certificate error|Failed to connect to OIDC provider|certificate/ + ); + expect(result.details).toBeDefined(); + expect(result.details).toHaveProperty('type'); + // Should be either SSL_ERROR or FETCH_ERROR + expect(['SSL_ERROR', 'FETCH_ERROR']).toContain((result.details as any).type); + }); + }); +}); From 35ddd363a3b6265b3f7c6a699bfd2961958083ee Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 24 Aug 2025 10:47:38 -0400 Subject: [PATCH 04/74] feat(api): enhance logging in OidcAuthService and OidcValidationService - Added detailed debug logging for authorization URL construction, token exchange processes, and discovery operations in `OidcAuthService`. - Improved error handling and logging in `OidcValidationService`, including specific fetch error types and additional context for discovery failures. - Enhanced logging for HTTP response details and error causes to facilitate better debugging and issue resolution. --- .../graph/resolvers/sso/oidc-auth.service.ts | 116 ++++++++++++-- .../resolvers/sso/oidc-validation.service.ts | 149 +++++++++++++++++- 2 files changed, 248 insertions(+), 17 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts index 1818e1754e..d36c879739 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts @@ -63,6 +63,11 @@ export class OidcAuthService { authUrl.searchParams.set('state', secureState); authUrl.searchParams.set('response_type', 'code'); + this.logger.debug(`Built authorization URL: ${authUrl.href}`); + this.logger.debug( + `Authorization parameters: client_id=${provider.clientId}, redirect_uri=${redirectUri}, scope=${provider.scopes.join(' ')}, response_type=code` + ); + return authUrl.href; } @@ -89,6 +94,9 @@ export class OidcAuthService { const authUrl = client.buildAuthorizationUrl(config, parameters); + this.logger.debug(`Built authorization URL via discovery: ${authUrl.href}`); + this.logger.debug(`Authorization parameters: ${JSON.stringify(parameters, null, 2)}`); + return authUrl.href; } @@ -189,6 +197,15 @@ export class OidcAuthService { this.logger.debug(`Config issuer: ${config.serverMetadata().issuer}`); this.logger.debug(`Config token endpoint: ${config.serverMetadata().token_endpoint}`); + // Log the complete token exchange request details + const tokenEndpoint = config.serverMetadata().token_endpoint; + this.logger.debug(`Full token endpoint URL: ${tokenEndpoint}`); + this.logger.debug(`Authorization code: ${code.substring(0, 10)}...`); + this.logger.debug(`Redirect URI in token request: ${redirectUri}`); + this.logger.debug(`Client ID: ${provider.clientId}`); + this.logger.debug(`Client secret configured: ${provider.clientSecret ? 'Yes' : 'No'}`); + this.logger.debug(`Expected state value: ${originalState}`); + // For HTTP endpoints, we need to pass the allowInsecureRequests option const serverUrl = new URL(provider.issuer || ''); let clientOptions: any = undefined; @@ -215,6 +232,55 @@ export class OidcAuthService { tokenError instanceof Error ? tokenError.message : String(tokenError); this.logger.error(`Token exchange failed: ${errorMessage}`); + // Enhanced error logging for debugging + if (tokenError instanceof Error) { + // Log the error type and full details + this.logger.error(`Error type: ${tokenError.constructor.name}`); + if (tokenError.stack) { + this.logger.debug(`Stack trace: ${tokenError.stack}`); + } + + // Check for common openid-client error patterns + if ('response' in tokenError) { + const response = (tokenError as any).response; + if (response) { + this.logger.error(`HTTP Response Status: ${response.status}`); + this.logger.error(`HTTP Response Status Text: ${response.statusText}`); + if (response.body) { + this.logger.error( + `HTTP Response Body: ${JSON.stringify(response.body, null, 2)}` + ); + } + if (response.headers) { + this.logger.debug( + `HTTP Response Headers: ${JSON.stringify(response.headers, null, 2)}` + ); + } + } + } + + // Check for cause property (newer error patterns) + if ('cause' in tokenError && tokenError.cause) { + this.logger.error(`Error cause: ${JSON.stringify(tokenError.cause, null, 2)}`); + } + + // Log any additional error properties + const errorKeys = Object.keys(tokenError).filter( + (k) => k !== 'message' && k !== 'stack' + ); + if (errorKeys.length > 0) { + this.logger.debug(`Additional error properties: ${errorKeys.join(', ')}`); + for (const key of errorKeys) { + const value = (tokenError as any)[key]; + if (value !== undefined && value !== null) { + this.logger.debug( + `${key}: ${typeof value === 'object' ? JSON.stringify(value, null, 2) : value}` + ); + } + } + } + } + // Check if error message contains the "unexpected JWT claim" text if (errorMessage.includes('unexpected JWT claim value encountered')) { this.logger.error( @@ -229,6 +295,8 @@ export class OidcAuthService { `This error typically means the 'iss' claim in the JWT doesn't match the expected issuer` ); this.logger.error(`Check that your provider's issuer URL is configured correctly`); + this.logger.error(`Expected issuer: ${config.serverMetadata().issuer}`); + this.logger.error(`Provider configured issuer: ${provider.issuer}`); } throw tokenError; @@ -336,6 +404,10 @@ export class OidcAuthService { `Authorization endpoint: ${config.serverMetadata().authorization_endpoint}` ); this.logger.debug(`Token endpoint: ${config.serverMetadata().token_endpoint}`); + this.logger.debug(`JWKS URI: ${config.serverMetadata().jwks_uri || 'Not provided'}`); + this.logger.debug( + `Userinfo endpoint: ${config.serverMetadata().userinfo_endpoint || 'Not provided'}` + ); this.configCache.set(cacheKey, config); return config; } catch (discoveryError) { @@ -344,16 +416,42 @@ export class OidcAuthService { this.logger.warn(`Discovery failed for ${provider.id}: ${errorMessage}`); // Log more details about the discovery error - this.logger.debug( - `Discovery URL attempted: ${provider.issuer}/.well-known/openid-configuration` - ); - this.logger.debug( - `Full discovery error: ${JSON.stringify(discoveryError, null, 2)}` - ); + const discoveryUrl = `${provider.issuer}/.well-known/openid-configuration`; + this.logger.debug(`Discovery URL attempted: ${discoveryUrl}`); + + // Enhanced discovery error logging + if (discoveryError instanceof Error) { + this.logger.debug(`Discovery error type: ${discoveryError.constructor.name}`); + + // Check for response details in the error + if ('response' in discoveryError) { + const response = (discoveryError as any).response; + if (response) { + this.logger.error(`Discovery HTTP Status: ${response.status}`); + this.logger.error(`Discovery HTTP Status Text: ${response.statusText}`); + if (response.body) { + this.logger.error( + `Discovery Response Body: ${typeof response.body === 'string' ? response.body : JSON.stringify(response.body, null, 2)}` + ); + } + } + } + + // Check for cause + if ('cause' in discoveryError && discoveryError.cause) { + this.logger.debug( + `Discovery error cause: ${JSON.stringify(discoveryError.cause, null, 2)}` + ); + } - // Log stack trace for better debugging - if (discoveryError instanceof Error && discoveryError.stack) { - this.logger.debug(`Stack trace: ${discoveryError.stack}`); + this.logger.debug( + `Full discovery error: ${JSON.stringify(discoveryError, null, 2)}` + ); + + // Log stack trace for better debugging + if (discoveryError.stack) { + this.logger.debug(`Stack trace: ${discoveryError.stack}`); + } } // If discovery fails but we have manual endpoints, use them diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts index bbdc814c62..786905ea0b 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts @@ -63,11 +63,88 @@ export class OidcValidationService { // Log the raw error for debugging this.logger.debug(`Raw discovery error for ${provider.id}: ${errorMessage}`); + // Log additional error details if available + if (error instanceof Error) { + this.logger.debug(`Error type: ${error.constructor.name}`); + if ('stack' in error && error.stack) { + this.logger.debug(`Stack trace: ${error.stack}`); + } + if ('response' in error) { + const response = (error as any).response; + if (response) { + this.logger.debug(`Response status: ${response.status}`); + this.logger.debug(`Response body: ${response.body}`); + } + } + } + // Provide specific error messages for common issues let userFriendlyError = errorMessage; let details: Record = {}; - if (errorMessage.includes('getaddrinfo ENOTFOUND')) { + // Check for fetch-specific errors (Node.js fetch API) + if (errorMessage.includes('fetch failed')) { + // Try to extract more specific information from the error + if (error instanceof Error && 'cause' in error) { + const cause = (error as any).cause; + if (cause) { + this.logger.debug(`Fetch error cause: ${JSON.stringify(cause, null, 2)}`); + + // Check the cause for specific error types + if (cause.code === 'ENOTFOUND' || cause.message?.includes('ENOTFOUND')) { + userFriendlyError = `Cannot resolve domain name. Please check that '${provider.issuer}' is accessible and spelled correctly.`; + details = { + type: 'DNS_ERROR', + originalError: errorMessage, + cause: cause.message || cause.code, + }; + } else if ( + cause.code === 'ECONNREFUSED' || + cause.message?.includes('ECONNREFUSED') + ) { + userFriendlyError = `Connection refused. The server at '${provider.issuer}' is not accepting connections.`; + details = { + type: 'CONNECTION_ERROR', + originalError: errorMessage, + cause: cause.message || cause.code, + }; + } else if ( + cause.code === 'CERT_HAS_EXPIRED' || + cause.message?.includes('certificate') + ) { + userFriendlyError = `SSL/TLS certificate error. The server certificate may be invalid or expired.`; + details = { + type: 'SSL_ERROR', + originalError: errorMessage, + cause: cause.message || cause.code, + }; + } else if (cause.code === 'ETIMEDOUT' || cause.message?.includes('ETIMEDOUT')) { + userFriendlyError = `Connection timeout. The server at '${provider.issuer}' is not responding.`; + details = { + type: 'TIMEOUT_ERROR', + originalError: errorMessage, + cause: cause.message || cause.code, + }; + } else { + // Generic fetch failed with cause details + userFriendlyError = `Failed to connect to OIDC provider at '${provider.issuer}'. ${cause.message || cause.code || 'Unknown network error'}`; + details = { + type: 'FETCH_ERROR', + originalError: errorMessage, + cause: cause.message || cause.code, + }; + } + } else { + // Generic fetch failed without cause + userFriendlyError = `Failed to connect to OIDC provider at '${provider.issuer}'. Please verify the URL is correct and accessible.`; + details = { type: 'FETCH_ERROR', originalError: errorMessage }; + } + } else { + // Fetch failed but no cause information + userFriendlyError = `Failed to connect to OIDC provider at '${provider.issuer}'. Please verify the URL is correct and accessible.`; + details = { type: 'FETCH_ERROR', originalError: errorMessage }; + } + } else if (errorMessage.includes('getaddrinfo ENOTFOUND')) { userFriendlyError = `Cannot resolve domain name. Please check that '${provider.issuer}' is accessible and spelled correctly.`; details = { type: 'DNS_ERROR', originalError: errorMessage }; } else if (errorMessage.includes('ECONNREFUSED')) { @@ -142,6 +219,12 @@ export class OidcValidationService { : undefined; const serverUrl = new URL(provider.issuer); + const discoveryUrl = `${provider.issuer}/.well-known/openid-configuration`; + + this.logger.debug(`Starting OIDC discovery for provider ${provider.id}`); + this.logger.debug(`Discovery URL: ${discoveryUrl}`); + this.logger.debug(`Client ID: ${provider.clientId}`); + this.logger.debug(`Client secret configured: ${provider.clientSecret ? 'Yes' : 'No'}`); // Use provided client options or create default options with HTTP support if needed if (!clientOptions && serverUrl.protocol === 'http:') { @@ -153,12 +236,62 @@ export class OidcValidationService { }; } - return client.discovery( - serverUrl, - provider.clientId, - undefined, // client metadata - clientAuth, - clientOptions - ); + try { + const config = await client.discovery( + serverUrl, + provider.clientId, + undefined, // client metadata + clientAuth, + clientOptions + ); + + this.logger.debug(`Discovery successful for ${provider.id}`); + this.logger.debug(`Discovery response metadata:`); + this.logger.debug(` - issuer: ${config.serverMetadata().issuer}`); + this.logger.debug( + ` - authorization_endpoint: ${config.serverMetadata().authorization_endpoint}` + ); + this.logger.debug(` - token_endpoint: ${config.serverMetadata().token_endpoint}`); + this.logger.debug( + ` - userinfo_endpoint: ${config.serverMetadata().userinfo_endpoint || 'not provided'}` + ); + this.logger.debug(` - jwks_uri: ${config.serverMetadata().jwks_uri || 'not provided'}`); + this.logger.debug( + ` - response_types_supported: ${config.serverMetadata().response_types_supported?.join(', ') || 'not provided'}` + ); + this.logger.debug( + ` - scopes_supported: ${config.serverMetadata().scopes_supported?.join(', ') || 'not provided'}` + ); + + return config; + } catch (discoveryError) { + this.logger.error(`Discovery failed for ${provider.id} at ${discoveryUrl}`); + + if (discoveryError instanceof Error) { + this.logger.error(`Error type: ${discoveryError.constructor.name}`); + this.logger.error(`Error message: ${discoveryError.message}`); + + // Log response details if available + if ('response' in discoveryError) { + const response = (discoveryError as any).response; + if (response) { + this.logger.error(`HTTP Status: ${response.status}`); + this.logger.error(`HTTP Status Text: ${response.statusText}`); + if (response.body) { + this.logger.error( + `Response body: ${typeof response.body === 'string' ? response.body : JSON.stringify(response.body, null, 2)}` + ); + } + } + } + + // Log cause if available + if ('cause' in discoveryError && discoveryError.cause) { + this.logger.error(`Error cause: ${JSON.stringify(discoveryError.cause, null, 2)}`); + } + } + + throw discoveryError; + } } } From 7b6b9cdcc912c2b899a949d8d7fbf06addb56039 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 24 Aug 2025 10:52:06 -0400 Subject: [PATCH 05/74] fix(api): make OIDC issuer field optional in OidcProvider model - Updated the `issuer` field in the `OidcProvider` class to be optional, allowing for greater flexibility in OIDC configurations. - Added nullable attribute to the GraphQL field definition for better schema clarity. --- api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts index 163b4065ea..10b0e9e39b 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts @@ -80,9 +80,11 @@ export class OidcProvider { @Field(() => String, { description: 'OIDC issuer URL (e.g., https://accounts.google.com). Required for auto-discovery via /.well-known/openid-configuration', + nullable: true, }) @IsUrl() - issuer!: string; + @IsOptional() + issuer?: string; @Field(() => String, { nullable: true, From 62b0e5f46b28c2ef4f63948c0809ec241706201d Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 24 Aug 2025 12:21:09 -0400 Subject: [PATCH 06/74] feat: enhance oidc logging on config (#1621) --- api/generated-schema.graphql | 6 +- api/src/unraid-api/cli/generated/graphql.ts | 4 +- api/src/unraid-api/cli/generated/index.ts | 4 +- .../graph/resolvers/logs/logs.module.ts | 2 + .../resolvers/logs/logs.resolver.spec.ts | 10 +- .../graph/resolvers/logs/logs.resolver.ts | 45 ++- .../graph/resolvers/logs/logs.service.ts | 287 ++++++++++-------- .../metrics.resolver.integration.spec.ts | 48 +-- .../sso/oidc-auth.service.integration.test.ts | 26 +- .../graph/resolvers/sso/oidc-auth.service.ts | 20 +- .../resolvers/sso/oidc-config.service.ts | 3 +- .../graph/resolvers/sso/oidc-error.helper.ts | 263 ++++++++++++++++ .../resolvers/sso/oidc-validation.service.ts | 198 ++---------- .../graph/services/services.module.ts | 6 +- .../services/subscription-helper.service.ts | 20 +- .../services/subscription-manager.service.ts | 191 ++++++++++++ .../services/subscription-polling.service.ts | 91 ------ .../services/subscription-tracker.service.ts | 40 ++- .../ConnectSettings/ConnectSettings.ce.vue | 4 + .../ConnectSettings/OidcDebugLogs.vue | 100 ++++++ web/components/Logs/FilteredLogModal.vue | 67 ++++ web/components/Logs/LogViewer.ce.vue | 43 ++- web/components/Logs/OidcDebugButton.vue | 28 ++ web/components/Logs/SingleLogViewer.vue | 4 +- web/components/Logs/log.query.ts | 4 +- web/components/Logs/log.subscription.ts | 4 +- web/composables/gql/gql.ts | 12 +- web/composables/gql/graphql.ts | 12 +- web/composables/gql/index.ts | 2 +- 29 files changed, 1060 insertions(+), 484 deletions(-) create mode 100644 api/src/unraid-api/graph/resolvers/sso/oidc-error.helper.ts create mode 100644 api/src/unraid-api/graph/services/subscription-manager.service.ts delete mode 100644 api/src/unraid-api/graph/services/subscription-polling.service.ts create mode 100644 web/components/ConnectSettings/OidcDebugLogs.vue create mode 100644 web/components/Logs/FilteredLogModal.vue create mode 100644 web/components/Logs/OidcDebugButton.vue diff --git a/api/generated-schema.graphql b/api/generated-schema.graphql index b996b8ffcd..07eef5a1b8 100644 --- a/api/generated-schema.graphql +++ b/api/generated-schema.graphql @@ -1854,7 +1854,7 @@ type OidcProvider { """ OIDC issuer URL (e.g., https://accounts.google.com). Required for auto-discovery via /.well-known/openid-configuration """ - issuer: String! + issuer: String """ OAuth2 authorization endpoint URL. If omitted, will be auto-discovered from issuer/.well-known/openid-configuration @@ -2308,7 +2308,7 @@ type Query { config: Config! flash: Flash! logFiles: [LogFile!]! - logFile(path: String!, lines: Int, startLine: Int): LogFileContent! + logFile(path: String!, lines: Int, startLine: Int, filter: String): LogFileContent! me: UserAccount! """Get all notifications""" @@ -2590,7 +2590,7 @@ input AccessUrlInput { } type Subscription { - logFile(path: String!): LogFileContent! + logFile(path: String!, filter: String): LogFileContent! notificationAdded: Notification! notificationsOverview: NotificationOverview! ownerSubscription: Owner! diff --git a/api/src/unraid-api/cli/generated/graphql.ts b/api/src/unraid-api/cli/generated/graphql.ts index e25fc42552..e89389d756 100644 --- a/api/src/unraid-api/cli/generated/graphql.ts +++ b/api/src/unraid-api/cli/generated/graphql.ts @@ -1455,7 +1455,7 @@ export type OidcProvider = { /** The unique identifier for the OIDC provider */ id: Scalars['PrefixedID']['output']; /** OIDC issuer URL (e.g., https://accounts.google.com). Required for auto-discovery via /.well-known/openid-configuration */ - issuer: Scalars['String']['output']; + issuer?: Maybe; /** JSON Web Key Set URI for token validation. If omitted, will be auto-discovered from issuer/.well-known/openid-configuration */ jwksUri?: Maybe; /** Display name of the OIDC provider */ @@ -1704,6 +1704,7 @@ export type QueryGetPermissionsForRolesArgs = { export type QueryLogFileArgs = { + filter?: InputMaybe; lines?: InputMaybe; path: Scalars['String']['input']; startLine?: InputMaybe; @@ -2030,6 +2031,7 @@ export type Subscription = { export type SubscriptionLogFileArgs = { + filter?: InputMaybe; path: Scalars['String']['input']; }; diff --git a/api/src/unraid-api/cli/generated/index.ts b/api/src/unraid-api/cli/generated/index.ts index 873144cb2c..6cf863446e 100644 --- a/api/src/unraid-api/cli/generated/index.ts +++ b/api/src/unraid-api/cli/generated/index.ts @@ -1,2 +1,2 @@ -export * from './fragment-masking.js'; -export * from './gql.js'; +export * from "./fragment-masking.js"; +export * from "./gql.js"; \ No newline at end of file diff --git a/api/src/unraid-api/graph/resolvers/logs/logs.module.ts b/api/src/unraid-api/graph/resolvers/logs/logs.module.ts index 23b6b94e23..419bfd40a4 100644 --- a/api/src/unraid-api/graph/resolvers/logs/logs.module.ts +++ b/api/src/unraid-api/graph/resolvers/logs/logs.module.ts @@ -2,8 +2,10 @@ import { Module } from '@nestjs/common'; import { LogsResolver } from '@app/unraid-api/graph/resolvers/logs/logs.resolver.js'; import { LogsService } from '@app/unraid-api/graph/resolvers/logs/logs.service.js'; +import { ServicesModule } from '@app/unraid-api/graph/services/services.module.js'; @Module({ + imports: [ServicesModule], providers: [LogsResolver, LogsService], exports: [LogsService], }) diff --git a/api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts b/api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts index f128191e1f..b0bbd34ec2 100644 --- a/api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts +++ b/api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts @@ -1,9 +1,10 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { LogsResolver } from '@app/unraid-api/graph/resolvers/logs/logs.resolver.js'; import { LogsService } from '@app/unraid-api/graph/resolvers/logs/logs.service.js'; +import { SubscriptionHelperService } from '@app/unraid-api/graph/services/subscription-helper.service.js'; describe('LogsResolver', () => { let resolver: LogsResolver; @@ -18,6 +19,13 @@ describe('LogsResolver', () => { // Add mock implementations for service methods used by resolver }, }, + { + provide: SubscriptionHelperService, + useValue: { + // Add mock implementations for subscription helper methods + wrapAsyncIterator: vi.fn(), + }, + }, ], }).compile(); resolver = module.get(LogsResolver); diff --git a/api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts b/api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts index 4369937376..23aa285d7f 100644 --- a/api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts +++ b/api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts @@ -3,13 +3,17 @@ import { Args, Int, Query, Resolver, Subscription } from '@nestjs/graphql'; import { AuthAction, Resource } from '@unraid/shared/graphql.model.js'; import { UsePermissions } from '@unraid/shared/use-permissions.directive.js'; -import { createSubscription, PUBSUB_CHANNEL } from '@app/core/pubsub.js'; +import { PUBSUB_CHANNEL } from '@app/core/pubsub.js'; import { LogFile, LogFileContent } from '@app/unraid-api/graph/resolvers/logs/logs.model.js'; import { LogsService } from '@app/unraid-api/graph/resolvers/logs/logs.service.js'; +import { SubscriptionHelperService } from '@app/unraid-api/graph/services/subscription-helper.service.js'; @Resolver(() => LogFile) export class LogsResolver { - constructor(private readonly logsService: LogsService) {} + constructor( + private readonly logsService: LogsService, + private readonly subscriptionHelper: SubscriptionHelperService + ) {} @Query(() => [LogFile]) @UsePermissions({ @@ -28,9 +32,10 @@ export class LogsResolver { async logFile( @Args('path') path: string, @Args('lines', { nullable: true, type: () => Int }) lines?: number, - @Args('startLine', { nullable: true, type: () => Int }) startLine?: number + @Args('startLine', { nullable: true, type: () => Int }) startLine?: number, + @Args('filter', { nullable: true }) filter?: string ): Promise { - return this.logsService.getLogFileContent(path, lines, startLine); + return this.logsService.getLogFileContent(path, lines, startLine, filter); } @Subscription(() => LogFileContent, { name: 'logFile' }) @@ -38,27 +43,15 @@ export class LogsResolver { action: AuthAction.READ_ANY, resource: Resource.LOGS, }) - async logFileSubscription(@Args('path') path: string) { - // Start watching the file - this.logsService.getLogFileSubscriptionChannel(path); - - // Create the async iterator - const asyncIterator = createSubscription(PUBSUB_CHANNEL.LOG_FILE); - - // Store the original return method to wrap it - const originalReturn = asyncIterator.return; - - // Override the return method to clean up resources - asyncIterator.return = async () => { - // Stop watching the file when subscription ends - this.logsService.stopWatchingLogFile(path); - - // Call the original return method - return originalReturn - ? originalReturn.call(asyncIterator) - : Promise.resolve({ value: undefined, done: true }); - }; - - return asyncIterator; + logFileSubscription( + @Args('path') path: string, + @Args('filter', { nullable: true }) filter?: string + ) { + // Register the topic and get the key + const topicKey = this.logsService.registerLogFileSubscription(path, filter); + + // Use the helper service to create a tracked subscription + // This automatically handles subscribe/unsubscribe with reference counting + return this.subscriptionHelper.createTrackedSubscription(topicKey as PUBSUB_CHANNEL); } } diff --git a/api/src/unraid-api/graph/resolvers/logs/logs.service.ts b/api/src/unraid-api/graph/resolvers/logs/logs.service.ts index 03210e169f..5dcc5c6224 100644 --- a/api/src/unraid-api/graph/resolvers/logs/logs.service.ts +++ b/api/src/unraid-api/graph/resolvers/logs/logs.service.ts @@ -1,6 +1,6 @@ -import { Injectable, Logger } from '@nestjs/common'; +import { Injectable, Logger, OnModuleInit } from '@nestjs/common'; import { createReadStream } from 'node:fs'; -import { readdir, readFile, stat } from 'node:fs/promises'; +import { readdir, stat } from 'node:fs/promises'; import { basename, join } from 'node:path'; import { createInterface } from 'node:readline'; @@ -8,6 +8,7 @@ import * as chokidar from 'chokidar'; import { pubsub, PUBSUB_CHANNEL } from '@app/core/pubsub.js'; import { getters } from '@app/store/index.js'; +import { SubscriptionTrackerService } from '@app/unraid-api/graph/services/subscription-tracker.service.js'; interface LogFile { name: string; @@ -24,14 +25,18 @@ interface LogFileContent { } @Injectable() -export class LogsService { +export class LogsService implements OnModuleInit { private readonly logger = new Logger(LogsService.name); - private readonly logWatchers = new Map< - string, - { watcher: chokidar.FSWatcher; position: number; subscriptionCount: number } - >(); + private readonly logWatchers = new Map(); private readonly DEFAULT_LINES = 100; + constructor(private readonly subscriptionTracker: SubscriptionTrackerService) {} + + onModuleInit() { + // Log file subscriptions are registered dynamically as needed + this.logger.debug('LogsService initialized'); + } + /** * Get the base path for log files */ @@ -73,11 +78,13 @@ export class LogsService { * @param path Path to the log file * @param lines Number of lines to read from the end of the file (default: 100) * @param startLine Optional starting line number (1-indexed) + * @param filter Optional filter to apply to the content */ async getLogFileContent( path: string, lines = this.DEFAULT_LINES, - startLine?: number + startLine?: number, + filter?: string ): Promise { try { // Validate that the path is within the log directory @@ -90,10 +97,10 @@ export class LogsService { if (startLine !== undefined) { // Read from specific starting line - content = await this.readLinesFromPosition(normalizedPath, startLine, lines); + content = await this.readLinesFromPosition(normalizedPath, startLine, lines, filter); } else { // Read the last N lines (default behavior) - content = await this.readLastLines(normalizedPath, lines); + content = await this.readLastLines(normalizedPath, lines, filter); } return { @@ -111,137 +118,164 @@ export class LogsService { } /** - * Get the subscription channel for a log file + * Register and get the topic key for a log file subscription * @param path Path to the log file + * @param filter Optional filter to apply + * @returns The subscription topic key */ - getLogFileSubscriptionChannel(path: string): PUBSUB_CHANNEL { + registerLogFileSubscription(path: string, filter?: string): string { const normalizedPath = join(this.logBasePath, basename(path)); - - // Start watching the file if not already watching - if (!this.logWatchers.has(normalizedPath)) { - this.startWatchingLogFile(normalizedPath); - } else { - // Increment subscription count for existing watcher - const watcher = this.logWatchers.get(normalizedPath); - if (watcher) { - watcher.subscriptionCount++; - this.logger.debug( - `Incremented subscription count for ${normalizedPath} to ${watcher.subscriptionCount}` - ); - } + const topicKey = `LOG_FILE:${normalizedPath}:${filter || ''}`; + + // Register the topic if not already registered + if (!this.subscriptionTracker.getSubscriberCount(topicKey)) { + this.logger.debug(`Registering log file subscription topic: ${topicKey}`); + + this.subscriptionTracker.registerTopic( + topicKey, + // onStart handler + () => { + this.logger.debug(`Starting log file watcher for topic: ${topicKey}`); + this.startWatchingLogFile(normalizedPath, filter); + }, + // onStop handler + () => { + this.logger.debug(`Stopping log file watcher for topic: ${topicKey}`); + this.stopWatchingLogFile(normalizedPath, filter); + } + ); } - return PUBSUB_CHANNEL.LOG_FILE; + return topicKey; } /** * Start watching a log file for changes using chokidar * @param path Path to the log file + * @param filter Optional filter to apply */ - private async startWatchingLogFile(path: string): Promise { - try { - // Get initial file size - const stats = await stat(path); - let position = stats.size; - - // Create a watcher for the file using chokidar - const watcher = chokidar.watch(path, { - persistent: true, - awaitWriteFinish: { - stabilityThreshold: 300, - pollInterval: 100, - }, - }); - - watcher.on('change', async () => { - try { - const newStats = await stat(path); - - // If the file has grown - if (newStats.size > position) { - // Read only the new content - const stream = createReadStream(path, { - start: position, - end: newStats.size - 1, - }); - - let newContent = ''; - stream.on('data', (chunk) => { - newContent += chunk.toString(); - }); - - stream.on('end', () => { - if (newContent) { - pubsub.publish(PUBSUB_CHANNEL.LOG_FILE, { - logFile: { - path, - content: newContent, - totalLines: 0, // We don't need to count lines for updates - }, - }); - } - - // Update position for next read - position = newStats.size; - }); - } else if (newStats.size < position) { - // File was truncated, reset position and read from beginning - position = 0; - this.logger.debug(`File ${path} was truncated, resetting position`); + private startWatchingLogFile(path: string, filter?: string): void { + const watcherKey = `${path}:${filter || ''}`; - // Read the entire file content - const content = await this.getLogFileContent(path); + // If already watching, don't create a new watcher + if (this.logWatchers.has(watcherKey)) { + this.logger.debug(`Already watching log file: ${watcherKey}`); + return; + } - pubsub.publish(PUBSUB_CHANNEL.LOG_FILE, { - logFile: content, - }); + // Get initial file size and set up watcher + stat(path) + .then((stats) => { + let position = stats.size; + + // Create a watcher for the file using chokidar + const watcher = chokidar.watch(path, { + persistent: true, + awaitWriteFinish: { + stabilityThreshold: 300, + pollInterval: 100, + }, + }); + + watcher.on('change', async () => { + try { + const newStats = await stat(path); + + // If the file has grown + if (newStats.size > position) { + // Read only the new content + const stream = createReadStream(path, { + start: position, + end: newStats.size - 1, + }); + + let newContent = ''; + stream.on('data', (chunk) => { + newContent += chunk.toString(); + }); + + stream.on('end', () => { + if (newContent) { + // Filter content if filter is provided + const filteredContent = filter + ? this.filterContent(newContent, filter) + : newContent; + if (filteredContent) { + pubsub.publish(PUBSUB_CHANNEL.LOG_FILE, { + logFile: { + path, + content: filteredContent, + totalLines: 0, // We don't need to count lines for updates + }, + }); + } + } + + // Update position for next read + position = newStats.size; + }); + } else if (newStats.size < position) { + // File was truncated, reset position and read from beginning + position = 0; + this.logger.debug(`File ${path} was truncated, resetting position`); + + // Read the entire file content + const content = await this.getLogFileContent(path); + + pubsub.publish(PUBSUB_CHANNEL.LOG_FILE, { + logFile: content, + }); - position = newStats.size; + position = newStats.size; + } + } catch (error: unknown) { + this.logger.error(`Error processing file change for ${path}: ${error}`); } - } catch (error: unknown) { - this.logger.error(`Error processing file change for ${path}: ${error}`); - } - }); + }); - watcher.on('error', (error) => { - this.logger.error(`Chokidar watcher error for ${path}: ${error}`); - }); + watcher.on('error', (error) => { + this.logger.error(`Chokidar watcher error for ${path}: ${error}`); + }); - // Store the watcher and current position with initial subscription count of 1 - this.logWatchers.set(path, { watcher, position, subscriptionCount: 1 }); + // Store the watcher and current position + this.logWatchers.set(watcherKey, { watcher, position }); - this.logger.debug( - `Started watching log file with chokidar: ${path} (subscription count: 1)` - ); - } catch (error: unknown) { - this.logger.error(`Error setting up chokidar file watcher for ${path}: ${error}`); - } + this.logger.debug( + `Started watching log file with chokidar: ${path} with filter: ${filter || 'none'}` + ); + }) + .catch((error) => { + this.logger.error(`Error setting up file watcher for ${path}: ${error}`); + }); } /** * Stop watching a log file * @param path Path to the log file + * @param filter Optional filter that was used when starting the watcher */ - public stopWatchingLogFile(path: string): void { - const normalizedPath = join(this.logBasePath, basename(path)); - const watcher = this.logWatchers.get(normalizedPath); + private stopWatchingLogFile(path: string, filter?: string): void { + const watcherKey = `${path}:${filter || ''}`; + const watcher = this.logWatchers.get(watcherKey); if (watcher) { - // Decrement subscription count - watcher.subscriptionCount--; - this.logger.debug( - `Decremented subscription count for ${normalizedPath} to ${watcher.subscriptionCount}` - ); - - // Only close the watcher when subscription count reaches 0 - if (watcher.subscriptionCount <= 0) { - watcher.watcher.close(); - this.logWatchers.delete(normalizedPath); - this.logger.debug(`Stopped watching log file: ${normalizedPath} (no more subscribers)`); - } + watcher.watcher.close(); + this.logWatchers.delete(watcherKey); + this.logger.debug(`Stopped watching log file: ${watcherKey}`); } } + /** + * Filter content based on a filter string + * @param content The content to filter + * @param filter The filter string to apply + */ + private filterContent(content: string, filter: string): string { + const lines = content.split('\n'); + const filteredLines = lines.filter((line) => line.includes(filter)); + return filteredLines.join('\n'); + } + /** * Count the number of lines in a file * @param filePath Path to the file @@ -273,8 +307,9 @@ export class LogsService { * Read the last N lines of a file * @param filePath Path to the file * @param lineCount Number of lines to read + * @param filter Optional filter to apply */ - private async readLastLines(filePath: string, lineCount: number): Promise { + private async readLastLines(filePath: string, lineCount: number, filter?: string): Promise { const totalLines = await this.countFileLines(filePath); const linesToSkip = Math.max(0, totalLines - lineCount); @@ -291,7 +326,10 @@ export class LogsService { rl.on('line', (line) => { currentLine++; if (currentLine > linesToSkip) { - content += line + '\n'; + // Apply filter if provided + if (!filter || line.includes(filter)) { + content += line + '\n'; + } } }); @@ -310,11 +348,13 @@ export class LogsService { * @param filePath Path to the file * @param startLine Starting line number (1-indexed) * @param lineCount Number of lines to read + * @param filter Optional filter to apply */ private async readLinesFromPosition( filePath: string, startLine: number, - lineCount: number + lineCount: number, + filter?: string ): Promise { return new Promise((resolve, reject) => { let currentLine = 0; @@ -332,13 +372,16 @@ export class LogsService { // Skip lines before the starting position if (currentLine >= startLine) { - // Only read the requested number of lines - if (linesRead < lineCount) { - content += line + '\n'; - linesRead++; - } else { - // We've read enough lines, close the stream - rl.close(); + // Apply filter if provided + if (!filter || line.includes(filter)) { + // Only read the requested number of lines + if (linesRead < lineCount) { + content += line + '\n'; + linesRead++; + } else { + // We've read enough lines, close the stream + rl.close(); + } } } }); diff --git a/api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts b/api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts index dc0bed6985..a1da827161 100644 --- a/api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts +++ b/api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts @@ -9,7 +9,7 @@ import { CpuService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.service import { MemoryService } from '@app/unraid-api/graph/resolvers/info/memory/memory.service.js'; import { MetricsResolver } from '@app/unraid-api/graph/resolvers/metrics/metrics.resolver.js'; import { SubscriptionHelperService } from '@app/unraid-api/graph/services/subscription-helper.service.js'; -import { SubscriptionPollingService } from '@app/unraid-api/graph/services/subscription-polling.service.js'; +import { SubscriptionManagerService } from '@app/unraid-api/graph/services/subscription-manager.service.js'; import { SubscriptionTrackerService } from '@app/unraid-api/graph/services/subscription-tracker.service.js'; describe('MetricsResolver Integration Tests', () => { @@ -25,7 +25,7 @@ describe('MetricsResolver Integration Tests', () => { MemoryService, SubscriptionTrackerService, SubscriptionHelperService, - SubscriptionPollingService, + SubscriptionManagerService, ], }).compile(); @@ -36,8 +36,8 @@ describe('MetricsResolver Integration Tests', () => { afterEach(async () => { // Clean up polling service - const pollingService = module.get(SubscriptionPollingService); - pollingService.stopAll(); + const subscriptionManager = module.get(SubscriptionManagerService); + subscriptionManager.stopAll(); await module.close(); }); @@ -202,10 +202,13 @@ describe('MetricsResolver Integration Tests', () => { it('should handle errors in CPU polling gracefully', async () => { const service = module.get(CpuService); const trackerService = module.get(SubscriptionTrackerService); - const pollingService = module.get(SubscriptionPollingService); + const subscriptionManager = + module.get(SubscriptionManagerService); // Mock logger to capture error logs - const loggerSpy = vi.spyOn(pollingService['logger'], 'error').mockImplementation(() => {}); + const loggerSpy = vi + .spyOn(subscriptionManager['logger'], 'error') + .mockImplementation(() => {}); vi.spyOn(service, 'generateCpuLoad').mockRejectedValueOnce(new Error('CPU error')); // Trigger polling @@ -215,7 +218,7 @@ describe('MetricsResolver Integration Tests', () => { await new Promise((resolve) => setTimeout(resolve, 1100)); expect(loggerSpy).toHaveBeenCalledWith( - expect.stringContaining('Error in polling task'), + expect.stringContaining('Error in subscription callback'), expect.any(Error) ); @@ -226,10 +229,13 @@ describe('MetricsResolver Integration Tests', () => { it('should handle errors in memory polling gracefully', async () => { const service = module.get(MemoryService); const trackerService = module.get(SubscriptionTrackerService); - const pollingService = module.get(SubscriptionPollingService); + const subscriptionManager = + module.get(SubscriptionManagerService); // Mock logger to capture error logs - const loggerSpy = vi.spyOn(pollingService['logger'], 'error').mockImplementation(() => {}); + const loggerSpy = vi + .spyOn(subscriptionManager['logger'], 'error') + .mockImplementation(() => {}); vi.spyOn(service, 'generateMemoryLoad').mockRejectedValueOnce(new Error('Memory error')); // Trigger polling @@ -239,7 +245,7 @@ describe('MetricsResolver Integration Tests', () => { await new Promise((resolve) => setTimeout(resolve, 2100)); expect(loggerSpy).toHaveBeenCalledWith( - expect.stringContaining('Error in polling task'), + expect.stringContaining('Error in subscription callback'), expect.any(Error) ); @@ -251,22 +257,30 @@ describe('MetricsResolver Integration Tests', () => { describe('Polling cleanup on module destroy', () => { it('should clean up timers when module is destroyed', async () => { const trackerService = module.get(SubscriptionTrackerService); - const pollingService = module.get(SubscriptionPollingService); + const subscriptionManager = + module.get(SubscriptionManagerService); // Start polling trackerService.subscribe(PUBSUB_CHANNEL.CPU_UTILIZATION); trackerService.subscribe(PUBSUB_CHANNEL.MEMORY_UTILIZATION); - // Verify polling is active - expect(pollingService.isPolling(PUBSUB_CHANNEL.CPU_UTILIZATION)).toBe(true); - expect(pollingService.isPolling(PUBSUB_CHANNEL.MEMORY_UTILIZATION)).toBe(true); + // Wait a bit for subscriptions to be fully set up + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify subscriptions are active + expect(subscriptionManager.isSubscriptionActive(PUBSUB_CHANNEL.CPU_UTILIZATION)).toBe(true); + expect(subscriptionManager.isSubscriptionActive(PUBSUB_CHANNEL.MEMORY_UTILIZATION)).toBe( + true + ); // Clean up the module await module.close(); - // Timers should be cleaned up - expect(pollingService.isPolling(PUBSUB_CHANNEL.CPU_UTILIZATION)).toBe(false); - expect(pollingService.isPolling(PUBSUB_CHANNEL.MEMORY_UTILIZATION)).toBe(false); + // Subscriptions should be cleaned up + expect(subscriptionManager.isSubscriptionActive(PUBSUB_CHANNEL.CPU_UTILIZATION)).toBe(false); + expect(subscriptionManager.isSubscriptionActive(PUBSUB_CHANNEL.MEMORY_UTILIZATION)).toBe( + false + ); }); }); }); diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts index e3bc923903..d491a332ba 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.integration.test.ts @@ -18,12 +18,14 @@ describe('OidcAuthService Integration Tests - Enhanced Logging', () => { let debugLogs: string[] = []; let errorLogs: string[] = []; let warnLogs: string[] = []; + let logLogs: string[] = []; beforeEach(async () => { // Clear log arrays debugLogs = []; errorLogs = []; warnLogs = []; + logLogs = []; const module: TestingModule = await Test.createTestingModule({ imports: [ @@ -76,7 +78,9 @@ describe('OidcAuthService Integration Tests - Enhanced Logging', () => { warn: vi.spyOn(Logger.prototype, 'warn').mockImplementation((message: string) => { warnLogs.push(message); }), - log: vi.spyOn(Logger.prototype, 'log').mockImplementation(() => {}), + log: vi.spyOn(Logger.prototype, 'log').mockImplementation((message: string) => { + logLogs.push(message); + }), verbose: vi.spyOn(Logger.prototype, 'verbose').mockImplementation(() => {}), }; }); @@ -172,9 +176,9 @@ describe('OidcAuthService Integration Tests - Enhanced Logging', () => { // Expected to fail } - // Check that HTTP status details are logged - expect(debugLogs.some((log) => log.includes('Discovery URL:'))).toBe(true); - expect(debugLogs.some((log) => log.includes('Client ID:'))).toBe(true); + // Check that HTTP status details are logged (now in log level) + expect(logLogs.some((log) => log.includes('Discovery URL:'))).toBe(true); + expect(logLogs.some((log) => log.includes('Client ID:'))).toBe(true); }); it('should log authorization URL building details', async () => { @@ -198,8 +202,8 @@ describe('OidcAuthService Integration Tests - Enhanced Logging', () => { ); // Verify URL building logs - expect(debugLogs.some((log) => log.includes('Built authorization URL'))).toBe(true); - expect(debugLogs.some((log) => log.includes('Authorization parameters:'))).toBe(true); + expect(logLogs.some((log) => log.includes('Built authorization URL'))).toBe(true); + expect(logLogs.some((log) => log.includes('Authorization parameters:'))).toBe(true); } catch (error) { // May fail due to real discovery, but we're interested in the logs } @@ -227,8 +231,8 @@ describe('OidcAuthService Integration Tests - Enhanced Logging', () => { ); // Verify manual endpoint logs - expect(debugLogs.some((log) => log.includes('Built authorization URL:'))).toBe(true); - expect(debugLogs.some((log) => log.includes('client_id=test-client-id'))).toBe(true); + expect(logLogs.some((log) => log.includes('Built authorization URL'))).toBe(true); + expect(logLogs.some((log) => log.includes('client_id=test-client-id'))).toBe(true); expect(authUrl).toContain('https://auth.example.com/authorize'); }); @@ -292,9 +296,9 @@ describe('OidcAuthService Integration Tests - Enhanced Logging', () => { // May fail due to network, but we're checking logs } - // Verify discovery logging - expect(debugLogs.some((log) => log.includes('Starting OIDC discovery'))).toBe(true); - expect(debugLogs.some((log) => log.includes('Discovery URL:'))).toBe(true); + // Verify discovery logging (now in log level) + expect(logLogs.some((log) => log.includes('Starting discovery'))).toBe(true); + expect(logLogs.some((log) => log.includes('Discovery URL:'))).toBe(true); }); it('should log discovery failures with malformed JSON response', async () => { diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts index d36c879739..97a5a35189 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts @@ -63,8 +63,8 @@ export class OidcAuthService { authUrl.searchParams.set('state', secureState); authUrl.searchParams.set('response_type', 'code'); - this.logger.debug(`Built authorization URL: ${authUrl.href}`); - this.logger.debug( + this.logger.log(`Built authorization URL for provider ${provider.id}`); + this.logger.log( `Authorization parameters: client_id=${provider.clientId}, redirect_uri=${redirectUri}, scope=${provider.scopes.join(' ')}, response_type=code` ); @@ -94,8 +94,8 @@ export class OidcAuthService { const authUrl = client.buildAuthorizationUrl(config, parameters); - this.logger.debug(`Built authorization URL via discovery: ${authUrl.href}`); - this.logger.debug(`Authorization parameters: ${JSON.stringify(parameters, null, 2)}`); + this.logger.log(`Built authorization URL via discovery for provider ${provider.id}`); + this.logger.log(`Authorization parameters: ${JSON.stringify(parameters, null, 2)}`); return authUrl.href; } @@ -798,15 +798,7 @@ export class OidcAuthService { } private buildOriginWithPort(url: URL): string { - const { protocol, hostname, port } = url; - - // Check if port is empty, or is default for the protocol - const isDefaultPort = - !port || - (protocol === 'https:' && port === '443') || - (protocol === 'http:' && port === '80'); - - // Build origin with port only if non-default - return isDefaultPort ? `${protocol}//${hostname}` : `${protocol}//${hostname}:${port}`; + // URL.origin properly handles IPv6, default ports, and URL composition + return url.origin; } } diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts index d4ffee1781..c12ee029cc 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts @@ -1,4 +1,4 @@ -import { Injectable, Logger } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { RuleEffect } from '@jsonforms/core'; @@ -14,7 +14,6 @@ import { import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/oidc-validation.service.js'; import { createAccordionLayout, - createLabeledControl, createSimpleLabeledControl, } from '@app/unraid-api/graph/utils/form-utils.js'; import { SettingSlice } from '@app/unraid-api/types/json-forms.js'; diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-error.helper.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-error.helper.ts new file mode 100644 index 0000000000..3fe4fb37f9 --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-error.helper.ts @@ -0,0 +1,263 @@ +import { Logger } from '@nestjs/common'; + +export interface OidcErrorDetails { + userFriendlyError: string; + details: Record; +} + +export class OidcErrorHelper { + private static readonly logger = new Logger(OidcErrorHelper.name); + + /** + * Parse fetch errors and return user-friendly error messages + */ + static parseFetchError(error: unknown, issuerUrl?: string): OidcErrorDetails { + const errorMessage = error instanceof Error ? error.message : String(error); + let userFriendlyError = errorMessage; + let details: Record = { originalError: errorMessage }; + + // Extract cause information if available + if (error instanceof Error && 'cause' in error) { + const cause = (error as any).cause; + if (cause) { + this.logger.log(`Fetch error cause: ${JSON.stringify(cause, null, 2)}`); + + const errorCode = cause.code || ''; + const causeMessage = cause.message || ''; + + // Map error codes to user-friendly messages + switch (errorCode) { + case 'ENOTFOUND': + userFriendlyError = `Cannot resolve domain name. Please check that '${issuerUrl}' is accessible and spelled correctly.`; + details = { + type: 'DNS_ERROR', + originalError: errorMessage, + cause: causeMessage || errorCode, + }; + break; + + case 'ECONNREFUSED': + userFriendlyError = `Connection refused. The server at '${issuerUrl}' is not accepting connections.`; + details = { + type: 'CONNECTION_ERROR', + originalError: errorMessage, + cause: causeMessage || errorCode, + }; + break; + + case 'CERT_HAS_EXPIRED': + userFriendlyError = `SSL/TLS certificate error. The server certificate may be invalid or expired.`; + details = { + type: 'SSL_ERROR', + originalError: errorMessage, + cause: causeMessage || errorCode, + }; + break; + + case 'ETIMEDOUT': + userFriendlyError = `Connection timeout. The server at '${issuerUrl}' is not responding.`; + details = { + type: 'TIMEOUT_ERROR', + originalError: errorMessage, + cause: causeMessage || errorCode, + }; + break; + + default: + // Check message patterns if code doesn't match + if (causeMessage.includes('ENOTFOUND')) { + userFriendlyError = `Cannot resolve domain name. Please check that '${issuerUrl}' is accessible and spelled correctly.`; + details = { + type: 'DNS_ERROR', + originalError: errorMessage, + cause: causeMessage, + }; + } else if (causeMessage.includes('ECONNREFUSED')) { + userFriendlyError = `Connection refused. The server at '${issuerUrl}' is not accepting connections.`; + details = { + type: 'CONNECTION_ERROR', + originalError: errorMessage, + cause: causeMessage, + }; + } else if ( + causeMessage.includes('certificate') || + causeMessage.includes('SSL') || + causeMessage.includes('TLS') + ) { + userFriendlyError = `SSL/TLS certificate error. The server certificate may be invalid or expired.`; + details = { + type: 'SSL_ERROR', + originalError: errorMessage, + cause: causeMessage, + }; + } else if (causeMessage.includes('ETIMEDOUT')) { + userFriendlyError = `Connection timeout. The server at '${issuerUrl}' is not responding.`; + details = { + type: 'TIMEOUT_ERROR', + originalError: errorMessage, + cause: causeMessage, + }; + } else { + userFriendlyError = `Failed to connect to OIDC provider at '${issuerUrl}'. ${causeMessage || errorCode || 'Unknown network error'}`; + details = { + type: 'FETCH_ERROR', + originalError: errorMessage, + cause: causeMessage || errorCode, + }; + } + break; + } + } else { + // Generic fetch failed without cause + userFriendlyError = `Failed to connect to OIDC provider at '${issuerUrl}'. Please verify the URL is correct and accessible.`; + details = { type: 'FETCH_ERROR', originalError: errorMessage }; + } + } else if (errorMessage.includes('fetch failed')) { + // Fetch failed but no cause information + userFriendlyError = `Failed to connect to OIDC provider at '${issuerUrl}'. Please verify the URL is correct and accessible.`; + details = { type: 'FETCH_ERROR', originalError: errorMessage }; + } + + return { userFriendlyError, details }; + } + + /** + * Parse HTTP status errors and return user-friendly error messages + */ + static parseHttpError(errorMessage: string, issuerUrl?: string): OidcErrorDetails { + let userFriendlyError = errorMessage; + let details: Record = { originalError: errorMessage }; + + if (errorMessage.includes('404') || errorMessage.includes('Not Found')) { + const baseUrl = issuerUrl?.endsWith('/.well-known/openid-configuration') + ? issuerUrl.replace('/.well-known/openid-configuration', '') + : issuerUrl; + userFriendlyError = `OIDC discovery endpoint not found. Please verify that '${baseUrl}/.well-known/openid-configuration' exists.`; + details = { type: 'DISCOVERY_NOT_FOUND', originalError: errorMessage }; + } else if (errorMessage.includes('401') || errorMessage.includes('403')) { + userFriendlyError = `Access denied to discovery endpoint. Please check the issuer URL and any authentication requirements.`; + details = { type: 'AUTHENTICATION_ERROR', originalError: errorMessage }; + } else if (errorMessage.includes('unexpected HTTP response status code')) { + // Extract status code if possible + const statusMatch = errorMessage.match(/status code (\d+)/); + const statusCode = statusMatch ? statusMatch[1] : 'unknown'; + const baseUrl = issuerUrl?.endsWith('/.well-known/openid-configuration') + ? issuerUrl.replace('/.well-known/openid-configuration', '') + : issuerUrl; + userFriendlyError = `HTTP ${statusCode} error from discovery endpoint. Please check that '${baseUrl}/.well-known/openid-configuration' returns a valid OIDC discovery document.`; + details = { type: 'HTTP_STATUS_ERROR', statusCode, originalError: errorMessage }; + } + + return { userFriendlyError, details }; + } + + /** + * Parse generic OIDC errors and return user-friendly error messages + */ + static parseGenericError(error: unknown, issuerUrl?: string): OidcErrorDetails { + const errorMessage = error instanceof Error ? error.message : String(error); + let userFriendlyError = errorMessage; + let details: Record = { originalError: errorMessage }; + + // Check for specific error patterns + if (errorMessage.includes('getaddrinfo ENOTFOUND')) { + userFriendlyError = `Cannot resolve domain name. Please check that '${issuerUrl}' is accessible and spelled correctly.`; + details = { type: 'DNS_ERROR', originalError: errorMessage }; + } else if (errorMessage.includes('ECONNREFUSED')) { + userFriendlyError = `Connection refused. The server at '${issuerUrl}' is not accepting connections.`; + details = { type: 'CONNECTION_ERROR', originalError: errorMessage }; + } else if (errorMessage.includes('ECONNRESET') || errorMessage.includes('ETIMEDOUT')) { + userFriendlyError = `Connection timeout. The server at '${issuerUrl}' is not responding.`; + details = { type: 'TIMEOUT_ERROR', originalError: errorMessage }; + } else if ( + errorMessage.includes('certificate') || + errorMessage.includes('SSL') || + errorMessage.includes('TLS') + ) { + userFriendlyError = `SSL/TLS certificate error. The server certificate may be invalid or expired.`; + details = { type: 'SSL_ERROR', originalError: errorMessage }; + } else if (errorMessage.includes('JSON') || errorMessage.includes('parse')) { + userFriendlyError = `Invalid OIDC discovery response. The server returned malformed JSON.`; + details = { type: 'INVALID_JSON', originalError: errorMessage }; + } else if (error && (error as any).code === 'OAUTH_RESPONSE_IS_NOT_CONFORM') { + const baseUrl = issuerUrl?.endsWith('/.well-known/openid-configuration') + ? issuerUrl.replace('/.well-known/openid-configuration', '') + : issuerUrl; + userFriendlyError = `Invalid OIDC discovery document. The server at '${baseUrl}/.well-known/openid-configuration' returned a response that doesn't conform to the OpenID Connect Discovery specification. Please verify the endpoint returns valid OIDC metadata.`; + details = { type: 'INVALID_OIDC_DOCUMENT', originalError: errorMessage }; + } + + return { userFriendlyError, details }; + } + + /** + * Parse OIDC discovery errors and return user-friendly error messages + */ + static parseDiscoveryError(error: unknown, issuerUrl?: string): OidcErrorDetails { + const errorMessage = error instanceof Error ? error.message : String(error); + + // Log additional error details for debugging + if (error instanceof Error) { + this.logger.log(`Error type: ${error.constructor.name}`); + if ('stack' in error && error.stack) { + this.logger.debug(`Stack trace: ${error.stack}`); + } + if ('response' in error) { + const response = (error as any).response; + if (response) { + this.logger.log(`Response status: ${response.status}`); + this.logger.log(`Response body: ${response.body}`); + } + } + } + + // Check for fetch-specific errors first + if (errorMessage.includes('fetch failed')) { + return this.parseFetchError(error, issuerUrl); + } + + // Check for HTTP status errors + const httpError = this.parseHttpError(errorMessage, issuerUrl); + if (httpError.details.type !== undefined) { + return httpError; + } + + // Fall back to generic error parsing + return this.parseGenericError(error, issuerUrl); + } + + /** + * Log response details from an error + */ + static logErrorDetails(error: unknown, logger: Logger, context: string): void { + if (!(error instanceof Error)) { + return; + } + + logger.error(`${context} Error type: ${error.constructor.name}`); + logger.error(`${context} Error message: ${error.message}`); + + // Log response details if available + if ('response' in error) { + const response = (error as any).response; + if (response) { + logger.error(`${context} HTTP Status: ${response.status}`); + logger.error(`${context} HTTP Status Text: ${response.statusText}`); + if (response.body) { + logger.error( + `${context} Response body: ${ + typeof response.body === 'string' + ? response.body + : JSON.stringify(response.body, null, 2) + }` + ); + } + } + } + + // Log cause if available + if ('cause' in error && error.cause) { + logger.error(`${context} Error cause: ${JSON.stringify(error.cause, null, 2)}`); + } + } +} diff --git a/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts b/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts index 786905ea0b..0d65aa4e3a 100644 --- a/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts @@ -3,6 +3,7 @@ import { ConfigService } from '@nestjs/config'; import * as client from 'openid-client'; +import { OidcErrorHelper } from '@app/unraid-api/graph/resolvers/sso/oidc-error.helper.js'; import { OidcProvider } from '@app/unraid-api/graph/resolvers/sso/oidc-provider.model.js'; @Injectable() @@ -46,8 +47,8 @@ export class OidcValidationService { // Configure client options for HTTP if needed let clientOptions: any = undefined; if (serverUrl.protocol === 'http:') { - this.logger.debug( - `HTTP issuer URL detected for provider ${provider.id}: ${provider.issuer}` + this.logger.warn( + `HTTP issuer URL detected for provider ${provider.id}: ${provider.issuer} - This is insecure` ); clientOptions = { execute: [client.allowInsecureRequests], @@ -61,143 +62,23 @@ export class OidcValidationService { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; // Log the raw error for debugging - this.logger.debug(`Raw discovery error for ${provider.id}: ${errorMessage}`); + this.logger.log(`Raw discovery error for ${provider.id}: ${errorMessage}`); - // Log additional error details if available - if (error instanceof Error) { - this.logger.debug(`Error type: ${error.constructor.name}`); - if ('stack' in error && error.stack) { - this.logger.debug(`Stack trace: ${error.stack}`); - } - if ('response' in error) { - const response = (error as any).response; - if (response) { - this.logger.debug(`Response status: ${response.status}`); - this.logger.debug(`Response body: ${response.body}`); - } - } - } - - // Provide specific error messages for common issues - let userFriendlyError = errorMessage; - let details: Record = {}; - - // Check for fetch-specific errors (Node.js fetch API) - if (errorMessage.includes('fetch failed')) { - // Try to extract more specific information from the error - if (error instanceof Error && 'cause' in error) { - const cause = (error as any).cause; - if (cause) { - this.logger.debug(`Fetch error cause: ${JSON.stringify(cause, null, 2)}`); - - // Check the cause for specific error types - if (cause.code === 'ENOTFOUND' || cause.message?.includes('ENOTFOUND')) { - userFriendlyError = `Cannot resolve domain name. Please check that '${provider.issuer}' is accessible and spelled correctly.`; - details = { - type: 'DNS_ERROR', - originalError: errorMessage, - cause: cause.message || cause.code, - }; - } else if ( - cause.code === 'ECONNREFUSED' || - cause.message?.includes('ECONNREFUSED') - ) { - userFriendlyError = `Connection refused. The server at '${provider.issuer}' is not accepting connections.`; - details = { - type: 'CONNECTION_ERROR', - originalError: errorMessage, - cause: cause.message || cause.code, - }; - } else if ( - cause.code === 'CERT_HAS_EXPIRED' || - cause.message?.includes('certificate') - ) { - userFriendlyError = `SSL/TLS certificate error. The server certificate may be invalid or expired.`; - details = { - type: 'SSL_ERROR', - originalError: errorMessage, - cause: cause.message || cause.code, - }; - } else if (cause.code === 'ETIMEDOUT' || cause.message?.includes('ETIMEDOUT')) { - userFriendlyError = `Connection timeout. The server at '${provider.issuer}' is not responding.`; - details = { - type: 'TIMEOUT_ERROR', - originalError: errorMessage, - cause: cause.message || cause.code, - }; - } else { - // Generic fetch failed with cause details - userFriendlyError = `Failed to connect to OIDC provider at '${provider.issuer}'. ${cause.message || cause.code || 'Unknown network error'}`; - details = { - type: 'FETCH_ERROR', - originalError: errorMessage, - cause: cause.message || cause.code, - }; - } - } else { - // Generic fetch failed without cause - userFriendlyError = `Failed to connect to OIDC provider at '${provider.issuer}'. Please verify the URL is correct and accessible.`; - details = { type: 'FETCH_ERROR', originalError: errorMessage }; - } - } else { - // Fetch failed but no cause information - userFriendlyError = `Failed to connect to OIDC provider at '${provider.issuer}'. Please verify the URL is correct and accessible.`; - details = { type: 'FETCH_ERROR', originalError: errorMessage }; - } - } else if (errorMessage.includes('getaddrinfo ENOTFOUND')) { - userFriendlyError = `Cannot resolve domain name. Please check that '${provider.issuer}' is accessible and spelled correctly.`; - details = { type: 'DNS_ERROR', originalError: errorMessage }; - } else if (errorMessage.includes('ECONNREFUSED')) { - userFriendlyError = `Connection refused. The server at '${provider.issuer}' is not accepting connections.`; - details = { type: 'CONNECTION_ERROR', originalError: errorMessage }; - } else if (errorMessage.includes('ECONNRESET') || errorMessage.includes('ETIMEDOUT')) { - userFriendlyError = `Connection timeout. The server at '${provider.issuer}' is not responding.`; - details = { type: 'TIMEOUT_ERROR', originalError: errorMessage }; - } else if (errorMessage.includes('404') || errorMessage.includes('Not Found')) { - const baseUrl = provider.issuer?.endsWith('/.well-known/openid-configuration') - ? provider.issuer.replace('/.well-known/openid-configuration', '') - : provider.issuer; - userFriendlyError = `OIDC discovery endpoint not found. Please verify that '${baseUrl}/.well-known/openid-configuration' exists.`; - details = { type: 'DISCOVERY_NOT_FOUND', originalError: errorMessage }; - } else if (errorMessage.includes('401') || errorMessage.includes('403')) { - userFriendlyError = `Access denied to discovery endpoint. Please check the issuer URL and any authentication requirements.`; - details = { type: 'AUTHENTICATION_ERROR', originalError: errorMessage }; - } else if (errorMessage.includes('unexpected HTTP response status code')) { - // Extract status code if possible - const statusMatch = errorMessage.match(/status code (\d+)/); - const statusCode = statusMatch ? statusMatch[1] : 'unknown'; - const baseUrl = provider.issuer?.endsWith('/.well-known/openid-configuration') - ? provider.issuer.replace('/.well-known/openid-configuration', '') - : provider.issuer; - userFriendlyError = `HTTP ${statusCode} error from discovery endpoint. Please check that '${baseUrl}/.well-known/openid-configuration' returns a valid OIDC discovery document.`; - details = { type: 'HTTP_STATUS_ERROR', statusCode, originalError: errorMessage }; - } else if ( - errorMessage.includes('certificate') || - errorMessage.includes('SSL') || - errorMessage.includes('TLS') - ) { - userFriendlyError = `SSL/TLS certificate error. The server certificate may be invalid or expired.`; - details = { type: 'SSL_ERROR', originalError: errorMessage }; - } else if (errorMessage.includes('JSON') || errorMessage.includes('parse')) { - userFriendlyError = `Invalid OIDC discovery response. The server returned malformed JSON.`; - details = { type: 'INVALID_JSON', originalError: errorMessage }; - } else if (error && (error as any).code === 'OAUTH_RESPONSE_IS_NOT_CONFORM') { - const baseUrl = provider.issuer?.endsWith('/.well-known/openid-configuration') - ? provider.issuer.replace('/.well-known/openid-configuration', '') - : provider.issuer; - userFriendlyError = `Invalid OIDC discovery document. The server at '${baseUrl}/.well-known/openid-configuration' returned a response that doesn't conform to the OpenID Connect Discovery specification. Please verify the endpoint returns valid OIDC metadata.`; - details = { type: 'INVALID_OIDC_DOCUMENT', originalError: errorMessage }; - } + // Use the helper to parse the error + const { userFriendlyError, details } = OidcErrorHelper.parseDiscoveryError( + error, + provider.issuer + ); - this.logger.warn(`OIDC validation failed for provider ${provider.id}: ${errorMessage}`); + this.logger.error(`Validation failed for provider ${provider.id}: ${errorMessage}`); // Add debug logging for HTTP status errors if (errorMessage.includes('unexpected HTTP response status code')) { const baseUrl = provider.issuer?.endsWith('/.well-known/openid-configuration') ? provider.issuer.replace('/.well-known/openid-configuration', '') : provider.issuer; - this.logger.debug(`Attempted to fetch: ${baseUrl}/.well-known/openid-configuration`); - this.logger.debug(`Full error details: ${errorMessage}`); + this.logger.log(`Attempted to fetch: ${baseUrl}/.well-known/openid-configuration`); + this.logger.error(`Full error details: ${errorMessage}`); } return { @@ -221,14 +102,16 @@ export class OidcValidationService { const serverUrl = new URL(provider.issuer); const discoveryUrl = `${provider.issuer}/.well-known/openid-configuration`; - this.logger.debug(`Starting OIDC discovery for provider ${provider.id}`); - this.logger.debug(`Discovery URL: ${discoveryUrl}`); - this.logger.debug(`Client ID: ${provider.clientId}`); - this.logger.debug(`Client secret configured: ${provider.clientSecret ? 'Yes' : 'No'}`); + this.logger.log(`Starting discovery for provider ${provider.id}`); + this.logger.log(`Discovery URL: ${discoveryUrl}`); + this.logger.log(`Client ID: ${provider.clientId}`); + this.logger.log(`Client secret configured: ${provider.clientSecret ? 'Yes' : 'No'}`); // Use provided client options or create default options with HTTP support if needed if (!clientOptions && serverUrl.protocol === 'http:') { - this.logger.debug(`Allowing HTTP for ${provider.id} as specified by user`); + this.logger.warn( + `Allowing HTTP for ${provider.id} - This is insecure and should only be used for testing` + ); // For openid-client v6, use allowInsecureRequests in the execute array // This is deprecated but needed for local development with HTTP endpoints clientOptions = { @@ -245,21 +128,21 @@ export class OidcValidationService { clientOptions ); - this.logger.debug(`Discovery successful for ${provider.id}`); - this.logger.debug(`Discovery response metadata:`); - this.logger.debug(` - issuer: ${config.serverMetadata().issuer}`); - this.logger.debug( + this.logger.log(`Discovery successful for ${provider.id}`); + this.logger.log(`Discovery response metadata:`); + this.logger.log(` - issuer: ${config.serverMetadata().issuer}`); + this.logger.log( ` - authorization_endpoint: ${config.serverMetadata().authorization_endpoint}` ); - this.logger.debug(` - token_endpoint: ${config.serverMetadata().token_endpoint}`); - this.logger.debug( + this.logger.log(` - token_endpoint: ${config.serverMetadata().token_endpoint}`); + this.logger.log( ` - userinfo_endpoint: ${config.serverMetadata().userinfo_endpoint || 'not provided'}` ); - this.logger.debug(` - jwks_uri: ${config.serverMetadata().jwks_uri || 'not provided'}`); - this.logger.debug( + this.logger.log(` - jwks_uri: ${config.serverMetadata().jwks_uri || 'not provided'}`); + this.logger.log( ` - response_types_supported: ${config.serverMetadata().response_types_supported?.join(', ') || 'not provided'}` ); - this.logger.debug( + this.logger.log( ` - scopes_supported: ${config.serverMetadata().scopes_supported?.join(', ') || 'not provided'}` ); @@ -267,29 +150,8 @@ export class OidcValidationService { } catch (discoveryError) { this.logger.error(`Discovery failed for ${provider.id} at ${discoveryUrl}`); - if (discoveryError instanceof Error) { - this.logger.error(`Error type: ${discoveryError.constructor.name}`); - this.logger.error(`Error message: ${discoveryError.message}`); - - // Log response details if available - if ('response' in discoveryError) { - const response = (discoveryError as any).response; - if (response) { - this.logger.error(`HTTP Status: ${response.status}`); - this.logger.error(`HTTP Status Text: ${response.statusText}`); - if (response.body) { - this.logger.error( - `Response body: ${typeof response.body === 'string' ? response.body : JSON.stringify(response.body, null, 2)}` - ); - } - } - } - - // Log cause if available - if ('cause' in discoveryError && discoveryError.cause) { - this.logger.error(`Error cause: ${JSON.stringify(discoveryError.cause, null, 2)}`); - } - } + // Use the helper to log error details + OidcErrorHelper.logErrorDetails(discoveryError, this.logger, ''); throw discoveryError; } diff --git a/api/src/unraid-api/graph/services/services.module.ts b/api/src/unraid-api/graph/services/services.module.ts index 6f5399a05c..685e726a55 100644 --- a/api/src/unraid-api/graph/services/services.module.ts +++ b/api/src/unraid-api/graph/services/services.module.ts @@ -2,12 +2,12 @@ import { Module } from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { SubscriptionHelperService } from '@app/unraid-api/graph/services/subscription-helper.service.js'; -import { SubscriptionPollingService } from '@app/unraid-api/graph/services/subscription-polling.service.js'; +import { SubscriptionManagerService } from '@app/unraid-api/graph/services/subscription-manager.service.js'; import { SubscriptionTrackerService } from '@app/unraid-api/graph/services/subscription-tracker.service.js'; @Module({ imports: [], - providers: [SubscriptionTrackerService, SubscriptionHelperService, SubscriptionPollingService], - exports: [SubscriptionTrackerService, SubscriptionHelperService, SubscriptionPollingService], + providers: [SubscriptionTrackerService, SubscriptionHelperService, SubscriptionManagerService], + exports: [SubscriptionTrackerService, SubscriptionHelperService], // SubscriptionManagerService is internal }) export class ServicesModule {} diff --git a/api/src/unraid-api/graph/services/subscription-helper.service.ts b/api/src/unraid-api/graph/services/subscription-helper.service.ts index 2df982d127..3ef733601e 100644 --- a/api/src/unraid-api/graph/services/subscription-helper.service.ts +++ b/api/src/unraid-api/graph/services/subscription-helper.service.ts @@ -4,7 +4,25 @@ import { createSubscription, PUBSUB_CHANNEL } from '@app/core/pubsub.js'; import { SubscriptionTrackerService } from '@app/unraid-api/graph/services/subscription-tracker.service.js'; /** - * Helper service for creating tracked GraphQL subscriptions with automatic cleanup + * High-level helper service for creating GraphQL subscriptions with automatic cleanup. + * + * This service provides a convenient way to create GraphQL subscriptions that: + * - Automatically track subscriber count via SubscriptionTrackerService + * - Properly clean up resources when subscriptions end + * - Handle errors gracefully + * + * **When to use this service:** + * - In GraphQL resolvers when implementing subscriptions + * - When you need automatic reference counting for shared resources + * - When you want to ensure proper cleanup on subscription termination + * + * @example + * // In a GraphQL resolver + * \@Subscription(() => MetricsUpdate) + * async metricsSubscription() { + * // Topic must be registered first via SubscriptionTrackerService + * return this.subscriptionHelper.createTrackedSubscription(PUBSUB_CHANNEL.METRICS); + * } */ @Injectable() export class SubscriptionHelperService { diff --git a/api/src/unraid-api/graph/services/subscription-manager.service.ts b/api/src/unraid-api/graph/services/subscription-manager.service.ts new file mode 100644 index 0000000000..7d49b9ce91 --- /dev/null +++ b/api/src/unraid-api/graph/services/subscription-manager.service.ts @@ -0,0 +1,191 @@ +import { Injectable, Logger, OnModuleDestroy } from '@nestjs/common'; +import { SchedulerRegistry } from '@nestjs/schedule'; + +/** + * Configuration for managed subscriptions + */ +export interface SubscriptionConfig { + /** Unique identifier for the subscription */ + name: string; + + /** + * Polling interval in milliseconds. + * - If set to a number, the callback will be called at that interval + * - If null/undefined, the subscription is event-based (no polling) + */ + intervalMs?: number | null; + + /** Function to call periodically (for polling) or once (for setup) */ + callback: () => Promise; + + /** Optional function called when the subscription starts */ + onStart?: () => Promise; + + /** Optional function called when the subscription stops */ + onStop?: () => Promise; +} + +/** + * Low-level service for managing both polling and event-based subscriptions. + * + * ⚠️ **IMPORTANT**: This is an internal service. Do not use directly in resolvers or business logic. + * Instead, use one of the higher-level services: + * - **SubscriptionTrackerService**: For subscriptions that need reference counting + * - **SubscriptionHelperService**: For GraphQL subscriptions with automatic cleanup + * + * This service provides the underlying implementation for: + * - **Polling subscriptions**: Execute a callback at regular intervals + * - **Event-based subscriptions**: Set up event listeners or watchers that persist until stopped + * + * @internal + */ +@Injectable() +export class SubscriptionManagerService implements OnModuleDestroy { + private readonly logger = new Logger(SubscriptionManagerService.name); + private readonly activeSubscriptions = new Map< + string, + { isPolling: boolean; config?: SubscriptionConfig } + >(); + + constructor(private readonly schedulerRegistry: SchedulerRegistry) {} + + onModuleDestroy() { + this.stopAll(); + } + + /** + * Start a managed subscription (polling or event-based). + * + * @param config - The subscription configuration + * @throws Will throw an error if the onStart callback fails + */ + async startSubscription(config: SubscriptionConfig): Promise { + const { name, intervalMs, callback, onStart } = config; + + // Clean up any existing subscription with the same name + await this.stopSubscription(name); + + // Initialize subscription state with config + this.activeSubscriptions.set(name, { isPolling: false, config }); + + // Call onStart callback if provided + if (onStart) { + try { + await onStart(); + this.logger.debug(`Called onStart for '${name}'`); + } catch (error) { + this.logger.error(`Error in onStart for '${name}'`, error); + throw error; + } + } + + // If intervalMs is null, this is a continuous/event-based subscription + if (intervalMs === null || intervalMs === undefined) { + this.logger.debug(`Started continuous subscription for '${name}' (no polling)`); + return; + } + + // Create the polling function with guard against overlapping executions + const pollFunction = async () => { + const subscription = this.activeSubscriptions.get(name); + if (!subscription || subscription.isPolling) { + return; + } + + subscription.isPolling = true; + try { + await callback(); + } catch (error) { + this.logger.error(`Error in subscription callback '${name}'`, error); + } finally { + if (subscription) { + subscription.isPolling = false; + } + } + }; + + // Create and register the interval + const interval = setInterval(pollFunction, intervalMs); + this.schedulerRegistry.addInterval(name, interval); + + this.logger.debug(`Started polling for '${name}' every ${intervalMs}ms`); + } + + /** + * Stop a managed subscription. + * + * This will: + * 1. Stop any active polling interval + * 2. Call the onStop callback if provided + * 3. Clean up internal state + * + * @param name - The unique identifier of the subscription to stop + */ + async stopSubscription(name: string): Promise { + // Get the config before deleting + const subscription = this.activeSubscriptions.get(name); + const onStop = subscription?.config?.onStop; + + try { + if (this.schedulerRegistry.doesExist('interval', name)) { + this.schedulerRegistry.deleteInterval(name); + this.logger.debug(`Stopped polling interval for '${name}'`); + } + } catch (error) { + // Interval doesn't exist, which is fine + } + + // Call onStop callback if provided + if (onStop) { + try { + await onStop(); + this.logger.debug(`Called onStop for '${name}'`); + } catch (error) { + this.logger.error(`Error in onStop for '${name}'`, error); + } + } + + // Clean up subscription state + this.activeSubscriptions.delete(name); + } + + /** + * Stop all active subscriptions. + * + * This is automatically called when the module is destroyed. + */ + stopAll(): void { + const intervals = this.schedulerRegistry.getIntervals(); + intervals.forEach((key) => this.stopSubscription(key)); + this.activeSubscriptions.clear(); + } + + /** + * Check if a subscription is active. + * + * @param name - The unique identifier of the subscription + * @returns true if the subscription exists (either polling or event-based) + */ + isSubscriptionActive(name: string): boolean { + // Check both for polling intervals and event-based subscriptions + return this.activeSubscriptions.has(name) || this.schedulerRegistry.doesExist('interval', name); + } + + /** + * Get the total number of active subscriptions. + * + * @returns The count of all active subscriptions (polling and event-based) + */ + getActiveSubscriptionCount(): number { + return this.activeSubscriptions.size; + } + + /** + * Get a list of all active subscription names. + * + * @returns Array of subscription identifiers + */ + getActiveSubscriptionNames(): string[] { + return Array.from(this.activeSubscriptions.keys()); + } +} diff --git a/api/src/unraid-api/graph/services/subscription-polling.service.ts b/api/src/unraid-api/graph/services/subscription-polling.service.ts deleted file mode 100644 index f806b13df7..0000000000 --- a/api/src/unraid-api/graph/services/subscription-polling.service.ts +++ /dev/null @@ -1,91 +0,0 @@ -import { Injectable, Logger, OnModuleDestroy } from '@nestjs/common'; -import { SchedulerRegistry } from '@nestjs/schedule'; - -export interface PollingConfig { - name: string; - intervalMs: number; - callback: () => Promise; -} - -@Injectable() -export class SubscriptionPollingService implements OnModuleDestroy { - private readonly logger = new Logger(SubscriptionPollingService.name); - private readonly activePollers = new Map(); - - constructor(private readonly schedulerRegistry: SchedulerRegistry) {} - - onModuleDestroy() { - this.stopAll(); - } - - /** - * Start polling for a specific subscription topic - */ - startPolling(config: PollingConfig): void { - const { name, intervalMs, callback } = config; - - // Clean up any existing interval - this.stopPolling(name); - - // Initialize polling state - this.activePollers.set(name, { isPolling: false }); - - // Create the polling function with guard against overlapping executions - const pollFunction = async () => { - const poller = this.activePollers.get(name); - if (!poller || poller.isPolling) { - return; - } - - poller.isPolling = true; - try { - await callback(); - } catch (error) { - this.logger.error(`Error in polling task '${name}'`, error); - } finally { - if (poller) { - poller.isPolling = false; - } - } - }; - - // Create and register the interval - const interval = setInterval(pollFunction, intervalMs); - this.schedulerRegistry.addInterval(name, interval); - - this.logger.debug(`Started polling for '${name}' every ${intervalMs}ms`); - } - - /** - * Stop polling for a specific subscription topic - */ - stopPolling(name: string): void { - try { - if (this.schedulerRegistry.doesExist('interval', name)) { - this.schedulerRegistry.deleteInterval(name); - this.logger.debug(`Stopped polling for '${name}'`); - } - } catch (error) { - // Interval doesn't exist, which is fine - } - - // Clean up polling state - this.activePollers.delete(name); - } - - /** - * Stop all active polling tasks - */ - stopAll(): void { - const intervals = this.schedulerRegistry.getIntervals(); - intervals.forEach((key) => this.stopPolling(key)); - this.activePollers.clear(); - } - - /** - * Check if polling is active for a given name - */ - isPolling(name: string): boolean { - return this.schedulerRegistry.doesExist('interval', name); - } -} diff --git a/api/src/unraid-api/graph/services/subscription-tracker.service.ts b/api/src/unraid-api/graph/services/subscription-tracker.service.ts index 7876bab51e..aa93d972be 100644 --- a/api/src/unraid-api/graph/services/subscription-tracker.service.ts +++ b/api/src/unraid-api/graph/services/subscription-tracker.service.ts @@ -1,14 +1,44 @@ import { Injectable, Logger } from '@nestjs/common'; -import { SubscriptionPollingService } from '@app/unraid-api/graph/services/subscription-polling.service.js'; - +import { SubscriptionManagerService } from '@app/unraid-api/graph/services/subscription-manager.service.js'; + +/** + * Service for managing subscriptions with automatic reference counting. + * + * This service tracks the number of active subscribers for each topic and automatically + * starts/stops the underlying subscription based on subscriber count. + * + * **When to use this service:** + * - When you have multiple GraphQL subscriptions that share the same data source + * - When you need to start a resource (polling, file watcher, etc.) only when there are active subscribers + * - When you need automatic cleanup when the last subscriber disconnects + * + * @example + * // Register a polling subscription + * subscriptionTracker.registerTopic( + * 'metrics-update', + * async () => { + * const metrics = await fetchMetrics(); + * pubsub.publish('metrics-update', { metrics }); + * }, + * 5000 // Poll every 5 seconds + * ); + * + * @example + * // Register an event-based subscription (e.g., file watching) + * subscriptionTracker.registerTopic( + * 'log-file-updates', + * () => startFileWatcher('/var/log/app.log'), // onStart + * () => stopFileWatcher('/var/log/app.log') // onStop + * ); + */ @Injectable() export class SubscriptionTrackerService { private readonly logger = new Logger(SubscriptionTrackerService.name); private subscriberCounts = new Map(); private topicHandlers = new Map void; onStop: () => void }>(); - constructor(private readonly pollingService: SubscriptionPollingService) {} + constructor(private readonly subscriptionManager: SubscriptionManagerService) {} /** * Register a topic with optional polling support @@ -29,8 +59,8 @@ export class SubscriptionTrackerService { callback: async () => callbackOrOnStart(), }; this.topicHandlers.set(topic, { - onStart: () => this.pollingService.startPolling(pollingConfig), - onStop: () => this.pollingService.stopPolling(topic), + onStart: () => this.subscriptionManager.startSubscription(pollingConfig), + onStop: () => this.subscriptionManager.stopSubscription(topic), }); } else { // Legacy API: onStart and onStop handlers diff --git a/web/components/ConnectSettings/ConnectSettings.ce.vue b/web/components/ConnectSettings/ConnectSettings.ce.vue index b72cdcd1eb..1296ed2a74 100644 --- a/web/components/ConnectSettings/ConnectSettings.ce.vue +++ b/web/components/ConnectSettings/ConnectSettings.ce.vue @@ -16,6 +16,7 @@ import { useServerStore } from '~/store/server'; // import type { ConnectSettingsValues } from '~/composables/gql/graphql'; import { getConnectSettingsForm, updateConnectSettings } from './graphql/settings.query'; +import OidcDebugLogs from './OidcDebugLogs.vue'; const { connectPluginInstalled } = storeToRefs(useServerStore()); @@ -120,6 +121,9 @@ const onChange = ({ data }: { data: Record }) => { :readonly="isUpdating" @change="onChange" /> + + +
diff --git a/web/components/ConnectSettings/OidcDebugLogs.vue b/web/components/ConnectSettings/OidcDebugLogs.vue new file mode 100644 index 0000000000..8f8e7c3ede --- /dev/null +++ b/web/components/ConnectSettings/OidcDebugLogs.vue @@ -0,0 +1,100 @@ + + + diff --git a/web/components/Logs/FilteredLogModal.vue b/web/components/Logs/FilteredLogModal.vue new file mode 100644 index 0000000000..a615d474d4 --- /dev/null +++ b/web/components/Logs/FilteredLogModal.vue @@ -0,0 +1,67 @@ + + + diff --git a/web/components/Logs/LogViewer.ce.vue b/web/components/Logs/LogViewer.ce.vue index 2dafd4d3f8..d5e83467b5 100644 --- a/web/components/Logs/LogViewer.ce.vue +++ b/web/components/Logs/LogViewer.ce.vue @@ -20,10 +20,12 @@ interface LogFile { } // Component state -const selectedLogFile = ref(''); +const selectedLogFile = ref(null); const lineCount = ref(100); const autoScroll = ref(true); const highlightLanguage = ref('plaintext'); +const filterText = ref(''); +const presetFilter = ref('none'); // Available highlight languages const highlightLanguages = [ @@ -39,6 +41,15 @@ const highlightLanguages = [ { value: 'php', label: 'PHP' }, ]; +// Preset filter options +const presetFilters = [ + { value: 'none', label: 'No Filter' }, + { value: 'OIDC', label: 'OIDC Logs' }, + { value: 'ERROR', label: 'Errors' }, + { value: 'WARNING', label: 'Warnings' }, + { value: 'AUTH', label: 'Authentication' }, +]; + // Fetch log files const { result: logFilesResult, @@ -102,6 +113,15 @@ watch(selectedLogFile, (newValue) => { highlightLanguage.value = autoDetectLanguage(newValue); } }); + +// Watch for preset filter changes to update the filter text +watch(presetFilter, (newValue) => { + if (newValue && newValue !== 'none') { + filterText.value = newValue; + } else if (newValue === 'none') { + filterText.value = ''; + } +});