KEYCLOAK-16134 Allow webauthn idless login flow by vanrar68 · Pull Request #7860 · keycloak/keycloak

vanrar68 · 2021-03-16T19:11:38Z

Rework of a previously closed PR #7564 with changes requested by @mposolda
Prior to this PR WebAuthN authenticators (regular and passwordless) required a user to be set before asking for a webauthn device.
This PR removes the need for a user to be previously set when a webauthn credential containing a user id is submitted.

vanrar68 · 2021-03-17T17:12:57Z

Added some modifications of the authenticator setup code in DefaultAuthenticationFlow.java.
Previously authenticator setup was only possible for authenticators that required users to be set (authenticator.requiresUser() == true). Now we also need to allow authenticator setup for authenticators that don't always require the user to be set.

mposolda · 2021-11-16T12:48:47Z

@vanrar68 Sorry for late response. To me, the changes looks good.

TBH I would prefer to let them in once we have WebAuthn automated tests in place to make sure that our currently supported WebAuthn use-cases (with WebAuthn as 2nd-factor or passwordless) are not somehow broken by this change. We have PR for WebAuthn automated testing already in progress and hopefully will be added soon #8534 .

@tnorimat Does this change looks good to you or do you see any further issues with it?

mposolda · 2021-11-16T12:56:03Z

@vanrar68 It will be also good to have some documentation for this. If you have an example how to integrate Keycloak with WebAuthn idless login, it will be nice to be added to the WebAuthn section of Keycloak documentation. Ideally details like:

How to configure Keycloak authentication flow and WebAuthn policies together with some info etc (Ideally with the usage of WebAuthn passwordless policy)
Which WebAuthn key you used for testing for idless (version, setup of the key if applicable etc).

tnorimat · 2021-11-16T20:50:02Z

Hello @vanrar68 , @mposolda
I will review this PR today.

tnorimat · 2021-11-17T01:36:08Z

It seems that L435 and L436 changes the original codes' logic for condition of throwing AuthenticationFlowException in L442.

To keep the same condition for throwing it in the original codes, how about the following?

