andwati/zitadel
1
0
Fork 0
mirror of https://github.com/andwati/zitadel.git synced 2026-10-27 09:42:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(RT): correct restricting null columns (#11123)

Browse Source 
# 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 <[email protected]>
This commit is contained in:
Tim Möhlmann
2025-12-03 16:15:49 +00:00
committed by GitHub
co-authored by Elio Bischof
parent 96fffd9829
commit b4732579cb
7 changed files with 99 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/api/project.json
+1
View File
@@ -6,6 +6,7 @@
"sources": [
"{workspaceRoot}/cmd/**/*.go",
"{workspaceRoot}/internal/**/*.go",
"{workspaceRoot}/backend/**/*.go",
"{workspaceRoot}/proto/**/*.go",
"{workspaceRoot}/pkg/**/*.go",
"{workspaceRoot}/main.go"
backend/v3/storage/database/condition.go
+23 -3
View File
@@ -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)
backend/v3/storage/database/condition_test.go
+10 -10
View File
@@ -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)
})
}
backend/v3/storage/database/events_testing/settings_instance_test.go
+6 -6
View File
@@ -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) {
backend/v3/storage/database/repository/settings.go
+8 -10
View File
@@ -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
backend/v3/storage/database/repository/settings_test.go
+50 -30
View File
@@ -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)
internal/query/projection/settings_relational.go
+1 -5
View File
@@ -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