Skip to content

Commit 751a509

Browse files
javirlnjavirln
authored
fix(group): Only org admin/owners and group maintainers can list group members (#2269)
Signed-off-by: Javier Rodriguez <[email protected]>
1 parent 17dbde7 commit 751a509

File tree

5 files changed

+291
-96
lines changed

5 files changed

+291
-96
lines changed

‎app/controlplane/internal/service/group.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ import (
2020
"fmt"
2121

2222
pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1"
23+
"github.com/chainloop-dev/chainloop/app/controlplane/internal/usercontext"
24+
"github.com/chainloop-dev/chainloop/app/controlplane/internal/usercontext/entities"
25+
"github.com/chainloop-dev/chainloop/app/controlplane/pkg/authz"
2326
"github.com/chainloop-dev/chainloop/app/controlplane/pkg/biz"
2427

2528
"github.com/go-kratos/kratos/v2/errors"
@@ -233,17 +236,33 @@ func (g *GroupService) ListMembers(ctx context.Context, req *pb.GroupServiceList
233236
return nil, err
234237
}
235238

239+
if err := g.userHasPermissionToListGroupMember(ctx, currentOrg.ID, req.GetGroupReference()); err != nil {
240+
return nil, err
241+
}
242+
243+
currentUser, err := requireCurrentUser(ctx)
244+
if err != nil {
245+
return nil, err
246+
}
247+
236248
// Parse orgID
237249
orgUUID, err := uuid.Parse(currentOrg.ID)
238250
if err != nil {
239251
return nil, errors.BadRequest("invalid", "invalid organization ID")
240252
}
241253

254+
// Parse requesterID (current user)
255+
requesterUUID, err := uuid.Parse(currentUser.ID)
256+
if err != nil {
257+
return nil, errors.BadRequest("invalid", "invalid user ID")
258+
}
259+
242260
// Initialize the options for listing members
243261
opts := &biz.ListMembersOpts{
244262
IdentityReference: &biz.IdentityReference{},
245263
Maintainers: req.Maintainers,
246264
MemberEmail: req.MemberEmail,
265+
RequesterID: requesterUUID,
247266
}
248267

249268
// Parse groupID and groupName from the request
@@ -531,6 +550,79 @@ func (g *GroupService) ListProjects(ctx context.Context, req *pb.GroupServiceLis
531550
}, nil
532551
}
533552

553+
// userHasPermissionToAddGroupMember checks if the user has permission to add members to a group
554+
func (g *GroupService) userHasPermissionToAddGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
555+
return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupAddMemberships)
556+
}
557+
558+
// userHasPermissionToRemoveGroupMember checks if the user has permission to remove members from a group
559+
func (g *GroupService) userHasPermissionToRemoveGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
560+
return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupRemoveMemberships)
561+
}
562+
563+
// userHasPermissionToListPendingGroupInvitations checks if the user has permission to list pending group invitations
564+
func (g *GroupService) userHasPermissionToListPendingGroupInvitations(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
565+
return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupListPendingInvitations)
566+
}
567+
568+
// userHasPermissionToListGroupMember checks if the user has permission to list group members
569+
func (g *GroupService) userHasPermissionToListGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
570+
return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupListMemberships)
571+
}
572+
573+
// userHasPermissionToUpdateMembership checks if the user has permission to remove members from a group
574+
func (g *GroupService) userHasPermissionToUpdateMembership(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
575+
return g.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupUpdateMemberships)
576+
}
577+
578+
// userHasPermissionOnGroupMembershipsWithPolicy is the core implementation that checks if a user has permission on a group
579+
// with an optional specific policy check. If the policy is nil, it falls back to the basic permission check.
580+
func (g *GroupService) userHasPermissionOnGroupMembershipsWithPolicy(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference, policy *authz.Policy) error {
581+
// Check if the user has admin or owner role in the organization
582+
userRole := usercontext.CurrentAuthzSubject(ctx)
583+
if userRole == "" {
584+
return errors.NotFound("not found", "current membership not found")
585+
}
586+
587+
// Allow if user has admin or owner role
588+
if userRole == string(authz.RoleAdmin) || userRole == string(authz.RoleOwner) {
589+
return nil
590+
}
591+
592+
groupID, groupName, err := groupIdentifier.Parse()
593+
if err != nil {
594+
return errors.BadRequest("invalid", fmt.Sprintf("invalid project reference: %s", err.Error()))
595+
}
596+
597+
orgUUID, err := uuid.Parse(orgID)
598+
if err != nil {
599+
return errors.BadRequest("invalid", "invalid organization ID")
600+
}
601+
602+
// Resolve the group identifier to a valid group ID
603+
resolvedGroupID, err := g.groupUseCase.ValidateGroupIdentifier(ctx, orgUUID, groupID, groupName)
604+
if err != nil {
605+
return handleUseCaseErr(err, g.log)
606+
}
607+
608+
// Check the user's membership in the organization
609+
m := entities.CurrentMembership(ctx)
610+
for _, rm := range m.Resources {
611+
if rm.ResourceType == authz.ResourceTypeGroup && rm.ResourceID == resolvedGroupID {
612+
pass, err := g.enforcer.Enforce(string(rm.Role), policy)
613+
if err != nil {
614+
return handleUseCaseErr(err, g.log)
615+
}
616+
if pass {
617+
return nil
618+
}
619+
}
620+
}
621+
622+
// If neither a maintainer nor admin/owner, nor has specific policy permission, forbid the operation
623+
return errors.Forbidden("forbidden", "operation not allowed")
624+
}
625+
534626
// bizGroupToPb converts a biz.Group to a pb.Group protobuf message.
535627
func bizGroupToPb(gr *biz.Group) *pb.Group {
536628
base := &pb.Group{

‎app/controlplane/internal/service/service.go‎

Lines changed: 0 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -253,74 +253,6 @@ func (s *service) userHasPermissionOnProject(ctx context.Context, orgID string,
253253
return p, nil
254254
}
255255

256-
// userHasPermissionToAddGroupMember checks if the user has permission to add members to a group
257-
func (s *service) userHasPermissionToAddGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
258-
return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupAddMemberships)
259-
}
260-
261-
// userHasPermissionToRemoveGroupMember checks if the user has permission to remove members from a group
262-
func (s *service) userHasPermissionToRemoveGroupMember(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
263-
return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupRemoveMemberships)
264-
}
265-
266-
// userHasPermissionToListPendingGroupInvitations checks if the user has permission to list pending group invitations
267-
func (s *service) userHasPermissionToListPendingGroupInvitations(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
268-
return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupListPendingInvitations)
269-
}
270-
271-
// userHasPermissionToUpdateMembership checks if the user has permission to remove members from a group
272-
func (s *service) userHasPermissionToUpdateMembership(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference) error {
273-
return s.userHasPermissionOnGroupMembershipsWithPolicy(ctx, orgID, groupIdentifier, authz.PolicyGroupUpdateMemberships)
274-
}
275-
276-
// userHasPermissionOnGroupMembershipsWithPolicy is the core implementation that checks if a user has permission on a group
277-
// with an optional specific policy check. If the policy is nil, it falls back to the basic permission check.
278-
func (s *service) userHasPermissionOnGroupMembershipsWithPolicy(ctx context.Context, orgID string, groupIdentifier *pb.IdentityReference, policy *authz.Policy) error {
279-
// Check if the user has admin or owner role in the organization
280-
userRole := usercontext.CurrentAuthzSubject(ctx)
281-
if userRole == "" {
282-
return errors.NotFound("not found", "current membership not found")
283-
}
284-
285-
// Allow if user has admin or owner role
286-
if userRole == string(authz.RoleAdmin) || userRole == string(authz.RoleOwner) {
287-
return nil
288-
}
289-
290-
groupID, groupName, err := groupIdentifier.Parse()
291-
if err != nil {
292-
return errors.BadRequest("invalid", fmt.Sprintf("invalid project reference: %s", err.Error()))
293-
}
294-
295-
orgUUID, err := uuid.Parse(orgID)
296-
if err != nil {
297-
return errors.BadRequest("invalid", "invalid organization ID")
298-
}
299-
300-
// Resolve the group identifier to a valid group ID
301-
resolvedGroupID, err := s.groupUseCase.ValidateGroupIdentifier(ctx, orgUUID, groupID, groupName)
302-
if err != nil {
303-
return handleUseCaseErr(err, s.log)
304-
}
305-
306-
// Check the user's membership in the organization
307-
m := entities.CurrentMembership(ctx)
308-
for _, rm := range m.Resources {
309-
if rm.ResourceType == authz.ResourceTypeGroup && rm.ResourceID == resolvedGroupID {
310-
pass, err := s.enforcer.Enforce(string(rm.Role), policy)
311-
if err != nil {
312-
return handleUseCaseErr(err, s.log)
313-
}
314-
if pass {
315-
return nil
316-
}
317-
}
318-
}
319-
320-
// If neither a maintainer nor admin/owner, nor has specific policy permission, forbid the operation
321-
return errors.Forbidden("forbidden", "operation not allowed")
322-
}
323-
324256
// visibleProjects returns projects where the user has any role (currently ProjectAdmin and ProjectViewer)
325257
func (s *service) visibleProjects(ctx context.Context) []uuid.UUID {
326258
if !rbacEnabled(ctx) {

‎app/controlplane/pkg/authz/authz.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,7 @@ var RolesMap = map[Role][]*Policy{
328328
// RoleGroupMaintainer: represents a group maintainer role.
329329
RoleGroupMaintainer: {
330330
PolicyGroupListPendingInvitations,
331+
PolicyGroupListMemberships,
331332
PolicyGroupAddMemberships,
332333
PolicyGroupRemoveMemberships,
333334
PolicyGroupUpdateMemberships,
@@ -399,10 +400,10 @@ var ServerOperationsMap = map[string][]*Policy{
399400
"/controlplane.v1.GroupService/List": {PolicyGroupList},
400401
"/controlplane.v1.GroupService/Get": {PolicyGroupRead},
401402
// Group Memberships
402-
"/controlplane.v1.GroupService/ListMembers": {PolicyGroupListMemberships},
403403
"/controlplane.v1.GroupService/ListProjects": {PolicyGroupListProjects},
404404
// For the following endpoints, we rely on the service layer to check the permissions
405405
// That's why we let everyone access them (empty policies)
406+
"/controlplane.v1.GroupService/ListMembers": {},
406407
"/controlplane.v1.GroupService/AddMember": {},
407408
"/controlplane.v1.GroupService/RemoveMember": {},
408409
"/controlplane.v1.GroupService/ListPendingInvitations": {},

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

Lines changed: 14 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,9 @@ type ListMembersOpts struct {
128128
Maintainers *bool
129129
// MemberEmail is the email of the member to filter by.
130130
MemberEmail *string
131+
// RequesterID is the ID of the user who is requesting to list mmebers. Optional.
132+
// If provided, the requester must be a maintainer or admin.
133+
RequesterID uuid.UUID
131134
}
132135

133136
// 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 *
230233
return nil, 0, err
231234
}
232235

236+
// Validate requester permissions only if RequesterID is provided
237+
if opts.RequesterID != uuid.Nil {
238+
if err := uc.validateRequesterPermissions(ctx, orgID, opts.RequesterID, resolvedGroupID); err != nil {
239+
return nil, 0, fmt.Errorf("failed to validate requester permissions: %w", err)
240+
}
241+
}
242+
233243
pgOpts := pagination.NewDefaultOffsetPaginationOpts()
234244
if paginationOpts != nil {
235245
pgOpts = paginationOpts
@@ -512,7 +522,7 @@ func (uc *GroupUseCase) validateRequesterPermissions(ctx context.Context, orgID,
512522

513523
// If not a maintainer of this group, deny access
514524
if requesterGroupMembership == nil || requesterGroupMembership.Role != authz.RoleGroupMaintainer {
515-
return NewErrValidationStr("requester does not have permission to add members to this group")
525+
return NewErrValidationStr("requester does not have permission to manage members on this group")
516526
}
517527

518528
return nil
@@ -633,33 +643,10 @@ func (uc *GroupUseCase) RemoveMemberFromGroup(ctx context.Context, orgID uuid.UU
633643
return NewErrNotFound("group")
634644
}
635645

646+
// Validate requester permissions only if RequesterID is provided
636647
if opts.RequesterID != uuid.Nil {
637-
// Check if the requester is part of the organization
638-
requesterMembership, err := uc.membershipRepo.FindByOrgAndUser(ctx, orgID, opts.RequesterID)
639-
if err != nil && !IsNotFound(err) {
640-
return NewErrValidationStr("failed to check existing membership")
641-
}
642-
643-
if requesterMembership == nil {
644-
return NewErrValidationStr("requester is not a member of the organization")
645-
}
646-
647-
// Check if the requester has sufficient permissions
648-
// Allow if the requester is an org owner or admin
649-
isAdminOrOwner := requesterMembership.Role == authz.RoleOwner || requesterMembership.Role == authz.RoleAdmin
650-
651-
// If not an admin/owner, check if the requester is a maintainer of this group
652-
if !isAdminOrOwner {
653-
// Check if the requester is a maintainer of this group
654-
requesterGroupMembership, err := uc.membershipRepo.FindByUserAndResourceID(ctx, opts.RequesterID, resolvedGroupID)
655-
if err != nil && !IsNotFound(err) {
656-
return fmt.Errorf("failed to check requester's group membership: %w", err)
657-
}
658-
659-
// If not a maintainer of this group, deny access
660-
if requesterGroupMembership == nil || requesterGroupMembership.Role != authz.RoleGroupMaintainer {
661-
return NewErrValidationStr("requester does not have permission to add members to this group")
662-
}
648+
if err := uc.validateRequesterPermissions(ctx, orgID, opts.RequesterID, resolvedGroupID); err != nil {
649+
return fmt.Errorf("failed to validate requester permissions: %w", err)
663650
}
664651
}
665652

0 commit comments

Comments
 (0)