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

Issue #9464 - Add optional configuration to log user out after OpenID idToken expires. (Jetty-10) #9528

Merged
merged 7 commits into from Apr 11, 2023

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented Mar 23, 2023

Issue #9464

Add new configuration parameter called respectIdTokenExpiry which if set to true will log the user out after the idToken has expired and attempt to redirect back to the OpenID provider to be re-authenticated.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
If respectIdTokenExpiry is true always attempt re-authentication if idToken expires even for non-mandatory requests.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I've not reviewed deeply, but I think the core logic change could be better described in comments and perhaps a slight restructure of the if/else

Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

@lachlan-roberts I have concerns about this change. Let's discuss them when possible.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
travisspencer
travisspencer previously approved these changes Mar 28, 2023
Copy link
Contributor

@travisspencer travisspencer left a comment

Choose a reason for hiding this comment

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

Looks good. Didn't review the test closely, but the run-time code looks good IMO.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

loggedInUsers.decrement();
resp.setContentType("text/plain");
resp.sendRedirect(logoutRedirect);

Check warning

Code scanning / CodeQL

URL redirection from remote source Medium

Untrusted URL redirection depends on a
user-provided value
.
Copy link
Contributor

Choose a reason for hiding this comment

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

This was dismissed, as this was a test case

@lachlan-roberts
Copy link
Contributor Author

Will wait until the jetty-security changes in jetty-12 are done before I merge this, as the OpenID code will be moved to jetty-core. (see #9405)

@lachlan-roberts lachlan-roberts merged commit 24b7d06 into jetty-10.0.x Apr 11, 2023
4 checks passed
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-OpenIdLogoutRedirect branch April 11, 2023 02:20
@gregw gregw added the Sponsored This issue affects a user with a commercial support agreement label Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants