Repository navigation
Continue client browser flow after User login from Identity Provider - #12891
cgeorgilakis wants to merge 2 commits into
Conversation
5243afe to
da9ff6a
Compare
|
I have added a check for CLIENT_AUTHENTICATION_EXECUTION and CLIENT_FLOW_ID not being null in method finishBrokerAuthentication of IdentityBrokerService for overcome errors and test failures. A warn is logged if at least one of CLIENT_AUTHENTICATION_EXECUTION and CLIENT_FLOW_ID is null. However, I am not sure if this check is needed and I do not understand why these variables are null in some cases. Is it related with test authedication flow implementation? |
da9ff6a to
0d511d8
Compare
c39ca9d to
0d511d8
Compare
|
After rebasing some test methods( Moreover, SAML tests ( methods |
0d511d8 to
1114d78
Compare
|
I have rebased it. Now, only failure tests is related to AbstractAdvancedBrokerTest.testPostBrokerLoginFlowWithOTP_bruteForceEnabled. |
|
Thanks a lot for the PR and sorry for it not yet being reviewed. We will try to review this once we have some time... |
|
@mposolda any luck with this review? |
|
Any news on this? Just discovered this bug myself and am very keen to get it fixed.... |
|
Same here. It would be amazing if this bug could be fixed! Thank you for all your great work. |
|
Looking forward to the review whenever the team has a chance. |
|
Looking for this feature!!! |
|
Any updates on this?Looking for this feature!!! |
|
Any updates on this? We are looking forward to this feature |
|
I need this feature too during my transition phase |
| String client_execution = authSession.getAuthNote(AuthenticationProcessor.CLIENT_AUTHENTICATION_EXECUTION); | ||
| String client_flow = authSession.getAuthNote(AuthenticationProcessor.CLIENT_FLOW_ID); | ||
| authSession.clearAuthNotes(); | ||
| //keep CLIENT_AUTHENTICATION_EXECUTION and CLIENT_FLOW_ID if they exist | ||
| if (client_execution != null) | ||
| authSession.setAuthNote(AuthenticationProcessor.CLIENT_AUTHENTICATION_EXECUTION,client_execution); | ||
| if (client_flow != null) | ||
| authSession.setAuthNote(AuthenticationProcessor.CLIENT_FLOW_ID,client_flow); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| Response response = processResult(result, true); | ||
| if (response == null) { | ||
| return continueAuthenticationAfterSuccessfulAction(model); | ||
| } else return response; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -75,11 +76,13 @@ public void authenticate(AuthenticationFlowContext context) { | |||
| } | |||
|
|
|||
| private void redirect(AuthenticationFlowContext context, String providerId) { | |||
| Optional<IdentityProviderModel> idp = context.getRealm().getIdentityProvidersStream() | |||
| Optional<IdentityProviderModel> idp = context.getRealm().getIdentityProvidersStream() | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| //BROKER_USER_ID not null user have been login with Idp -> return success | ||
| if (context.getAuthenticationSession().getUserSessionNotes().get(Details.IDENTITY_PROVIDER_USERNAME) != null) | ||
| context.success(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
| if (!validateForm(context, formData)) { | ||
| return; | ||
| //BROKER_USER_ID not null user have been login with Idp -> return success |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| String execution = authSession.getAuthNote(AuthenticationProcessor.CLIENT_AUTHENTICATION_EXECUTION); | ||
| String flowId = authSession.getAuthNote(AuthenticationProcessor.CLIENT_FLOW_ID); | ||
|
|
||
| if (execution != null && flowId!=null) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| AuthenticationFlow authenticationFlow = processor.createFlowExecution(flowId, model); | ||
| //maybe find next flow | ||
| Response challenge = authenticationFlow.continueClientAuthAfterIdPLogin(model); | ||
| if (challenge != null) return challenge; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| //return to client flow -> otp has been configured -> ask again | ||
| loginTotpPage.login(totp.generateTOTP(totpSecret)); | ||
| waitForAccountManagementTitle(); | ||
| AccountHelper.logout(adminClient.realm(bc.consumerRealmName()), bc.getUserLogin()); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aec0feb to
447fff5
Compare
| if (context.getAuthenticationSession().getUserSessionNotes().get(Details.IDENTITY_PROVIDER_USERNAME) != null) | ||
| context.success(); |
There was a problem hiding this comment.
| if (context.getAuthenticationSession().getUserSessionNotes().get(Details.IDENTITY_PROVIDER_USERNAME) != null) | |
| context.success(); | |
| if (context.getAuthenticationSession().getUserSessionNotes().get(Details.IDENTITY_PROVIDER_USERNAME) != null) { | |
| context.success(); | |
| } |
|
Unfortunately, I’m not familiar with the Keycloak codebase so I can’t tell whether it’s conceptually and logically correct. However, I’m also affected by this bug/flaw and look forward to the review. |
|
@mposolda just a friendly ping here. |
|
@cgeorgilakis Sorry for the late review. Thanks for the PR. Unfortunately I think it makes sense even more complex and fragile (authentication flows are already quite fragile in general). And the test failures are also problematic... I believe we can address your use-case (and hopefully most other use-cases as well) by some easier way. I've commented on the issue #10250 (comment) ). |
Closes keycloak#10250 Signed-off-by: cgeorgilakis-grnet <cgeorgilakis@admin.grnet.gr>
447fff5 to
553ab09
Compare
|
Closing this as it's been open a long time without consensus. |
|
Are you kidding us? It’s not a lack of consensus, but a lack of care from the Keycloak devs. The problem is still present and unresolved. |
Closes #10250
Our implementation saves in AuthNotes of AuthenticationSessionModel all needed information for Client Browser Flow ( AuthenticationProcessor.CURRENT_AUTHENTICATION_EXECUTION of Client Browser Flow, flowid) before redirecting to Identity Provider login. After Identity Provider login has successfully finished, Keycloak can retrieve all needed information for Client Browser Flow from AuthNotes of AuthenticationSessionModel and proceed with Client Browser Flow.
Browser flows execution are now continuing after Identity Provider Login. That's why some tests need to be changed. Some changes indicate that Browser flow with Identity Provider login, is working well. I do not know if we need extra tests.
More tests failures are related with SamlClientBuilder and its related code. I do not know well test implementation of SamlClientBuilder in order to make appropriate changes in it or found possible errors in my code. Finally, KcOidcBrokerTest/KcSamlBrokerTest test method testPostBrokerLoginFlowWithOTP failure is strange.