Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Option to disable 2FA for OAuth users #32945

Merged
merged 29 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
214015c
implement new setting
yash-rajpal Jul 30, 2024
969caf2
Hide option in UI for oAuth users
yash-rajpal Jul 30, 2024
fdb0fac
add cs
yash-rajpal Jul 31, 2024
6a1a5a5
OAuth user definition
yash-rajpal Aug 2, 2024
5bf39c7
oops
yash-rajpal Aug 2, 2024
ae866f7
remove console.log
yash-rajpal Aug 2, 2024
53ff5ad
fix return condition
yash-rajpal Aug 2, 2024
1480e82
fix review
yash-rajpal Aug 2, 2024
87e974a
Merge branch 'develop' into feat/disable-email-2FA-oauth
yash-rajpal Aug 2, 2024
7c79251
revert oAuth types to any
yash-rajpal Aug 5, 2024
4d97527
fix review
yash-rajpal Aug 5, 2024
063ff50
replace setting behavior
yash-rajpal Aug 5, 2024
4149692
add unit tests
yash-rajpal Aug 5, 2024
c542bd0
Merge branch 'develop' into feat/disable-email-2FA-oauth
yash-rajpal Aug 12, 2024
351f0a0
fix review
yash-rajpal Aug 12, 2024
c9ad659
revert password fallack password check
yash-rajpal Aug 12, 2024
56bdbf9
translations
yash-rajpal Aug 12, 2024
94c57df
Merge branch 'develop' into feat/disable-email-2FA-oauth
yash-rajpal Aug 16, 2024
d7fc847
jest new test config
yash-rajpal Aug 16, 2024
c9bb363
don't mock isOAuthUser function
yash-rajpal Aug 20, 2024
0b3d6f3
Update apps/meteor/app/2fa/server/code/EmailCheck.spec.ts
yash-rajpal Aug 20, 2024
c9edf9e
control disabling and showing from higher component
yash-rajpal Aug 20, 2024
3c5ebca
unit testing the component no longer makes sense
yash-rajpal Aug 20, 2024
560df75
get entire services object
yash-rajpal Aug 20, 2024
2abce3c
setting name semantics
yash-rajpal Aug 22, 2024
d4cdd53
Merge branch 'develop' into feat/disable-email-2FA-oauth
kodiakhq[bot] Sep 9, 2024
974fe47
Merge branch 'develop' into feat/disable-email-2FA-oauth
scuciatto Sep 9, 2024
bb717b6
Merge branch 'develop' into feat/disable-email-2FA-oauth
kodiakhq[bot] Sep 9, 2024
b878dc7
Merge branch 'develop' into feat/disable-email-2FA-oauth
kodiakhq[bot] Sep 9, 2024
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
6 changes: 6 additions & 0 deletions .changeset/tiny-geckos-kiss.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@rocket.chat/i18n': minor
'@rocket.chat/meteor': minor
---

Added a new setting which allows workspace admins to disable email two factor authentication for SSO (OAuth) users. If enabled, SSO users won't be asked for email two factor authentication.
70 changes: 70 additions & 0 deletions apps/meteor/app/2fa/server/code/EmailCheck.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { expect } from 'chai';
import { describe, it } from 'mocha';
import proxyquire from 'proxyquire';
import sinon from 'sinon';

const settingsMock = sinon.stub();

const { EmailCheck } = proxyquire.noCallThru().load('./EmailCheck', {
'@rocket.chat/models': {
Users: {},
},
'meteor/accounts-base': {
Accounts: {
_bcryptRounds: () => '123',
},
},
'../../../../server/lib/i18n': {
i18n: {
t: (key: string) => key,
},
},
'../../../mailer/server/api': {
send: () => undefined,
},
'../../../settings/server': {
settings: {
get: settingsMock,
},
},
});

const normalUserMock = { services: { email2fa: { enabled: true } }, emails: [{ email: '[email protected]', verified: true }] };
const normalUserWithUnverifiedEmailMock = {
services: { email2fa: { enabled: true } },
emails: [{ email: '[email protected]', verified: false }],
};
const OAuthUserMock = { services: { google: {} }, emails: [{ email: '[email protected]', verified: true }] };

