From a89235db53c1317efa90da6e64d062db23df924e Mon Sep 17 00:00:00 2001 From: Miguel Martinez Date: Sun, 17 Aug 2025 17:03:36 +0200 Subject: [PATCH 1/4] fix org deletion Signed-off-by: Miguel Martinez --- .../internal/service/organization.go | 17 +++++++++++ app/controlplane/internal/service/service.go | 28 +++++++++++++++++-- app/controlplane/pkg/authz/authz.go | 27 +++++++++++------- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/app/controlplane/internal/service/organization.go b/app/controlplane/internal/service/organization.go index 6db796efb..ba41400ef 100644 --- a/app/controlplane/internal/service/organization.go +++ b/app/controlplane/internal/service/organization.go @@ -103,6 +103,23 @@ func (s *OrganizationService) Delete(ctx context.Context, req *pb.OrganizationSe return nil, err } + // Find the organization to get its UUID for authorization + org, err := s.orgUC.FindByName(ctx, req.Name) + if err != nil { + return nil, handleUseCaseErr(err, s.log) + } + + orgUUID, err := uuid.Parse(org.ID) + if err != nil { + return nil, handleUseCaseErr(biz.NewErrInvalidUUID(err), s.log) + } + + // Check if user has permission to delete this specific organization + // Force RBAC to ensure only owners can delete, even if they have admin privileges elsewhere + if err := s.authorizeResource(ctx, authz.PolicyOrganizationDelete, authz.ResourceTypeOrganization, orgUUID, withForceRBAC()); err != nil { + return nil, err + } + if err := s.orgUC.DeleteByUser(ctx, req.Name, currentUser.ID); err != nil { return nil, handleUseCaseErr(err, s.log) } diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index b422265f4..0df473111 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -164,14 +164,32 @@ func WithGroupUseCase(groupUseCase *biz.GroupUseCase) NewOpt { } } +type authorizeResourceOpts struct { + forceRBAC bool +} + +type AuthorizeResourceOpt func(*authorizeResourceOpts) + +// withForceRBAC forces RBAC checks even for admin roles that would normally skip RBAC +func withForceRBAC() AuthorizeResourceOpt { + return func(opts *authorizeResourceOpts) { + opts.forceRBAC = true + } +} + // 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. // This method is available to every service that embeds `service` // It goes through all the memberships of the user, direct memberships and indirect memberships (Groups) // and checks if the user has any role that allows the operation on the resourceType and resourceID. -func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resourceType authz.ResourceType, resourceID uuid.UUID) error { - if !rbacEnabled(ctx) { +func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resourceType authz.ResourceType, resourceID uuid.UUID, opts ...AuthorizeResourceOpt) error { + options := &authorizeResourceOpts{} + for _, opt := range opts { + opt(options) + } + + if !options.forceRBAC && !rbacEnabled(ctx) { return nil } @@ -188,22 +206,26 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou return errors.Forbidden("forbidden", fmt.Errorf("operation not allowed: This auth token is valid only with the project %q", *token.ProjectName).Error()) } + var defaultMessage = fmt.Sprintf("you do not have permissions to access the %q with id %q", resourceType, resourceID.String()) // 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 m := entities.CurrentMembership(ctx) var matchingResources []*entities.ResourceMembership + var foundRoles []string // First, collect all memberships that match the requested resource type and ID for _, rm := range m.Resources { if rm.ResourceType == resourceType && rm.ResourceID == resourceID { matchingResources = append(matchingResources, rm) + foundRoles = append(foundRoles, string(rm.Role)) } } - 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) + } else { + defaultMessage = fmt.Sprintf("%s, roles=%v", defaultMessage, foundRoles) } // Try to enforce the policy with each matching role diff --git a/app/controlplane/pkg/authz/authz.go b/app/controlplane/pkg/authz/authz.go index f9f2dd29e..89fd0789e 100644 --- a/app/controlplane/pkg/authz/authz.go +++ b/app/controlplane/pkg/authz/authz.go @@ -167,7 +167,9 @@ var ( // Projects PolicyProjectCreate = &Policy{ResourceProject, ActionCreate} + // Organization PolicyOrganizationCreate = &Policy{Organization, ActionCreate} + PolicyOrganizationDelete = &Policy{Organization, ActionDelete} // User Membership PolicyOrganizationRead = &Policy{Organization, ActionRead} PolicyOrganizationListMemberships = &Policy{OrganizationMemberships, ActionList} @@ -200,6 +202,18 @@ var RolesMap = map[Role][]*Policy{ RoleInstanceAdmin: { PolicyOrganizationCreate, }, + RoleOwner: { + PolicyOrganizationDelete, + }, + // RoleAdmin is an org-scoped role that provides super admin privileges (it's the higher role) + RoleAdmin: { + // We do a manual check in the artifact upload endpoint + // so we need the actual policy in place skipping it is not enough + PolicyArtifactUpload, + // We manually check this policy to be able to know if the user can invite users to the system + PolicyOrganizationInvitationsCreate, + // + all the policies from the viewer role inherited automatically + }, // RoleViewer is an org-scoped role that provides read-only access to all resources RoleViewer: { // Referrer @@ -234,16 +248,6 @@ var RolesMap = map[Role][]*Policy{ // List organization memberships PolicyOrganizationListMemberships, }, - // RoleAdmin is an org-scoped role that provides super admin privileges (it's the higher role) - RoleAdmin: { - // We do a manual check in the artifact upload endpoint - // so we need the actual policy in place skipping it is not enough - PolicyArtifactUpload, - // We manually check this policy to be able to know if the user can invite users to the system - PolicyOrganizationInvitationsCreate, - // + all the policies from the viewer role inherited automatically - }, - // RoleOrgMember is an org-scoped role that enables RBAC in the underlying resources. Users with this role at // the organization level will need specific project roles to access their contents RoleOrgContributor: { @@ -403,6 +407,9 @@ var ServerOperationsMap = map[string][]*Policy{ // since all the permissions here are in the context of an organization // Create new organization "/controlplane.v1.OrganizationService/Create": {}, + // Delete an organization makes checks at the service level since the + // user can explicitly set the org they want to delete and might not be the current one + "/controlplane.v1.OrganizationService/Delete": {}, // List global memberships "/controlplane.v1.OrganizationService/ListMemberships": {PolicyOrganizationListMemberships}, From e96c3622f55e06f8c9dd7f66b4dc2564cdabbda6 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Date: Sun, 17 Aug 2025 17:13:50 +0200 Subject: [PATCH 2/4] remove duplicates Signed-off-by: Miguel Martinez --- .../internal/service/organization.go | 5 +-- app/controlplane/pkg/biz/organization.go | 37 ------------------- 2 files changed, 2 insertions(+), 40 deletions(-) diff --git a/app/controlplane/internal/service/organization.go b/app/controlplane/internal/service/organization.go index ba41400ef..f198696de 100644 --- a/app/controlplane/internal/service/organization.go +++ b/app/controlplane/internal/service/organization.go @@ -98,8 +98,7 @@ func (s *OrganizationService) Update(ctx context.Context, req *pb.OrganizationSe } func (s *OrganizationService) Delete(ctx context.Context, req *pb.OrganizationServiceDeleteRequest) (*pb.OrganizationServiceDeleteResponse, error) { - currentUser, err := requireCurrentUser(ctx) - if err != nil { + if _, err := requireCurrentUser(ctx); err != nil { return nil, err } @@ -120,7 +119,7 @@ func (s *OrganizationService) Delete(ctx context.Context, req *pb.OrganizationSe return nil, err } - if err := s.orgUC.DeleteByUser(ctx, req.Name, currentUser.ID); err != nil { + if err := s.orgUC.Delete(ctx, orgUUID.String()); err != nil { return nil, handleUseCaseErr(err, s.log) } diff --git a/app/controlplane/pkg/biz/organization.go b/app/controlplane/pkg/biz/organization.go index 02ef20707..9890cfefd 100644 --- a/app/controlplane/pkg/biz/organization.go +++ b/app/controlplane/pkg/biz/organization.go @@ -290,43 +290,6 @@ func (uc *OrganizationUseCase) Delete(ctx context.Context, id string) error { return uc.orgRepo.Delete(ctx, orgUUID) } -// DeleteByUser deletes an organization initiated by a user with owner validation -// Only organization owners can delete an organization -func (uc *OrganizationUseCase) DeleteByUser(ctx context.Context, orgName, userID string) error { - // Find organization by name - org, err := uc.orgRepo.FindByName(ctx, orgName) - if err != nil { - return err - } else if org == nil { - return NewErrNotFound("organization") - } - - orgUUID, err := uuid.Parse(org.ID) - if err != nil { - return NewErrInvalidUUID(err) - } - - userUUID, err := uuid.Parse(userID) - if err != nil { - return NewErrInvalidUUID(err) - } - - // Check if user is an owner of the organization - m, err := uc.membershipRepo.FindByOrgAndUser(ctx, orgUUID, userUUID) - if err != nil { - return fmt.Errorf("failed to find owners: %w", err) - } - - if m == nil || m.Role != authz.RoleOwner { - return NewErrValidationStr("only organization owners can delete the organization") - } - - uc.logger.Infow("msg", "User deleting organization", "user_id", userID, "organization_id", org.ID) - - // Use the existing Delete method to handle the actual deletion - return uc.Delete(ctx, org.ID) -} - // AutoOnboardOrganizations creates the organizations specified in the onboarding config and assigns the user to them // with the specified role if they are not already a member. func (uc *OrganizationUseCase) AutoOnboardOrganizations(ctx context.Context, userID string) error { From 6436a153cd289ecf3453929adc1e3744fce01762 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Date: Sun, 17 Aug 2025 17:16:21 +0200 Subject: [PATCH 3/4] fix message Signed-off-by: Miguel Martinez --- app/controlplane/internal/service/organization.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controlplane/internal/service/organization.go b/app/controlplane/internal/service/organization.go index f198696de..72f24f3ae 100644 --- a/app/controlplane/internal/service/organization.go +++ b/app/controlplane/internal/service/organization.go @@ -116,7 +116,7 @@ func (s *OrganizationService) Delete(ctx context.Context, req *pb.OrganizationSe // Check if user has permission to delete this specific organization // Force RBAC to ensure only owners can delete, even if they have admin privileges elsewhere if err := s.authorizeResource(ctx, authz.PolicyOrganizationDelete, authz.ResourceTypeOrganization, orgUUID, withForceRBAC()); err != nil { - return nil, err + return nil, errors.Forbidden("forbidden", "only organization owners can delete the organization") } if err := s.orgUC.Delete(ctx, orgUUID.String()); err != nil { From fbc454bb0a0084b7788273a32c9c25b4fae78d84 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Date: Sun, 17 Aug 2025 17:16:52 +0200 Subject: [PATCH 4/4] fix message Signed-off-by: Miguel Martinez --- app/controlplane/internal/service/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controlplane/internal/service/service.go b/app/controlplane/internal/service/service.go index 0df473111..bf5f31dab 100644 --- a/app/controlplane/internal/service/service.go +++ b/app/controlplane/internal/service/service.go @@ -224,10 +224,10 @@ func (s *service) authorizeResource(ctx context.Context, op *authz.Policy, resou // If no matching resources were found, return forbidden error if len(matchingResources) == 0 { return errors.Forbidden("forbidden", defaultMessage) - } else { - defaultMessage = fmt.Sprintf("%s, roles=%v", defaultMessage, foundRoles) } + defaultMessage = fmt.Sprintf("%s, roles=%v", defaultMessage, foundRoles) + // Try to enforce the policy with each matching role // If any role passes, authorize the request for _, rm := range matchingResources {