Skip to content

fix(pro-connect): add DISABLED/OPTIONAL/REQUIRED 2FA modes - #172

Open
UnknownIQ wants to merge 1 commit into
InseeFr:masterfrom
UnknownIQ:feat/proconnect-2fa-modes
Open

fix(pro-connect): add DISABLED/OPTIONAL/REQUIRED 2FA modes#172
UnknownIQ wants to merge 1 commit into
InseeFr:masterfrom
UnknownIQ:feat/proconnect-2fa-modes

Conversation

@UnknownIQ

Copy link
Copy Markdown

Fixes #171

Problem

The Double authentification (2FA) toggle added in #169 is all-or-nothing: enabling it makes the authorization request use claims with "essential": true, and validateAcrClaim only 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 with IdentityBrokerException("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_enabled with a 3-way mfa_mode:

  • DISABLED (default) — same behavior as before this feature existed: acr_values is set from the configured eIDAS level, no claims parameter. 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 (claims with "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: claims with "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=true value is read as REQUIRED so existing realms keep their current behavior after upgrading; mfa_enabled (unset or false) reads as DISABLED.

Testing

  • ./mvnw test — added coverage for all three modes (authorization URL construction incl. the essential flag, ACR validation/acceptance/rejection per mode) and for the legacy-property migration.
  • Ran the built jar against a live Keycloak deployment for ~40h with OPTIONAL mode enabled: confirmed no MFA-related rejections and that non-MFA logins keep succeeding. (Log review also turned up an unrelated pre-existing issue — acr=eidas0 isn't recognized by EidasLevel at all — which we're filing separately, since it happens the same way regardless of this fix.)

Compatibility

DISABLED/OPTIONAL/REQUIRED are stored as the new mfa_mode property; the old mfa_enabled boolean is still read as a fallback for realms that already set it, so no manual migration is required.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ProConnect 2FA toggle blocks logins for users who don't complete MFA

1 participant