Zanzana: Improve server side error handling (#106378)

* Zanzana: Split client and server logs

* Zanzana: Improve error handling and logging

* log internal error at the server side

* refactor

* improve errors for list request

* update go modules

* handle errors for read and write

* refactor

* reset go.mod changes

pkg/services/authz/zanzana.go

@@ -37,7 +37,7 @@ func ProvideZanzana(cfg *setting.Cfg, db db.DB, tracer tracing.Tracer, features
 		return zanzana.NewNoopClient(), nil
 	}
 
-	logger := log.New("zanzana")
+	logger := log.New("zanzana.server")
 
 	var client zanzana.Client
 	switch cfg.ZanzanaClient.Mode {
@@ -83,7 +83,7 @@ func ProvideZanzana(cfg *setting.Cfg, db db.DB, tracer tracing.Tracer, features
 			return nil, fmt.Errorf("failed to start zanzana: %w", err)
 		}
 
-		openfga, err := zanzana.NewOpenFGAServer(cfg.ZanzanaServer, store, logger)
+		openfga, err := zanzana.NewOpenFGAServer(cfg.ZanzanaServer, store)
 		if err != nil {
 			return nil, fmt.Errorf("failed to start zanzana: %w", err)
 		}
@@ -131,7 +131,7 @@ func ProvideZanzanaService(cfg *setting.Cfg, features featuremgmt.FeatureToggles
 	s := &Zanzana{
 		cfg:      cfg,
 		features: features,
-		logger:   log.New("zanzana"),
+		logger:   log.New("zanzana.server"),
 	}
 
 	s.BasicService = services.NewBasicService(s.start, s.running, s.stopping).WithName("zanzana")
@@ -167,7 +167,7 @@ func (z *Zanzana) start(ctx context.Context) error {
 		return fmt.Errorf("failed to initilize zanana store: %w", err)
 	}
 
-	openfgaServer, err := zanzana.NewOpenFGAServer(z.cfg.ZanzanaServer, store, z.logger)
+	openfgaServer, err := zanzana.NewOpenFGAServer(z.cfg.ZanzanaServer, store)
 	if err != nil {
 		return fmt.Errorf("failed to start zanzana: %w", err)
 	}

pkg/services/authz/zanzana/client/client.go

@@ -30,7 +30,7 @@ func New(cc grpc.ClientConnInterface) (*Client, error) {
 		authzlibclient: authzlibclient,
 		authz:          authzv1.NewAuthzServiceClient(cc),
 		authzext:       authzextv1.NewAuthzExtentionServiceClient(cc),
-		logger:         log.New("zanzana-client"),
+		logger:         log.New("zanzana.client"),
 	}
 
 	return c, nil

pkg/services/authz/zanzana/server.go

@@ -21,8 +21,8 @@ func NewHealthServer(target server.DiagnosticServer) *server.HealthServer {
 	return server.NewHealthServer(target)
 }
 
-func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store openfgastorage.OpenFGADatastore, logger log.Logger) (*openfgaserver.Server, error) {
-	return server.NewOpenFGAServer(cfg, store, logger)
+func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store openfgastorage.OpenFGADatastore) (*openfgaserver.Server, error) {
+	return server.NewOpenFGAServer(cfg, store)
 }
 
 func NewOpenFGAHttpServer(cfg setting.ZanzanaServerSettings, srv grpcserver.Provider) (*http.Server, error) {

pkg/services/authz/zanzana/server/openfga_server.go

@@ -26,7 +26,9 @@ import (
 	zlogger "github.com/grafana/grafana/pkg/services/authz/zanzana/logger"
 )
 
-func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store storage.OpenFGADatastore, logger log.Logger) (*server.Server, error) {
+func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store storage.OpenFGADatastore) (*server.Server, error) {
+	logger := log.New("openfga.server")
+
 	opts := []server.OpenFGAServiceV1Option{
 		server.WithDatastore(store),
 		server.WithLogger(zlogger.New(logger)),

pkg/services/authz/zanzana/server/server_check.go

@@ -2,9 +2,12 @@ package server
 
 import (
 	"context"
+	"errors"
+	"fmt"
 
 	authzv1 "github.com/grafana/authlib/authz/proto/v1"
 	openfgav1 "github.com/openfga/api/proto/openfga/v1"
+	"google.golang.org/protobuf/types/known/structpb"
 
 	"github.com/grafana/grafana/pkg/services/authz/zanzana/common"
 )
@@ -13,26 +16,36 @@ func (s *Server) Check(ctx context.Context, r *authzv1.CheckRequest) (*authzv1.C
 	ctx, span := s.tracer.Start(ctx, "server.Check")
 	defer span.End()
 
+	res, err := s.check(ctx, r)
+	if err != nil {
+		s.logger.Error("failed to perform check request", "error", err, "namespace", r.GetNamespace())
+		return nil, errors.New("failed to perform check request")
+	}
+
+	return res, nil
+}
+
+func (s *Server) check(ctx context.Context, r *authzv1.CheckRequest) (*authzv1.CheckResponse, error) {
 	if err := authorize(ctx, r.GetNamespace()); err != nil {
 		return nil, err
 	}
 
 	store, err := s.getStoreInfo(ctx, r.GetNamespace())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get openfga store: %w", err)
 	}
 
 	relation := common.VerbMapping[r.GetVerb()]
 
 	contextuals, err := s.getContextuals(r.GetSubject())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get contextual tuples: %w", err)
 	}
 
 	resource := common.NewResourceInfoFromCheck(r)
 	res, err := s.checkGroupResource(ctx, r.GetSubject(), relation, resource, contextuals, store)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to check group resource: %w", err)
 	}
 
 	if res.GetAllowed() {
@@ -40,10 +53,18 @@ func (s *Server) Check(ctx context.Context, r *authzv1.CheckRequest) (*authzv1.C
 	}
 
 	if resource.IsGeneric() {
-		return s.checkGeneric(ctx, r.GetSubject(), relation, resource, contextuals, store)
+		res, err = s.checkGeneric(ctx, r.GetSubject(), relation, resource, contextuals, store)
+		if err != nil {
+			return nil, fmt.Errorf("failed to check generic resource: %w", err)
+		}
+		return res, nil
 	}
 
-	return s.checkTyped(ctx, r.GetSubject(), relation, resource, contextuals, store)
+	res, err = s.checkTyped(ctx, r.GetSubject(), relation, resource, contextuals, store)
+	if err != nil {
+		return nil, fmt.Errorf("failed to check typed resource: %w", err)
+	}
+	return res, nil
 }
 
 // checkGroupResource check if subject has access to the full "GroupResource", if they do they can access every object
@@ -53,16 +74,7 @@ func (s *Server) checkGroupResource(ctx context.Context, subject, relation strin
 		return &authzv1.CheckResponse{Allowed: false}, nil
 	}
 
-	res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{
-		StoreId:              store.ID,
-		AuthorizationModelId: store.ModelID,
-		TupleKey: &openfgav1.CheckRequestTupleKey{
-			User:     subject,
-			Relation: relation,
-			Object:   resource.GroupResourceIdent(),
-		},
-		ContextualTuples: contextuals,
-	})
+	res, err := s.openfgaCheck(ctx, store, subject, relation, resource.GroupResourceIdent(), contextuals, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -84,18 +96,7 @@ func (s *Server) checkTyped(ctx context.Context, subject, relation string, resou
 
 	if resource.HasSubresource() {
 		// Check if subject has access as a subresource
-		res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{
-			StoreId:              store.ID,
-			AuthorizationModelId: store.ModelID,
-			TupleKey: &openfgav1.CheckRequestTupleKey{
-				User:     subject,
-				Relation: subresourceRelation,
-				Object:   resourceIdent,
-			},
-			Context:          resourceCtx,
-			ContextualTuples: contextuals,
-		})
-
+		res, err := s.openfgaCheck(ctx, store, subject, subresourceRelation, resourceIdent, contextuals, resourceCtx)
 		if err != nil {
 			return nil, err
 		}
@@ -110,25 +111,12 @@ func (s *Server) checkTyped(ctx context.Context, subject, relation string, resou
 	}
 
 	// Check if subject has direct access to resource
-	res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{
-		StoreId:              store.ID,
-		AuthorizationModelId: store.ModelID,
-		TupleKey: &openfgav1.CheckRequestTupleKey{
-			User:     subject,
-			Relation: relation,
-			Object:   resourceIdent,
-		},
-		ContextualTuples: contextuals,
-	})
+	res, err := s.openfgaCheck(ctx, store, subject, relation, resourceIdent, contextuals, nil)
 	if err != nil {
 		return nil, err
 	}
 
-	if res.GetAllowed() {
-		return &authzv1.CheckResponse{Allowed: true}, nil
-	}
-
-	return &authzv1.CheckResponse{Allowed: false}, nil
+	return &authzv1.CheckResponse{Allowed: res.GetAllowed()}, nil
 }
 
 // checkGeneric check our generic "resource" type. It checks:
@@ -143,18 +131,7 @@ func (s *Server) checkGeneric(ctx context.Context, subject, relation string, res
 
 	if folderIdent != "" && common.IsSubresourceRelation(folderRelation) {
 		// Check if subject has access as a sub resource for the folder
-		res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{
-			StoreId:              store.ID,
-			AuthorizationModelId: store.ModelID,
-			TupleKey: &openfgav1.CheckRequestTupleKey{
-				User:     subject,
-				Relation: folderRelation,
-				Object:   folderIdent,
-			},
-			Context:          resourceCtx,
-			ContextualTuples: contextuals,
-		})
-
+		res, err := s.openfgaCheck(ctx, store, subject, folderRelation, folderIdent, contextuals, resourceCtx)
 		if err != nil {
 			return nil, err
 		}
@@ -170,21 +147,33 @@ func (s *Server) checkGeneric(ctx context.Context, subject, relation string, res
 	}
 
 	// Check if subject has direct access to resource
+	res, err := s.openfgaCheck(ctx, store, subject, relation, resourceIdent, contextuals, resourceCtx)
+	if err != nil {
+		return nil, err
+	}
+
+	return &authzv1.CheckResponse{Allowed: res.GetAllowed()}, nil
+}
+
+func (s *Server) openfgaCheck(ctx context.Context, store *storeInfo, subject, relation, object string, contextuals *openfgav1.ContextualTupleKeys, resourceCtx *structpb.Struct) (*openfgav1.CheckResponse, error) {
 	res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{
 		StoreId:              store.ID,
 		AuthorizationModelId: store.ModelID,
 		TupleKey: &openfgav1.CheckRequestTupleKey{
 			User:     subject,
 			Relation: relation,
-			Object:   resourceIdent,
+			Object:   object,
 		},
 		Context:          resourceCtx,
 		ContextualTuples: contextuals,
 	})
 
 	if err != nil {
-		return nil, err
+		// error is decorated by openfga with a public-facing error message, so we need to unwrap it to get the actual error and log it server-side,
+		// but we want to return wrapped error to the client to prevent leaking internal error details
+		s.logger.Error("failed to perform check", "error", errors.Unwrap(err), "subject", subject, "relation", relation, "object", object)
+		return nil, fmt.Errorf("failed to perform openfga Check request: %w", err)
 	}
 
-	return &authzv1.CheckResponse{Allowed: res.GetAllowed()}, nil
+	return res, nil
 }

