From ccf238863d381227a04229f5f4eb8c11bb8153a9 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Thu, 10 Nov 2022 10:20:58 +0100 Subject: [PATCH] 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. --- consent/manager_test_helpers.go | 51 +++++++++++++++++++ .../20220210000001000000_nid.sqlite.up.sql | 2 +- ...0000000_fix_foreign_key.cockroach.down.sql | 1 + ...10000000_fix_foreign_key.cockroach.up.sql | 3 ++ ...00010000000_fix_foreign_key.mysql.down.sql | 1 + ...9000010000000_fix_foreign_key.mysql.up.sql | 3 ++ ...10000000_fix_foreign_key.postgres.down.sql | 1 + ...010000000_fix_foreign_key.postgres.up.sql | 3 ++ ...0010000000_fix_foreign_key.sqlite.down.sql | 1 + ...000010000000_fix_foreign_key.sqlite.up.sql | 1 + persistence/sql/persister_consent.go | 8 +-- .../20220210000001000000_nid.sqlite.up.sql | 2 +- 12 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.cockroach.down.sql create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.cockroach.up.sql create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.mysql.down.sql create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.mysql.up.sql create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.postgres.down.sql create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.postgres.up.sql create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.sqlite.down.sql create mode 100644 persistence/sql/migrations/20221109000010000000_fix_foreign_key.sqlite.up.sql diff --git a/consent/manager_test_helpers.go b/consent/manager_test_helpers.go index 85a0f8b2b71..869c5aceb32 100644 --- 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 index 4c2728d6a26..7ef7791a371 100644 --- 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 new file mode 100644 index 00000000000..e9d3f3c24fe --- /dev/null +++ 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 new file mode 100644 index 00000000000..02ba8a741d5 --- /dev/null +++ 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 new file mode 100644 index 00000000000..e9d3f3c24fe --- /dev/null +++ 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 new file mode 100644 index 00000000000..2aa1368e628 --- /dev/null +++ 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 new file mode 100644 index 00000000000..e9d3f3c24fe --- /dev/null +++ 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 new file mode 100644 index 00000000000..02ba8a741d5 --- /dev/null +++ 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 new file mode 100644 index 00000000000..4ba280517af --- /dev/null +++ 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 new file mode 100644 index 00000000000..4ba280517af --- /dev/null +++ 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 index 6372a99f5e1..d99d18a78d0 100644 --- 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 index 71765b2c0bc..eb9af4106ed 100644 --- 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,