From 8219e9f07b95845bbd4365b09fbaeaf9fa81d79f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sn=C3=A6r=20Seljan=20=C3=9E=C3=B3roddsson?=
 <snaerth@gmail.com>
Date: Mon, 16 Dec 2024 12:40:15 +0000
Subject: [PATCH] fix(services-bff): Add missing exp check on the user endpoint
 (#17247)

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

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

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
---
 .../app/modules/user/user.controller.spec.ts  | 112 ++++++++++++++++++
 .../bff/src/app/modules/user/user.service.ts  |   7 +-
 2 files changed, 118 insertions(+), 1 deletion(-)

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