Add ability to force MFA on OIDC Logins #971
Conversation
There was a problem hiding this comment.
Thanks for this enhancement!
For what it is worth, I am a contributor but not part of Grist Labs, my review is advisory and the merge has to be done by an employee. I proposed some comments with the hope they may be useful.
I will try using deploying OIDC + MFA to test your PR.
| 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.")), |
There was a problem hiding this comment.
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?).
| 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.")), |
There was a problem hiding this comment.
The plus should work as well, you're right. I'll test it later this week.
| "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" |
There was a problem hiding this comment.
Thanks for the localization, but could you do them through Weblate instead, please:
https://github.com/gristlabs/grist-core/blob/main/documentation/translations.md
Co-authored-by: Florent <florent.git@zeteo.me>
| cssButtonWrap(bigPrimaryButtonLink( | ||
| t("Set up Multi-factor authentication"), | ||
| {href: getGristConfig().mfaSettingsUrl, target: '_blank'}, | ||
| testId('error-setup-mfa') | ||
| )), |
There was a problem hiding this comment.
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.
| 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, |
| suffix: getPageTitleSuffix(getGristConfig()) | ||
| }); | ||
|
|
||
| const searchParams = new URL(location.href).searchParams; |
There was a problem hiding this comment.
Nitpicking: you can otherwise use the URLSearchParams class:
| const searchParams = new URL(location.href).searchParams; | |
| const searchParams = new URLSearchParams(location.search); |
| } | ||
|
|
||
| res.redirect(`/login/error/mfa-not-enabled?next=${targetUrlRelative}`); | ||
| return; |
There was a problem hiding this comment.
In another PR, I made something that may be of some help :
- the changes in the backend: https://github.com/gristlabs/grist-core/pull/883/files#diff-ee4d5464063d6cfc9dc24b7b63d8858ff7b978dbe767ad2a712c93fe52f7ca1dR229-R248
- the changes in the frontend (you can see how I made the button to allow the user to login again, no need to retrieve particular data from the backend, so you would not need to pass the
nextparam): https://github.com/gristlabs/grist-core/pull/883/files#diff-6d05e89e392b7fe826b41130d739c6b32c40abe5bb59f4e31a6404bd1faba0c3R102
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).
| * 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 |
There was a problem hiding this comment.
If you agree with my suggestion above, don't forget to adapt the comment here :).
This PR implements the ability to force users logging in through OIDC to have Multi-factor authentication enabled. It does this by looking at the
amrclaim provided by the IDP. This claim must contain themfavalue. Make sure that the IDP returns the amr claim correctly, otherwise authentication will fail.Two new env vars have been added:
If set to "true", the user will be forced to have multi-factor authentication enabled.
This is needed when GRIST_OIDC_SP_FORCE_MFA is set to true. Enter the URL where the user will be able to configure Multi-factor authentication on their account. This will be shown in the UI if the user does not have MFA enabled.
How to test:
Using Keycloak, it's fairly hard to add the
amrclaim, but Zitadel returns it by default. To test, create a new account on their cloud offering, hook it up to Grist and setGRIST_OIDC_SP_FORCE_MFAtotrue.Log in without MFA enabled and get an error page.