describe('EmailCheck', () => {
let emailCheck: typeof EmailCheck;
beforeEach(() => {
settingsMock.reset();

emailCheck = new EmailCheck();
});

it('should return EmailCheck is enabled for a normal user', () => {
settingsMock.returns(true);

const isEmail2FAEnabled = emailCheck.isEnabled(normalUserMock);

expect(isEmail2FAEnabled).to.be.equal(true);
});

it('should return EmailCheck is not enabled for a normal user with unverified email', () => {
settingsMock.returns(true);

const isEmail2FAEnabled = emailCheck.isEnabled(normalUserWithUnverifiedEmailMock);

expect(isEmail2FAEnabled).to.be.equal(false);
});

it('should return EmailCheck is not enabled for a OAuth user with setting being false', () => {
yash-rajpal marked this conversation as resolved.
Show resolved Hide resolved
settingsMock.returns(true);

const isEmail2FAEnabled = emailCheck.isEnabled(OAuthUserMock);

expect(isEmail2FAEnabled).to.be.equal(false);
});
});
6 changes: 5 additions & 1 deletion apps/meteor/app/2fa/server/code/EmailCheck.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { IUser } from '@rocket.chat/core-typings';
import { isOAuthUser, type IUser } from '@rocket.chat/core-typings';
import { Users } from '@rocket.chat/models';
import { Random } from '@rocket.chat/random';
import bcrypt from 'bcrypt';
Expand All @@ -24,6 +24,10 @@ export class EmailCheck implements ICodeCheck {
return false;
}

if (!settings.get('Accounts_twoFactorAuthentication_email_available_for_oAuth_users') && isOAuthUser(user)) {
return false;
yash-rajpal marked this conversation as resolved.
Show resolved Hide resolved
}

