From 15c3621e2d08ba15e93cecd0fd9fd69b961d705a Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Thu, 14 Nov 2024 14:08:41 +0000 Subject: [PATCH 01/13] Implement refresh token lock with polling --- .../bff/src/app/modules/auth/auth.service.ts | 32 +-- .../app/modules/auth/token-refresh.service.ts | 201 ++++++++++++++++++ .../bff/src/app/modules/ids/ids.service.ts | 77 ++----- .../bff/src/app/modules/ids/ids.types.ts | 12 -- .../bff/src/app/modules/proxy/proxy.module.ts | 12 +- .../src/app/modules/proxy/proxy.service.ts | 82 ++++--- .../src/app/modules/user/user.controller.ts | 18 +- .../bff/src/app/modules/user/user.module.ts | 12 +- .../bff/src/app/modules/user/user.service.ts | 85 ++++---- .../bff/src/app/services/error.service.ts | 100 +++++++++ .../bff/src/app/utils/get-cookie-options.ts | 13 ++ 11 files changed, 466 insertions(+), 178 deletions(-) create mode 100644 apps/services/bff/src/app/modules/auth/token-refresh.service.ts create mode 100644 apps/services/bff/src/app/services/error.service.ts create mode 100644 apps/services/bff/src/app/utils/get-cookie-options.ts diff --git a/apps/services/bff/src/app/modules/auth/auth.service.ts b/apps/services/bff/src/app/modules/auth/auth.service.ts index 0c7a36f31750..72e81cd08296 100644 --- a/apps/services/bff/src/app/modules/auth/auth.service.ts +++ b/apps/services/bff/src/app/modules/auth/auth.service.ts @@ -9,7 +9,7 @@ import { UnauthorizedException, } from '@nestjs/common' import { ConfigType } from '@nestjs/config' -import { CookieOptions, Request, Response } from 'express' +import { Request, Response } from 'express' import jwksClient from 'jwks-rsa' import { jwtDecode } from 'jwt-decode' @@ -26,6 +26,7 @@ import { CreateErrorQueryStrArgs, createErrorQueryStr, } from '../../utils/create-error-query-str' +import { getCookieOptions } from '../../utils/get-cookie-options' import { validateUri } from '../../utils/validate-uri' import { CacheService } from '../cache/cache.service' import { IdsService } from '../ids/ids.service' @@ -55,17 +56,6 @@ export class AuthService { this.baseUrl = this.config.ids.issuer } - private getCookieOptions(): CookieOptions { - return { - httpOnly: true, - secure: true, - // The lax setting allows cookies to be sent on top-level navigations (such as redirects), - // while still providing some protection against CSRF attacks. - sameSite: 'lax', - path: environment.keyPath, - } - } - /** * Creates the client base URL with the path appended. */ @@ -212,12 +202,8 @@ export class AuthService { prompt, }) - if (parResponse.type === 'error') { - throw parResponse.data - } - searchParams = new URLSearchParams({ - request_uri: parResponse.data.request_uri, + request_uri: parResponse.request_uri, client_id: this.config.ids.clientId, }) } else { @@ -297,13 +283,7 @@ export class AuthService { codeVerifier: loginAttemptData.codeVerifier, }) - if (tokenResponse.type === 'error') { - throw tokenResponse.data - } - - const updatedTokenResponse = await this.updateTokenCache( - tokenResponse.data, - ) + const updatedTokenResponse = await this.updateTokenCache(tokenResponse) // Clean up the login attempt from the cache since we have a successful login. this.cacheService @@ -316,7 +296,7 @@ export class AuthService { res.cookie( SESSION_COOKIE_NAME, updatedTokenResponse.userProfile.sid, - this.getCookieOptions(), + getCookieOptions(), ) // Check if there is an old session cookie and clean up the cache @@ -424,7 +404,7 @@ export class AuthService { * - Delete the current login from the cache * - Clear the session cookie */ - res.clearCookie(SESSION_COOKIE_NAME, this.getCookieOptions()) + res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) this.cacheService .delete(currentLoginCacheKey) diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts new file mode 100644 index 000000000000..5ca2d08a5b98 --- /dev/null +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts @@ -0,0 +1,201 @@ +import { LOGGER_PROVIDER, Logger } from '@island.is/logging' +import { Inject, Injectable } from '@nestjs/common' +import { CacheService } from '../cache/cache.service' +import { IdsService } from '../ids/ids.service' +import { AuthService } from './auth.service' +import { CachedTokenResponse } from './auth.types' + +/** + * Service responsible for handling token refresh operations + * Provides concurrent request protection and token refresh polling + */ +@Injectable() +export class TokenRefreshService { + private static POLL_INTERVAL = 100 // ms + private static MAX_POLL_TIME = 3000 // 3 seconds + + constructor( + @Inject(LOGGER_PROVIDER) + private logger: Logger, + + private readonly authService: AuthService, + private readonly cacheService: CacheService, + private readonly idsService: IdsService, + ) {} + + /** + * Creates a unique key for tracking refresh token operations in progress + * This key is used to prevent concurrent refresh token requests for the same session + * + * @param sid - Session ID + * @returns Formatted key string for refresh token tracking + */ + private createRefreshTokenKey(sid: string): string { + return `refresh_token_in_progress:${sid}` + } + + /** + * Creates a key for storing token response data in cache + * This key is used to store and retrieve the current token data for a session + * + * @param sid - Session ID + * @returns Formatted key string for token response data + */ + private createTokenResponseKey(sid: string): string { + return this.cacheService.createSessionKeyType('current', sid) + } + + /** + * Executes the token refresh operation and updates the cache + * This method: + * 1. Sets a flag in cache to indicate refresh is in progress + * 2. Requests new tokens from the identity server + * 3. Updates the cache with the new token data + * 4. Cleans up the refresh flag + * + * @param params.refreshTokenKey - Redis key for tracking refresh status + * @param params.encryptedRefreshToken - Encrypted refresh token for getting new tokens + * + * @returns Promise Updated token data + * @throws Will throw if token refresh fails or cache operations fail + */ + private async executeTokenRefresh({ + refreshTokenKey, + encryptedRefreshToken, + }: { + refreshTokenKey: string + encryptedRefreshToken: string + }) { + // Set refresh in progress + await this.cacheService.save({ + key: refreshTokenKey, + value: true, + ttl: TokenRefreshService.MAX_POLL_TIME, + }) + + const tokenResponse = await this.idsService.refreshToken( + encryptedRefreshToken, + ) + + // Update cache with new token data + const updatedTokenResponse = await this.authService.updateTokenCache( + tokenResponse, + ) + + // Delete the refresh token key to signal that the refresh operation is complete + await this.cacheService.delete(refreshTokenKey) + + return updatedTokenResponse + } + + /** + * Polls the cache to check if a refresh token operation has completed + * This prevents multiple concurrent refresh token requests and ensures + * all requests wait for the ongoing refresh to complete + * + * @param sid - Session ID + * + * @returns Promise that resolves when refresh is complete or rejects on timeout + * @throws Rejects if polling times out or encounters an error + */ + private async pollForRefreshCompletion(sid: string): Promise { + return new Promise((resolve, reject) => { + const timeoutId = setTimeout(() => { + clearInterval(pollInterval) + reject( + new Error( + `Polling timed out for token refresh completion for session ${sid}`, + ), + ) + }, TokenRefreshService.MAX_POLL_TIME) + + const pollInterval = setInterval(async () => { + try { + const refreshTokenInProgress = await this.cacheService.get( + this.createRefreshTokenKey(sid), + false, + ) + + if (!refreshTokenInProgress) { + clearInterval(pollInterval) + clearTimeout(timeoutId) + resolve() + } + } catch (error) { + clearInterval(pollInterval) + clearTimeout(timeoutId) + reject(new Error(`Error polling for refresh completion: ${error}`)) + } + }, TokenRefreshService.POLL_INTERVAL) + }) + } + + /** + * Handles the complete token refresh process with concurrent request protection + * This method: + * + * 1. Checks if a refresh is already in progress + * 2. If yes, waits for it to complete + * 3. If no, initiates a new refresh + * 4. Updates the cache with new token data + * 5. Cleans up tracking flags + * + * @param params.sid - Session ID + * @param params.encryptedRefreshToken - Encrypted refresh token to use for getting new tokens + * + * @returns Promise resolving to updated token response data + * @throws Forwards any errors from the refresh process after logging + */ + public async refreshToken({ + sid, + encryptedRefreshToken, + }: { + sid: string + encryptedRefreshToken: string + }): Promise { + const refreshTokenKey = this.createRefreshTokenKey(sid) + const tokenResponseKey = this.createTokenResponseKey(sid) + + try { + // Check if refresh is already in progress + const refreshTokenInProgress = await this.cacheService.get( + refreshTokenKey, + false, + ) + + if (refreshTokenInProgress) { + try { + // Wait for the ongoing refresh to complete + await this.pollForRefreshCompletion(sid) + + // Get the updated token response from cache + const tokenResponse = + await this.cacheService.get(tokenResponseKey) + + return tokenResponse + } catch (error) { + this.logger.error(error) + + // If polling times out, then retry the refresh + const updatedTokenResponse = await this.executeTokenRefresh({ + refreshTokenKey, + encryptedRefreshToken, + }) + + return updatedTokenResponse + } + } + + const updatedTokenResponse = await this.executeTokenRefresh({ + refreshTokenKey, + encryptedRefreshToken, + }) + + return updatedTokenResponse + } catch (error) { + this.logger.error(`Token refresh failed for sid: ${sid}`) + + throw error + } + } +} diff --git a/apps/services/bff/src/app/modules/ids/ids.service.ts b/apps/services/bff/src/app/modules/ids/ids.service.ts index 494c40550a02..6b84856e9b70 100644 --- a/apps/services/bff/src/app/modules/ids/ids.service.ts +++ b/apps/services/bff/src/app/modules/ids/ids.service.ts @@ -6,8 +6,6 @@ import { BffConfig } from '../../bff.config' import { CryptoService } from '../../services/crypto.service' import { ENHANCED_FETCH_PROVIDER_KEY } from '../enhancedFetch/enhanced-fetch.provider' import { - ApiResponse, - ErrorRes, GetLoginSearchParamsReturnValue, ParResponse, TokenResponse, @@ -35,60 +33,28 @@ export class IdsService { private async postRequest( endpoint: string, body: Record, - ): Promise> { - try { - const response = await this.enhancedFetch( - `${this.issuerUrl}${endpoint}`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - Authorization: this.createPARAuthorizationHeader(), - }, - body: new URLSearchParams(body).toString(), - }, - ) - - const contentType = response.headers.get('content-type') || '' - - if (contentType.includes('application/json')) { - const data = await response.json() - - if (!response.ok) { - // If error response from Ids is not in the expected format, throw the data as is - if (!data.error || !data.error_description) { - throw data - } - - return { - type: 'error', - data: { - error: data.error, - error_description: data.error_description, - }, - } as ErrorRes - } - - return { - type: 'success', - data: data as T, - } - } - - // Handle plain text responses - const textResponse = await response.text() - - if (!response.ok) { - throw textResponse - } - - return { - type: 'success', - data: textResponse, - } as ApiResponse - } catch (error) { - throw new Error(error) + ): Promise { + const response = await this.enhancedFetch(`${this.issuerUrl}${endpoint}`, { + method: 'POST', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + Authorization: this.createPARAuthorizationHeader(), + }, + body: new URLSearchParams(body).toString(), + }) + + const contentType = response.headers.get('content-type') || '' + + if (contentType.includes('application/json')) { + const data = await response.json() + + return data } + + // Handle plain text responses + const textResponse = await response.text() + + return textResponse as T } public getLoginSearchParams({ @@ -103,6 +69,7 @@ export class IdsService { prompt?: string }): GetLoginSearchParamsReturnValue { const { ids } = this.config + return { client_id: ids.clientId, redirect_uri: this.config.callbacksRedirectUris.login, diff --git a/apps/services/bff/src/app/modules/ids/ids.types.ts b/apps/services/bff/src/app/modules/ids/ids.types.ts index 8f88ced6f832..9b353df763a1 100644 --- a/apps/services/bff/src/app/modules/ids/ids.types.ts +++ b/apps/services/bff/src/app/modules/ids/ids.types.ts @@ -47,18 +47,6 @@ export type TokenResponse = { scope: string } -export interface SuccessResponse { - type: 'success' - data: T -} - -export interface ErrorRes { - type: 'error' - data: ErrorResponse -} - -export type ApiResponse = SuccessResponse | ErrorRes - export type LogoutTokenPayload = { // Issuer of the token iss: string diff --git a/apps/services/bff/src/app/modules/proxy/proxy.module.ts b/apps/services/bff/src/app/modules/proxy/proxy.module.ts index d767e6c014d6..6cbeba5c5733 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.module.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.module.ts @@ -1,13 +1,21 @@ import { Module } from '@nestjs/common' +import { CryptoService } from '../../services/crypto.service' +import { ErrorService } from '../../services/error.service' import { AuthModule } from '../auth/auth.module' +import { TokenRefreshService } from '../auth/token-refresh.service' import { IdsService } from '../ids/ids.service' import { ProxyController } from './proxy.controller' import { ProxyService } from './proxy.service' -import { CryptoService } from '../../services/crypto.service' @Module({ imports: [AuthModule], controllers: [ProxyController], - providers: [ProxyService, IdsService, CryptoService], + providers: [ + ProxyService, + CryptoService, + TokenRefreshService, + ErrorService, + IdsService, + ], }) export class ProxyModule {} diff --git a/apps/services/bff/src/app/modules/proxy/proxy.service.ts b/apps/services/bff/src/app/modules/proxy/proxy.service.ts index e075b0e64c71..d9cc4eb2b144 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.service.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.service.ts @@ -8,7 +8,7 @@ import { } from '@nestjs/common' import { ConfigType } from '@nestjs/config' import AgentKeepAlive, { HttpsAgent } from 'agentkeepalive' -import { Request, Response } from 'express' +import type { Request, Response } from 'express' import fetch from 'node-fetch' import { BffConfig } from '../../bff.config' import { CryptoService } from '../../services/crypto.service' @@ -17,12 +17,12 @@ import { AGENT_DEFAULT_FREE_SOCKET_TIMEOUT, AGENT_DEFAULT_MAX_SOCKETS, } from '@island.is/shared/constants' -import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' +import { SESSION_COOKIE_NAME } from '../../constants/cookies' +import { ErrorService } from '../../services/error.service' import { validateUri } from '../../utils/validate-uri' -import { AuthService } from '../auth/auth.service' import { CachedTokenResponse } from '../auth/auth.types' +import { TokenRefreshService } from '../auth/token-refresh.service' import { CacheService } from '../cache/cache.service' -import { IdsService } from '../ids/ids.service' import { ApiProxyDto } from './dto/api-proxy.dto' const droppedResponseHeaders = [ @@ -41,6 +41,7 @@ const agentOptions: AgentKeepAlive.HttpOptions = { freeSocketTimeout: AGENT_DEFAULT_FREE_SOCKET_TIMEOUT, maxSockets: AGENT_DEFAULT_MAX_SOCKETS, } + const customAgent = new AgentKeepAlive(agentOptions) /** * Only applies to none same domain requests. @@ -63,9 +64,9 @@ export class ProxyService { private readonly config: ConfigType, private readonly cacheService: CacheService, - private readonly idsService: IdsService, - private readonly authService: AuthService, private readonly cryptoService: CryptoService, + private readonly tokenRefreshService: TokenRefreshService, + private readonly errorService: ErrorService, ) {} /** @@ -73,47 +74,53 @@ export class ProxyService { * - If the token is expired, it will attempt to update tokens with the refresh token from cache. * - Then access token is decrypted and returned. */ - private async getAccessToken(req: Request) { - const sid = req.cookies['sid'] + private async getAccessToken({ + req, + res, + }: { + req: Request + res: Response + }): Promise { + const sid = req.cookies[SESSION_COOKIE_NAME] if (!sid) { throw new UnauthorizedException() } + const tokenResponseKey = this.cacheService.createSessionKeyType( + 'current', + sid, + ) + try { let cachedTokenResponse = - await this.cacheService.get( - this.cacheService.createSessionKeyType('current', sid), - ) - - if (hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp)) { - const tokenResponse = await this.idsService.refreshToken( - cachedTokenResponse.encryptedRefreshToken, - ) - - if (tokenResponse.type === 'error') { - throw tokenResponse.data - } + await this.cacheService.get(tokenResponseKey) - cachedTokenResponse = await this.authService.updateTokenCache( - tokenResponse.data, - ) + if (cachedTokenResponse.accessTokenExp) { + cachedTokenResponse = await this.tokenRefreshService.refreshToken({ + sid, + encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, + }) } return this.cryptoService.decrypt( cachedTokenResponse.encryptedAccessToken, ) } catch (error) { - this.logger.error('Error getting access token:', error) - - throw new UnauthorizedException() + return this.errorService.handleAuthorizedError({ + error, + res, + sid, + tokenResponseKey, + operation: `${ProxyService.name}.getAccessToken`, + }) } } /** * This method proxies the request to the target URL and streams the response back to the client. */ - async executeStreamRequest({ + private async executeStreamRequest({ targetUrl, accessToken, req, @@ -144,31 +151,23 @@ export class ProxyService { }, }) - // Set the status code of the response res.status(response.status) response.headers.forEach((value, key) => { - // Only set headers that are not in the droppedResponseHeaders array if (!droppedResponseHeaders.includes(key.toLowerCase())) { res.setHeader(key, value) } }) - // Pipe the response body directly to the client response.body.pipe(res) response.body.on('error', (err) => { this.logger.error('Proxy stream error:', err) - - // This check ensures that `res.end()` is only called if the response has not already been ended. if (!res.writableEnded) { - // Ensure the response is properly ended if an error occurs res.end() } }) - // Make sure to end the response when the stream ends, - // so that the client knows the request is complete. response.body.on('end', () => { if (!res.writableEnded) { res.end() @@ -176,7 +175,6 @@ export class ProxyService { }) } catch (error) { this.logger.error('Error during proxy request processing: ', error) - res.status(error.status || 500).send('Failed to proxy request') } } @@ -192,13 +190,13 @@ export class ProxyService { req: Request res: Response }): Promise { - const accessToken = await this.getAccessToken(req) + const accessToken = await this.getAccessToken({ req, res }) const queryString = req.url.split('?')[1] const targetUrl = `${this.config.graphqlApiEndpoint}${ queryString ? `?${queryString}` : '' }` - this.executeStreamRequest({ + await this.executeStreamRequest({ accessToken, targetUrl, req, @@ -210,7 +208,7 @@ export class ProxyService { * Forwards an incoming HTTP GET request to the specified URL (provided in the query string), * managing authentication, refreshing tokens if needed, and streaming the response back to the client. */ - async forwardGetApiRequest({ + public async forwardGetApiRequest({ req, res, query, @@ -218,16 +216,16 @@ export class ProxyService { req: Request res: Response query: ApiProxyDto - }) { + }): Promise { const { url } = query if (!validateUri(url, this.config.allowedExternalApiUrls)) { this.logger.error('Invalid external api url provided:', url) - throw new BadRequestException('Proxing url failed!') + throw new BadRequestException('Proxying url failed!') } - const accessToken = await this.getAccessToken(req) + const accessToken = await this.getAccessToken({ req, res }) this.executeStreamRequest({ accessToken, diff --git a/apps/services/bff/src/app/modules/user/user.controller.ts b/apps/services/bff/src/app/modules/user/user.controller.ts index 2e3045db2b67..9cb9e6e173b4 100644 --- a/apps/services/bff/src/app/modules/user/user.controller.ts +++ b/apps/services/bff/src/app/modules/user/user.controller.ts @@ -1,7 +1,14 @@ -import type { Request } from 'express' +import type { Request, Response } from 'express' import type { BffUser } from '@island.is/shared/types' -import { Controller, Get, Query, Req, VERSION_NEUTRAL } from '@nestjs/common' +import { + Controller, + Get, + Query, + Req, + Res, + VERSION_NEUTRAL, +} from '@nestjs/common' import { qsValidationPipe } from '../../utils/qs-validation-pipe' import { GetUserDto } from './dto/get-user.dto' @@ -17,9 +24,14 @@ export class UserController { @Get() async getUser( @Req() req: Request, + @Res({ passthrough: true }) res: Response, @Query(qsValidationPipe) query: GetUserDto, ): Promise { - return this.userService.getUser(req, query.refresh === 'true') + return this.userService.getUser({ + req, + res, + refresh: query.refresh === 'true', + }) } } diff --git a/apps/services/bff/src/app/modules/user/user.module.ts b/apps/services/bff/src/app/modules/user/user.module.ts index 6ad5d5368d65..d95c6b1cd7cc 100644 --- a/apps/services/bff/src/app/modules/user/user.module.ts +++ b/apps/services/bff/src/app/modules/user/user.module.ts @@ -1,13 +1,21 @@ import { Module } from '@nestjs/common' +import { CryptoService } from '../../services/crypto.service' +import { ErrorService } from '../../services/error.service' import { AuthModule } from '../auth/auth.module' +import { TokenRefreshService } from '../auth/token-refresh.service' import { IdsService } from '../ids/ids.service' import { UserController } from './user.controller' import { UserService } from './user.service' -import { CryptoService } from '../../services/crypto.service' @Module({ imports: [AuthModule], controllers: [UserController], - providers: [UserService, IdsService, CryptoService], + providers: [ + UserService, + IdsService, + CryptoService, + TokenRefreshService, + ErrorService, + ], }) export class UserModule {} diff --git a/apps/services/bff/src/app/modules/user/user.service.ts b/apps/services/bff/src/app/modules/user/user.service.ts index 2400ad7056e6..9ecacd2eaa31 100644 --- a/apps/services/bff/src/app/modules/user/user.service.ts +++ b/apps/services/bff/src/app/modules/user/user.service.ts @@ -1,30 +1,27 @@ -import type { Logger } from '@island.is/logging' -import { LOGGER_PROVIDER } from '@island.is/logging' -import { Inject, Injectable, UnauthorizedException } from '@nestjs/common' -import { Request } from 'express' +import { Injectable, UnauthorizedException } from '@nestjs/common' +import type { Request, Response } from 'express' import { BffUser } from '@island.is/shared/types' import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { CryptoService } from '../../services/crypto.service' +import { ErrorService } from '../../services/error.service' import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' -import { AuthService } from '../auth/auth.service' import { CachedTokenResponse } from '../auth/auth.types' +import { TokenRefreshService } from '../auth/token-refresh.service' import { CacheService } from '../cache/cache.service' -import { IdsService } from '../ids/ids.service' @Injectable() export class UserService { constructor( - @Inject(LOGGER_PROVIDER) - private logger: Logger, - - private readonly cryptoService: CryptoService, private readonly cacheService: CacheService, - private readonly idsService: IdsService, - private readonly authService: AuthService, + private readonly tokenRefreshService: TokenRefreshService, + private readonly errorService: ErrorService, ) {} + /** + * Maps the cached token response to BFF user format + */ private mapToBffUser(value: CachedTokenResponse): BffUser { return { scopes: value.scopes, @@ -32,21 +29,40 @@ export class UserService { } } - public async getUser(req: Request, refresh = true): Promise { + /** + * Creates a key for storing token response data in cache + */ + private createTokenResponseKey(sid: string): string { + return this.cacheService.createSessionKeyType('current', sid) + } + + /** + * Gets the current user data, refreshing the token if needed + */ + public async getUser({ + req, + res, + refresh = true, + }: { + req: Request + res: Response + refresh: boolean + }): Promise { const sid = req.cookies[SESSION_COOKIE_NAME] if (!sid) { throw new UnauthorizedException() } - try { - const cachedTokenResponse = - await this.cacheService.get( - this.cacheService.createSessionKeyType('current', sid), - // Do not throw error if the key is not found - false, - ) + const tokenResponseKey = this.createTokenResponseKey(sid) + const cachedTokenResponse = + await this.cacheService.get( + tokenResponseKey, + // Don't throw if the key is not found + false, + ) + try { if (!cachedTokenResponse) { throw new UnauthorizedException() } @@ -56,27 +72,24 @@ export class UserService { ) if (accessTokenHasExpired && refresh) { - // Get new token data with refresh token - const tokenResponse = await this.idsService.refreshToken( - cachedTokenResponse.encryptedRefreshToken, - ) - - if (tokenResponse.type === 'error') { - throw tokenResponse.data - } - - // Update cache with new token data - const value: CachedTokenResponse = - await this.authService.updateTokenCache(tokenResponse.data) + const updatedTokenResponse = + await this.tokenRefreshService.refreshToken({ + sid, + encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, + }) - return this.mapToBffUser(value) + return this.mapToBffUser(updatedTokenResponse) } return this.mapToBffUser(cachedTokenResponse) } catch (error) { - this.logger.error('Get user error: ', error) - - throw new UnauthorizedException() + return this.errorService.handleAuthorizedError({ + error, + res, + sid, + tokenResponseKey, + operation: `${UserService.name}.getUser`, + }) } } } diff --git a/apps/services/bff/src/app/services/error.service.ts b/apps/services/bff/src/app/services/error.service.ts new file mode 100644 index 000000000000..3d20c50bd894 --- /dev/null +++ b/apps/services/bff/src/app/services/error.service.ts @@ -0,0 +1,100 @@ +import type { Logger } from '@island.is/logging' +import { LOGGER_PROVIDER } from '@island.is/logging' +import { + Inject, + Injectable, + InternalServerErrorException, + UnauthorizedException, +} from '@nestjs/common' +import type { Response } from 'express' +import { SESSION_COOKIE_NAME } from '../constants/cookies' +import { CacheService } from '../modules/cache/cache.service' +import { getCookieOptions } from '../utils/get-cookie-options' + +/** + * Standard OAuth2 error codes returned by Identity Server + * @see https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 + */ +export type OAuth2ErrorCode = + | 'invalid_grant' // Refresh token expired, invalid, or revoked + | 'invalid_client' // Client authentication failed + | 'invalid_request' // Missing or invalid parameters + | 'unauthorized_client' // Client not allowed to use grant type + | 'unsupported_grant_type' // Grant type not supported + | 'invalid_scope' // Requested scope is invalid/unknown + +export const OAUTH2_ERROR_CODES: OAuth2ErrorCode[] = [ + 'invalid_grant', + 'invalid_client', + 'invalid_request', + 'unauthorized_client', + 'unsupported_grant_type', + 'invalid_scope', +] + +@Injectable() +export class ErrorService { + constructor( + @Inject(LOGGER_PROVIDER) + private logger: Logger, + + private readonly cacheService: CacheService, + ) {} + + /** + * Validates if the given string is a known OAuth2 error code + * @see https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 + */ + private isKnownOAuth2ErrorCode(code: string): code is OAuth2ErrorCode { + return OAUTH2_ERROR_CODES.includes(code as OAuth2ErrorCode) + } + + /** + * Handles authorization errors by cleaning up user session and logging the error + * + * @param params Object containing error handling parameters + * @param params.res - Express Response object for clearing cookies + * @param params.sid - Session ID of the user + * @param params.operation - Name of the operation that failed (e.g., 'get user', 'refresh token') + * @param params.error - The error that was caught + * @param params.tokenResponseKey - Redis key for the token that needs to be cleaned up + * + * @throws UnauthorizedException after cleanup is complete + */ + async handleAuthorizedError({ + res, + sid, + operation, + error, + tokenResponseKey, + }: { + res: Response + sid: string + operation: string + error: unknown + tokenResponseKey: string + }): Promise { + this.logger.error(`${operation} failed: `, error) + + // If the session cookie is missing or the error is an UnauthorizedException + if (!sid || error instanceof UnauthorizedException) { + throw new UnauthorizedException() + } + + const errorCode = (error as { body?: { error?: string } })?.body?.error + + // If the error is an OAuth2 error + // 1. Delete the cached token response + // 2. Clear the session cookie + // 3. Throw an UnauthorizedException + if (errorCode && this.isKnownOAuth2ErrorCode(errorCode)) { + res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) + + await this.cacheService.delete(tokenResponseKey) + + throw new UnauthorizedException() + } + + throw new InternalServerErrorException() + } +} diff --git a/apps/services/bff/src/app/utils/get-cookie-options.ts b/apps/services/bff/src/app/utils/get-cookie-options.ts new file mode 100644 index 000000000000..ab59c25646ca --- /dev/null +++ b/apps/services/bff/src/app/utils/get-cookie-options.ts @@ -0,0 +1,13 @@ +import { CookieOptions } from 'express' +import { environment } from '../../environment' + +export const getCookieOptions = (): CookieOptions => { + return { + httpOnly: true, + secure: true, + // The lax setting allows cookies to be sent on top-level navigations (such as redirects), + // while still providing some protection against CSRF attacks. + sameSite: 'lax', + path: environment.keyPath, + } +} From bc3c8c1078e67fc7e8648644da98e901128aecd1 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Thu, 14 Nov 2024 14:21:01 +0000 Subject: [PATCH 02/13] fixes after merge --- .../src/app/modules/proxy/proxy.service.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/apps/services/bff/src/app/modules/proxy/proxy.service.ts b/apps/services/bff/src/app/modules/proxy/proxy.service.ts index d9cc4eb2b144..77bfa4358e7a 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.service.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.service.ts @@ -2,6 +2,7 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' import { BadRequestException, + HttpStatus, Inject, Injectable, UnauthorizedException, @@ -19,6 +20,7 @@ import { } from '@island.is/shared/constants' import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { ErrorService } from '../../services/error.service' +import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' import { validateUri } from '../../utils/validate-uri' import { CachedTokenResponse } from '../auth/auth.types' import { TokenRefreshService } from '../auth/token-refresh.service' @@ -96,7 +98,7 @@ export class ProxyService { let cachedTokenResponse = await this.cacheService.get(tokenResponseKey) - if (cachedTokenResponse.accessTokenExp) { + if (hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp)) { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ sid, encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, @@ -151,23 +153,31 @@ export class ProxyService { }, }) + // Set the status code of the response res.status(response.status) response.headers.forEach((value, key) => { + // Only set headers that are not in the droppedResponseHeaders array if (!droppedResponseHeaders.includes(key.toLowerCase())) { res.setHeader(key, value) } }) + // Pipe the response body directly to the client response.body.pipe(res) response.body.on('error', (err) => { this.logger.error('Proxy stream error:', err) + + // This check ensures that `res.end()` is only called if the response has not already been ended. if (!res.writableEnded) { + // Ensure the response is properly ended if an error occurs res.end() } }) + // Make sure to end the response when the stream ends, + // so that the client knows the request is complete. response.body.on('end', () => { if (!res.writableEnded) { res.end() @@ -175,7 +185,10 @@ export class ProxyService { }) } catch (error) { this.logger.error('Error during proxy request processing: ', error) - res.status(error.status || 500).send('Failed to proxy request') + + res + .status(error.status || HttpStatus.INTERNAL_SERVER_ERROR) + .send('Failed to proxy request') } } @@ -196,7 +209,7 @@ export class ProxyService { queryString ? `?${queryString}` : '' }` - await this.executeStreamRequest({ + this.executeStreamRequest({ accessToken, targetUrl, req, From ce95573cf83d737d8e4c423a76b8198898e0580f Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Thu, 14 Nov 2024 14:58:36 +0000 Subject: [PATCH 03/13] Fixes after self review --- .../bff/src/app/modules/auth/auth.service.ts | 2 +- .../bff/src/app/modules/user/user.service.ts | 13 ++++--------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/apps/services/bff/src/app/modules/auth/auth.service.ts b/apps/services/bff/src/app/modules/auth/auth.service.ts index 72e81cd08296..a8a17b80ba1c 100644 --- a/apps/services/bff/src/app/modules/auth/auth.service.ts +++ b/apps/services/bff/src/app/modules/auth/auth.service.ts @@ -9,7 +9,7 @@ import { UnauthorizedException, } from '@nestjs/common' import { ConfigType } from '@nestjs/config' -import { Request, Response } from 'express' +import type { Request, Response } from 'express' import jwksClient from 'jwks-rsa' import { jwtDecode } from 'jwt-decode' diff --git a/apps/services/bff/src/app/modules/user/user.service.ts b/apps/services/bff/src/app/modules/user/user.service.ts index 9ecacd2eaa31..8d824f4c14a4 100644 --- a/apps/services/bff/src/app/modules/user/user.service.ts +++ b/apps/services/bff/src/app/modules/user/user.service.ts @@ -3,7 +3,6 @@ import type { Request, Response } from 'express' import { BffUser } from '@island.is/shared/types' import { SESSION_COOKIE_NAME } from '../../constants/cookies' -import { CryptoService } from '../../services/crypto.service' import { ErrorService } from '../../services/error.service' import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' @@ -29,13 +28,6 @@ export class UserService { } } - /** - * Creates a key for storing token response data in cache - */ - private createTokenResponseKey(sid: string): string { - return this.cacheService.createSessionKeyType('current', sid) - } - /** * Gets the current user data, refreshing the token if needed */ @@ -54,7 +46,10 @@ export class UserService { throw new UnauthorizedException() } - const tokenResponseKey = this.createTokenResponseKey(sid) + const tokenResponseKey = this.cacheService.createSessionKeyType( + 'current', + sid, + ) const cachedTokenResponse = await this.cacheService.get( tokenResponseKey, From 08aa8a2496f0504962b9420753890575bf35d0ee Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Fri, 15 Nov 2024 09:19:47 +0000 Subject: [PATCH 04/13] Fix imports --- .../services/bff/src/app/modules/auth/token-refresh.service.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts index 5ca2d08a5b98..19b23af1fab4 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts @@ -1,4 +1,5 @@ -import { LOGGER_PROVIDER, Logger } from '@island.is/logging' +import type { Logger } from '@island.is/logging' +import { LOGGER_PROVIDER } from '@island.is/logging' import { Inject, Injectable } from '@nestjs/common' import { CacheService } from '../cache/cache.service' import { IdsService } from '../ids/ids.service' From 37c023d319b2fb97b98b4012343b91208c7b4ea7 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Fri, 15 Nov 2024 19:29:42 +0000 Subject: [PATCH 05/13] Add tests for token refresh service --- .../auth/token-refresh.service.spec.ts | 257 ++++++++++++++++++ 1 file changed, 257 insertions(+) create mode 100644 apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts new file mode 100644 index 000000000000..d8cd2934c411 --- /dev/null +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts @@ -0,0 +1,257 @@ +import { LOGGER_PROVIDER } from '@island.is/logging' +import { Test } from '@nestjs/testing' +import { CacheService } from '../cache/cache.service' +import { IdsService } from '../ids/ids.service' +import { TokenResponse } from '../ids/ids.types' +import { AuthService } from './auth.service' +import { CachedTokenResponse } from './auth.types' +import { TokenRefreshService } from './token-refresh.service' + +const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) + +jest.mock('uuid', () => ({ + v4: jest.fn().mockReturnValue('fake_uuid'), +})) + +const mockLogger = { + error: jest.fn(), +} + +const mockCacheStore = new Map() + +const mockTokenResponse: CachedTokenResponse = { + id_token: 'mock.id.token', + expires_in: 3600, + token_type: 'Bearer', + scope: 'openid profile offline_access', + scopes: ['openid', 'profile', 'offline_access'], + userProfile: { + sid: 'test-session-id', + nationalId: '1234567890', + name: 'Test User', + idp: 'test-idp', + subjectType: 'person', + delegationType: [], + locale: 'is', + birthdate: '1990-01-01', + }, + accessTokenExp: Date.now() + 3600000, // Current time + 1 hour in milliseconds + encryptedAccessToken: 'encrypted.access.token', + encryptedRefreshToken: 'encrypted.refresh.token', +} + +// When mocking IdsService.refreshToken response, we need TokenResponse type: +const mockIdsTokenResponse: TokenResponse = { + id_token: 'mock.id.token', + access_token: 'mock.access.token', + refresh_token: 'mock.refresh.token', + expires_in: 3600, + token_type: 'Bearer', + scope: 'openid profile offline_access', +} + +describe('TokenRefreshService', () => { + let service: TokenRefreshService + let authService: AuthService + let idsService: IdsService + let cacheService: CacheService + const testSid = 'test-sid' + const testRefreshToken = 'test-refresh-token' + const refreshInProgressPrefix = 'refresh_token_in_progress' + const refreshInProgressKey = `${refreshInProgressPrefix}:${testSid}` + + beforeEach(async () => { + const module = await Test.createTestingModule({ + providers: [ + TokenRefreshService, + { + provide: LOGGER_PROVIDER, + useValue: mockLogger, + }, + { + provide: AuthService, + useValue: { + updateTokenCache: jest.fn().mockResolvedValue(mockTokenResponse), + }, + }, + { + provide: IdsService, + useValue: { + refreshToken: jest.fn().mockResolvedValue(mockTokenResponse), + }, + }, + { + provide: CacheService, + useValue: { + save: jest.fn().mockImplementation(async ({ key, value }) => { + mockCacheStore.set(key, value) + }), + get: jest + .fn() + .mockImplementation(async (key) => mockCacheStore.get(key)), + delete: jest + .fn() + .mockImplementation(async (key) => mockCacheStore.delete(key)), + createSessionKeyType: jest.fn((type, sid) => `${type}_${sid}`), + }, + }, + ], + }).compile() + + service = module.get(TokenRefreshService) + authService = module.get(AuthService) + idsService = module.get(IdsService) + cacheService = module.get(CacheService) + }) + + afterEach(() => { + mockCacheStore.clear() + jest.clearAllMocks() + }) + + describe('refreshToken', () => { + it('should successfully refresh token when no refresh is in progress', async () => { + // Act + const result = await service.refreshToken({ + sid: testSid, + encryptedRefreshToken: testRefreshToken, + }) + + // Assert + expect(idsService.refreshToken).toHaveBeenCalledWith(testRefreshToken) + expect(authService.updateTokenCache).toHaveBeenCalledWith( + mockTokenResponse, + ) + expect(result).toEqual(mockTokenResponse) + }) + + it('should wait for ongoing refresh and return cached result', async () => { + // Arrange + await cacheService.save({ + key: refreshInProgressKey, + value: true, + ttl: 3000, + }) + + // Simulate another service updating the token while we wait + setTimeout(async () => { + await cacheService.delete(refreshInProgressKey) + await cacheService.save({ + key: `current_${testSid}`, + value: mockTokenResponse, + ttl: 3600, + }) + }, 500) + + // Act + const result = await service.refreshToken({ + sid: testSid, + encryptedRefreshToken: testRefreshToken, + }) + + // Assert + expect(result).toEqual(mockTokenResponse) + expect(idsService.refreshToken).not.toHaveBeenCalled() + }) + + it('should retry refresh if polling times out', async () => { + // Arrange + await cacheService.save({ + key: refreshInProgressKey, + value: true, + ttl: 3000, + }) + + // Act + const result = await service.refreshToken({ + sid: testSid, + encryptedRefreshToken: testRefreshToken, + }) + + // Assert + expect(mockLogger.error).toHaveBeenCalled() + expect(idsService.refreshToken).toHaveBeenCalledWith(testRefreshToken) + expect(result).toEqual(mockTokenResponse) + }) + + it('should handle refresh token failure', async () => { + // Arrange + const error = new Error('Refresh token failed') + jest.spyOn(idsService, 'refreshToken').mockRejectedValueOnce(error) + + // Act & Assert + await expect( + service.refreshToken({ + sid: testSid, + encryptedRefreshToken: testRefreshToken, + }), + ).rejects.toThrow(error) + + expect(mockLogger.error).toHaveBeenCalledWith( + `Token refresh failed for sid: ${testSid}`, + ) + }) + + it('should prevent concurrent refresh token requests', async () => { + // Arrange + const refreshPromises = [] + const refreshCount = 5 + let firstRequestStarted = false + + // Mock cache.get to make sure first request get in progress lock and other requests waits + jest.spyOn(cacheService, 'get').mockImplementation(async (key) => { + if (key.includes(refreshInProgressPrefix)) { + return firstRequestStarted + } + return mockTokenResponse + }) + + // Mock cache.save to track first request + jest.spyOn(cacheService, 'save').mockImplementation(async ({ key }) => { + if (key.includes(refreshInProgressPrefix)) { + firstRequestStarted = true + // Add delay after setting lock + await delay(50) + } + }) + + // Mock cache.delete to clear the lock + jest.spyOn(cacheService, 'delete').mockImplementation(async (key) => { + if (key.includes(refreshInProgressPrefix)) { + firstRequestStarted = false + } + }) + + // Act + // First request + refreshPromises.push( + service.refreshToken({ + sid: testSid, + encryptedRefreshToken: testRefreshToken, + }), + ) + + // Wait a tick to ensure first request starts + await delay(10) + + // Remaining requests + for (let i = 1; i < refreshCount; i++) { + refreshPromises.push( + service.refreshToken({ + sid: testSid, + encryptedRefreshToken: testRefreshToken, + }), + ) + } + + // Wait for all promises to resolve + const results = await Promise.all(refreshPromises) + + // Assert + expect(idsService.refreshToken).toHaveBeenCalledTimes(1) + results.forEach((result) => { + expect(result).toEqual(mockTokenResponse) + }) + }) + }) +}) From ad2ffa7a1c6975843c765ae7567f8a1bd1a10a83 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Mon, 18 Nov 2024 09:24:55 +0000 Subject: [PATCH 06/13] Fix tests --- .../app/modules/auth/auth.controller.spec.ts | 20 +++++----------- .../app/modules/auth/token-refresh.service.ts | 11 ++++++++- .../modules/proxy/proxy.controller.spec.ts | 24 ++++--------------- .../src/app/modules/proxy/proxy.service.ts | 22 ++++++++++------- 4 files changed, 34 insertions(+), 43 deletions(-) diff --git a/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts b/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts index ac2b1d951a9d..de1eadb25cd0 100644 --- a/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts +++ b/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts @@ -5,11 +5,11 @@ import jwt from 'jsonwebtoken' import request from 'supertest' import { setupTestServer } from '../../../../test/setupTestServer' import { - mockedTokensResponse as tokensResponse, - SID_VALUE, - SESSION_COOKIE_NAME, ALGORITM_TYPE, + SESSION_COOKIE_NAME, + SID_VALUE, getLoginSearchParmsFn, + mockedTokensResponse as tokensResponse, } from '../../../../test/sharedConstants' import { BffConfig } from '../../bff.config' import { IdsService } from '../ids/ids.service' @@ -58,17 +58,9 @@ const parResponse: ParResponse = { const allowedTargetLinkUri = 'http://test-client.com/testclient' const mockIdsService = { - getPar: jest.fn().mockResolvedValue({ - type: 'success', - data: parResponse, - }), - getTokens: jest.fn().mockResolvedValue({ - type: 'success', - data: tokensResponse, - }), - revokeToken: jest.fn().mockResolvedValue({ - type: 'success', - }), + getPar: jest.fn().mockResolvedValue(parResponse), + getTokens: jest.fn().mockResolvedValue(tokensResponse), + revokeToken: jest.fn().mockResolvedValue(undefined), getLoginSearchParams: jest.fn().mockImplementation(getLoginSearchParmsFn), } diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts index 19b23af1fab4..58c01fda7324 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts @@ -171,7 +171,16 @@ export class TokenRefreshService { // Get the updated token response from cache const tokenResponse = - await this.cacheService.get(tokenResponseKey) + await this.cacheService.get( + tokenResponseKey, + false, + ) + + if (!tokenResponse) { + throw new Error( + `Refresh token polling completed but token data not found for sid: ${sid}`, + ) + } return tokenResponse } catch (error) { diff --git a/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts b/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts index 1e97fcbe39c8..03a0c732b13a 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts @@ -39,17 +39,9 @@ const EXPIRED_TOKEN_RESPONSE: TokenResponse = { } const mockIdsService = { - refreshToken: jest.fn().mockResolvedValue({ - type: 'success', - data: tokensResponse, - }), - getTokens: jest.fn().mockResolvedValue({ - type: 'success', - data: tokensResponse, - }), - revokeToken: jest.fn().mockResolvedValue({ - type: 'success', - }), + refreshToken: jest.fn().mockResolvedValue(tokensResponse), + getTokens: jest.fn().mockResolvedValue(tokensResponse), + revokeToken: jest.fn().mockResolvedValue(undefined), getLoginSearchParams: jest.fn().mockImplementation(getLoginSearchParmsFn), } @@ -160,10 +152,7 @@ describe('ProxyController', () => { describe('GET /api/graphql', () => { beforeEach(() => { jest.clearAllMocks() - mockIdsService.getTokens.mockResolvedValue({ - type: 'success', - data: tokensResponse, - }) + mockIdsService.getTokens.mockResolvedValue(tokensResponse) }) it('should throw 401 unauthorized when not logged in', async () => { @@ -222,10 +211,7 @@ describe('ProxyController', () => { it('should call refreshToken and cache token response when access_token is expired', async () => { // Arrange - mockIdsService.getTokens.mockResolvedValue({ - type: 'success', - data: EXPIRED_TOKEN_RESPONSE, - }) + mockIdsService.getTokens.mockResolvedValue(EXPIRED_TOKEN_RESPONSE) // Act await server.get('/login') diff --git a/apps/services/bff/src/app/modules/proxy/proxy.service.ts b/apps/services/bff/src/app/modules/proxy/proxy.service.ts index 77bfa4358e7a..083b7228c110 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.service.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.service.ts @@ -1,11 +1,11 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' import { - BadRequestException, - HttpStatus, - Inject, - Injectable, - UnauthorizedException, + BadRequestException, + HttpStatus, + Inject, + Injectable, + UnauthorizedException, } from '@nestjs/common' import { ConfigType } from '@nestjs/config' import AgentKeepAlive, { HttpsAgent } from 'agentkeepalive' @@ -15,8 +15,8 @@ import { BffConfig } from '../../bff.config' import { CryptoService } from '../../services/crypto.service' import { - AGENT_DEFAULT_FREE_SOCKET_TIMEOUT, - AGENT_DEFAULT_MAX_SOCKETS, + AGENT_DEFAULT_FREE_SOCKET_TIMEOUT, + AGENT_DEFAULT_MAX_SOCKETS, } from '@island.is/shared/constants' import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { ErrorService } from '../../services/error.service' @@ -96,7 +96,11 @@ export class ProxyService { try { let cachedTokenResponse = - await this.cacheService.get(tokenResponseKey) + await this.cacheService.get(tokenResponseKey, false) + + if(!cachedTokenResponse){ + throw new UnauthorizedException() + } if (hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp)) { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ @@ -122,7 +126,7 @@ export class ProxyService { /** * This method proxies the request to the target URL and streams the response back to the client. */ - private async executeStreamRequest({ + public async executeStreamRequest({ targetUrl, accessToken, req, From d6782fd25ea746c56f1e30c76fbffde50455dbe4 Mon Sep 17 00:00:00 2001 From: andes-it Date: Mon, 18 Nov 2024 09:43:00 +0000 Subject: [PATCH 07/13] chore: nx format:write update dirty files --- .../src/app/modules/proxy/proxy.service.ts | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/apps/services/bff/src/app/modules/proxy/proxy.service.ts b/apps/services/bff/src/app/modules/proxy/proxy.service.ts index 083b7228c110..3cb393213b09 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.service.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.service.ts @@ -1,11 +1,11 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' import { - BadRequestException, - HttpStatus, - Inject, - Injectable, - UnauthorizedException, + BadRequestException, + HttpStatus, + Inject, + Injectable, + UnauthorizedException, } from '@nestjs/common' import { ConfigType } from '@nestjs/config' import AgentKeepAlive, { HttpsAgent } from 'agentkeepalive' @@ -15,8 +15,8 @@ import { BffConfig } from '../../bff.config' import { CryptoService } from '../../services/crypto.service' import { - AGENT_DEFAULT_FREE_SOCKET_TIMEOUT, - AGENT_DEFAULT_MAX_SOCKETS, + AGENT_DEFAULT_FREE_SOCKET_TIMEOUT, + AGENT_DEFAULT_MAX_SOCKETS, } from '@island.is/shared/constants' import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { ErrorService } from '../../services/error.service' @@ -96,9 +96,12 @@ export class ProxyService { try { let cachedTokenResponse = - await this.cacheService.get(tokenResponseKey, false) + await this.cacheService.get( + tokenResponseKey, + false, + ) - if(!cachedTokenResponse){ + if (!cachedTokenResponse) { throw new UnauthorizedException() } From 32ca2e45c57679327e1ee17e2cc222c7a865bef5 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Mon, 18 Nov 2024 10:41:27 +0000 Subject: [PATCH 08/13] Add tests --- .../app/modules/auth/auth.controller.spec.ts | 3 +- .../modules/proxy/proxy.controller.spec.ts | 81 +++++++ .../app/modules/user/user.controller.spec.ts | 220 ++++++++++++++++++ 3 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 apps/services/bff/src/app/modules/user/user.controller.spec.ts diff --git a/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts b/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts index de1eadb25cd0..7ccdeda46922 100644 --- a/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts +++ b/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts @@ -11,6 +11,7 @@ import { getLoginSearchParmsFn, mockedTokensResponse as tokensResponse, } from '../../../../test/sharedConstants' +import { environment } from '../../../environment' import { BffConfig } from '../../bff.config' import { IdsService } from '../ids/ids.service' import { ParResponse } from '../ids/ids.types' @@ -81,7 +82,7 @@ describe('AuthController', () => { }) mockConfig = app.get>(BffConfig.KEY) - baseUrlWithKey = `${mockConfig.clientBaseUrl}${process.env.BFF_CLIENT_KEY_PATH}` + baseUrlWithKey = `${mockConfig.clientBaseUrl}${environment.keyPath}` server = request(app.getHttpServer()) }) diff --git a/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts b/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts index 03a0c732b13a..c5d4ba8e89e3 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts @@ -228,5 +228,86 @@ describe('ProxyController', () => { expect(mockIdsService.refreshToken).toHaveBeenCalled() expect(res.status).toEqual(HttpStatus.OK) }) + + it('should handle polling timeout and retry', async () => { + // Arrange + const encryptedTokens = { + encryptedAccessToken: 'encrypted_access_token', + encryptedRefreshToken: 'encrypted_refresh_token', + accessTokenExp: Date.now(), + } + + mockIdsService.getTokens.mockResolvedValue(EXPIRED_TOKEN_RESPONSE) + mockIdsService.refreshToken.mockResolvedValue(tokensResponse) + + // Set up initial cache state + mockCacheStore.set(`current_${SID_VALUE}`, { + ...EXPIRED_TOKEN_RESPONSE, + ...encryptedTokens, + }) + mockCacheStore.set(`refresh_token_in_progress:${SID_VALUE}`, true) + + // Act + await server.get('/login') + await server + .get('/callbacks/login') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .query({ code: 'some_code', state: SID_VALUE }) + + const res = await server + .post('/api/graphql') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + expect(mockIdsService.refreshToken).toHaveBeenCalled() + expect(res.status).toEqual(HttpStatus.OK) + expect(res.body).toEqual({ message: 'success' }) + }) + + it('should handle polling and not call ids refresh token', async () => { + // Arrange + const encryptedTokens = { + encryptedAccessToken: 'encrypted_access_token', + encryptedRefreshToken: 'encrypted_refresh_token', + accessTokenExp: Date.now() + 3600000, // Valid for 1 hour + } + + mockIdsService.getTokens.mockResolvedValue({ + ...tokensResponse, + ...encryptedTokens, + }) + + // Set up initial cache state + mockCacheStore.set(`current_${SID_VALUE}`, { + ...tokensResponse, + ...encryptedTokens, + }) + mockCacheStore.set(`refresh_token_in_progress:${SID_VALUE}`, true) + + // Simulate another service completing the refresh + setTimeout(() => { + mockCacheStore.delete(`refresh_token_in_progress:${SID_VALUE}`) + mockCacheStore.set(`current_${SID_VALUE}`, { + ...tokensResponse, + ...encryptedTokens, + }) + }, 100) + + // Act + await server.get('/login') + await server + .get('/callbacks/login') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .query({ code: 'some_code', state: SID_VALUE }) + + const res = await server + .post('/api/graphql') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + expect(mockIdsService.refreshToken).not.toHaveBeenCalled() + expect(res.status).toEqual(HttpStatus.OK) + expect(res.body).toEqual({ message: 'success' }) + }) }) }) diff --git a/apps/services/bff/src/app/modules/user/user.controller.spec.ts b/apps/services/bff/src/app/modules/user/user.controller.spec.ts new file mode 100644 index 000000000000..a99becccb459 --- /dev/null +++ b/apps/services/bff/src/app/modules/user/user.controller.spec.ts @@ -0,0 +1,220 @@ +import { ConfigType } from '@island.is/nest/config' +import { CACHE_MANAGER } from '@nestjs/cache-manager' +import { HttpStatus, INestApplication } from '@nestjs/common' +import request from 'supertest' + +import { setupTestServer } from '../../../../test/setupTestServer' +import { + SESSION_COOKIE_NAME, + SID_VALUE, + getLoginSearchParmsFn, + mockedTokensResponse as tokensResponse, +} from '../../../../test/sharedConstants' +import { environment } from '../../../environment' +import { BffConfig } from '../../bff.config' +import { TokenRefreshService } from '../auth/token-refresh.service' +import { IdsService } from '../ids/ids.service' + +const mockCacheStore = new Map() + +const mockCacheManagerValue = { + set: jest.fn((key, value) => mockCacheStore.set(key, value)), + get: jest.fn((key) => mockCacheStore.get(key)), + del: jest.fn((key) => mockCacheStore.delete(key)), +} + +const generateTokenTimestamps = () => { + const now = Math.floor(Date.now() / 1000) + + return { + iat: now, + exp: now + 3600, + } +} + +const { iat, exp } = generateTokenTimestamps() + +const mockUserProfile = { + sub: '1234567890', + iss: 'https://example.com', + sid: SID_VALUE, + iat, + exp, +} + +const mockCachedTokenResponse = { + ...tokensResponse, + scopes: ['openid', 'profile', 'email'], + userProfile: mockUserProfile, + accessTokenExp: Date.now() + 3600000, + encryptedAccessToken: 'encrypted.access.token', + encryptedRefreshToken: 'encrypted.refresh.token', +} + +const mockTokenRefreshService = { + refreshToken: jest.fn().mockResolvedValue(mockCachedTokenResponse), +} + +const mockIdsService = { + getTokens: jest.fn().mockResolvedValue(mockCachedTokenResponse), + revokeToken: jest.fn().mockResolvedValue(undefined), + getLoginSearchParams: jest.fn().mockImplementation(getLoginSearchParmsFn), +} + +const createLoginAttempt = (mockConfig: ConfigType) => ({ + originUrl: `${mockConfig.clientBaseUrl}${environment.keyPath}`, + codeVerifier: 'test_code_verifier', + targetLinkUri: undefined, +}) + +describe('UserController', () => { + let app: INestApplication + let server: request.SuperTest + let mockConfig: ConfigType + + beforeAll(async () => { + const app = await setupTestServer({ + override: (builder) => + builder + .overrideProvider(CACHE_MANAGER) + .useValue(mockCacheManagerValue) + .overrideProvider(TokenRefreshService) + .useValue(mockTokenRefreshService) + .overrideProvider(IdsService) + .useValue(mockIdsService), + }) + + mockConfig = app.get>(BffConfig.KEY) + server = request(app.getHttpServer()) + }) + + afterEach(() => { + mockCacheStore.clear() + jest.clearAllMocks() + }) + + afterAll(async () => { + if (app) { + await app.close() + } + }) + + describe('GET /user', () => { + it('should throw unauthorized exception when sid cookie is not provided', async () => { + // Act + const res = await server.get('/user') + + // Assert + expect(res.status).toEqual(HttpStatus.UNAUTHORIZED) + }) + + it('should throw unauthorized exception when cache key is not found', async () => { + // Act + const res = await server + .get('/user') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + expect(res.status).toEqual(HttpStatus.UNAUTHORIZED) + }) + + it('should return user data when valid session exists', async () => { + // Arrange - Set up login attempt in cache + mockCacheStore.set(`attempt_${SID_VALUE}`, createLoginAttempt(mockConfig)) + + // Initialize session with login + await server.get('/login') + const callbackRes = await server + .get('/callbacks/login') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .query({ code: 'some_code', state: SID_VALUE }) + + expect(callbackRes.status).toBe(HttpStatus.FOUND) + + // Act - Get user data + const res = await server + .get('/user') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + expect(res.status).toEqual(HttpStatus.OK) + expect(res.body).toEqual({ + scopes: ['openid', 'profile', 'email'], + profile: expect.objectContaining({ + ...mockUserProfile, + iat: expect.any(Number), + exp: expect.any(Number), + }), + }) + }) + + it('should refresh token when access token is expired and refresh=true', async () => { + // Arrange - Set up login attempt in cache + mockCacheStore.set(`attempt_${SID_VALUE}`, createLoginAttempt(mockConfig)) + + // Initialize session + await server.get('/login') + await server + .get('/callbacks/login') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .query({ code: 'some_code', state: SID_VALUE }) + + // Set expired token in cache + const expiredTokenResponse = { + ...mockCachedTokenResponse, + accessTokenExp: Date.now() - 1000, // Expired token + } + mockCacheStore.set(`current_${SID_VALUE}`, expiredTokenResponse) + + // Act + const res = await server + .get('/user') + .query({ refresh: 'true' }) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + expect(mockTokenRefreshService.refreshToken).toHaveBeenCalledWith({ + sid: SID_VALUE, + encryptedRefreshToken: expiredTokenResponse.encryptedRefreshToken, + }) + expect(res.status).toEqual(HttpStatus.OK) + expect(res.body).toEqual({ + scopes: mockCachedTokenResponse.scopes, + profile: mockCachedTokenResponse.userProfile, + }) + }) + + it('should not refresh token when access token is expired but refresh=false', async () => { + // Arrange - Set up login attempt in cache + mockCacheStore.set(`attempt_${SID_VALUE}`, createLoginAttempt(mockConfig)) + + // Initialize session + await server.get('/login') + await server + .get('/callbacks/login') + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .query({ code: 'some_code', state: SID_VALUE }) + + // Set expired token in cache + const expiredTokenResponse = { + ...mockCachedTokenResponse, + accessTokenExp: Date.now() - 1000, // Expired token + } + mockCacheStore.set(`current_${SID_VALUE}`, expiredTokenResponse) + + // Act + const res = await server + .get('/user') + .query({ refresh: 'false' }) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + expect(mockTokenRefreshService.refreshToken).not.toHaveBeenCalled() + expect(res.status).toEqual(HttpStatus.OK) + expect(res.body).toEqual({ + scopes: expiredTokenResponse.scopes, + profile: expiredTokenResponse.userProfile, + }) + }) + }) +}) From 6eeab1bb0427d0272767d45926d1df70c73a1da6 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Mon, 18 Nov 2024 10:53:36 +0000 Subject: [PATCH 09/13] Prevent flaky test and always clean up redis cache --- .../app/modules/auth/token-refresh.service.ts | 36 ++++++++++--------- .../app/modules/user/user.controller.spec.ts | 6 ++-- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts index 58c01fda7324..4a2d8e511ef4 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts @@ -67,26 +67,28 @@ export class TokenRefreshService { refreshTokenKey: string encryptedRefreshToken: string }) { - // Set refresh in progress - await this.cacheService.save({ - key: refreshTokenKey, - value: true, - ttl: TokenRefreshService.MAX_POLL_TIME, - }) - - const tokenResponse = await this.idsService.refreshToken( - encryptedRefreshToken, - ) + try { + // Set refresh in progress + await this.cacheService.save({ + key: refreshTokenKey, + value: true, + ttl: TokenRefreshService.MAX_POLL_TIME, + }) - // Update cache with new token data - const updatedTokenResponse = await this.authService.updateTokenCache( - tokenResponse, - ) + const tokenResponse = await this.idsService.refreshToken( + encryptedRefreshToken, + ) - // Delete the refresh token key to signal that the refresh operation is complete - await this.cacheService.delete(refreshTokenKey) + // Update cache with new token data + const updatedTokenResponse = await this.authService.updateTokenCache( + tokenResponse, + ) - return updatedTokenResponse + return updatedTokenResponse + } finally { + // Always clean up the refresh token key + await this.cacheService.delete(refreshTokenKey) + } } /** diff --git a/apps/services/bff/src/app/modules/user/user.controller.spec.ts b/apps/services/bff/src/app/modules/user/user.controller.spec.ts index a99becccb459..4a2ca620d94b 100644 --- a/apps/services/bff/src/app/modules/user/user.controller.spec.ts +++ b/apps/services/bff/src/app/modules/user/user.controller.spec.ts @@ -23,8 +23,8 @@ const mockCacheManagerValue = { del: jest.fn((key) => mockCacheStore.delete(key)), } -const generateTokenTimestamps = () => { - const now = Math.floor(Date.now() / 1000) +const generateTokenTimestamps = (baseTime = 1700000000) => { + const now = baseTime return { iat: now, @@ -73,7 +73,7 @@ describe('UserController', () => { let mockConfig: ConfigType beforeAll(async () => { - const app = await setupTestServer({ + app = await setupTestServer({ override: (builder) => builder .overrideProvider(CACHE_MANAGER) From 835e96f2d4d31ac15ef4c0b0fa4c4aea2360979f Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Mon, 25 Nov 2024 19:56:52 +0000 Subject: [PATCH 10/13] Update refresh token flow with logging in mind --- .../bff/src/app/modules/auth/auth.service.ts | 4 + .../auth/token-refresh.service.spec.ts | 19 +- .../app/modules/auth/token-refresh.service.ts | 210 +++++++++--------- .../src/app/modules/proxy/proxy.service.ts | 16 +- .../bff/src/app/modules/user/user.service.ts | 36 ++- .../bff/src/app/services/error.service.ts | 7 +- 6 files changed, 139 insertions(+), 153 deletions(-) diff --git a/apps/services/bff/src/app/modules/auth/auth.service.ts b/apps/services/bff/src/app/modules/auth/auth.service.ts index a8a17b80ba1c..b3472b86f688 100644 --- a/apps/services/bff/src/app/modules/auth/auth.service.ts +++ b/apps/services/bff/src/app/modules/auth/auth.service.ts @@ -292,6 +292,10 @@ export class AuthService { this.logger.warn(err) }) + // Clear any existing session cookie first + // This prevents multiple session cookies being set. + res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) + // Create session cookie with successful login session id res.cookie( SESSION_COOKIE_NAME, diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts index d8cd2934c411..8b1dc46546e5 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts @@ -15,6 +15,7 @@ jest.mock('uuid', () => ({ const mockLogger = { error: jest.fn(), + warn: jest.fn(), } const mockCacheStore = new Map() @@ -169,7 +170,7 @@ describe('TokenRefreshService', () => { }) // Assert - expect(mockLogger.error).toHaveBeenCalled() + expect(mockLogger.warn).toHaveBeenCalled() expect(idsService.refreshToken).toHaveBeenCalledWith(testRefreshToken) expect(result).toEqual(mockTokenResponse) }) @@ -179,15 +180,15 @@ describe('TokenRefreshService', () => { const error = new Error('Refresh token failed') jest.spyOn(idsService, 'refreshToken').mockRejectedValueOnce(error) - // Act & Assert - await expect( - service.refreshToken({ - sid: testSid, - encryptedRefreshToken: testRefreshToken, - }), - ).rejects.toThrow(error) + // Act + const cachedTokenResponse = await service.refreshToken({ + sid: testSid, + encryptedRefreshToken: testRefreshToken, + }) + // + expect(cachedTokenResponse).toBe(null) - expect(mockLogger.error).toHaveBeenCalledWith( + expect(mockLogger.warn).toHaveBeenCalledWith( `Token refresh failed for sid: ${testSid}`, ) }) diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts index 4a2d8e511ef4..868601a2dfe8 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts @@ -1,6 +1,7 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' import { Inject, Injectable } from '@nestjs/common' +import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' import { CacheService } from '../cache/cache.service' import { IdsService } from '../ids/ids.service' import { AuthService } from './auth.service' @@ -12,24 +13,24 @@ import { CachedTokenResponse } from './auth.types' */ @Injectable() export class TokenRefreshService { - private static POLL_INTERVAL = 100 // ms + private static POLL_INTERVAL = 200 // ms private static MAX_POLL_TIME = 3000 // 3 seconds constructor( @Inject(LOGGER_PROVIDER) private logger: Logger, - private readonly authService: AuthService, private readonly cacheService: CacheService, private readonly idsService: IdsService, ) {} + private delay(ms: number) { + return new Promise((resolve) => setTimeout(resolve, ms)) + } + /** * Creates a unique key for tracking refresh token operations in progress * This key is used to prevent concurrent refresh token requests for the same session - * - * @param sid - Session ID - * @returns Formatted key string for refresh token tracking */ private createRefreshTokenKey(sid: string): string { return `refresh_token_in_progress:${sid}` @@ -38,9 +39,6 @@ export class TokenRefreshService { /** * Creates a key for storing token response data in cache * This key is used to store and retrieve the current token data for a session - * - * @param sid - Session ID - * @returns Formatted key string for token response data */ private createTokenResponseKey(sid: string): string { return this.cacheService.createSessionKeyType('current', sid) @@ -48,17 +46,7 @@ export class TokenRefreshService { /** * Executes the token refresh operation and updates the cache - * This method: - * 1. Sets a flag in cache to indicate refresh is in progress - * 2. Requests new tokens from the identity server - * 3. Updates the cache with the new token data - * 4. Cleans up the refresh flag - * - * @param params.refreshTokenKey - Redis key for tracking refresh status - * @param params.encryptedRefreshToken - Encrypted refresh token for getting new tokens - * - * @returns Promise Updated token data - * @throws Will throw if token refresh fails or cache operations fail + * Sets a flag during refresh, gets new tokens, and updates cache */ private async executeTokenRefresh({ refreshTokenKey, @@ -66,7 +54,9 @@ export class TokenRefreshService { }: { refreshTokenKey: string encryptedRefreshToken: string - }) { + }): Promise { + let tokenResponse: CachedTokenResponse | null = null + try { // Set refresh in progress await this.cacheService.save({ @@ -75,62 +65,80 @@ export class TokenRefreshService { ttl: TokenRefreshService.MAX_POLL_TIME, }) - const tokenResponse = await this.idsService.refreshToken( + const newTokens = await this.idsService.refreshToken( encryptedRefreshToken, ) + tokenResponse = await this.authService.updateTokenCache(newTokens) + } catch (error) { + this.logger.warn('Failed to refresh tokens: ', error) + } finally { + await this.cacheService.delete(refreshTokenKey) + } + + return tokenResponse + } - // Update cache with new token data - const updatedTokenResponse = await this.authService.updateTokenCache( - tokenResponse, + /** + * Waits for an ongoing refresh operation to complete + * Uses polling with a maximum wait time + */ + private async waitForRefreshCompletion(sid: string): Promise { + let attempts = 0 + // Calculate how many attempts we should make + // maxAttempts = 3000 / 200 = ~15 ish attempts + const maxAttempts = + TokenRefreshService.MAX_POLL_TIME / TokenRefreshService.POLL_INTERVAL + + while (attempts < maxAttempts) { + const refreshTokenInProgress = await this.cacheService.get( + this.createRefreshTokenKey(sid), + false, ) - return updatedTokenResponse - } finally { - // Always clean up the refresh token key - await this.cacheService.delete(refreshTokenKey) + // If refresh is no longer in progress, we're done + if (!refreshTokenInProgress) { + return true + } + + // Wait for for POLL_INTERVAL before next attempt + await this.delay(TokenRefreshService.POLL_INTERVAL) + + attempts++ } + + // We've made all attempts (~15 attempts in 3 seconds total) and still no success + this.logger.warn( + `Polling timed out for token refresh completion for session ${sid}`, + ) + + return false } /** - * Polls the cache to check if a refresh token operation has completed - * This prevents multiple concurrent refresh token requests and ensures - * all requests wait for the ongoing refresh to complete - * - * @param sid - Session ID - * - * @returns Promise that resolves when refresh is complete or rejects on timeout - * @throws Rejects if polling times out or encounters an error + * Retrieves and validates token from cache + * Checks for existence and expiration */ - private async pollForRefreshCompletion(sid: string): Promise { - return new Promise((resolve, reject) => { - const timeoutId = setTimeout(() => { - clearInterval(pollInterval) - reject( - new Error( - `Polling timed out for token refresh completion for session ${sid}`, - ), - ) - }, TokenRefreshService.MAX_POLL_TIME) - - const pollInterval = setInterval(async () => { - try { - const refreshTokenInProgress = await this.cacheService.get( - this.createRefreshTokenKey(sid), - false, - ) - - if (!refreshTokenInProgress) { - clearInterval(pollInterval) - clearTimeout(timeoutId) - resolve() - } - } catch (error) { - clearInterval(pollInterval) - clearTimeout(timeoutId) - reject(new Error(`Error polling for refresh completion: ${error}`)) - } - }, TokenRefreshService.POLL_INTERVAL) - }) + private async getTokenFromCache( + tokenResponseKey: string, + ): Promise { + const tokenResponse = await this.cacheService.get( + tokenResponseKey, + false, + ) + + if (!tokenResponse) { + this.logger.warn('No token response found in cache') + + return null + } + + if (hasTimestampExpiredInMS(tokenResponse.accessTokenExp)) { + this.logger.warn('Cached token has expired') + + return null + } + + return tokenResponse } /** @@ -149,65 +157,47 @@ export class TokenRefreshService { * @returns Promise resolving to updated token response data * @throws Forwards any errors from the refresh process after logging */ + public async refreshToken({ sid, encryptedRefreshToken, }: { sid: string encryptedRefreshToken: string - }): Promise { + }): Promise { const refreshTokenKey = this.createRefreshTokenKey(sid) const tokenResponseKey = this.createTokenResponseKey(sid) - try { - // Check if refresh is already in progress - const refreshTokenInProgress = await this.cacheService.get( - refreshTokenKey, - false, - ) + // Check if refresh is already in progress + const refreshTokenInProgress = await this.cacheService.get( + refreshTokenKey, + false, + ) + + if (refreshTokenInProgress) { + const refreshCompleted = await this.waitForRefreshCompletion(sid) + + if (refreshCompleted) { + const cachedToken = await this.getTokenFromCache(tokenResponseKey) - if (refreshTokenInProgress) { - try { - // Wait for the ongoing refresh to complete - await this.pollForRefreshCompletion(sid) - - // Get the updated token response from cache - const tokenResponse = - await this.cacheService.get( - tokenResponseKey, - false, - ) - - if (!tokenResponse) { - throw new Error( - `Refresh token polling completed but token data not found for sid: ${sid}`, - ) - } - - return tokenResponse - } catch (error) { - this.logger.error(error) - - // If polling times out, then retry the refresh - const updatedTokenResponse = await this.executeTokenRefresh({ - refreshTokenKey, - encryptedRefreshToken, - }) - - return updatedTokenResponse + if (cachedToken) { + return cachedToken } } - const updatedTokenResponse = await this.executeTokenRefresh({ - refreshTokenKey, - encryptedRefreshToken, - }) + // If waiting failed or no valid token found, proceed with new refresh + this.logger.warn('Retrying token refresh after failed wait') + } - return updatedTokenResponse - } catch (error) { - this.logger.error(`Token refresh failed for sid: ${sid}`) + const updatedTokenResponse = await this.executeTokenRefresh({ + refreshTokenKey, + encryptedRefreshToken, + }) - throw error + if (!updatedTokenResponse) { + this.logger.warn(`Token refresh failed for sid: ${sid}`) } + + return updatedTokenResponse } } diff --git a/apps/services/bff/src/app/modules/proxy/proxy.service.ts b/apps/services/bff/src/app/modules/proxy/proxy.service.ts index 3cb393213b09..0f7244ca7de6 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.service.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.service.ts @@ -95,23 +95,26 @@ export class ProxyService { ) try { - let cachedTokenResponse = + let cachedTokenResponse: CachedTokenResponse | null = await this.cacheService.get( tokenResponseKey, false, ) - if (!cachedTokenResponse) { - throw new UnauthorizedException() - } - - if (hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp)) { + if ( + cachedTokenResponse && + hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp) + ) { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ sid, encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, }) } + if (!cachedTokenResponse) { + throw new UnauthorizedException() + } + return this.cryptoService.decrypt( cachedTokenResponse.encryptedAccessToken, ) @@ -119,7 +122,6 @@ export class ProxyService { return this.errorService.handleAuthorizedError({ error, res, - sid, tokenResponseKey, operation: `${ProxyService.name}.getAccessToken`, }) diff --git a/apps/services/bff/src/app/modules/user/user.service.ts b/apps/services/bff/src/app/modules/user/user.service.ts index 8d824f4c14a4..6c59df441650 100644 --- a/apps/services/bff/src/app/modules/user/user.service.ts +++ b/apps/services/bff/src/app/modules/user/user.service.ts @@ -5,7 +5,6 @@ import { BffUser } from '@island.is/shared/types' import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { ErrorService } from '../../services/error.service' -import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' import { CachedTokenResponse } from '../auth/auth.types' import { TokenRefreshService } from '../auth/token-refresh.service' import { CacheService } from '../cache/cache.service' @@ -50,30 +49,24 @@ export class UserService { 'current', sid, ) - const cachedTokenResponse = - await this.cacheService.get( - tokenResponseKey, - // Don't throw if the key is not found - false, - ) try { - if (!cachedTokenResponse) { - throw new UnauthorizedException() - } + let cachedTokenResponse: CachedTokenResponse | null = + await this.cacheService.get( + tokenResponseKey, + // Don't throw if the key is not found + false, + ) - const accessTokenHasExpired = hasTimestampExpiredInMS( - cachedTokenResponse.accessTokenExp, - ) - - if (accessTokenHasExpired && refresh) { - const updatedTokenResponse = - await this.tokenRefreshService.refreshToken({ - sid, - encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, - }) + if (cachedTokenResponse) { + cachedTokenResponse = await this.tokenRefreshService.refreshToken({ + sid, + encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, + }) + } - return this.mapToBffUser(updatedTokenResponse) + if (!cachedTokenResponse) { + throw new UnauthorizedException() } return this.mapToBffUser(cachedTokenResponse) @@ -81,7 +74,6 @@ export class UserService { return this.errorService.handleAuthorizedError({ error, res, - sid, tokenResponseKey, operation: `${UserService.name}.getUser`, }) diff --git a/apps/services/bff/src/app/services/error.service.ts b/apps/services/bff/src/app/services/error.service.ts index 3d20c50bd894..d2d29ec3b585 100644 --- a/apps/services/bff/src/app/services/error.service.ts +++ b/apps/services/bff/src/app/services/error.service.ts @@ -63,21 +63,18 @@ export class ErrorService { */ async handleAuthorizedError({ res, - sid, operation, error, tokenResponseKey, }: { res: Response - sid: string operation: string error: unknown tokenResponseKey: string }): Promise { - this.logger.error(`${operation} failed: `, error) + this.logger.warn(`${operation} failed: `, error) - // If the session cookie is missing or the error is an UnauthorizedException - if (!sid || error instanceof UnauthorizedException) { + if (error instanceof UnauthorizedException) { throw new UnauthorizedException() } From 001bf0e57380af12559238e6190cf92808fd8fbc Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Mon, 25 Nov 2024 20:03:50 +0000 Subject: [PATCH 11/13] Simplify error service logic --- .../bff/src/app/services/error.service.ts | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/apps/services/bff/src/app/services/error.service.ts b/apps/services/bff/src/app/services/error.service.ts index d2d29ec3b585..8e4ad8efd551 100644 --- a/apps/services/bff/src/app/services/error.service.ts +++ b/apps/services/bff/src/app/services/error.service.ts @@ -1,11 +1,6 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' -import { - Inject, - Injectable, - InternalServerErrorException, - UnauthorizedException, -} from '@nestjs/common' +import { Inject, Injectable, UnauthorizedException } from '@nestjs/common' import type { Response } from 'express' import { SESSION_COOKIE_NAME } from '../constants/cookies' import { CacheService } from '../modules/cache/cache.service' @@ -72,12 +67,6 @@ export class ErrorService { error: unknown tokenResponseKey: string }): Promise { - this.logger.warn(`${operation} failed: `, error) - - if (error instanceof UnauthorizedException) { - throw new UnauthorizedException() - } - const errorCode = (error as { body?: { error?: string } })?.body?.error // If the error is an OAuth2 error @@ -85,13 +74,17 @@ export class ErrorService { // 2. Clear the session cookie // 3. Throw an UnauthorizedException if (errorCode && this.isKnownOAuth2ErrorCode(errorCode)) { - res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) + this.logger.warn( + `${operation} failed with OAuth2 error: ${errorCode}`, + error, + ) + res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) await this.cacheService.delete(tokenResponseKey) throw new UnauthorizedException() } - throw new InternalServerErrorException() + throw error } } From 8dda727f710d4ba47e4d415d2b6c3bbb0d3d07ae Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Tue, 26 Nov 2024 08:42:42 +0000 Subject: [PATCH 12/13] Fix after accidental remove of refresh --- apps/services/bff/src/app/modules/user/user.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/services/bff/src/app/modules/user/user.service.ts b/apps/services/bff/src/app/modules/user/user.service.ts index 6c59df441650..4babb4df7a4e 100644 --- a/apps/services/bff/src/app/modules/user/user.service.ts +++ b/apps/services/bff/src/app/modules/user/user.service.ts @@ -58,7 +58,7 @@ export class UserService { false, ) - if (cachedTokenResponse) { + if (cachedTokenResponse && refresh) { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ sid, encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, From 2cb31ac6994aff071a4bf717f259c005bce3cd21 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Tue, 26 Nov 2024 08:53:34 +0000 Subject: [PATCH 13/13] Add comments --- .../app/modules/auth/token-refresh.service.ts | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts index 868601a2dfe8..0af2bac09fb5 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts @@ -31,6 +31,9 @@ export class TokenRefreshService { /** * Creates a unique key for tracking refresh token operations in progress * This key is used to prevent concurrent refresh token requests for the same session + * + * @param sid - Session ID + * @returns Formatted key string for refresh token tracking */ private createRefreshTokenKey(sid: string): string { return `refresh_token_in_progress:${sid}` @@ -39,6 +42,9 @@ export class TokenRefreshService { /** * Creates a key for storing token response data in cache * This key is used to store and retrieve the current token data for a session + * + * @param sid - Session ID + * @returns Formatted key string for token response data */ private createTokenResponseKey(sid: string): string { return this.cacheService.createSessionKeyType('current', sid) @@ -46,7 +52,17 @@ export class TokenRefreshService { /** * Executes the token refresh operation and updates the cache - * Sets a flag during refresh, gets new tokens, and updates cache + * This method: + * 1. Sets a flag in cache to indicate refresh is in progress + * 2. Requests new tokens from the identity server + * 3. Updates the cache with the new token data + * 4. Cleans up the refresh flag + * + * @param params.refreshTokenKey - Redis key for tracking refresh status + * @param params.encryptedRefreshToken - Encrypted refresh token for getting new tokens + * + * @returns Promise Updated token data + * @throws Will throw if token refresh fails or cache operations fail */ private async executeTokenRefresh({ refreshTokenKey, @@ -81,6 +97,8 @@ export class TokenRefreshService { /** * Waits for an ongoing refresh operation to complete * Uses polling with a maximum wait time + * + * @param sid Session ID */ private async waitForRefreshCompletion(sid: string): Promise { let attempts = 0 @@ -117,6 +135,8 @@ export class TokenRefreshService { /** * Retrieves and validates token from cache * Checks for existence and expiration + * + * @param tokenResponseKey - Key in cache service for token response data */ private async getTokenFromCache( tokenResponseKey: string,