Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect user to Idp or continue flow if already signed in #312

Closed
wants to merge 2 commits into from

Conversation

BlackVoid
Copy link

Solved #309

Trigger redirect to IdP or continue to next authenticator if user is already authenticated and keycloak requires re-authentication ex. when using application initiated actions.

This change results in the user being redirected to the IdP or IdP selection page if or if the user is using a password only, then it's sent to the next authenticator. If it's the username-password form then the user is prompted to enter the password only instead of having to enter the username again.

image

…already signed in and re-authentication is required
@sventorben
Copy link
Owner

Hello Felix,

thanks for the PR and the effort you put into this. I looked into the issue description that you have provided in #309 and think I get where this is heading to.

I see a few challenges with the approach that you have taken in the PR. While I think it may work for your special case (can't say for sure, because of missing tests), for this extension we need to take a broader look at for a more generic solution.

I especially see the following:

  • we need to define the wanted behaviour with all combinations of configuration options. You PR currenlty mainly handles the case where login page should be skipped
  • take into account different flow combinations of the authenticator (e.g. whether it is required or an alternative, or where it is placed in a flow)
  • what if a user has multiple credentials (e.g. a local password and a linked IdP), your approach would make it impossible for the user to decide how to reauthenticate (aka "Try another way" button). Not sure here, if we should always redirect.
  • what if a user is linked to multiple IdPs and has selected one of them for authentication. Should the user be redirected to the IdP that was used on first login? Should the user have the choice again?
  • In your scenario the user must reauthenticate and not reuse the cookie for the SSO session. If the user is redirected to the upstream IdP, the SSO from the upstream will be reused. However, the user should be forced to reauthenticate there as well. For OIDC parameters like prompt=login or max-age=... need to be send to the upstream IdP. Need to think about SAML as well.
  • When the upstream redirects back, it needs to be assured that reauthentication has taken place, but currently there is no way to check that and it would heavily depend on information/claims provided by the upstream IdP whether this could be checked or not.
  • How do prevent an indenfinite redirect loop with this approach?

There are too many open questions for me and I come to the conclusion that this needs a more thorough upfront design. I think this would be a good feature request, indeed.

Best regards
Sven-Torben

PS: Please also respect the contribution guidelines for this repository, especially upfront discussion and testing.


final UserModel user = authenticationFlowContext.getAuthenticationSession().getAuthenticatedUser();
if (loginHint == null && user != null) {
loginHint = trimToNull(user.getEmail());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hard coded to the email address, but needs to respect authenticator configuration. Try to use something like user.getFirstAttribute(context.config().userAttribute()).

@BlackVoid
Copy link
Author

Thanks for your feedback, I'll answer your concerns the best I can.

* we need to define the wanted behaviour with all combinations of configuration options. You PR currenlty mainly handles the case where login page should be skipped

Makes sense, I'll make sure to add tests once we've agreed on a solution and you are right that it only handles the case where "bypass" login page is configured. The final solution should handle both cases.

* take into account different flow combinations of the authenticator (e.g. whether it is required or an alternative, or where it is placed in a flow)

So basically a conditional call "attempted" based on if it's required or alternative?

* what if a user has multiple credentials (e.g. a local password and a linked IdP), your approach would make it impossible for the user to decide how to reauthenticate (aka "Try another way" button). Not sure here, if we should always redirect.
* what if a user is linked to multiple IdPs and has selected one of them for authentication. Should the user be redirected to the IdP that was used on first login? Should the user have the choice again?

With the solution in this PR it would follow the same flow as if bypass login page is on and login_hint has been provided, so it makes use of the settings for the authenticator. Perhaps an additional setting to control if the bypass should be enabled when the user is already logged in would make sense. So in the end the login page bypass would be based on what the admin of the instance has configured.

* In your scenario the user must reauthenticate and not reuse the cookie for the SSO session. If the user is redirected to the upstream IdP, the SSO from the upstream will be reused. However, the user should be forced to reauthenticate there as well. For OIDC parameters like `prompt=login` or `max-age=...` need to be send to the upstream IdP. Need to think about SAML as well.
* When the upstream redirects back, it needs to be assured that reauthentication has taken place, but currently there is no way to check that and it would heavily depend on information/claims provided by the upstream IdP whether this could be checked or not.

I don't think this is something that Keycloak injects or checks out of the box when it requires re-authentication, so is this something that should be handled by this extension?

* How do prevent an indenfinite redirect loop with this approach?

It would have the same issue as when login_hint is provided, so I guess there would have to be some loop detection in both cases?

There are too many open questions for me and I come to the conclusion that this needs a more thorough upfront design. I think this would be a good feature request, indeed.

Do you have any solution in mind to solve this, my main issue with the existing behavior is that the user needs to input their own username when they are already signed in and I don't think that makes any sense. So I'm open to suggested solutions that would solve this and work in more than just my use-case.

PS: Please also respect the contribution guidelines for this repository, especially upfront discussion and testing.

Sorry about that, I thought that showing code might clarify the problem I'm facing, it was not intended as a finalized PR.

@sventorben
Copy link
Owner

@BlackVoid thanks for your reply. I just don't have the time to deal intensively with your comments right now. Give me a few days and I'll come to you with a suggestion.

@sventorben
Copy link
Owner

  • In your scenario the user must reauthenticate and not reuse the cookie for the SSO session. If the user is redirected to the upstream IdP, the SSO from the upstream will be reused. However, the user should be forced to reauthenticate there as well. For OIDC parameters like prompt=login or max-age=... need to be send to the upstream IdP. Need to think about SAML as well.
  • When the upstream redirects back, it needs to be assured that reauthentication has taken place, but currently there is no way to check that and it would heavily depend on information/claims provided by the upstream IdP whether this could be checked or not.

With reference to the PR on the Keycloak project (see above), these points are better handled upstream within Keycloak itself and do not need to be implemented within this extension.

@BlackVoid
Copy link
Author

No worries @sventorben, there is no hurry from my side to solve this, just something I discovered while testing various use cases with the extension. Once we have decided on a suitable solution I'd be happy to implement it.

sventorben added a commit that referenced this pull request Feb 1, 2024
Implement username persistence on the login form to
improve user experience by eliminating the need for users
to re-enter their usernames during the reauthentication
process.

Relates to #312
@sventorben
Copy link
Owner

I'd rather go with something like this: d672d83
This way it makes the reauthentication case more explicit in the code.

I will add some integrations test when I find some time and can add the feature to one of the next releases once done.

@BlackVoid
Copy link
Author

I tried your solution and I think it will work great. One issue I noticed is that the code change breaks when not signed in. In order to try the solution I made the Reauthentication.required method return false if the user is null, not sure if it causes any unintended consequences, but it appeared to work when testing logins with oauth apps and when re-authentication is required.

@BlackVoid
Copy link
Author

Hi @sventorben

Just wanted to check when you think your change can be merged and if there is anything I can do to help with getting it ready.

@sventorben
Copy link
Owner

@BlackVoid I am currently working on a larger refactoring to support some other upcoming features. I need to delay this issue until that refactoring is done and then reapply the changes. I think this should be done by the end of this month, but can't promise for sure.

@BlackVoid
Copy link
Author

Okay, thank you for the update. Good luck with the refactoring.

@sventorben sventorben linked an issue May 15, 2024 that may be closed by this pull request
1 task
sventorben added a commit that referenced this pull request May 15, 2024
Implement username persistence on the login form to
improve user experience by eliminating the need for users
to re-enter their usernames during the reauthentication
process.

Relates to #312
sventorben added a commit that referenced this pull request May 15, 2024
Implement username persistence on the login form to
improve user experience by eliminating the need for users
to re-enter their usernames during the reauthentication
process.

Relates to #312
sventorben added a commit that referenced this pull request May 15, 2024
Implement username persistence on the login form to
improve user experience by eliminating the need for users
to re-enter their usernames during the reauthentication
process.

Relates to #312
@sventorben sventorben self-assigned this May 15, 2024
@sventorben sventorben added the enhancement New feature or request label May 15, 2024
@sventorben
Copy link
Owner

Closing this as it should be fixed with #366

@sventorben sventorben closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] On re-authentiction the currently logged in user is ignored
2 participants