Skip to content

Commit

Permalink
fix(core): Make sure mfa secret and recovery codes are not returned o…
Browse files Browse the repository at this point in the history
…n login (#7936)

## Summary

What: Fix issue of login endpoint returning secret and recovery codes
when MFA is enabled. Bug was introduced in this
[PR](#6994), specifically in this
[line](https://github.com/n8n-io/n8n/pull/6994/files#diff-95a87cb029a3d26e6722df2e68132453fc254fc1f4540cbdaa95cfdbda1893deL91).

Why: We should not be filtering the secret and recovery codes

Same PR caused the issues on ticket ->
https://linear.app/n8n/issue/ADO-1494/on-user-list-copy-password-reset-link-and-copy-invite-link-are-broken

## Review / Merge checklist
- [x] PR title and summary are descriptive. **Remember, the title
automatically goes into the changelog. Use `(no-changelog)` otherwise.**
([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md))
- [x] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up
ticket created.
- [x] Tests included.
> A bug is not considered fixed, unless a test is added to prevent it
from happening again. A feature is not complete without tests.
  >
> *(internal)* You can use Slack commands to trigger [e2e
tests](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#a39f9e5ba64a48b58a71d81c837e8227)
or [deploy test
instance](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#f6a177d32bde4b57ae2da0b8e454bfce)
or [deploy early access version on
Cloud](https://www.notion.so/n8n/Cloudbot-3dbe779836004972b7057bc989526998?pvs=4#fef2d36ab02247e1a0f65a74f6fb534e).
  • Loading branch information
RicardoE105 authored Dec 6, 2023
1 parent 46dd4d3 commit f5502cc
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 1 deletion.
3 changes: 2 additions & 1 deletion packages/cli/src/services/user.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ export class UserService {
user: User,
options?: { withInviteUrl?: boolean; posthog?: PostHogClient; withScopes?: boolean },
) {
const { password, updatedAt, apiKey, authIdentities, ...rest } = user;
const { password, updatedAt, apiKey, authIdentities, mfaRecoveryCodes, mfaSecret, ...rest } =
user;

const ldapIdentity = authIdentities?.find((i) => i.providerType === 'ldap');

Expand Down
53 changes: 53 additions & 0 deletions packages/cli/test/integration/auth.api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import * as utils from './shared/utils/';
import { getGlobalMemberRole, getGlobalOwnerRole } from './shared/db/roles';
import { createUser, createUserShell } from './shared/db/users';
import { UserRepository } from '@db/repositories/user.repository';
import { MfaService } from '@/Mfa/mfa.service';

let globalOwnerRole: Role;
let globalMemberRole: Role;
Expand All @@ -22,9 +23,12 @@ const ownerPassword = randomValidPassword();
const testServer = utils.setupTestServer({ endpointGroups: ['auth'] });
const license = testServer.license;

let mfaService: MfaService;

beforeAll(async () => {
globalOwnerRole = await getGlobalOwnerRole();
globalMemberRole = await getGlobalMemberRole();
mfaService = Container.get(MfaService);
});

beforeEach(async () => {
Expand Down Expand Up @@ -59,6 +63,8 @@ describe('POST /login', () => {
globalRole,
apiKey,
globalScopes,
mfaSecret,
mfaRecoveryCodes,
} = response.body.data;

expect(validator.isUUID(id)).toBe(true);
Expand All @@ -73,6 +79,53 @@ describe('POST /login', () => {
expect(globalRole.scope).toBe('global');
expect(apiKey).toBeUndefined();
expect(globalScopes).toBeDefined();
expect(mfaRecoveryCodes).toBeUndefined();
expect(mfaSecret).toBeUndefined();

const authToken = utils.getAuthToken(response);
expect(authToken).toBeDefined();
});

test('should log user with MFA enabled', async () => {
const secret = 'test';
const recoveryCodes = ['1'];
await mfaService.saveSecretAndRecoveryCodes(owner.id, secret, recoveryCodes);
await mfaService.enableMfa(owner.id);

const response = await testServer.authlessAgent.post('/login').send({
email: owner.email,
password: ownerPassword,
mfaToken: mfaService.totp.generateTOTP(secret),
});

expect(response.statusCode).toBe(200);

const {
id,
email,
firstName,
lastName,
password,
personalizationAnswers,
globalRole,
apiKey,
mfaRecoveryCodes,
mfaSecret,
} = response.body.data;

expect(validator.isUUID(id)).toBe(true);
expect(email).toBe(owner.email);
expect(firstName).toBe(owner.firstName);
expect(lastName).toBe(owner.lastName);
expect(password).toBeUndefined();
expect(personalizationAnswers).toBeNull();
expect(password).toBeUndefined();
expect(globalRole).toBeDefined();
expect(globalRole.name).toBe('owner');
expect(globalRole.scope).toBe('global');
expect(apiKey).toBeUndefined();
expect(mfaRecoveryCodes).toBeUndefined();
expect(mfaSecret).toBeUndefined();

const authToken = utils.getAuthToken(response);
expect(authToken).toBeDefined();
Expand Down

0 comments on commit f5502cc

Please sign in to comment.