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

feat: add ability to revoke login sessions by SessionID #3450

Merged
merged 7 commits into from
Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 31 additions & 7 deletions consent/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ type listOAuth2ConsentSessions struct {
// in: query
// required: true
Subject string `json:"subject"`

// The login session id to list the consent sessions for.
//
// in: query
Expand Down Expand Up @@ -229,17 +230,28 @@ type revokeOAuth2LoginSessions struct {
// The subject to revoke authentication sessions for.
//
// in: query
// required: true
Subject string `json:"subject"`

// OAuth 2.0 Subject
//
// The subject to revoke authentication sessions for.
//
// in: query
SessionID string `json:"sid"`
}

// swagger:route DELETE /admin/oauth2/auth/sessions/login oAuth2 revokeOAuth2LoginSessions
//
// # Revokes All OAuth 2.0 Login Sessions of a Subject
// # Revokes OAuth 2.0 Login Sessions by either a Subject or a SessionID
//
// This endpoint invalidates authentication sessions. After revoking the authentication session(s), the subject
// has to re-authenticate at the Ory OAuth2 Provider. This endpoint does not invalidate any tokens.
//
// 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.
Copy link
Contributor Author

@sgal sgal Feb 26, 2023

Choose a reason for hiding this comment

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

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

//
// This endpoint invalidates a subject's authentication session. After revoking the authentication session, the subject
// 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.
// Alternatively, you can send a SessionID via `sid` query param, in which case, only the session that is connected
// to that SessionID is revoked. OpenID Connect Back-channel logout is performed in this case.
//
// Consumes:
// - application/json
Expand All @@ -253,9 +265,21 @@ type revokeOAuth2LoginSessions struct {
// 204: emptyResponse
// default: errorOAuth2
func (h *Handler) revokeOAuth2LoginSessions(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
sid := r.URL.Query().Get("sid")
subject := r.URL.Query().Get("subject")
if subject == "" {
h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'subject' is not defined but should have been.`)))

if sid == "" && subject == "" {
h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint(`Either 'subject' or 'sid' query parameters need to be defined.`)))
return
}

if sid != "" {
if err := h.r.ConsentStrategy().HandleHeadlessLogout(r.Context(), w, r, sid); err != nil {
h.r.Writer().WriteError(w, r, err)
return
}

w.WriteHeader(http.StatusNoContent)
return
}

Expand Down
1 change: 1 addition & 0 deletions consent/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ var _ Strategy = new(DefaultStrategy)
type Strategy interface {
HandleOAuth2AuthorizationRequest(ctx context.Context, w http.ResponseWriter, r *http.Request, req fosite.AuthorizeRequester) (*AcceptOAuth2ConsentRequest, error)
HandleOpenIDConnectLogout(ctx context.Context, w http.ResponseWriter, r *http.Request) (*LogoutResult, error)
HandleHeadlessLogout(ctx context.Context, w http.ResponseWriter, r *http.Request, sid string) error
hperl marked this conversation as resolved.
Show resolved Hide resolved
ObfuscateSubjectIdentifier(ctx context.Context, cl fosite.Client, subject, forcedIdentifier string) (string, error)
}
57 changes: 45 additions & 12 deletions consent/strategy_default.go
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,25 @@ func (s *DefaultStrategy) issueLogoutVerifier(ctx context.Context, w http.Respon
return nil, errorsx.WithStack(ErrAbortOAuth2Request)
}

func (s *DefaultStrategy) performBackChannelLogoutAndDeleteSession(ctx context.Context, r *http.Request, subject string, sid string) error {
if err := s.executeBackChannelLogout(r.Context(), r, subject, sid); err != nil {
return err
}

// We delete the session after back channel log out has worked as the session is otherwise removed
// from the store which will break the query for finding all the channels.
//
// executeBackChannelLogout only fails on system errors so not on URL errors, so this should be fine
// even if an upstream URL fails!
if err := s.r.ConsentManager().DeleteLoginSession(r.Context(), sid); errors.Is(err, sqlcon.ErrNoRows) {
// This is ok (session probably already revoked), do nothing!
} else if err != nil {
return err
}

return nil
}

func (s *DefaultStrategy) completeLogout(ctx context.Context, w http.ResponseWriter, r *http.Request) (*LogoutResult, error) {
verifier := r.URL.Query().Get("logout_verifier")

Expand Down Expand Up @@ -948,18 +967,7 @@ func (s *DefaultStrategy) completeLogout(ctx context.Context, w http.ResponseWri
return nil, err
}

if err := s.executeBackChannelLogout(r.Context(), r, lr.Subject, lr.SessionID); err != nil {
return nil, err
}

// We delete the session after back channel log out has worked as the session is otherwise removed
// from the store which will break the query for finding all the channels.
//
// executeBackChannelLogout only fails on system errors so not on URL errors, so this should be fine
// even if an upstream URL fails!
if err := s.r.ConsentManager().DeleteLoginSession(r.Context(), lr.SessionID); errors.Is(err, sqlcon.ErrNoRows) {
// This is ok (session probably already revoked), do nothing!
} else if err != nil {
if err := s.performBackChannelLogoutAndDeleteSession(r.Context(), r, lr.Subject, lr.SessionID); err != nil {
return nil, err
}

Expand All @@ -983,6 +991,31 @@ func (s *DefaultStrategy) HandleOpenIDConnectLogout(ctx context.Context, w http.
return s.completeLogout(ctx, w, r)
}

func (s *DefaultStrategy) HandleHeadlessLogout(ctx context.Context, w http.ResponseWriter, r *http.Request, sid string) error {
loginSession, lsErr := s.r.ConsentManager().GetRememberedLoginSession(ctx, sid)

if errors.Is(lsErr, x.ErrNotFound) {
// This is ok (session probably already revoked), do nothing!
// Not triggering the back-channel logout because subject is not available
// See https://github.com/ory/hydra/pull/3450#discussion_r1127798485
return nil
} else if lsErr != nil {
return lsErr
}

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

s.r.AuditLogger().
WithRequest(r).
WithField("subject", loginSession.Subject).
WithField("sid", sid).
Info("User logout completed via headless flow!")

return nil
}

func (s *DefaultStrategy) HandleOAuth2AuthorizationRequest(ctx context.Context, w http.ResponseWriter, r *http.Request, req fosite.AuthorizeRequester) (*AcceptOAuth2ConsentRequest, error) {
authenticationVerifier := strings.TrimSpace(req.GetRequestForm().Get("login_verifier"))
consentVerifier := strings.TrimSpace(req.GetRequestForm().Get("consent_verifier"))
Expand Down
73 changes: 68 additions & 5 deletions consent/strategy_logout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,29 @@ func TestLogoutFlows(t *testing.T) {
return string(ioutilx.MustReadAll(resp.Body)), resp
}

makeHeadlessLogoutRequest := func(t *testing.T, hc *http.Client, values url.Values) (body string, resp *http.Response) {
var err error
req, err := http.NewRequest(http.MethodDelete, adminTS.URL+"/admin/oauth2/auth/sessions/login?"+values.Encode(), nil)
aeneasr marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)

resp, err = hc.Do(req)

require.NoError(t, err)
defer resp.Body.Close()
return string(ioutilx.MustReadAll(resp.Body)), resp
}

logoutViaHeadlessAndExpectNoContent := func(t *testing.T, browser *http.Client, values url.Values) {
_, res := makeHeadlessLogoutRequest(t, browser, values)
assert.EqualValues(t, http.StatusNoContent, res.StatusCode)
}

logoutViaHeadlessAndExpectError := func(t *testing.T, browser *http.Client, values url.Values, expectedErrorMessage string) {
body, res := makeHeadlessLogoutRequest(t, browser, values)
assert.EqualValues(t, http.StatusBadRequest, res.StatusCode)
assert.Contains(t, body, expectedErrorMessage)
}

logoutAndExpectErrorPage := func(t *testing.T, browser *http.Client, method string, values url.Values, expectedErrorMessage string) {
body, res := makeLogoutRequest(t, browser, method, values)
assert.EqualValues(t, http.StatusInternalServerError, res.StatusCode)
Expand Down Expand Up @@ -153,7 +176,7 @@ func TestLogoutFlows(t *testing.T) {
reg.Config().MustSet(ctx, config.KeyLogoutURL, server.URL)
}

acceptLoginAsAndWatchSid := func(t *testing.T, subject string, sid chan string) {
acceptLoginAsAndWatchSidForConsumers := func(t *testing.T, subject string, sid chan string, remember bool, numSidConsumers int) {
testhelpers.NewLoginConsentUI(t, reg.Config(),
checkAndAcceptLoginHandler(t, adminApi, subject, func(t *testing.T, res *hydra.OAuth2LoginRequest, err error) hydra.AcceptOAuth2LoginRequest {
require.NoError(t, err)
Expand All @@ -164,16 +187,22 @@ func TestLogoutFlows(t *testing.T) {
require.NoError(t, err)
if sid != nil {
go func() {
sid <- *res.LoginSessionId
for i := 0; i < numSidConsumers; i++ {
aeneasr marked this conversation as resolved.
Show resolved Hide resolved
sid <- *res.LoginSessionId
}
}()
}
return hydra.AcceptOAuth2ConsentRequest{Remember: pointerx.Bool(true)}
return hydra.AcceptOAuth2ConsentRequest{Remember: pointerx.Bool(remember)}
}))

}

acceptLoginAsAndWatchSid := func(t *testing.T, subject string, sid chan string) {
acceptLoginAsAndWatchSidForConsumers(t, subject, sid, true, 1)
}

acceptLoginAs := func(t *testing.T, subject string) {
acceptLoginAsAndWatchSid(t, subject, nil)
acceptLoginAsAndWatchSidForConsumers(t, subject, nil, true, 0)
}

subject := "aeneas-rekkas"
Expand Down Expand Up @@ -399,7 +428,7 @@ func TestLogoutFlows(t *testing.T) {

t.Run("case=should pass rp-inititated flow without any action because SID is unknown", func(t *testing.T) {
c := createSampleClient(t)
acceptLoginAsAndWatchSid(t, subject, nil)
acceptLoginAs(t, subject)

checkAndAcceptLogout(t, nil, func(t *testing.T, res *hydra.OAuth2LogoutRequest, err error) {
t.Fatalf("Logout should not have been called")
Expand Down Expand Up @@ -490,4 +519,38 @@ func TestLogoutFlows(t *testing.T) {

wg.Wait()
})

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

backChannelWG := newWg(1)
c := createClientWithBackchannelLogout(t, backChannelWG, func(t *testing.T, logoutToken gjson.Result) {
assert.EqualValues(t, <-sid, logoutToken.Get("sid").String(), logoutToken.Raw)
sgal marked this conversation as resolved.
Show resolved Hide resolved
assert.Empty(t, logoutToken.Get("sub").String(), logoutToken.Raw) // The sub claim should be empty because it doesn't work with forced obfuscation and thus we can't easily recover it.
assert.Empty(t, logoutToken.Get("nonce").String(), logoutToken.Raw)
})

logoutViaHeadlessAndExpectNoContent(t, createBrowserWithSession(t, c), url.Values{"sid": {<-sid}})

backChannelWG.Wait() // we want to ensure that all back channels have been called!
})

t.Run("case=should logout in headless flow with non-existing sid", func(t *testing.T) {
logoutViaHeadlessAndExpectNoContent(t, browserWithoutSession, url.Values{"sid": {"non-existing-sid"}})
})

t.Run("case=should logout in headless flow with session that has remember=false", func(t *testing.T) {
sid := make(chan string)
acceptLoginAsAndWatchSidForConsumers(t, subject, sid, false, 1)

c := createSampleClient(t)

logoutViaHeadlessAndExpectNoContent(t, createBrowserWithSession(t, c), url.Values{"sid": {<-sid}})
})

t.Run("case=should fail headless logout because neither sid nor subject were provided", func(t *testing.T) {
logoutViaHeadlessAndExpectError(t, browserWithoutSession, url.Values{}, `Either 'subject' or 'sid' query parameters need to be defined.`)
})
}
4 changes: 4 additions & 0 deletions oauth2/oauth2_helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ func (c *consentMock) HandleOpenIDConnectLogout(ctx context.Context, w http.Resp
panic("not implemented")
}

func (c *consentMock) HandleHeadlessLogout(ctx context.Context, w http.ResponseWriter, r *http.Request, sid string) error {
panic("not implemented")
}

func (c *consentMock) ObfuscateSubjectIdentifier(ctx context.Context, cl fosite.Client, subject, forcedIdentifier string) (string, error) {
if c, ok := cl.(*client.Client); ok && c.SubjectType == "pairwise" {
panic("not implemented")
Expand Down