Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/meteor/.mocharc.api.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ module.exports = /** @satisfies {import('mocha').MochaOptions} */ ({
retries: 0,
file: 'tests/end-to-end/teardown.ts',
reporter: 'tests/end-to-end/reporter.ts',
spec: ['tests/end-to-end/api/*.ts', 'tests/end-to-end/api/helpers/**/*', 'tests/end-to-end/api/methods/**/*', 'tests/end-to-end/apps/*'],
// spec: ['tests/end-to-end/api/*.ts', 'tests/end-to-end/api/helpers/**/*', 'tests/end-to-end/api/methods/**/*', 'tests/end-to-end/apps/*'],
spec: ['tests/end-to-end/api/login-code.ts'],
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Resolved
});
1 change: 1 addition & 0 deletions apps/meteor/app/api/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import './v1/teams';
import './v1/moderation';
import './v1/uploads';
import './v1/twoFactorChallenges';
import './v1/loginCode';
// This has to come last so all endpoints are registered before generating the OpenAPI documentation
import './default/openApi';

Expand Down
71 changes: 71 additions & 0 deletions apps/meteor/app/api/server/v1/loginCode.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { LoginCodes } from '@rocket.chat/models';
import { ajv, validateBadRequestErrorResponse, validateUnauthorizedErrorResponse } from '@rocket.chat/rest-typings';
import type { JSONSchemaType } from 'ajv';
import { Accounts } from 'meteor/accounts-base';

import type { ExtractRoutesFromAPI } from '../ApiClass';
import { API } from '../api';

const loginCodeRedeemResponse = ajv.compile<{ loginToken: string; userId: string }>({
type: 'object',
properties: {
loginToken: { type: 'string' },
userId: { type: 'string' },
success: { type: 'boolean', enum: [true] },
},
required: ['loginToken', 'userId', 'success'],
additionalProperties: false,
});

type LoginCodeRedeemParams = { code: string };

const LoginCodeRedeemSchema: JSONSchemaType<LoginCodeRedeemParams> = {
type: 'object',
properties: {
code: { type: 'string', minLength: 64, maxLength: 64 },
Comment thread
yash-rajpal marked this conversation as resolved.
Resolved
},
required: ['code'],
additionalProperties: false,
};

const isLoginCodeRedeemParamsPOST = ajv.compile<LoginCodeRedeemParams>(LoginCodeRedeemSchema);

const loginCodeEndpoints = API.v1.post(
'loginCode.redeem',
{
authRequired: false,
body: isLoginCodeRedeemParamsPOST,
rateLimiterOptions: { intervalTimeInMS: 60000, numRequestsAllowed: 10 },
response: {
200: loginCodeRedeemResponse,
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
},
},
async function action() {
const { code } = this.bodyParams;

const loginCode = await LoginCodes.findOneNotExpiredByCodeAndDelete(code);

if (!loginCode) {
return API.v1.failure('error-invalid-code');
}

const { userId } = loginCode;

const stampedToken = Accounts._generateStampedLoginToken();
await Accounts._insertLoginToken(userId, stampedToken);

return API.v1.success({
loginToken: stampedToken.token,
userId,
});
},
);

type LoginCodeEndpoints = ExtractRoutesFromAPI<typeof loginCodeEndpoints>;

