diff --git a/.github/workflows/e2e-ui-tests.yml b/.github/workflows/e2e-ui-tests.yml index 6dc8130cc0..787f22cc3d 100644 --- a/.github/workflows/e2e-ui-tests.yml +++ b/.github/workflows/e2e-ui-tests.yml @@ -11,10 +11,20 @@ permissions: jobs: build: + name: E2E (${{ matrix.configuration }}) runs-on: ubuntu-24.04 + strategy: + fail-fast: false + matrix: + include: + - configuration: default-jit + registration_disabled: '' + - configuration: registration-policy-sso + registration_disabled: sso env: CYPRESS_TESTING: true NODE_ENV: test + REGISTRATION_DISABLED: ${{ matrix.registration_disabled }} services: postgres: image: postgres:latest @@ -64,17 +74,27 @@ jobs: yarn backend sequelize db:migrate yarn backend sequelize db:seed:all - - name: Cypress run - uses: cypress-io/github-action@v7 + - name: Cypress run (default JIT policy) + if: matrix.configuration == 'default-jit' + uses: cypress-io/github-action@fa4a118725a8f001170d49631ea89e5d66fee626 # v7 with: + config: excludeSpecPattern=test/integration/registration-policy.cy.ts start: yarn start, yarn run cypress-test mock-json, yarn run cypress-test mock-openid wait-on: 'http://127.0.0.1:3000, http://127.0.0.1:3001' + - name: Cypress run (SSO provisioning denied) + if: matrix.configuration == 'registration-policy-sso' + uses: cypress-io/github-action@fa4a118725a8f001170d49631ea89e5d66fee626 # v7 + with: + start: yarn start, yarn run cypress-test mock-json, yarn run cypress-test mock-openid + wait-on: 'http://127.0.0.1:3000, http://127.0.0.1:3001' + spec: test/integration/registration-policy.cy.ts + - name: Upload test screenshots and videos if: failure() uses: actions/upload-artifact@v7 with: - name: cypress-recording + name: cypress-recording-${{ matrix.configuration }} path: | test/screenshots test/videos diff --git a/CHANGELOG b/CHANGELOG index 9d04dbf031..454c13bc8e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,9 @@ +v2.13.2 + +## Breaking Changes + +- **BREAKING:** `REGISTRATION_DISABLED` is now a fail-fast, scoped enum. `true` disables local registration and SSO/LDAP just-in-time account creation; previously tolerated invalid values now prevent startup. Review the [v2.13.2 registration policy upgrade note](docs/upgrade-notes/registration-disabled-enum.md) before upgrading. + v2.13.1 ## What's New diff --git a/apps/backend/.env-example b/apps/backend/.env-example index 1a3d724fa2..6890ac1f3d 100644 --- a/apps/backend/.env-example +++ b/apps/backend/.env-example @@ -16,7 +16,7 @@ ADMIN_EMAIL= LOCAL_LOGIN_DISABLED= -REGISTRATION_DISABLED= +REGISTRATION_DISABLED= ONE_SESSION_PER_USER= JWT_SECRET= JWT_EXPIRE_TIME= @@ -90,3 +90,4 @@ OIDC_USER_INFO_URL= OIDC_CLIENT_SECRET= OIDC_EXTERNAL_GROUPS= +OIDC_USES_VERIFIED_EMAIL= diff --git a/apps/backend/src/authn/authn.service.spec.ts b/apps/backend/src/authn/authn.service.spec.ts new file mode 100644 index 0000000000..326040943c --- /dev/null +++ b/apps/backend/src/authn/authn.service.spec.ts @@ -0,0 +1,327 @@ +import { NotFoundException, UnauthorizedException } from '@nestjs/common'; +import { JwtService } from '@nestjs/jwt'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ApiKeyService } from '../apikeys/apikey.service'; +import { ConfigService } from '../config/config.service'; +import { UsersService } from '../users/users.service'; +import { AuthnService } from './authn.service'; + +const NOT_PROVISIONED_RESPONSE = { + error: 'account_not_provisioned', + message: + 'No Heimdall account exists for this SSO user. Please ask your system administrator to create the account.', + statusCode: 401, +}; + +const MISSING_EMAIL_RESPONSE = { + error: 'external_identity_missing_email', + message: 'External identity did not provide an email address.', + statusCode: 401, +}; + +const expectUnauthorizedResponse = async ( + validation: Promise, + response: { error: string; message: string; statusCode: number }, +) => { + try { + await validation; + } catch (error) { + expect(error).toBeInstanceOf(UnauthorizedException); + if (!(error instanceof UnauthorizedException)) { + throw error; + } + expect(error.getResponse()).toEqual(response); + return; + } + throw new Error('Expected validation to reject'); +}; + +const userFixture = (firstName = 'First', lastName = 'Last') => ({ + email: 'user@example.com', + firstName, + lastName, + save: vi.fn(), +}); + +describe('AuthnService', () => { + const apiKeyService = {}; + const configService = new ConfigService(); + const jwtService = {}; + const usersService = { + create: vi.fn(), + findByEmail: vi.fn(), + updateLoginMetadata: vi.fn(), + }; + let authnService: AuthnService; + + beforeEach(async () => { + vi.restoreAllMocks(); + vi.resetAllMocks(); + configService.set('REGISTRATION_DISABLED', undefined); + const module = await Test.createTestingModule({ + providers: [ + { provide: ApiKeyService, useValue: apiKeyService }, + { + inject: [ApiKeyService, ConfigService, UsersService, JwtService], + provide: AuthnService, + useFactory: ( + apiKeys: ApiKeyService, + config: ConfigService, + users: UsersService, + jwt: JwtService, + ) => new AuthnService(apiKeys, config, users, jwt), + }, + { provide: ConfigService, useValue: configService }, + { provide: JwtService, useValue: jwtService }, + { provide: UsersService, useValue: usersService }, + ], + }).compile(); + authnService = module.get(AuthnService); + vi.spyOn(authnService.logger, 'warn').mockImplementation( + () => authnService.logger, + ); + }); + + describe('validateOrCreateUser', () => { + it('formats audit events as raw single-line JSON', () => { + const message = '{"event":"external_auth.login.rejected_not_provisioned"}'; + const formatted = authnService.logger.format.transform({ + auditEvent: true, + level: 'warn', + message, + }); + if (typeof formatted === 'boolean') { + throw new TypeError('Expected the logger to format the audit event'); + } + + expect(formatted[Symbol.for('message')]).toBe(message); + }); + + it('logs exactly one five-field audit event when SSO provisioning is denied', async () => { + configService.set('REGISTRATION_DISABLED', 'sso'); + usersService.findByEmail.mockRejectedValue(new NotFoundException()); + const warn = vi.mocked(authnService.logger.warn); + + const validation = authnService.validateOrCreateUser( + ' User@Example.com ', + 'SensitiveFirstName', + 'SensitiveLastName', + 'oidc', + ); + + await expectUnauthorizedResponse(validation, NOT_PROVISIONED_RESPONSE); + expect(warn).toHaveBeenCalledExactlyOnceWith({ + auditEvent: true, + message: expect.any(String), + }); + + const logEntry = warn.mock.calls[0][0]; + if (!('message' in logEntry) || typeof logEntry.message !== 'string') { + throw new Error('Expected a serialized audit message'); + } + const serializedEvent = logEntry.message; + const auditEvent = JSON.parse(serializedEvent); + expect(serializedEvent).not.toContain('\n'); + expect(Object.keys(auditEvent)).toEqual([ + 'event', + 'provider', + 'email', + 'reason', + 'timestamp', + ]); + expect(auditEvent).toEqual({ + email: 'user@example.com', + event: 'external_auth.login.rejected_not_provisioned', + provider: 'oidc', + reason: 'registrationDisabledForSso', + timestamp: expect.any(String), + }); + expect(new Date(auditEvent.timestamp).toISOString()).toBe( + auditEvent.timestamp, + ); + expect(serializedEvent).not.toContain('SensitiveFirstName'); + expect(serializedEvent).not.toContain('SensitiveLastName'); + expect(serializedEvent).not.toContain('password'); + expect(serializedEvent).not.toContain('token'); + expect(usersService.create).not.toHaveBeenCalled(); + }); + + it.each([undefined, 'false', 'local'])( + 'creates a missing SSO user under REGISTRATION_DISABLED=%s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + const createdUser = userFixture(); + usersService.findByEmail + .mockRejectedValueOnce(new NotFoundException()) + .mockResolvedValueOnce(createdUser); + + const result = await authnService.validateOrCreateUser( + ' Jane.Doe@Agency.gov ', + 'Jane', + 'Doe', + 'oidc', + ); + + expect(result).toBe(createdUser); + expect(usersService.findByEmail).toHaveBeenNthCalledWith( + 1, + 'jane.doe@agency.gov', + ); + expect(usersService.findByEmail).toHaveBeenNthCalledWith( + 2, + 'jane.doe@agency.gov', + ); + const createUser = usersService.create.mock.calls[0][0]; + expect(usersService.create).toHaveBeenCalledExactlyOnceWith({ + creationMethod: 'oidc', + email: 'jane.doe@agency.gov', + firstName: 'Jane', + lastName: 'Doe', + organization: '', + password: createUser.password, + passwordConfirmation: createUser.password, + role: 'user', + title: '', + }); + expect(createUser.password).toBe(createUser.passwordConfirmation); + expect(createUser.password).toHaveLength(256); + expect(authnService.logger.warn).not.toHaveBeenCalled(); + }, + ); + + it.each(['true', 'sso'])( + 'rejects a missing SSO user under REGISTRATION_DISABLED=%s without creating a user', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + usersService.findByEmail.mockRejectedValue(new NotFoundException()); + + const validation = authnService.validateOrCreateUser( + ' User@Example.com ', + 'First', + 'Last', + 'oidc', + ); + + await expectUnauthorizedResponse( + validation, + NOT_PROVISIONED_RESPONSE, + ); + expect(usersService.findByEmail).toHaveBeenCalledExactlyOnceWith('user@example.com'); + expect(usersService.create).not.toHaveBeenCalled(); + expect(authnService.logger.warn).toHaveBeenCalledOnce(); + }, + ); + + it('propagates non-NotFoundException lookup failures unchanged', async () => { + const databaseError = new Error('database unavailable'); + usersService.findByEmail.mockRejectedValue(databaseError); + const registrationAllowed = vi.spyOn( + configService, + 'isRegistrationAllowed', + ); + + const validation = authnService.validateOrCreateUser( + 'user@example.com', + 'First', + 'Last', + 'oidc', + ); + + await expect(validation).rejects.toBe(databaseError); + expect(registrationAllowed).not.toHaveBeenCalled(); + expect(usersService.create).not.toHaveBeenCalled(); + expect(authnService.logger.warn).not.toHaveBeenCalled(); + }); + + it.each([undefined, '', ' '.repeat(3)])( + 'rejects missing external email %s before lookup', + async (email) => { + const validation = authnService.validateOrCreateUser( + email, + 'First', + 'Last', + 'oidc', + ); + + await expectUnauthorizedResponse(validation, MISSING_EMAIL_RESPONSE); + expect(usersService.findByEmail).not.toHaveBeenCalled(); + expect(usersService.create).not.toHaveBeenCalled(); + expect(authnService.logger.warn).not.toHaveBeenCalled(); + }, + ); + + it.each(['true', 'sso'])( + 'matches a normalized pre-provisioned user under REGISTRATION_DISABLED=%s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + const existingUser = userFixture('Jane', 'Doe'); + usersService.findByEmail.mockResolvedValue(existingUser); + + const result = await authnService.validateOrCreateUser( + ' Jane.Doe@Agency.gov ', + 'Jane', + 'Doe', + 'oidc', + ); + + expect(result).toBe(existingUser); + expect(usersService.findByEmail).toHaveBeenCalledWith( + 'jane.doe@agency.gov', + ); + expect(usersService.create).not.toHaveBeenCalled(); + expect(usersService.updateLoginMetadata).toHaveBeenCalledWith( + existingUser, + ); + }, + ); + + it.each([undefined, 'false', 'true', 'local', 'sso'])( + 'preserves existing-user profile updates under REGISTRATION_DISABLED=%s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + const existingUser = userFixture('Old', 'Name'); + usersService.findByEmail.mockResolvedValue(existingUser); + const registrationAllowed = vi.spyOn( + configService, + 'isRegistrationAllowed', + ); + + const result = await authnService.validateOrCreateUser( + 'USER@EXAMPLE.COM', + 'New', + 'Name', + 'oidc', + ); + + expect(result).toBe(existingUser); + expect(existingUser.firstName).toBe('New'); + expect(existingUser.lastName).toBe('Name'); + expect(existingUser.save).toHaveBeenCalledOnce(); + expect(usersService.updateLoginMetadata).toHaveBeenCalledWith( + existingUser, + ); + expect(registrationAllowed).not.toHaveBeenCalled(); + expect(usersService.create).not.toHaveBeenCalled(); + expect(authnService.logger.warn).not.toHaveBeenCalled(); + }, + ); + + it('does not save an unchanged existing profile', async () => { + const existingUser = userFixture(); + usersService.findByEmail.mockResolvedValue(existingUser); + + await authnService.validateOrCreateUser( + 'user@example.com', + 'First', + 'Last', + 'oidc', + ); + + expect(existingUser.save).not.toHaveBeenCalled(); + expect(usersService.updateLoginMetadata).toHaveBeenCalledWith( + existingUser, + ); + }); + }); +}); diff --git a/apps/backend/src/authn/authn.service.ts b/apps/backend/src/authn/authn.service.ts index db2e9f3117..31230dbf10 100644 --- a/apps/backend/src/authn/authn.service.ts +++ b/apps/backend/src/authn/authn.service.ts @@ -1,7 +1,9 @@ import { ForbiddenException, + HttpStatus, Injectable, - UnauthorizedException + NotFoundException, + UnauthorizedException, } from '@nestjs/common'; import {JwtService} from '@nestjs/jwt'; import {compare} from 'bcryptjs'; @@ -31,7 +33,9 @@ export class AuthnService { }), winston.format.printf( (info) => - `${this.line}[${[info.timestamp]}] (Authn Service): ${info.message}` + info.auditEvent === true + ? String(info.message) + : `${this.line}[${[info.timestamp]}] (Authn Service): ${info.message}` ) ) }); @@ -97,18 +101,51 @@ export class AuthnService { } async validateOrCreateUser( - email: string, + email: string | undefined, firstName: string, lastName: string, creationMethod: string ): Promise { + const normalizedEmail = email?.trim().toLowerCase(); + if (!normalizedEmail) { + throw new UnauthorizedException({ + error: 'external_identity_missing_email', + message: 'External identity did not provide an email address.', + statusCode: HttpStatus.UNAUTHORIZED, + }); + } + let user: User; try { - user = await this.usersService.findByEmail(email); - } catch { + user = await this.usersService.findByEmail(normalizedEmail); + } catch (error) { + if (!(error instanceof NotFoundException)) { + throw error; + } + + if (!this.configService.isRegistrationAllowed('sso')) { + const auditDetails = Object.fromEntries([ + ['event', 'external_auth.login.rejected_not_provisioned'], + ['provider', creationMethod], + ['email', normalizedEmail], + ['reason', 'registrationDisabledForSso'], + ['timestamp', new Date().toISOString()], + ]); + this.logger.warn({ + auditEvent: true, + message: JSON.stringify(auditDetails), + }); + throw new UnauthorizedException({ + error: 'account_not_provisioned', + message: + 'No Heimdall account exists for this SSO user. Please ask your system administrator to create the account.', + statusCode: HttpStatus.UNAUTHORIZED, + }); + } + const randomPass = crypto.randomBytes(128).toString('hex'); const createUser: CreateUserDto = { - email: email, + email: normalizedEmail, password: randomPass, passwordConfirmation: randomPass, firstName: firstName, @@ -119,7 +156,7 @@ export class AuthnService { creationMethod: creationMethod }; await this.usersService.create(createUser); - user = await this.usersService.findByEmail(email); + user = await this.usersService.findByEmail(normalizedEmail); } if (user) { diff --git a/apps/backend/src/authn/ldap.strategy.spec.ts b/apps/backend/src/authn/ldap.strategy.spec.ts new file mode 100644 index 0000000000..1cd5640053 --- /dev/null +++ b/apps/backend/src/authn/ldap.strategy.spec.ts @@ -0,0 +1,172 @@ +import { NotFoundException, UnauthorizedException } from '@nestjs/common'; +import { JwtService } from '@nestjs/jwt'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ApiKeyService } from '../apikeys/apikey.service'; +import { ConfigService } from '../config/config.service'; +import { UsersService } from '../users/users.service'; +import { AuthnService } from './authn.service'; +import { LDAPStrategy } from './ldap.strategy'; + +const NOT_PROVISIONED_RESPONSE = { + error: 'account_not_provisioned', + message: + 'No Heimdall account exists for this SSO user. Please ask your system administrator to create the account.', + statusCode: 401, +}; + +const MISSING_EMAIL_RESPONSE = { + error: 'external_identity_missing_email', + message: 'External identity did not provide an email address.', + statusCode: 401, +}; + +type LDAPVerify = (profile: unknown, done: VerifyDone) => Promise; +type VerifyDone = (error: unknown, user?: unknown) => void; + +const ldapVerify = (strategy: LDAPStrategy): LDAPVerify => + Reflect.get(strategy, 'verify') as LDAPVerify; + +const expectPassportError = ( + done: ReturnType, + response: { error: string; message: string; statusCode: number }, +) => { + expect(done).toHaveBeenCalledOnce(); + const [error, user] = done.mock.calls[0]; + expect(error).toBeInstanceOf(UnauthorizedException); + if (!(error instanceof UnauthorizedException)) { + throw error; + } + expect(error.getResponse()).toEqual(response); + expect(user).toBeNull(); +}; + +describe('LDAPStrategy', () => { + const apiKeyService = {}; + const jwtService = {}; + const usersService = { + create: vi.fn(), + findByEmail: vi.fn(), + updateLoginMetadata: vi.fn(), + }; + let configService: ConfigService; + let strategy: LDAPStrategy; + + beforeEach(async () => { + vi.restoreAllMocks(); + vi.resetAllMocks(); + configService = new ConfigService(); + configService.set('LDAP_NAMEFIELD', 'cn'); + configService.set('LDAP_MAILFIELD', 'mail'); + configService.set('LDAP_SSL', 'false'); + + const module = await Test.createTestingModule({ + providers: [ + { provide: ApiKeyService, useValue: apiKeyService }, + { provide: ConfigService, useValue: configService }, + { provide: JwtService, useValue: jwtService }, + { provide: UsersService, useValue: usersService }, + { + inject: [ApiKeyService, ConfigService, UsersService, JwtService], + provide: AuthnService, + useFactory: ( + apiKeys: ApiKeyService, + config: ConfigService, + users: UsersService, + jwt: JwtService, + ) => new AuthnService(apiKeys, config, users, jwt), + }, + { + inject: [AuthnService, ConfigService], + provide: LDAPStrategy, + useFactory: (authnService: AuthnService, config: ConfigService) => + new LDAPStrategy(authnService, config), + }, + ], + }).compile(); + + strategy = module.get(LDAPStrategy); + vi.spyOn(module.get(AuthnService).logger, 'warn').mockImplementation( + () => module.get(AuthnService).logger, + ); + }); + + it.each(['true', 'sso'])( + 'passes account_not_provisioned to done when a valid LDAP user has no Heimdall account under %s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + usersService.findByEmail.mockRejectedValue(new NotFoundException()); + const done = vi.fn(); + + await ldapVerify(strategy)( + { cn: 'Philip Fry', mail: 'fry@example.com' }, + done, + ); + + expectPassportError(done, NOT_PROVISIONED_RESPONSE); + expect(usersService.findByEmail).toHaveBeenCalledExactlyOnceWith( + 'fry@example.com', + ); + expect(usersService.create).not.toHaveBeenCalled(); + }, + ); + + it('awaits validation and passes the resolved user to done once', async () => { + const user = { + email: 'fry@example.com', + firstName: 'Philip', + lastName: 'Fry', + save: vi.fn(), + }; + usersService.findByEmail.mockResolvedValue(user); + const done = vi.fn(); + + await ldapVerify(strategy)( + { cn: 'Philip Fry', mail: 'fry@example.com' }, + done, + ); + + expect(done).toHaveBeenCalledExactlyOnceWith(null, user); + }); + + it('uses the first value from a multi-valued mail attribute', async () => { + const user = { + email: 'first@example.com', + firstName: 'Philip', + lastName: 'Fry', + save: vi.fn(), + }; + usersService.findByEmail.mockResolvedValue(user); + const done = vi.fn(); + + await ldapVerify(strategy)( + { + cn: 'Philip Fry', + mail: ['first@example.com', 'second@example.com'], + }, + done, + ); + + expect(usersService.findByEmail).toHaveBeenCalledExactlyOnceWith( + 'first@example.com', + ); + expect(done).toHaveBeenCalledExactlyOnceWith(null, user); + }); + + it.each([ + ['missing', undefined], + ['empty', ''], + ['empty multi-valued', []], + ])( + 'passes external_identity_missing_email to done for a %s mail attribute', + async (_description, mail) => { + const done = vi.fn(); + + await ldapVerify(strategy)({ cn: 'Philip Fry', mail }, done); + + expectPassportError(done, MISSING_EMAIL_RESPONSE); + expect(usersService.findByEmail).not.toHaveBeenCalled(); + expect(usersService.create).not.toHaveBeenCalled(); + }, + ); +}); diff --git a/apps/backend/src/authn/ldap.strategy.ts b/apps/backend/src/authn/ldap.strategy.ts index 67363cef08..e771a6ff12 100644 --- a/apps/backend/src/authn/ldap.strategy.ts +++ b/apps/backend/src/authn/ldap.strategy.ts @@ -4,6 +4,7 @@ import * as fs from 'fs'; import _ from 'lodash'; import Strategy from 'passport-ldapauth'; import {ConfigService} from '../config/config.service'; +import { User } from '../users/user.model'; import {AuthnService} from './authn.service'; @Injectable() @@ -67,21 +68,27 @@ export class LDAPStrategy extends PassportStrategy(Strategy, 'ldap') { }); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async validate(user: unknown, done: any) { - const {firstName, lastName} = this.authnService.splitName( - _.get(user, this.configService.get('LDAP_NAMEFIELD') || 'name') + async validate(user: unknown): Promise { + const { firstName, lastName } = this.authnService.splitName( + _.get(user, this.configService.get('LDAP_NAMEFIELD') || 'name'), ); - const email: string = _.get( + const mailAttribute: unknown = _.get( user, - this.configService.get('LDAP_MAILFIELD') || 'mail' + this.configService.get('LDAP_MAILFIELD') || 'mail', ); - const validatedUser = this.authnService.validateOrCreateUser( - Array.isArray(email) ? email[0] : email, + let email: string | undefined; + if (Array.isArray(mailAttribute)) { + const [firstMailValue] = mailAttribute; + email = typeof firstMailValue === 'string' ? firstMailValue : undefined; + } else { + email = typeof mailAttribute === 'string' ? mailAttribute : undefined; + } + const validatedUser = await this.authnService.validateOrCreateUser( + email, firstName, lastName, - 'ldap' + 'ldap', ); - return done(null, validatedUser); + return validatedUser; } } diff --git a/apps/backend/src/authn/oidc.strategy.spec.ts b/apps/backend/src/authn/oidc.strategy.spec.ts new file mode 100644 index 0000000000..6452361bf3 --- /dev/null +++ b/apps/backend/src/authn/oidc.strategy.spec.ts @@ -0,0 +1,180 @@ +import { NotFoundException, UnauthorizedException } from '@nestjs/common'; +import { JwtService } from '@nestjs/jwt'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ApiKeyService } from '../apikeys/apikey.service'; +import { ConfigService } from '../config/config.service'; +import { GroupsService } from '../groups/groups.service'; +import { UsersService } from '../users/users.service'; +import { AuthnService } from './authn.service'; +import { OidcStrategy } from './oidc.strategy'; + +const NOT_PROVISIONED_RESPONSE = { + error: 'account_not_provisioned', + message: + 'No Heimdall account exists for this SSO user. Please ask your system administrator to create the account.', + statusCode: 401, +}; + +type OIDCProfile = { + _json: { + email: string; + email_verified: boolean; + family_name: string; + given_name: string; + groups: string[]; + }; + _raw: string; + displayName: string; + emails: [{ value: string }]; + id: string; + name: { familyName: string; givenName: string }; +}; + +type OIDCVerify = ( + issuer: string, + uiProfile: OIDCProfile, + idProfile: object, + context: object, + idToken: string, + accessToken: string, + refreshToken: string, + parameters: object, + done: VerifyDone, +) => Promise; +type VerifyDone = (error: unknown, user?: unknown) => void; + +const oidcVerify = (strategy: OidcStrategy): OIDCVerify => + Reflect.get(strategy, '_verify') as OIDCVerify; + +const profile = (): OIDCProfile => ({ + _json: { + email: 'fry@example.com', + email_verified: true, + family_name: 'Fry', + given_name: 'Philip', + groups: [], + }, + _raw: '{}', + displayName: 'Philip Fry', + emails: [{ value: 'fry@example.com' }], + id: 'fry', + name: { familyName: 'Fry', givenName: 'Philip' }, +}); + +const verifyProfile = async ( + strategy: OidcStrategy, + done: VerifyDone, +): Promise => { + await oidcVerify(strategy)( + 'https://issuer.example', + profile(), + {}, + {}, + 'id-token', + 'access-token', + 'refresh-token', + {}, + done, + ); +}; + +describe('OidcStrategy', () => { + const apiKeyService = {}; + const groupsService = { syncUserGroups: vi.fn() }; + const jwtService = {}; + const usersService = { + create: vi.fn(), + findByEmail: vi.fn(), + updateLoginMetadata: vi.fn(), + }; + let configService: ConfigService; + let strategy: OidcStrategy; + + beforeEach(async () => { + vi.restoreAllMocks(); + vi.resetAllMocks(); + configService = new ConfigService(); + + const module = await Test.createTestingModule({ + providers: [ + { provide: ApiKeyService, useValue: apiKeyService }, + { provide: ConfigService, useValue: configService }, + { provide: GroupsService, useValue: groupsService }, + { provide: JwtService, useValue: jwtService }, + { provide: UsersService, useValue: usersService }, + { + inject: [ApiKeyService, ConfigService, UsersService, JwtService], + provide: AuthnService, + useFactory: ( + apiKeys: ApiKeyService, + config: ConfigService, + users: UsersService, + jwt: JwtService, + ) => new AuthnService(apiKeys, config, users, jwt), + }, + { + inject: [AuthnService, ConfigService, GroupsService], + provide: OidcStrategy, + useFactory: ( + authnService: AuthnService, + config: ConfigService, + groups: GroupsService, + ) => new OidcStrategy(authnService, config, groups), + }, + ], + }).compile(); + + strategy = module.get(OidcStrategy); + vi.spyOn(module.get(AuthnService).logger, 'warn').mockImplementation( + () => module.get(AuthnService).logger, + ); + vi.spyOn(strategy.logger, 'debug').mockImplementation( + () => strategy.logger, + ); + }); + + it.each(['true', 'sso'])( + 'passes account_not_provisioned to done when a valid OIDC user has no Heimdall account under %s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + usersService.findByEmail.mockRejectedValue(new NotFoundException()); + const done = vi.fn(); + + await verifyProfile(strategy, done); + + expect(done).toHaveBeenCalledOnce(); + const [error, user] = done.mock.calls[0]; + expect(error).toBeInstanceOf(UnauthorizedException); + if (!(error instanceof UnauthorizedException)) { + throw error; + } + expect(error.getResponse()).toEqual(NOT_PROVISIONED_RESPONSE); + expect(user).toBeNull(); + expect(usersService.findByEmail).toHaveBeenCalledExactlyOnceWith( + 'fry@example.com', + ); + expect(usersService.create).not.toHaveBeenCalled(); + }, + ); + + it('registers the nine-argument verifier needed to receive the raw OIDC profile', () => { + expect(oidcVerify(strategy)).toHaveLength(9); + }); + + it('passes a resolved existing user to done once', async () => { + const user = { + email: 'fry@example.com', + firstName: 'Philip', + lastName: 'Fry', + save: vi.fn(), + }; + usersService.findByEmail.mockResolvedValue(user); + const done = vi.fn(); + + await verifyProfile(strategy, done); + + expect(done).toHaveBeenCalledExactlyOnceWith(null, user); + expect(usersService.create).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/backend/src/authn/oidc.strategy.ts b/apps/backend/src/authn/oidc.strategy.ts index a56eaf1708..825b648e69 100644 --- a/apps/backend/src/authn/oidc.strategy.ts +++ b/apps/backend/src/authn/oidc.strategy.ts @@ -5,6 +5,7 @@ import {Strategy} from '@govtechsg/passport-openidconnect'; import winston from 'winston'; import {ConfigService} from '../config/config.service'; import {GroupsService} from '../groups/groups.service'; +import { User } from '../users/user.model'; import {AuthnService} from './authn.service'; interface OIDCProfile { @@ -23,8 +24,7 @@ interface OIDCProfile { } @Injectable() -//eslint-disable-next-line @typescript-eslint/no-explicit-any -- Passport v11 changed their types and many 3rd party strategies are not compatible with the types despite actually still working just fine -export class OidcStrategy extends PassportStrategy(Strategy as any, 'oidc') { +export class OidcStrategy extends PassportStrategy(Strategy, 'oidc', 9) { private readonly line = '_______________________________________________\n'; public loggingTimeFormat = 'MMM-DD-YYYY HH:mm:ss Z'; public logger = winston.createLogger({ @@ -46,70 +46,42 @@ export class OidcStrategy extends PassportStrategy(Strategy as any, 'oidc') { private readonly groupsService: GroupsService, private readonly httpsAgent?: Agent ) { - super( - { - issuer: configService.get('OIDC_ISSUER') || 'disabled', - authorizationURL: - configService.get('OIDC_AUTHORIZATION_URL') || 'disabled', - tokenURL: configService.get('OIDC_TOKEN_URL') || 'disabled', - userInfoURL: configService.get('OIDC_USER_INFO_URL') || 'disabled', - clientID: configService.get('OIDC_CLIENTID') || 'disabled', - clientSecret: configService.get('OIDC_CLIENT_SECRET') || 'disabled', - callbackURL: `${configService.getExternalUrl()}/authn/oidc_callback`, - pkce: - configService.get('OIDC_USES_PKCE_S256') === 'true' - ? 'S256' - : configService.get('OIDC_USES_PKCE_PLAIN') === 'true' - ? 'plain' - : undefined, - scope: ['openid', 'email', 'profile'], - skipUserProfile: false, - proxy: - configService.get('OIDC_USE_HTTPS_PROXY') === 'true' - ? true + super({ + issuer: configService.get('OIDC_ISSUER') || 'disabled', + authorizationURL: + configService.get('OIDC_AUTHORIZATION_URL') || 'disabled', + tokenURL: configService.get('OIDC_TOKEN_URL') || 'disabled', + userInfoURL: configService.get('OIDC_USER_INFO_URL') || 'disabled', + clientID: configService.get('OIDC_CLIENTID') || 'disabled', + clientSecret: configService.get('OIDC_CLIENT_SECRET') || 'disabled', + callbackURL: `${configService.getExternalUrl()}/authn/oidc_callback`, + pkce: + configService.get('OIDC_USES_PKCE_S256') === 'true' + ? 'S256' + : configService.get('OIDC_USES_PKCE_PLAIN') === 'true' + ? 'plain' : undefined, - agent: httpsAgent - }, - // using the 9-arity function so that we can access the underlying JSON response and extract the 'email_verified' attribute - async ( - _issuer: string, - uiProfile: OIDCProfile, - _idProfile: object, - _context: object, - _idToken: string, - _accessToken: string, - _refreshToken: string, - _params: object, - //eslint-disable-next-line @typescript-eslint/no-explicit-any - done: any - ) => { - return this.validate( - _issuer, - uiProfile, - _idProfile, - _context, - _idToken, - _accessToken, - _refreshToken, - _params, - done - ); - } - ); + scope: ['openid', 'email', 'profile'], + skipUserProfile: false, + proxy: + configService.get('OIDC_USE_HTTPS_PROXY') === 'true' + ? true + : undefined, + agent: httpsAgent + }); } - async validate( - _issuer: string, - uiProfile: OIDCProfile, - _idProfile: object, - _context: object, - _idToken: string, - _accessToken: string, - _refreshToken: string, - _params: object, - //eslint-disable-next-line @typescript-eslint/no-explicit-any - done: any - ) { + async validate(...parameters: [ + string, + OIDCProfile, + object, + object, + string, + string, + string, + object, + ]): Promise { + const [, uiProfile] = parameters; this.logger.debug('in oidc strategy file'); this.logger.debug(JSON.stringify(uiProfile, null, 2)); const userData = uiProfile._json; @@ -132,12 +104,10 @@ export class OidcStrategy extends PassportStrategy(Strategy as any, 'oidc') { await this.groupsService.syncUserGroups(user, groups); } - return done(null, user); + return user; } - return done( - new UnauthorizedException( - 'Please verify your name and email with your identity provider before logging into Heimdall.' - ) + throw new UnauthorizedException( + 'Please verify your name and email with your identity provider before logging into Heimdall.' ); } } diff --git a/apps/backend/src/config/config.service.spec.ts b/apps/backend/src/config/config.service.spec.ts index 5f24415668..94760e7339 100644 --- a/apps/backend/src/config/config.service.spec.ts +++ b/apps/backend/src/config/config.service.spec.ts @@ -1,6 +1,15 @@ import * as dotenv from 'dotenv'; import mock from 'mock-fs'; -import {afterAll, beforeAll, describe, expect, it, vi} from 'vitest'; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from 'vitest'; import { DATABASE_URL_MOCK_ENV, ENV_MOCK_FILE, @@ -8,6 +17,21 @@ import { } from '../../test/constants/env-test.constant'; import {ConfigService} from './config.service'; +const withRegistrationDisabled = (value: string | undefined) => { + const configService = new ConfigService(); + configService.set('REGISTRATION_DISABLED', value); + return configService; +}; + +const stubWarningLogger = (configService: ConfigService) => { + const warn = vi.fn(); + Object.defineProperty(configService, 'logger', { value: { warn } }); + return warn; +}; + +const registrationMigrationWarning = 'REGISTRATION_DISABLED=true disables SSO/LDAP auto-account creation as well as local registration. New external-authentication users will be rejected until an administrator pre-creates their accounts. Set REGISTRATION_DISABLED=local for the previous behavior (local registration disabled, SSO auto-creation enabled). If pre-provisioned-only access is intended, no action is needed.'; +const unverifiedOidcEmailWarning = 'OIDC_USES_VERIFIED_EMAIL=false allows an unverified OIDC email claim to bind to a pre-provisioned Heimdall account when REGISTRATION_DISABLED=true or sso, which can enable account takeover. Set OIDC_USES_VERIFIED_EMAIL=true and require administrator-controlled email claims at the identity provider before using pre-provisioned access.'; + // If you run the test without --silent , you need to add console.log() before you mock out the file system in the beforeAll() or it'll throw an error (this is a documented bug which can be found at https://github.com/tschaub/mock-fs/issues/234). If you run the test with --silent (which we do by default), you don't need the log statement. describe('Config Service', () => { beforeAll(async () => { @@ -149,4 +173,251 @@ describe('Config Service', () => { expect(configService.get('test')).toBe('value'); }); }); + + describe('Registration policy', () => { + const originalRegistrationDisabled = process.env.REGISTRATION_DISABLED; + + beforeEach(() => { + delete process.env.REGISTRATION_DISABLED; + }); + + afterEach(() => { + if (originalRegistrationDisabled === undefined) { + delete process.env.REGISTRATION_DISABLED; + } else { + process.env.REGISTRATION_DISABLED = originalRegistrationDisabled; + } + }); + + it('rejects SSO registration when REGISTRATION_DISABLED is sso', () => { + const configService = withRegistrationDisabled('sso'); + + expect(configService.isRegistrationAllowed('sso')).toBe(false); + }); + + it.each([ + [undefined, true, true, true], + ['false', true, true, true], + ['FALSE', true, true, true], + ['true', false, false, false], + ['TRUE', false, false, false], + ['local', false, false, true], + ['LOCAL', false, false, true], + ['sso', true, true, false], + ['SSO', true, true, false], + [' true ', false, false, false], + [' local ', false, false, true], + [' sso ', true, true, false], + ])( + 'maps REGISTRATION_DISABLED=%s across default, local, and SSO scopes', + (value, defaultAllowed, localAllowed, ssoAllowed) => { + const configService = withRegistrationDisabled(value); + + expect(configService.isRegistrationAllowed()).toBe(defaultAllowed); + expect(configService.isRegistrationAllowed('local')).toBe( + localAllowed, + ); + expect(configService.isRegistrationAllowed('sso')).toBe(ssoAllowed); + }, + ); + + it.each([ + undefined, + '', + ' '.repeat(3), + 'false', + 'FALSE', + 'true', + 'TRUE', + 'local', + 'LOCAL', + 'sso', + 'SSO', + ' true ', + ])('accepts REGISTRATION_DISABLED=%s at startup', (value) => { + const configService = withRegistrationDisabled(value); + + expect(() => configService.validateRegistrationDisabled()).not.toThrow(); + }); + + it.each(['1', 'yes', '0', 'banana', 'ture', 'sso-approval'])( + 'rejects invalid REGISTRATION_DISABLED=%s at startup', + (value) => { + const configService = withRegistrationDisabled(value); + + expect(() => configService.validateRegistrationDisabled()).toThrowError( + new Error( + `Invalid REGISTRATION_DISABLED value "${value}". Valid values: false, true, local, sso (case-insensitive).`, + ), + ); + }, + ); + + describe('Startup policy warnings', () => { + it('warns once when REGISTRATION_DISABLED is true and OAuth is the only enabled external strategy', () => { + const configService = withRegistrationDisabled('true'); + configService.set('OIDC_CLIENTID', 'oidc-client-id'); + configService.set('OIDC_CLIENT_SECRET', 'must-not-be-logged'); + const warn = stubWarningLogger(configService); + + configService.validateRegistrationDisabled(); + + expect(configService.enabledOauthStrategies()).toEqual(['oidc']); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith(registrationMigrationWarning); + }); + + it('warns once when REGISTRATION_DISABLED is true and LDAP is the only enabled external strategy', () => { + const configService = withRegistrationDisabled('true'); + configService.set('LDAP_ENABLED', 'true'); + const warn = stubWarningLogger(configService); + + configService.validateRegistrationDisabled(); + + expect(configService.enabledOauthStrategies()).toEqual([]); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith(registrationMigrationWarning); + }); + + it.each<[string, string | undefined, boolean]>([ + ['true with no external strategy', 'true', false], + ['unset with OAuth', undefined, true], + ['false with OAuth', 'false', true], + ['local with OAuth', 'local', true], + ['sso with OAuth', 'sso', true], + ])( + 'does not emit the migration warning for %s', + (_name, registrationDisabled, oauthEnabled) => { + const configService = withRegistrationDisabled(registrationDisabled); + if (oauthEnabled) { + configService.set('OIDC_CLIENTID', 'oidc-client-id'); + } + const warn = stubWarningLogger(configService); + + configService.validateRegistrationDisabled(); + + expect(warn).not.toHaveBeenCalled(); + }, + ); + + it('does not repeat startup warnings during ordinary registration-policy reads', () => { + const configService = withRegistrationDisabled('true'); + configService.set('LDAP_ENABLED', 'true'); + const warn = stubWarningLogger(configService); + + configService.validateRegistrationDisabled(); + expect(configService.isRegistrationAllowed()).toBe(false); + expect(configService.isRegistrationAllowed('local')).toBe(false); + expect(configService.isRegistrationAllowed('sso')).toBe(false); + + expect(warn).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith(registrationMigrationWarning); + }); + + it.each<[string, number]>([ + ['true', 2], + ['sso', 1], + ])( + 'warns about unverified OIDC email matching under REGISTRATION_DISABLED=%s', + (registrationDisabled, warningCount) => { + const configService = withRegistrationDisabled(registrationDisabled); + configService.set('OIDC_CLIENTID', 'oidc-client-id'); + configService.set('OIDC_USES_VERIFIED_EMAIL', 'false'); + const warn = stubWarningLogger(configService); + + configService.validateRegistrationDisabled(); + + expect(warn).toHaveBeenCalledTimes(warningCount); + expect(warn).toHaveBeenCalledWith(unverifiedOidcEmailWarning); + }, + ); + + it.each([ + [undefined, 'false'], + ['false', 'false'], + ['local', 'false'], + ['true', undefined], + ['true', 'true'], + ['true', 'FALSE'], + ['sso', undefined], + ['sso', 'true'], + ['sso', 'FALSE'], + ])( + 'does not emit the unverified-email warning for REGISTRATION_DISABLED=%s and OIDC_USES_VERIFIED_EMAIL=%s', + (registrationDisabled, oidcUsesVerifiedEmail) => { + const configService = withRegistrationDisabled( + registrationDisabled, + ); + configService.set( + 'OIDC_USES_VERIFIED_EMAIL', + oidcUsesVerifiedEmail, + ); + const warn = stubWarningLogger(configService); + + configService.validateRegistrationDisabled(); + + expect(warn).not.toHaveBeenCalled(); + }, + ); + }); + + it.each([ + [undefined, true], + ['false', true], + ['true', false], + ['local', false], + ['sso', true], + ])( + 'maps REGISTRATION_DISABLED=%s to registrationEnabled=%s', + (value, registrationEnabled) => { + const configService = withRegistrationDisabled(value); + + expect( + configService.frontendStartupSettings().registrationEnabled, + ).toBe(registrationEnabled); + }, + ); + }); +}); + +describe('Backend bootstrap', () => { + afterEach(() => { + vi.doUnmock('@nestjs/core'); + vi.doUnmock('../app.module'); + vi.doUnmock('../token/token.providers'); + vi.resetModules(); + }); + + it('validates REGISTRATION_DISABLED exactly once before listening', async () => { + const validateRegistrationDisabled = vi.fn(); + const listen = vi.fn().mockResolvedValue(undefined); + const configService = { + enabledOauthStrategies: vi.fn().mockReturnValue([]), + get: vi.fn().mockReturnValue(undefined), + getSplunkHostUrl: vi.fn().mockReturnValue(''), + getTenableHostUrl: vi.fn().mockReturnValue(''), + isInProductionMode: vi.fn().mockReturnValue(false), + validateRegistrationDisabled, + }; + const app = { + enableShutdownHooks: vi.fn(), + get: vi.fn().mockReturnValue(configService), + listen, + set: vi.fn(), + use: vi.fn(), + useGlobalPipes: vi.fn(), + }; + + vi.doMock('@nestjs/core', () => ({ NestFactory: { create: vi.fn().mockResolvedValue(app) } })); + vi.doMock('../app.module', () => ({ AppModule: class AppModule {} })); + vi.doMock('../token/token.providers', () => ({ generateDefault: vi.fn() })); + + await import('../main.js'); + await vi.waitFor(() => expect(listen).toHaveBeenCalledOnce()); + + expect(validateRegistrationDisabled).toHaveBeenCalledOnce(); + expect(validateRegistrationDisabled.mock.invocationCallOrder[0]).toBeLessThan( + listen.mock.invocationCallOrder[0], + ); + }); }); diff --git a/apps/backend/src/config/config.service.ts b/apps/backend/src/config/config.service.ts index b56c39f48a..0d692dcf0d 100644 --- a/apps/backend/src/config/config.service.ts +++ b/apps/backend/src/config/config.service.ts @@ -1,15 +1,24 @@ import {SequelizeOptions} from 'sequelize-typescript'; +import { createLogger, format, transports } from 'winston'; import AppConfig from '../../config/app_config'; import {StartupSettingsDto} from './dto/startup-settings.dto'; +export type RegistrationScope = 'local' | 'sso'; + export class ConfigService { - private readonly appConfig: AppConfig; - public defaultGithubBaseURL = 'https://github.com/'; - public defaultGithubAPIURL = 'https://api.github.com/'; + private static readonly VALID_REGISTRATION_VALUES = new Set([ + 'false', + 'local', + 'sso', + 'true', + ]); - constructor() { - this.appConfig = new AppConfig(); - } + public defaultGithubAPIURL = 'https://api.github.com/'; + public defaultGithubBaseURL = 'https://github.com/'; + public logger = createLogger({ + format: format.simple(), + transports: [new transports.Console()], + }); public sensitiveKeys = [ /cookie/i, @@ -22,8 +31,53 @@ export class ConfigService { /data/i ]; - isRegistrationAllowed(): boolean { - return this.get('REGISTRATION_DISABLED')?.toLowerCase() !== 'true'; + private readonly appConfig: AppConfig; + + constructor() { + this.appConfig = new AppConfig(); + } + + isRegistrationAllowed(scope: RegistrationScope = 'local'): boolean { + const registrationDisabled = this.registrationDisabledValue()?.normalized; + return registrationDisabled !== 'true' && registrationDisabled !== scope; + } + + validateRegistrationDisabled(): void { + const registrationDisabled = this.registrationDisabledValue(); + if (registrationDisabled === undefined) { + return; + } + + if ( + !ConfigService.VALID_REGISTRATION_VALUES.has( + registrationDisabled.normalized, + ) + ) { + throw new Error( + `Invalid REGISTRATION_DISABLED value "${registrationDisabled.raw}". Valid values: false, true, local, sso (case-insensitive).`, + ); + } + + const isExternalAuthenticationEnabled + = this.enabledOauthStrategies().length > 0 + || this.get('LDAP_ENABLED')?.toLowerCase() === 'true'; + if ( + registrationDisabled.normalized === 'true' + && isExternalAuthenticationEnabled + ) { + this.logger.warn( + 'REGISTRATION_DISABLED=true disables SSO/LDAP auto-account creation as well as local registration. New external-authentication users will be rejected until an administrator pre-creates their accounts. Set REGISTRATION_DISABLED=local for the previous behavior (local registration disabled, SSO auto-creation enabled). If pre-provisioned-only access is intended, no action is needed.', + ); + } + + if ( + ['sso', 'true'].includes(registrationDisabled.normalized) + && this.get('OIDC_USES_VERIFIED_EMAIL') === 'false' + ) { + this.logger.warn( + 'OIDC_USES_VERIFIED_EMAIL=false allows an unverified OIDC email claim to bind to a pre-provisioned Heimdall account when REGISTRATION_DISABLED=true or sso, which can enable account takeover. Set OIDC_USES_VERIFIED_EMAIL=true and require administrator-controlled email claims at the identity provider before using pre-provisioned access.', + ); + } } isLocalLoginAllowed(): boolean { @@ -93,6 +147,13 @@ export class ConfigService { get(key: string): string | undefined { return this.appConfig.get(key); } + + private registrationDisabledValue(): + | undefined + | { normalized: string; raw: string } { + const raw = this.get('REGISTRATION_DISABLED')?.trim(); + return raw ? { normalized: raw.toLowerCase(), raw } : undefined; + } } export const supportedOauth: string[] = [ 'github', diff --git a/apps/backend/src/filters/authentication-exception.filter.ts b/apps/backend/src/filters/authentication-exception.filter.ts index deaff398cf..d8ec711f23 100644 --- a/apps/backend/src/filters/authentication-exception.filter.ts +++ b/apps/backend/src/filters/authentication-exception.filter.ts @@ -1,46 +1,71 @@ -import {ArgumentsHost, Catch, ExceptionFilter} from '@nestjs/common'; +import { + ArgumentsHost, + Catch, + ExceptionFilter, + HttpException, +} from '@nestjs/common'; +import type { Request, Response } from 'express'; import _ from 'lodash'; import winston from 'winston'; -import {ConfigService} from '../config/config.service'; +import { ConfigService } from '../config/config.service'; + +const OAUTH_SECRET_QUERY_KEYS = new Set(['code', 'state']); +const SAFE_DIAGNOSTIC_HEADERS = ['host', 'referer', 'user-agent']; @Catch(Error) export class AuthenticationExceptionFilter implements ExceptionFilter { configService = new ConfigService(); - private readonly line = '_______________________________________________\n'; public loggingTimeFormat = 'MMM-DD-YYYY HH:mm:ss Z'; + private readonly line = '_______________________________________________\n'; public logger = winston.createLogger({ - transports: [new winston.transports.Console()], format: winston.format.combine( - winston.format.timestamp({ - format: this.loggingTimeFormat - }), + winston.format.timestamp({ format: this.loggingTimeFormat }), winston.format.printf( - (info) => - `${this.line}[${[info.timestamp]}] (Authentication Exception Filter): ${info.message}` - ) - ) + info => + `${this.line}[${[info.timestamp]}] (Authentication Exception Filter): ${info.message}`, + ), + ), + transports: [new winston.transports.Console()], }); catch(exception: Error, host: ArgumentsHost): void { - const ctx = host.switchToHttp(); - const request = ctx.getRequest(); - const response = ctx.getResponse(); - const errInfo = { + const context_ = host.switchToHttp(); + const request = context_.getRequest(); + const response = context_.getResponse(); + const authInfo = _.get(request, 'authInfo.message'); + const authInfoMessage = typeof authInfo === 'string' ? authInfo : ''; + const redactedQuery = _.mapValues(request.query, (value, key) => + OAUTH_SECRET_QUERY_KEYS.has(key.toLowerCase()) ? '[REDACTED]' : value, + ); + const errorInfo = { + authInfo: authInfoMessage, + headers: _.pick(request.headers, SAFE_DIAGNOSTIC_HEADERS), message: exception.message, + query: redactedQuery, stack: exception.stack, - authInfo: _.get(request, 'authInfo'), - query: request.query, - headers: request.headers }; this.logger.warn( - `Authentication Error\n${JSON.stringify(errInfo, null, 2)}` + `Authentication Error\n${JSON.stringify(errorInfo, null, 2)}`, ); - const authError = - `${_.has(request, 'authInfo.message') ? _.get(request, 'authInfo.message') : ''}\n${exception.message}`.trim(); - response.cookie('authenticationError', authError, { - secure: this.configService.isInProductionMode() - }); + const authError + = `${authInfoMessage}\n${exception.message}`.trim(); + const cookieOptions = { secure: this.configService.isInProductionMode() }; + response.cookie('authenticationError', authError, cookieOptions); + const exceptionResponse + = exception instanceof HttpException ? exception.getResponse() : undefined; + if ( + typeof exceptionResponse === 'object' + && exceptionResponse !== null + && 'error' in exceptionResponse + && exceptionResponse.error === 'account_not_provisioned' + ) { + response.cookie( + 'authenticationErrorCode', + exceptionResponse.error, + cookieOptions, + ); + } response.redirect(302, '/'); } } diff --git a/apps/backend/src/filters/authentication_exception.filter.spec.ts b/apps/backend/src/filters/authentication_exception.filter.spec.ts new file mode 100644 index 0000000000..4ce8c76646 --- /dev/null +++ b/apps/backend/src/filters/authentication_exception.filter.spec.ts @@ -0,0 +1,266 @@ +import { request as sendHttpRequest } from 'node:http'; +import { UnauthorizedException } from '@nestjs/common'; +import { + EXCEPTION_FILTERS_METADATA, + MODULE_METADATA, +} from '@nestjs/common/constants'; +import { APP_FILTER, Reflector } from '@nestjs/core'; +import { ExecutionContextHost } from '@nestjs/core/helpers/execution-context-host'; +import { AuthGuard } from '@nestjs/passport'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { AppModule } from '../app.module'; +import { AuthnController } from '../authn/authn.controller'; +import { AuthnService } from '../authn/authn.service'; +import { ConfigService } from '../config/config.service'; +import { LocalAuthGuard } from '../guards/local-auth.guard'; +import { AuthenticationExceptionFilter } from './authentication-exception.filter'; + +const NOT_PROVISIONED_RESPONSE = { + error: 'account_not_provisioned', + message: + 'No Heimdall account exists for this SSO user. Please ask your system administrator to create the account.', + statusCode: 401, +}; + +const LdapAuthGuard = AuthGuard('ldap'); + +const createContext = ( + { + authInfo = { message: 'Identity provider rejected login' }, + headers = {}, + query = {}, + }: { + authInfo?: Record; + headers?: Record; + query?: Record; + } = {}, +) => { + const request = { authInfo, headers, query }; + const response = { cookie: vi.fn(), redirect: vi.fn() }; + return { + host: new ExecutionContextHost([request, response]), + response, + }; +}; + +const controllerHandler = (name: 'login' | 'loginToLDAP') => { + const handler = Object.getOwnPropertyDescriptor( + AuthnController.prototype, + name, + )?.value; + if (typeof handler !== 'function') { + throw new TypeError(`Expected AuthnController.${name} to be a function`); + } + return handler; +}; + +const post = (url: string) => + new Promise<{ + body: unknown; + headers: Record; + statusCode: number | undefined; + }>((resolve, reject) => { + const request = sendHttpRequest(url, { method: 'POST' }, (response) => { + const chunks: Buffer[] = []; + response.on('data', (chunk: Buffer) => { + chunks.push(chunk); + }); + response.on('end', () => { + resolve({ + body: JSON.parse(Buffer.concat(chunks).toString('utf8')), + headers: response.headers, + statusCode: response.statusCode, + }); + }); + }); + request.on('error', reject); + request.end(); + }); + +describe('AuthenticationExceptionFilter', () => { + let applyFilter: AuthenticationExceptionFilter['catch']; + let filter: AuthenticationExceptionFilter; + + beforeEach(() => { + vi.restoreAllMocks(); + filter = new AuthenticationExceptionFilter(); + applyFilter = filter.catch.bind(filter); + vi.spyOn(filter.logger, 'warn').mockImplementation(() => filter.logger); + }); + + it('sets authenticationErrorCode for a provisioning denial and redirects to root', () => { + vi.spyOn(filter.configService, 'isInProductionMode').mockReturnValue(true); + const { host, response } = createContext(); + + applyFilter(new UnauthorizedException(NOT_PROVISIONED_RESPONSE), host); + + expect(response.cookie).toHaveBeenNthCalledWith( + 1, + 'authenticationError', + `Identity provider rejected login\n${NOT_PROVISIONED_RESPONSE.message}`, + { secure: true }, + ); + expect(response.cookie).toHaveBeenNthCalledWith( + 2, + 'authenticationErrorCode', + 'account_not_provisioned', + { secure: true }, + ); + expect(response.cookie).toHaveBeenCalledTimes(2); + expect(response.redirect).toHaveBeenCalledExactlyOnceWith(302, '/'); + }); + + it('preserves the existing cookie only for a generic error', () => { + vi.spyOn(filter.configService, 'isInProductionMode').mockReturnValue(false); + const { host, response } = createContext(); + + applyFilter(new Error('OAuth authentication failed'), host); + + expect(response.cookie).toHaveBeenCalledExactlyOnceWith( + 'authenticationError', + 'Identity provider rejected login\nOAuth authentication failed', + { secure: false }, + ); + expect(response.redirect).toHaveBeenCalledExactlyOnceWith(302, '/'); + }); + + it('redacts OAuth query secrets and omits credential headers from WARN diagnostics', () => { + const { host } = createContext({ + authInfo: { + message: 'Identity provider rejected login', + token: 'auth-info-token-secret', + }, + headers: { + authorization: 'Bearer authorization-secret', + cookie: 'session=cookie-secret', + host: 'localhost:3100', + referer: 'http://localhost:3100/login', + 'user-agent': 'curl/8.0', + 'x-api-key': 'header-api-key-secret', + }, + query: { + code: 'oauth-code-secret', + error: 'access_denied', + state: 'oauth-state-secret', + }, + }); + const warn = vi.mocked(filter.logger.warn); + let loggedWarning: unknown; + warn.mockImplementation((message) => { + loggedWarning = message; + return filter.logger; + }); + + applyFilter(new UnauthorizedException(NOT_PROVISIONED_RESPONSE), host); + + expect(warn).toHaveBeenCalledOnce(); + const logMessage = loggedWarning; + if (typeof logMessage !== 'string') { + throw new TypeError('Expected serialized WARN diagnostics'); + } + expect(logMessage).not.toContain('oauth-code-secret'); + expect(logMessage).not.toContain('oauth-state-secret'); + expect(logMessage).not.toContain('authorization-secret'); + expect(logMessage).not.toContain('cookie-secret'); + expect(logMessage).not.toContain('auth-info-token-secret'); + expect(logMessage).not.toContain('header-api-key-secret'); + + const diagnostics = JSON.parse( + logMessage.slice('Authentication Error\n'.length), + ); + expect(diagnostics.authInfo).toBe('Identity provider rejected login'); + expect(diagnostics.headers).toEqual({ + host: 'localhost:3100', + referer: 'http://localhost:3100/login', + 'user-agent': 'curl/8.0', + }); + expect(diagnostics.query).toEqual({ + code: '[REDACTED]', + error: 'access_denied', + state: '[REDACTED]', + }); + }); + + it('leaves LDAP and local POST error transport unchanged', async () => { + const exception = new UnauthorizedException(NOT_PROVISIONED_RESPONSE); + const reflector = new Reflector(); + + const denialGuard = { + canActivate: () => { + throw exception; + }, + }; + const moduleReference = await Test.createTestingModule({ + controllers: [AuthnController], + providers: [ + { provide: AuthnService, useValue: { login: vi.fn() } }, + { + provide: ConfigService, + useValue: { isLocalLoginAllowed: vi.fn().mockReturnValue(true) }, + }, + ], + }) + .overrideGuard(LocalAuthGuard) + .useValue(denialGuard) + .overrideGuard(LdapAuthGuard) + .useValue(denialGuard) + .compile(); + const app = moduleReference.createNestApplication(); + + await app.listen(0, '127.0.0.1'); + try { + const appUrl = await app.getUrl(); + for (const path of ['/authn/login', '/authn/login/ldap']) { + const response = await post(`${appUrl}${path}`); + + expect(response.statusCode).toBe(401); + expect(response.body).toEqual(NOT_PROVISIONED_RESPONSE); + expect(response.headers.location).toBeUndefined(); + expect(response.headers['set-cookie']).toBeUndefined(); + } + } finally { + await app.close(); + } + + expect(exception.getResponse()).toEqual(NOT_PROVISIONED_RESPONSE); + expect( + reflector.getAllAndMerge(EXCEPTION_FILTERS_METADATA, [ + controllerHandler('login'), + AuthnController, + ]), + ).toEqual([]); + expect( + reflector.getAllAndMerge(EXCEPTION_FILTERS_METADATA, [ + controllerHandler('loginToLDAP'), + AuthnController, + ]), + ).toEqual([]); + + const globalProviders + = reflector.get(MODULE_METADATA.PROVIDERS, AppModule) ?? []; + expect( + globalProviders.some( + provider => + typeof provider === 'object' + && provider !== null + && 'provide' in provider + && provider.provide === APP_FILTER + && 'useClass' in provider + && provider.useClass === AuthenticationExceptionFilter, + ), + ).toBe(false); + expect( + reflector.get( + EXCEPTION_FILTERS_METADATA, + controllerHandler('login'), + ), + ).toBeUndefined(); + expect( + reflector.get( + EXCEPTION_FILTERS_METADATA, + controllerHandler('loginToLDAP'), + ), + ).toBeUndefined(); + }); +}); diff --git a/apps/backend/src/main.ts b/apps/backend/src/main.ts index 54768d316b..a1ebcde9bf 100644 --- a/apps/backend/src/main.ts +++ b/apps/backend/src/main.ts @@ -6,9 +6,9 @@ import rateLimit from 'express-rate-limit'; import helmet from 'helmet'; import multer from 'multer'; import winston from 'winston'; -import passport = require('passport'); -import postgresSessionStore = require('connect-pg-simple'); -import session = require('express-session'); +import passport from 'passport'; +import postgresSessionStore from 'connect-pg-simple'; +import session from 'express-session'; import {AppModule} from './app.module'; import {ConfigService} from './config/config.service'; import {generateDefault} from './token/token.providers'; @@ -30,6 +30,7 @@ const logger = winston.createLogger({ async function bootstrap() { const app = await NestFactory.create(AppModule); const configService = app.get(ConfigService); + configService.validateRegistrationDisabled(); app.set('query parser', 'extended'); app.enableShutdownHooks(); app.use(helmet()); diff --git a/apps/backend/src/users/users.controller.spec.ts b/apps/backend/src/users/users.controller.spec.ts index 3a2b730d62..9807c241f2 100644 --- a/apps/backend/src/users/users.controller.spec.ts +++ b/apps/backend/src/users/users.controller.spec.ts @@ -85,6 +85,7 @@ describe('UsersController Unit Tests', () => { beforeEach(async () => { await databaseService.cleanAll(); + configService.set('REGISTRATION_DISABLED', undefined); const userDto = await usersService.create(CREATE_USER_DTO_TEST_OBJ); basicUser = await usersService.findByPkBang(userDto.id); const adminDto = await usersService.create(CREATE_ADMIN_DTO); @@ -126,20 +127,62 @@ describe('UsersController Unit Tests', () => { }); }); - describe('Create function with registration enabled', () => { - // Tests the create function with valid dto (basic positive test) - it('should test the create function with valid dto', async () => { - expect.assertions(1); + describe('Local registration policy', () => { + it.each([undefined, 'false', 'sso'])( + 'allows anonymous local registration when REGISTRATION_DISABLED=%s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); - const createdUser = await usersController.create( - CREATE_USER_DTO_TEST_OBJ_2, - {} - ); - expect(createdUser).toEqual( - new UserDto(await usersService.findById(createdUser.id)) - ); - }); + const createdUser = await usersController.create( + CREATE_USER_DTO_TEST_OBJ_2, + {}, + ); + + expect(createdUser).toEqual( + new UserDto(await usersService.findById(createdUser.id)), + ); + expect(await usersService.count()).toBe(3); + }, + ); + + it.each(['true', 'local'])( + 'rejects anonymous local registration when REGISTRATION_DISABLED=%s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + + const registration = usersController.create( + CREATE_USER_DTO_TEST_OBJ_2, + {}, + ); + + await expect(registration).rejects.toBeInstanceOf(ForbiddenError); + await expect(registration).rejects.toHaveProperty( + 'message', + 'User registration is disabled. Please ask your system administrator to create the account.', + ); + expect(await usersService.count()).toBe(2); + }, + ); + + it.each(['true', 'local'])( + 'allows the administrator ForceRegistration bypass when REGISTRATION_DISABLED=%s', + async (registrationDisabled) => { + configService.set('REGISTRATION_DISABLED', registrationDisabled); + + const createdUser = await usersController.create( + CREATE_USER_DTO_TEST_OBJ_2, + { user: adminUser }, + ); + + expect(createdUser).toEqual( + new UserDto(await usersService.findById(createdUser.id)), + ); + expect(await usersService.count()).toBe(3); + }, + ); + }); + describe('Create function with registration enabled', () => { // Tests the create function with dto that is missing email it('should test the create function with missing email field', async () => { expect.assertions(1); @@ -177,18 +220,6 @@ describe('UsersController Unit Tests', () => { }); }); - describe('Create function with registration disabled', () => { - it('should test the create function with valid dto', async () => { - expect.assertions(1); - - configService.set('REGISTRATION_DISABLED', 'true'); - - await expect( - usersController.create(CREATE_USER_DTO_TEST_OBJ_2, {}) - ).rejects.toBeInstanceOf(ForbiddenError); - }); - }); - describe('Update function', () => { // Tests the update function with valid dto (basic positive test) it('should test the update function with a valid update dto', async () => { diff --git a/apps/backend/test/.env-ci b/apps/backend/test/.env-ci index 31a2147a75..9d5eac4460 100644 --- a/apps/backend/test/.env-ci +++ b/apps/backend/test/.env-ci @@ -7,6 +7,9 @@ JWT_SECRET=abc123 NODE_ENV=test EXTERNAL_URL=http://127.0.0.1:3000 +# REGISTRATION_DISABLED is intentionally unset for the ordinary JIT-on job. +# The isolated registration-policy CI matrix job injects REGISTRATION_DISABLED=sso. + GITHUB_ENTERPRISE_INSTANCE_BASE_URL=http://127.0.0.1:3001/ GITHUB_ENTERPRISE_INSTANCE_API_URL=http://127.0.0.1:3001/ diff --git a/apps/frontend/src/components/global/login/LDAPLogin.vue b/apps/frontend/src/components/global/login/LDAPLogin.vue index 58722c6e01..ab649a1ed4 100644 --- a/apps/frontend/src/components/global/login/LDAPLogin.vue +++ b/apps/frontend/src/components/global/login/LDAPLogin.vue @@ -42,12 +42,13 @@ diff --git a/apps/frontend/src/components/global/login/LocalLogin.vue b/apps/frontend/src/components/global/login/LocalLogin.vue index c9e14fc22b..1e3544fc54 100644 --- a/apps/frontend/src/components/global/login/LocalLogin.vue +++ b/apps/frontend/src/components/global/login/LocalLogin.vue @@ -155,17 +155,28 @@