From f6124344ba5d63ebbede2cdc2cdd4dfcd3ac2424 Mon Sep 17 00:00:00 2001
From: Georges Chaudy <chaudyg@gmail.com>
Date: Thu, 28 Nov 2024 13:03:54 +0100
Subject: [PATCH] authnz: Fix panic in the authenticator and rename metric
 (#97150)

* Fix: panic

* suggestion
---
 .../authn/grpcutils/grpc_authenticator.go     | 33 +++++++++++--------
 pkg/storage/unified/resource/access.go        | 12 +++----
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/pkg/services/authn/grpcutils/grpc_authenticator.go b/pkg/services/authn/grpcutils/grpc_authenticator.go
index 8014a62beac..1b326d0d553 100644
--- a/pkg/services/authn/grpcutils/grpc_authenticator.go
+++ b/pkg/services/authn/grpcutils/grpc_authenticator.go
@@ -107,42 +107,49 @@ func FallbackUsed(ctx context.Context) bool {
 func (f *AuthenticatorWithFallback) Authenticate(ctx context.Context) (context.Context, error) {
 	ctx, span := f.tracer.Start(ctx, "grpcutils.AuthenticatorWithFallback.Authenticate")
 	defer span.End()
-	span.SetAttributes(attribute.Bool("fallback_used", false))
+
 	// Try to authenticate with the new authenticator first
+	span.SetAttributes(attribute.Bool("fallback_used", false))
 	newCtx, err := f.authenticator.Authenticate(ctx)
-	if err != nil {
-		// In case of error, fallback to the legacy authenticator
-		newCtx, err = f.fallback.Authenticate(ctx)
-		f.metrics.fallbackCounter.WithLabelValues(fmt.Sprintf("%t", err == nil)).Inc()
-		span.SetAttributes(attribute.Bool("fallback_used", true))
+	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.Authenticate(ctx)
+	if newCtx != nil {
 		newCtx = context.WithValue(newCtx, contextFallbackKey{}, true)
 	}
+	f.metrics.requestsTotal.WithLabelValues("true", fmt.Sprintf("%t", err == nil)).Inc()
 	return newCtx, err
 }
 
 const (
 	metricsNamespace = "grafana"
-	metricsSubSystem = "grpc_authenticator"
+	metricsSubSystem = "grpc_authenticator_with_fallback"
 )
 
 type metrics struct {
-	fallbackCounter *prometheus.CounterVec
+	requestsTotal *prometheus.CounterVec
 }
 
 func newMetrics(reg prometheus.Registerer) *metrics {
 	m := &metrics{
-		fallbackCounter: prometheus.NewCounterVec(
+		requestsTotal: prometheus.NewCounterVec(
 			prometheus.CounterOpts{
 				Namespace: metricsNamespace,
 				Subsystem: metricsSubSystem,
-				Name:      "fallback_total",
-				Help:      "Number of times the fallback authenticator was used",
-			}, []string{"result"}),
+				Name:      "requests_total",
+				Help:      "Number requests using the authenticator with fallback",
+			}, []string{"fallback_used", "result"}),
 	}
 
 	if reg != nil {
 		once.Do(func() {
-			reg.MustRegister(m.fallbackCounter)
+			reg.MustRegister(m.requestsTotal)
 		})
 	}
 
diff --git a/pkg/storage/unified/resource/access.go b/pkg/storage/unified/resource/access.go
index 97a8082373e..da6aad7fc2b 100644
--- a/pkg/storage/unified/resource/access.go
+++ b/pkg/storage/unified/resource/access.go
@@ -39,8 +39,8 @@ type groupResource map[string]map[string]interface{}
 // For now, it makes one call to the authz service for each list items. This is known to be inefficient.
 type authzLimitedClient struct {
 	client authz.AccessChecker
-	// whitelist is a map of group to resources that are compatible with RBAC.
-	whitelist groupResource
+	// allowlist is a map of group to resources that are compatible with RBAC.
+	allowlist groupResource
 	logger    *slog.Logger
 	tracer    trace.Tracer
 }
@@ -57,7 +57,7 @@ func NewAuthzLimitedClient(client authz.AccessChecker, opts AuthzOptions) authz.
 	}
 	return &authzLimitedClient{
 		client: client,
-		whitelist: groupResource{
+		allowlist: groupResource{
 			"dashboard.grafana.app": map[string]interface{}{"dashboards": nil},
 			"folder.grafana.app":    map[string]interface{}{"folders": nil},
 		},
@@ -107,7 +107,7 @@ func (c authzLimitedClient) Compile(ctx context.Context, id claims.AuthInfo, req
 		))
 		defer span.End()
 		if grpcutils.FallbackUsed(ctx) {
-			c.logger.Debug("Check", "group", req.Group, "resource", req.Resource, "fallback", true, "rbac", false, "allowed", true)
+			c.logger.Debug("Compile.Check", "group", req.Group, "resource", req.Resource, "fallback", true, "rbac", false, "allowed", true)
 			return true
 		}
 		// TODO: Implement For now we perform the check for each item.
@@ -134,8 +134,8 @@ func (c authzLimitedClient) Compile(ctx context.Context, id claims.AuthInfo, req
 }
 
 func (c authzLimitedClient) IsCompatibleWithRBAC(group, resource string) bool {
-	if _, ok := c.whitelist[group]; ok {
-		if _, ok := c.whitelist[group][resource]; ok {
+	if _, ok := c.allowlist[group]; ok {
+		if _, ok := c.allowlist[group][resource]; ok {
 			return true
 		}
 	}