From 552d3fec8dc1f57c1738240cb54ebd7416780c9b Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Thu, 25 Aug 2022 12:50:27 +0200 Subject: [PATCH] RBAC: Fix resolver issue on wildcard resulting in wrong status code for endpoints (#54208) * RBAC: Test evaluation before attaching mutator * RBAC: Return error if no resolver is found for scope * RBAC: Sync changes to evaluation in mock * RBAC: Check for resolver not found error and just fail the evaluation in that case --- pkg/services/accesscontrol/errors.go | 1 + pkg/services/accesscontrol/mock/mock.go | 12 +++++- .../ossaccesscontrol/accesscontrol.go | 10 +++++ pkg/services/accesscontrol/resolvers.go | 2 +- pkg/services/accesscontrol/resolvers_test.go | 40 ++++++++++++------- 5 files changed, 47 insertions(+), 18 deletions(-) diff --git a/pkg/services/accesscontrol/errors.go b/pkg/services/accesscontrol/errors.go index 78ef9498ab0..806422827f5 100644 --- a/pkg/services/accesscontrol/errors.go +++ b/pkg/services/accesscontrol/errors.go @@ -6,4 +6,5 @@ var ( ErrFixedRolePrefixMissing = errors.New("fixed role should be prefixed with '" + FixedRolePrefix + "'") ErrInvalidBuiltinRole = errors.New("built-in role is not valid") ErrInvalidScope = errors.New("invalid scope") + ErrResolverNotFound = errors.New("no resolver found") ) diff --git a/pkg/services/accesscontrol/mock/mock.go b/pkg/services/accesscontrol/mock/mock.go index e9191198867..a21a7b1bbbe 100644 --- a/pkg/services/accesscontrol/mock/mock.go +++ b/pkg/services/accesscontrol/mock/mock.go @@ -2,6 +2,7 @@ package mock import ( "context" + "errors" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -105,11 +106,18 @@ func (m *Mock) Evaluate(ctx context.Context, usr *user.SignedInUser, evaluator a permissions = accesscontrol.GroupScopesByAction(userPermissions) } - attributeMutator := m.scopeResolvers.GetScopeAttributeMutator(usr.OrgID) - resolvedEvaluator, err := evaluator.MutateScopes(ctx, attributeMutator) + if evaluator.Evaluate(permissions) { + return true, nil + } + + resolvedEvaluator, err := evaluator.MutateScopes(ctx, m.scopeResolvers.GetScopeAttributeMutator(usr.OrgID)) if err != nil { + if errors.Is(err, accesscontrol.ErrResolverNotFound) { + return false, nil + } return false, err } + return resolvedEvaluator.Evaluate(permissions), nil } diff --git a/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go b/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go index 929223965f1..0c3279962e5 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go @@ -2,6 +2,7 @@ package ossaccesscontrol import ( "context" + "errors" "github.com/prometheus/client_golang/prometheus" @@ -45,10 +46,19 @@ func (a *AccessControl) Evaluate(ctx context.Context, user *user.SignedInUser, e user.Permissions[user.OrgID] = accesscontrol.GroupScopesByAction(permissions) } + // Test evaluation without scope resolver first, this will prevent 403 for wildcard scopes when resource does not exist + if evaluator.Evaluate(user.Permissions[user.OrgID]) { + return true, nil + } + resolvedEvaluator, err := evaluator.MutateScopes(ctx, a.resolvers.GetScopeAttributeMutator(user.OrgID)) if err != nil { + if errors.Is(err, accesscontrol.ErrResolverNotFound) { + return false, nil + } return false, err } + return resolvedEvaluator.Evaluate(user.Permissions[user.OrgID]), nil } diff --git a/pkg/services/accesscontrol/resolvers.go b/pkg/services/accesscontrol/resolvers.go index 6fa933dad21..c59edffd098 100644 --- a/pkg/services/accesscontrol/resolvers.go +++ b/pkg/services/accesscontrol/resolvers.go @@ -69,7 +69,7 @@ func (s *Resolvers) GetScopeAttributeMutator(orgID int64) ScopeAttributeMutator s.log.Debug("resolved scope", "scope", scope, "resolved_scopes", scopes) return scopes, nil } - return []string{scope}, nil + return nil, ErrResolverNotFound } } diff --git a/pkg/services/accesscontrol/resolvers_test.go b/pkg/services/accesscontrol/resolvers_test.go index f7d4f8e8b7d..0b0b782cd90 100644 --- a/pkg/services/accesscontrol/resolvers_test.go +++ b/pkg/services/accesscontrol/resolvers_test.go @@ -89,24 +89,34 @@ func TestResolvers_AttributeScope(t *testing.T) { wantEvaluator: accesscontrol.EvalPermission("datasources:read", accesscontrol.Scope("datasources", "id", "5")), wantCalls: 1, }, + { + name: "should return error if no resolver is found for scope", + orgID: 1, + evaluator: accesscontrol.EvalPermission("dashboards:read", "dashboards:id:1"), + wantEvaluator: nil, + wantCalls: 0, + wantErr: accesscontrol.ErrResolverNotFound, + }, } for _, tt := range tests { - resolvers := accesscontrol.NewResolvers(log.NewNopLogger()) + t.Run(tt.name, func(t *testing.T) { + resolvers := accesscontrol.NewResolvers(log.NewNopLogger()) - // Reset calls counter - calls = 0 - // Register a resolution method - resolvers.AddScopeAttributeResolver("datasources:name:", fakeDataSourceResolver) + // Reset calls counter + calls = 0 + // Register a resolution method + resolvers.AddScopeAttributeResolver("datasources:name:", fakeDataSourceResolver) - // Test - mutate := resolvers.GetScopeAttributeMutator(tt.orgID) - resolvedEvaluator, err := tt.evaluator.MutateScopes(context.Background(), mutate) - if tt.wantErr != nil { - assert.ErrorAs(t, err, &tt.wantErr, "expected an error during the resolution of the scope") - return - } - assert.NoError(t, err) - assert.EqualValues(t, tt.wantEvaluator, resolvedEvaluator, "permission did not match expected resolution") - assert.Equal(t, tt.wantCalls, calls, "cache has not been used") + // Test + mutate := resolvers.GetScopeAttributeMutator(tt.orgID) + resolvedEvaluator, err := tt.evaluator.MutateScopes(context.Background(), mutate) + if tt.wantErr != nil { + assert.ErrorAs(t, err, &tt.wantErr, "expected an error during the resolution of the scope") + return + } + assert.NoError(t, err) + assert.EqualValues(t, tt.wantEvaluator, resolvedEvaluator, "permission did not match expected resolution") + assert.Equal(t, tt.wantCalls, calls, "cache has not been used") + }) } }