fix: incorrect consent removal on authentication revokation

This patch resolves a regression where, in a certain condition, an accepted consent could be incorrectly deleted when the related authentication session was removed.
ory · Nov 10, 2022 · ccf2388 · ccf2388
1 parent cec489f
commit ccf2388
Show file tree

Hide file tree

Showing 12 changed files with 71 additions and 6 deletions.
diff --git a/consent/manager_test_helpers.go b/consent/manager_test_helpers.go
@@ -886,6 +886,57 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 				}
 			})
 		})
+
+		t.Run("case=foreign key regression", func(t *testing.T) {
+			cl := &client.Client{LegacyClientID: uuid.New().String()}
+			require.NoError(t, clientManager.CreateClient(context.Background(), cl))
+
+			subject := uuid.New().String()
+			s := LoginSession{
+				ID:              uuid.New().String(),
+				AuthenticatedAt: sqlxx.NullTime(time.Now().Round(time.Minute).Add(-time.Minute).UTC()),
+				Subject:         subject,
+			}
+
+			err := m.CreateLoginSession(context.Background(), &s)
+			require.NoError(t, err)
+
+			lr := &LoginRequest{
+				ID:              uuid.New().String(),
+				Subject:         uuid.New().String(),
+				Verifier:        uuid.New().String(),
+				Client:          cl,
+				AuthenticatedAt: sqlxx.NullTime(time.Now()),
+				RequestedAt:     time.Now(),
+				SessionID:       sqlxx.NullString(s.ID),
+			}
+
+			require.NoError(t, m.CreateLoginRequest(context.Background(), lr))
+			expected := &OAuth2ConsentRequest{
+				ID:                   uuid.New().String(),
+				Skip:                 true,
+				Subject:              subject,
+				OpenIDConnectContext: nil,
+				Client:               cl,
+				ClientID:             cl.LegacyClientID,
+				RequestURL:           "",
+				LoginChallenge:       sqlxx.NullString(lr.ID),
+				LoginSessionID:       sqlxx.NullString(s.ID),
+				Verifier:             uuid.New().String(),
+				CSRF:                 uuid.New().String(),
+			}
+			require.NoError(t, m.CreateConsentRequest(context.Background(), expected))
+
+			result, err := m.GetConsentRequest(context.Background(), expected.ID)
+			require.NoError(t, err)
+			assert.EqualValues(t, expected.ID, result.ID)
+
+			require.NoError(t, m.DeleteLoginSession(context.Background(), s.ID))
+
+			result, err = m.GetConsentRequest(context.Background(), expected.ID)
+			require.NoError(t, err)
+			assert.EqualValues(t, expected.ID, result.ID)
+		})
 	}
 }
 

