From e4517cf15ac911a9ba374d73c194f86d3204c466 Mon Sep 17 00:00:00 2001 From: Stefan Benz <46600784+stebenz@users.noreply.github.com> Date: Thu, 28 Aug 2025 14:09:52 +0200 Subject: [PATCH] fix: correctly handle user grants on project grant to same organization (#10568) # Which Problems Are Solved Authorizations (aka user grants) could not be managed correctly if they were created on a project grant, which itself was based on a project granted to the own organization. The error persisted if the corresponding (potentially unintended) project grant was removed again. # How the Problems Are Solved Fixed checks for managing user grants: Roles from projects and project grants get handled individually to ensure cases like project grants on the own organization. # Additional Changes Additional tests for the 3 failing scenarios. # Additional Context Closes #10556 --------- Co-authored-by: Livio Spring (cherry picked from commit 8e60cce20dac7cf154562408538595fdfd98bdbb) --- internal/command/user_grant.go | 7 +- internal/command/user_grant_model.go | 96 +++++--- internal/command/user_grant_test.go | 353 ++++++++++++++++++++++----- 3 files changed, 359 insertions(+), 97 deletions(-) diff --git a/internal/command/user_grant.go b/internal/command/user_grant.go index 2b0ff4cf183..cd659d0bc1e 100644 --- a/internal/command/user_grant.go +++ b/internal/command/user_grant.go @@ -469,7 +469,7 @@ func (c *Commands) checkUserGrantPreConditionOld(ctx context.Context, usergrant usergrant.ResourceOwner = preConditions.ProjectResourceOwner } if usergrant.ProjectGrantID == "" { - usergrant.ProjectGrantID = preConditions.ProjectGrantID + usergrant.ProjectGrantID = preConditions.FoundGrantID } if !preConditions.UserExists { return zerrors.ThrowPreconditionFailed(err, "COMMAND-4f8sg", "Errors.User.NotFound") @@ -478,10 +478,11 @@ func (c *Commands) checkUserGrantPreConditionOld(ctx context.Context, usergrant if projectIsOwned && !preConditions.ProjectExists { return zerrors.ThrowPreconditionFailed(err, "COMMAND-3n77S", "Errors.Project.NotFound") } - if !projectIsOwned && !preConditions.ProjectGrantExists { + if !projectIsOwned && preConditions.FoundGrantID == "" { return zerrors.ThrowPreconditionFailed(err, "COMMAND-4m9ff", "Errors.Project.Grant.NotFound") } - if usergrant.HasInvalidRoles(preConditions.ExistingRoleKeys) { + // Either check roles from project or project grant + if usergrant.HasInvalidRoles(preConditions.existingRoles()) { return zerrors.ThrowPreconditionFailed(err, "COMMAND-mm9F4", "Errors.Project.Role.NotFound") } if check != nil { diff --git a/internal/command/user_grant_model.go b/internal/command/user_grant_model.go index b35c96a2d50..b9d9d27a20b 100644 --- a/internal/command/user_grant_model.go +++ b/internal/command/user_grant_model.go @@ -1,6 +1,8 @@ package command import ( + "slices" + "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" "github.com/zitadel/zitadel/internal/repository/project" @@ -87,23 +89,30 @@ func UserGrantAggregateFromWriteModel(wm *eventstore.WriteModel) *eventstore.Agg type UserGrantPreConditionReadModel struct { eventstore.WriteModel - UserID string - ProjectID string - ProjectResourceOwner string - ProjectGrantID string - ResourceOwner string - UserExists bool - ProjectExists bool - ProjectGrantExists bool - ExistingRoleKeys []string + UserID string + ProjectID string + ProjectResourceOwner string + ProjectGrantID string + FoundGrantID string + ResourceOwner string + UserExists bool + ProjectExists bool + ExistingRoleKeysProject []string + ExistingRoleKeysGrant []string } func NewUserGrantPreConditionReadModel(userID, projectID, projectGrantID string, resourceOwner string) *UserGrantPreConditionReadModel { return &UserGrantPreConditionReadModel{ - UserID: userID, - ProjectID: projectID, + UserID: userID, + ProjectID: projectID, + // ProjectGrantID can be empty, if grantedOrgID is in the resourceowner ProjectGrantID: projectGrantID, - ResourceOwner: resourceOwner, + // resourceowner is either empty to use the project organization + // or filled with the project organization + // or filled with the organization the project is granted to + ResourceOwner: resourceOwner, + ExistingRoleKeysGrant: make([]string, 0), + ExistingRoleKeysProject: make([]string, 0), } } @@ -119,51 +128,60 @@ func (wm *UserGrantPreConditionReadModel) Reduce() error { case *user.UserRemovedEvent: wm.UserExists = false case *project.ProjectAddedEvent: - if wm.ResourceOwner == "" || wm.ResourceOwner == e.Aggregate().ResourceOwner { + if projectExistsOnOrganization(wm.ResourceOwner, e.Aggregate().ResourceOwner) { wm.ProjectExists = true } + // We store the organization of the project for later checks, e.g. in case of a project grant wm.ProjectResourceOwner = e.Aggregate().ResourceOwner case *project.ProjectRemovedEvent: wm.ProjectExists = false case *project.GrantAddedEvent: - if (wm.ProjectGrantID == e.GrantID || wm.ProjectGrantID == "") && wm.ResourceOwner != "" && wm.ResourceOwner == e.GrantedOrgID { - wm.ProjectGrantExists = true - wm.ExistingRoleKeys = e.RoleKeys - if wm.ProjectGrantID == "" { - wm.ProjectGrantID = e.GrantID - } + if projectGrantExistsOnOrganization(wm.ProjectGrantID, wm.ResourceOwner, e.GrantID, e.GrantedOrgID) { + wm.ExistingRoleKeysGrant = e.RoleKeys + wm.FoundGrantID = e.GrantID } case *project.GrantChangedEvent: - if wm.ProjectGrantID == e.GrantID { - wm.ExistingRoleKeys = e.RoleKeys + if wm.FoundGrantID == e.GrantID { + wm.ExistingRoleKeysGrant = e.RoleKeys } case *project.GrantRemovedEvent: - if wm.ProjectGrantID == e.GrantID { - wm.ProjectGrantExists = false - wm.ExistingRoleKeys = []string{} + if wm.FoundGrantID == e.GrantID { + wm.ExistingRoleKeysGrant = []string{} + wm.FoundGrantID = "" } case *project.RoleAddedEvent: - if wm.ProjectGrantID != "" { - continue - } - wm.ExistingRoleKeys = append(wm.ExistingRoleKeys, e.Key) + wm.ExistingRoleKeysProject = append(wm.ExistingRoleKeysProject, e.Key) case *project.RoleRemovedEvent: - if wm.ProjectGrantID != "" { - continue - } - for i, key := range wm.ExistingRoleKeys { - if key == e.Key { - copy(wm.ExistingRoleKeys[i:], wm.ExistingRoleKeys[i+1:]) - wm.ExistingRoleKeys[len(wm.ExistingRoleKeys)-1] = "" - wm.ExistingRoleKeys = wm.ExistingRoleKeys[:len(wm.ExistingRoleKeys)-1] - continue - } - } + wm.ExistingRoleKeysProject = slices.DeleteFunc(wm.ExistingRoleKeysProject, func(key string) bool { + return key == e.Key + }) } } return wm.WriteModel.Reduce() } +func projectExistsOnOrganization(requiredOrganization, projectResourceOwner string) bool { + // Depending on the API, a request can either require a project to be part of a specific organization + // or not. In the former case, the project must belong to the required organization. + // In the latter case, it is sufficient that the project exists at all, since the user will be granted + // automatically in the organization the project belongs to. + return requiredOrganization == "" || requiredOrganization == projectResourceOwner +} + +func projectGrantExistsOnOrganization(requiredGrantID, requiredOrganization, projectGrantID, grantedOrganization string) bool { + // Depending on the API, a request can either require a project grant (ID) and/or an organization (ID), + // where the project must be granted to. + return (requiredGrantID == "" || requiredGrantID == projectGrantID) && + (requiredOrganization == "" || requiredOrganization == grantedOrganization) +} + +func (wm *UserGrantPreConditionReadModel) existingRoles() []string { + if wm.FoundGrantID != "" { + return wm.ExistingRoleKeysGrant + } + return wm.ExistingRoleKeysProject +} + func (wm *UserGrantPreConditionReadModel) Query() *eventstore.SearchQueryBuilder { query := eventstore.NewSearchQueryBuilder(eventstore.ColumnsEvent). AddQuery(). diff --git a/internal/command/user_grant_test.go b/internal/command/user_grant_test.go index b12e190d824..14be9f324e1 100644 --- a/internal/command/user_grant_test.go +++ b/internal/command/user_grant_test.go @@ -492,6 +492,176 @@ func TestCommandSide_AddUserGrant(t *testing.T) { }, }, }, + { + name: "usergrant for project, and removed project grant on same resourceowner, ok", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username1", + "firstname1", + "lastname1", + "nickname1", + "displayname1", + language.German, + domain.GenderMale, + "email1", + true, + ), + ), + eventFromEventPusher( + project.NewProjectAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "projectname1", true, true, true, + domain.PrivateLabelingSettingUnspecified, + ), + ), + eventFromEventPusher( + project.NewRoleAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "rolekey1", + "rolekey", + "", + ), + ), + eventFromEventPusher( + project.NewGrantAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "projectgrant1", + "org1", + []string{"rolekey1"}, + ), + ), + eventFromEventPusher( + project.NewGrantRemovedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "projectgrant1", + "org1", + ), + ), + ), + expectPush( + usergrant.NewUserGrantAddedEvent(context.Background(), + &usergrant.NewAggregate("usergrant1", "org1").Aggregate, + "user1", + "project1", + "", + []string{"rolekey1"}, + ), + ), + ), + idGenerator: func(t *testing.T) id.Generator { + return id_mock.NewIDGeneratorExpectIDs(t, "usergrant1") + }, + }, + args: args{ + ctx: authz.NewMockContextWithPermissions("", "", "", []string{domain.RoleProjectOwner}), + userGrant: &domain.UserGrant{ + UserID: "user1", + ProjectID: "project1", + RoleKeys: []string{"rolekey1"}, + ObjectRoot: models.ObjectRoot{ + ResourceOwner: "org1", + }, + }, + }, + res: res{ + want: &domain.UserGrant{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "usergrant1", + ResourceOwner: "org1", + }, + UserID: "user1", + ProjectID: "project1", + RoleKeys: []string{"rolekey1"}, + State: domain.UserGrantStateActive, + }, + }, + }, + { + name: "usergrant for project, and project grant on same resourceowner, ok", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username1", + "firstname1", + "lastname1", + "nickname1", + "displayname1", + language.German, + domain.GenderMale, + "email1", + true, + ), + ), + eventFromEventPusher( + project.NewProjectAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "projectname1", true, true, true, + domain.PrivateLabelingSettingUnspecified, + ), + ), + eventFromEventPusher( + project.NewRoleAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "rolekey1", + "rolekey", + "", + ), + ), + eventFromEventPusher( + project.NewGrantAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "projectgrant1", + "org1", + []string{"rolekey1"}, + ), + ), + ), + expectPush( + usergrant.NewUserGrantAddedEvent(context.Background(), + &usergrant.NewAggregate("usergrant1", "org1").Aggregate, + "user1", + "project1", + "projectgrant1", + []string{"rolekey1"}, + ), + ), + ), + idGenerator: func(t *testing.T) id.Generator { + return id_mock.NewIDGeneratorExpectIDs(t, "usergrant1") + }, + }, + args: args{ + ctx: authz.NewMockContextWithPermissions("", "", "", []string{domain.RoleProjectOwner}), + userGrant: &domain.UserGrant{ + UserID: "user1", + ProjectID: "project1", + RoleKeys: []string{"rolekey1"}, + ObjectRoot: models.ObjectRoot{ + ResourceOwner: "org1", + }, + }, + }, + res: res{ + want: &domain.UserGrant{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "usergrant1", + ResourceOwner: "org1", + }, + UserID: "user1", + ProjectID: "project1", + ProjectGrantID: "projectgrant1", + RoleKeys: []string{"rolekey1"}, + State: domain.UserGrantStateActive, + }, + }, + }, { name: "usergrant without resource owner on project, ok", fields: fields{ @@ -683,14 +853,14 @@ func TestCommandSide_AddUserGrant(t *testing.T) { project.NewGrantAddedEvent(context.Background(), &project.NewAggregate("project1", "org1").Aggregate, "projectgrant1", - "org1", + "org2", []string{"rolekey1"}, ), ), ), expectPush( usergrant.NewUserGrantAddedEvent(context.Background(), - &usergrant.NewAggregate("usergrant1", "org1").Aggregate, + &usergrant.NewAggregate("usergrant1", "org2").Aggregate, "user1", "project1", "projectgrant1", @@ -709,7 +879,7 @@ func TestCommandSide_AddUserGrant(t *testing.T) { ProjectID: "project1", RoleKeys: []string{"rolekey1"}, ObjectRoot: models.ObjectRoot{ - ResourceOwner: "org1", + ResourceOwner: "org2", }, }, }, @@ -717,7 +887,7 @@ func TestCommandSide_AddUserGrant(t *testing.T) { want: &domain.UserGrant{ ObjectRoot: models.ObjectRoot{ AggregateID: "usergrant1", - ResourceOwner: "org1", + ResourceOwner: "org2", }, UserID: "user1", ProjectID: "project1", @@ -728,55 +898,32 @@ func TestCommandSide_AddUserGrant(t *testing.T) { }, }, } - t.Run("without permission check", func(t *testing.T) { - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := &Commands{ - eventstore: tt.fields.eventstore(t), - } - if tt.fields.idGenerator != nil { - r.idGenerator = tt.fields.idGenerator(t) - } - got, err := r.AddUserGrant(tt.args.ctx, tt.args.userGrant, nil) - if tt.res.err == nil { - assert.NoError(t, err) - } - if tt.res.err != nil && !tt.res.err(err) { - t.Errorf("got wrong err: %v ", err) - } - if tt.res.err == nil { - assert.Equal(t, tt.res.want, got) - } - }) - } - }) - t.Run("with succeeding permission check", func(t *testing.T) { - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := &Commands{ - eventstore: tt.fields.eventstore(t), - } - if tt.fields.idGenerator != nil { - r.idGenerator = tt.fields.idGenerator(t) - } - // we use an empty context and only rely on the permission check implementation - got, err := r.AddUserGrant(context.Background(), tt.args.userGrant, succeedingUserGrantPermissionCheck) - if tt.res.err == nil { - assert.NoError(t, err) - } - if tt.res.err != nil && !tt.res.err(err) { - t.Errorf("got wrong err: %v ", err) - } - if tt.res.err == nil { - assert.Equal(t, tt.res.want, got) - } - }) - } - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Commands{ + eventstore: tt.fields.eventstore(t), + } + if tt.fields.idGenerator != nil { + r.idGenerator = tt.fields.idGenerator(t) + } + got, err := r.AddUserGrant(tt.args.ctx, tt.args.userGrant, succeedingUserGrantPermissionCheck) + if tt.res.err == nil { + assert.NoError(t, err) + } + if tt.res.err != nil && !tt.res.err(err) { + t.Errorf("got wrong err: %v ", err) + } + if tt.res.err == nil { + assert.Equal(t, tt.res.want, got) + } + }) + } +} + +func TestCommandSide_AddUserGrant_permission_failed(t *testing.T) { t.Run("with failing permission check", func(t *testing.T) { r := &Commands{ - eventstore: eventstoreExpect( - t, + eventstore: expectEventstore( expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), @@ -808,7 +955,7 @@ func TestCommandSide_AddUserGrant(t *testing.T) { ), ), ), - ), + )(t), } // we use an empty context and only rely on the permission check implementation _, err := r.AddUserGrant(context.Background(), &domain.UserGrant{ @@ -1479,12 +1626,108 @@ func TestCommandSide_ChangeUserGrant(t *testing.T) { expectFilter( eventFromEventPusher( usergrant.NewUserGrantAddedEvent(context.Background(), - &usergrant.NewAggregate("usergrant1", "org1").Aggregate, + &usergrant.NewAggregate("usergrant1", "org2").Aggregate, "user1", "project1", "projectgrant1", []string{"rolekey1"}), ), ), + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username1", + "firstname1", + "lastname1", + "nickname1", + "displayname1", + language.German, + domain.GenderMale, + "email1", + true, + ), + ), + eventFromEventPusher( + project.NewProjectAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "projectname1", true, true, true, + domain.PrivateLabelingSettingUnspecified, + ), + ), + eventFromEventPusher( + project.NewRoleAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "rolekey1", + "rolekey", + "", + ), + ), + eventFromEventPusher( + project.NewRoleAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "rolekey2", + "rolekey2", + "", + ), + ), + eventFromEventPusher( + project.NewGrantAddedEvent(context.Background(), + &project.NewAggregate("project1", "org1").Aggregate, + "projectgrant1", + "org2", + []string{"rolekey1", "rolekey2"}, + ), + ), + ), + expectPush( + usergrant.NewUserGrantChangedEvent(context.Background(), + &usergrant.NewAggregate("usergrant1", "org2").Aggregate, + "user1", + []string{"rolekey1", "rolekey2"}, + ), + ), + ), + }, + args: args{ + ctx: authz.NewMockContextWithPermissions("", "", "", []string{domain.RoleProjectOwner}), + userGrant: &domain.UserGrant{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "usergrant1", + ResourceOwner: "org2", + }, + UserID: "user1", + ProjectID: "project1", + ProjectGrantID: "projectgrant1", + RoleKeys: []string{"rolekey1", "rolekey2"}, + }, + }, + res: res{ + want: &domain.UserGrant{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "usergrant1", + ResourceOwner: "org2", + }, + UserID: "user1", + ProjectID: "project1", + ProjectGrantID: "projectgrant1", + RoleKeys: []string{"rolekey1", "rolekey2"}, + State: domain.UserGrantStateActive, + }, + }, + }, + { + name: "usergrant for projectgrant same resource owner, ok", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + usergrant.NewUserGrantAddedEvent(context.Background(), + &usergrant.NewAggregate("usergrant1", "org1").Aggregate, + "user1", + "project1", + "", []string{"rolekey1"}), + ), + ), expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), @@ -1550,7 +1793,7 @@ func TestCommandSide_ChangeUserGrant(t *testing.T) { }, UserID: "user1", ProjectID: "project1", - ProjectGrantID: "projectgrant1", + ProjectGrantID: "", RoleKeys: []string{"rolekey1", "rolekey2"}, }, }, @@ -1562,7 +1805,7 @@ func TestCommandSide_ChangeUserGrant(t *testing.T) { }, UserID: "user1", ProjectID: "project1", - ProjectGrantID: "projectgrant1", + ProjectGrantID: "", RoleKeys: []string{"rolekey1", "rolekey2"}, State: domain.UserGrantStateActive, },