declare module '@rocket.chat/rest-typings' {
// eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface
interface Endpoints extends LoginCodeEndpoints {}
}
2 changes: 2 additions & 0 deletions apps/meteor/client/views/root/AppLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { useLoginOtherClients } from './hooks/useLoginOtherClients';
import { useLoginViaQuery } from './hooks/useLoginViaQuery';
import { useMessageLinkClicks } from './hooks/useMessageLinkClicks';
import { useNotificationPermission } from './hooks/useNotificationPermission';
import { useOAuthLogin } from './hooks/useOAuthLogin';
import { useRedirectToSetupWizard } from './hooks/useRedirectToSetupWizard';
import { useSettingsOnLoadSiteUrl } from './hooks/useSettingsOnLoadSiteUrl';
import { useShareSessionWithOtherClients } from './hooks/useShareSessionWithOtherClients';
Expand Down Expand Up @@ -73,6 +74,7 @@ const AppLayout = () => {
useCodeHighlight();
useLoginViaQuery();
useLoginOtherClients();
useOAuthLogin();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: OAuth login error handling is overridden by an unconditional /home redirect in finally, causing incorrect post-failure navigation.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/root/AppLayout.tsx, line 77:

<comment>OAuth login error handling is overridden by an unconditional `/home` redirect in `finally`, causing incorrect post-failure navigation.</comment>

<file context>
@@ -73,6 +74,7 @@ const AppLayout = () => {
 	useCodeHighlight();
 	useLoginViaQuery();
 	useLoginOtherClients();
+	useOAuthLogin();
 	useShareSessionWithOtherClients();
 	useLoadMissedMessages();
</file context>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Adding useOAuthLogin alongside useLoginViaQuery introduces a concurrent dual-login race when both query credentials are present.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/root/AppLayout.tsx, line 77:

<comment>Adding `useOAuthLogin` alongside `useLoginViaQuery` introduces a concurrent dual-login race when both query credentials are present.</comment>

<file context>
@@ -73,6 +74,7 @@ const AppLayout = () => {
 	useCodeHighlight();
 	useLoginViaQuery();
 	useLoginOtherClients();
+	useOAuthLogin();
 	useShareSessionWithOtherClients();
 	useLoadMissedMessages();
</file context>

useShareSessionWithOtherClients();
useLoadMissedMessages();
useDesktopFavicon();
Expand Down
7 changes: 1 addition & 6 deletions apps/meteor/client/views/root/hooks/useLoginViaQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,12 @@ export const useLoginViaQuery = () => {

useEffect(() => {
const handleLogin = async () => {
const { resumeToken, loginClient } = router.getSearchParameters();
const { resumeToken } = router.getSearchParameters();

if (!resumeToken) {
return;
Comment thread
yash-rajpal marked this conversation as resolved.
Resolved
}

//Case handled by useLoginOtherClients, we don't want to login here.
if (loginClient) {
return;
}

try {
await loginWithToken(resumeToken);

Expand Down
49 changes: 49 additions & 0 deletions apps/meteor/client/views/root/hooks/useOAuthLogin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { useEndpoint, useRouter, useSearchParameter, useLoginWithToken } from '@rocket.chat/ui-contexts';
import { useEffect } from 'react';

import { buildDeepLinkURL } from '../../../lib/buildAuthDeeplinkURL';

export const useOAuthLogin = () => {
const router = useRouter();
const loginCode = useSearchParameter('loginCode');
const loginClient = useSearchParameter('loginClient');
const redeemLoginCode = useEndpoint('POST', '/v1/loginCode.redeem');
const loginWithToken = useLoginWithToken();

useEffect(() => {
if (!loginCode) {
return;
}

let timeout: NodeJS.Timeout;

const handleLogin = async () => {
try {
const { loginToken, userId } = await redeemLoginCode({ code: loginCode });

if (!loginToken || !userId) {
throw new Error('Invalid response from login code redemption');
}

if (loginClient === 'desktop' || loginClient === 'mobile') {
const loginURL = buildDeepLinkURL(loginToken, userId);
window.location.href = loginURL;
return;
}

await loginWithToken(loginToken);

timeout = setTimeout(() => {
router.navigate('/home', { replace: true });
}, 0);
} catch (error) {
console.error('Failed to redeem login code for client redirect', error);
router.navigate('/login', { replace: true });
}
};
Comment thread
yash-rajpal marked this conversation as resolved.
Resolved

handleLogin();

return () => clearTimeout(timeout);
}, [loginCode, router, redeemLoginCode, loginWithToken, loginClient]);
};
8 changes: 3 additions & 5 deletions apps/meteor/server/lib/oauth/passportOAuthCallback.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { IUser } from '@rocket.chat/core-typings';
import { Logger } from '@rocket.chat/logger';
import { LoginCodes } from '@rocket.chat/models';
import type { Request, Response } from 'express';
import { Accounts } from 'meteor/accounts-base';

import { doesUserRequire2FA } from './twoFactorAuth';

Expand Down Expand Up @@ -29,13 +29,11 @@ export const passportOAuthCallback = (siteUrl: string) => async (req: Request, r
return res.redirect(twoFARedirectUrl.toString());
}

const stampedToken = Accounts._generateStampedLoginToken();
await Accounts._insertLoginToken(oAuthUser._id, stampedToken);
const loginCode = await LoginCodes.createCode(oAuthUser._id);

const redirectUrl = new URL(`/home`, siteUrl);

redirectUrl.searchParams.set('resumeToken', stampedToken.token);
redirectUrl.searchParams.set('userId', oAuthUser._id);
redirectUrl.searchParams.set('loginCode', loginCode);

if (loginClient) {
redirectUrl.searchParams.set('loginClient', loginClient);
Expand Down
2 changes: 2 additions & 0 deletions apps/meteor/server/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
OAuthAccessTokensRaw,
OAuthAppsRaw,
OAuthAuthCodesRaw,
LoginCodesRaw,
OAuthRefreshTokensRaw,
OEmbedCacheRaw,
PermissionsRaw,
Expand Down Expand Up @@ -138,6 +139,7 @@ registerModel('INpsVoteModel', new NpsVoteRaw(db));
registerModel('IOAuthAccessTokensModel', new OAuthAccessTokensRaw(db));
registerModel('IOAuthAppsModel', new OAuthAppsRaw(db));
registerModel('IOAuthAuthCodesModel', new OAuthAuthCodesRaw(db));
registerModel('ILoginCodesModel', new LoginCodesRaw(db));
registerModel('IOAuthRefreshTokensModel', new OAuthRefreshTokensRaw(db));
registerModel('IOEmbedCacheModel', new OEmbedCacheRaw(db));
registerModel('IPermissionsModel', new PermissionsRaw(db, trashCollection));
Expand Down
140 changes: 140 additions & 0 deletions apps/meteor/tests/end-to-end/api/login-code.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import { randomBytes } from 'crypto';

import type { ILoginCode, IUser } from '@rocket.chat/core-typings';
import { expect } from 'chai';
import { after, before, describe, it } from 'mocha';
import { MongoClient } from 'mongodb';

import { api, request, getCredentials } from '../../data/api-data';
import type { TestUser } from '../../data/users.helper';
import { createUser, deleteUser } from '../../data/users.helper';
import { URL_MONGODB } from '../../e2e/config/constants';

const CODE_TTL_MS = 60 * 1000;
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Resolved

async function insertLoginCode(connection: MongoClient, userId: string, insertExpiredToken: boolean = false): Promise<string> {
const now = new Date();
const code = randomBytes(32).toString('hex');

let expireAt = new Date(now.getTime() + CODE_TTL_MS);

if (insertExpiredToken) {
expireAt = new Date(now.getTime() - 1);
}

await connection.db().collection<ILoginCode>('rocketchat_login_codes').insertOne({
_id: code,
userId,
createdAt: now,
expireAt,
});

return code;
}

describe('[Login Codes]', () => {
let connection: MongoClient;

before((done) => getCredentials(done));

before(async () => {
connection = await MongoClient.connect(URL_MONGODB);
});

after(async () => {
await connection.close();
});

describe('POST [/loginCode.redeem]', () => {
let testUser: TestUser<IUser>;

before(async () => {
testUser = await createUser();
});

after(async () => {
await deleteUser(testUser);
});

it('should fail when code does not exist in the database', async () => {
await request
.post(api('loginCode.redeem'))
.send({ code: 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2' })
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
});
});

it('should fail when code is an empty string', async () => {
await request
.post(api('loginCode.redeem'))
.send({ code: '' })
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('errorType', 'invalid-params');
});
});

it('should return a loginToken and userId for a valid code', async () => {
const code = await insertLoginCode(connection, testUser._id);

await request
.post(api('loginCode.redeem'))
.send({ code })
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('loginToken').that.is.a('string').and.is.not.empty;
expect(res.body).to.have.property('userId', testUser._id);
});
});

it('should invalidate a code after it has been redeemed', async () => {
const code = await insertLoginCode(connection, testUser._id);

await request.post(api('loginCode.redeem')).send({ code }).expect(200);

await request
.post(api('loginCode.redeem'))
.send({ code })
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('error', 'error-invalid-code');
});
});

it('should return a loginToken that can be used for subsequent authenticated requests', async () => {
const code = await insertLoginCode(connection, testUser._id);

const redeemRes = await request.post(api('loginCode.redeem')).send({ code }).expect(200);

const { loginToken, userId } = redeemRes.body;

await request
.get(api('me'))
.set('X-Auth-Token', loginToken)
.set('X-User-Id', userId)
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('_id', testUser._id);
});
});

it('should fail when code is expired', async () => {
const code = await insertLoginCode(connection, testUser._id, true);

await request
.post(api('loginCode.redeem'))
.send({ code })
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('error', 'error-invalid-code');
});
});
});
});
6 changes: 6 additions & 0 deletions packages/core-typings/src/ILoginCode.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export interface ILoginCode {
_id: string;
userId: string;
createdAt: Date;
expireAt: Date;
}
1 change: 1 addition & 0 deletions packages/core-typings/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export type * from './IEmojiCustom';
export type * from './ICustomEmojiDescriptor';
export type * from './IAnalytics';
export type * from './ICredentialToken';
export type * from './ILoginCode';
export type * from './IAvatar';
export type * from './ICustomUserStatus';
export type * from './IEmailMessageHistory';
Expand Down
1 change: 1 addition & 0 deletions packages/model-typings/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export type * from './models/IBaseModel';
export type * from './models/IBaseUploadsModel';
export type * from './models/ICannedResponseModel';
export type * from './models/ICredentialTokensModel';
export type * from './models/ILoginCodesModel';
export type * from './models/ICustomSoundsModel';
export type * from './models/ICustomUserStatusModel';
export type * from './models/IEmailInboxModel';
Expand Down
8 changes: 8 additions & 0 deletions packages/model-typings/src/models/ILoginCodesModel.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { ILoginCode } from '@rocket.chat/core-typings';

import type { IBaseModel } from './IBaseModel';

export interface ILoginCodesModel extends IBaseModel<ILoginCode> {
createCode(userId: string): Promise<string>;
findOneNotExpiredByCodeAndDelete(code: string): Promise<ILoginCode | null>;
}
Loading
Loading