fix(api): honor bypassTwoFactor PAT on twoFactorRequired endpoints (#41065)

Author: ggazzo

.changeset/sup-1064-pat-bypass-two-factor.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/meteor': patch
+'@rocket.chat/core-typings': patch
+---
+
+Fixes REST API endpoints that require two-factor authentication (such as `users.update`) rejecting requests authenticated with a Personal Access Token created with "Ignore Two Factor Authentication", returning `totp-required` even though the token was meant to bypass the check. The two-factor authorization check now resolves the login token from the REST connection, so `bypassTwoFactor` tokens are honored again.

apps/meteor/.mocharc.js

@@ -27,6 +27,7 @@ module.exports = {
 		'tests/unit/lib/**/*.spec.ts',
 		'tests/unit/server/**/*.tests.ts',
 		'tests/unit/server/**/*.spec.ts',
+		'app/2fa/server/**/*.spec.ts',
 		'app/api/server/lib/**/*.spec.ts',
 		'app/file-upload/server/**/*.spec.ts',
 		'app/statistics/server/**/*.spec.ts',

apps/meteor/app/2fa/server/code/index.spec.ts

@@ -0,0 +1,135 @@
+import { expect } from 'chai';
+import { afterEach, beforeEach, describe, it } from 'mocha';
+import proxyquire from 'proxyquire';
+import sinon from 'sinon';
+
+const settingsMock = sinon.stub();
+const getLoginTokenStub = sinon.stub();
+
+class TOTPCheckMock {
+	name = 'totp';
+
+	isEnabled() {
+		return true;
+	}
+
+	async processInvalidCode() {
+		return {};
+	}
+
+	async verify() {
+		return false;
+	}
+
+	async maxFaildedAttemtpsReached() {
+		return false;
+	}
+}
+
+class DisabledCheckMock {
+	name = 'email';
+
+	isEnabled() {
+		return false;
+	}
+}
+
+class MeteorErrorMock extends Error {
+	error: string;
+
+	details: unknown;
+
+	constructor(error: string, reason?: string, details?: unknown) {
+		super(reason);
+		this.error = error;
+		this.details = details;
+	}
+}
+
+const { checkCodeForUser } = proxyquire.noCallThru().load('./index', {
+	'./TOTPCheck': { TOTPCheck: TOTPCheckMock },
+	'./EmailCheck': { EmailCheck: DisabledCheckMock },
+	'./PasswordCheckFallback': { PasswordCheckFallback: class extends DisabledCheckMock {} },
+	'../../../lib/server/functions/getModifiedHttpHeaders': { normalizeHeaders: (headers: unknown) => headers },
+	'../../../settings/server': { settings: { get: settingsMock } },
+	'@rocket.chat/models': {
+		Users: {
+			findOneById: async () => null,
+			setTwoFactorAuthorizationHashAndUntilForUserIdAndToken: async () => undefined,
+		},
+	},
+	'meteor/accounts-base': { Accounts: { _getLoginToken: getLoginTokenStub } },
+	'meteor/meteor': { Meteor: { Error: MeteorErrorMock } },
+});
+
+const HASHED_TOKEN = 'hashed-login-token';
+
+const userWithBypassToken = {
+	_id: 'user-id',
+	services: { resume: { loginTokens: [{ hashedToken: HASHED_TOKEN, bypassTwoFactor: true }] } },
+} as any;
+
+const connection = {
+	id: 'connection-id',
+	clientAddress: '127.0.0.1',
+	httpHeaders: {},
+} as any;
+
+describe('checkCodeForUser - bypassTwoFactor token resolution (SUP-1064)', () => {
+	let testMode: string | undefined;
+
+	beforeEach(() => {
+		// TEST_MODE short-circuits the whole check; remove it so the token resolution path runs.
+		testMode = process.env.TEST_MODE;
+		delete process.env.TEST_MODE;
+
+		settingsMock.reset();
+		settingsMock.withArgs('Accounts_TwoFactorAuthentication_Enabled').returns(true);
+		getLoginTokenStub.reset();
+	});
+
+	afterEach(() => {
+		if (testMode !== undefined) {
+			process.env.TEST_MODE = testMode;
+		}
+	});
+
+	it('should honor a bypassTwoFactor token resolved from the REST connection (connection.token)', async () => {
+		// REST: the token is not registered in account data, only carried on the connection.
+		getLoginTokenStub.returns(undefined);
+
+		const authorized = await checkCodeForUser({
+			user: userWithBypassToken,
+			connection: { ...connection, token: HASHED_TOKEN },
+			options: {},
+		});
+
+		expect(authorized).to.be.equal(true);
+	});
+
+	it('should honor a bypassTwoFactor token resolved from account data (DDP, _getLoginToken)', async () => {
+		// DDP: the token is registered in Accounts._accountData and read via _getLoginToken.
+		getLoginTokenStub.returns(HASHED_TOKEN);
+
+		const authorized = await checkCodeForUser({
+			user: userWithBypassToken,
+			connection: { ...connection, token: undefined },
+			options: {},
+		});
+
+		expect(authorized).to.be.equal(true);
+	});
+
+	it('should still require a second factor when the token cannot be resolved from either source', async () => {
+		// Regression guard: this is the buggy state (#38017) the fix addresses.
+		getLoginTokenStub.returns(undefined);
+
+		await expect(
+			checkCodeForUser({
+				user: userWithBypassToken,
+				connection: { ...connection, token: undefined },
+				options: {},
+			}),
+		).to.be.rejectedWith('TOTP Required');
+	});
+});

apps/meteor/app/2fa/server/code/index.ts

@@ -77,7 +77,11 @@ function getRememberDate(from: Date = new Date()): Date | undefined {
 }
 
 function isAuthorizedForToken(connection: IMethodConnection, user: IUser, options: ITwoFactorOptions): boolean {
-	const currentToken = Accounts._getLoginToken(connection.id);
+	// Resolve the current login token from both transports:
+	// - DDP: the login flow registers it in `Accounts._accountData`, read via `_getLoginToken`.
+	// - REST: it is not registered in account data, so it is carried on `connection.token`.
+	// Both are needed; REST is the fallback that fixes `bypassTwoFactor` PATs (SUP-1064).
+	const currentToken = Accounts._getLoginToken(connection.id) || connection.token;
 	const tokenObject = user.services?.resume?.loginTokens?.find((i) => i.hashedToken === currentToken);
 
 	if (!tokenObject) {
@@ -119,7 +123,9 @@ function isAuthorizedForToken(connection: IMethodConnection, user: IUser, option
 }
 
 async function rememberAuthorization(connection: IMethodConnection, user: IUser): Promise<void> {
-	const currentToken = Accounts._getLoginToken(connection.id);
+	// Same dual-transport resolution as `isAuthorizedForToken`: DDP reads from `Accounts._accountData`
+	// via `_getLoginToken`, REST falls back to the token carried on `connection.token`.
+	const currentToken = Accounts._getLoginToken(connection.id) || connection.token;
 
 	const expires = getRememberDate();
 	if (!expires) {

packages/core-typings/src/IMethodConnection.ts

@@ -4,4 +4,10 @@ export interface IMethodConnection {
 	onClose(fn: (...args: any[]) => void): void;
 	clientAddress: string;
 	httpHeaders: Record<string, any>;
+	/**
+	 * Hashed login token carried by REST connections. Unlike DDP connections, REST requests do not
+	 * register the login token in `Accounts._accountData`, so it is exposed here for the two-factor
+	 * authorization check to resolve the token without mutating global account data.
+	 */
+	token?: string;
 }