pkg/services/authz/zanzana/server/server_list.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/base64"
 	"errors"
+	"fmt"
 	"hash/fnv"
 	"io"
 	"strings"
@@ -18,18 +19,28 @@ func (s *Server) List(ctx context.Context, r *authzv1.ListRequest) (*authzv1.Lis
 	ctx, span := s.tracer.Start(ctx, "server.List")
 	defer span.End()
 
+	res, err := s.list(ctx, r)
+	if err != nil {
+		s.logger.Error("failed to perform list request", "error", err, "namespace", r.GetNamespace())
+		return nil, errors.New("failed to perform list request")
+	}
+
+	return res, nil
+}
+
+func (s *Server) list(ctx context.Context, r *authzv1.ListRequest) (*authzv1.ListResponse, error) {
 	if err := authorize(ctx, r.GetNamespace()); err != nil {
 		return nil, err
 	}
 
 	store, err := s.getStoreInfo(ctx, r.GetNamespace())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get openfga store: %w", err)
 	}
 
 	contextuals, err := s.getContextuals(r.GetSubject())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get contextual tuples: %w", err)
 	}
 
 	relation := common.VerbMapping[r.GetVerb()]
@@ -37,7 +48,7 @@ func (s *Server) List(ctx context.Context, r *authzv1.ListRequest) (*authzv1.Lis
 
 	res, err := s.checkGroupResource(ctx, r.GetSubject(), relation, resource, contextuals, store)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to check group resource: %w", err)
 	}
 
 	if res.GetAllowed() {
@@ -161,7 +172,13 @@ func (s *Server) listObjects(ctx context.Context, req *openfgav1.ListObjectsRequ
 		return s.listObjectCached(ctx, req, fn)
 	}
 
-	return fn(ctx, req)
+	res, err := fn(ctx, req)
+	if err != nil {
+		s.logger.Error("failed to perform openfga ListObjects request", "error", errors.Unwrap(err), "user", req.GetUser(), "type", req.GetType(), "relation", req.GetRelation())
+		return nil, err
+	}
+
+	return res, nil
 }
 
 type listFn func(ctx context.Context, req *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error)

