Skip to content

Commit b1e76a8

Browse files
jiparisjiparis
authored
feat(rbac): skip invalid role combinations (#2266)
Signed-off-by: Jose I. Paris <[email protected]>
1 parent fd347bd commit b1e76a8

File tree

4 files changed

+112
-8
lines changed

4 files changed

+112
-8
lines changed

‎app/controlplane/pkg/biz/membership.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,21 @@ func (uc *MembershipUseCase) ListAllMembershipsForUser(ctx context.Context, user
340340
return nil, fmt.Errorf("failed to list group memberships for user: %w", err)
341341
}
342342

343-
return append(userMemberships, groupMemberships...), nil
343+
// remove incompatible/illegal combinations (org viewer and project admin)
344+
combined := make([]*Membership, 0)
345+
combined = append(combined, userMemberships...)
346+
for _, um := range userMemberships {
347+
for _, gm := range groupMemberships {
348+
if um.ResourceType == authz.ResourceTypeOrganization && um.Role == authz.RoleViewer &&
349+
gm.Role == authz.RoleProjectAdmin && gm.OrganizationID == um.OrganizationID {
350+
// if user is org viewer and project admin through a group, skip it.
351+
continue
352+
}
353+
combined = append(combined, gm)
354+
}
355+
}
356+
357+
return combined, nil
344358
}
345359

346360
// SetProjectOwner sets the project owner (admin role). It skips the operation if an owner exists already

‎app/controlplane/pkg/biz/membership_integration_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package biz_test
1717

1818
import (
1919
"context"
20+
"slices"
2021
"testing"
2122

2223
"github.com/chainloop-dev/chainloop/app/controlplane/pkg/authz"
@@ -486,6 +487,90 @@ func (s *membershipIntegrationTestSuite) TestDeleteWithGroups() {
486487
})
487488
}
488489

