fix: validate not empty username or email

[pdt-ayidi]

Previously if a username (non-empty) is not found it would throw an error in a different page. Now it provides the error message in the input field. This PR improves the validation logic, loosely based on: ValidateUsername.java#L51.

Before:

After:

[sventorben]

Hello @pdt-ayidi,

thanks for your PR and your effort in improving this extension. I understand your use case, but seeing a few issues with your approach.

Let's say, we have an IdP configured with an email domain example.com. If you try to login with a not yet existing user [email protected], your implementation will not find the user locally. Your implementation will present the user with an error message as shown in your second screenshot. But that is not the desired behaviour, because we want Keycloak to redirect to the IdP, because it has a matching email domain.

The current implementation shows the error message in a different page as shown in your first screen. This a kind of a feature. Keycloak shows this page, if no authenticator could authenticate the user. Your implementation will present the error message near the input field. However, this prevents an automatic fall-through to an alternative authenticator. Consider the following scenario where Keycloak should redirect to a default IdP if the user cannot be found and the email domain does not match any IdP.

So, one would need to check whether there are any further alternative authenticators configured. If so, the authenticator needs to fall through. If not, it may challenge the user as with your approach. This would need to work all kind of deeply nested sub-flows. Due to the complexity this brings, I would rather not want to implement it.

Would you mind to show me the flow that you are currently using? I would like to understand a bit more how this extension is used. Maybe we can work out a good solution that could combine your use case with the other features.

Regards Sven-Torben

[pdt-ayidi]

Hi @sventorben!

Thanks so much for the reply.

After further testing I did screw up the logic, thanks for catching it early. Currently working on fixing it on my fork, feel free to close this PR and we can continue this in a GitHub discussion.

Here's what I'm trying to implement: https://excalidraw.com/#json=OWxqHDmADEY479rlG8txa,ApVsxCFwIV_0825Je9OA2Q

The authentication flow looks like this:

I would like to avoid the user to have to manually select the IdP or have to click a button to proceed to login with their home IdP. Secondly, if a user is not found it the error be inline.

Thanks again for this extension and being open to collaborate.

Arthur

[pdt-ayidi]

The PR now reflects the logic in the comment above. https://github.com/sventorben/keycloak-home-idp-discovery/pull/82#issuecomment-1193023902

As expected the tests break but the code is a good conduit for conversation.

[sventorben]

Thanks for this. I can take a more in-depth look by the end of the week.

Just a few quick notes:

• Based on you decision diagram, I would like to invert the domain matches an IdP rule and user registered. Whenever there is a registered user with an already established IdP link that link should take preference over other IdPs. That is already how it works today and would be a breaking change if switched.
• The error state in you decision diagram is problematic. As I mentioned before (see https://github.com/sventorben/keycloak-home-idp-discovery/pull/82#issuecomment-1192998605), we may need to continue the flow if there are other alternative authenticators available. Though it may make sense in the flow that you have shown, it is not always feasible to stop the flow there. Let me think about this in more detail throughout the next days. Maybe I need to check if alternative flows are available or something like that.
• I am currently thinking about splitting the existing authenticator into two - one for already registered users and another that just matches based on the domain. This could be more flexible overall and may make the implementation easier. It would also give the flexibility to support the case from your decision diagram and the way it currently works. Any thoughts on this?

[pdt-ayidi]

I would like to invert the domain matches an IdP rule and user registered.

Would be great to figure out how to include this use case. I prefer if the domain rules override the user's preference. Maybe expose it as an option?

we may need to continue the flow if there are other alternative authenticators available

Could we do something like a Condition - User required to accommodate this use case?

I am currently thinking about splitting the existing authenticator into two

That might work as long the end user UX stays the same.

I can also give it more thought as to how to best incorporate all the use cases and configuration.

Take your time, thanks!

[sventorben]

Could we do something like a Condition - User required to accommodate this use case?

Maybe we could add this as a config option, though I would prefer to infer this based on the overall flow configuration. Like: if there is no further alternative challenge the user again.

I am still considering splitting this authenticator into two separate once as mentioned before. However, I will defer my decision on how to proceed here until the outcome of https://github.com/keycloak/keycloak/issues/12102 is clear. This may influence on how to implement the authenticators.

[mnaser]

Just wanted to know if there's been any updates to this, we like the flow proposed here :)

[sventorben]

Sorry, didn't had the time to look into this any further. However, there was not much progress on keycloak/keycloak#12102 recently. So, I am currently reluctant to work on this further without knowing in which direction the feature is heading.

Anyways, can you try whether a flow like this could work for you, please!

[mnaser]

Awesome. I got that working here so that's pretty neat. Thanks!

[pdt-ayidi]

I'm no longer pursuing Keycloak, closing PR. Another contributor should open a discussion!