pkg/services/authz/zanzana/server/server_read.go

@@ -2,6 +2,8 @@ package server
 
 import (
 	"context"
+	"errors"
+	"fmt"
 
 	openfgav1 "github.com/openfga/api/proto/openfga/v1"
 
@@ -13,13 +15,23 @@ func (s *Server) Read(ctx context.Context, req *authzextv1.ReadRequest) (*authze
 	ctx, span := s.tracer.Start(ctx, "server.Read")
 	defer span.End()
 
+	res, err := s.read(ctx, req)
+	if err != nil {
+		s.logger.Error("failed to perform read request", "error", err, "namespace", req.GetNamespace())
+		return nil, errors.New("failed to perform read request")
+	}
+
+	return res, nil
+}
+
+func (s *Server) read(ctx context.Context, req *authzextv1.ReadRequest) (*authzextv1.ReadResponse, error) {
 	if err := authorize(ctx, req.GetNamespace()); err != nil {
 		return nil, err
 	}
 
 	storeInf, err := s.getStoreInfo(ctx, req.Namespace)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get openfga store: %w", err)
 	}
 
 	readReq := &openfgav1.ReadRequest{
@@ -38,6 +50,7 @@ func (s *Server) Read(ctx context.Context, req *authzextv1.ReadRequest) (*authze
 
 	res, err := s.openfga.Read(ctx, readReq)
 	if err != nil {
+		s.logger.Error("failed to perform openfga Read request", "error", errors.Unwrap(err))
 		return nil, err
 	}
 

pkg/services/authz/zanzana/server/server_test.go

@@ -89,7 +89,7 @@ func setup(t *testing.T, testDB db.DB, cfg *setting.Cfg) *Server {
 
 	store, err := store.NewEmbeddedStore(cfg, testDB, log.NewNopLogger())
 	require.NoError(t, err)
-	openfga, err := NewOpenFGAServer(cfg.ZanzanaServer, store, log.NewNopLogger())
+	openfga, err := NewOpenFGAServer(cfg.ZanzanaServer, store)
 	require.NoError(t, err)
 
 	srv, err := NewServer(cfg.ZanzanaServer, openfga, log.NewNopLogger(), tracing.NewNoopTracerService())

pkg/services/authz/zanzana/server/server_write.go

@@ -2,6 +2,8 @@ package server
 
 import (
 	"context"
+	"errors"
+	"fmt"
 
 	openfgav1 "github.com/openfga/api/proto/openfga/v1"
 
@@ -13,13 +15,23 @@ func (s *Server) Write(ctx context.Context, req *authzextv1.WriteRequest) (*auth
 	ctx, span := s.tracer.Start(ctx, "server.Write")
 	defer span.End()
 
+	res, err := s.write(ctx, req)
+	if err != nil {
+		s.logger.Error("failed to perform write request", "error", err, "namespace", req.GetNamespace())
+		return nil, errors.New("failed to perform write request")
+	}
+
+	return res, nil
+}
+
+func (s *Server) write(ctx context.Context, req *authzextv1.WriteRequest) (*authzextv1.WriteResponse, error) {
 	if err := authorize(ctx, req.GetNamespace()); err != nil {
 		return nil, err
 	}
 
 	storeInf, err := s.getStoreInfo(ctx, req.Namespace)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get openfga store: %w", err)
 	}
 
 	writeTuples := make([]*openfgav1.TupleKey, 0)
@@ -49,6 +61,7 @@ func (s *Server) Write(ctx context.Context, req *authzextv1.WriteRequest) (*auth
 
 	_, err = s.openfga.Write(ctx, writeReq)
 	if err != nil {
+		s.logger.Error("failed to perform openfga Write request", "error", errors.Unwrap(err))
 		return nil, err
 	}