if (!user.services?.email2fa?.enabled) {
return false;
}
Expand Down
12 changes: 4 additions & 8 deletions apps/meteor/app/2fa/server/code/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,10 @@ function getAvailableMethodNames(user: IUser): string[] {
export async function getUserForCheck(userId: string): Promise<IUser | null> {
return Users.findOneById(userId, {
projection: {
'emails': 1,
'language': 1,
'createdAt': 1,
'services.totp': 1,
'services.email2fa': 1,
'services.emailCode': 1,
'services.password': 1,
'services.resume.loginTokens': 1,
emails: 1,
language: 1,
createdAt: 1,
services: 1,
},
});
}
Expand Down
14 changes: 9 additions & 5 deletions apps/meteor/app/api/server/v1/misc.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import crypto from 'crypto';

import type { IUser } from '@rocket.chat/core-typings';
import { isOAuthUser, type IUser } from '@rocket.chat/core-typings';
import { Settings, Users } from '@rocket.chat/models';
import {
isShieldSvgProps,
Expand All @@ -26,7 +26,7 @@ import { hasPermissionAsync } from '../../../authorization/server/functions/hasP
import { passwordPolicy } from '../../../lib/server';
import { notifyOnSettingChangedById } from '../../../lib/server/lib/notifyListener';
import { settings } from '../../../settings/server';
import { getDefaultUserFields } from '../../../utils/server/functions/getDefaultUserFields';
import { getBaseUserFields } from '../../../utils/server/functions/getBaseUserFields';
import { isSMTPConfigured } from '../../../utils/server/functions/isSMTPConfigured';
import { getURL } from '../../../utils/server/getURL';
import { API } from '../api';
Expand Down Expand Up @@ -176,15 +176,19 @@ API.v1.addRoute(
{ authRequired: true },
{
async get() {
const fields = getDefaultUserFields();
const { services, ...user } = (await Users.findOneById(this.userId, { projection: fields })) as IUser;
const userFields = { ...getBaseUserFields(), services: 1 };
const { services, ...user } = (await Users.findOneById(this.userId, { projection: userFields })) as IUser;

return API.v1.success(
await getUserInfo({
...user,
isOAuthUser: isOAuthUser({ ...user, services }),
yash-rajpal marked this conversation as resolved.
Show resolved Hide resolved
...(services && {
services: {
...services,
...(services.github && { github: services.github }),
...(services.gitlab && { gitlab: services.gitlab }),
...(services.email2fa?.enabled && { email2fa: { enabled: services.email2fa.enabled } }),
...(services.totp?.enabled && { totp: { enabled: services.totp.enabled } }),
password: {
// The password hash shouldn't be leaked but the client may need to know if it exists.
exists: Boolean(services?.password?.bcrypt),
Expand Down
34 changes: 34 additions & 0 deletions apps/meteor/app/utils/server/functions/getBaseUserFields.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
type UserFields = {
[k: string]: number;
};

export const getBaseUserFields = (): UserFields => ({
'name': 1,
'username': 1,
'nickname': 1,
'emails': 1,
'status': 1,
'statusDefault': 1,
'statusText': 1,
'statusConnection': 1,
'bio': 1,
'avatarOrigin': 1,
'utcOffset': 1,
'language': 1,
'settings': 1,
'enableAutoAway': 1,
'idleTimeLimit': 1,
'roles': 1,
'active': 1,
'defaultRoom': 1,
'customFields': 1,
'requirePasswordChange': 1,
'requirePasswordChangeReason': 1,
'statusLivechat': 1,
'banners': 1,
'oauth.authorizedClients': 1,
'_updatedAt': 1,
'avatarETag': 1,
'extension': 1,
'openBusinessHours': 1,
});
35 changes: 5 additions & 30 deletions apps/meteor/app/utils/server/functions/getDefaultUserFields.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,14 @@
type DefaultUserFields = {
import { getBaseUserFields } from './getBaseUserFields';

type UserFields = {
[k: string]: number;
};

export const getDefaultUserFields = (): DefaultUserFields => ({
'name': 1,
'username': 1,
'nickname': 1,
'emails': 1,
'status': 1,
'statusDefault': 1,
'statusText': 1,
'statusConnection': 1,
'bio': 1,
'avatarOrigin': 1,
'utcOffset': 1,
'language': 1,
'settings': 1,
'enableAutoAway': 1,
'idleTimeLimit': 1,
'roles': 1,
'active': 1,
'defaultRoom': 1,
'customFields': 1,
'requirePasswordChange': 1,
'requirePasswordChangeReason': 1,
export const getDefaultUserFields = (): UserFields => ({
...getBaseUserFields(),
'services.github': 1,
'services.gitlab': 1,
'services.password.bcrypt': 1,
'services.totp.enabled': 1,
'services.email2fa.enabled': 1,
'statusLivechat': 1,
'banners': 1,
'oauth.authorizedClients': 1,
'_updatedAt': 1,
'avatarETag': 1,
'extension': 1,
'openBusinessHours': 1,
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Box, Accordion, ButtonGroup, Button } from '@rocket.chat/fuselage';
import { useUniqueId } from '@rocket.chat/fuselage-hooks';
import { useSetting, useTranslation } from '@rocket.chat/ui-contexts';
import { useSetting, useTranslation, useUser } from '@rocket.chat/ui-contexts';
import type { ReactElement } from 'react';
import React from 'react';
import { FormProvider, useForm } from 'react-hook-form';
Expand All @@ -15,6 +15,11 @@ const passwordDefaultValues = { password: '', confirmationPassword: '' };

const AccountSecurityPage = (): ReactElement => {
const t = useTranslation();
const user = useUser();

const isEmail2FAAvailableForOAuth = useSetting('Accounts_twoFactorAuthentication_email_available_for_oAuth_users');
yash-rajpal marked this conversation as resolved.
Show resolved Hide resolved
const isOAuthUser = user?.isOAuthUser;
const isEmail2FAAllowed = !isOAuthUser || isEmail2FAAvailableForOAuth;

const methods = useForm({
defaultValues: passwordDefaultValues,
Expand All @@ -30,6 +35,7 @@ const AccountSecurityPage = (): ReactElement => {
const twoFactorByEmailEnabled = useSetting('Accounts_TwoFactorAuthentication_By_Email_Enabled');
const e2eEnabled = useSetting('E2E_Enable');
const allowPasswordChange = useSetting('Accounts_AllowPasswordChange');
const showEmailTwoFactor = twoFactorByEmailEnabled && isEmail2FAAllowed;

const passwordFormId = useUniqueId();

Expand All @@ -48,10 +54,10 @@ const AccountSecurityPage = (): ReactElement => {
</FormProvider>
)}
<Accordion>
{(twoFactorTOTP || twoFactorByEmailEnabled) && twoFactorEnabled && (
{(twoFactorTOTP || showEmailTwoFactor) && twoFactorEnabled && (
<Accordion.Item title={t('Two Factor Authentication')}>
{twoFactorTOTP && <TwoFactorTOTP />}
{twoFactorByEmailEnabled && <TwoFactorEmail />}
{showEmailTwoFactor && <TwoFactorEmail />}
</Accordion.Item>
)}
{e2eEnabled && (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { Box, Button, Margins } from '@rocket.chat/fuselage';
import { useUser, useTranslation } from '@rocket.chat/ui-contexts';
import type { ComponentProps, ReactElement } from 'react';
import type { ComponentProps } from 'react';
import React, { useCallback } from 'react';

import { useEndpointAction } from '../../../hooks/useEndpointAction';

const TwoFactorEmail = (props: ComponentProps<typeof Box>): ReactElement => {
const TwoFactorEmail = (props: ComponentProps<typeof Box>) => {
const t = useTranslation();
const user = useUser();

Expand Down
12 changes: 12 additions & 0 deletions apps/meteor/server/settings/accounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@ export const createAccountSettings = () =>
public: true,
});

await this.add('Accounts_twoFactorAuthentication_email_available_for_oAuth_users', true, {
yash-rajpal marked this conversation as resolved.
Show resolved Hide resolved
type: 'boolean',
enableQuery: [
enable2FA,
{
_id: 'Accounts_TwoFactorAuthentication_By_Email_Enabled',
value: true,
},
],
public: true,
});

await this.add('Accounts_TwoFactorAuthentication_By_Email_Auto_Opt_In', true, {
type: 'boolean',
enableQuery: [
Expand Down
Loading
Loading