diff --git a/persistence/sql/migrations/20220210000001000000_nid.sqlite.up.sql b/persistence/sql/migrations/20220210000001000000_nid.sqlite.up.sql
@@ -261,7 +261,7 @@ CREATE TABLE "_hydra_oauth2_flow_tmp" (
     client_id                 VARCHAR(255) NOT NULL,
     requested_at              TIMESTAMP    NOT NULL DEFAULT CURRENT_TIMESTAMP,
     oidc_context              TEXT         NOT NULL,
-    login_session_id          VARCHAR(40)  NULL REFERENCES hydra_oauth2_authentication_session (id) ON DELETE CASCADE DEFAULT '',
+    login_session_id          VARCHAR(40)  NULL REFERENCES hydra_oauth2_authentication_session (id) ON DELETE SET NULL,
     requested_at_audience     TEXT         NULL DEFAULT '',
     login_initialized_at      TIMESTAMP    NULL,
 

diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.cockroach.down.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.cockroach.down.sql
@@ -0,0 +1 @@
+ALTER TABLE ONLY hydra_oauth2_flow ALTER COLUMN login_session_id SET DEFAULT '';
diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.cockroach.up.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.cockroach.up.sql
@@ -0,0 +1,3 @@
+ALTER TABLE hydra_oauth2_flow DROP CONSTRAINT hydra_oauth2_flow_login_session_id_fk;
+ALTER TABLE hydra_oauth2_flow ADD CONSTRAINT hydra_oauth2_flow_login_session_id_fk FOREIGN KEY (login_session_id) REFERENCES public.hydra_oauth2_authentication_session(id) ON DELETE SET NULL;
+ALTER TABLE hydra_oauth2_flow ALTER COLUMN login_session_id DROP DEFAULT;
diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.mysql.down.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.mysql.down.sql
@@ -0,0 +1 @@
+ALTER TABLE ONLY hydra_oauth2_flow ALTER COLUMN login_session_id SET DEFAULT '';
diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.mysql.up.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.mysql.up.sql
@@ -0,0 +1,3 @@
+ALTER TABLE hydra_oauth2_flow DROP CONSTRAINT hydra_oauth2_flow_login_session_id_fk;
+ALTER TABLE hydra_oauth2_flow ADD CONSTRAINT hydra_oauth2_flow_login_session_id_fk FOREIGN KEY (login_session_id) REFERENCES hydra_oauth2_authentication_session(id) ON DELETE SET NULL;
+ALTER TABLE hydra_oauth2_flow ALTER COLUMN login_session_id DROP DEFAULT;
diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.postgres.down.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.postgres.down.sql
@@ -0,0 +1 @@
+ALTER TABLE ONLY hydra_oauth2_flow ALTER COLUMN login_session_id SET DEFAULT '';
diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.postgres.up.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.postgres.up.sql
@@ -0,0 +1,3 @@
+ALTER TABLE hydra_oauth2_flow DROP CONSTRAINT hydra_oauth2_flow_login_session_id_fk;
+ALTER TABLE hydra_oauth2_flow ADD CONSTRAINT hydra_oauth2_flow_login_session_id_fk FOREIGN KEY (login_session_id) REFERENCES public.hydra_oauth2_authentication_session(id) ON DELETE SET NULL;
+ALTER TABLE hydra_oauth2_flow ALTER COLUMN login_session_id DROP DEFAULT;
diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.sqlite.down.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.sqlite.down.sql
@@ -0,0 +1 @@
+--
diff --git a/persistence/sql/migrations/20221109000010000000_fix_foreign_key.sqlite.up.sql b/persistence/sql/migrations/20221109000010000000_fix_foreign_key.sqlite.up.sql
@@ -0,0 +1 @@
+--
diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go
@@ -225,13 +225,13 @@ func (p *Persister) CreateLoginRequest(ctx context.Context, req *consent.LoginRe
 	return sqlcon.HandleError(p.CreateWithNetwork(ctx, f))
 }
 
-func (p *Persister) GetFlow(ctx context.Context, login_challenge string) (*flow.Flow, error) {
+func (p *Persister) GetFlow(ctx context.Context, loginChallenge string) (*flow.Flow, error) {
 	ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.GetFlow")
 	defer span.End()
 
 	var f flow.Flow
 	return &f, p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
-		if err := p.QueryWithNetwork(ctx).Where("login_challenge = ?", login_challenge).First(&f); err != nil {
+		if err := p.QueryWithNetwork(ctx).Where("login_challenge = ?", loginChallenge).First(&f); err != nil {
 			if errors.Is(err, sql.ErrNoRows) {
 				return errorsx.WithStack(x.ErrNotFound)
 			}
@@ -242,14 +242,14 @@ func (p *Persister) GetFlow(ctx context.Context, login_challenge string) (*flow.
 	})
 }
 
-func (p *Persister) GetLoginRequest(ctx context.Context, login_challenge string) (*consent.LoginRequest, error) {
+func (p *Persister) GetLoginRequest(ctx context.Context, loginChallenge string) (*consent.LoginRequest, error) {
 	ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.GetLoginRequest")
 	defer span.End()
 
 	var lr *consent.LoginRequest
 	return lr, p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
 		var f flow.Flow
-		if err := p.QueryWithNetwork(ctx).Where("login_challenge = ?", login_challenge).First(&f); err != nil {
+		if err := p.QueryWithNetwork(ctx).Where("login_challenge = ?", loginChallenge).First(&f); err != nil {
 			if errors.Is(err, sql.ErrNoRows) {
 				return errorsx.WithStack(x.ErrNotFound)
 			}

diff --git a/persistence/sql/src/20220210000001_nid/20220210000001000000_nid.sqlite.up.sql b/persistence/sql/src/20220210000001_nid/20220210000001000000_nid.sqlite.up.sql
@@ -259,7 +259,7 @@ CREATE TABLE "_hydra_oauth2_flow_tmp" (
     client_id                 VARCHAR(255) NOT NULL,
     requested_at              TIMESTAMP    NOT NULL DEFAULT CURRENT_TIMESTAMP,
     oidc_context              TEXT         NOT NULL,
-    login_session_id          VARCHAR(40)  NULL REFERENCES hydra_oauth2_authentication_session (id) ON DELETE CASCADE DEFAULT '',
+    login_session_id          VARCHAR(40)  NULL REFERENCES hydra_oauth2_authentication_session (id) ON DELETE SET NULL,
     requested_at_audience     TEXT         NULL DEFAULT '',
     login_initialized_at      TIMESTAMP    NULL,