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 expiry and requested times to logout table #3837

Merged
merged 7 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions consent/strategy_default.go
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,8 @@ func (s *DefaultStrategy) issueLogoutVerifier(ctx context.Context, w http.Respon
Subject: session.Subject,
SessionID: session.ID,
Verifier: uuid.New(),
RequestedAt: sqlxx.NullTime(time.Now().UTC().Round(time.Second)),
ExpiresAt: sqlxx.NullTime(time.Now().UTC().Round(time.Second).Add(s.c.ConsentRequestMaxAge(ctx))),
RPInitiated: false,

// PostLogoutRedirectURI is set to the value from config.Provider().LogoutRedirectURL()
Expand Down
2 changes: 1 addition & 1 deletion flow/.snapshots/TestLogoutRequest_MarshalJSON.json
Original file line number Diff line number Diff line change
@@ -1 +1 @@
"{\"challenge\":\"\",\"subject\":\"\",\"request_url\":\"\",\"rp_initiated\":false,\"client\":null}"
"{\"challenge\":\"\",\"subject\":\"\",\"request_url\":\"\",\"rp_initiated\":false,\"expires_at\":null,\"requested_at\":null,\"client\":null}"
2 changes: 2 additions & 0 deletions flow/consent_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,8 @@ type LogoutRequest struct {
Accepted bool `json:"-" db:"accepted"`
Rejected bool `db:"rejected" json:"-"`
ClientID sql.NullString `json:"-" db:"client_id"`
ExpiresAt sqlxx.NullTime `json:"expires_at" db:"expires_at"`
RequestedAt sqlxx.NullTime `json:"requested_at" db:"requested_at"`
Client *client.Client `json:"client" db:"-"`
}

Expand Down
8 changes: 8 additions & 0 deletions flow/error.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Copyright © 2024 Ory Corp
// SPDX-License-Identifier: Apache-2.0

package flow

import "github.com/ory/fosite"

var ErrorLogoutFlowExpired = fosite.ErrRequestUnauthorized.WithHint("The logout request has expired, please try the flow again.")
347 changes: 347 additions & 0 deletions internal/httpclient/go.sum

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@
"sid": "session_id-0009",
"request_url": "http://request/0009",
"rp_initiated": true,
"expires_at": null,
"requested_at": null,
"client": null
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@
"sid": "session_id-0010",
"request_url": "http://request/0010",
"rp_initiated": true,
"expires_at": null,
"requested_at": null,
"client": null
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@
"sid": "session_id-0011",
"request_url": "http://request/0011",
"rp_initiated": true,
"expires_at": null,
"requested_at": null,
"client": null
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@
"sid": "session_id-0012",
"request_url": "http://request/0012",
"rp_initiated": true,
"expires_at": null,
"requested_at": null,
"client": null
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@
"sid": "session_id-0013",
"request_url": "http://request/0013",
"rp_initiated": true,
"expires_at": null,
"requested_at": null,
"client": null
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@
"sid": "session_id-0014",
"request_url": "http://request/0014",
"rp_initiated": true,
"expires_at": null,
"requested_at": null,
"client": null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"challenge": "challenge-20240916105610000001",
"subject": "subject-0014",
"sid": "session_id-0014",
"request_url": "http://request/0014",
"rp_initiated": true,
"expires_at": null,
"requested_at": null,
"client": null
}
2 changes: 1 addition & 1 deletion persistence/sql/migratest/migration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ func TestMigrations(t *testing.T) {
t.Run("case=hydra_oauth2_logout_request", func(t *testing.T) {
lrs := []flow.LogoutRequest{}
c.All(&lrs)
require.Equal(t, 6, len(lrs))
require.Equal(t, 7, len(lrs))

for _, s := range lrs {
testhelpersuuid.AssertUUID(t, s.NID)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
INSERT INTO hydra_oauth2_logout_request (challenge, verifier, subject, sid, client_id, nid, request_url, redir_url,
was_used, accepted, rejected, rp_initiated, expires_at, requested_at)
VALUES ('challenge-20240916105610000001', 'verifier-20240916105610000001', 'subject-0014', 'session_id-0014', 'client-0014',
(SELECT id FROM networks LIMIT 1), 'http://request/0014', 'http://post_logout/0014', true, true, false, true, 'now()', 'now()');
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE hydra_oauth2_logout_request DROP expires_at;
ALTER TABLE hydra_oauth2_logout_request DROP requested_at;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE hydra_oauth2_logout_request ADD expires_at timestamp NULL;
ALTER TABLE hydra_oauth2_logout_request ADD requested_at timestamp NULL;
7 changes: 7 additions & 0 deletions persistence/sql/persister_consent.go
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,13 @@ WHERE nid = ?
return nil, err
}

if expiry := time.Time(lr.ExpiresAt);
// If the expiry is unset, we are in a legacy use case (allow logout).
// TODO: Remove this in the future.
!expiry.IsZero() && expiry.Before(time.Now().UTC()) {
return nil, errorsx.WithStack(flow.ErrorLogoutFlowExpired)
}

return &lr, nil
}

Expand Down
63 changes: 45 additions & 18 deletions persistence/sql/persister_nid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2071,27 +2071,54 @@ func (s *PersisterTestSuite) TestVerifyAndInvalidateLogoutRequest() {
t := s.T()
for k, r := range s.registries {
t.Run(k, func(t *testing.T) {
lr := newLogoutRequest()
lr.Verifier = uuid.Must(uuid.NewV4()).String()
lr.Accepted = true
lr.Rejected = false
require.NoError(t, r.ConsentManager().CreateLogoutRequest(s.t1, lr))
run := func(t *testing.T, lr *flow.LogoutRequest) {
lr.Verifier = uuid.Must(uuid.NewV4()).String()
lr.Accepted = true
lr.Rejected = false
require.NoError(t, r.ConsentManager().CreateLogoutRequest(s.t1, lr))

expected, err := r.ConsentManager().GetLogoutRequest(s.t1, lr.ID)
require.NoError(t, err)

lrInvalidated, err := r.ConsentManager().VerifyAndInvalidateLogoutRequest(s.t2, lr.Verifier)
require.Error(t, err)
require.Nil(t, lrInvalidated)
actual := &flow.LogoutRequest{}
require.NoError(t, r.Persister().Connection(context.Background()).Find(actual, lr.ID))
require.Equal(t, expected, actual)

lrInvalidated, err = r.ConsentManager().VerifyAndInvalidateLogoutRequest(s.t1, lr.Verifier)
require.NoError(t, err)
require.NoError(t, r.Persister().Connection(context.Background()).Find(actual, lr.ID))
require.Equal(t, lrInvalidated, actual)
require.Equal(t, true, actual.WasHandled)
}

expected, err := r.ConsentManager().GetLogoutRequest(s.t1, lr.ID)
require.NoError(t, err)
t.Run("case=legacy logout request without expiry", func(t *testing.T) {
lr := newLogoutRequest()
run(t, lr)
})

lrInvalidated, err := r.ConsentManager().VerifyAndInvalidateLogoutRequest(s.t2, lr.Verifier)
require.Error(t, err)
require.Nil(t, lrInvalidated)
actual := &flow.LogoutRequest{}
require.NoError(t, r.Persister().Connection(context.Background()).Find(actual, lr.ID))
require.Equal(t, expected, actual)
t.Run("case=logout request with expiry", func(t *testing.T) {
lr := newLogoutRequest()
lr.ExpiresAt = sqlxx.NullTime(time.Now().Add(time.Hour))
run(t, lr)
})

lrInvalidated, err = r.ConsentManager().VerifyAndInvalidateLogoutRequest(s.t1, lr.Verifier)
require.NoError(t, err)
require.NoError(t, r.Persister().Connection(context.Background()).Find(actual, lr.ID))
require.Equal(t, lrInvalidated, actual)
require.Equal(t, true, actual.WasHandled)
t.Run("case=logout request that expired returns error", func(t *testing.T) {
lr := newLogoutRequest()
lr.ExpiresAt = sqlxx.NullTime(time.Now().Add(-time.Hour))
lr.Verifier = uuid.Must(uuid.NewV4()).String()
lr.Accepted = true
lr.Rejected = false
require.NoError(t, r.ConsentManager().CreateLogoutRequest(s.t1, lr))

_, err := r.ConsentManager().VerifyAndInvalidateLogoutRequest(s.t2, lr.Verifier)
require.ErrorIs(t, err, x.ErrNotFound)

_, err = r.ConsentManager().VerifyAndInvalidateLogoutRequest(s.t1, lr.Verifier)
require.ErrorIs(t, err, flow.ErrorLogoutFlowExpired)
})
})
}
}
Expand Down
Loading