Skip to content

Commit

Permalink
fix(services-bff): Add missing exp check on the user endpoint (#17253)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
snaerth and kodiakhq[bot] authored Dec 16, 2024
1 parent f567a35 commit 421ab90
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 1 deletion.
112 changes: 112 additions & 0 deletions apps/services/bff/src/app/modules/user/user.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
})
})
})
7 changes: 6 additions & 1 deletion apps/services/bff/src/app/modules/user/user.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 421ab90

Please sign in to comment.