From 9aad207ee42115584e77230cec04692dcb0526f4 Mon Sep 17 00:00:00 2001 From: Iraq <66622793+kkrime@users.noreply.github.com> Date: Thu, 20 Feb 2025 15:39:48 +0000 Subject: [PATCH] fix(permissions): return current user when calling ListUsers() when user does not have permissions (#9374) # Which Problems Are Solved When running `ListUsers()` with no permissions, the calling user shoud be returned # How the Problems Are Solved Added additional clause to SQL search statement # Additional Changes n/a # Additional Context - Closes https://github.com/zitadel/zitadel/issues/9355 --------- Co-authored-by: Iraq Jaber --- .../user/v2/integration_test/query_test.go | 65 ++++++++++++++++++- .../v2beta/integration_test/query_test.go | 65 ++++++++++++++++++- internal/query/permission.go | 17 ++++- internal/query/user.go | 2 +- 4 files changed, 145 insertions(+), 4 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 2551a4a833..ada9affcbb 100644 --- a/internal/api/grpc/user/v2/integration_test/query_test.go +++ b/internal/api/grpc/user/v2/integration_test/query_test.go @@ -17,6 +17,7 @@ import ( "github.com/zitadel/zitadel/internal/integration" "github.com/zitadel/zitadel/pkg/grpc/object/v2" + "github.com/zitadel/zitadel/pkg/grpc/session/v2" "github.com/zitadel/zitadel/pkg/grpc/user/v2" ) @@ -384,7 +385,7 @@ func TestServer_ListUsers(t *testing.T) { wantErr bool }{ { - name: "list user by id, no permission", + name: "list user by id, no permission machine user", args: args{ UserCTX, &user.ListUsersRequest{}, @@ -403,6 +404,68 @@ func TestServer_ListUsers(t *testing.T) { Result: []*user.User{}, }, }, + { + name: "list user by id, no permission human user", + args: func() args { + info := createUser(IamCTX, orgResp.OrganizationId, true) + // create session to get token + userID := info.UserID + createResp, err := Instance.Client.SessionV2.CreateSession(IamCTX, &session.CreateSessionRequest{ + Checks: &session.Checks{ + User: &session.CheckUser{ + Search: &session.CheckUser_UserId{UserId: userID}, + }, + Password: &session.CheckPassword{ + Password: integration.UserPassword, + }, + }, + }) + if err != nil { + require.NoError(t, err) + } + // use token to get ctx + HumanCTX := integration.WithAuthorizationToken(IamCTX, createResp.GetSessionToken()) + return args{ + HumanCTX, + &user.ListUsersRequest{}, + func(ctx context.Context, request *user.ListUsersRequest) userAttrs { + return []userAttr{info} + }, + } + }(), + want: &user.ListUsersResponse{ // human user should return itself when calling ListUsers() even if it has no permissions + Details: &object.ListDetails{ + TotalResult: 1, + Timestamp: timestamppb.Now(), + }, + SortingColumn: 0, + Result: []*user.User{ + { + State: user.UserState_USER_STATE_ACTIVE, + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "Mickey", + FamilyName: "Mouse", + NickName: gu.Ptr("Mickey"), + DisplayName: gu.Ptr("Mickey Mouse"), + PreferredLanguage: gu.Ptr("nl"), + Gender: user.Gender_GENDER_MALE.Enum(), + }, + Email: &user.HumanEmail{ + IsVerified: true, + }, + Phone: &user.HumanPhone{ + IsVerified: true, + }, + PasswordChangeRequired: true, + PasswordChanged: timestamppb.Now(), + }, + }, + }, + }, + }, + }, { name: "list user by id, ok", args: args{ 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 67fc609212..61b0d829ff 100644 --- a/internal/api/grpc/user/v2beta/integration_test/query_test.go +++ b/internal/api/grpc/user/v2beta/integration_test/query_test.go @@ -18,6 +18,7 @@ import ( "github.com/zitadel/zitadel/internal/integration" "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" user "github.com/zitadel/zitadel/pkg/grpc/user/v2beta" ) @@ -394,7 +395,7 @@ func TestServer_ListUsers(t *testing.T) { wantErr bool }{ { - name: "list user by id, no permission", + name: "list user by id, no permission machine user", args: args{ UserCTX, &user.ListUsersRequest{}, @@ -413,6 +414,68 @@ func TestServer_ListUsers(t *testing.T) { Result: []*user.User{}, }, }, + { + name: "list user by id, no permission human user", + args: func() args { + info := createUser(IamCTX, orgResp.OrganizationId, true) + // create session to get token + userID := info.UserID + createResp, err := Instance.Client.SessionV2.CreateSession(IamCTX, &session.CreateSessionRequest{ + Checks: &session.Checks{ + User: &session.CheckUser{ + Search: &session.CheckUser_UserId{UserId: userID}, + }, + Password: &session.CheckPassword{ + Password: integration.UserPassword, + }, + }, + }) + if err != nil { + require.NoError(t, err) + } + // use token to get ctx + HumanCTX := integration.WithAuthorizationToken(IamCTX, createResp.GetSessionToken()) + return args{ + HumanCTX, + &user.ListUsersRequest{}, + func(ctx context.Context, request *user.ListUsersRequest) userAttrs { + return []userAttr{info} + }, + } + }(), + want: &user.ListUsersResponse{ // human user should return itself when calling ListUsers() even if it has no permissions + Details: &object_v2beta.ListDetails{ + TotalResult: 1, + Timestamp: timestamppb.Now(), + }, + SortingColumn: 0, + Result: []*user.User{ + { + State: user.UserState_USER_STATE_ACTIVE, + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "Mickey", + FamilyName: "Mouse", + NickName: gu.Ptr("Mickey"), + DisplayName: gu.Ptr("Mickey Mouse"), + PreferredLanguage: gu.Ptr("nl"), + Gender: user.Gender_GENDER_MALE.Enum(), + }, + Email: &user.HumanEmail{ + IsVerified: true, + }, + Phone: &user.HumanPhone{ + IsVerified: true, + }, + PasswordChangeRequired: true, + PasswordChanged: timestamppb.Now(), + }, + }, + }, + }, + }, + }, { name: "list user by id, ok", args: args{ diff --git a/internal/query/permission.go b/internal/query/permission.go index 591493375e..aeda33e541 100644 --- a/internal/query/permission.go +++ b/internal/query/permission.go @@ -12,7 +12,8 @@ import ( const ( // eventstore.permitted_orgs(instanceid text, userid text, perm text, filter_orgs text) - wherePermittedOrgsClause = "%s = ANY(eventstore.permitted_orgs(?, ?, ?, ?))" + wherePermittedOrgsClause = "%s = ANY(eventstore.permitted_orgs(?, ?, ?, ?))" + wherePermittedOrgsOrCurrentUserClause = "(" + wherePermittedOrgsClause + " OR %s = ?" + ")" ) // wherePermittedOrgs sets a `WHERE` clause to the query that filters the orgs @@ -35,3 +36,17 @@ func wherePermittedOrgs(ctx context.Context, query sq.SelectBuilder, filterOrgId filterOrgIds, ) } + +func wherePermittedOrgsOrCurrentUser(ctx context.Context, query sq.SelectBuilder, filterOrgIds, orgIDColumn, userIdColum, permission string) sq.SelectBuilder { + userID := authz.GetCtxData(ctx).UserID + logging.WithFields("permission_check_v2_flag", authz.GetFeatures(ctx).PermissionCheckV2, "org_id_column", orgIDColumn, "user_id_colum", userIdColum, "permission", permission, "user_id", userID).Debug("permitted orgs check used") + + return query.Where( + fmt.Sprintf(wherePermittedOrgsOrCurrentUserClause, orgIDColumn, userIdColum), + authz.GetInstance(ctx).InstanceID(), + userID, + permission, + filterOrgIds, + userID, + ) +} diff --git a/internal/query/user.go b/internal/query/user.go index 0b00b45e03..3ee9a48463 100644 --- a/internal/query/user.go +++ b/internal/query/user.go @@ -655,7 +655,7 @@ func (q *Queries) searchUsers(ctx context.Context, queries *UserSearchQueries, f UserInstanceIDCol.identifier(): authz.GetInstance(ctx).InstanceID(), }) if permissionCheckV2 { - query = wherePermittedOrgs(ctx, query, filterOrgIds, UserResourceOwnerCol.identifier(), domain.PermissionUserRead) + query = wherePermittedOrgsOrCurrentUser(ctx, query, filterOrgIds, UserResourceOwnerCol.identifier(), UserIDCol.identifier(), domain.PermissionUserRead) } stmt, args, err := query.ToSql()