490+
func (s *membershipIntegrationTestSuite) TestListAllMemberships() {
491+
// test illegal combinations (viewer and project admin)
492+
493+
ctx := context.Background()
494+
495+
// Create a user
496+
user, err := s.User.UpsertByEmail(ctx, "[email protected]", nil)
497+
s.NoError(err)
498+
userUUID := uuid.MustParse(user.ID)
499+
500+
// Create an organization
501+
org, err := s.Organization.CreateWithRandomName(ctx)
502+
s.NoError(err)
503+
orgUUID := uuid.MustParse(org.ID)
504+
505+
// Add user to organization
506+
_, err = s.Membership.Create(ctx, org.ID, user.ID, biz.WithMembershipRole(authz.RoleViewer), biz.WithCurrentMembership())
507+
s.NoError(err)
508+
509+
groupProjectAdmin, err := s.Group.Create(ctx, orgUUID, "group-admin", "Group project admin", nil)
510+
s.NoError(err)
511+
512+
groupProjectViewer, err := s.Group.Create(ctx, orgUUID, "group-viewer", "Group project viewer", nil)
513+
s.NoError(err)
514+
515+
// Add user to both groups
516+
_, err = s.Group.AddMemberToGroup(ctx, orgUUID, &biz.AddMemberToGroupOpts{
517+
IdentityReference: &biz.IdentityReference{ID: &groupProjectAdmin.ID},
518+
UserEmail: user.Email,
519+
})
520+
s.NoError(err)
521+
522+
_, err = s.Group.AddMemberToGroup(ctx, orgUUID, &biz.AddMemberToGroupOpts{
523+
IdentityReference: &biz.IdentityReference{ID: &groupProjectViewer.ID},
524+
UserEmail: user.Email,
525+
})
526+
s.NoError(err)
527+
528+
// Create a project
529+
pr, err := s.Project.Create(ctx, org.ID, "test-project")
530+
s.NoError(err)
531+
projectRef := &biz.IdentityReference{ID: &pr.ID}
532+
533+
// try to add user to project as project admin
534+
_, err = s.Project.AddMemberToProject(ctx, orgUUID, &biz.AddMemberToProjectOpts{
535+
ProjectReference: projectRef,
536+
UserEmail: user.Email,
537+
Role: authz.RoleProjectAdmin,
538+
})
539+
// Expect error because of an illegal combination
540+
s.Error(err)
541+
542+
// Add group admin to project as project admin
543+
_, err = s.Project.AddMemberToProject(ctx, orgUUID, &biz.AddMemberToProjectOpts{
544+
ProjectReference: projectRef,
545+
GroupReference: &biz.IdentityReference{ID: &groupProjectAdmin.ID},
546+
Role: authz.RoleProjectAdmin,
547+
})
548+
s.NoError(err)
549+
550+
// User shouldn't acquire the project admin role
551+
mm, err := s.Membership.ListAllMembershipsForUser(ctx, userUUID)
552+
s.NoError(err)
553+
// Expect only org membership
554+
s.Equal(1, len(mm))
555+
s.Equal(authz.ResourceTypeOrganization, mm[0].ResourceType)
556+
557+
// Add group viewer
558+
_, err = s.Project.AddMemberToProject(ctx, orgUUID, &biz.AddMemberToProjectOpts{
559+
ProjectReference: projectRef,
560+
GroupReference: &biz.IdentityReference{ID: &groupProjectViewer.ID},
561+
Role: authz.RoleProjectViewer,
562+
})
563+
s.NoError(err)
564+
565+
// expect user to acquire the membership
566+
mm, err = s.Membership.ListAllMembershipsForUser(ctx, userUUID)
567+
s.NoError(err)
568+
s.Equal(2, len(mm))
569+
s.True(slices.ContainsFunc(mm, func(m *biz.Membership) bool {
570+
return m.ResourceType == authz.ResourceTypeProject && m.Role == authz.RoleProjectViewer && m.ResourceID == pr.ID
571+
}))
572+
}
573+
489574
// Run the tests
490575
func TestMembershipUseCase(t *testing.T) {
491576
suite.Run(t, new(membershipIntegrationTestSuite))

‎app/controlplane/pkg/biz/project.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,11 @@ func (uc *ProjectUseCase) addUserToProject(ctx context.Context, orgID uuid.UUID,
323323
return uc.handleNonExistingUser(ctx, orgID, projectID, opts)
324324
}
325325

326+
// Org viewers cannot be added as project admin, since they cannot perform updates on resources
327+
if opts.Role == authz.RoleProjectAdmin && userMembership.Role == authz.RoleViewer {
328+
return nil, NewErrValidationStr("users with org role Org Viewer cannot be Project Admins")
329+
}
330+
326331
userUUID := uuid.MustParse(userMembership.User.ID)
327332

328333
// Check if the user is already a member of the project

‎app/controlplane/pkg/biz/project_integration_test.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ func (s *projectMembersIntegrationTestSuite) TestListMembers() {
9191
// Add users to organization
9292
_, err = s.Membership.Create(ctx, s.org.ID, user2.ID)
9393
require.NoError(s.T(), err)
94-
_, err = s.Membership.Create(ctx, s.org.ID, user3.ID)
94+
_, err = s.Membership.Create(ctx, s.org.ID, user3.ID, biz.WithMembershipRole(authz.RoleOrgMember))
9595
require.NoError(s.T(), err)
9696

9797
// Add users to the project
@@ -201,7 +201,7 @@ func (s *projectMembersIntegrationTestSuite) TestAddMemberToProject() {
201201
// Add users to organization
202202
_, err = s.Membership.Create(ctx, s.org.ID, user2.ID)
203203
require.NoError(s.T(), err)
204-
_, err = s.Membership.Create(ctx, s.org.ID, user3.ID)
204+
_, err = s.Membership.Create(ctx, s.org.ID, user3.ID, biz.WithMembershipRole(authz.RoleOrgMember))
205205
require.NoError(s.T(), err)
206206

207207
projectID := s.project.ID
@@ -421,7 +421,7 @@ func (s *projectMembersIntegrationTestSuite) TestRemoveMemberFromProject() {
421421
// Add users to organization
422422
_, err = s.Membership.Create(ctx, s.org.ID, user2.ID)
423423
require.NoError(s.T(), err)
424-
_, err = s.Membership.Create(ctx, s.org.ID, user3.ID)
424+
_, err = s.Membership.Create(ctx, s.org.ID, user3.ID, biz.WithMembershipRole(authz.RoleOrgMember))
425425
require.NoError(s.T(), err)
426426
_, err = s.Membership.Create(ctx, s.org.ID, user4.ID)
427427
require.NoError(s.T(), err)
@@ -649,7 +649,7 @@ func (s *projectAdminPermissionsTestSuite) TestAdminPermissions() {
649649
require.NoError(s.T(), err)
650650

651651
// Add the user to the organization
652-
_, err = s.Membership.Create(ctx, s.org.ID, user2.ID, biz.WithCurrentMembership())
652+
_, err = s.Membership.Create(ctx, s.org.ID, user2.ID, biz.WithCurrentMembership(), biz.WithMembershipRole(authz.RoleOrgMember))
653653
require.NoError(s.T(), err)
654654

655655
// Grant project admin role to the user
@@ -770,15 +770,15 @@ func (s *projectPermissionsTestSuite) SetupTest() {
770770
assert.NoError(err)
771771

772772
// Add project admin user to organization as a regular member
773-
_, err = s.Membership.Create(ctx, s.org.ID, s.projectAdminUser.ID, biz.WithCurrentMembership())
773+
_, err = s.Membership.Create(ctx, s.org.ID, s.projectAdminUser.ID, biz.WithCurrentMembership(), biz.WithMembershipRole(authz.RoleOrgMember))
774774
assert.NoError(err)
775775

776776
// Create a regular user
777777
s.regularUser, err = s.User.UpsertByEmail(ctx, fmt.Sprintf("regular-user-%[email protected]", uuid.New().String()), nil)
778778
assert.NoError(err)
779779

780780
// Add regular user to organization as a regular member
781-
_, err = s.Membership.Create(ctx, s.org.ID, s.regularUser.ID)
781+
_, err = s.Membership.Create(ctx, s.org.ID, s.regularUser.ID, biz.WithMembershipRole(authz.RoleOrgMember))
782782
assert.NoError(err)
783783

784784
// Create a project for tests
@@ -1340,7 +1340,7 @@ func (s *projectMembersIntegrationTestSuite) TestUpdateUserRoleInProject() {
13401340
// Add users to organization
13411341
_, err = s.Membership.Create(ctx, s.org.ID, user1.ID)
13421342
require.NoError(s.T(), err)
1343-
_, err = s.Membership.Create(ctx, s.org.ID, user2.ID)
1343+
_, err = s.Membership.Create(ctx, s.org.ID, user2.ID, biz.WithMembershipRole(authz.RoleOrgMember))
13441344
require.NoError(s.T(), err)
13451345

13461346
projectID := s.project.ID

0 commit comments

Comments
 (0)