Ely 2534 OIDC logout support by rsearls · Pull Request #2245 · wildfly-security/wildfly-elytron

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

-        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

    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

+    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

+    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

-    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

-    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

rsearls · 2025-01-13T21:27:30Z

This PR replaced by #2249

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

Closed

[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

rsearls mentioned this pull request Jan 13, 2025

Logout ely 2534 #2249

Closed

rsearls closed this Jan 13, 2025

Conversation

rsearls commented Dec 10, 2024

Uh oh!

fjuma commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjuma commented Jan 3, 2025

Uh oh!

rsearls commented Jan 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fjuma commented Dec 20, 2024 •

edited

Loading