feat: add expiry and requested times to logout table (#3837)

aeneasr · web-flow · commit f83193f90814 · 2024-09-16T16:09:33.000+02:00
diff --git a/consent/strategy_default.go b/consent/strategy_default.go
@@ -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()
diff --git a/flow/.snapshots/TestLogoutRequest_MarshalJSON.json b/flow/.snapshots/TestLogoutRequest_MarshalJSON.json
@@ -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}"
diff --git a/flow/consent_types.go b/flow/consent_types.go
@@ -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:"-"`
 }
 
diff --git a/flow/error.go b/flow/error.go
@@ -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.")
diff --git a/internal/httpclient/go.sum b/internal/httpclient/go.sum
diff --git a/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0009.json b/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0009.json
@@ -4,5 +4,7 @@
   "sid": "session_id-0009",
   "request_url": "http://request/0009",
   "rp_initiated": true,
+  "expires_at": null,
+  "requested_at": null,
   "client": null
 }
diff --git a/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0010.json b/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0010.json
@@ -4,5 +4,7 @@
   "sid": "session_id-0010",
   "request_url": "http://request/0010",
   "rp_initiated": true,
+  "expires_at": null,
+  "requested_at": null,
   "client": null
 }
diff --git a/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0011.json b/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0011.json
@@ -4,5 +4,7 @@
   "sid": "session_id-0011",
   "request_url": "http://request/0011",
   "rp_initiated": true,
+  "expires_at": null,
+  "requested_at": null,
   "client": null
 }
diff --git a/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0012.json b/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0012.json
@@ -4,5 +4,7 @@
   "sid": "session_id-0012",
   "request_url": "http://request/0012",
   "rp_initiated": true,
+  "expires_at": null,
+  "requested_at": null,
   "client": null
 }
diff --git a/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0013.json b/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0013.json
@@ -4,5 +4,7 @@
   "sid": "session_id-0013",
   "request_url": "http://request/0013",
   "rp_initiated": true,
+  "expires_at": null,
+  "requested_at": null,
   "client": null
 }
diff --git a/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0014.json b/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-0014.json
@@ -4,5 +4,7 @@
   "sid": "session_id-0014",
   "request_url": "http://request/0014",
   "rp_initiated": true,
+  "expires_at": null,
+  "requested_at": null,
   "client": null
 }
diff --git a/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-20240916105610000001.json b/persistence/sql/migratest/fixtures/hydra_oauth2_logout_request/challenge-20240916105610000001.json
@@ -0,0 +1,10 @@
+{
+  "challenge": "challenge-20240916105610000001",
+  "subject": "subject-0014",
+  "sid": "session_id-0014",
+  "request_url": "http://request/0014",
+  "rp_initiated": true,
+  "expires_at": "2022-02-15T22:20:20Z",
+  "requested_at": "2022-02-15T22:20:20Z",
+  "client": null
+}
diff --git a/persistence/sql/migratest/migration_test.go b/persistence/sql/migratest/migration_test.go
@@ -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)
diff --git a/persistence/sql/migratest/testdata/20230313112801_testdata.sql b/persistence/sql/migratest/testdata/20230313112801_testdata.sql
diff --git a/persistence/sql/migratest/testdata/20230809122501_testdata.sql b/persistence/sql/migratest/testdata/20230809122501_testdata.sql
diff --git a/persistence/sql/migratest/testdata/20240916105610_testdata.sql b/persistence/sql/migratest/testdata/20240916105610_testdata.sql
@@ -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, '2022-02-15 22:20:20', '2022-02-15 22:20:20');
diff --git a/persistence/sql/migrations/20240916105610000001_add_logout_request_timestamps.down.sql b/persistence/sql/migrations/20240916105610000001_add_logout_request_timestamps.down.sql
@@ -0,0 +1,2 @@
+ALTER TABLE hydra_oauth2_logout_request DROP expires_at;
+ALTER TABLE hydra_oauth2_logout_request DROP requested_at;
diff --git a/persistence/sql/migrations/20240916105610000001_add_logout_request_timestamps.up.sql b/persistence/sql/migrations/20240916105610000001_add_logout_request_timestamps.up.sql
@@ -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;
diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go
@@ -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
 }
 
diff --git a/persistence/sql/persister_nid_test.go b/persistence/sql/persister_nid_test.go
@@ -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)
+			})
 		})
 	}
 }

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-"{\"challenge\":\"\",\"subject\":\"\",\"request_url\":\"\",\"rp_initiated\":false,\"client\":null}"`
	`1`	`+"{\"challenge\":\"\",\"subject\":\"\",\"request_url\":\"\",\"rp_initiated\":false,\"expires_at\":null,\"requested_at\":null,\"client\":null}"`
Original file line number	Diff line number	Diff line change
`@@ -507,6 +507,8 @@ type LogoutRequest struct {`
`507`	`507`	Accepted bool `json:"-" db:"accepted"`
`508`	`508`	Rejected bool `db:"rejected" json:"-"`
`509`	`509`	ClientID sql.NullString `json:"-" db:"client_id"`
	`510`	+ ExpiresAt sqlxx.NullTime `json:"expires_at" db:"expires_at"`
	`511`	+ RequestedAt sqlxx.NullTime `json:"requested_at" db:"requested_at"`
`510`	`512`	Client *client.Client `json:"client" db:"-"`
`511`	`513`	`}`
`512`	`514`
Original file line number	Diff line number	Diff line change
`@@ -4,5 +4,7 @@`
`4`	`4`	`"sid": "session_id-0009",`
`5`	`5`	`"request_url": "http://request/0009",`
`6`	`6`	`"rp_initiated": true,`
	`7`	`+ "expires_at": null,`
	`8`	`+ "requested_at": null,`
`7`	`9`	`"client": null`
`8`	`10`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,5 +4,7 @@`
`4`	`4`	`"sid": "session_id-0010",`
`5`	`5`	`"request_url": "http://request/0010",`
`6`	`6`	`"rp_initiated": true,`
	`7`	`+ "expires_at": null,`
	`8`	`+ "requested_at": null,`
`7`	`9`	`"client": null`
`8`	`10`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,5 +4,7 @@`
`4`	`4`	`"sid": "session_id-0011",`
`5`	`5`	`"request_url": "http://request/0011",`
`6`	`6`	`"rp_initiated": true,`
	`7`	`+ "expires_at": null,`
	`8`	`+ "requested_at": null,`
`7`	`9`	`"client": null`
`8`	`10`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,5 +4,7 @@`
`4`	`4`	`"sid": "session_id-0012",`
`5`	`5`	`"request_url": "http://request/0012",`
`6`	`6`	`"rp_initiated": true,`
	`7`	`+ "expires_at": null,`
	`8`	`+ "requested_at": null,`
`7`	`9`	`"client": null`
`8`	`10`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,5 +4,7 @@`
`4`	`4`	`"sid": "session_id-0013",`
`5`	`5`	`"request_url": "http://request/0013",`
`6`	`6`	`"rp_initiated": true,`
	`7`	`+ "expires_at": null,`
	`8`	`+ "requested_at": null,`
`7`	`9`	`"client": null`
`8`	`10`	`}`