From 7e45c97a9a6e5c079c5e289d986ca8f0dd15fd6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Germ=C3=A1n=20Mart=C3=ADn?= Date: Wed, 17 Dec 2025 23:15:36 +0100 Subject: [PATCH] Fix: Solve comments to pull request --- apps/api/src/app/auth/auth.controller.ts | 29 +++++----- apps/api/src/app/auth/auth.module.ts | 10 ++-- apps/api/src/app/auth/auth.service.ts | 34 +++++++----- .../api/src/app/auth/interfaces/interfaces.ts | 16 ++++++ apps/api/src/app/auth/oidc-state.store.ts | 53 ++++++++----------- apps/api/src/app/auth/oidc.strategy.ts | 32 +++-------- .../user-account-settings.component.ts | 5 +- 7 files changed, 87 insertions(+), 92 deletions(-) diff --git a/apps/api/src/app/auth/auth.controller.ts b/apps/api/src/app/auth/auth.controller.ts index 2f42ab255..cb0bcf2ed 100644 --- a/apps/api/src/app/auth/auth.controller.ts +++ b/apps/api/src/app/auth/auth.controller.ts @@ -10,10 +10,12 @@ import { import { Body, + ConflictException, Controller, Get, HttpException, Logger, + NotFoundException, Param, Post, Req, @@ -126,17 +128,18 @@ export class AuthController { @Req() request: Request, @Res() response: Response ) { - const result = request.user as OidcValidationResult; + const { linkState, thirdPartyId, jwt } = + request.user as OidcValidationResult; const rootUrl = this.configurationService.get('ROOT_URL'); // Check if this is a link mode callback - if (result.linkState?.linkMode) { + if (linkState?.linkMode) { try { // Link the OIDC account to the existing user - await this.authService.linkOidcToUser( - result.linkState.userId, - result.thirdPartyId - ); + await this.authService.linkOidcToUser({ + thirdPartyId, + userId: linkState.userId + }); // Redirect to account page with success message response.redirect( @@ -150,14 +153,14 @@ export class AuthController { 'AuthController' ); - // Determine error type for frontend + // Determine error type for frontend based on error type let errorCode = 'unknown'; - if (errorMessage.includes('already linked')) { - errorCode = 'already-linked'; - } else if (errorMessage.includes('not found')) { + if (error instanceof ConflictException) { + errorCode = error.message.includes('token authentication') + ? 'invalid-provider' + : 'already-linked'; + } else if (error instanceof NotFoundException) { errorCode = 'invalid-session'; - } else if (errorMessage.includes('token authentication')) { - errorCode = 'invalid-provider'; } response.redirect( @@ -168,8 +171,6 @@ export class AuthController { } // Normal OIDC login flow - const jwt: string = result.jwt; - if (jwt) { response.redirect(`${rootUrl}/${DEFAULT_LANGUAGE_CODE}/auth/${jwt}`); } else { diff --git a/apps/api/src/app/auth/auth.module.ts b/apps/api/src/app/auth/auth.module.ts index 4d981fde1..0f24e38b0 100644 --- a/apps/api/src/app/auth/auth.module.ts +++ b/apps/api/src/app/auth/auth.module.ts @@ -17,6 +17,7 @@ import { AuthController } from './auth.controller'; import { AuthService } from './auth.service'; import { GoogleStrategy } from './google.strategy'; import { JwtStrategy } from './jwt.strategy'; +import { OidcStateStore } from './oidc-state.store'; import { OidcStrategy } from './oidc.strategy'; @Module({ @@ -39,11 +40,13 @@ import { OidcStrategy } from './oidc.strategy'; AuthService, GoogleStrategy, JwtStrategy, + OidcStateStore, { - inject: [AuthService, ConfigurationService], + inject: [AuthService, OidcStateStore, ConfigurationService], provide: OidcStrategy, useFactory: async ( authService: AuthService, + stateStore: OidcStateStore, configurationService: ConfigurationService ) => { const isOidcEnabled = configurationService.get( @@ -113,10 +116,7 @@ import { OidcStrategy } from './oidc.strategy'; clientSecret: configurationService.get('OIDC_CLIENT_SECRET') }; - // Pass JWT secret for link mode validation - const jwtSecret = configurationService.get('JWT_SECRET_KEY'); - - return new OidcStrategy(authService, { ...options, jwtSecret }); + return new OidcStrategy(authService, stateStore, options); } }, WebAuthService diff --git a/apps/api/src/app/auth/auth.service.ts b/apps/api/src/app/auth/auth.service.ts index 21d7c4248..aa6d2206d 100644 --- a/apps/api/src/app/auth/auth.service.ts +++ b/apps/api/src/app/auth/auth.service.ts @@ -4,13 +4,18 @@ import { PropertyService } from '@ghostfolio/api/services/property/property.serv import { ConflictException, + ForbiddenException, Injectable, InternalServerErrorException, - Logger + Logger, + NotFoundException } from '@nestjs/common'; import { JwtService } from '@nestjs/jwt'; -import { ValidateOAuthLoginParams } from './interfaces/interfaces'; +import { + LinkOidcToUserParams, + ValidateOAuthLoginParams +} from './interfaces/interfaces'; @Injectable() export class AuthService { @@ -61,7 +66,7 @@ export class AuthService { await this.propertyService.isUserSignupEnabled(); if (!isUserSignupEnabled) { - throw new Error('Sign up forbidden'); + throw new ForbiddenException('Sign up forbidden'); } // Create new user if not found @@ -92,16 +97,17 @@ export class AuthService { * The user must have provider ANONYMOUS (token-based auth). * The thirdPartyId must not be already linked to another user. * - * @param userId - The ID of the user to link - * @param thirdPartyId - The OIDC subject identifier + * @param params - Parameters for linking OIDC to user + * @param params.userId - The ID of the user to link + * @param params.thirdPartyId - The OIDC subject identifier * @returns JWT token for the linked user * @throws ConflictException if thirdPartyId is already linked to another user * @throws Error if user not found or has invalid provider */ - public async linkOidcToUser( - userId: string, - thirdPartyId: string - ): Promise { + public async linkOidcToUser({ + thirdPartyId, + userId + }: LinkOidcToUserParams): Promise { // Check if thirdPartyId is already linked to another user const [existingUser] = await this.userService.users({ where: { thirdPartyId } @@ -130,17 +136,19 @@ export class AuthService { const user = await this.userService.user({ id: userId }); if (!user) { - throw new Error('User not found'); + throw new NotFoundException('User not found'); } if (user.provider !== 'ANONYMOUS') { - throw new Error('Only users with token authentication can link OIDC'); + throw new ConflictException( + 'Only users with token authentication can link OIDC' + ); } // Update user with thirdPartyId and switch provider to OIDC await this.userService.updateUser({ - where: { id: userId }, - data: { thirdPartyId, provider: 'OIDC' } + data: { thirdPartyId, provider: 'OIDC' }, + where: { id: userId } }); return this.jwtService.sign({ id: userId }); diff --git a/apps/api/src/app/auth/interfaces/interfaces.ts b/apps/api/src/app/auth/interfaces/interfaces.ts index 7ddfe41d2..3fd35e4e3 100644 --- a/apps/api/src/app/auth/interfaces/interfaces.ts +++ b/apps/api/src/app/auth/interfaces/interfaces.ts @@ -25,6 +25,22 @@ export interface OidcProfile { sub?: string; } +export interface LinkOidcToUserParams { + thirdPartyId: string; + userId: string; +} + +export interface OidcLinkState { + linkMode: boolean; + userId: string; +} + +export interface OidcValidationResult { + jwt?: string; + linkState?: OidcLinkState; + thirdPartyId: string; +} + export interface ValidateOAuthLoginParams { provider: Provider; thirdPartyId: string; diff --git a/apps/api/src/app/auth/oidc-state.store.ts b/apps/api/src/app/auth/oidc-state.store.ts index df9b961c5..cf177e07e 100644 --- a/apps/api/src/app/auth/oidc-state.store.ts +++ b/apps/api/src/app/auth/oidc-state.store.ts @@ -1,24 +1,20 @@ -import { Logger } from '@nestjs/common'; -import * as jwt from 'jsonwebtoken'; +import { Injectable, Logger } from '@nestjs/common'; +import { JwtService } from '@nestjs/jwt'; import ms from 'ms'; -export interface OidcLinkState { - linkMode: boolean; - userId: string; -} +import { OidcLinkState } from './interfaces/interfaces'; /** * Custom state store for OIDC authentication that doesn't rely on express-session. * This store manages OAuth2 state parameters in memory with automatic cleanup. * Supports link mode for linking existing token-authenticated users to OIDC. */ +@Injectable() export class OidcStateStore { private readonly STATE_EXPIRY_MS = ms('10 minutes'); private pendingLinkState?: OidcLinkState; - private jwtSecret?: string; - private stateMap = new Map< string, { @@ -30,11 +26,23 @@ export class OidcStateStore { } >(); + public constructor(private readonly jwtService: JwtService) {} + /** - * Set the JWT secret for token validation in link mode + * Get and clear pending link state (used internally by store) */ - public setJwtSecret(secret: string) { - this.jwtSecret = secret; + public getPendingLinkState(): OidcLinkState | undefined { + const linkState = this.pendingLinkState; + this.pendingLinkState = undefined; + return linkState; + } + + /** + * Set link state for an existing or upcoming state entry. + * This allows the controller to attach user information before the OIDC flow starts. + */ + public setLinkStateForNextStore(linkState: OidcLinkState) { + this.pendingLinkState = linkState; } /** @@ -74,11 +82,9 @@ export class OidcStateStore { token = request.headers.authorization.substring(7); } - if (token && this.jwtSecret) { + if (token) { try { - const decoded = jwt.verify(token, this.jwtSecret) as { - id: string; - }; + const decoded = this.jwtService.verify<{ id: string }>(token); if (decoded?.id) { linkState = { linkMode: true, @@ -187,21 +193,4 @@ export class OidcStateStore { Date.now().toString(36) ); } - - /** - * Set link state for an existing or upcoming state entry. - * This allows the controller to attach user information before the OIDC flow starts. - */ - public setLinkStateForNextStore(linkState: OidcLinkState) { - this.pendingLinkState = linkState; - } - - /** - * Get and clear pending link state (used internally by store) - */ - public getPendingLinkState(): OidcLinkState | undefined { - const linkState = this.pendingLinkState; - this.pendingLinkState = undefined; - return linkState; - } } diff --git a/apps/api/src/app/auth/oidc.strategy.ts b/apps/api/src/app/auth/oidc.strategy.ts index 63fbdd492..095455a30 100644 --- a/apps/api/src/app/auth/oidc.strategy.ts +++ b/apps/api/src/app/auth/oidc.strategy.ts @@ -8,43 +8,25 @@ import { AuthService } from './auth.service'; import { OidcContext, OidcIdToken, + OidcLinkState, OidcParams, - OidcProfile + OidcProfile, + OidcValidationResult } from './interfaces/interfaces'; -import { OidcLinkState, OidcStateStore } from './oidc-state.store'; - -export interface OidcValidationResult { - jwt?: string; - linkState?: OidcLinkState; - thirdPartyId: string; -} - -export interface OidcStrategyOptions extends StrategyOptions { - jwtSecret?: string; -} +import { OidcStateStore } from './oidc-state.store'; @Injectable() export class OidcStrategy extends PassportStrategy(Strategy, 'oidc') { - private static readonly stateStore = new OidcStateStore(); - - public static getStateStore(): OidcStateStore { - return OidcStrategy.stateStore; - } - public constructor( private readonly authService: AuthService, - options: OidcStrategyOptions + stateStore: OidcStateStore, + options: StrategyOptions ) { super({ ...options, passReqToCallback: true, - store: OidcStrategy.stateStore + store: stateStore }); - - // Configure JWT secret for link mode validation - if (options.jwtSecret) { - OidcStrategy.stateStore.setJwtSecret(options.jwtSecret); - } } public async validate( diff --git a/apps/client/src/app/components/user-account-settings/user-account-settings.component.ts b/apps/client/src/app/components/user-account-settings/user-account-settings.component.ts index 99a8312aa..15bd4e6f4 100644 --- a/apps/client/src/app/components/user-account-settings/user-account-settings.component.ts +++ b/apps/client/src/app/components/user-account-settings/user-account-settings.component.ts @@ -121,11 +121,11 @@ export class GfUserAccountSettingsComponent implements OnDestroy, OnInit { this.baseCurrency = baseCurrency; this.currencies = currencies; - // Check global permissions for auth methods this.hasPermissionForAuthOidc = hasPermission( globalPermissions, permissions.enableAuthOidc ); + this.hasPermissionForAuthToken = hasPermission( globalPermissions, permissions.enableAuthToken @@ -145,6 +145,7 @@ export class GfUserAccountSettingsComponent implements OnDestroy, OnInit { this.hasPermissionForAuthToken && this.user.provider === 'ANONYMOUS' && !!this.user.thirdPartyId; + this.canLinkOidc = this.hasPermissionForAuthOidc && this.hasPermissionForAuthToken && @@ -217,8 +218,6 @@ export class GfUserAccountSettingsComponent implements OnDestroy, OnInit { return 'Security Token'; case 'GOOGLE': return 'Google'; - case 'INTERNET_IDENTITY': - return 'Internet Identity'; case 'OIDC': return 'OpenID Connect (OIDC)'; default: