fix(pro-connect): add DISABLED/OPTIONAL/REQUIRED 2FA modes - #172
Open
UnknownIQ wants to merge 1 commit into
Open
fix(pro-connect): add DISABLED/OPTIONAL/REQUIRED 2FA modes#172UnknownIQ wants to merge 1 commit into
UnknownIQ wants to merge 1 commit into
Conversation
The boolean 2FA toggle from InseeFr#169 forced MFA for every ProConnect login once enabled: the authorization request always used claims with essential=true, and validateAcrClaim only accepted MFA-flavored ACR values (eidas0-mfa, eidas1-mfa, eidas2, eidas3). Any user who came back with a plain, non-MFA ACR was hard-rejected even if it already satisfied the configured eIDAS level. It also rejected MFA-flavored ACR values whenever the toggle was off, since the base eIDAS parser doesn't recognize the "-mfa" suffix. Replace the boolean with a 3-way mfa_mode: - DISABLED (default): same acr_values behavior as before this feature existed, but now also accepts an MFA-flavored ACR (once it meets the configured eIDAS floor) if a user happens to return one, instead of failing to parse it. - OPTIONAL: requests MFA as a claims preference (essential=false) and still accepts a plain eIDAS ACR as a fallback, so users without 2FA keep succeeding. - REQUIRED: unchanged hard enforcement (essential=true, non-MFA ACR rejected) for realms that need to guarantee MFA. An MFA-flavored ACR is always accepted once it meets the configured eIDAS floor, regardless of mode, since a user who did complete MFA should never be turned away. The legacy mfa_enabled=true value is read as REQUIRED so realms already using it keep their current behavior; unset/false reads as DISABLED. Signed-off-by: Quentin <lagarde.quentin@outlook.fr>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #171
Problem
The
Double authentification (2FA)toggle added in #169 is all-or-nothing: enabling it makes the authorization request useclaimswith"essential": true, andvalidateAcrClaimonly accepts MFA-flavored ACR values (eidas0-mfa,eidas1-mfa,eidas2,eidas3). Any login that comes back with a plain, non-MFA ACR is hard-rejected withIdentityBrokerException("The returned ACR value is insufficient for MFA authentication")— even if that ACR already satisfies the configured eIDAS level.So turning the toggle on to get eidas-mfa ACR values accepted also blocks every user who doesn't/can't complete MFA at ProConnect. We ran into this validating the feature against a real ProConnect environment: users without 2FA who used to log in fine at eIDAS1 started getting rejected outright once the toggle was enabled.
Fix
Replace the boolean
mfa_enabledwith a 3-waymfa_mode:DISABLED(default) — same behavior as before this feature existed:acr_valuesis set from the configured eIDAS level, noclaimsparameter. If a user still comes back with an MFA-flavored ACR (e.g. their ProConnect account has 2FA enabled independently of what we asked for), it's now accepted — when it meets the configured eIDAS floor — instead of failing with "the returned eIDAS level cannot be retrieved"; the previous code didn't recognize-mfa-suffixed ACR values at all in this mode.OPTIONAL— requests MFA as a preference (claimswith"essential": false). A user who completes MFA is accepted via the MFA ACR values; a user who doesn't still succeeds via the normal (non-MFA) eIDAS-level check. Login is never blocked solely for lacking 2FA.REQUIRED— unchanged from the current behavior:claimswith"essential": true, non-MFA ACR values are rejected. For realms that need to guarantee MFA.An MFA-flavored ACR is always accepted once it meets the configured eIDAS floor, in every mode — a user who did complete MFA should never be turned away regardless of what the realm requested.
The legacy
mfa_enabled=truevalue is read asREQUIREDso existing realms keep their current behavior after upgrading;mfa_enabled(unset orfalse) reads asDISABLED.Testing
./mvnw test— added coverage for all three modes (authorization URL construction incl. theessentialflag, ACR validation/acceptance/rejection per mode) and for the legacy-property migration.OPTIONALmode enabled: confirmed no MFA-related rejections and that non-MFA logins keep succeeding. (Log review also turned up an unrelated pre-existing issue —acr=eidas0isn't recognized byEidasLevelat all — which we're filing separately, since it happens the same way regardless of this fix.)Compatibility
DISABLED/OPTIONAL/REQUIREDare stored as the newmfa_modeproperty; the oldmfa_enabledboolean is still read as a fallback for realms that already set it, so no manual migration is required.