if (authUser != null && !authenticator.configuredFor(processor.getSession(), processor.getRealm(), authUser)) { if (factory.isUserSetupAllowed() && model.isRequired() && authenticator.areRequiredActionsEnabled(processor.getSession(), processor.getRealm())) {

If you intentionally changed this condition, could you tell me this reason?

The purpose of the modification is to handle credential setup for authenticators that don't require a user to be set. Your proposition looks good to me, I'll update the code accordingly in order to stay the closest possible to the existing code (though I think we should get rid of the meaningless comment line)

@vanrar68 Thank you. I've checked your update.

tnorimat

@vanrar68 I've added some review comments. Could you check them?

tnorimat · 2021-11-17T01:37:04Z

processor.getAuthenticationSession().getAuthenticatedUser() can be replaced with authUser .

Yep, will do

@vanrar68 Thank you. I've checked your update.

tnorimat · 2021-11-17T01:39:05Z

@vanrar68 As @mposolda has mentioned, the arquillian integration test for this ID-less WebAuthn login flow is needed.

vanrar68 · 2021-11-17T11:10:44Z

@vanrar68 As @mposolda has mentioned, the arquillian integration test for this ID-less WebAuthn login flow is needed.

Yes I agree, but as mentioned by @mposolda there seem to be some major work going on right now on webauthn testing framework, see #8534. I'd rather wait until this work is merged and implement the loginless test on top of that even if this means waiting some more to have loginless support in Keycloak. What do you think ?

vanrar68 · 2021-11-17T11:12:58Z

@vanrar68 It will be also good to have some documentation for this. If you have an example how to integrate Keycloak with WebAuthn idless login, it will be nice to be added to the WebAuthn section of Keycloak documentation. Ideally details like:

How to configure Keycloak authentication flow and WebAuthn policies together with some info etc (Ideally with the usage of WebAuthn passwordless policy)

Which WebAuthn key you used for testing for idless (version, setup of the key if applicable etc).

Yes, I'll create a PR for keycloak-documentation regarding the configuration of webauthn idless auth

tnorimat · 2021-11-17T19:16:18Z

@vanrar68

#7860 (comment)

Yes, I agree with you.

vanrar68 · 2021-11-17T22:03:59Z

@vanrar68 It will be also good to have some documentation for this. If you have an example how to integrate Keycloak with WebAuthn idless login, it will be nice to be added to the WebAuthn section of Keycloak documentation. Ideally details like:

How to configure Keycloak authentication flow and WebAuthn policies together with some info etc (Ideally with the usage of WebAuthn passwordless policy)

Which WebAuthn key you used for testing for idless (version, setup of the key if applicable etc).

@mposolda Does this need a JIRA/Github Issue to be opened on the keycloak-documentation project or is a PR enough ?

mposolda · 2021-11-18T16:19:58Z

@vanrar68 As @mposolda has mentioned, the arquillian integration test for this ID-less WebAuthn login flow is needed.

Yes I agree, but as mentioned by @mposolda there seem to be some major work going on right now on webauthn testing framework, see #8534. I'd rather wait until this work is merged and implement the loginless test on top of that even if this means waiting some more to have loginless support in Keycloak. What do you think ?

+1 from me as well

mposolda · 2021-11-18T16:20:50Z

@vanrar68 It will be also good to have some documentation for this. If you have an example how to integrate Keycloak with WebAuthn idless login, it will be nice to be added to the WebAuthn section of Keycloak documentation. Ideally details like:

How to configure Keycloak authentication flow and WebAuthn policies together with some info etc (Ideally with the usage of WebAuthn passwordless policy)

Which WebAuthn key you used for testing for idless (version, setup of the key if applicable etc).

@mposolda Does this need a JIRA/Github Issue to be opened on the keycloak-documentation project or is a PR enough ?

PR is enough. You can use the same JIRA in this PR to keycloak-documentation as you used in this PR.

mposolda · 2021-11-18T16:21:55Z

@vanrar68 @tnorimat Thanks for the review Takashi and Thanks Joaquim for incorporating Takashi's feedback!

However I see there are failing tests and looks it may be regression of this PR. This may need to be investigated. I guess that is the consequence of the changes in the DefaultAuthenticationFlow. If it is possible to address this PR without changes in DefaultAuthenticationFlow, it will be great (Not 100% sure it is possible).

vanrar68 · 2021-11-18T16:49:45Z

@mposolda, @tnorimat
There's something else that would be interesting to have in Keycloak regarding loginless auth: the possibility for the user to select the auth method he wants to use before having to enter a username.
Currently, my understanding is that if the auth flow is configured with alternative "executions" AND the user has already submitted his username then Keycloak allows the user to select which of the alternative methods he wants to use (with the "Try another way" link) because the code in DefaultAuthenticationFlow.java builds a list of the available credentials for this user and matches these credentials with the authentication flow configuration.
With loginless auth it would be interesting to give the user the possibility to change the method of authentication ("Try another way") without having to enter a username. This way a user could choose between a Username/Password form or a WebAuthn Loginless auth. What do you think ?

vanrar68 · 2021-11-18T16:53:22Z

@vanrar68 @tnorimat Thanks for the review Takashi and Thanks Joaquim for incorporating Takashi's feedback!

However I see there are failing tests and looks it may be regression of this PR. This may need to be investigated. I guess that is the consequence of the changes in the DefaultAuthenticationFlow. If it is possible to address this PR without changes in DefaultAuthenticationFlow, it will be great (Not 100% sure it is possible).

I'll have a look

vanrar68 · 2021-11-18T19:37:10Z

@vanrar68 @tnorimat Thanks for the review Takashi and Thanks Joaquim for incorporating Takashi's feedback!

However I see there are failing tests and looks it may be regression of this PR. This may need to be investigated. I guess that is the consequence of the changes in the DefaultAuthenticationFlow. If it is possible to address this PR without changes in DefaultAuthenticationFlow, it will be great (Not 100% sure it is possible).

LOL, actually there is a way to have webauthn loginless without touching DefaultAuthenticationFlow: PR7564
The reason behind having to touch DefaultAuthenticationFlow is because we are diverting the "user requiring" WebAuthnPasswordless credential type to support Loginless auth (by making it not require a user anymore).

I'll have a look at the failing tests anyway

mposolda · 2022-01-21T13:52:42Z

Documentation PR: keycloak/keycloak-documentation#1306

mabartos · 2022-01-27T09:51:02Z

@vanrar68 Thanks for the contribution! Before I review this PR, I'd like to discuss possible UI changes and improve usability of that. I created a discussion(#9821), so it'd be great if you'd share your thoughts or ideas.

vanrar68 · 2022-02-23T22:09:10Z

Includes an attempt at moving the "Try another way" link on the login page (before Identity Providers buttons).
@mabartos can you please have a look ?

vanrar68 · 2022-02-24T08:12:50Z

The failed tests are linked to the UI change not the idless feature itself

mabartos · 2022-02-24T11:51:17Z

@vanrar68 Thanks for the update. I'll check it.

vanrar68 · 2022-02-24T13:53:38Z

This is my best (and last) shot at the UI:

vanrar68 · 2022-03-02T12:24:50Z

@mabartos did you get a chance to review to UI part of this PR ?

mabartos

@vanrar68 Great work! In general, LGTM.

Just one nitpick with the parameter in the template.ftl file, which could be solved (see below). The design changes look good. The test itself looks good as well, but it needs some polishing, regarding new changes in the WebAuthn testsuite, in order to make it stable for a long time, but IMO, it might be solved in some follow-up task, where I can solve it. @mposolda WDYT?

I executed the pipeline tests for WebAuthn and they passed.
See https://keycloak-jenkins.com/job/universal-test-pipeline-server/1290/

(We need to focus only on the results for the specific test and browser Chrome... I verify the other failures are expected for this PR.)

@vanrar68 Thanks!

mabartos · 2022-03-03T09:11:43Z

I was wondering if it's a proper solution to obtain sessionFactory in each iteration as the session (AFAIK) should be the same for the whole method. However, as one of the main purposes of the AuthenticationProcessor class is to manage the state of the authentication process and there are no other calls to dynamically changing objects, I don't consider this as a blocker for this PR. Just wanted to take it into consideration.

@mabartos Something like this ? :

KeycloakSessionFactory sessionFactory = processor.getSession().getKeycloakSessionFactory(); typeAuthExecMap.forEach((key, value) -> { AuthenticatorFactory credbasedAuthenticatorFactory = (AuthenticatorFactory) sessionFactory.getProviderFactory(Authenticator.class, value.getAuthenticator()); Authenticator credbasedAuthenticator = credbasedAuthenticatorFactory.create(processor.getSession()); if (!credbasedAuthenticator.requiresUser()) { userlessCredBasedAuthenticationSelectionList.add(new AuthenticationSelectionOption(processor.getSession(), value)); } });

Yep, I have sth like that in my mind, or maybe for the whole method. But I think you can keep it there as is. It might not have any performance issues.

vanrar68 · 2022-03-03T10:19:42Z

@mabartos just rebased to latest main and removed the showAnotherWayIfPresent from template.ftl as requested

mabartos

@vanrar68 Thanks! LGTM

@tnorimat It'd be wonderful if you'd have a chance to review it as well?

And we need to wait for @mposolda approval.

tnorimat · 2022-03-03T12:17:22Z

@vanrar68 @mabartos Yes, I would like to review this PR.

tnorimat

@vanrar68 I've added one tiny comment. Could you check it?

vanrar68 · 2022-03-09T17:28:56Z

@mposolda did you get a chance to review this PR ?

mposolda

@vanrar68 Did the review and testing and it looks good to me. Thanks for all your work on this!

mposolda · 2022-03-21T10:31:22Z

@mabartos @tnorimat Thanks for your review and feedback!

vanrar68 mentioned this pull request Mar 16, 2021

KEYCLOAK-16134 Allow webauthn idless login flow #7564

Closed

abstractj added area/authentication Indicates an issue on Authentication area kind/feature Categorizes a PR related to a new feature labels Mar 17, 2021

vanrar68 force-pushed the webauthn-idless branch from 18d20b6 to 25f32a2 Compare March 17, 2021 16:59

vanrar68 force-pushed the webauthn-idless branch from 25f32a2 to 326f22b Compare March 17, 2021 20:01

vanrar68 force-pushed the webauthn-idless branch from 326f22b to 287e3ef Compare October 26, 2021 12:34

mposolda self-assigned this Nov 16, 2021

mposolda requested a review from tnorimat November 16, 2021 12:48

mposolda added the missing/docs label Nov 16, 2021

vanrar68 force-pushed the webauthn-idless branch from 287e3ef to ba326e3 Compare November 16, 2021 15:35

tnorimat reviewed Nov 17, 2021

View reviewed changes

vanrar68 force-pushed the webauthn-idless branch 2 times, most recently from a56a316 to 6da8bfe Compare November 17, 2021 16:57

vanrar68 mentioned this pull request Nov 18, 2021

Add webauthn loginless documentation keycloak/keycloak-documentation#1306

Merged

mabartos removed the area/authentication Indicates an issue on Authentication area label Jan 20, 2022

mposolda added area/authentication Indicates an issue on Authentication area area/docs and removed missing/docs area/authentication Indicates an issue on Authentication area labels Jan 21, 2022

vanrar68 force-pushed the webauthn-idless branch from 440968d to 53e12c4 Compare February 23, 2022 22:01

vanrar68 force-pushed the webauthn-idless branch from 53e12c4 to 1b91b42 Compare February 24, 2022 13:37

mabartos reviewed Mar 3, 2022

View reviewed changes

vanrar68 force-pushed the webauthn-idless branch from 1b91b42 to 3ce59cc Compare March 3, 2022 10:03

mabartos previously approved these changes Mar 3, 2022

View reviewed changes

tnorimat reviewed Mar 4, 2022

View reviewed changes

Comment thread ...n/tests/other/webauthn/src/test/java/org/keycloak/testsuite/webauthn/WebAuthnIdlessTest.java Outdated

KEYCLOAK-16134 Allow webauthn idless login flow

9db55f2

vanrar68 dismissed mabartos’s stale review via 9db55f2 March 4, 2022 10:28

vanrar68 force-pushed the webauthn-idless branch from 3ce59cc to 9db55f2 Compare March 4, 2022 10:28

mabartos approved these changes Mar 4, 2022

View reviewed changes

mposolda approved these changes Mar 21, 2022

View reviewed changes

mposolda mentioned this pull request Mar 21, 2022

Allow WebAuthn idless login flow #10832

Closed

mposolda linked an issue Mar 21, 2022 that may be closed by this pull request

Allow WebAuthn idless login flow #10832

Closed

mposolda merged commit 92c4e6d into keycloak:main Mar 21, 2022

Conversation

vanrar68 commented Mar 16, 2021

Uh oh!

vanrar68 commented Mar 17, 2021

Uh oh!

mposolda commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mposolda commented Nov 16, 2021

Uh oh!

tnorimat commented Nov 16, 2021

Uh oh!

tnorimat Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnorimat commented Nov 17, 2021

Uh oh!

vanrar68 commented Nov 17, 2021

Uh oh!

vanrar68 commented Nov 17, 2021

Uh oh!

tnorimat commented Nov 17, 2021

Uh oh!

vanrar68 commented Nov 17, 2021

Uh oh!

mposolda commented Nov 18, 2021

Uh oh!

mposolda commented Nov 18, 2021

Uh oh!

mposolda commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanrar68 commented Nov 18, 2021

Uh oh!

vanrar68 commented Nov 18, 2021

Uh oh!

vanrar68 commented Nov 18, 2021

Uh oh!

mposolda commented Jan 21, 2022

Uh oh!

mabartos commented Jan 27, 2022

Uh oh!

vanrar68 commented Feb 23, 2022

Uh oh!

vanrar68 commented Feb 24, 2022

Uh oh!

mabartos commented Feb 24, 2022

Uh oh!

vanrar68 commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanrar68 commented Mar 2, 2022

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanrar68 commented Mar 3, 2022

Uh oh!

mabartos left a comment

mposolda commented Nov 16, 2021 •

edited

Loading

tnorimat Nov 17, 2021 •

edited

Loading

mposolda commented Nov 18, 2021 •

edited

Loading

vanrar68 commented Feb 24, 2022 •

edited

Loading

mposolda left a comment •

edited

Loading