Refactor: Remove fallback logic from Check and Compile methods in authzLimitedClient

pkg/storage/unified/resource/access.go

@@ -110,24 +110,9 @@ func (c authzLimitedClient) Check(ctx context.Context, id claims.AuthInfo, req c
 		attribute.String("name", req.Name),
 		attribute.String("verb", req.Verb),
 		attribute.String("folder", folder),
-		attribute.Bool("fallback_used", FallbackUsed(ctx)),
 	))
 	defer span.End()
 
-	if FallbackUsed(ctx) {
-		if req.Namespace == "" {
-			// cross namespace queries are not allowed when fallback is used
-			span.SetAttributes(attribute.Bool("allowed", false))
-			span.SetStatus(codes.Error, "Namespace empty")
-			err := fmt.Errorf("namespace empty")
-			span.RecordError(err)
-			return claims.CheckResponse{Allowed: false}, err
-		}
-
-		span.SetAttributes(attribute.Bool("allowed", true))
-		return claims.CheckResponse{Allowed: true}, nil
-	}
-
 	if !claims.NamespaceMatches(id.GetNamespace(), req.Namespace) {
 		span.SetAttributes(attribute.Bool("allowed", false))
 		span.SetStatus(codes.Error, "Namespace mismatch")
@@ -155,28 +140,14 @@ func (c authzLimitedClient) Check(ctx context.Context, id claims.AuthInfo, req c
 // Compile implements claims.AccessClient.
 func (c authzLimitedClient) Compile(ctx context.Context, id claims.AuthInfo, req claims.ListRequest) (claims.ItemChecker, claims.Zookie, error) {
 	t := time.Now()
-	fallbackUsed := FallbackUsed(ctx)
 	ctx, span := tracer.Start(ctx, "resource.authzLimitedClient.Compile", trace.WithAttributes(
 		attribute.String("group", req.Group),
 		attribute.String("resource", req.Resource),
 		attribute.String("namespace", req.Namespace),
 		attribute.String("verb", req.Verb),
-		attribute.Bool("fallback_used", fallbackUsed),
 	))
 	defer span.End()
-	if fallbackUsed {
+
-		if req.Namespace == "" {
-			// cross namespace queries are not allowed when fallback is used
-			span.SetAttributes(attribute.Bool("allowed", false))
-			span.SetStatus(codes.Error, "Namespace empty")
-			err := fmt.Errorf("namespace empty")
-			span.RecordError(err)
-			return nil, claims.NoopZookie{}, err
-		}
-		return func(name, folder string) bool {
-			return true
-		}, claims.NoopZookie{}, nil
-	}
 	if !claims.NamespaceMatches(id.GetNamespace(), req.Namespace) {
 		span.SetAttributes(attribute.Bool("allowed", false))
 		span.SetStatus(codes.Error, "Namespace mismatch")
@@ -211,13 +182,3 @@ func (c authzLimitedClient) IsCompatibleWithRBAC(group, resource string) bool {
 }
 
 var _ claims.AccessClient = &authzLimitedClient{}
-
-type contextFallbackKey struct{}
-
-func WithFallback(ctx context.Context) context.Context {
-	return context.WithValue(ctx, contextFallbackKey{}, true)
-}
-
-func FallbackUsed(ctx context.Context) bool {
-	return ctx.Value(contextFallbackKey{}) != nil
-}

pkg/storage/unified/resource/access_test.go

@@ -158,67 +158,3 @@ func TestNamespaceMatching(t *testing.T) {
 		})
 	}
 }
-
-// TestNamespaceMatchingFallback tests namespace matching in Check and Compile methods when fallback is used
-func TestNamespaceMatchingFallback(t *testing.T) {
-	// Create a mock client that always returns allowed=true
-	mockClient := authlib.FixedAccessClient(true)
-	client := NewAuthzLimitedClient(mockClient, AuthzOptions{})
-
-	// Create a context with fallback disabled
-	ctx := context.Background()
-
-	tests := []struct {
-		name          string
-		authNamespace string
-		reqNamespace  string
-		expectError   bool
-	}{
-		{
-			name:         "with namespace fallback",
-			reqNamespace: "ns1",
-			expectError:  false,
-		},
-		{
-			name:         "empty request namespace with fallback",
-			reqNamespace: "",
-			expectError:  true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Test Check method with namespace matching
-			checkReq := authlib.CheckRequest{
-				Group:     "unknown.group", // Use unknown group to bypass RBAC check
-				Resource:  "unknown.resource",
-				Verb:      utils.VerbGet,
-				Namespace: tt.reqNamespace,
-			}
-			ctx = WithFallback(ctx)
-			// Create a mock auth info with the specified namespace
-			// Test Check method
-			user := &identity.StaticRequester{Namespace: tt.authNamespace}
-			_, checkErr := client.Check(ctx, user, checkReq, "")
-
-			// Test Compile method
-			compileReq := authlib.ListRequest{
-				Group:     "unknown.group", // Use unknown group to bypass RBAC check
-				Resource:  "unknown.resource",
-				Verb:      utils.VerbGet,
-				Namespace: tt.reqNamespace,
-			}
-			_, _, compileErr := client.Compile(ctx, user, compileReq)
-
-			if tt.expectError {
-				require.Error(t, checkErr, "Check should return error")
-				require.Error(t, compileErr, "Compile should return error")
-				assert.ErrorContains(t, checkErr, "namespace empty", "Check should return namespace mismatch error")
-				assert.ErrorContains(t, compileErr, "namespace empty", "Compile should return namespace mismatch error")
-			} else {
-				assert.NoError(t, checkErr, "Check should not return error when namespaces match")
-				assert.NoError(t, compileErr, "Compile should not return error when namespaces match")
-			}
-		})
-	}
-}

