From 4c26665b933dd763aa8c2779bce368708f7a7029 Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Wed, 24 Aug 2022 11:38:59 +0200 Subject: [PATCH] fix: improve user grants precondition checks (#4237) * fix: improve user grants precondition checks * build rc * fix prerelease * fix: build image * remove branch from releaserc --- internal/command/user_grant.go | 10 +-- internal/command/user_grant_model.go | 11 ++- internal/command/user_grant_test.go | 105 ++++++++++++++++++++++++++- 3 files changed, 116 insertions(+), 10 deletions(-) diff --git a/internal/command/user_grant.go b/internal/command/user_grant.go index cb5665c987..1e6c1be999 100644 --- a/internal/command/user_grant.go +++ b/internal/command/user_grant.go @@ -33,7 +33,7 @@ func (c *Commands) addUserGrant(ctx context.Context, userGrant *domain.UserGrant if !userGrant.IsValid() { return nil, nil, caos_errs.ThrowInvalidArgument(nil, "COMMAND-4M0fs", "Errors.UserGrant.Invalid") } - err = c.checkUserGrantPreCondition(ctx, userGrant) + err = c.checkUserGrantPreCondition(ctx, userGrant, resourceOwner) if err != nil { return nil, nil, err } @@ -91,7 +91,7 @@ func (c *Commands) changeUserGrant(ctx context.Context, userGrant *domain.UserGr } userGrant.ProjectID = existingUserGrant.ProjectID userGrant.ProjectGrantID = existingUserGrant.ProjectGrantID - err = c.checkUserGrantPreCondition(ctx, userGrant) + err = c.checkUserGrantPreCondition(ctx, userGrant, resourceOwner) if err != nil { return nil, nil, err } @@ -285,8 +285,8 @@ func (c *Commands) userGrantWriteModelByID(ctx context.Context, userGrantID, res return writeModel, nil } -func (c *Commands) checkUserGrantPreCondition(ctx context.Context, usergrant *domain.UserGrant) error { - preConditions := NewUserGrantPreConditionReadModel(usergrant.UserID, usergrant.ProjectID, usergrant.ProjectGrantID) +func (c *Commands) checkUserGrantPreCondition(ctx context.Context, usergrant *domain.UserGrant, resourceOwner string) error { + preConditions := NewUserGrantPreConditionReadModel(usergrant.UserID, usergrant.ProjectID, usergrant.ProjectGrantID, resourceOwner) err := c.eventstore.FilterToQueryReducer(ctx, preConditions) if err != nil { return err @@ -294,7 +294,7 @@ func (c *Commands) checkUserGrantPreCondition(ctx context.Context, usergrant *do if !preConditions.UserExists { return caos_errs.ThrowPreconditionFailed(err, "COMMAND-4f8sg", "Errors.User.NotFound") } - if !preConditions.ProjectExists { + if usergrant.ProjectGrantID == "" && !preConditions.ProjectExists { return caos_errs.ThrowPreconditionFailed(err, "COMMAND-3n77S", "Errors.Project.NotFound") } if usergrant.ProjectGrantID != "" && !preConditions.ProjectGrantExists { diff --git a/internal/command/user_grant_model.go b/internal/command/user_grant_model.go index 5156a750d5..b2490177d9 100644 --- a/internal/command/user_grant_model.go +++ b/internal/command/user_grant_model.go @@ -89,17 +89,19 @@ type UserGrantPreConditionReadModel struct { UserID string ProjectID string ProjectGrantID string + ResourceOwner string UserExists bool ProjectExists bool ProjectGrantExists bool ExistingRoleKeys []string } -func NewUserGrantPreConditionReadModel(userID, projectID, projectGrantID string) *UserGrantPreConditionReadModel { +func NewUserGrantPreConditionReadModel(userID, projectID, projectGrantID, resourceOwner string) *UserGrantPreConditionReadModel { return &UserGrantPreConditionReadModel{ UserID: userID, ProjectID: projectID, ProjectGrantID: projectGrantID, + ResourceOwner: resourceOwner, } } @@ -115,17 +117,18 @@ func (wm *UserGrantPreConditionReadModel) Reduce() error { case *user.UserRemovedEvent: wm.UserExists = false case *project.ProjectAddedEvent: - wm.ProjectExists = true + if wm.ProjectGrantID == "" && wm.ResourceOwner == e.Aggregate().ResourceOwner { + wm.ProjectExists = true + } case *project.ProjectRemovedEvent: wm.ProjectExists = false case *project.GrantAddedEvent: - if wm.ProjectGrantID == e.GrantID { + if wm.ProjectGrantID == e.GrantID && wm.ResourceOwner == e.GrantedOrgID { wm.ProjectGrantExists = true wm.ExistingRoleKeys = e.RoleKeys } case *project.GrantChangedEvent: if wm.ProjectGrantID == e.GrantID { - wm.ProjectGrantExists = true wm.ExistingRoleKeys = e.RoleKeys } case *project.GrantRemovedEvent: diff --git a/internal/command/user_grant_test.go b/internal/command/user_grant_test.go index babd79f0f3..e11d423657 100644 --- a/internal/command/user_grant_test.go +++ b/internal/command/user_grant_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + "golang.org/x/text/language" + "github.com/zitadel/zitadel/internal/api/authz" "github.com/zitadel/zitadel/internal/domain" caos_errs "github.com/zitadel/zitadel/internal/errors" @@ -16,7 +18,6 @@ import ( "github.com/zitadel/zitadel/internal/repository/project" "github.com/zitadel/zitadel/internal/repository/user" "github.com/zitadel/zitadel/internal/repository/usergrant" - "golang.org/x/text/language" ) func TestCommandSide_AddUserGrant(t *testing.T) { @@ -149,6 +150,48 @@ func TestCommandSide_AddUserGrant(t *testing.T) { err: caos_errs.IsPreconditionFailed, }, }, + { + name: "project on other org, precondition error", + fields: fields{ + eventstore: eventstoreExpect( + t, + 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, + ), + ), + ), + ), + }, + args: args{ + ctx: authz.NewMockContextWithPermissions("", "org2", "user", []string{domain.RoleProjectOwner}), + userGrant: &domain.UserGrant{ + UserID: "user1", + ProjectID: "project1", + }, + resourceOwner: "org2", + }, + res: res{ + err: caos_errs.IsPreconditionFailed, + }, + }, { name: "project roles not existing, precondition error", fields: fields{ @@ -296,6 +339,66 @@ func TestCommandSide_AddUserGrant(t *testing.T) { err: caos_errs.IsPreconditionFailed, }, }, + { + name: "project grant on other org, precondition error", + fields: fields{ + eventstore: eventstoreExpect( + t, + 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", + "org3", + []string{"rolekey1"}, + ), + ), + ), + ), + }, + args: args{ + ctx: authz.NewMockContextWithPermissions("", "org2", "user", []string{domain.RoleProjectOwner}), + userGrant: &domain.UserGrant{ + UserID: "user1", + ProjectID: "project1", + ProjectGrantID: "projectgrant1", + RoleKeys: []string{"rolekey1"}, + }, + resourceOwner: "org2", + }, + res: res{ + err: caos_errs.IsPreconditionFailed, + }, + }, { name: "usergrant for project, ok", fields: fields{