From bed46d8196e2ef93de9414dbeaf540402c5561b9 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 14:32:43 +0200 Subject: [PATCH 01/12] org viewers cannot become project admins Signed-off-by: Jose I. Paris --- app/controlplane/cmd/wire.go | 1 - app/controlplane/cmd/wire_gen.go | 2 +- app/controlplane/internal/service/group.go | 71 +++++++++++++++++ app/controlplane/internal/service/service.go | 81 ++------------------ app/controlplane/pkg/biz/project.go | 5 ++ 5 files changed, 82 insertions(+), 78 deletions(-) diff --git a/app/controlplane/cmd/wire.go b/app/controlplane/cmd/wire.go index 9c03ecb53..1e588dfff 100644 --- a/app/controlplane/cmd/wire.go +++ b/app/controlplane/cmd/wire.go @@ -100,7 +100,6 @@ func serviceOpts(l log.Logger, enforcer *authz.Enforcer, pUC *biz.ProjectUseCase service.WithLogger(l), service.WithEnforcer(enforcer), service.WithProjectUseCase(pUC), - service.WithGroupUseCase(gUC), } } diff --git a/app/controlplane/cmd/wire_gen.go b/app/controlplane/cmd/wire_gen.go index e78d95a63..b44147f73 100644 --- a/app/controlplane/cmd/wire_gen.go +++ b/app/controlplane/cmd/wire_gen.go @@ -340,7 +340,7 @@ func newPolicyProviderConfig(in []*conf.PolicyProvider) []*policies.NewRegistryC } func serviceOpts(l log.Logger, enforcer *authz.Enforcer, pUC *biz.ProjectUseCase, gUC *biz.GroupUseCase) []service.NewOpt { - return []service.NewOpt{service.WithLogger(l), service.WithEnforcer(enforcer), service.WithProjectUseCase(pUC), service.WithGroupUseCase(gUC)} + return []service.NewOpt{service.WithLogger(l), service.WithEnforcer(enforcer), service.WithProjectUseCase(pUC)} } func newCASServerOptions(in *conf.Bootstrap_CASServer) *biz.CASServerDefaultOpts { diff --git a/app/controlplane/internal/service/group.go b/app/controlplane/internal/service/group.go index 3294ad7ef..d5d170f00 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" @@ -531,6 +534,74 @@ 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) +} + +// 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..23b2a9cab 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -135,7 +135,6 @@ type service struct { log *log.Helper enforcer *authz.Enforcer projectUseCase *biz.ProjectUseCase - groupUseCase *biz.GroupUseCase } type NewOpt func(s *service) @@ -158,12 +157,6 @@ func WithProjectUseCase(projectUseCase *biz.ProjectUseCase) NewOpt { } } -func WithGroupUseCase(groupUseCase *biz.GroupUseCase) NewOpt { - return func(s *service) { - s.groupUseCase = groupUseCase - } -} - // authorizeResource is a helper that checks if the user has a particular `op` permission policy on a particular resource // For example: `s.authorizeResource(ctx, authz.PolicyAttachedIntegrationDetach, authz.ResourceTypeProject, projectUUID);` // checks if the user has a role in the project that allows to detach integrations on it. @@ -191,11 +184,15 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // 2 - We are a user // find the resource membership that matches the resource type and ID // for example admin in project1, then apply RBAC enforcement + orgRole := usercontext.CurrentAuthzSubject(ctx) m := entities.CurrentMembership(ctx) var matchingResources []*entities.ResourceMembership // First, collect all memberships that match the requested resource type and ID for _, rm := range m.Resources { - if rm.ResourceType == resourceType && rm.ResourceID == resourceID { + if rm.ResourceType == resourceType && rm.ResourceID == resourceID && + // Org Viewers cannot become Project Admins. Removing the role from the list in case it's inherited from a group + // nolint:staticcheck + !(orgRole == string(authz.RoleViewer) && rm.Role == authz.RoleProjectAdmin) { matchingResources = append(matchingResources, rm) } } @@ -253,74 +250,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/biz/project.go b/app/controlplane/pkg/biz/project.go index d4b86b231..afa594100 100644 --- a/app/controlplane/pkg/biz/project.go +++ b/app/controlplane/pkg/biz/project.go @@ -318,6 +318,11 @@ func (uc *ProjectUseCase) addUserToProject(ctx context.Context, orgID uuid.UUID, return nil, fmt.Errorf("failed to find user by email: %w", err) } + // 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") + } + // If the user is not in the organization, handle invitation flow if userMembership == nil { return uc.handleNonExistingUser(ctx, orgID, projectID, opts) From 3f5217d3f30736e1cc52c95842234f0fcf0b503a Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 14:57:00 +0200 Subject: [PATCH 02/12] fix condition and simplify method Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/service.go | 35 ++++++++------------ app/controlplane/pkg/biz/project.go | 2 +- 2 files changed, 14 insertions(+), 23 deletions(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index 23b2a9cab..9ea91d90c 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -186,37 +186,28 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // for example admin in project1, then apply RBAC enforcement orgRole := usercontext.CurrentAuthzSubject(ctx) m := entities.CurrentMembership(ctx) - var matchingResources []*entities.ResourceMembership - // First, collect all memberships that match the requested resource type and ID + + // iterate through all resource memberships and find any that matches for _, rm := range m.Resources { if rm.ResourceType == resourceType && rm.ResourceID == resourceID && - // Org Viewers cannot become Project Admins. Removing the role from the list in case it's inherited from a group + // Org Viewers cannot become Project Admins. Skipping this item in case it's inherited from a group // nolint:staticcheck !(orgRole == string(authz.RoleViewer) && rm.Role == authz.RoleProjectAdmin) { - matchingResources = append(matchingResources, rm) - } - } - var defaultMessage = fmt.Sprintf("you do not have permissions to access to the %s associated with this resource", resourceType) - // If no matching resources were found, return forbidden error - if len(matchingResources) == 0 { - return errors.Forbidden("forbidden", defaultMessage) - } + pass, err := s.enforcer.Enforce(string(rm.Role), op) + if err != nil { + return handleUseCaseErr(err, s.log) + } - // Try to enforce the policy with each matching role - // If any role passes, authorize the request - for _, rm := range matchingResources { - pass, err := s.enforcer.Enforce(string(rm.Role), op) - if err != nil { - return handleUseCaseErr(err, s.log) - } - - if pass { - s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) - return nil + if pass { + s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) + return nil + } } } + var defaultMessage = fmt.Sprintf("you do not have permissions to access to the %s associated with this resource", resourceType) + // If none of the roles pass, return forbidden error return errors.Forbidden("forbidden", defaultMessage) } diff --git a/app/controlplane/pkg/biz/project.go b/app/controlplane/pkg/biz/project.go index afa594100..3d304a6a4 100644 --- a/app/controlplane/pkg/biz/project.go +++ b/app/controlplane/pkg/biz/project.go @@ -319,7 +319,7 @@ func (uc *ProjectUseCase) addUserToProject(ctx context.Context, orgID uuid.UUID, } // Org viewers cannot be added as project admin, since they cannot perform updates on resources - if opts.Role == authz.RoleProjectAdmin && userMembership.Role != authz.RoleViewer { + if opts.Role == authz.RoleProjectAdmin && userMembership.Role == authz.RoleViewer { return nil, NewErrValidationStr("users with org role Org Viewer cannot be Project Admins") } From 94c8e4cdeff84e2e73257e44c8bc8b01a5c2d87b Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 15:31:48 +0200 Subject: [PATCH 03/12] remove newline Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/service.go | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index 9ea91d90c..adc1142e7 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -193,7 +193,6 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // Org Viewers cannot become Project Admins. Skipping this item in case it's inherited from a group // nolint:staticcheck !(orgRole == string(authz.RoleViewer) && rm.Role == authz.RoleProjectAdmin) { - pass, err := s.enforcer.Enforce(string(rm.Role), op) if err != nil { return handleUseCaseErr(err, s.log) From 49a8b55ff68f50ac4f5556fdc8eafc84be3e03c6 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 15:48:29 +0200 Subject: [PATCH 04/12] remove incompatible combinations Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/service.go | 7 +------ app/controlplane/pkg/biz/membership.go | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index adc1142e7..3ea70f019 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -184,16 +184,11 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // 2 - We are a user // find the resource membership that matches the resource type and ID // for example admin in project1, then apply RBAC enforcement - orgRole := usercontext.CurrentAuthzSubject(ctx) m := entities.CurrentMembership(ctx) // iterate through all resource memberships and find any that matches for _, rm := range m.Resources { - if rm.ResourceType == resourceType && rm.ResourceID == resourceID && - // Org Viewers cannot become Project Admins. Skipping this item in case it's inherited from a group - // nolint:staticcheck - !(orgRole == string(authz.RoleViewer) && rm.Role == authz.RoleProjectAdmin) { - pass, err := s.enforcer.Enforce(string(rm.Role), op) + pass, err := s.enforcer.Enforce(string(rm.Role), op) { if err != nil { return handleUseCaseErr(err, s.log) } diff --git a/app/controlplane/pkg/biz/membership.go b/app/controlplane/pkg/biz/membership.go index 539a2103c..86994278d 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 { + if um.ResourceType == authz.ResourceTypeOrganization && um.Role == authz.RoleViewer { + for _, gm := range groupMemberships { + if gm.Role == authz.RoleProjectAdmin { + continue + } + combined = append(combined, gm) + } + } + } + + return combined, nil } // SetProjectOwner sets the project owner (admin role). It skips the operation if an owner exists already From a0f80fd48f4fa9ffb45675b6d61cad1725761dbf Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 15:49:24 +0200 Subject: [PATCH 05/12] fix Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index 3ea70f019..0b2116590 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -188,7 +188,7 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // iterate through all resource memberships and find any that matches for _, rm := range m.Resources { - pass, err := s.enforcer.Enforce(string(rm.Role), op) { + pass, err := s.enforcer.Enforce(string(rm.Role), op) if err != nil { return handleUseCaseErr(err, s.log) } From 92474e7e23c7d294a421b3697a57771d6b109746 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 15:49:54 +0200 Subject: [PATCH 06/12] fix Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/service.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index 0b2116590..a0c2b6232 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -188,15 +188,14 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // iterate through all resource memberships and find any that matches for _, rm := range m.Resources { - pass, err := s.enforcer.Enforce(string(rm.Role), op) - if err != nil { - return handleUseCaseErr(err, s.log) - } - - if pass { - s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) - return nil - } + pass, err := s.enforcer.Enforce(string(rm.Role), op) + if err != nil { + return handleUseCaseErr(err, s.log) + } + + if pass { + s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) + return nil } } From e446f013eda5c8c6e04ad25cbca5df2ef11c6efb Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 15:51:16 +0200 Subject: [PATCH 07/12] fix condition Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/service.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index a0c2b6232..232818091 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -188,14 +188,16 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // iterate through all resource memberships and find any that matches for _, rm := range m.Resources { - pass, err := s.enforcer.Enforce(string(rm.Role), op) - if err != nil { - return handleUseCaseErr(err, s.log) - } - - if pass { - s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) - return nil + if rm.ResourceType == resourceType && rm.ResourceID == resourceID { + pass, err := s.enforcer.Enforce(string(rm.Role), op) + if err != nil { + return handleUseCaseErr(err, s.log) + } + + if pass { + s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) + return nil + } } } From bd773550a0e7993859d4da1953903756b618c813 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 16:34:17 +0200 Subject: [PATCH 08/12] restore unrelated changes Signed-off-by: Jose I. Paris --- app/controlplane/cmd/wire.go | 1 + app/controlplane/cmd/wire_gen.go | 2 +- app/controlplane/internal/service/group.go | 68 ------------------ app/controlplane/internal/service/service.go | 75 ++++++++++++++++++++ 4 files changed, 77 insertions(+), 69 deletions(-) diff --git a/app/controlplane/cmd/wire.go b/app/controlplane/cmd/wire.go index 1e588dfff..9c03ecb53 100644 --- a/app/controlplane/cmd/wire.go +++ b/app/controlplane/cmd/wire.go @@ -100,6 +100,7 @@ func serviceOpts(l log.Logger, enforcer *authz.Enforcer, pUC *biz.ProjectUseCase service.WithLogger(l), service.WithEnforcer(enforcer), service.WithProjectUseCase(pUC), + service.WithGroupUseCase(gUC), } } diff --git a/app/controlplane/cmd/wire_gen.go b/app/controlplane/cmd/wire_gen.go index b44147f73..e78d95a63 100644 --- a/app/controlplane/cmd/wire_gen.go +++ b/app/controlplane/cmd/wire_gen.go @@ -340,7 +340,7 @@ func newPolicyProviderConfig(in []*conf.PolicyProvider) []*policies.NewRegistryC } func serviceOpts(l log.Logger, enforcer *authz.Enforcer, pUC *biz.ProjectUseCase, gUC *biz.GroupUseCase) []service.NewOpt { - return []service.NewOpt{service.WithLogger(l), service.WithEnforcer(enforcer), service.WithProjectUseCase(pUC)} + return []service.NewOpt{service.WithLogger(l), service.WithEnforcer(enforcer), service.WithProjectUseCase(pUC), service.WithGroupUseCase(gUC)} } func newCASServerOptions(in *conf.Bootstrap_CASServer) *biz.CASServerDefaultOpts { diff --git a/app/controlplane/internal/service/group.go b/app/controlplane/internal/service/group.go index d5d170f00..7a33c439f 100644 --- a/app/controlplane/internal/service/group.go +++ b/app/controlplane/internal/service/group.go @@ -534,74 +534,6 @@ 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) -} - -// 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 232818091..79f874cd1 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -135,6 +135,7 @@ type service struct { log *log.Helper enforcer *authz.Enforcer projectUseCase *biz.ProjectUseCase + groupUseCase *biz.GroupUseCase } type NewOpt func(s *service) @@ -157,6 +158,12 @@ func WithProjectUseCase(projectUseCase *biz.ProjectUseCase) NewOpt { } } +func WithGroupUseCase(groupUseCase *biz.GroupUseCase) NewOpt { + return func(s *service) { + s.groupUseCase = groupUseCase + } +} + // authorizeResource is a helper that checks if the user has a particular `op` permission policy on a particular resource // For example: `s.authorizeResource(ctx, authz.PolicyAttachedIntegrationDetach, authz.ResourceTypeProject, projectUUID);` // checks if the user has a role in the project that allows to detach integrations on it. @@ -236,6 +243,74 @@ 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) { From 1043654ca29595db8ef35747f7c9d7e7408a9b24 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 16:34:50 +0200 Subject: [PATCH 09/12] remove imports Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/group.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/controlplane/internal/service/group.go b/app/controlplane/internal/service/group.go index 7a33c439f..3294ad7ef 100644 --- a/app/controlplane/internal/service/group.go +++ b/app/controlplane/internal/service/group.go @@ -20,9 +20,6 @@ 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" From 005fe7aa1d20cfbc0c0996165bf1d645cee7db15 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 16:37:19 +0200 Subject: [PATCH 10/12] restore unrelated changes Signed-off-by: Jose I. Paris --- app/controlplane/internal/service/service.go | 31 +++++++++++++------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index 79f874cd1..4c0000249 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -193,22 +193,33 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // for example admin in project1, then apply RBAC enforcement m := entities.CurrentMembership(ctx) - // iterate through all resource memberships and find any that matches + var matchingResources []*entities.ResourceMembership + // First, collect all memberships that match the requested resource type and ID for _, rm := range m.Resources { if rm.ResourceType == resourceType && rm.ResourceID == resourceID { - pass, err := s.enforcer.Enforce(string(rm.Role), op) - if err != nil { - return handleUseCaseErr(err, s.log) - } - - if pass { - s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) - return nil - } + matchingResources = append(matchingResources, rm) } } var defaultMessage = fmt.Sprintf("you do not have permissions to access to the %s associated with this resource", resourceType) + // If no matching resources were found, return forbidden error + if len(matchingResources) == 0 { + return errors.Forbidden("forbidden", defaultMessage) + } + + // Try to enforce the policy with each matching role + // If any role passes, authorize the request + for _, rm := range matchingResources { + pass, err := s.enforcer.Enforce(string(rm.Role), op) + if err != nil { + return handleUseCaseErr(err, s.log) + } + + if pass { + s.log.Debugw("msg", "authorized using user membership", "resource_id", resourceID.String(), "resource_type", resourceType, "role", rm.Role, "membership_id", rm.MembershipID, "user_id", m.UserID) + return nil + } + } // If none of the roles pass, return forbidden error return errors.Forbidden("forbidden", defaultMessage) From f9290b280c5d9901433725bd871d2e6800b3f983 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 14 Jul 2025 16:39:31 +0200 Subject: [PATCH 11/12] add comment Signed-off-by: Jose I. Paris --- app/controlplane/pkg/biz/membership.go | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controlplane/pkg/biz/membership.go b/app/controlplane/pkg/biz/membership.go index 86994278d..1e6f0208f 100644 --- a/app/controlplane/pkg/biz/membership.go +++ b/app/controlplane/pkg/biz/membership.go @@ -346,6 +346,7 @@ func (uc *MembershipUseCase) ListAllMembershipsForUser(ctx context.Context, user for _, um := range userMemberships { if um.ResourceType == authz.ResourceTypeOrganization && um.Role == authz.RoleViewer { for _, gm := range groupMemberships { + // if user is org viewer and project admin through a group, skip it. if gm.Role == authz.RoleProjectAdmin { continue } From e4daa883121b1d798d4004690b965dcb7d88917b Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Tue, 15 Jul 2025 10:24:50 +0200 Subject: [PATCH 12/12] fix tests Signed-off-by: Jose I. Paris --- app/controlplane/pkg/biz/project.go | 10 +++++----- .../pkg/biz/project_integration_test.go | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/controlplane/pkg/biz/project.go b/app/controlplane/pkg/biz/project.go index 3d304a6a4..a1aa48248 100644 --- a/app/controlplane/pkg/biz/project.go +++ b/app/controlplane/pkg/biz/project.go @@ -318,16 +318,16 @@ func (uc *ProjectUseCase) addUserToProject(ctx context.Context, orgID uuid.UUID, return nil, fmt.Errorf("failed to find user by email: %w", err) } - // 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") - } - // If the user is not in the organization, handle invitation flow if userMembership == nil { 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