-
Notifications
You must be signed in to change notification settings - Fork 13.8k
feat: Phishing resistant MFA #40721
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
Merged
Merged
feat: Phishing resistant MFA #40721
Changes from all commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
05b51b5
Revert "revert: Phishing resistant multi factor authentication (#40679)"
yash-rajpal 8f3741d
chore: Hide login buttons on mobile app (#40733)
yash-rajpal e80729e
feat: OAuth login using login codes (#40783)
yash-rajpal 9933083
regression: fork `passport-twitter` (#40647)
tassoevan 7160ce6
feat: `Accounts_OAuth_Use_Modern_Flow` Setting (#40876)
yash-rajpal 026ca89
fix condition
yash-rajpal 8cece39
fix: Popup style OAuth login for passport (#40882)
yash-rajpal df18e1b
fix: Passport OAuth accessible by URL even after disabled by settings
yash-rajpal 09a531b
fix: Iframe support for passport OAuth (#40949)
yash-rajpal 8cdc009
fix: Wordpress OAuth
yash-rajpal 1a4748e
fix: missing apple oauth clientId crashes server
yash-rajpal aa2810a
fix: github_enterprise edge case
yash-rajpal 4cd9910
chore: remove irrelevant code
yash-rajpal 86b93a6
fix: Gitlab OAuth config
yash-rajpal 6d06a1e
chore: move apple passport implementation
yash-rajpal 38afbdb
merge develop and fix conflicts
yash-rajpal 4228432
chore: toggle apple for passport and meteor routes
yash-rajpal 9e919fc
fix: trust just 1 previous reverse proxy
yash-rajpal 0a8058b
add changeset
yash-rajpal f0fc735
error handling
yash-rajpal 3b69175
fix TS
yash-rajpal e939406
Merge branch 'develop' into feat/phishing-resistant-mfa
yash-rajpal 39aaaf0
fix wordpress config
yash-rajpal 1e70d99
fix meteor OAuth service key
yash-rajpal f2bcd12
fix trailing slash in site url
yash-rajpal b7b0e3c
fix: watch for github_enterprise url setting change
yash-rajpal b23c8d0
chore: default wordpress paths config
yash-rajpal 05e3602
fix settings for e2e tests
yash-rajpal 2f9c086
refactor: prevent usage of global `Meteor` reference
tassoevan 48b08c3
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into feat…
tassoevan 3280137
use a map
KevLehman 587c37d
exclude rooms prop
KevLehman 2abde09
oauth
KevLehman 4970930
issuer & exp
KevLehman 2de57f0
avoid overriding email from spread
KevLehman d1f90ce
cubic
KevLehman bdd3f73
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
cardoso c0c9f4b
fix: missing validation & remove dead/duplicate code
cardoso 0809a18
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
cardoso d84da30
fix: passport-apple name acquisition
cardoso 6a88fa1
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
cardoso 50f6c69
fix: Accounts_OAuth_Use_Modern_Flow default
cardoso 6038d1d
merge and fix conflicts
yash-rajpal fc65e74
merge conflicts
yash-rajpal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| --- | ||
| '@rocket.chat/web-ui-registration': major | ||
| '@rocket.chat/model-typings': major | ||
| '@rocket.chat/core-typings': major | ||
| '@rocket.chat/rest-typings': major | ||
| '@rocket.chat/passport-x': major | ||
| '@rocket.chat/models': major | ||
| '@rocket.chat/i18n': major | ||
| '@rocket.chat/meteor': major | ||
| --- | ||
|
|
||
| ## Phishing-Resistant Multi-Factor Authentication | ||
|
|
||
| Introduces a more secure and reliable server-side OAuth authentication flow. | ||
|
|
||
| ### What’s New | ||
|
|
||
| - **Improved OAuth login security** | ||
| OAuth authentication now happens fully on the server, reducing the risk of token theft, phishing attacks, and client-side credential interception. | ||
|
|
||
| - **Built-in CSRF, state validation, and PKCE protection** | ||
| OAuth logins now include stronger protection against CSRF attacks, request tampering, and authorization code interception through secure state validation and PKCE support. | ||
|
|
||
| - **Improved two-step verification with OAuth logins** | ||
| Users with email or TOTP two-factor authentication enabled will now be asked to complete 2FA even when signing in with providers like Google, GitHub, GitLab, and others. | ||
|
|
||
| - **Improved mobile & desktop app login** | ||
| Mobile and desktop apps now support a smoother and more secure deep-link OAuth login flow. | ||
|
|
||
| - **A new setting to enable/disable new OAuth Flow** | ||
| Enable this new setting `Accounts_OAuth_Use_Modern_Flow` to use all of the above mentioned features. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --- | ||
| '@rocket.chat/web-ui-registration': minor | ||
| '@rocket.chat/model-typings': minor | ||
| '@rocket.chat/core-typings': minor | ||
| '@rocket.chat/rest-typings': minor | ||
| '@rocket.chat/desktop-api': minor | ||
| '@rocket.chat/models': minor | ||
| '@rocket.chat/i18n': minor | ||
| '@rocket.chat/meteor': minor | ||
| --- | ||
|
|
||
| ## Phishing-Resistant Multi-Factor Authentication | ||
|
|
||
| Introduces a more secure and reliable server-side OAuth authentication flow. | ||
|
|
||
| ### What’s New | ||
|
|
||
| - **Improved OAuth login security** | ||
| OAuth authentication now happens fully on the server, reducing the risk of token theft, phishing attacks, and client-side credential interception. | ||
|
|
||
| - **Built-in CSRF, state validation, and PKCE protection** | ||
| OAuth logins now include stronger protection against CSRF attacks, request tampering, and authorization code interception through secure state validation and PKCE support. | ||
|
|
||
| - **Improved two-step verification with OAuth logins** | ||
| Users with email or TOTP two-factor authentication enabled will now be asked to complete 2FA even when signing in with providers like Google, GitHub, GitLab, and others. | ||
|
|
||
| - **Improved mobile & desktop app login** | ||
| Mobile and desktop apps now support a smoother and more secure deep-link OAuth login flow. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import type { IUser } from '@rocket.chat/core-typings'; | ||
| import { TwoFactorChallenges } from '@rocket.chat/models'; | ||
| import { Meteor } from 'meteor/meteor'; | ||
|
|
||
| import { EmailCheck } from './EmailCheck'; | ||
|
|
||
| export class EmailCheckForOAuth extends EmailCheck { | ||
| public override readonly name = 'email-oauth'; | ||
|
|
||
| public readonly method = 'email'; | ||
|
|
||
| public async sendTwoFactorChallenge(user: IUser): Promise<string> { | ||
| const challengeId = await TwoFactorChallenges.createTwoFactorChallenge(user._id, 'email'); | ||
| await this.sendEmailCode(user); | ||
| return challengeId; | ||
| } | ||
|
|
||
| public async verifyEmailTwoFactorChallenge(user: IUser, challengeId: string, code: string): Promise<boolean> { | ||
|
yash-rajpal marked this conversation as resolved.
|
||
| const challenge = await TwoFactorChallenges.findOneByPendingChallengeId(challengeId); | ||
| if (!challenge) { | ||
| return false; | ||
| } | ||
|
|
||
| if (challenge.expireAt && challenge.expireAt < new Date()) { | ||
| throw new Meteor.Error('error-challenge-expired', 'challenge expired'); | ||
| } | ||
|
|
||
| const isCodeValid = await this.verify(user, code); | ||
|
|
||
| if (!isCodeValid) { | ||
| return false; | ||
| } | ||
|
|
||
| await TwoFactorChallenges.removeByPendingChallengeId(challengeId); | ||
|
yash-rajpal marked this conversation as resolved.
|
||
|
|
||
| return true; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import type { IUser } from '@rocket.chat/core-typings'; | ||
| import { TwoFactorChallenges } from '@rocket.chat/models'; | ||
| import { Meteor } from 'meteor/meteor'; | ||
|
|
||
| import { TOTPCheck } from './TOTPCheck'; | ||
|
|
||
| export class TOTPCheckForOAuth extends TOTPCheck { | ||
| public override readonly name = 'totp-oauth'; | ||
|
|
||
| public readonly method = 'totp'; | ||
|
|
||
| public async sendTwoFactorChallenge(user: IUser): Promise<string> { | ||
| return TwoFactorChallenges.createTwoFactorChallenge(user._id, 'totp'); | ||
| } | ||
|
|
||
| public async verifyEmailTwoFactorChallenge(user: IUser, challengeId: string, code: string): Promise<boolean> { | ||
| const challenge = await TwoFactorChallenges.findOneByPendingChallengeId(challengeId); | ||
| if (!challenge) { | ||
| return false; | ||
| } | ||
|
|
||
| if (challenge.expireAt && challenge.expireAt < new Date()) { | ||
| throw new Meteor.Error('error-challenge-expired', 'challenge expired'); | ||
| } | ||
|
|
||
| const isCodeValid = await this.verify(user, code); | ||
|
|
||
| if (!isCodeValid) { | ||
| return false; | ||
| } | ||
|
|
||
| await TwoFactorChallenges.removeByPendingChallengeId(challengeId); | ||
|
yash-rajpal marked this conversation as resolved.
|
||
|
|
||
| return true; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| import { MeteorError } from '@rocket.chat/core-services'; | ||
| import { Users } from '@rocket.chat/models'; | ||
| import express from 'express'; | ||
| import { Accounts } from 'meteor/accounts-base'; | ||
| import { ServiceConfiguration } from 'meteor/service-configuration'; | ||
| import passport from 'passport'; | ||
| import { Strategy as AppleStrategy } from 'passport-apple'; | ||
| import type { Profile } from 'passport-apple'; | ||
|
|
||
| import { AppleCustomOAuth } from './AppleCustomOAuth'; | ||
| import { oAuthRouter } from '../../../server/configuration/configurePassport'; | ||
| import { allowPassportOAuthMiddleware } from '../../../server/lib/oauth/allowPassportOAuthMiddleware'; | ||
| import { passportOAuthCallback } from '../../../server/lib/oauth/passportOAuthCallback'; | ||
| import { settings } from '../../settings/server'; | ||
| import { config } from '../lib/config'; | ||
| import { handleIdentityToken } from '../lib/handleIdentityToken'; | ||
|
|
||
| new AppleCustomOAuth('apple', config); | ||
|
|
||
| type RequestWithAppleProfile = { | ||
| appleProfile?: { | ||
| name?: { | ||
| firstName?: string; | ||
| middleName?: string; | ||
| lastName?: string; | ||
| }; | ||
| }; | ||
| }; | ||
|
|
||
| function isRequestWithAppleProfile(req: object): req is RequestWithAppleProfile { | ||
| if ( | ||
| 'appleProfile' in req && | ||
| req.appleProfile && | ||
| typeof req.appleProfile === 'object' && | ||
| 'name' in req.appleProfile && | ||
| req.appleProfile.name && | ||
| typeof req.appleProfile.name === 'object' | ||
| ) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| settings.watchMultiple( | ||
| [ | ||
| 'Accounts_OAuth_Apple', | ||
| 'Accounts_OAuth_Apple_id', | ||
| 'Accounts_OAuth_Apple_secretKey', | ||
| 'Accounts_OAuth_Apple_iss', | ||
| 'Accounts_OAuth_Apple_kid', | ||
| 'Accounts_OAuth_Use_Modern_Flow', | ||
| ], | ||
| async ([enabled, clientId, serverSecret, iss, kid, useModernFlow]) => { | ||
| passport.unuse('apple'); | ||
|
|
||
| if (!useModernFlow) { | ||
| return; | ||
| } | ||
|
|
||
| if (!enabled) { | ||
| return ServiceConfiguration.configurations.removeAsync({ | ||
| service: 'apple', | ||
| }); | ||
| } | ||
|
|
||
| // if everything is empty but Apple login is enabled, don't show the login button | ||
| if (!clientId && !serverSecret && !iss && !kid) { | ||
| await ServiceConfiguration.configurations.upsertAsync( | ||
| { | ||
| service: 'apple', | ||
| }, | ||
| { | ||
| $set: { | ||
| showButton: false, | ||
| enabled: settings.get('Accounts_OAuth_Apple'), | ||
| }, | ||
| }, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (typeof clientId !== 'string' || !clientId) { | ||
| return ServiceConfiguration.configurations.removeAsync({ | ||
| service: 'apple', | ||
| }); | ||
| } | ||
|
|
||
| passport.use( | ||
| 'apple', | ||
|
|
||
| new AppleStrategy( | ||
| { | ||
| clientID: clientId, | ||
| teamID: settings.get<string>('Accounts_OAuth_Apple_iss'), | ||
| keyID: settings.get<string>('Accounts_OAuth_Apple_kid'), | ||
| privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey').replace(/\\n/g, '\n'), | ||
| callbackURL: `${settings.get<string>('Site_Url').replace(/\/$/, '')}/_oauth/apple`, | ||
| scope: ['name', 'email'], | ||
| passReqToCallback: true, | ||
| state: false, | ||
|
yash-rajpal marked this conversation as resolved.
|
||
| }, | ||
|
yash-rajpal marked this conversation as resolved.
|
||
| async (req, accessToken: string, refreshToken: string, idToken: string, _profile: Profile, done) => { | ||
| try { | ||
| const serviceData = await handleIdentityToken(idToken, clientId); | ||
|
|
||
| if (isRequestWithAppleProfile(req)) { | ||
| const appleName = req.appleProfile?.name; | ||
| serviceData.name = [appleName?.firstName, appleName?.middleName, appleName?.lastName].filter(Boolean).join(' '); | ||
| } | ||
|
|
||
| const user = await Accounts.updateOrCreateUserFromExternalService( | ||
| 'apple', | ||
| { | ||
| accessToken, | ||
| refreshToken, | ||
| ...serviceData, | ||
| }, | ||
| {}, | ||
| ); | ||
|
|
||
| if (!user?.userId || typeof user?.userId !== 'string') { | ||
| return done(new Error('User not found')); | ||
| } | ||
|
|
||
| const userFromDB = await Users.findOneById(user.userId); | ||
|
|
||
| if (!userFromDB) { | ||
| return done(new Error('User not found')); | ||
| } | ||
|
|
||
| return done(null, userFromDB); | ||
| } catch (error: any) { | ||
| done(error); | ||
| return { | ||
| type: 'apple', | ||
| error: new MeteorError(Accounts.LoginCancelledError.numericError, error.message), | ||
| }; | ||
| } | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| const callbackHandler = [ | ||
| allowPassportOAuthMiddleware('apple'), | ||
| express.urlencoded({ extended: true }), | ||
| passport.authenticate('apple', { failWithError: true, session: true, keepSessionInfo: true }), | ||
| passportOAuthCallback(settings.get<string>('Site_Url').replace(/\/$/, '')), | ||
| ]; | ||
|
|
||
| oAuthRouter.get( | ||
| '/oauth/apple', | ||
| allowPassportOAuthMiddleware('apple'), | ||
| (req, _res, next) => { | ||
| const { loginClient } = req.query; | ||
| if (loginClient === 'mobile' || loginClient === 'desktop') { | ||
| req.session.loginClient = loginClient; | ||
| req.session.save(() => { | ||
| next(); | ||
| }); | ||
| } else { | ||
| //delete stale value from previous sessions if any | ||
| delete req.session.loginClient; | ||
| next(); | ||
| } | ||
| }, | ||
| passport.authenticate('apple', { | ||
| scope: ['name', 'email'], | ||
| }), | ||
| ); | ||
|
|
||
| oAuthRouter | ||
| .route('/_oauth/apple') | ||
| .post(...callbackHandler) | ||
| .get(...callbackHandler); | ||
| }, | ||
|
yash-rajpal marked this conversation as resolved.
|
||
| ); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| import './appleOauthRegisterService'; | ||
| import './loginHandler'; | ||
| import './applePassportOAuth'; |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.