From 479f90231d0a03c69b17189384812b5a1d81ef3d Mon Sep 17 00:00:00 2001 From: Jon Date: Thu, 2 Nov 2023 03:02:49 +0000 Subject: [PATCH] fix(core): Fix issue that prevents owner logging in when using ldap (#7408) This PR prioritises the internal email account over LDAP for the Owner. --------- Co-authored-by: ricardo --- packages/cli/src/Ldap/helpers.ts | 6 +++- packages/cli/src/auth/methods/email.ts | 3 +- .../cli/src/controllers/auth.controller.ts | 8 ++++- .../test/integration/ldap/ldap.api.test.ts | 34 +++++++++++++++++-- 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/Ldap/helpers.ts b/packages/cli/src/Ldap/helpers.ts index 999c28d5b3..ea8bad5479 100644 --- a/packages/cli/src/Ldap/helpers.ts +++ b/packages/cli/src/Ldap/helpers.ts @@ -29,7 +29,7 @@ import { isLdapCurrentAuthenticationMethod, setCurrentAuthenticationMethod, } from '@/sso/ssoHelpers'; -import { InternalServerError } from '../ResponseHelper'; +import { BadRequestError, InternalServerError } from '../ResponseHelper'; import { RoleService } from '@/services/role.service'; import { Logger } from '@/Logger'; @@ -155,6 +155,10 @@ export const updateLdapConfig = async (ldapConfig: LdapConfig): Promise => throw new Error(message); } + if (ldapConfig.loginEnabled && getCurrentAuthenticationMethod() === 'saml') { + throw new BadRequestError('LDAP cannot be enabled if SSO in enabled'); + } + LdapManager.updateConfig({ ...ldapConfig }); ldapConfig.bindingAdminPassword = Container.get(Cipher).encrypt(ldapConfig.bindingAdminPassword); diff --git a/packages/cli/src/auth/methods/email.ts b/packages/cli/src/auth/methods/email.ts index c55d8a2964..d1e7475381 100644 --- a/packages/cli/src/auth/methods/email.ts +++ b/packages/cli/src/auth/methods/email.ts @@ -4,6 +4,7 @@ import { compareHash } from '@/UserManagement/UserManagementHelper'; import * as ResponseHelper from '@/ResponseHelper'; import { Container } from 'typedi'; import { InternalHooks } from '@/InternalHooks'; +import { isLdapLoginEnabled } from '@/Ldap/helpers'; export const handleEmailLogin = async ( email: string, @@ -21,7 +22,7 @@ export const handleEmailLogin = async ( // At this point if the user has a LDAP ID, means it was previously an LDAP user, // so suggest to reset the password to gain access to the instance. const ldapIdentity = user?.authIdentities?.find((i) => i.providerType === 'ldap'); - if (user && ldapIdentity) { + if (user && ldapIdentity && !isLdapLoginEnabled()) { void Container.get(InternalHooks).userLoginFailedDueToLdapDisabled({ user_id: user.id, }); diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index bb0581ef73..81737c71ca 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -66,7 +66,13 @@ export class AuthController { throw new AuthError('SSO is enabled, please log in with SSO'); } } else if (isLdapCurrentAuthenticationMethod()) { - user = await handleLdapLogin(email, password); + const preliminaryUser = await handleEmailLogin(email, password); + if (preliminaryUser?.globalRole?.name === 'owner') { + user = preliminaryUser; + usedAuthenticationMethod = 'email'; + } else { + user = await handleLdapLogin(email, password); + } } else { user = await handleEmailLogin(email, password); } diff --git a/packages/cli/test/integration/ldap/ldap.api.test.ts b/packages/cli/test/integration/ldap/ldap.api.test.ts index 81d8641f42..64cf2525fe 100644 --- a/packages/cli/test/integration/ldap/ldap.api.test.ts +++ b/packages/cli/test/integration/ldap/ldap.api.test.ts @@ -50,14 +50,12 @@ beforeAll(async () => { globalMemberRole = fetchedGlobalMemberRole; - owner = await testDb.createUser({ globalRole: globalOwnerRole }); + owner = await testDb.createUser({ globalRole: globalOwnerRole, password: 'password' }); authOwnerAgent = testServer.authAgentFor(owner); defaultLdapConfig.bindingAdminPassword = Container.get(Cipher).encrypt( defaultLdapConfig.bindingAdminPassword, ); - - await setCurrentAuthenticationMethod('email'); }); beforeEach(async () => { @@ -75,6 +73,8 @@ beforeEach(async () => { jest.mock('@/telemetry'); config.set('userManagement.isInstanceOwnerSetUp', true); + + await setCurrentAuthenticationMethod('email'); }); const createLdapConfig = async (attributes: Partial = {}): Promise => { @@ -145,6 +145,19 @@ describe('PUT /ldap/config', () => { expect(response.body.data.loginLabel).toBe(''); }); + test('route should fail due to trying to enable LDAP login with SSO as current authentication method', async () => { + const validPayload = { + ...LDAP_DEFAULT_CONFIGURATION, + loginEnabled: true, + }; + + config.set('userManagement.authenticationMethod', 'saml'); + + const response = await authOwnerAgent.put('/ldap/config').send(validPayload); + + expect(response.statusCode).toBe(400); + }); + test('should apply "Convert all LDAP users to email users" strategy when LDAP login disabled', async () => { const ldapConfig = await createLdapConfig(); LdapManager.updateConfig(ldapConfig); @@ -471,6 +484,8 @@ describe('POST /login', () => { const ldapConfig = await createLdapConfig(); LdapManager.updateConfig(ldapConfig); + await setCurrentAuthenticationMethod('ldap'); + jest.spyOn(LdapService.prototype, 'searchWithAdminBinding').mockResolvedValue([ldapUser]); jest.spyOn(LdapService.prototype, 'validUser').mockResolvedValue(); @@ -528,6 +543,19 @@ describe('POST /login', () => { await runTest(ldapUser); }); + test('should allow instance owner to sign in with email/password when LDAP is enabled', async () => { + const ldapConfig = await createLdapConfig(); + LdapManager.updateConfig(ldapConfig); + + const response = await testServer.authlessAgent + .post('/login') + .send({ email: owner.email, password: 'password' }); + + expect(response.status).toBe(200); + expect(response.body.data?.signInType).toBeDefined(); + expect(response.body.data?.signInType).toBe('email'); + }); + test('should transform email user into LDAP user when match found', async () => { const ldapUser = { mail: randomEmail(),