pkg/storage/unified/sql/service.go

@@ -13,9 +13,7 @@ import (
 
 	"github.com/gorilla/mux"
 	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/client_golang/prometheus/promauto"
 	"go.opentelemetry.io/otel"
-	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
 	"google.golang.org/grpc/health/grpc_health_v1"
 
@@ -34,7 +32,6 @@ import (
 	"github.com/grafana/grafana/pkg/services/grpcserver/interceptors"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/storage/unified/resource"
-	"github.com/grafana/grafana/pkg/storage/unified/resource/grpc"
 	"github.com/grafana/grafana/pkg/storage/unified/resourcepb"
 	"github.com/grafana/grafana/pkg/storage/unified/search"
 	"github.com/grafana/grafana/pkg/util/scheduler"
@@ -103,12 +100,8 @@ func ProvideUnifiedStorageGrpcService(
 	var err error
 	tracer := otel.Tracer("unified-storage")
 
-	// FIXME: This is a temporary solution while we are migrating to the new authn interceptor
+	authCfg := ReadGrpcServerConfig(cfg)
-	// grpcutils.NewGrpcAuthenticator should be used instead.
+	authn := grpcutils.NewAuthenticator(authCfg, tracer)
-	authn := NewAuthenticatorWithFallback(cfg, reg, tracer, func(ctx context.Context) (context.Context, error) {
-		auth := grpc.Authenticator{Tracer: tracer}
-		return auth.Authenticate(ctx)
-	})
 
 	s := &service{
 		backend:            backend,
@@ -378,50 +371,6 @@ func (s *service) stopping(_ error) error {
 	return nil
 }
 
-type authenticatorWithFallback struct {
-	authenticator func(ctx context.Context) (context.Context, error)
-	fallback      func(ctx context.Context) (context.Context, error)
-	metrics       *metrics
-	tracer        trace.Tracer
-}
-
-type metrics struct {
-	requestsTotal *prometheus.CounterVec
-}
-
-func (f *authenticatorWithFallback) Authenticate(ctx context.Context) (context.Context, error) {
-	ctx, span := f.tracer.Start(ctx, "grpcutils.AuthenticatorWithFallback.Authenticate")
-	defer span.End()
-
-	// Try to authenticate with the new authenticator first
-	span.SetAttributes(attribute.Bool("fallback_used", false))
-	newCtx, err := f.authenticator(ctx)
-	if err == nil {
-		// fallback not used, authentication successful
-		f.metrics.requestsTotal.WithLabelValues("false", "true").Inc()
-		return newCtx, nil
-	}
-
-	// In case of error, fallback to the legacy authenticator
-	span.SetAttributes(attribute.Bool("fallback_used", true))
-	newCtx, err = f.fallback(ctx)
-	if newCtx != nil {
-		newCtx = resource.WithFallback(newCtx)
-	}
-	f.metrics.requestsTotal.WithLabelValues("true", fmt.Sprintf("%t", err == nil)).Inc()
-	return newCtx, err
-}
-
-func newMetrics(reg prometheus.Registerer) *metrics {
-	return &metrics{
-		requestsTotal: promauto.With(reg).NewCounterVec(
-			prometheus.CounterOpts{
-				Name: "grafana_grpc_authenticator_with_fallback_requests_total",
-				Help: "Number requests using the authenticator with fallback",
-			}, []string{"fallback_used", "result"}),
-	}
-}
-
 func ReadGrpcServerConfig(cfg *setting.Cfg) *grpcutils.AuthenticatorConfig {
 	section := cfg.SectionWithEnvOverrides("grpc_server_authentication")
 
@@ -432,21 +381,6 @@ func ReadGrpcServerConfig(cfg *setting.Cfg) *grpcutils.AuthenticatorConfig {
 	}
 }
 
-func NewAuthenticatorWithFallback(cfg *setting.Cfg, reg prometheus.Registerer, tracer trace.Tracer, fallback func(context.Context) (context.Context, error)) func(context.Context) (context.Context, error) {
-	authCfg := ReadGrpcServerConfig(cfg)
-	authenticator := grpcutils.NewAuthenticator(authCfg, tracer)
-	metrics := newMetrics(reg)
-	return func(ctx context.Context) (context.Context, error) {
-		a := &authenticatorWithFallback{
-			authenticator: authenticator,
-			fallback:      fallback,
-			tracer:        tracer,
-			metrics:       metrics,
-		}
-		return a.Authenticate(ctx)
-	}
-}
-
 func toLifecyclerConfig(cfg *setting.Cfg, logger log.Logger) (ring.BasicLifecyclerConfig, error) {
 	instanceAddr, err := ring.GetInstanceAddr(cfg.MemberlistBindAddr, netutil.PrivateNetworkInterfacesWithFallback([]string{"eth0", "en0"}, logger), logger, true)
 	if err != nil {