Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(service-bff): Integrate encryption for session cookies #17277

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 42 additions & 12 deletions apps/services/bff/src/app/modules/auth/auth.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ConfigType } from '@island.is/nest/config'
import { LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION } from '@island.is/shared/constants'
import { CACHE_MANAGER } from '@nestjs/cache-manager'
import { HttpStatus, INestApplication } from '@nestjs/common'
import type { Request, Response } from 'express'
import jwt from 'jsonwebtoken'
import request from 'supertest'
import { setupTestServer } from '../../../../test/setupTestServer'
Expand All @@ -13,6 +14,7 @@ import {
mockedTokensResponse as tokensResponse,
} from '../../../../test/sharedConstants'
import { BffConfig } from '../../bff.config'
import { SessionCookieService } from '../../services/sessionCookie.service'
import { IdsService } from '../ids/ids.service'
import { ParResponse } from '../ids/ids.types'

Expand Down Expand Up @@ -70,6 +72,19 @@ describe('AuthController', () => {
let server: request.SuperTest<request.Test>
let mockConfig: ConfigType<typeof BffConfig>
let baseUrlWithKey: string
let sessionCookieService: SessionCookieService
let hashedSid: string

// Mock Response object
const mockResponse: Partial<Response> = {
cookie: jest.fn(),
clearCookie: jest.fn(),
}

// Mock Request object
const mockRequest: Partial<Request> = {
cookies: {},
}

beforeAll(async () => {
const app = await setupTestServer({
Expand All @@ -81,9 +96,25 @@ describe('AuthController', () => {
.useValue(mockIdsService),
})

sessionCookieService = app.get<SessionCookieService>(SessionCookieService)
mockConfig = app.get<ConfigType<typeof BffConfig>>(BffConfig.KEY)

baseUrlWithKey = `${mockConfig.clientBaseUrl}${mockConfig.clientBasePath}`

// Set the hashed SID
sessionCookieService.set({
res: mockResponse as Response,
value: SID_VALUE,
})

// Capture the hashed value from the mock cookie call
hashedSid = (mockResponse.cookie as jest.Mock).mock.calls[0][1]

// Set up the mock request cookies for subsequent get() calls
mockRequest.cookies = {
[SESSION_COOKIE_NAME]: hashedSid,
}

server = request(app.getHttpServer())
})

Expand Down Expand Up @@ -311,7 +342,7 @@ describe('AuthController', () => {
const loginAttempt = setCacheSpy.mock.calls[0]

// Assert - First request should cache the login attempt
expect(setCacheSpy.mock.calls[0]).toContain(
expect(loginAttempt[0]).toContain(
`attempt::${mockConfig.name}::${SID_VALUE}`,
)
expect(loginAttempt[1]).toMatchObject({
Expand All @@ -322,16 +353,15 @@ describe('AuthController', () => {
// Then make a callback to the login endpoint
const res = await server
.get('/callbacks/login')
.set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`])
.set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`])
.query({ code, state: SID_VALUE })

const currentLogin = setCacheSpy.mock.calls[1]

// Assert
expect(setCacheSpy).toHaveBeenCalled()

expect(currentLogin[0]).toContain(
`current::${mockConfig.name}::${SID_VALUE}`,
expect(currentLogin[0]).toBe(
`current::${mockConfig.name}::${hashedSid}`,
)
// Check if the cache contains the correct values for the current login
expect(currentLogin[1]).toMatchObject(tokensResponse)
Expand All @@ -357,7 +387,7 @@ describe('AuthController', () => {

await server
.get('/callbacks/login')
.set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`])
.set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`])
.query({ code: 'some_code', state: SID_VALUE })
const res = await server.get('/logout')

Expand Down Expand Up @@ -410,13 +440,13 @@ describe('AuthController', () => {

await server
.get('/callbacks/login')
.set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`])
.set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`])
.query({ code: 'some_code', state: SID_VALUE })

const res = await server
.get('/logout')
.query({ sid: SID_VALUE })
.set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`])
.set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`])

// Assert
expect(revokeRefreshTokenSpy).toHaveBeenCalled()
Expand Down Expand Up @@ -445,7 +475,7 @@ describe('AuthController', () => {

it('should throw 400 if logout_token is missing from body', async () => {
// Act
const res = await await server.post('/callbacks/logout')
const res = await server.post('/callbacks/logout')

// Assert
expect(res.status).toEqual(HttpStatus.BAD_REQUEST)
Expand Down Expand Up @@ -514,7 +544,7 @@ describe('AuthController', () => {

await server
.get('/callbacks/login')
.set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`])
.set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`])
.query({ code: 'some_code', state: SID_VALUE })

const res = await server
Expand Down Expand Up @@ -545,7 +575,7 @@ describe('AuthController', () => {
// First login - create initial session with attempt data
await server.get('/login')

const currentKey = `current::${mockConfig.name}::${SID_VALUE}`
const currentKey = `current::${mockConfig.name}::${hashedSid}`

mockCacheManagerValue.set(currentKey, tokensResponse)

Expand All @@ -563,7 +593,7 @@ describe('AuthController', () => {
// Act - Try to complete second login
const res = await server
.get('/callbacks/login')
.set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`])
.set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`])
.query({ code, state: SID_VALUE })

// Assert
Expand Down
15 changes: 12 additions & 3 deletions apps/services/bff/src/app/modules/auth/auth.module.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
import { Module } from '@nestjs/common'
import { CryptoService } from '../../services/crypto.service'
import { CryptoKeyService } from '../../services/cryptoKey.service'
import { PKCEService } from '../../services/pkce.service'
import { SessionCookieService } from '../../services/sessionCookie.service'
import { IdsService } from '../ids/ids.service'
import { AuthController } from './auth.controller'
import { AuthService } from './auth.service'
import { PKCEService } from '../../services/pkce.service'
import { CryptoService } from '../../services/crypto.service'

@Module({
controllers: [AuthController],
providers: [AuthService, PKCEService, IdsService, CryptoService],
providers: [
AuthService,
PKCEService,
IdsService,
CryptoService,
SessionCookieService,
CryptoKeyService,
],
exports: [AuthService],
})
export class AuthModule {}
52 changes: 31 additions & 21 deletions apps/services/bff/src/app/modules/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,14 @@ import { IdTokenClaims } from '@island.is/shared/types'
import { Algorithm, decode, verify } from 'jsonwebtoken'
import { v4 as uuid } from 'uuid'
import { BffConfig } from '../../bff.config'
import { SESSION_COOKIE_NAME } from '../../constants/cookies'
import { FIVE_SECONDS_IN_MS } from '../../constants/time'
import { CryptoService } from '../../services/crypto.service'
import { PKCEService } from '../../services/pkce.service'
import { SessionCookieService } from '../../services/sessionCookie.service'
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'
Expand All @@ -52,6 +51,7 @@ export class AuthService {
private readonly cacheService: CacheService,
private readonly idsService: IdsService,
private readonly cryptoService: CryptoService,
private readonly sessionCookieService: SessionCookieService,
) {
this.baseUrl = this.config.ids.issuer
}
Expand Down Expand Up @@ -125,7 +125,10 @@ export class AuthService {

// Save the tokenResponse to the cache
await this.cacheService.save({
key: this.cacheService.createSessionKeyType('current', userProfile.sid),
key: this.cacheService.createSessionKeyType(
'current',
this.sessionCookieService.hash(userProfile.sid),
),
value,
ttl: this.config.cacheUserProfileTTLms,
})
Expand Down Expand Up @@ -263,7 +266,7 @@ export class AuthService {
res: Response
loginAttemptCacheKey: string
}) {
const sid = req.cookies[SESSION_COOKIE_NAME]
const sid = this.sessionCookieService.get(req)

// Check if older session exists
if (sid) {
Expand Down Expand Up @@ -356,25 +359,28 @@ export class AuthService {

// Clear any existing session cookie first
// This prevents multiple session cookies being set.
res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions())
this.sessionCookieService.clear(res)

// Create session cookie with successful login session id
res.cookie(
SESSION_COOKIE_NAME,
updatedTokenResponse.userProfile.sid,
getCookieOptions(),
)
this.sessionCookieService.set({
res,
value: updatedTokenResponse.userProfile.sid,
})

// Check if there is an old session cookie and clean up the cache
const oldSessionCookie = req.cookies[SESSION_COOKIE_NAME]
// Check if there is an old session cookie
const oldHashedSessionCookie = this.sessionCookieService.get(req)

if (
oldSessionCookie &&
oldSessionCookie !== updatedTokenResponse.userProfile.sid
oldHashedSessionCookie &&
// Check if the old session cookie is different from the new one
!this.sessionCookieService.verify(
req,
updatedTokenResponse.userProfile.sid,
)
) {
const oldSessionCacheKey = this.cacheService.createSessionKeyType(
'current',
oldSessionCookie,
oldHashedSessionCookie,
)

const oldSessionData = await this.cacheService.get<CachedTokenResponse>(
Expand Down Expand Up @@ -422,17 +428,21 @@ export class AuthService {
res: Response
query: LogoutDto
}) {
const sidCookie = req.cookies[SESSION_COOKIE_NAME]
const sidCookie = this.sessionCookieService.get(req)

if (!sidCookie) {
this.logger.error('Logout failed: No session cookie found')

return res.redirect(this.config.logoutRedirectUri)
}

if (sidCookie !== query.sid) {
const hashedQuerySid = this.sessionCookieService.hash(query.sid)

if (sidCookie !== hashedQuerySid) {
this.logger.error(
`Logout failed: Cookie sid "${sidCookie}" does not match the session id in query param "${query.sid}"`,
`Logout failed: Cookie "${this.cacheService.createKeyError(
sidCookie,
)}" does not match the session id in query param "sid"`,
)

return this.redirectWithError(res, {
Expand All @@ -443,7 +453,7 @@ export class AuthService {

const currentLoginCacheKey = this.cacheService.createSessionKeyType(
'current',
query.sid,
hashedQuerySid,
)

const cachedTokenResponse =
Expand All @@ -470,7 +480,7 @@ export class AuthService {
* - Delete the current login from the cache
* - Clear the session cookie
*/
res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions())
this.sessionCookieService.clear(res)

this.cacheService
.delete(currentLoginCacheKey)
Expand Down Expand Up @@ -553,7 +563,7 @@ export class AuthService {
// Create cache key and retrieve cached token response
const cacheKey = this.cacheService.createSessionKeyType(
'current',
payload.sid,
this.sessionCookieService.hash(payload.sid),
)
const cachedTokenResponse =
await this.cacheService.get<CachedTokenResponse>(
Expand Down
Loading
Loading