From b5179597f3ec4ae468b2eb91969fa56322088e6f Mon Sep 17 00:00:00 2001 From: Michael Auerswald Date: Fri, 3 Mar 2023 10:05:30 +0100 Subject: [PATCH] feat(core): Limit user changes when saml is enabled (#5577) * consolidate SSO settings * update saml settings * fix type error * limit user changes when saml is enabled * add test --- packages/cli/src/controllers/me.controller.ts | 25 ++++++ .../saml/middleware/samlEnabledMiddleware.ts | 5 +- packages/cli/src/sso/saml/samlHelpers.ts | 5 ++ packages/cli/src/sso/ssoHelpers.ts | 6 ++ .../test/integration/saml/saml.api.test.ts | 78 +++++++++++++++++++ 5 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 packages/cli/test/integration/saml/saml.api.test.ts diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 5c830a6803..e56c820155 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/packages/cli/src/controllers/me.controller.ts @@ -22,6 +22,7 @@ import type { IInternalHooksClass, } from '@/Interfaces'; import { randomBytes } from 'crypto'; +import { isSamlLicensedAndEnabled } from '../sso/saml/samlHelpers'; @RestController('/me') export class MeController { @@ -77,6 +78,20 @@ export class MeController { await validateEntity(payload); + // If SAML is enabled, we don't allow the user to change their email address + if (isSamlLicensedAndEnabled()) { + if (email !== currentEmail) { + this.logger.debug( + 'Request to update user failed because SAML user may not change their email', + { + userId, + payload, + }, + ); + throw new BadRequestError('SAML user may not change their email'); + } + } + await this.userRepository.update(userId, payload); const user = await this.userRepository.findOneOrFail({ where: { id: userId }, @@ -105,6 +120,16 @@ export class MeController { async updatePassword(req: MeRequest.Password, res: Response) { const { currentPassword, newPassword } = req.body; + // If SAML is enabled, we don't allow the user to change their email address + if (isSamlLicensedAndEnabled()) { + this.logger.debug('Attempted to change password for user, while SAML is enabled', { + userId: req.user?.id, + }); + throw new BadRequestError( + 'With SAML enabled, users need to use their SAML provider to change passwords', + ); + } + if (typeof currentPassword !== 'string' || typeof newPassword !== 'string') { throw new BadRequestError('Invalid payload.'); } diff --git a/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts b/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts index bf15030d83..257d694e48 100644 --- a/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts +++ b/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts @@ -1,7 +1,6 @@ import type { RequestHandler } from 'express'; import type { AuthenticatedRequest } from '../../../requests'; -import { isSamlCurrentAuthenticationMethod } from '../../ssoHelpers'; -import { isSamlLoginEnabled, isSamlLicensed } from '../samlHelpers'; +import { isSamlLicensed, isSamlLicensedAndEnabled } from '../samlHelpers'; export const samlLicensedOwnerMiddleware: RequestHandler = ( req: AuthenticatedRequest, @@ -16,7 +15,7 @@ export const samlLicensedOwnerMiddleware: RequestHandler = ( }; export const samlLicensedAndEnabledMiddleware: RequestHandler = (req, res, next) => { - if (isSamlLoginEnabled() && isSamlLicensed() && isSamlCurrentAuthenticationMethod()) { + if (isSamlLicensedAndEnabled()) { next(); } else { res.status(401).json({ status: 'error', message: 'Unauthorized' }); diff --git a/packages/cli/src/sso/saml/samlHelpers.ts b/packages/cli/src/sso/saml/samlHelpers.ts index c6c0c6c30f..fb9e0a3b7f 100644 --- a/packages/cli/src/sso/saml/samlHelpers.ts +++ b/packages/cli/src/sso/saml/samlHelpers.ts @@ -10,6 +10,7 @@ import type { SamlUserAttributes } from './types/samlUserAttributes'; import type { FlowResult } from 'samlify/types/src/flow'; import type { SamlAttributeMapping } from './types/samlAttributeMapping'; import { SAML_ENTERPRISE_FEATURE_ENABLED, SAML_LOGIN_ENABLED, SAML_LOGIN_LABEL } from './constants'; +import { isSamlCurrentAuthenticationMethod } from '../ssoHelpers'; /** * Check whether the SAML feature is licensed and enabled in the instance */ @@ -37,6 +38,10 @@ export function isSamlLicensed(): boolean { ); } +export function isSamlLicensedAndEnabled(): boolean { + return isSamlLoginEnabled() && isSamlLicensed() && isSamlCurrentAuthenticationMethod(); +} + export const isSamlPreferences = (candidate: unknown): candidate is SamlPreferences => { const o = candidate as SamlPreferences; return ( diff --git a/packages/cli/src/sso/ssoHelpers.ts b/packages/cli/src/sso/ssoHelpers.ts index f00ddc0fe5..9c8b03e450 100644 --- a/packages/cli/src/sso/ssoHelpers.ts +++ b/packages/cli/src/sso/ssoHelpers.ts @@ -11,3 +11,9 @@ export function isSsoJustInTimeProvisioningEnabled(): boolean { export function doRedirectUsersFromLoginToSsoFlow(): boolean { return config.getEnv('sso.redirectLoginToSso'); } + +export function setCurrentAuthenticationMethod( + authenticationMethod: 'email' | 'ldap' | 'saml', +): void { + config.set('userManagement.authenticationMethod', authenticationMethod); +} diff --git a/packages/cli/test/integration/saml/saml.api.test.ts b/packages/cli/test/integration/saml/saml.api.test.ts new file mode 100644 index 0000000000..02176989f1 --- /dev/null +++ b/packages/cli/test/integration/saml/saml.api.test.ts @@ -0,0 +1,78 @@ +import express from 'express'; + +import config from '@/config'; +import type { Role } from '@db/entities/Role'; +import { randomEmail, randomName, randomValidPassword } from '../shared/random'; +import * as testDb from '../shared/testDb'; +import type { AuthAgent } from '../shared/types'; +import * as utils from '../shared/utils'; +import { setSamlLoginEnabled } from '../../../src/sso/saml/samlHelpers'; +import { setCurrentAuthenticationMethod } from '../../../src/sso/ssoHelpers'; + +let app: express.Application; +let globalOwnerRole: Role; +let globalMemberRole: Role; +let authAgent: AuthAgent; + +function enableSaml(enable: boolean) { + setSamlLoginEnabled(enable); + setCurrentAuthenticationMethod(enable ? 'saml' : 'email'); + config.set('enterprise.features.saml', enable); +} + +beforeAll(async () => { + app = await utils.initTestServer({ endpointGroups: ['me'] }); + + globalOwnerRole = await testDb.getGlobalOwnerRole(); + globalMemberRole = await testDb.getGlobalMemberRole(); + + authAgent = utils.createAuthAgent(app); +}); + +beforeEach(async () => { + await testDb.truncate(['User']); +}); + +afterAll(async () => { + await testDb.terminate(); +}); + +describe('Owner shell', () => { + test('PATCH /me should succeed with valid inputs', async () => { + const ownerShell = await testDb.createUserShell(globalOwnerRole); + const authOwnerShellAgent = authAgent(ownerShell); + const response = await authOwnerShellAgent.patch('/me').send({ + email: randomEmail(), + firstName: randomName(), + lastName: randomName(), + password: randomValidPassword(), + }); + expect(response.statusCode).toBe(200); + }); + + test('PATCH /me should throw BadRequestError if email is changed when SAML is enabled', async () => { + enableSaml(true); + const ownerShell = await testDb.createUserShell(globalOwnerRole); + const authOwnerShellAgent = authAgent(ownerShell); + const response = await authOwnerShellAgent.patch('/me').send({ + email: randomEmail(), + firstName: randomName(), + lastName: randomName(), + }); + expect(response.statusCode).toBe(400); + expect(response.body.message).toContain('SAML'); + enableSaml(false); + }); + + test('PATCH /password should throw BadRequestError if password is changed when SAML is enabled', async () => { + enableSaml(true); + const ownerShell = await testDb.createUserShell(globalOwnerRole); + const authOwnerShellAgent = authAgent(ownerShell); + const response = await authOwnerShellAgent.patch('/me/password').send({ + password: randomValidPassword(), + }); + expect(response.statusCode).toBe(400); + expect(response.body.message).toContain('SAML'); + enableSaml(false); + }); +});