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 82ab38692b8..2c9ef696d28 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/models" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -104,11 +105,18 @@ func (m *Mock) Evaluate(ctx context.Context, user *models.SignedInUser, evaluato permissions = accesscontrol.GroupScopesByAction(userPermissions) } - attributeMutator := m.scopeResolvers.GetScopeAttributeMutator(user.OrgId) - resolvedEvaluator, err := evaluator.MutateScopes(ctx, attributeMutator) + if evaluator.Evaluate(permissions) { + return true, nil + } + + resolvedEvaluator, err := evaluator.MutateScopes(ctx, m.scopeResolvers.GetScopeAttributeMutator(user.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/ossaccesscontrol.go b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go index 410fc2994e2..7cc485b2b24 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go @@ -94,8 +94,12 @@ func (ac *OSSAccessControlService) Evaluate(ctx context.Context, user *models.Si user.Permissions[user.OrgId] = accesscontrol.GroupScopesByAction(permissions) } - attributeMutator := ac.scopeResolvers.GetScopeAttributeMutator(user.OrgId) - resolvedEvaluator, err := evaluator.MutateScopes(ctx, attributeMutator) + // 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, ac.scopeResolvers.GetScopeAttributeMutator(user.OrgId)) if err != nil { return false, err } diff --git a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol_test.go b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol_test.go index eadffb873cb..de8ae58bf99 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol_test.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol_test.go @@ -44,10 +44,11 @@ func extractRawPermissionsHelper(perms []accesscontrol.Permission) []accesscontr } type evaluatingPermissionsTestCase struct { - desc string - user userTestCase - endpoints []endpointTestCase - evalResult bool + desc string + user userTestCase + endpoints []endpointTestCase + evalResult bool + expectedErr error } type userTestCase struct { @@ -85,7 +86,8 @@ func TestEvaluatingPermissions(t *testing.T) { endpoints: []endpointTestCase{ {evaluator: accesscontrol.EvalPermission(accesscontrol.ActionUsersCreate, accesscontrol.ScopeGlobalUsersAll)}, }, - evalResult: false, + evalResult: false, + expectedErr: accesscontrol.ErrResolverNotFound, }, } for _, tc := range testCases { @@ -109,7 +111,7 @@ func TestEvaluatingPermissions(t *testing.T) { for _, endpoint := range tc.endpoints { result, err := ac.Evaluate(context.Background(), user, endpoint.evaluator) - require.NoError(t, err) + assert.ErrorIs(t, err, tc.expectedErr) assert.Equal(t, tc.evalResult, result) } }) diff --git a/pkg/services/accesscontrol/resolvers.go b/pkg/services/accesscontrol/resolvers.go index ae123ec62d8..c9e90103a10 100644 --- a/pkg/services/accesscontrol/resolvers.go +++ b/pkg/services/accesscontrol/resolvers.go @@ -56,7 +56,7 @@ func (s *ScopeResolvers) GetScopeAttributeMutator(orgID int64) ScopeAttributeMut 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 913b4477dd6..794adb627ae 100644 --- a/pkg/services/accesscontrol/resolvers_test.go +++ b/pkg/services/accesscontrol/resolvers_test.go @@ -139,24 +139,34 @@ func TestResolveAttributeScope(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.NewScopeResolvers() + t.Run(tt.name, func(t *testing.T) { + resolvers := accesscontrol.NewScopeResolvers() - // 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") + }) } }