From 748d625c7b58583f6e578eb875245eec098e0aa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20Insaurralde?= Date: Thu, 25 Jun 2026 23:38:18 -0300 Subject: [PATCH 1/2] fix(authz): enforce admin policy on org membership delete/update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add explicit ServerOperationsMap entries so DeleteMembership no longer inherits the empty OrganizationService/Delete policy via regex prefix match. Signed-off-by: Matías Insaurralde --- app/controlplane/pkg/authz/authz.go | 14 ++++++-- .../pkg/authz/middleware/middleware_test.go | 36 +++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/app/controlplane/pkg/authz/authz.go b/app/controlplane/pkg/authz/authz.go index f16e55f9b..f7bfdf442 100644 --- a/app/controlplane/pkg/authz/authz.go +++ b/app/controlplane/pkg/authz/authz.go @@ -160,8 +160,10 @@ var ( PolicyOrganizationCreate = &Policy{Organization, ActionCreate} PolicyOrganizationDelete = &Policy{Organization, ActionDelete} // User Membership - PolicyOrganizationRead = &Policy{Organization, ActionRead} - PolicyOrganizationListMemberships = &Policy{OrganizationMemberships, ActionList} + PolicyOrganizationRead = &Policy{Organization, ActionRead} + PolicyOrganizationListMemberships = &Policy{OrganizationMemberships, ActionList} + PolicyOrganizationMembershipsDelete = &Policy{OrganizationMemberships, ActionDelete} + PolicyOrganizationMembershipsUpdate = &Policy{OrganizationMemberships, ActionUpdate} // Group Memberships PolicyGroupListPendingInvitations = &Policy{ResourceGroup, ActionList} @@ -203,6 +205,8 @@ var RolesMap = map[Role][]*Policy{ RoleOwner: { PolicyOrganizationDelete, PolicyOrganizationManageOwners, + PolicyOrganizationMembershipsDelete, + PolicyOrganizationMembershipsUpdate, }, // RoleAdmin is an org-scoped role that provides super admin privileges (it's the higher role) RoleAdmin: { @@ -213,6 +217,8 @@ var RolesMap = map[Role][]*Policy{ PolicyOrganizationInvitationsCreate, // Being able to read from the default backend PolicyDefaultBackendArtifactRead, + PolicyOrganizationMembershipsDelete, + PolicyOrganizationMembershipsUpdate, // + all the policies from the viewer role inherited automatically }, // RoleViewer is an org-scoped role that provides read-only access to all resources @@ -417,7 +423,9 @@ var ServerOperationsMap = map[string]*OperationPolicy{ "/controlplane.v1.OrganizationService/Delete": {}, // List global memberships - "/controlplane.v1.OrganizationService/ListMemberships": {Policies: []*Policy{PolicyOrganizationListMemberships}}, + "/controlplane.v1.OrganizationService/ListMemberships": {Policies: []*Policy{PolicyOrganizationListMemberships}}, + "/controlplane.v1.OrganizationService/DeleteMembership": {Policies: []*Policy{PolicyOrganizationMembershipsDelete}}, + "/controlplane.v1.OrganizationService/UpdateMembership": {Policies: []*Policy{PolicyOrganizationMembershipsUpdate}}, // NOTE: this is about listing my own memberships, not about listing all the memberships in the organization "/controlplane.v1.UserService/ListMemberships": {}, diff --git a/app/controlplane/pkg/authz/middleware/middleware_test.go b/app/controlplane/pkg/authz/middleware/middleware_test.go index 673530033..8e9432d29 100644 --- a/app/controlplane/pkg/authz/middleware/middleware_test.go +++ b/app/controlplane/pkg/authz/middleware/middleware_test.go @@ -214,6 +214,14 @@ func TestPoliciesLookup(t *testing.T) { name: "contract apply operation found", operation: "/controlplane.v1.WorkflowContractService/Apply", }, + { + name: "organization delete membership operation found", + operation: "/controlplane.v1.OrganizationService/DeleteMembership", + }, + { + name: "organization update membership operation found", + operation: "/controlplane.v1.OrganizationService/UpdateMembership", + }, } for _, tc := range testCases { @@ -234,3 +242,31 @@ func TestPoliciesLookupContractApply(t *testing.T) { assert.NoError(t, err) assert.Equal(t, []*authz.Policy{authz.PolicyWorkflowContractCreate, authz.PolicyWorkflowContractUpdate}, policies) } + +func TestPoliciesLookupDeleteMembership(t *testing.T) { + policies, err := policiesLookup("/controlplane.v1.OrganizationService/DeleteMembership") + assert.NoError(t, err) + assert.Equal(t, []*authz.Policy{authz.PolicyOrganizationMembershipsDelete}, policies) +} + +func TestPoliciesLookupUpdateMembership(t *testing.T) { + policies, err := policiesLookup("/controlplane.v1.OrganizationService/UpdateMembership") + assert.NoError(t, err) + assert.Equal(t, []*authz.Policy{authz.PolicyOrganizationMembershipsUpdate}, policies) +} + +func TestViewerDeniedDeleteMembership(t *testing.T) { + logger := log.NewHelper(log.NewStdLogger(io.Discard)) + + ctx := context.Background() + ctx = usercontext.WithAuthzSubject(ctx, string(authz.RoleViewer)) + ctx = transport.NewServerContext(ctx, &mockTransport{operation: "/controlplane.v1.OrganizationService/DeleteMembership"}) + + e := NewMockEnforcer(t) + e.On("Enforce", mock.Anything, string(authz.RoleViewer), authz.PolicyOrganizationMembershipsDelete).Return(false, nil) + + m := WithAuthzMiddleware(e, logger) + _, err := m(emptyHandler)(ctx, nil) + assert.Error(t, err) + assert.True(t, errors.IsForbidden(err)) +} From 9b071806255df354b4048d64dc9aa40b82b7fff8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20Insaurralde?= Date: Fri, 26 Jun 2026 09:42:41 -0300 Subject: [PATCH 2/2] test(authz): add viewer-denied middleware test for UpdateMembership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirror TestViewerDeniedDeleteMembership to cover the UpdateMembership deny path through the authz middleware. Signed-off-by: Matías Insaurralde --- .../pkg/authz/middleware/middleware_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/app/controlplane/pkg/authz/middleware/middleware_test.go b/app/controlplane/pkg/authz/middleware/middleware_test.go index 8e9432d29..fb9755ac3 100644 --- a/app/controlplane/pkg/authz/middleware/middleware_test.go +++ b/app/controlplane/pkg/authz/middleware/middleware_test.go @@ -270,3 +270,19 @@ func TestViewerDeniedDeleteMembership(t *testing.T) { assert.Error(t, err) assert.True(t, errors.IsForbidden(err)) } + +func TestViewerDeniedUpdateMembership(t *testing.T) { + logger := log.NewHelper(log.NewStdLogger(io.Discard)) + + ctx := context.Background() + ctx = usercontext.WithAuthzSubject(ctx, string(authz.RoleViewer)) + ctx = transport.NewServerContext(ctx, &mockTransport{operation: "/controlplane.v1.OrganizationService/UpdateMembership"}) + + e := NewMockEnforcer(t) + e.On("Enforce", mock.Anything, string(authz.RoleViewer), authz.PolicyOrganizationMembershipsUpdate).Return(false, nil) + + m := WithAuthzMiddleware(e, logger) + _, err := m(emptyHandler)(ctx, nil) + assert.Error(t, err) + assert.True(t, errors.IsForbidden(err)) +}