From b4732579cb020050de5861b9f07c376ca2831344 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Wed, 3 Dec 2025 17:15:49 +0100 Subject: [PATCH] fix(RT): correct restricting null columns (#11123) # Which Problems Are Solved `IS NULL` and `IS NOT NULL` conditions were incorrectly marked as restricting columns, and currently fails unit tests. The restricting columns are mostly used to ensure single row selection during DELETE and UPDATE . When a `IS NOT NULL` condition is passed, the filter may (unintentionally) apply to all rows, while still allowing the query to execute. Settings and other repositories (still to be done) have unified tables for instance-level settings and organization-level settings. In those cases the organization ID may be null or have a value. The unique condition is a compound of multiple columns, in which case `IS NULL` is valid as part of the other restricting columns. # How the Problems Are Solved In order to prevent unsafe usage of null related filters and not break settings: - null conditions are no longer considered restricting columns - A function to force the restricting column state on a condition is provided - The settings unique condition function forces restricting column state on the returned condition. # Additional Changes - add `backend/v3` to the NX build. # Additional Context - [pipeline failures](https://github.com/zitadel/zitadel/actions/runs/19852295780/job/56881654340?pr=11115#step:13:3197) --------- Co-authored-by: Elio Bischof --- apps/api/project.json | 1 + backend/v3/storage/database/condition.go | 26 +++++- backend/v3/storage/database/condition_test.go | 20 ++--- .../events_testing/settings_instance_test.go | 12 +-- .../storage/database/repository/settings.go | 18 ++--- .../database/repository/settings_test.go | 80 ++++++++++++------- .../query/projection/settings_relational.go | 6 +- 7 files changed, 99 insertions(+), 64 deletions(-) diff --git a/apps/api/project.json b/apps/api/project.json index d360ef155..32fd798ed 100644 --- a/apps/api/project.json +++ b/apps/api/project.json @@ -6,6 +6,7 @@ "sources": [ "{workspaceRoot}/cmd/**/*.go", "{workspaceRoot}/internal/**/*.go", + "{workspaceRoot}/backend/**/*.go", "{workspaceRoot}/proto/**/*.go", "{workspaceRoot}/pkg/**/*.go", "{workspaceRoot}/main.go" diff --git a/backend/v3/storage/database/condition.go b/backend/v3/storage/database/condition.go index b000018d0..4d9ada49a 100644 --- a/backend/v3/storage/database/condition.go +++ b/backend/v3/storage/database/condition.go @@ -1,6 +1,8 @@ package database -import "go.uber.org/mock/gomock" +import ( + "go.uber.org/mock/gomock" +) // Condition represents a SQL condition. // Its written after the WHERE keyword in a SQL statement. @@ -158,7 +160,7 @@ func IsNull(column Column) *isNull { // IsRestrictingColumn implements [Condition]. func (i isNull) IsRestrictingColumn(col Column) bool { - return true + return false } var _ Condition = (*isNull)(nil) @@ -194,7 +196,7 @@ func IsNotNull(column Column) *isNotNull { // IsRestrictingColumn implements [Condition]. func (i isNotNull) IsRestrictingColumn(col Column) bool { - return true + return false } var _ Condition = (*isNotNull)(nil) @@ -344,3 +346,21 @@ func (e existsCondition) IsRestrictingColumn(col Column) bool { } var _ Condition = (*existsCondition)(nil) + +type forcedRestrictingColumnCondition struct { + Condition + isRestricting bool +} + +func (c forcedRestrictingColumnCondition) IsRestrictingColumn(col Column) bool { + return c.isRestricting +} + +func ForceRestrictingColumn(cond Condition, isRestriction bool) Condition { + return forcedRestrictingColumnCondition{ + Condition: cond, + isRestricting: isRestriction, + } +} + +var _ Condition = (*forcedRestrictingColumnCondition)(nil) diff --git a/backend/v3/storage/database/condition_test.go b/backend/v3/storage/database/condition_test.go index 4ac1c78ae..01dac0b4d 100644 --- a/backend/v3/storage/database/condition_test.go +++ b/backend/v3/storage/database/condition_test.go @@ -174,13 +174,13 @@ func TestWrite(t *testing.T) { func TestIsRestrictingColumn(t *testing.T) { for _, test := range []struct { name string - col Column + arg Column cond Condition want bool }{ { name: "and with restricting column", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: And( NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column2")), NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column3")), @@ -189,7 +189,7 @@ func TestIsRestrictingColumn(t *testing.T) { }, { name: "and without restricting column", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: And( NewColumnCondition(NewColumn("table", "column2"), NewColumn("other_table", "column3")), IsNull(NewColumn("table", "column4")), @@ -199,7 +199,7 @@ func TestIsRestrictingColumn(t *testing.T) { }, { name: "or with restricting column", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: Or( NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column2")), NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column3")), @@ -208,7 +208,7 @@ func TestIsRestrictingColumn(t *testing.T) { }, { name: "or without restricting column", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: Or( NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column3")), IsNotNull(NewColumn("table", "column4")), @@ -218,19 +218,19 @@ func TestIsRestrictingColumn(t *testing.T) { }, { name: "is null never restricts", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: IsNull(NewColumn("table", "column1")), want: false, }, { name: "is not null never restricts", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: IsNotNull(NewColumn("table", "column1")), want: false, }, { name: "exists with restricting column", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: Exists("table", And( NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column2")), NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column3")), @@ -239,7 +239,7 @@ func TestIsRestrictingColumn(t *testing.T) { }, { name: "exists without restricting column", - col: NewColumn("table", "column1"), + arg: NewColumn("table", "column1"), cond: Exists("table", Or( NewColumnCondition(NewColumn("table", "column1"), NewColumn("other_table", "column3")), IsNotNull(NewColumn("table", "column4")), @@ -249,7 +249,7 @@ func TestIsRestrictingColumn(t *testing.T) { }, } { t.Run(test.name, func(t *testing.T) { - isRestricting := test.cond.IsRestrictingColumn(test.col) + isRestricting := test.cond.IsRestrictingColumn(test.arg) assert.Equal(t, test.want, isRestricting) }) } diff --git a/backend/v3/storage/database/events_testing/settings_instance_test.go b/backend/v3/storage/database/events_testing/settings_instance_test.go index 87a9ce595..239626ad5 100644 --- a/backend/v3/storage/database/events_testing/settings_instance_test.go +++ b/backend/v3/storage/database/events_testing/settings_instance_test.go @@ -326,9 +326,9 @@ func TestServer_TestInstanceLabelSettingsReduces(t *testing.T) { settingsRepo := repository.BrandingSettingsRepository() - before := time.Now() + before := time.Now().Add(-time.Second) newInstance := integration.NewInstance(t.Context()) - after := time.Now() + after := time.Now().Add(time.Second) IAMCTX := newInstance.WithAuthorizationToken(t.Context(), integration.UserTypeIAMOwner) @@ -413,10 +413,10 @@ func TestServer_TestInstanceLabelSettingsReduces(t *testing.T) { t.Run("test label settings activated", func(t *testing.T) { // activate label - before := time.Now() + before := time.Now().Add(-time.Second) _, err := newInstance.Client.Admin.ActivateLabelPolicy(IAMCTX, &admin.ActivateLabelPolicyRequest{}) require.NoError(t, err) - after := time.Now() + after := time.Now().Add(time.Second) retryDuration, tick := integration.WaitForAndTickWithMaxDuration(IAMCTX, time.Second*20) assert.EventuallyWithT(t, func(collect *assert.CollectT) { @@ -436,9 +436,9 @@ func TestServer_TestInstanceLabelSettingsReduces(t *testing.T) { t.Run("test policy label logo light added", func(t *testing.T) { // set logo light - before := time.Now() + before := time.Now().Add(-time.Second) uploadInstanceAsset(IAMCTX, t, newInstance, "/instance/policy/label/logo", bytes.NewReader(picture)) - after := time.Now() + after := time.Now().Add(time.Second) retryDuration, tick := integration.WaitForAndTickWithMaxDuration(CTX, time.Second*20) assert.EventuallyWithT(t, func(collect *assert.CollectT) { diff --git a/backend/v3/storage/database/repository/settings.go b/backend/v3/storage/database/repository/settings.go index 5496e1811..0af22e94e 100644 --- a/backend/v3/storage/database/repository/settings.go +++ b/backend/v3/storage/database/repository/settings.go @@ -86,11 +86,14 @@ func (s settings) PrimaryKeyCondition(instanceID, id string) database.Condition } func (s settings) UniqueCondition(instanceID string, orgID *string, typ domain.SettingType, state domain.SettingState) database.Condition { - return database.And( - s.InstanceIDCondition(instanceID), - s.OrganizationIDCondition(orgID), - s.TypeCondition(typ), - s.StateCondition(state), + return database.ForceRestrictingColumn( + database.And( + s.InstanceIDCondition(instanceID), + s.OrganizationIDCondition(orgID), + s.TypeCondition(typ), + s.StateCondition(state), + ), + true, ) } @@ -633,11 +636,6 @@ func (s brandingSettings) ActivateAt(ctx context.Context, client database.QueryE } func (s brandingSettings) activateAt(ctx context.Context, client database.QueryExecutor, condition database.Condition, updatedAt time.Time) (int64, error) { - condition = database.And( - condition, - s.TypeCondition(domain.SettingTypeBranding), - s.StateCondition(domain.SettingStatePreview), - ) // if you want to update the roles you have to have a unique condition, otherwise multiple branding settings get activated if err := checkRestrictingColumns(condition, s.UniqueColumns()...); err != nil { return 0, err diff --git a/backend/v3/storage/database/repository/settings_test.go b/backend/v3/storage/database/repository/settings_test.go index c8af17090..5c51da1d6 100644 --- a/backend/v3/storage/database/repository/settings_test.go +++ b/backend/v3/storage/database/repository/settings_test.go @@ -1252,34 +1252,42 @@ func TestActivateBrandingSettings(t *testing.T) { }{ { name: "activate instance", - condition: database.And( - repo.InstanceIDCondition(existingInstanceSettings.InstanceID), - repo.OrganizationIDCondition(nil), + condition: repo.UniqueCondition( + existingInstanceSettings.InstanceID, + nil, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), wantRowsAffected: 1, }, { name: "activate organization", - condition: database.And( - repo.InstanceIDCondition(existingOrganizationSettings.InstanceID), - repo.OrganizationIDCondition(existingOrganizationSettings.OrganizationID), + condition: repo.UniqueCondition( + existingInstanceSettings.InstanceID, + existingOrganizationSettings.OrganizationID, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), wantRowsAffected: 1, }, { name: "non-existing instance", - condition: database.And( - repo.InstanceIDCondition("foo"), - repo.OrganizationIDCondition(nil), + condition: repo.UniqueCondition( + "foo", + nil, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), wantRowsAffected: 0, }, { name: "non-existing organization", - condition: database.And( - repo.InstanceIDCondition(existingOrganizationSettings.InstanceID), - repo.OrganizationIDCondition(gu.Ptr("foo")), + condition: repo.UniqueCondition( + existingInstanceSettings.InstanceID, + gu.Ptr("foo"), + domain.SettingTypeBranding, + domain.SettingStatePreview, ), wantRowsAffected: 0, }, @@ -1370,27 +1378,33 @@ func TestActivateAtBrandingSettings(t *testing.T) { }{ { name: "activate instance", - condition: database.And( - repo.InstanceIDCondition(existingInstanceSettings.InstanceID), - repo.OrganizationIDCondition(nil), + condition: repo.UniqueCondition( + existingInstanceSettings.InstanceID, + nil, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), updatedAt: time.Now(), wantRowsAffected: 1, }, { name: "activate organization", - condition: database.And( - repo.InstanceIDCondition(existingOrganizationSettings.InstanceID), - repo.OrganizationIDCondition(existingOrganizationSettings.OrganizationID), + condition: repo.UniqueCondition( + existingOrganizationSettings.InstanceID, + existingOrganizationSettings.OrganizationID, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), updatedAt: time.Now(), wantRowsAffected: 1, }, { name: "non-existing instance", - condition: database.And( - repo.InstanceIDCondition("foo"), - repo.OrganizationIDCondition(nil), + condition: repo.UniqueCondition( + "foo", + nil, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), updatedAt: time.Now(), wantRowsAffected: 0, @@ -1398,9 +1412,11 @@ func TestActivateAtBrandingSettings(t *testing.T) { { name: "non-existing organization", - condition: database.And( - repo.InstanceIDCondition(existingOrganizationSettings.InstanceID), - repo.OrganizationIDCondition(gu.Ptr("foo")), + condition: repo.UniqueCondition( + existingOrganizationSettings.InstanceID, + gu.Ptr("foo"), + domain.SettingTypeBranding, + domain.SettingStatePreview, ), updatedAt: time.Now(), wantRowsAffected: 0, @@ -1454,9 +1470,11 @@ func TestDeleteBrandingSettings(t *testing.T) { require.NoError(t, err) _, err = repo.Activate(t.Context(), tx, - database.And( - repo.InstanceIDCondition(existingInstanceSettings.InstanceID), - repo.OrganizationIDCondition(existingInstanceSettings.OrganizationID), + repo.UniqueCondition( + existingInstanceSettings.InstanceID, + existingInstanceSettings.OrganizationID, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), ) require.NoError(t, err) @@ -1492,9 +1510,11 @@ func TestDeleteBrandingSettings(t *testing.T) { require.NoError(t, err) _, err = repo.Activate(t.Context(), tx, - database.And( - repo.InstanceIDCondition(existingOrganizationSettings.InstanceID), - repo.OrganizationIDCondition(existingOrganizationSettings.OrganizationID), + repo.UniqueCondition( + existingOrganizationSettings.InstanceID, + existingOrganizationSettings.OrganizationID, + domain.SettingTypeBranding, + domain.SettingStatePreview, ), ) require.NoError(t, err) diff --git a/internal/query/projection/settings_relational.go b/internal/query/projection/settings_relational.go index 8409c3eae..2b7dffefd 100644 --- a/internal/query/projection/settings_relational.go +++ b/internal/query/projection/settings_relational.go @@ -8,7 +8,6 @@ import ( "github.com/muhlemmer/gu" "github.com/zitadel/zitadel/backend/v3/domain" - "github.com/zitadel/zitadel/backend/v3/storage/database" v3_sql "github.com/zitadel/zitadel/backend/v3/storage/database/dialect/sql" "github.com/zitadel/zitadel/backend/v3/storage/database/repository" "github.com/zitadel/zitadel/internal/eventstore" @@ -766,10 +765,7 @@ func (s *settingsRelationalProjection) reduceLabelActivated(event eventstore.Eve settingsRepo := repository.BrandingSettingsRepository() _, err := settingsRepo.ActivateAt(ctx, v3_sql.SQLTx(tx), - database.And( - settingsRepo.InstanceIDCondition(event.Aggregate().InstanceID), - settingsRepo.OrganizationIDCondition(orgId), - ), + settingsRepo.UniqueCondition(policyEvent.Aggregate().InstanceID, orgId, domain.SettingTypeBranding, domain.SettingStatePreview), policyEvent.Creation, ) return err