From e6b5f559f08a4b4a9d3d38aa5f9a31cfebf46d1b Mon Sep 17 00:00:00 2001 From: Marco A Date: Wed, 10 Dec 2025 13:13:56 +0100 Subject: [PATCH] fix: Force v2 permission checks on user listing # Which Problems Are Solved When the feature flag for enabling permission checks v2 is disabled, a user without permission could list users across instances and get the total number of users available. # How the Problems Are Solved Disregard the state of the feature flag and always enforce permission checks v2 on v2 APIs. --------- Co-authored-by: Livio Spring (cherry picked from commit 826039c6208fe71df57b3a94c982b5ac5b0af12c) (cherry picked from commit 0e17d0005a98ccbf92139961ef702ef03208ffd3) --- .../user/v2/integration_test/query_test.go | 194 ++++++------------ .../v2beta/integration_test/query_test.go | 145 ++++--------- internal/query/user.go | 15 +- internal/query/user_test.go | 124 ----------- 4 files changed, 104 insertions(+), 374 deletions(-) diff --git a/internal/api/grpc/user/v2/integration_test/query_test.go b/internal/api/grpc/user/v2/integration_test/query_test.go index 15dc9591518..9917db560a3 100644 --- a/internal/api/grpc/user/v2/integration_test/query_test.go +++ b/internal/api/grpc/user/v2/integration_test/query_test.go @@ -4,7 +4,6 @@ package user_test import ( "context" - "errors" "fmt" "slices" "testing" @@ -17,61 +16,11 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" "github.com/zitadel/zitadel/internal/integration" - "github.com/zitadel/zitadel/pkg/grpc/feature/v2" "github.com/zitadel/zitadel/pkg/grpc/object/v2" "github.com/zitadel/zitadel/pkg/grpc/session/v2" "github.com/zitadel/zitadel/pkg/grpc/user/v2" ) -var ( - permissionCheckV2SetFlagInital bool - permissionCheckV2SetFlag bool -) - -type permissionCheckV2SettingsStruct struct { - TestNamePrependString string - SetFlag bool -} - -var permissionCheckV2Settings []permissionCheckV2SettingsStruct = []permissionCheckV2SettingsStruct{ - { - SetFlag: false, - TestNamePrependString: "permission_check_v2 IS NOT SET" + " ", - }, - { - SetFlag: true, - TestNamePrependString: "permission_check_v2 IS SET" + " ", - }, -} - -func setPermissionCheckV2Flag(t *testing.T, setFlag bool) { - if permissionCheckV2SetFlagInital && permissionCheckV2SetFlag == setFlag { - return - } - - _, err := Instance.Client.FeatureV2.SetInstanceFeatures(IamCTX, &feature.SetInstanceFeaturesRequest{ - PermissionCheckV2: &setFlag, - }) - require.NoError(t, err) - - var flagSet bool - for i := 0; !flagSet || i < 6; i++ { - res, err := Instance.Client.FeatureV2.GetInstanceFeatures(IamCTX, &feature.GetInstanceFeaturesRequest{}) - require.NoError(t, err) - if res.PermissionCheckV2.Enabled == setFlag { - flagSet = true - continue - } - time.Sleep(10 * time.Second) - } - - if !flagSet { - require.NoError(t, errors.New("unable to set permission_check_v2 flag")) - } - permissionCheckV2SetFlagInital = true - permissionCheckV2SetFlag = setFlag -} - func TestServer_GetUserByID(t *testing.T) { orgResp := Instance.CreateOrganization(IamCTX, fmt.Sprintf("GetUserByIDOrg-%s", gofakeit.AppName()), gofakeit.Email()) type args struct { @@ -433,11 +382,6 @@ func createUserWithUserName(ctx context.Context, username string, orgID string, } func TestServer_ListUsers(t *testing.T) { - defer func() { - _, err := Instance.Client.FeatureV2.ResetInstanceFeatures(IamCTX, &feature.ResetInstanceFeaturesRequest{}) - require.NoError(t, err) - }() - orgResp := Instance.CreateOrganization(IamCTX, fmt.Sprintf("ListUsersOrg-%s", gofakeit.AppName()), gofakeit.Email()) type args struct { ctx context.Context @@ -1119,7 +1063,7 @@ func TestServer_ListUsers(t *testing.T) { }, want: &user.ListUsersResponse{ Details: &object.ListDetails{ - TotalResult: 0, + TotalResult: 1, Timestamp: timestamppb.Now(), }, SortingColumn: 0, @@ -1130,65 +1074,54 @@ func TestServer_ListUsers(t *testing.T) { }, }, } - for _, f := range permissionCheckV2Settings { - f := f - for _, tt := range tests { - t.Run(f.TestNamePrependString+tt.name, func(t *testing.T) { - setPermissionCheckV2Flag(t, f.SetFlag) - infos := tt.args.dep(IamCTX, tt.args.req) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + infos := tt.args.dep(IamCTX, tt.args.req) - retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.args.ctx, 10*time.Minute) - require.EventuallyWithT(t, func(ttt *assert.CollectT) { - got, err := Client.ListUsers(tt.args.ctx, tt.args.req) - if tt.wantErr { - require.Error(ttt, err) - return - } - require.NoError(ttt, err) + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.args.ctx, 10*time.Minute) + require.EventuallyWithT(t, func(ttt *assert.CollectT) { + got, err := Client.ListUsers(tt.args.ctx, tt.args.req) + if tt.wantErr { + require.Error(ttt, err) + return + } + require.NoError(ttt, err) - // always only give back dependency infos which are required for the response - require.Len(ttt, tt.want.Result, len(infos)) - if assert.Len(ttt, got.Result, len(tt.want.Result)) { - tt.want.Details.TotalResult = got.Details.TotalResult - - // fill in userid and username as it is generated - for i := range infos { - if tt.want.Result[i] == nil { - continue - } - tt.want.Result[i].UserId = infos[i].UserID - tt.want.Result[i].Username = infos[i].Username - tt.want.Result[i].PreferredLoginName = infos[i].Username - tt.want.Result[i].LoginNames = []string{infos[i].Username} - if human := tt.want.Result[i].GetHuman(); human != nil { - human.Email.Email = infos[i].Username - human.Phone.Phone = infos[i].Phone - if tt.want.Result[i].GetHuman().GetPasswordChanged() != nil { - human.PasswordChanged = infos[i].Changed - } - } - tt.want.Result[i].Details = infos[i].Details + // always only give back dependency infos which are required for the response + require.Len(ttt, tt.want.Result, len(infos)) + if assert.Len(ttt, got.Result, len(tt.want.Result)) { + // fill in userid and username as it is generated + for i := range infos { + if tt.want.Result[i] == nil { + continue } - for i := range tt.want.Result { - if tt.want.Result[i] == nil { - continue + tt.want.Result[i].UserId = infos[i].UserID + tt.want.Result[i].Username = infos[i].Username + tt.want.Result[i].PreferredLoginName = infos[i].Username + tt.want.Result[i].LoginNames = []string{infos[i].Username} + if human := tt.want.Result[i].GetHuman(); human != nil { + human.Email.Email = infos[i].Username + human.Phone.Phone = infos[i].Phone + if tt.want.Result[i].GetHuman().GetPasswordChanged() != nil { + human.PasswordChanged = infos[i].Changed } - assert.EqualExportedValues(ttt, got.Result[i], tt.want.Result[i]) } + tt.want.Result[i].Details = infos[i].Details } - integration.AssertListDetails(ttt, tt.want, got) - }, retryDuration, tick, "timeout waiting for expected user result") - }) - } + for i := range tt.want.Result { + if tt.want.Result[i] == nil { + continue + } + assert.EqualExportedValues(ttt, got.Result[i], tt.want.Result[i]) + } + } + integration.AssertListDetails(ttt, tt.want, got) + }, retryDuration, tick, "timeout waiting for expected user result") + }) } } func TestServer_SystemUsers_ListUsers(t *testing.T) { - defer func() { - _, err := Instance.Client.FeatureV2.ResetInstanceFeatures(IamCTX, &feature.ResetInstanceFeaturesRequest{}) - require.NoError(t, err) - }() - org1 := Instance.CreateOrganization(IamCTX, fmt.Sprintf("ListUsersOrg-%s", gofakeit.AppName()), gofakeit.Email()) org2 := Instance.CreateOrganization(IamCTX, fmt.Sprintf("ListUsersOrg-%s", gofakeit.AppName()), "org2@zitadel.com") org3 := Instance.CreateOrganization(IamCTX, fmt.Sprintf("ListUsersOrg-%s", gofakeit.AppName()), gofakeit.Email()) @@ -1239,38 +1172,33 @@ func TestServer_SystemUsers_ListUsers(t *testing.T) { }, } - for _, f := range permissionCheckV2Settings { - f := f - for _, tt := range tests { - t.Run(f.TestNamePrependString+tt.name, func(t *testing.T) { - setPermissionCheckV2Flag(t, f.SetFlag) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.ctx, 1*time.Minute) + require.EventuallyWithT(t, func(ttt *assert.CollectT) { + got, err := Client.ListUsers(tt.ctx, tt.req) + require.NoError(ttt, err) - retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.ctx, 1*time.Minute) - require.EventuallyWithT(t, func(ttt *assert.CollectT) { - got, err := Client.ListUsers(tt.ctx, tt.req) - require.NoError(ttt, err) + if tt.checkNumberOfUsersReturned { + require.Equal(t, len(tt.expectedFoundUsernames), len(got.Result)) + } - if tt.checkNumberOfUsersReturned { - require.Equal(t, len(tt.expectedFoundUsernames), len(got.Result)) - } - - if tt.expectedFoundUsernames != nil { - for _, user := range got.Result { - for i, username := range tt.expectedFoundUsernames { - if username == user.Username { - tt.expectedFoundUsernames = tt.expectedFoundUsernames[i+1:] - break - } - } - if len(tt.expectedFoundUsernames) == 0 { - return + if tt.expectedFoundUsernames != nil { + for _, user := range got.Result { + for i, username := range tt.expectedFoundUsernames { + if username == user.Username { + tt.expectedFoundUsernames = tt.expectedFoundUsernames[i+1:] + break } } - require.FailNow(t, "unable to find all users with specified usernames") + if len(tt.expectedFoundUsernames) == 0 { + return + } } - }, retryDuration, tick, "timeout waiting for expected user result") - }) - } + require.FailNow(t, "unable to find all users with specified usernames") + } + }, retryDuration, tick, "timeout waiting for expected user result") + }) } } diff --git a/internal/api/grpc/user/v2beta/integration_test/query_test.go b/internal/api/grpc/user/v2beta/integration_test/query_test.go index 73bff3fd0da..f54d7bd61cc 100644 --- a/internal/api/grpc/user/v2beta/integration_test/query_test.go +++ b/internal/api/grpc/user/v2beta/integration_test/query_test.go @@ -4,7 +4,6 @@ package user_test import ( "context" - "errors" "fmt" "slices" "testing" @@ -17,7 +16,6 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" "github.com/zitadel/zitadel/internal/integration" - "github.com/zitadel/zitadel/pkg/grpc/feature/v2" "github.com/zitadel/zitadel/pkg/grpc/object/v2" object_v2beta "github.com/zitadel/zitadel/pkg/grpc/object/v2beta" "github.com/zitadel/zitadel/pkg/grpc/session/v2" @@ -32,55 +30,6 @@ func detailsV2ToV2beta(obj *object.Details) *object_v2beta.Details { } } -var ( - permissionCheckV2SetFlagInital bool - permissionCheckV2SetFlag bool -) - -type permissionCheckV2SettingsStruct struct { - TestNamePrependString string - SetFlag bool -} - -var permissionCheckV2Settings []permissionCheckV2SettingsStruct = []permissionCheckV2SettingsStruct{ - { - SetFlag: false, - TestNamePrependString: "permission_check_v2 IS NOT SET" + " ", - }, - { - SetFlag: true, - TestNamePrependString: "permission_check_v2 IS SET" + " ", - }, -} - -func setPermissionCheckV2Flag(t *testing.T, setFlag bool) { - if permissionCheckV2SetFlagInital && permissionCheckV2SetFlag == setFlag { - return - } - - _, err := Instance.Client.FeatureV2.SetInstanceFeatures(IamCTX, &feature.SetInstanceFeaturesRequest{ - PermissionCheckV2: &setFlag, - }) - require.NoError(t, err) - - var flagSet bool - for i := 0; !flagSet || i < 6; i++ { - res, err := Instance.Client.FeatureV2.GetInstanceFeatures(IamCTX, &feature.GetInstanceFeaturesRequest{}) - require.NoError(t, err) - if res.PermissionCheckV2.Enabled == setFlag { - flagSet = true - continue - } - time.Sleep(10 * time.Second) - } - - if !flagSet { - require.NoError(t, errors.New("unable to set permission_check_v2 flag")) - } - permissionCheckV2SetFlagInital = true - permissionCheckV2SetFlag = setFlag -} - func TestServer_GetUserByID(t *testing.T) { orgResp := Instance.CreateOrganization(IamCTX, fmt.Sprintf("GetUserByIDOrg-%s", gofakeit.AppName()), gofakeit.Email()) type args struct { @@ -433,11 +382,6 @@ func createUser(ctx context.Context, orgID string, passwordChangeRequired bool) } func TestServer_ListUsers(t *testing.T) { - defer func() { - _, err := Instance.Client.FeatureV2.ResetInstanceFeatures(IamCTX, &feature.ResetInstanceFeaturesRequest{}) - require.NoError(t, err) - }() - orgResp := Instance.CreateOrganization(IamCTX, fmt.Sprintf("ListUsersOrg-%s", gofakeit.AppName()), gofakeit.Email()) type args struct { ctx context.Context @@ -1122,7 +1066,7 @@ func TestServer_ListUsers(t *testing.T) { }, want: &user.ListUsersResponse{ Details: &object_v2beta.ListDetails{ - TotalResult: 0, + TotalResult: 1, Timestamp: timestamppb.Now(), }, SortingColumn: 0, @@ -1133,59 +1077,52 @@ func TestServer_ListUsers(t *testing.T) { }, }, } - for _, f := range permissionCheckV2Settings { - f := f - for _, tt := range tests { - t.Run(f.TestNamePrependString+tt.name, func(t *testing.T) { - setPermissionCheckV2Flag(t, f.SetFlag) - infos := tt.args.dep(IamCTX, tt.args.req) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + infos := tt.args.dep(IamCTX, tt.args.req) - // retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.args.ctx, 10*time.Minute) - retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.args.ctx, 20*time.Second) - require.EventuallyWithT(t, func(ttt *assert.CollectT) { - got, err := Client.ListUsers(tt.args.ctx, tt.args.req) - if tt.wantErr { - require.Error(ttt, err) - return - } - require.NoError(ttt, err) + // retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.args.ctx, 10*time.Minute) + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(tt.args.ctx, 20*time.Second) + require.EventuallyWithT(t, func(ttt *assert.CollectT) { + got, err := Client.ListUsers(tt.args.ctx, tt.args.req) + if tt.wantErr { + require.Error(ttt, err) + return + } + require.NoError(ttt, err) - // always only give back dependency infos which are required for the response - require.Len(ttt, tt.want.Result, len(infos)) - // always first check length, otherwise its failed anyway - if assert.Len(ttt, got.Result, len(tt.want.Result)) { - // totalResult is unrelated to the tests here so gets carried over, can vary from the count of results due to permissions - tt.want.Details.TotalResult = got.Details.TotalResult - - // fill in userid and username as it is generated - for i := range infos { - if tt.want.Result[i] == nil { - continue - } - tt.want.Result[i].UserId = infos[i].UserID - tt.want.Result[i].Username = infos[i].Username - tt.want.Result[i].PreferredLoginName = infos[i].Username - tt.want.Result[i].LoginNames = []string{infos[i].Username} - if human := tt.want.Result[i].GetHuman(); human != nil { - human.Email.Email = infos[i].Username - human.Phone.Phone = infos[i].Phone - if tt.want.Result[i].GetHuman().GetPasswordChanged() != nil { - human.PasswordChanged = infos[i].Changed - } - } - tt.want.Result[i].Details = detailsV2ToV2beta(infos[i].Details) + // always only give back dependency infos which are required for the response + require.Len(ttt, tt.want.Result, len(infos)) + // always first check length, otherwise its failed anyway + if assert.Len(ttt, got.Result, len(tt.want.Result)) { + // fill in userid and username as it is generated + for i := range infos { + if tt.want.Result[i] == nil { + continue } - for i := range tt.want.Result { - if tt.want.Result[i] == nil { - continue + tt.want.Result[i].UserId = infos[i].UserID + tt.want.Result[i].Username = infos[i].Username + tt.want.Result[i].PreferredLoginName = infos[i].Username + tt.want.Result[i].LoginNames = []string{infos[i].Username} + if human := tt.want.Result[i].GetHuman(); human != nil { + human.Email.Email = infos[i].Username + human.Phone.Phone = infos[i].Phone + if tt.want.Result[i].GetHuman().GetPasswordChanged() != nil { + human.PasswordChanged = infos[i].Changed } - assert.EqualExportedValues(ttt, got.Result[i], tt.want.Result[i]) } + tt.want.Result[i].Details = detailsV2ToV2beta(infos[i].Details) } - integration.AssertListDetails(ttt, tt.want, got) - }, retryDuration, tick, "timeout waiting for expected user result") - }) - } + for i := range tt.want.Result { + if tt.want.Result[i] == nil { + continue + } + assert.EqualExportedValues(ttt, got.Result[i], tt.want.Result[i]) + } + } + integration.AssertListDetails(ttt, tt.want, got) + }, retryDuration, tick, "timeout waiting for expected user result") + }) } } diff --git a/internal/query/user.go b/internal/query/user.go index c0fc3de97c8..63444f4afb1 100644 --- a/internal/query/user.go +++ b/internal/query/user.go @@ -5,7 +5,6 @@ import ( "database/sql" _ "embed" "errors" - "slices" "strings" "time" @@ -124,14 +123,6 @@ type NotifyUser struct { PasswordSet bool } -func usersCheckPermission(ctx context.Context, users *Users, permissionCheck domain.PermissionCheck) { - users.Users = slices.DeleteFunc(users.Users, - func(user *User) bool { - return userCheckPermission(ctx, user.ResourceOwner, user.ID, permissionCheck) != nil - }, - ) -} - type UserSearchQueries struct { SearchRequest Queries []SearchQuery @@ -601,13 +592,11 @@ func (q *Queries) CountUsers(ctx context.Context, queries *UserSearchQueries) (c } func (q *Queries) SearchUsers(ctx context.Context, queries *UserSearchQueries, filterOrgIds string, permissionCheck domain.PermissionCheck) (*Users, error) { - users, err := q.searchUsers(ctx, queries, filterOrgIds, permissionCheck != nil && authz.GetFeatures(ctx).PermissionCheckV2) + users, err := q.searchUsers(ctx, queries, filterOrgIds, permissionCheck != nil) if err != nil { return nil, err } - if permissionCheck != nil && !authz.GetFeatures(ctx).PermissionCheckV2 { - usersCheckPermission(ctx, users, permissionCheck) - } + return users, nil } diff --git a/internal/query/user_test.go b/internal/query/user_test.go index ae5f6be2074..4f519040c63 100644 --- a/internal/query/user_test.go +++ b/internal/query/user_test.go @@ -10,7 +10,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "golang.org/x/text/language" "github.com/zitadel/zitadel/internal/api/authz" @@ -19,129 +18,6 @@ import ( "github.com/zitadel/zitadel/internal/zerrors" ) -func TestUser_usersCheckPermission(t *testing.T) { - type want struct { - users []*User - } - tests := []struct { - name string - want want - users *Users - permissions []string - }{ - { - "permissions for all users", - want{ - users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - &Users{ - Users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - []string{"first", "second", "third"}, - }, - { - "permissions for one user, first", - want{ - users: []*User{ - {ID: "first"}, - }, - }, - &Users{ - Users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - []string{"first"}, - }, - { - "permissions for one user, second", - want{ - users: []*User{ - {ID: "second"}, - }, - }, - &Users{ - Users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - []string{"second"}, - }, - { - "permissions for one user, third", - want{ - users: []*User{ - {ID: "third"}, - }, - }, - &Users{ - Users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - []string{"third"}, - }, - { - "permissions for two users, first", - want{ - users: []*User{ - {ID: "first"}, {ID: "third"}, - }, - }, - &Users{ - Users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - []string{"first", "third"}, - }, - { - "permissions for two users, second", - want{ - users: []*User{ - {ID: "second"}, {ID: "third"}, - }, - }, - &Users{ - Users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - []string{"second", "third"}, - }, - { - "no permissions", - want{ - users: []*User{}, - }, - &Users{ - Users: []*User{ - {ID: "first"}, {ID: "second"}, {ID: "third"}, - }, - }, - []string{}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - checkPermission := func(ctx context.Context, permission, orgID, resourceID string) (err error) { - for _, perm := range tt.permissions { - if resourceID == perm { - return nil - } - } - return errors.New("failed") - } - usersCheckPermission(context.Background(), tt.users, checkPermission) - require.Equal(t, tt.want.users, tt.users.Users) - }) - } -} - func TestUser_userCheckPermission(t *testing.T) { type args struct { ctxData string