feat: add ability to revoke login sessions by SessionID by sgal · Pull Request #3450 · ory/hydra

sgal · 2023-02-26T19:55:56Z

Added support to revokeOAuth2LoginSessions API to revoke a single session by a SessionID (sid claim in the id_token) and execute an OpenID Connect Back-channel logout.

Related issue(s)

#3448

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

consent/handler.go

sgal · 2023-02-26T19:57:46Z

consent/handler.go

-// has to re-authenticate at the Ory OAuth2 Provider. This endpoint does not invalidate any tokens and
-// does not work with OpenID Connect Front- or Back-channel logout.
+// If you send the subject in a query param, all authentication sessions that belong to that subject are revoked.
+// No OpennID Connect Front- or Back-channel logout is performed in this case.


Looks like part of this can be fixed as well (back-channel), maybe outside of this change.

consent/strategy.go

sgal · 2023-02-26T19:59:20Z

consent/strategy_default.go

+		return nil
+	}
+
+	if err := s.executeBackChannelLogout(r.Context(), r, loginSession.Subject, sid); err != nil {


Everything from here until the end of the function is the same as in completeLogout function. Is this the correct direction to take here?

That looks good to me! Since this is called via an API, we won't have front-channel log-out (because no browser is involved). I think this is fine, but should probably be clarified in the API / function doc. You may opt to create a new function with the flow as to avoid duplication, but it's not required since it's only 2 functional statements

aeneasr

Looks quite good already! We'll a few tests for this but then this can ship

consent/handler.go

aeneasr · 2023-02-28T07:32:25Z

consent/strategy_default.go

+	s.r.AuditLogger().
+		WithRequest(r).
+		WithField("subject", loginSession.Subject).
+		Info("User logout completed!")


Clarify that this is the headless flow

aeneasr · 2023-02-28T07:32:27Z

consent/strategy_default.go

+		return nil
+	}
+
+	if err := s.executeBackChannelLogout(r.Context(), r, loginSession.Subject, sid); err != nil {


That looks good to me! Since this is called via an API, we won't have front-channel log-out (because no browser is involved). I think this is fine, but should probably be clarified in the API / function doc. You may opt to create a new function with the flow as to avoid duplication, but it's not required since it's only 2 functional statements

codecov · 2023-02-28T08:22:13Z

Codecov Report

Merging #3450 (a5090fe) into master (ffe4943) will decrease coverage by 0.01%.
The diff coverage is 58.06%.

❗ Current head a5090fe differs from pull request most recent head 43bc7d9. Consider uploading reports for the commit 43bc7d9 to get more accurate results

@@            Coverage Diff             @@
##           master    #3450      +/-   ##
==========================================
- Coverage   76.86%   76.85%   -0.01%     
==========================================
  Files         123      123              
  Lines        9108     9131      +23     
==========================================
+ Hits         7001     7018      +17     
- Misses       1663     1666       +3     
- Partials      444      447       +3

Impacted Files	Coverage Δ
consent/strategy_default.go	`69.50% <54.54%> (-0.09%)`	⬇️
consent/handler.go	`66.77% <66.66%> (+0.76%)`	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

consent/strategy_logout_test.go

sgal · 2023-03-03T07:16:02Z

consent/strategy_logout_test.go

 	})
+
+	t.Run("case=should execute backchannel logout in headless flow with sid", func(t *testing.T) {
+		sid := make(chan string, 2)


Using a buffered channel here to make sure that we have both values in it and do not block the goroutine that tries to read it.

hperl

Thanks, this looks very good! I have some small comments and then it's ready to merge :)

consent/handler.go

consent/strategy_default.go

sgal · 2023-03-07T13:39:45Z

@hperl Fixed the comments, please have a look.

hperl

Thanks for the changes, LGTM! 🎉

…dless logout

kabirbaidhya · 2023-03-21T13:42:25Z

Hey @aeneasr, when are you planning to release this change?

API `revokeOAuth2LoginSessions` can now revoke a single session by a SessionID (`sid` claim in the id_token) and execute an OpenID Connect Back-channel logout. Closes ory#3448

sgal commented Feb 26, 2023

View reviewed changes

consent/handler.go Outdated Show resolved Hide resolved

sgal commented Feb 26, 2023

View reviewed changes

consent/strategy.go Show resolved Hide resolved

sgal commented Feb 26, 2023

View reviewed changes

sgal changed the title ~~[WIP] feat: add ability to revoke login sessions by SessionID~~ feat: add ability to revoke login sessions by SessionID Feb 26, 2023

aeneasr reviewed Feb 28, 2023

View reviewed changes

sgal commented Mar 1, 2023

View reviewed changes

consent/strategy_logout_test.go Outdated Show resolved Hide resolved

sgal commented Mar 1, 2023

View reviewed changes

consent/strategy_logout_test.go Show resolved Hide resolved

sgal commented Mar 1, 2023

View reviewed changes

consent/strategy_logout_test.go Show resolved Hide resolved

sgal commented Mar 3, 2023

View reviewed changes

consent/strategy_logout_test.go Show resolved Hide resolved

sgal commented Mar 3, 2023

View reviewed changes

consent/strategy_logout_test.go Outdated Show resolved Hide resolved

sgal commented Mar 3, 2023

View reviewed changes

sgal marked this pull request as ready for review March 3, 2023 09:47

hperl self-requested a review March 6, 2023 09:53

hperl requested changes Mar 7, 2023

View reviewed changes

consent/handler.go Outdated Show resolved Hide resolved

consent/strategy_default.go Outdated Show resolved Hide resolved

sgal added 6 commits March 7, 2023 13:48

feat: add ability to revoke login sessions by session_id

8536d94

fix: add headless logout method to test helpers

7dbf5d7

add test for headless logout flow

cee7055

fix tcp connection error in headless logout test

3231be1

fix headless logout tests

a3d1174

fix: address the review comment

f616ce1

hperl previously approved these changes Mar 7, 2023

View reviewed changes

fix: add test for case when neither sid nor subject are passed in hea…

43bc7d9

…dless logout

sgal dismissed hperl’s stale review via 43bc7d9 March 11, 2023 10:55

aeneasr approved these changes Mar 13, 2023

View reviewed changes

aeneasr merged commit b42482b into ory:master Mar 13, 2023

sgal deleted the add-revoke-login-session-by-sid branch March 13, 2023 10:10

github-actions bot mentioned this pull request Mar 14, 2023

upstream reference closed: github.com/ory/hydra/pull/3450 #3466

Closed

1 task

kabirbaidhya mentioned this pull request Mar 21, 2023

Update revokeOAuth2LoginSessions client API to include the sid parameter ory/sdk#259

Closed

6 tasks

aarmam mentioned this pull request Apr 17, 2023

feat: endpoint to delete login session by session id #2876

Closed

7 tasks

Conversation

sgal commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue(s)

Checklist

Further Comments

Uh oh!

Uh oh!

sgal Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sgal Feb 26, 2023

Choose a reason for hiding this comment

Uh oh!

aeneasr Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeneasr Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

aeneasr Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sgal Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

hperl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sgal commented Mar 7, 2023

Uh oh!

hperl left a comment

Choose a reason for hiding this comment

Uh oh!

kabirbaidhya commented Mar 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sgal commented Feb 26, 2023 •

edited

Loading

sgal Feb 26, 2023 •

edited

Loading

codecov bot commented Feb 28, 2023 •

edited

Loading