diff --git a/app/controlplane/pkg/authz/authz.go b/app/controlplane/pkg/authz/authz.go index f7bfdf442..97fbb83ed 100644 --- a/app/controlplane/pkg/authz/authz.go +++ b/app/controlplane/pkg/authz/authz.go @@ -160,10 +160,8 @@ var ( PolicyOrganizationCreate = &Policy{Organization, ActionCreate} PolicyOrganizationDelete = &Policy{Organization, ActionDelete} // User Membership - PolicyOrganizationRead = &Policy{Organization, ActionRead} - PolicyOrganizationListMemberships = &Policy{OrganizationMemberships, ActionList} - PolicyOrganizationMembershipsDelete = &Policy{OrganizationMemberships, ActionDelete} - PolicyOrganizationMembershipsUpdate = &Policy{OrganizationMemberships, ActionUpdate} + PolicyOrganizationRead = &Policy{Organization, ActionRead} + PolicyOrganizationListMemberships = &Policy{OrganizationMemberships, ActionList} // Group Memberships PolicyGroupListPendingInvitations = &Policy{ResourceGroup, ActionList} @@ -205,8 +203,6 @@ 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: { @@ -217,8 +213,6 @@ 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 @@ -391,7 +385,9 @@ var ServerOperationsMap = map[string]*OperationPolicy{ "/controlplane.v1.IntegrationsService/Attach": {Policies: []*Policy{PolicyAttachedIntegrationAttach}}, "/controlplane.v1.IntegrationsService/Detach": {Policies: []*Policy{PolicyAttachedIntegrationDetach}}, // Metrics - "/controlplane.v1.OrgMetricsService/.*": {Policies: []*Policy{PolicyOrgMetricsRead}}, + "/controlplane.v1.OrgMetricsService/Totals": {Policies: []*Policy{PolicyOrgMetricsRead}}, + "/controlplane.v1.OrgMetricsService/TopWorkflowsByRunsCount": {Policies: []*Policy{PolicyOrgMetricsRead}}, + "/controlplane.v1.OrgMetricsService/DailyRunsCount": {Policies: []*Policy{PolicyOrgMetricsRead}}, // Robot Account "/controlplane.v1.RobotAccountService/List": {Policies: []*Policy{PolicyRobotAccountList}}, "/controlplane.v1.RobotAccountService/Create": {Policies: []*Policy{PolicyRobotAccountCreate}}, @@ -423,9 +419,7 @@ var ServerOperationsMap = map[string]*OperationPolicy{ "/controlplane.v1.OrganizationService/Delete": {}, // List global memberships - "/controlplane.v1.OrganizationService/ListMemberships": {Policies: []*Policy{PolicyOrganizationListMemberships}}, - "/controlplane.v1.OrganizationService/DeleteMembership": {Policies: []*Policy{PolicyOrganizationMembershipsDelete}}, - "/controlplane.v1.OrganizationService/UpdateMembership": {Policies: []*Policy{PolicyOrganizationMembershipsUpdate}}, + "/controlplane.v1.OrganizationService/ListMemberships": {Policies: []*Policy{PolicyOrganizationListMemberships}}, // 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.go b/app/controlplane/pkg/authz/middleware/middleware.go index b4befe8c5..0ae48a3d0 100644 --- a/app/controlplane/pkg/authz/middleware/middleware.go +++ b/app/controlplane/pkg/authz/middleware/middleware.go @@ -18,7 +18,6 @@ package middleware import ( "context" "errors" - "regexp" errorsAPI "github.com/go-kratos/kratos/v2/errors" @@ -103,25 +102,13 @@ func checkPolicies(ctx context.Context, subject, apiOperation string, enforcer E return nil } -// policiesLookup returns the policies required for a given API operation -// it performs a two run lookup -// 1 - It checks if there is an entry in the map -// 2 - if there is not, it runs a regex match in each key in case one of those keys contains a regex +// policiesLookup returns the policies required for a given API operation. +// It looks up the operation directly in the ServerOperationsMap. func policiesLookup(apiOperation string) ([]*authz.Policy, error) { - // Direct match entry, found := authz.ServerOperationsMap[apiOperation] if found { return entry.Policies, nil } - // second pass trying to match a regex - // i.e "/controlplane.v1.OrgMetricsService/.*" -> "/controlplane.v1.OrgMetricsService/Totals" - for k, entry := range authz.ServerOperationsMap { - found, _ := regexp.MatchString(k, apiOperation) - if found { - return entry.Policies, nil - } - } - return nil, errors.New("operation not allowed") } diff --git a/app/controlplane/pkg/authz/middleware/middleware_test.go b/app/controlplane/pkg/authz/middleware/middleware_test.go index fb9755ac3..3d19cb57a 100644 --- a/app/controlplane/pkg/authz/middleware/middleware_test.go +++ b/app/controlplane/pkg/authz/middleware/middleware_test.go @@ -193,20 +193,25 @@ func TestPoliciesLookup(t *testing.T) { operation: "/controlplane.v1.WorkflowContractService/List", }, { - name: "operation found with regexp", - operation: "/controlplane.v1.OrgMetricsService/List", + name: "org metrics operation found", + operation: "/controlplane.v1.OrgMetricsService/Totals", }, { - name: "operation found with regexp 2", + name: "org metrics operation found 2", + operation: "/controlplane.v1.OrgMetricsService/DailyRunsCount", + }, + { + name: "non-existing org metrics operation, error not found", operation: "/controlplane.v1.OrgMetricsService/boom", + wantErr: true, }, { - name: "operation found with regexp, error wrong prefix", + name: "operation error wrong prefix", operation: "/boom/controlplane.v1.OrgMetricsService", wantErr: true, }, { - name: "operation found with regexp, error not found", + name: "operation error not found", operation: "/controlplane.v1.OrgMetricsService", wantErr: true, }, @@ -214,14 +219,6 @@ 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 { @@ -242,47 +239,3 @@ 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)) -}