fix(core): Do not validate email when LDAP is enabled (#13605)

This commit is contained in:
Ricardo Espinoza 2025-03-03 19:15:52 +01:00 committed by GitHub
parent 24681f843c
commit 17738c5096
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
16 changed files with 187 additions and 83 deletions

View file

@ -36,7 +36,7 @@ describe('User Management', { disableAutoLogin: true }, () => {
it('should login and logout', () => {
cy.visit('/');
cy.get('input[name="email"]').type(INSTANCE_OWNER.email);
cy.get('input[name="emailOrLdapLoginId"]').type(INSTANCE_OWNER.email);
cy.get('input[name="password"]').type(INSTANCE_OWNER.password);
cy.getByTestId('form-submit-button').click();
mainSidebar.getters.logo().should('be.visible');
@ -47,7 +47,7 @@ describe('User Management', { disableAutoLogin: true }, () => {
mainSidebar.actions.openUserMenu();
cy.getByTestId('user-menu-item-logout').click();
cy.get('input[name="email"]').type(INSTANCE_MEMBERS[0].email);
cy.get('input[name="emailOrLdapLoginId"]').type(INSTANCE_MEMBERS[0].email);
cy.get('input[name="password"]').type(INSTANCE_MEMBERS[0].password);
cy.getByTestId('form-submit-button').click();
mainSidebar.getters.logo().should('be.visible');

View file

@ -506,7 +506,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
mainSidebar.actions.openUserMenu();
cy.getByTestId('user-menu-item-logout').click();
cy.get('input[name="email"]').type(INSTANCE_MEMBERS[0].email);
cy.get('input[name="emailOrLdapLoginId"]').type(INSTANCE_MEMBERS[0].email);
cy.get('input[name="password"]').type(INSTANCE_MEMBERS[0].password);
cy.getByTestId('form-submit-button').click();

View file

@ -15,7 +15,7 @@ export class SigninPage extends BasePage {
getters = {
form: () => cy.getByTestId('auth-form'),
email: () => cy.getByTestId('email'),
email: () => cy.getByTestId('emailOrLdapLoginId'),
password: () => cy.getByTestId('password'),
submit: () => cy.get('button'),
};

View file

@ -69,7 +69,7 @@ Cypress.Commands.add('signin', ({ email, password }) => {
.request({
method: 'POST',
url: `${BACKEND_BASE_URL}/rest/login`,
body: { email, password },
body: { emailOrLdapLoginId: email, password },
failOnStatusCode: false,
})
.then((response) => {

View file

@ -6,7 +6,7 @@ describe('LoginRequestDto', () => {
{
name: 'complete valid login request',
request: {
email: 'test@example.com',
emailOrLdapLoginId: 'test@example.com',
password: 'securePassword123',
mfaCode: '123456',
},
@ -14,14 +14,14 @@ describe('LoginRequestDto', () => {
{
name: 'login request without optional MFA',
request: {
email: 'test@example.com',
emailOrLdapLoginId: 'test@example.com',
password: 'securePassword123',
},
},
{
name: 'login request with both mfaCode and mfaRecoveryCode',
request: {
email: 'test@example.com',
emailOrLdapLoginId: 'test@example.com',
password: 'securePassword123',
mfaCode: '123456',
mfaRecoveryCode: 'recovery-code-123',
@ -30,7 +30,7 @@ describe('LoginRequestDto', () => {
{
name: 'login request with only mfaRecoveryCode',
request: {
email: 'test@example.com',
emailOrLdapLoginId: 'test@example.com',
password: 'securePassword123',
mfaRecoveryCode: 'recovery-code-123',
},
@ -44,43 +44,35 @@ describe('LoginRequestDto', () => {
describe('Invalid requests', () => {
test.each([
{
name: 'invalid email',
name: 'invalid emailOrLdapLoginId',
request: {
email: 'invalid-email',
emailOrLdapLoginId: 0,
password: 'securePassword123',
},
expectedErrorPath: ['email'],
expectedErrorPath: ['emailOrLdapLoginId'],
},
{
name: 'empty password',
request: {
email: 'test@example.com',
emailOrLdapLoginId: 'test@example.com',
password: '',
},
expectedErrorPath: ['password'],
},
{
name: 'missing email',
name: 'missing emailOrLdapLoginId',
request: {
password: 'securePassword123',
},
expectedErrorPath: ['email'],
expectedErrorPath: ['emailOrLdapLoginId'],
},
{
name: 'missing password',
request: {
email: 'test@example.com',
emailOrLdapLoginId: 'test@example.com',
},
expectedErrorPath: ['password'],
},
{
name: 'whitespace in email and password',
request: {
email: ' test@example.com ',
password: ' securePassword123 ',
},
expectedErrorPath: ['email'],
},
])('should fail validation for $name', ({ request, expectedErrorPath }) => {
const result = LoginRequestDto.safeParse(request);
expect(result.success).toBe(false);

View file

@ -2,7 +2,12 @@ import { z } from 'zod';
import { Z } from 'zod-class';
export class LoginRequestDto extends Z.class({
email: z.string().email(),
/*
* The LDAP username does not need to be an email, so email validation
* is not enforced here. The controller determines whether this is an
* email and validates when LDAP is disabled
*/
emailOrLdapLoginId: z.string().trim(),
password: z.string().min(1),
mfaCode: z.string().optional(),
mfaRecoveryCode: z.string().optional(),

View file

@ -0,0 +1,99 @@
import type { LoginRequestDto } from '@n8n/api-types';
import { Container } from '@n8n/di';
import type { Response } from 'express';
import { mock } from 'jest-mock-extended';
import { Logger } from 'n8n-core';
import * as auth from '@/auth';
import { AuthService } from '@/auth/auth.service';
import config from '@/config';
import type { User } from '@/databases/entities/user';
import { UserRepository } from '@/databases/repositories/user.repository';
import { EventService } from '@/events/event.service';
import { License } from '@/license';
import { MfaService } from '@/mfa/mfa.service';
import { PostHogClient } from '@/posthog';
import type { AuthenticatedRequest } from '@/requests';
import { UserService } from '@/services/user.service';
import { mockInstance } from '@test/mocking';
import { AuthController } from '../auth.controller';
jest.mock('@/auth');
const mockedAuth = auth as jest.Mocked<typeof auth>;
describe('AuthController', () => {
mockInstance(Logger);
mockInstance(EventService);
mockInstance(AuthService);
mockInstance(MfaService);
mockInstance(UserService);
mockInstance(UserRepository);
mockInstance(PostHogClient);
mockInstance(License);
const controller = Container.get(AuthController);
const userService = Container.get(UserService);
const authService = Container.get(AuthService);
const eventsService = Container.get(EventService);
const postHog = Container.get(PostHogClient);
describe('login', () => {
it('should not validate email in "emailOrLdapLoginId" if LDAP is enabled', async () => {
// Arrange
const browserId = '1';
const member = mock<User>({
id: '123',
role: 'global:member',
mfaEnabled: false,
});
const body = mock<LoginRequestDto>({
emailOrLdapLoginId: 'non email',
password: 'password',
});
const req = mock<AuthenticatedRequest>({
user: member,
body,
browserId,
});
const res = mock<Response>();
mockedAuth.handleEmailLogin.mockResolvedValue(member);
mockedAuth.handleLdapLogin.mockResolvedValue(member);
config.set('userManagement.authenticationMethod', 'ldap');
// Act
await controller.login(req, res, body);
// Assert
expect(mockedAuth.handleEmailLogin).toHaveBeenCalledWith(
body.emailOrLdapLoginId,
body.password,
);
expect(mockedAuth.handleLdapLogin).toHaveBeenCalledWith(
body.emailOrLdapLoginId,
body.password,
);
expect(authService.issueCookie).toHaveBeenCalledWith(res, member, browserId);
expect(eventsService.emit).toHaveBeenCalledWith('user-logged-in', {
user: member,
authenticationMethod: 'ldap',
});
expect(userService.toPublic).toHaveBeenCalledWith(member, {
posthog: postHog,
withScopes: true,
});
});
});
});

View file

@ -1,4 +1,5 @@
import { LoginRequestDto, ResolveSignupTokenQueryDto } from '@n8n/api-types';
import { isEmail } from 'class-validator';
import { Response } from 'express';
import { Logger } from 'n8n-core';
@ -44,14 +45,19 @@ export class AuthController {
res: Response,
@Body payload: LoginRequestDto,
): Promise<PublicUser | undefined> {
const { email, password, mfaCode, mfaRecoveryCode } = payload;
const { emailOrLdapLoginId, password, mfaCode, mfaRecoveryCode } = payload;
let user: User | undefined;
let usedAuthenticationMethod = getCurrentAuthenticationMethod();
if (usedAuthenticationMethod === 'email' && !isEmail(emailOrLdapLoginId)) {
throw new BadRequestError('Invalid email address');
}
if (isSamlCurrentAuthenticationMethod()) {
// attempt to fetch user data with the credentials, but don't log in yet
const preliminaryUser = await handleEmailLogin(email, password);
const preliminaryUser = await handleEmailLogin(emailOrLdapLoginId, password);
// if the user is an owner, continue with the login
if (
preliminaryUser?.role === 'global:owner' ||
@ -63,15 +69,15 @@ export class AuthController {
throw new AuthError('SSO is enabled, please log in with SSO');
}
} else if (isLdapCurrentAuthenticationMethod()) {
const preliminaryUser = await handleEmailLogin(email, password);
const preliminaryUser = await handleEmailLogin(emailOrLdapLoginId, password);
if (preliminaryUser?.role === 'global:owner') {
user = preliminaryUser;
usedAuthenticationMethod = 'email';
} else {
user = await handleLdapLogin(email, password);
user = await handleLdapLogin(emailOrLdapLoginId, password);
}
} else {
user = await handleEmailLogin(email, password);
user = await handleEmailLogin(emailOrLdapLoginId, password);
}
if (user) {
@ -101,7 +107,7 @@ export class AuthController {
}
this.eventService.emit('user-login-failed', {
authenticationMethod: usedAuthenticationMethod,
userEmail: email,
userEmail: emailOrLdapLoginId,
reason: 'wrong credentials',
});
throw new AuthError('Wrong username or password. Do you have caps lock on?');

View file

@ -43,7 +43,7 @@ describe('POST /login', () => {
test('should log user in', async () => {
const response = await testServer.authlessAgent.post('/login').send({
email: owner.email,
emailOrLdapLoginId: owner.email,
password: ownerPassword,
});
@ -87,7 +87,7 @@ describe('POST /login', () => {
await mfaService.enableMfa(owner.id);
const response = await testServer.authlessAgent.post('/login').send({
email: owner.email,
emailOrLdapLoginId: owner.email,
password: ownerPassword,
mfaCode: mfaService.totp.generateTOTP(secret),
});
@ -131,7 +131,7 @@ describe('POST /login', () => {
});
const response = await testServer.authlessAgent.post('/login').send({
email: member.email,
emailOrLdapLoginId: member.email,
password,
});
expect(response.statusCode).toBe(403);
@ -148,19 +148,16 @@ describe('POST /login', () => {
expect(response.statusCode).toBe(200);
});
test('should fail on invalid email in the payload', async () => {
test('should fail with invalid email in the payload is the current authentication method is "email"', async () => {
config.set('userManagement.authenticationMethod', 'email');
const response = await testServer.authlessAgent.post('/login').send({
email: 'invalid-email',
emailOrLdapLoginId: 'invalid-email',
password: ownerPassword,
});
expect(response.statusCode).toBe(400);
expect(response.body).toEqual({
validation: 'email',
code: 'invalid_string',
message: 'Invalid email',
path: ['email'],
});
expect(response.body.message).toBe('Invalid email address');
});
});

View file

@ -470,7 +470,7 @@ describe('POST /login', () => {
const response = await testServer.authlessAgent
.post('/login')
.send({ email: ldapUser.mail, password: 'password' });
.send({ emailOrLdapLoginId: ldapUser.mail, password: 'password' });
expect(response.statusCode).toBe(200);
expect(response.headers['set-cookie']).toBeDefined();
@ -529,7 +529,7 @@ describe('POST /login', () => {
const response = await testServer.authlessAgent
.post('/login')
.send({ email: owner.email, password: 'password' });
.send({ emailOrLdapLoginId: owner.email, password: 'password' });
expect(response.status).toBe(200);
expect(response.body.data?.signInType).toBeDefined();

View file

@ -268,7 +268,7 @@ describe('Change password with MFA enabled', () => {
.authAgentFor(user)
.post('/login')
.send({
email: user.email,
emailOrLdapLoginId: user.email,
password: newPassword,
mfaCode: new TOTPService().generateTOTP(rawSecret),
})
@ -306,7 +306,10 @@ describe('Login', () => {
const user = await createUser({ password });
await testServer.authlessAgent.post('/login').send({ email: user.email, password }).expect(200);
await testServer.authlessAgent
.post('/login')
.send({ emailOrLdapLoginId: user.email, password })
.expect(200);
});
test('GET /login should not include mfaSecret and mfaRecoveryCodes property in response', async () => {
@ -323,7 +326,7 @@ describe('Login', () => {
await testServer.authlessAgent
.post('/login')
.send({ email: user.email, password: rawPassword })
.send({ emailOrLdapLoginId: user.email, password: rawPassword })
.expect(401);
});
@ -333,7 +336,7 @@ describe('Login', () => {
await testServer.authlessAgent
.post('/login')
.send({ email: user.email, password: rawPassword, mfaCode: 'wrongvalue' })
.send({ emailOrLdapLoginId: user.email, password: rawPassword, mfaCode: 'wrongvalue' })
.expect(401);
});
@ -342,7 +345,7 @@ describe('Login', () => {
const response = await testServer.authlessAgent
.post('/login')
.send({ email: user.email, password: rawPassword })
.send({ emailOrLdapLoginId: user.email, password: rawPassword })
.expect(401);
expect(response.body.code).toBe(998);
@ -355,7 +358,7 @@ describe('Login', () => {
const response = await testServer.authlessAgent
.post('/login')
.send({ email: user.email, password: rawPassword, mfaCode: token })
.send({ emailOrLdapLoginId: user.email, password: rawPassword, mfaCode: token })
.expect(200);
const data = response.body.data;
@ -370,7 +373,11 @@ describe('Login', () => {
await testServer.authlessAgent
.post('/login')
.send({ email: user.email, password: rawPassword, mfaRecoveryCode: 'wrongvalue' })
.send({
emailOrLdapLoginId: user.email,
password: rawPassword,
mfaRecoveryCode: 'wrongvalue',
})
.expect(401);
});
@ -379,7 +386,11 @@ describe('Login', () => {
const response = await testServer.authlessAgent
.post('/login')
.send({ email: user.email, password: rawPassword, mfaRecoveryCode: rawRecoveryCodes[0] })
.send({
emailOrLdapLoginId: user.email,
password: rawPassword,
mfaRecoveryCode: rawRecoveryCodes[0],
})
.expect(200);
const data = response.body.data;

View file

@ -1,4 +1,5 @@
import type {
LoginRequestDto,
PasswordUpdateRequestDto,
SettingsUpdateRequestDto,
UserUpdateRequestDto,
@ -21,7 +22,7 @@ export async function loginCurrentUser(
export async function login(
context: IRestApiContext,
params: { email: string; password: string; mfaCode?: string; mfaRecoveryToken?: string },
params: LoginRequestDto,
): Promise<CurrentUserResponse> {
return await makeRestApiRequest(context, 'POST', '/login', params);
}

View file

@ -1,4 +1,5 @@
import type {
LoginRequestDto,
PasswordUpdateRequestDto,
SettingsUpdateRequestDto,
UserUpdateRequestDto,
@ -181,12 +182,7 @@ export const useUsersStore = defineStore(STORES.USERS, () => {
};
};
const loginWithCreds = async (params: {
email: string;
password: string;
mfaCode?: string;
mfaRecoveryCode?: string;
}) => {
const loginWithCreds = async (params: LoginRequestDto) => {
const user = await usersApi.login(rootStore.restApiContext, params);
if (!user) {
return;

View file

@ -3,6 +3,7 @@ import Logo from '@/components/Logo/Logo.vue';
import SSOLogin from '@/components/SSOLogin.vue';
import type { IFormBoxConfig } from '@/Interface';
import { useSettingsStore } from '@/stores/settings.store';
import type { EmailOrLdapLoginIdAndPassword } from './SigninView.vue';
withDefaults(
defineProps<{
@ -19,7 +20,7 @@ withDefaults(
const emit = defineEmits<{
update: [{ name: string; value: string }];
submit: [values: { [key: string]: string }];
submit: [values: EmailOrLdapLoginIdAndPassword];
secondaryClick: [];
}>();
@ -27,7 +28,7 @@ const onUpdate = (e: { name: string; value: string }) => {
emit('update', e);
};
const onSubmit = (values: { [key: string]: string }) => {
const onSubmit = (values: EmailOrLdapLoginIdAndPassword) => {
emit('submit', values);
};

View file

@ -85,7 +85,7 @@ describe('SigninView', () => {
await userEvent.click(submitButton);
expect(usersStore.loginWithCreds).toHaveBeenCalledWith({
email: 'test@n8n.io',
emailOrLdapLoginId: 'test@n8n.io',
password: 'password',
mfaCode: undefined,
mfaRecoveryCode: undefined,

View file

@ -15,6 +15,14 @@ import { useCloudPlanStore } from '@/stores/cloudPlan.store';
import type { IFormBoxConfig } from '@/Interface';
import { MFA_AUTHENTICATION_REQUIRED_ERROR_CODE, VIEWS, MFA_FORM } from '@/constants';
import type { LoginRequestDto } from '@n8n/api-types';
export type EmailOrLdapLoginIdAndPassword = Pick<
LoginRequestDto,
'emailOrLdapLoginId' | 'password'
>;
export type MfaCodeOrMfaRecoveryCode = Pick<LoginRequestDto, 'mfaCode' | 'mfaRecoveryCode'>;
const usersStore = useUsersStore();
const settingsStore = useSettingsStore();
@ -29,7 +37,7 @@ const telemetry = useTelemetry();
const loading = ref(false);
const showMfaView = ref(false);
const email = ref('');
const emailOrLdapLoginId = ref('');
const password = ref('');
const reportError = ref(false);
@ -50,7 +58,7 @@ const formConfig: IFormBoxConfig = reactive({
redirectLink: '/forgot-password',
inputs: [
{
name: 'email',
name: 'emailOrLdapLoginId',
properties: {
label: emailLabel.value,
type: 'email',
@ -78,23 +86,16 @@ const formConfig: IFormBoxConfig = reactive({
],
});
const onMFASubmitted = async (form: { mfaCode?: string; mfaRecoveryCode?: string }) => {
const onMFASubmitted = async (form: MfaCodeOrMfaRecoveryCode) => {
await login({
email: email.value,
emailOrLdapLoginId: emailOrLdapLoginId.value,
password: password.value,
mfaCode: form.mfaCode,
mfaRecoveryCode: form.mfaRecoveryCode,
});
};
const isFormWithEmailAndPassword = (values: {
[key: string]: string;
}): values is { email: string; password: string } => {
return 'email' in values && 'password' in values;
};
const onEmailPasswordSubmitted = async (form: { [key: string]: string }) => {
if (!isFormWithEmailAndPassword(form)) return;
const onEmailPasswordSubmitted = async (form: EmailOrLdapLoginIdAndPassword) => {
await login(form);
};
@ -111,16 +112,11 @@ const getRedirectQueryParameter = () => {
return redirect;
};
const login = async (form: {
email: string;
password: string;
mfaCode?: string;
mfaRecoveryCode?: string;
}) => {
const login = async (form: LoginRequestDto) => {
try {
loading.value = true;
await usersStore.loginWithCreds({
email: form.email,
emailOrLdapLoginId: form.emailOrLdapLoginId,
password: form.password,
mfaCode: form.mfaCode,
mfaRecoveryCode: form.mfaRecoveryCode,
@ -185,8 +181,8 @@ const onFormChanged = (toForm: string) => {
reportError.value = false;
}
};
const cacheCredentials = (form: { email: string; password: string }) => {
email.value = form.email;
const cacheCredentials = (form: EmailOrLdapLoginIdAndPassword) => {
emailOrLdapLoginId.value = form.emailOrLdapLoginId;
password.value = form.password;
};
</script>