fix(authz): close DeleteMembership authorization bypass (#3245)

Signed-off-by: Matías Insaurralde <matias@chainloop.dev>

Author: matiasinsaurralde

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": {},

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,47 @@ 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))
+}
+
+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))
+}