-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Issue #9464 - Add optional configuration to log user out after OpenID idToken expires. (Jetty-10) #9528
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
If respectIdTokenExpiry is true always attempt re-authentication if idToken expires even for non-mandatory requests. Signed-off-by: Lachlan Roberts <[email protected]>
There was a problem hiding this 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
jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
Outdated
Show resolved
Hide resolved
jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
Show resolved
Hide resolved
jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
Show resolved
Hide resolved
There was a problem hiding this 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.
jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdCredentials.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
There was a problem hiding this 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.
jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
Show resolved
Hide resolved
jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdProvider.java
Show resolved
Hide resolved
jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdCredentials.java
Outdated
Show resolved
Hide resolved
jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdProvider.java
Outdated
Show resolved
Hide resolved
jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdProvider.java
Outdated
Show resolved
Hide resolved
jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdProvider.java
Outdated
Show resolved
Hide resolved
jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdProvider.java
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <[email protected]>
|
||
loggedInUsers.decrement(); | ||
resp.setContentType("text/plain"); | ||
resp.sendRedirect(logoutRedirect); |
Check warning
Code scanning / CodeQL
URL redirection from remote source
There was a problem hiding this comment.
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
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) |
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.