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

Add ability to force MFA on OIDC Logins #971

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions app/client/ui/errorPages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export function createErrPage(appModel: AppModel) {
errPage === 'not-found' ? createNotFoundPage(appModel, errMessage) :
errPage === 'access-denied' ? createForbiddenPage(appModel, errMessage) :
errPage === 'account-deleted' ? createAccountDeletedPage(appModel) :
errPage === 'mfa-not-enabled' ? createMfaNotEnabledErrorPage(appModel) :
createOtherErrorPage(appModel, errMessage);
}

Expand Down Expand Up @@ -81,6 +82,32 @@ export function createAccountDeletedPage(appModel: AppModel) {
]);
}

/**
* Creates a page that show the user's account does not have multifactor authentication enabled, despite being needed.
*/
export function createMfaNotEnabledErrorPage(appModel: AppModel) {
document.title = t("Multi-factor authentication required{{suffix}}", {
suffix: getPageTitleSuffix(getGristConfig())
});

const searchParams = new URL(location.href).searchParams;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking: you can otherwise use the URLSearchParams class:

Suggested change
const searchParams = new URL(location.href).searchParams;
const searchParams = new URLSearchParams(location.search);


return pagePanelsError(appModel, t("Multi-factor authentication required{{suffix}}", {suffix: ''}), [
cssErrorText(t("Multi-factor-authentication is required for accessing this site, but it is not set up on your \
account. Please enable it and try again.")),
Comment on lines +96 to +97
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest avoiding the \ at the end of a string, it is not standard JS (or is it required for the translation key collection?).

Suggested change
cssErrorText(t("Multi-factor-authentication is required for accessing this site, but it is not set up on your \
account. Please enable it and try again.")),
cssErrorText(t("Multi-factor-authentication is required for accessing this site, but it is not set up on your " +
"account. Please enable it and try again.")),

Copy link
Author

Choose a reason for hiding this comment

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

The plus should work as well, you're right. I'll test it later this week.

cssButtonWrap(bigPrimaryButtonLink(
t("Set up Multi-factor authentication"),
{href: getGristConfig().mfaSettingsUrl, target: '_blank'},
testId('error-setup-mfa')
)),
Comment on lines +98 to +102
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of showing that property only when getGristConfig().mfaSettingsUrl is filled with some value? This way, GRIST_OIDC_SP_MFA_SETTINGS_URL would be optional even when GRIST_OIDC_SP_FORCE_MFA is set.

Suggested change
cssButtonWrap(bigPrimaryButtonLink(
t("Set up Multi-factor authentication"),
{href: getGristConfig().mfaSettingsUrl, target: '_blank'},
testId('error-setup-mfa')
)),
getGristConfig().mfaSettingsUrl ? cssButtonWrap(bigPrimaryButtonLink(
t("Set up Multi-factor authentication"),
{href: getGristConfig().mfaSettingsUrl, target: '_blank'},
testId('error-setup-mfa')
)) : null,

cssButtonWrap(bigBasicButtonLink(
t("Try again"),
{href: getSignupUrl({ nextUrl: searchParams.get("next") || "" })},
testId('error-signin')
))
]);
}

/**
* Creates a "Page not found" page.
*/
Expand Down
3 changes: 3 additions & 0 deletions app/common/gristUrls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,9 @@ export interface GristLoadConfig {

// If backend has an email service for sending notifications.
notifierEnabled?: boolean;

// The URL to the external IDP, where the user can set up Multi-factor authentication
mfaSettingsUrl?: string;
}

export const Features = StringUnion(
Expand Down
5 changes: 5 additions & 0 deletions app/server/lib/FlexServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,11 @@ export class FlexServer implements GristServer {
this.app.get('/signed-out', expressWrap((req, resp) =>
this._sendAppPage(req, resp, {path: 'error.html', status: 200, config: {errPage: 'signed-out'}})));

// Add a static "mfa-not-enabled" page. This is where logout typically lands when GRIST_OIDC_SP_FORCE_MFA is true
// but the user hasn't configured multi-factor authentication.
this.app.get('/login/error/mfa-not-enabled', expressWrap((req, resp) =>
this._sendAppPage(req, resp, {path: 'error.html', status: 401, config: {errPage: 'mfa-not-enabled'}})));

const comment = await this._loginMiddleware.addEndpoints(this.app);
this.info.push(['loginMiddlewareComment', comment]);

Expand Down
34 changes: 34 additions & 0 deletions app/server/lib/OIDCConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@
* env GRIST_OIDC_SP_IGNORE_EMAIL_VERIFIED
* If set to "true", the user will be allowed to login even if the email is not verified by the IDP.
* Defaults to false.
* env GRIST_OIDC_SP_FORCE_MFA
* If set to "true", the user will be forced to have multi-factor authentication enabled. The state of MFA will
* be determined by OIDC's amr claim: It must include "mfa". Make sure that the IDP returns the amr claim
* correctly, otherwise authentication will fail.
* env GRIST_OIDC_SP_MFA_SETTINGS_URL
* This is needed when GRIST_OIDC_SP_FORCE_MFA is set to true. Enter the URL where the user will be able to
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you agree with my suggestion above, don't forget to adapt the comment here :).

* configure Multi-factor authentication on their account. This will be shown in the UI if the user does not have
* MFA enabled.
*
* This version of OIDCConfig has been tested with Keycloak OIDC IdP following the instructions
* at:
Expand Down Expand Up @@ -69,6 +77,7 @@ export class OIDCConfig {
private _endSessionEndpoint: string;
private _skipEndSessionEndpoint: boolean;
private _ignoreEmailVerified: boolean;
private _forceMfa: boolean;

public constructor() {
}
Expand Down Expand Up @@ -113,6 +122,11 @@ export class OIDCConfig {
defaultValue: false,
})!;

this._forceMfa = section.flag('forceMfa').readBool({
envVar: 'GRIST_OIDC_SP_FORCE_MFA',
defaultValue: false,
})!;

const issuer = await Issuer.discover(issuerUrl);
this._redirectUrl = new URL(CALLBACK_URL, spHost).href;
this._client = new issuer.Client({
Expand Down Expand Up @@ -159,6 +173,26 @@ export class OIDCConfig {
throw new Error(`OIDCConfig: email not verified for ${userInfo.email}`);
}

const amr = tokenSet.claims().amr;
if (this._forceMfa) {
if (!amr) {
throw new Error('OIDCConfig: could not verify mfa status due to missing amr claim.');
} else if (!amr.includes("mfa")) {
log.error(`OIDCConfig: multi-factor-authentication is not enabled for ${userInfo.email}.`);
delete mreq.session.oidc;

// Convert absolute URL into relative, since it will be prefixed further down the line
const targetURL = new URL(targetUrl as string);
let targetUrlRelative = targetURL.pathname;
if (targetURL.searchParams.toString()) {
targetUrlRelative += "?" + targetURL.searchParams.toString();
}

res.redirect(`/login/error/mfa-not-enabled?next=${targetUrlRelative}`);
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In another PR, I made something that may be of some help :

I hope my PR will be merged soon and that you could benefit from the changes.

You could still create another error page specifically for MFA.

However, I do not take advantage of the next param, and I do not see yet how to pass it using _sendAppPage. Is it something we would like to have absolutely? I tend to think it does not probably bring much discomfort (at least compared to the error itself).

}
}

const profile = this._makeUserProfileFromUserInfo(userInfo);
log.info(`OIDCConfig: got OIDC response for ${profile.email} (${profile.name}) redirecting to ${targetUrl}`);

Expand Down
2 changes: 2 additions & 0 deletions app/server/lib/sendAppPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import * as handlebars from 'handlebars';
import jsesc from 'jsesc';
import * as path from 'path';
import difference = require('lodash/difference');
import * as process from "node:process";

const translate = (req: express.Request, key: string, args?: any) => req.t(`sendAppPage.${key}`, args);

Expand Down Expand Up @@ -98,6 +99,7 @@ export function makeGristConfig(options: MakeGristConfigOptions): GristLoadConfi
canCloseAccount: isAffirmative(process.env.GRIST_ACCOUNT_CLOSE),
experimentalPlugins: isAffirmative(process.env.GRIST_EXPERIMENTAL_PLUGINS),
notifierEnabled: server?.hasNotifier(),
mfaSettingsUrl: process.env.GRIST_OIDC_SP_MFA_SETTINGS_URL,
...extra,
};
}
Expand Down
6 changes: 5 additions & 1 deletion static/locales/de.client.json
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,11 @@
"An unknown error occurred.": "Ein unbekannter Fehler ist aufgetreten.",
"Powered by": "Angetrieben durch",
"Build your own form": "Erstellen Sie Ihr eigenes Formular",
"Form not found": "Formular nicht gefunden"
"Form not found": "Formular nicht gefunden",
"Multi-factor authentication required{{suffix}}": "Multi-Faktor-Authentifizierung nötig{{suffix}}",
"Multi-factor-authentication is required for accessing this site, but it is not set up on your account. Please enable it and try again.": "Für den Zugriff auf diese Website ist Multi-Faktor-Authentifizierung erforderlich. Diese ist nicht in Ihrem Konto eingerichtet. Bitte richten Sie sie ein und versuchen Sie es erneut.",
"Set up Multi-factor authentication": "Multi-Faktor-Authentifizierung einrichten",
"Try again": "Erneut versuchen"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the localization, but could you do them through Weblate instead, please:
https://github.com/gristlabs/grist-core/blob/main/documentation/translations.md

},
"menus": {
"* Workspaces are available on team plans. ": "* Arbeitsbereiche sind in Teamplänen verfügbar. ",
Expand Down
6 changes: 5 additions & 1 deletion static/locales/en.client.json
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,11 @@
"An unknown error occurred.": "An unknown error occurred.",
"Build your own form": "Build your own form",
"Form not found": "Form not found",
"Powered by": "Powered by"
"Powered by": "Powered by",
"Multi-factor authentication required{{suffix}}": "Multi-factor authentication required{{suffix}}",
"Multi-factor-authentication is required for accessing this site, but it is not set up on your account. Please enable it and try again.": "Multi-factor-authentication is required for accessing this site, but it is not set up on your account. Please enable it and try again.",
"Set up Multi-factor authentication": "Set up Multi-factor authentication",
"Try again": "Try again"
},
"menus": {
"* Workspaces are available on team plans. ": "* Workspaces are available on team plans. ",
Expand Down