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 <livio.a@gmail.com>
(cherry picked from commit 8e60cce20d)

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 {

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
+	UserID                  string
-	ProjectID            string
+	ProjectID               string
-	ProjectResourceOwner string
+	ProjectResourceOwner    string
-	ProjectGrantID       string
+	ProjectGrantID          string
-	ResourceOwner        string
+	FoundGrantID            string
-	UserExists           bool
+	ResourceOwner           string
-	ProjectExists        bool
+	UserExists              bool
-	ProjectGrantExists   bool
+	ProjectExists           bool
-	ExistingRoleKeys     []string
+	ExistingRoleKeysProject []string
+	ExistingRoleKeysGrant   []string
 }
 
 func NewUserGrantPreConditionReadModel(userID, projectID, projectGrantID string, resourceOwner string) *UserGrantPreConditionReadModel {
 	return &UserGrantPreConditionReadModel{
-		UserID:         userID,
+		UserID:    userID,
-		ProjectID:      projectID,
+		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 {
+			if projectGrantExistsOnOrganization(wm.ProjectGrantID, wm.ResourceOwner, e.GrantID, e.GrantedOrgID) {
-				wm.ProjectGrantExists = true
+				wm.ExistingRoleKeysGrant = e.RoleKeys
-				wm.ExistingRoleKeys = e.RoleKeys
+				wm.FoundGrantID = e.GrantID
-				if wm.ProjectGrantID == "" {
-					wm.ProjectGrantID = e.GrantID
-				}
 			}
 		case *project.GrantChangedEvent:
-			if wm.ProjectGrantID == e.GrantID {
+			if wm.FoundGrantID == e.GrantID {
-				wm.ExistingRoleKeys = e.RoleKeys
+				wm.ExistingRoleKeysGrant = e.RoleKeys
 			}
 		case *project.GrantRemovedEvent:
-			if wm.ProjectGrantID == e.GrantID {
+			if wm.FoundGrantID == e.GrantID {
-				wm.ProjectGrantExists = false
+				wm.ExistingRoleKeysGrant = []string{}
-				wm.ExistingRoleKeys = []string{}
+				wm.FoundGrantID = ""
 			}
 		case *project.RoleAddedEvent:
-			if wm.ProjectGrantID != "" {
+			wm.ExistingRoleKeysProject = append(wm.ExistingRoleKeysProject, e.Key)
-				continue
-			}
-			wm.ExistingRoleKeys = append(wm.ExistingRoleKeys, e.Key)
 		case *project.RoleRemovedEvent:
-			if wm.ProjectGrantID != "" {
+			wm.ExistingRoleKeysProject = slices.DeleteFunc(wm.ExistingRoleKeysProject, func(key string) bool {
-				continue
+				return key == e.Key
-			}
+			})
-			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
-				}
-			}
 		}
 	}
 	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().

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 {
-		for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
-			t.Run(tt.name, func(t *testing.T) {
+			r := &Commands{
-				r := &Commands{
+				eventstore: tt.fields.eventstore(t),
-					eventstore: tt.fields.eventstore(t),
+			}
-				}
+			if tt.fields.idGenerator != nil {
-				if tt.fields.idGenerator != nil {
+				r.idGenerator = tt.fields.idGenerator(t)
-					r.idGenerator = tt.fields.idGenerator(t)
+			}
-				}
+			got, err := r.AddUserGrant(tt.args.ctx, tt.args.userGrant, succeedingUserGrantPermissionCheck)
-				got, err := r.AddUserGrant(tt.args.ctx, tt.args.userGrant, nil)
+			if tt.res.err == nil {
-				if tt.res.err == nil {
+				assert.NoError(t, err)
-					assert.NoError(t, err)
+			}
-				}
+			if tt.res.err != nil && !tt.res.err(err) {
-				if tt.res.err != nil && !tt.res.err(err) {
+				t.Errorf("got wrong err: %v ", err)
-					t.Errorf("got wrong err: %v ", err)
+			}
-				}
+			if tt.res.err == nil {
-				if tt.res.err == nil {
+				assert.Equal(t, tt.res.want, got)
-					assert.Equal(t, tt.res.want, got)
+			}
-				}
+		})
-			})
+	}
-		}
+}
-	})
+
-	t.Run("with succeeding permission check", func(t *testing.T) {
+func TestCommandSide_AddUserGrant_permission_failed(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)
-				}
-			})
-		}
-	})
 	t.Run("with failing permission check", func(t *testing.T) {
 		r := &Commands{
-			eventstore: eventstoreExpect(
+			eventstore: expectEventstore(
-				t,
 				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,
 				},