Skip to content

fix(group): Only org admin/owners and group maintainers can list group members #2269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 18, 2025
Merged

fix(group): Only org admin/owners and group maintainers can list group members #2269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51806b2
fix(group): Only org admin/owners and group maintainers can list grou…
javirln Jul 17, 2025
b6ecc71
remove duplicated code
javirln Jul 17, 2025
ffb373e
change string
javirln Jul 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions app/controlplane/internal/service/group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just for testing? or is it needed for any use case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same way we have AddMembers and `RemoveMembers, I pass the requester to check the permissions on biz, only if present and to test it, yes. In this way we allow third party code to call this code without the problem of forcing a requester.

}

// Parse groupID and groupName from the request
Expand Down Expand Up @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved all these methods from servier to GroupService and this is the new one that checks the policy for listing members in a group

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{
Expand Down
68 changes: 0 additions & 68 deletions app/controlplane/internal/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion app/controlplane/pkg/authz/authz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ var RolesMap = map[Role][]*Policy{
// RoleGroupMaintainer: represents a group maintainer role.
RoleGroupMaintainer: {
PolicyGroupListPendingInvitations,
PolicyGroupListMemberships,
PolicyGroupAddMemberships,
PolicyGroupRemoveMemberships,
PolicyGroupUpdateMemberships,
Expand Down Expand Up @@ -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": {},
Expand Down
41 changes: 14 additions & 27 deletions app/controlplane/pkg/biz/group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this requester part? When is it meant to not to exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the calling this method from a third party code. For example, we have another service calling AddMembers and RemoveMembers where we don't check the requester because it's a system request.

It's basically here to check permissions a part from the service

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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this code duplicated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this code can be found in validateRequesterPermissions that's why I removed it. I must have left it here after some refactoring.

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)
}
}

Expand Down
Loading
Loading