Skip to content

Fix user getting pushed to required userVerification#107

Closed
happydude wants to merge 1 commit into
simplesamlphp:masterfrom
happydude:master
Closed

Fix user getting pushed to required userVerification#107
happydude wants to merge 1 commit into
simplesamlphp:masterfrom
happydude:master

Conversation

@happydude

Copy link
Copy Markdown
Contributor

Fix unexpected requirement of users to use more strict passwordless auth requirements

I think this would happen when the user already has a session and then the webauthn module is enabled.

@tvdijen tvdijen requested a review from restena-sw June 10, 2026 08:04
@restena-sw

Copy link
Copy Markdown
Collaborator

The change itself should really be a NOOP, as it inverts the actions on a strictly type-checked boolean.

Can you elaborate when exactly the new code would make a difference, as compared to the original code?

@happydude

Copy link
Copy Markdown
Contributor Author

@restena-sw I think what happens is if the user has an existing session, and this module gets enabled, the FIDO2PasswordlessAuthMode ends up undefined, so they get pushed to the more strict passwordless auth.

image

@restena-sw

Copy link
Copy Markdown
Collaborator

But then this is a transient issue while you are upgrading / changing config in the installation, with existing user sessions?

That would rather be bad practice then: it is prudent to remove existing user sessions if a behaviour-changing update is made to code/config.

If I were to accept the PR, the issue would not be cured; it would just be inverted: if the new configuration is meant to require UV then these pre-existing sessions would now be treated as UP-only, thereby lowering the security bar below the config requirements.

In that sense, treating undefined as the more-secure option is better than treating undefined as the less-secure option.

@restena-sw restena-sw closed this Jun 11, 2026
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.

2 participants