diff --git a/app/controlplane/internal/service/group.go b/app/controlplane/internal/service/group.go index 0180f8f5a..b9b612efd 100644 --- a/app/controlplane/internal/service/group.go +++ b/app/controlplane/internal/service/group.go @@ -20,6 +20,9 @@ import ( "fmt" pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" + "github.com/chainloop-dev/chainloop/app/controlplane/internal/usercontext" + "github.com/chainloop-dev/chainloop/app/controlplane/internal/usercontext/entities" + "github.com/chainloop-dev/chainloop/app/controlplane/pkg/authz" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/biz" "github.com/go-kratos/kratos/v2/errors" @@ -233,17 +236,33 @@ func (g *GroupService) ListMembers(ctx context.Context, req *pb.GroupServiceList return nil, err } + if err := g.userHasPermissionToListGroupMember(ctx, currentOrg.ID, req.GetGroupReference()); err != nil { + return nil, err + } + + currentUser, err := requireCurrentUser(ctx) + if err != nil { + return nil, err + } + // Parse orgID orgUUID, err := uuid.Parse(currentOrg.ID) if err != nil { return nil, errors.BadRequest("invalid", "invalid organization ID") } + // Parse requesterID (current user) + requesterUUID, err := uuid.Parse(currentUser.ID) + if err != nil { + return nil, errors.BadRequest("invalid", "invalid user ID") + } + // Initialize the options for listing members opts := &biz.ListMembersOpts{ IdentityReference: &biz.IdentityReference{}, Maintainers: req.Maintainers, MemberEmail: req.MemberEmail, + RequesterID: requesterUUID, } // Parse groupID and groupName from the request @@ -531,6 +550,79 @@ func (g *GroupService) ListProjects(ctx context.Context, req *pb.GroupServiceLis }, nil } +// userHasPermissionToAddGroupMember checks if the user has permission to add members to a group +func (g *GroupService) userHasPermissionToAddGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { + return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupAddMemberships) +} + +// userHasPermissionToRemoveGroupMember checks if the user has permission to remove members from a group +func (g *GroupService) userHasPermissionToRemoveGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { + return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupRemoveMemberships) +} + +// userHasPermissionToListPendingGroupInvitations checks if the user has permission to list pending group invitations +func (g *GroupService) userHasPermissionToListPendingGroupInvitations(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { + return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupListPendingInvitations) +} + +// userHasPermissionToListGroupMember checks if the user has permission to list group members +func (g *GroupService) userHasPermissionToListGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { + return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupListMemberships) +} + +// userHasPermissionToUpdateMembership checks if the user has permission to remove members from a group +func (g *GroupService) userHasPermissionToUpdateMembership(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { + return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupUpdateMemberships) +} + +// userHasPermissionOnGroupMembershipsWithPolicy is the core implementation that checks if a user has permission on a group +// with an optional specific policy check. If the policy is nil, it falls back to the basic permission check. +func (g *GroupService) userHasPermissionOnGroupMembershipsWithPolicy(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference, policy *authz.Policy) error { + // Check if the user has admin or owner role in the organization + userRole := usercontext.CurrentAuthzSubject(ctx) + if userRole == "" { + return errors.NotFound("not found", "current membership not found") + } + + // Allow if user has admin or owner role + if userRole == string(authz.RoleAdmin) || userRole == string(authz.RoleOwner) { + return nil + } + + groupID, groupName, err := groupIdentifier.Parse() + if err != nil { + return errors.BadRequest("invalid", fmt.Sprintf("invalid project reference: %s", err.Error())) + } + + orgUUID, err := uuid.Parse(orgID) + if err != nil { + return errors.BadRequest("invalid", "invalid organization ID") + } + + // Resolve the group identifier to a valid group ID + resolvedGroupID, err := g.groupUseCase.ValidateGroupIdentifier(ctx, orgUUID, groupID, groupName) + if err != nil { + return handleUseCaseErr(err, g.log) + } + + // Check the user's membership in the organization + m := entities.CurrentMembership(ctx) + for _, rm := range m.Resources { + if rm.ResourceType == authz.ResourceTypeGroup && rm.ResourceID == resolvedGroupID { + pass, err := g.enforcer.Enforce(string(rm.Role), policy) + if err != nil { + return handleUseCaseErr(err, g.log) + } + if pass { + return nil + } + } + } + + // If neither a maintainer nor admin/owner, nor has specific policy permission, forbid the operation + return errors.Forbidden("forbidden", "operation not allowed") +} + // bizGroupToPb converts a biz.Group to a pb.Group protobuf message. func bizGroupToPb(gr *biz.Group) *pb.Group { base := &pb.Group{ diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index 9329dd224..63f794428 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -253,74 +253,6 @@ func (s *service) userHasPermissionOnProject(ctx context.Context, orgID string, return p, nil } -// userHasPermissionToAddGroupMember checks if the user has permission to add members to a group -func (s *service) userHasPermissionToAddGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { - return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupAddMemberships) -} - -// userHasPermissionToRemoveGroupMember checks if the user has permission to remove members from a group -func (s *service) userHasPermissionToRemoveGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { - return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupRemoveMemberships) -} - -// userHasPermissionToListPendingGroupInvitations checks if the user has permission to list pending group invitations -func (s *service) userHasPermissionToListPendingGroupInvitations(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { - return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupListPendingInvitations) -} - -// userHasPermissionToUpdateMembership checks if the user has permission to remove members from a group -func (s *service) userHasPermissionToUpdateMembership(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error { - return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupUpdateMemberships) -} - -// userHasPermissionOnGroupMembershipsWithPolicy is the core implementation that checks if a user has permission on a group -// with an optional specific policy check. If the policy is nil, it falls back to the basic permission check. -func (s *service) userHasPermissionOnGroupMembershipsWithPolicy(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference, policy *authz.Policy) error { - // Check if the user has admin or owner role in the organization - userRole := usercontext.CurrentAuthzSubject(ctx) - if userRole == "" { - return errors.NotFound("not found", "current membership not found") - } - - // Allow if user has admin or owner role - if userRole == string(authz.RoleAdmin) || userRole == string(authz.RoleOwner) { - return nil - } - - groupID, groupName, err := groupIdentifier.Parse() - if err != nil { - return errors.BadRequest("invalid", fmt.Sprintf("invalid project reference: %s", err.Error())) - } - - orgUUID, err := uuid.Parse(orgID) - if err != nil { - return errors.BadRequest("invalid", "invalid organization ID") - } - - // Resolve the group identifier to a valid group ID - resolvedGroupID, err := s.groupUseCase.ValidateGroupIdentifier(ctx, orgUUID, groupID, groupName) - if err != nil { - return handleUseCaseErr(err, s.log) - } - - // Check the user's membership in the organization - m := entities.CurrentMembership(ctx) - for _, rm := range m.Resources { - if rm.ResourceType == authz.ResourceTypeGroup && rm.ResourceID == resolvedGroupID { - pass, err := s.enforcer.Enforce(string(rm.Role), policy) - if err != nil { - return handleUseCaseErr(err, s.log) - } - if pass { - return nil - } - } - } - - // If neither a maintainer nor admin/owner, nor has specific policy permission, forbid the operation - return errors.Forbidden("forbidden", "operation not allowed") -} - // visibleProjects returns projects where the user has any role (currently ProjectAdmin and ProjectViewer) func (s *service) visibleProjects(ctx context.Context) []uuid.UUID { if !rbacEnabled(ctx) { diff --git a/app/controlplane/pkg/authz/authz.go b/app/controlplane/pkg/authz/authz.go index a62ad3cb3..4ae0d226e 100644 --- a/app/controlplane/pkg/authz/authz.go +++ b/app/controlplane/pkg/authz/authz.go @@ -328,6 +328,7 @@ var RolesMap = map[Role][]*Policy{ // RoleGroupMaintainer: represents a group maintainer role. RoleGroupMaintainer: { PolicyGroupListPendingInvitations, + PolicyGroupListMemberships, PolicyGroupAddMemberships, PolicyGroupRemoveMemberships, PolicyGroupUpdateMemberships, @@ -399,10 +400,10 @@ var ServerOperationsMap = map[string][]*Policy{ "/controlplane.v1.GroupService/List": {PolicyGroupList}, "/controlplane.v1.GroupService/Get": {PolicyGroupRead}, // Group Memberships - "/controlplane.v1.GroupService/ListMembers": {PolicyGroupListMemberships}, "/controlplane.v1.GroupService/ListProjects": {PolicyGroupListProjects}, // For the following endpoints, we rely on the service layer to check the permissions // That's why we let everyone access them (empty policies) + "/controlplane.v1.GroupService/ListMembers": {}, "/controlplane.v1.GroupService/AddMember": {}, "/controlplane.v1.GroupService/RemoveMember": {}, "/controlplane.v1.GroupService/ListPendingInvitations": {}, diff --git a/app/controlplane/pkg/biz/group.go b/app/controlplane/pkg/biz/group.go index 3f506410b..483dd48e8 100644 --- a/app/controlplane/pkg/biz/group.go +++ b/app/controlplane/pkg/biz/group.go @@ -128,6 +128,9 @@ type ListMembersOpts struct { Maintainers *bool // MemberEmail is the email of the member to filter by. MemberEmail *string + // RequesterID is the ID of the user who is requesting to list mmebers. Optional. + // If provided, the requester must be a maintainer or admin. + RequesterID uuid.UUID } // AddMemberToGroupOpts defines options for adding a member to a group. @@ -230,6 +233,13 @@ func (uc *GroupUseCase) ListMembers(ctx context.Context, orgID uuid.UUID, opts * return nil, 0, err } + // Validate requester permissions only if RequesterID is provided + if opts.RequesterID != uuid.Nil { + if err := uc.validateRequesterPermissions(ctx, orgID, opts.RequesterID, resolvedGroupID); err != nil { + return nil, 0, fmt.Errorf("failed to validate requester permissions: %w", err) + } + } + pgOpts := pagination.NewDefaultOffsetPaginationOpts() if paginationOpts != nil { pgOpts = paginationOpts @@ -512,7 +522,7 @@ func (uc *GroupUseCase) validateRequesterPermissions(ctx context.Context, orgID, // If not a maintainer of this group, deny access if requesterGroupMembership == nil || requesterGroupMembership.Role != authz.RoleGroupMaintainer { - return NewErrValidationStr("requester does not have permission to add members to this group") + return NewErrValidationStr("requester does not have permission to manage members on this group") } return nil @@ -633,33 +643,10 @@ func (uc *GroupUseCase) RemoveMemberFromGroup(ctx context.Context, orgID uuid.UU return NewErrNotFound("group") } + // Validate requester permissions only if RequesterID is provided if opts.RequesterID != uuid.Nil { - // Check if the requester is part of the organization - requesterMembership, err := uc.membershipRepo.FindByOrgAndUser(ctx, orgID, opts.RequesterID) - if err != nil && !IsNotFound(err) { - return NewErrValidationStr("failed to check existing membership") - } - - if requesterMembership == nil { - return NewErrValidationStr("requester is not a member of the organization") - } - - // Check if the requester has sufficient permissions - // Allow if the requester is an org owner or admin - isAdminOrOwner := requesterMembership.Role == authz.RoleOwner || requesterMembership.Role == authz.RoleAdmin - - // If not an admin/owner, check if the requester is a maintainer of this group - if !isAdminOrOwner { - // Check if the requester is a maintainer of this group - requesterGroupMembership, err := uc.membershipRepo.FindByUserAndResourceID(ctx, opts.RequesterID, resolvedGroupID) - if err != nil && !IsNotFound(err) { - return fmt.Errorf("failed to check requester's group membership: %w", err) - } - - // If not a maintainer of this group, deny access - if requesterGroupMembership == nil || requesterGroupMembership.Role != authz.RoleGroupMaintainer { - return NewErrValidationStr("requester does not have permission to add members to this group") - } + if err := uc.validateRequesterPermissions(ctx, orgID, opts.RequesterID, resolvedGroupID); err != nil { + return fmt.Errorf("failed to validate requester permissions: %w", err) } } diff --git a/app/controlplane/pkg/biz/group_integration_test.go b/app/controlplane/pkg/biz/group_integration_test.go index b1c5cd901..414afee70 100644 --- a/app/controlplane/pkg/biz/group_integration_test.go +++ b/app/controlplane/pkg/biz/group_integration_test.go @@ -689,6 +689,189 @@ func (s *groupMembersIntegrationTestSuite) TestListMembers() { s.Equal(1, count) s.Equal(s.user.ID, members[0].User.ID) }) + + s.Run("authorization tests for listing members", func() { + // Create additional users with different roles + regularUser, err := s.User.UpsertByEmail(ctx, "regular-user@example.com", nil) + require.NoError(s.T(), err) + + viewerUser, err := s.User.UpsertByEmail(ctx, "viewer-user@example.com", nil) + require.NoError(s.T(), err) + + adminUser, err := s.User.UpsertByEmail(ctx, "admin-user@example.com", nil) + require.NoError(s.T(), err) + + ownerUser, err := s.User.UpsertByEmail(ctx, "owner-user@example.com", nil) + require.NoError(s.T(), err) + + maintainerUser, err := s.User.UpsertByEmail(ctx, "maintainer-user@example.com", nil) + require.NoError(s.T(), err) + + // Add users to organization with different roles + _, err = s.Membership.Create(ctx, s.org.ID, regularUser.ID, biz.WithMembershipRole(authz.RoleOrgMember)) + require.NoError(s.T(), err) + + _, err = s.Membership.Create(ctx, s.org.ID, viewerUser.ID, biz.WithMembershipRole(authz.RoleViewer)) + require.NoError(s.T(), err) + + _, err = s.Membership.Create(ctx, s.org.ID, adminUser.ID, biz.WithMembershipRole(authz.RoleAdmin)) + require.NoError(s.T(), err) + + _, err = s.Membership.Create(ctx, s.org.ID, ownerUser.ID, biz.WithMembershipRole(authz.RoleOwner)) + require.NoError(s.T(), err) + + _, err = s.Membership.Create(ctx, s.org.ID, maintainerUser.ID, biz.WithMembershipRole(authz.RoleOrgMember)) + require.NoError(s.T(), err) + + // Create a second group and make maintainerUser a maintainer of it + maintainerUserUUID := uuid.MustParse(maintainerUser.ID) + secondGroup, err := s.Group.Create(ctx, uuid.MustParse(s.org.ID), "second-test-group", "Second group for testing", &maintainerUserUUID) + require.NoError(s.T(), err) + + // Add maintainerUser as a maintainer to the original test group + addMemberOpts := &biz.AddMemberToGroupOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + UserEmail: maintainerUser.Email, + RequesterID: uuid.MustParse(s.user.ID), // Original creator (admin) adds the maintainer + Maintainer: true, + } + _, err = s.Group.AddMemberToGroup(ctx, uuid.MustParse(s.org.ID), addMemberOpts) + require.NoError(s.T(), err) + + s.Run("org admin can list members", func() { + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + RequesterID: uuid.MustParse(adminUser.ID), + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.NoError(err) + s.Greater(count, 0) + s.Len(members, count) + }) + + s.Run("org owner can list members", func() { + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + RequesterID: uuid.MustParse(ownerUser.ID), + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.NoError(err) + s.Greater(count, 0) + s.Len(members, count) + }) + + s.Run("group maintainer can list members", func() { + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + RequesterID: uuid.MustParse(maintainerUser.ID), + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.NoError(err) + s.Greater(count, 0) + s.Len(members, count) + }) + + s.Run("regular org member cannot list members", func() { + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + RequesterID: uuid.MustParse(regularUser.ID), + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.Error(err) + s.Contains(err.Error(), "requester does not have permission") + s.Nil(members) + s.Equal(0, count) + }) + + s.Run("org viewer cannot list members", func() { + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + RequesterID: uuid.MustParse(viewerUser.ID), + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.Error(err) + s.Contains(err.Error(), "requester does not have permission") + s.Nil(members) + s.Equal(0, count) + }) + + s.Run("non-org member cannot list members", func() { + nonOrgUser, err := s.User.UpsertByEmail(ctx, "non-org-user@example.com", nil) + require.NoError(s.T(), err) + + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + RequesterID: uuid.MustParse(nonOrgUser.ID), + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.Error(err) + s.Contains(err.Error(), "requester is not a member of the organization") + s.Nil(members) + s.Equal(0, count) + }) + + s.Run("maintainer of different group cannot list members", func() { + // maintainerUser is a maintainer of secondGroup but not the original group (except we added them above) + // Let's create a third user who is maintainer of only the second group + thirdGroupMaintainer, err := s.User.UpsertByEmail(ctx, "third-group-maintainer@example.com", nil) + require.NoError(s.T(), err) + + _, err = s.Membership.Create(ctx, s.org.ID, thirdGroupMaintainer.ID, biz.WithMembershipRole(authz.RoleOrgMember)) + require.NoError(s.T(), err) + + // Add as maintainer to the second group only + addToSecondGroupOpts := &biz.AddMemberToGroupOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &secondGroup.ID, + }, + UserEmail: thirdGroupMaintainer.Email, + RequesterID: uuid.MustParse(maintainerUser.ID), // maintainerUser is the creator of secondGroup + Maintainer: true, + } + _, err = s.Group.AddMemberToGroup(ctx, uuid.MustParse(s.org.ID), addToSecondGroupOpts) + require.NoError(s.T(), err) + + // Try to list members of the original group (should fail) + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + RequesterID: uuid.MustParse(thirdGroupMaintainer.ID), + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.Error(err) + s.Contains(err.Error(), "requester does not have permission") + s.Nil(members) + s.Equal(0, count) + }) + + s.Run("listing members without requester ID works (backwards compatibility)", func() { + opts := &biz.ListMembersOpts{ + IdentityReference: &biz.IdentityReference{ + ID: &s.group.ID, + }, + // No RequesterID provided + } + members, count, err := s.Group.ListMembers(ctx, uuid.MustParse(s.org.ID), opts, nil) + s.NoError(err) + s.Greater(count, 0) + s.Len(members, count) + }) + }) } // Test adding members to groups