Repository navigation
Introducing IdpLinkAction as AIA to replace client-initiated account … - #38952
Conversation
0b19e3a to
b71343c
Compare
There was a problem hiding this comment.
Perhaps have the idp_link as a enum in IdentityProviderModel similar to how they are defined for users?
There was a problem hiding this comment.
I am not sure what you mean by idp_link being an enum. Do you mean that it should be constant instead of hardcoded string?
There was a problem hiding this comment.
I run a test here where the user is not yet authenticated and trying to login to some application (e.g.: the account console). At the login page, I change the URL to append the kc_action=idp_link:<alias> parameter.
Once I authenticate as the user, there is no identity cookie yet and the call to context.ignore happens without executing the linking.
Is that expected? Or in this case, the user should be automatically redirected to the IdP to link the account?
I'm also wondering if this AIA should have an intermediary page to tell users they are about to link their account and eventually be redirected to another URL (the IdP login page)?
There was a problem hiding this comment.
Thanks for pointing this! For now, trigger the idp_link currently requires UserSessionModel as the previous "Client initiated account linking" also required UserSessionModel and user already being authenticated hence also the callback code in IdentityBrokerService also required the UserSessionModel. However it is true that in case of AIA, the UserSessionModel may not be needed and you can have possibility to trigger AIA without userSession. It will require some more changes in IdentityBrokerService and other places, but I can give it a try.
For the intermediary page: I am not 100% sure about usability as it means that when user clicks on "Link Facebook" in the account console, it will show him another page where he would need to confirm "Do you really want to link Facebook?" . Not sure if it is another boiler-plate page? On the other hand, some other actions like Delete credential also have another page and nobody had issues when we introduced it. So I can give it a try as well.
There was a problem hiding this comment.
@pedroigor Addressed both your points. Could you please re-review?
cc80f1b to
affd612
Compare
…linking to IDP closes keycloak#37269 closes keycloak#35446 Signed-off-by: mposolda <mposolda@gmail.com> Co-authored-by: andymunro <48995441+andymunro@users.noreply.github.com> Signed-off-by: Marek Posolda <mposolda@gmail.com>
Signed-off-by: mposolda <mposolda@gmail.com>
Signed-off-by: mposolda <mposolda@gmail.com>
affd612 to
519c49d
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.AuthenticatorSubflowsTest2#testSubflow2Keycloak CI - Forms IT (chrome) |
…linking to IDP
closes #37269
closes #35446
The PR introduces AIA
IdpLinkActionfor linking user to the identity provider. This allows that IDP linking can be triggered from the client similarly like the other AIA and there is no need to have custom protocol just for the client-initiated account linking.The custom protocol for client-initiated account linking is deprecated now and might be removed in the Keycloak 27. I will create follow-up issue for this. This means one less protocol to maintain and handle possible issues.
In relation to that, the PR also fixes the bug External IDP error during Step-Up Authentication does no longer route back to browser flow #37269. After authentication with the IDP, there is the code in the
IdentityBrokerService, which tries to detect if the IDP authentication was triggered from the authentication or from the already authenticated application, which wanted the user to link his account. The problem is, that this detection was broken and in fact worked only for account console, but not for any other application. This was fixed by Other clients but Account-Management can link account #35728, however it introduced other problem External IDP error during Step-Up Authentication does no longer route back to browser flow #37269 as when userSession already existed (for example during re-authentication or other scenarios), it detected that it is "account linking" when it should be "authentication" . The detection is now based on the existence of authentication session noteLINKING_IDENTITY_PROVIDERto be in the correct format (Note is used by both the AIA and the legacy mechanism. I've added the test for the legacy mechanism as it seems we did not had any automated test for testing it besides UI tests of account console).Account console also updated to use the new AIA based mechanism. The account REST endpoint
LinkedAccountsResource.buildLinkedAccountURIis deprecated now and not used anymore by account UI.NOTE: The failure in the documentation CI in
external linkstest is not regression of this PR and it is going to be fixed by the other PR #38094 .