From f703e4cb300651a720d133cb8dc6f0cae43cc363 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Mon, 16 Dec 2024 11:18:48 +0000 Subject: [PATCH 1/2] fix(user): enhance token refresh logic with expiration check Add a utility function to check if the access token has expired before attempting to refresh it. This change improves the efficiency of the token refresh process by ensuring that refresh operations are only triggered when necessary. --- apps/services/bff/src/app/modules/user/user.service.ts | 7 ++++++- 1 file changed, 6 insertions(+), 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 4babb4df7a4e..74f576a5d8aa 100644 --- a/apps/services/bff/src/app/modules/user/user.service.ts +++ b/apps/services/bff/src/app/modules/user/user.service.ts @@ -5,6 +5,7 @@ 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' @@ -58,7 +59,11 @@ export class UserService { false, ) - if (cachedTokenResponse && refresh) { + if ( + cachedTokenResponse && + hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp) && + refresh + ) { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ sid, encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, From ca67dd99d060908a9f198d95854ae60e8ba69e51 Mon Sep 17 00:00:00 2001 From: snaerseljan Date: Mon, 16 Dec 2024 11:28:05 +0000 Subject: [PATCH 2/2] test: enhance user token refresh logic in tests Add tests to verify token refresh behavior based on token expiration and refresh query parameter. Ensure that the refreshToken service is called only when the token exists, is expired, and refresh is set to true. This improves test coverage and ensures correct handling of token refresh scenarios. --- .../app/modules/user/user.controller.spec.ts | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) 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 dba1a6918e0a..a6dfc9d72ef6 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 @@ -230,5 +230,117 @@ describe('UserController', () => { profile: expiredTokenResponse.userProfile, }) }) + + it('should not refresh token when token exists but is not expired', async () => { + // Arrange - Set up login attempt in cache + mockCacheStore.set( + `attempt::${mockConfig.name}::${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 valid (not expired) token in cache + const validTokenResponse = { + ...mockCachedTokenResponse, + accessTokenExp: Date.now() + 1000, // Future expiration + } + mockCacheStore.set( + `current::${mockConfig.name}::${SID_VALUE}`, + validTokenResponse, + ) + + // Act + const res = await server + .get('/user') + .query({ refresh: 'true' }) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + expect(mockTokenRefreshService.refreshToken).not.toHaveBeenCalled() + expect(res.status).toEqual(HttpStatus.OK) + expect(res.body).toEqual({ + scopes: validTokenResponse.scopes, + profile: validTokenResponse.userProfile, + }) + }) + + it('should refresh token only when all conditions are met (token exists, is expired, and refresh=true)', async () => { + // Arrange - Set up login attempt in cache + mockCacheStore.set( + `attempt::${mockConfig.name}::${SID_VALUE}`, + createLoginAttempt(mockConfig), + ) + + const testCases = [ + { + exists: true, + expired: true, + refresh: true, + shouldCallRefresh: true, + }, + { + exists: true, + expired: true, + refresh: false, + shouldCallRefresh: false, + }, + { + exists: true, + expired: false, + refresh: true, + shouldCallRefresh: false, + }, + { + exists: false, + expired: true, + refresh: true, + shouldCallRefresh: false, + }, + ] + + for (const testCase of testCases) { + // Reset mocks + jest.clearAllMocks() + mockCacheStore.clear() + + if (testCase.exists) { + const tokenResponse = { + ...mockCachedTokenResponse, + accessTokenExp: testCase.expired + ? Date.now() - 1000 // Expired + : Date.now() + 1000, // Not expired + } + mockCacheStore.set( + `current::${mockConfig.name}::${SID_VALUE}`, + tokenResponse, + ) + } + + // Act + const res = await server + .get('/user') + .query({ refresh: testCase.refresh.toString() }) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + + // Assert + if (testCase.shouldCallRefresh) { + expect(mockTokenRefreshService.refreshToken).toHaveBeenCalled() + } else { + expect(mockTokenRefreshService.refreshToken).not.toHaveBeenCalled() + } + + if (testCase.exists) { + expect(res.status).toEqual(HttpStatus.OK) + } else { + expect(res.status).toEqual(HttpStatus.UNAUTHORIZED) + } + } + }) }) })