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

Alternative implementation to logout with id_token_hint #3053

Closed
wants to merge 5 commits into from

Conversation

strehle
Copy link
Member

@strehle strehle commented Sep 17, 2024

@strehle strehle marked this pull request as ready for review September 26, 2024 13:45
@strehle strehle changed the title [WIP ]Draft to logout Alternative implementation to logout with id_token_hint Sep 26, 2024
@@ -155,6 +157,10 @@ public Authentication authenticate(Authentication request) throws Authentication
uaaAuthenticationDetails = UaaAuthenticationDetails.UNKNOWN;
}
UaaAuthentication success = new UaaAuthentication(new UaaPrincipal(user), user.getAuthorities(), uaaAuthenticationDetails);
success.setSamlMessageContext(new SAMLMessageContext());
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to add this? Not sure how it is related to the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not Saml, but you see here is the set of the SAML credential which can be re-retrieved after login

@coolgang123
Copy link
Contributor

Looks good to me ! okay to merge after addressing comment from Mike.

@@ -155,6 +157,10 @@ public Authentication authenticate(Authentication request) throws Authentication
uaaAuthenticationDetails = UaaAuthenticationDetails.UNKNOWN;
}
UaaAuthentication success = new UaaAuthentication(new UaaPrincipal(user), user.getAuthorities(), uaaAuthenticationDetails);
success.setSamlMessageContext(new SAMLMessageContext());
if (authenticationData instanceof ExternalOAuthAuthenticationManager.AuthenticationData authenticationInternal) {
success.setIdpIdToken(authenticationInternal.getIdToken());
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You added line 160 as part of this PL so I'm not sure of your point. Ideally this ExternalLoginAuthenticationManager wouldn't have SAML or OIDC specific code in it. That's why my PL keeps all the id_token_hint changes in the OIDC classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

no it not the check, but the fact that UaaLoginHint is used to parse the login_hint parameter.

My request is: use either UaaAuthentication because here are all session relevant information.

In case of SAML the credential is also stored here.

The other class is used to parse requests and therefore you had to jsonIgore

Copy link
Contributor

Choose a reason for hiding this comment

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

See my new commit on PL 3049. I'm storing the id_token in the UaaAuthentication now. I think the only significant difference is that I use the existing ExternalOAuthCodeToken to carry the id_token over to where it can be set in the UaaAuthentication, and all of my changes are contained in the OAuth packages.

@strehle
Copy link
Member Author

strehle commented Oct 4, 2024

not needed

@strehle strehle closed this Oct 4, 2024
@strehle strehle deleted the draftFixLogout branch October 4, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants