[v9.1.x] RBAC: Fix resolver issue on wildcard resulting in wrong status code (#54692)

* 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

(cherry picked from commit 552d3fec8d)

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")
 )

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
 }
 

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
 	}

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)
 			}
 		})

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
 	}
 }
 

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")
+		})
 	}
 }