From 10edeff96120efec0927531c26deb465aba78b42 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Wed, 16 Jul 2025 13:33:37 +0200 Subject: [PATCH] skip invalid combinations from groups Signed-off-by: Jose I. Paris --- app/controlplane/pkg/biz/membership.go | 16 +++- .../pkg/biz/membership_integration_test.go | 85 +++++++++++++++++++ app/controlplane/pkg/biz/project.go | 5 ++ .../pkg/biz/project_integration_test.go | 14 +-- 4 files changed, 112 insertions(+), 8 deletions(-) diff --git a/app/controlplane/pkg/biz/membership.go b/app/controlplane/pkg/biz/membership.go index 539a2103c..8bb9332ac 100644 --- a/app/controlplane/pkg/biz/membership.go +++ b/app/controlplane/pkg/biz/membership.go @@ -340,7 +340,21 @@ func (uc *MembershipUseCase) ListAllMembershipsForUser(ctx context.Context, user return nil, fmt.Errorf("failed to list group memberships for user: %w", err) } - return append(userMemberships, groupMemberships...), nil + // remove incompatible/illegal combinations (org viewer and project admin) + combined := make([]*Membership, 0) + combined = append(combined, userMemberships...) + for _, um := range userMemberships { + for _, gm := range groupMemberships { + if um.ResourceType == authz.ResourceTypeOrganization && um.Role == authz.RoleViewer && + gm.Role == authz.RoleProjectAdmin && gm.OrganizationID == um.OrganizationID { + // if user is org viewer and project admin through a group, skip it. + continue + } + combined = append(combined, gm) + } + } + + return combined, nil } // SetProjectOwner sets the project owner (admin role). It skips the operation if an owner exists already diff --git a/app/controlplane/pkg/biz/membership_integration_test.go b/app/controlplane/pkg/biz/membership_integration_test.go index 3ec61a4d5..433b6de4f 100644 --- a/app/controlplane/pkg/biz/membership_integration_test.go +++ b/app/controlplane/pkg/biz/membership_integration_test.go @@ -17,6 +17,7 @@ package biz_test import ( "context" + "slices" "testing" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/authz" @@ -486,6 +487,90 @@ func (s *membershipIntegrationTestSuite) TestDeleteWithGroups() { }) } +func (s *membershipIntegrationTestSuite) TestListAllMemberships() { + // test illegal combinations (viewer and project admin) + + ctx := context.Background() + + // Create a user + user, err := s.User.UpsertByEmail(ctx, "user@example.com", nil) + s.NoError(err) + userUUID := uuid.MustParse(user.ID) + + // Create an organization + org, err := s.Organization.CreateWithRandomName(ctx) + s.NoError(err) + orgUUID := uuid.MustParse(org.ID) + + // Add user to organization + _, err = s.Membership.Create(ctx, org.ID, user.ID, biz.WithMembershipRole(authz.RoleViewer), biz.WithCurrentMembership()) + s.NoError(err) + + groupProjectAdmin, err := s.Group.Create(ctx, orgUUID, "group-admin", "Group project admin", nil) + s.NoError(err) + + groupProjectViewer, err := s.Group.Create(ctx, orgUUID, "group-viewer", "Group project viewer", nil) + s.NoError(err) + + // Add user to both groups + _, err = s.Group.AddMemberToGroup(ctx, orgUUID, &biz.AddMemberToGroupOpts{ + IdentityReference: &biz.IdentityReference{ID: &groupProjectAdmin.ID}, + UserEmail: user.Email, + }) + s.NoError(err) + + _, err = s.Group.AddMemberToGroup(ctx, orgUUID, &biz.AddMemberToGroupOpts{ + IdentityReference: &biz.IdentityReference{ID: &groupProjectViewer.ID}, + UserEmail: user.Email, + }) + s.NoError(err) + + // Create a project + pr, err := s.Project.Create(ctx, org.ID, "test-project") + s.NoError(err) + projectRef := &biz.IdentityReference{ID: &pr.ID} + + // try to add user to project as project admin + _, err = s.Project.AddMemberToProject(ctx, orgUUID, &biz.AddMemberToProjectOpts{ + ProjectReference: projectRef, + UserEmail: user.Email, + Role: authz.RoleProjectAdmin, + }) + // Expect error because of an illegal combination + s.Error(err) + + // Add group admin to project as project admin + _, err = s.Project.AddMemberToProject(ctx, orgUUID, &biz.AddMemberToProjectOpts{ + ProjectReference: projectRef, + GroupReference: &biz.IdentityReference{ID: &groupProjectAdmin.ID}, + Role: authz.RoleProjectAdmin, + }) + s.NoError(err) + + // User shouldn't acquire the project admin role + mm, err := s.Membership.ListAllMembershipsForUser(ctx, userUUID) + s.NoError(err) + // Expect only org membership + s.Equal(1, len(mm)) + s.Equal(authz.ResourceTypeOrganization, mm[0].ResourceType) + + // Add group viewer + _, err = s.Project.AddMemberToProject(ctx, orgUUID, &biz.AddMemberToProjectOpts{ + ProjectReference: projectRef, + GroupReference: &biz.IdentityReference{ID: &groupProjectViewer.ID}, + Role: authz.RoleProjectViewer, + }) + s.NoError(err) + + // expect user to acquire the membership + mm, err = s.Membership.ListAllMembershipsForUser(ctx, userUUID) + s.NoError(err) + s.Equal(2, len(mm)) + s.True(slices.ContainsFunc(mm, func(m *biz.Membership) bool { + return m.ResourceType == authz.ResourceTypeProject && m.Role == authz.RoleProjectViewer && m.ResourceID == pr.ID + })) +} + // Run the tests func TestMembershipUseCase(t *testing.T) { suite.Run(t, new(membershipIntegrationTestSuite)) diff --git a/app/controlplane/pkg/biz/project.go b/app/controlplane/pkg/biz/project.go index 508d6d3df..df6131a7f 100644 --- a/app/controlplane/pkg/biz/project.go +++ b/app/controlplane/pkg/biz/project.go @@ -323,6 +323,11 @@ func (uc *ProjectUseCase) addUserToProject(ctx context.Context, orgID uuid.UUID, return uc.handleNonExistingUser(ctx, orgID, projectID, opts) } + // Org viewers cannot be added as project admin, since they cannot perform updates on resources + if opts.Role == authz.RoleProjectAdmin && userMembership.Role == authz.RoleViewer { + return nil, NewErrValidationStr("users with org role Org Viewer cannot be Project Admins") + } + userUUID := uuid.MustParse(userMembership.User.ID) // Check if the user is already a member of the project diff --git a/app/controlplane/pkg/biz/project_integration_test.go b/app/controlplane/pkg/biz/project_integration_test.go index a48858ff3..146405f05 100644 --- a/app/controlplane/pkg/biz/project_integration_test.go +++ b/app/controlplane/pkg/biz/project_integration_test.go @@ -91,7 +91,7 @@ func (s *projectMembersIntegrationTestSuite) TestListMembers() { // Add users to organization _, err = s.Membership.Create(ctx, s.org.ID, user2.ID) require.NoError(s.T(), err) - _, err = s.Membership.Create(ctx, s.org.ID, user3.ID) + _, err = s.Membership.Create(ctx, s.org.ID, user3.ID, biz.WithMembershipRole(authz.RoleOrgMember)) require.NoError(s.T(), err) // Add users to the project @@ -201,7 +201,7 @@ func (s *projectMembersIntegrationTestSuite) TestAddMemberToProject() { // Add users to organization _, err = s.Membership.Create(ctx, s.org.ID, user2.ID) require.NoError(s.T(), err) - _, err = s.Membership.Create(ctx, s.org.ID, user3.ID) + _, err = s.Membership.Create(ctx, s.org.ID, user3.ID, biz.WithMembershipRole(authz.RoleOrgMember)) require.NoError(s.T(), err) projectID := s.project.ID @@ -421,7 +421,7 @@ func (s *projectMembersIntegrationTestSuite) TestRemoveMemberFromProject() { // Add users to organization _, err = s.Membership.Create(ctx, s.org.ID, user2.ID) require.NoError(s.T(), err) - _, err = s.Membership.Create(ctx, s.org.ID, user3.ID) + _, err = s.Membership.Create(ctx, s.org.ID, user3.ID, biz.WithMembershipRole(authz.RoleOrgMember)) require.NoError(s.T(), err) _, err = s.Membership.Create(ctx, s.org.ID, user4.ID) require.NoError(s.T(), err) @@ -649,7 +649,7 @@ func (s *projectAdminPermissionsTestSuite) TestAdminPermissions() { require.NoError(s.T(), err) // Add the user to the organization - _, err = s.Membership.Create(ctx, s.org.ID, user2.ID, biz.WithCurrentMembership()) + _, err = s.Membership.Create(ctx, s.org.ID, user2.ID, biz.WithCurrentMembership(), biz.WithMembershipRole(authz.RoleOrgMember)) require.NoError(s.T(), err) // Grant project admin role to the user @@ -770,7 +770,7 @@ func (s *projectPermissionsTestSuite) SetupTest() { assert.NoError(err) // Add project admin user to organization as a regular member - _, err = s.Membership.Create(ctx, s.org.ID, s.projectAdminUser.ID, biz.WithCurrentMembership()) + _, err = s.Membership.Create(ctx, s.org.ID, s.projectAdminUser.ID, biz.WithCurrentMembership(), biz.WithMembershipRole(authz.RoleOrgMember)) assert.NoError(err) // Create a regular user @@ -778,7 +778,7 @@ func (s *projectPermissionsTestSuite) SetupTest() { assert.NoError(err) // Add regular user to organization as a regular member - _, err = s.Membership.Create(ctx, s.org.ID, s.regularUser.ID) + _, err = s.Membership.Create(ctx, s.org.ID, s.regularUser.ID, biz.WithMembershipRole(authz.RoleOrgMember)) assert.NoError(err) // Create a project for tests @@ -1340,7 +1340,7 @@ func (s *projectMembersIntegrationTestSuite) TestUpdateUserRoleInProject() { // Add users to organization _, err = s.Membership.Create(ctx, s.org.ID, user1.ID) require.NoError(s.T(), err) - _, err = s.Membership.Create(ctx, s.org.ID, user2.ID) + _, err = s.Membership.Create(ctx, s.org.ID, user2.ID, biz.WithMembershipRole(authz.RoleOrgMember)) require.NoError(s.T(), err) projectID := s.project.ID