Ely 2534 OIDC logout support #2245

rsearls · 2024-12-10T19:59:37Z

https://issues.redhat.com/browse/ELY-2534

…ck-channel logout

…an active session

…ing doesn't rely on an active session and ensure the session will get invalidated appropriately upon subsequent requests

…validation check to before the authenticator#authenticate call and update the check to not remove the session from the map to ensure that the user gets logged out from any other apps too

… the sessionsMarkedForInvalidation map

fjuma · 2024-12-20T20:20:10Z

@rsearls We discussed this a bit before but just wanted to add a comment here so we don't forget.

The last commit in this PR is currently a bit hard to review because it contains a few changes that I had made in separate commits along with subsequent changes you made altogether in one commit.

To make it a bit easier to review and understand the changes that were made, I have created a branch here that preserves the original commits we had and incorporates the changes from your branch:

https://github.com/fjuma/wildfly-elytron/tree/updated-oidc-logout

I have verified that the Elytron testsuite passes with this branch.

I have submitted a PR against your branch (rsearls#2) so you could just click merge to get these changes included here or alternatively, you could check out my branch and then create a new PR based off that: https://github.com/fjuma/wildfly-elytron/tree/updated-oidc-logout

Let me know if you have any questions about this. Thanks!

Updated this branch to preserve some individual commits for better readability

fjuma · 2025-01-03T21:11:54Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/AuthenticationError.java

-        NO_QUERY_PARAMETER_ACCESS_TOKEN
+        NO_QUERY_PARAMETER_ACCESS_TOKEN,
+        NO_SESSION_ID,
+        METHOD_NOT_ALLOWED


I think NO_SESSION_ID and METHOD_NOT_ALLOWED can be removed, I don't see them being used.

fjuma · 2025-01-03T21:12:58Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

@@ -278,5 +278,12 @@ interface ElytronMessages extends BasicLogger {

    @Message(id = 23070, value = "Authentication request format must be one of the following: oauth2, request, request_uri.")
    RuntimeException invalidAuthenticationRequestFormat();
+
+    @Message(id = 23071, value = "%s is not a valid value for %s")
+    RuntimeException invalidLogoutPath(String pathValue, String pathName);


Might be good to mention the logout path is invalid, e.g., "Invalid logout output: %s is not a valid value for %s"

fjuma · 2025-01-03T21:13:19Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

+    RuntimeException invalidLogoutPath(String pathValue, String pathName);
+
+    @Message(id = 23072, value = "The end substring of %s: %s can not be identical to %s: %s")
+    RuntimeException invalidLogoutCallbackPath(String callbackPathTitle, String callbacPathkValue,


Same here, might be good to provide more details in the error message

fjuma · 2025-01-03T21:13:32Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

+    RuntimeException invalidLogoutPath(String pathValue, String pathName);
+
+    @Message(id = 23072, value = "The end substring of %s: %s can not be identical to %s: %s")
+    RuntimeException invalidLogoutCallbackPath(String callbackPathTitle, String callbacPathkValue,


s/callbacPathkValue/callbackPathValue

Maybe s/logoutPathTitle/logoutPathName?

fjuma · 2025-01-03T21:14:36Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/LogoutHandler.java

-    private static final String POST_LOGOUT_REDIRECT_URI_PARAM = "post_logout_redirect_uri";
-    private static final String ID_TOKEN_HINT_PARAM = "id_token_hint";
+    public static final String POST_LOGOUT_REDIRECT_URI_PARAM = "post_logout_redirect_uri";
+    public static final String ID_TOKEN_HINT_PARAM = "id_token_hint";


Do these need to be public?

fjuma · 2025-01-03T21:14:46Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/LogoutHandler.java

-    private static final String ISS = "iss";
+    private static final String CLIENT_ID_SID_SEPARATOR = "-";
+    public static final String SID = "sid";
+    public static final String ISS = "iss";


Do these need to be public?

fjuma · 2025-01-03T21:24:41Z

@rsearls Was just starting to provide some feedback on your commits but just noticed an additional commit (ebdeafe) seems to have popped up again that contains a mixture of your changes and my changes so it's a bit hard to see what has changed. I don't see that one in my branch here:

https://github.com/fjuma/wildfly-elytron/commits/updated-oidc-logout/

…ade private

pedroigor and others added 11 commits March 22, 2023 17:24

OpenID Connect Logout support

4698b60

Merge remote-tracking branch 'upstream/2.x' into ELY-2534

298d503

[ELY-2534] Update copyright headers

4fe600b

[ELY-2534] Update the creation of the test HTTP server request for ba…

d430574

…ck-channel logout

[ELY-2534] Update back-channel logout handling so it doesn't rely on …

c16a6cb

…an active session

[ELY-2534] Farah's branch merged with 2.x

0ba9eea

logout file with debug stmts

ad376b0

working code for rpinitiated logout. contains tmp debug stmts

3affa81

[ELY-2534] Restore the changes made so that back-channel logout handl…

509a811

…ing doesn't rely on an active session and ensure the session will get invalidated appropriately upon subsequent requests

[ELY-2534] Fix back-channel logout by moving the isSessionMarkedForIn…

77d4911

…validation check to before the authenticator#authenticate call and update the check to not remove the session from the map to ensure that the user gets logged out from any other apps too

[ELY-2534] Use the client ID and sid from the ID token in the key for…

3aebc41

… the sessionsMarkedForInvalidation map

rsearls mentioned this pull request Dec 13, 2024

WFLY-19314 added integration tests for oidc logout wildfly/wildfly#18523

Draft

[ELY-2534] OIDC logout support

ebdeafe

rsearls force-pushed the ELY-2534-OIDC-logout-Farah-RLS-CHANGES-V2 branch from aef15f7 to ebdeafe Compare December 18, 2024 13:26

[ELY-2534] OIDC logout support

9a61d67

Merge pull request #2 from fjuma/updated-oidc-logout

d20aa8a

Updated this branch to preserve some individual commits for better readability

fjuma mentioned this pull request Jan 3, 2025

Ely 2534 OIDC logout farah #2230

Closed

fjuma reviewed Jan 3, 2025

View reviewed changes

fjuma mentioned this pull request Jan 3, 2025

Baseline for OpenID Connect Logout #1882

Closed

3 tasks

rsearls added 5 commits January 9, 2025 15:10

[ELY-2534] OIDC logout support

48c15dd

support post-logout-path as only an absolute URI

6974f0a

fixed unit test to support post_logout_path as a URI

edd3d93

working test fix

324e71c

remove obsolete ENUMs. Text change in a few msgs. Several constants m…

8d1e837

…ade private

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ely 2534 OIDC logout support #2245

Ely 2534 OIDC logout support #2245

rsearls commented Dec 10, 2024

fjuma commented Dec 20, 2024 •

edited

Loading

fjuma Jan 3, 2025

rsearls Jan 10, 2025

fjuma Jan 3, 2025

rsearls Jan 10, 2025

fjuma Jan 3, 2025

rsearls Jan 10, 2025

fjuma Jan 3, 2025

fjuma Jan 3, 2025

rsearls Jan 10, 2025

fjuma Jan 3, 2025

rsearls Jan 10, 2025

fjuma Jan 3, 2025

rsearls Jan 10, 2025

fjuma commented Jan 3, 2025

Ely 2534 OIDC logout support #2245

Are you sure you want to change the base?

Ely 2534 OIDC logout support #2245

Conversation

rsearls commented Dec 10, 2024

fjuma commented Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma commented Jan 3, 2025

fjuma commented Dec 20, 2024 •

edited

Loading