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

OpenID logout / more extensibible OpenIdConfiguration #8216

Closed
keller-j opened this issue Jun 28, 2022 · 7 comments
Closed

OpenID logout / more extensibible OpenIdConfiguration #8216

keller-j opened this issue Jun 28, 2022 · 7 comments

Comments

@keller-j
Copy link
Contributor

Target Jetty version(s)
10.0.x
11.0.x
12.0.x

Additional Details
I'm currently working with jetty-11 and available OpenIdAuthenticator. My OpenID (SSO) server is running on keycloak-18 while trying to logout via client application (jetty) I realized that this isn't working at the moment. While having a look at OpenIdConfiguraion I realized that no information about end_session_endpoint isn't even processed in OpenIdConfiguration:
https://github.com/eclipse/jetty.project/blob/5fe999caa25755dec57d21bc3d694f67de4e45ab/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java#L119-#L130
At the moment I'm extending OpenIdConfiguration and loading end_session_endpoint information with a separate call similar to https://github.com/eclipse/jetty.project/blob/5fe999caa25755dec57d21bc3d694f67de4e45ab/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java#L141-#L172 . Then in an extended OpenIdAuthenticator I'm reacting to a certain logout page with redirecting user to end_session_endpoint or (depending on class configuration) also providing id_token_hint (e.g., available via HttpSession) and post_logout_redirect_uri as stated in OpenID documentation: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout

Enhancement Description

  1. For more easy extensibility of OpenIdConfiguration would be great if results fetched from OpenID configuration endpoint could be further processed for own needs by simply extending OpenIdConfiguration and implementing / overriding protected method, e.g., processMetadata(Map<?, ?> fetchedMetadata) which can be called in doStart with information of discoveryDocument:
    https://github.com/eclipse/jetty.project/blob/5fe999caa25755dec57d21bc3d694f67de4e45ab/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java#L119 . Since there are lot of configuration settings not yet considered for current implementation possibility to simply process and store them along the configuration could come in handy.
  2. OpenIdAuthenticator could also support logout from SSO. Therefore additional information must be processed in OpenIdConfiguration and considered in OpenIdAuthenticator.
@lachlan-roberts
Copy link
Contributor

@keller-j I have a PR to make OpenIdConfiguration more easily extensible. Take a look at PR #8221 and let me know if that would help with your usage.

Not sure how we would implement the logout, but I will take a closer look tomorrow. Feel free to open a PR yourself if you have an idea of how to do this.

keller-j added a commit to keller-j/jetty.project that referenced this issue Jun 29, 2022
Signed-off-by: Johannes Keller <keller.johannes95@gmail.com>
@keller-j
Copy link
Contributor Author

keller-j commented Jun 29, 2022

@lachlan-roberts thanks a lot for the support. This fitted exactly my needs. That way I could easily also process the end_session_endpoint etc. information.

I included my current implementation for OpenIdAuthenticator logout in my project fork https://github.com/keller-j/jetty.project/tree/jetty-10.0.x-8216-openid-logout as I wasn't able to push to your branch. Also created pull request to your openid-logout branch PR #8224 .

Note this isn't the perfect solution for the logout problem. Also I'm not quite sure whether it's OK to return Authentication.SEND_CONTINUE in validateRequest while redirecting response to the end_session_endpoint. Furthermore there are further logout options via end_session_endpoint with supplying URL parameter for id_token_hint which is dependent on the redirectAfterLogoutPath parameter of my proposed snippet.

Feel free to re-use / and change the proposed snippet for logout.

NOTE: didn't test the new implementation in production, but a similar implementation in my project where I'm extending OpenIdAuthenticator and OpenIdConfiguration to fit to these needs and come up with this implementation. Yet could still happen that there are further minor bugs in my uploaded version

lachlan-roberts added a commit that referenced this issue Jul 4, 2022
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 4, 2022
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 4, 2022
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 4, 2022
Issue #8216 - make processing of metadata in OpenIdConfiguration extensible
@keller-j
Copy link
Contributor Author

@lachlan-roberts did you have the chance to think about OpenIdAuthenticator logout?

lachlan-roberts added a commit that referenced this issue Jul 12, 2022
…endpoint redirect

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 12, 2022
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor

@keller-j yes I have now created PR #8286 which includes your initial commit.
So take a look and let me know what you think.

@keller-j
Copy link
Contributor Author

@lachlan-roberts looks really good to me! Thanks a lot for the support.

Found a minor issue in one of the constructors, left a comment at the pull-request.

lachlan-roberts added a commit that referenced this issue Jul 13, 2022
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 13, 2022
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 20, 2022
@lachlan-roberts
Copy link
Contributor

@keller-j the changes have been merged with PR #8286.
Will be available in next release (10.0.12 / 11.0.12).

Jetty 10.0.12 / 11.0.12 automation moved this from To do to Done Jul 20, 2022
@keller-j
Copy link
Contributor Author

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants