Provisioning: Use AccessChecker to verify if request has access to the parsed object (#103646)

pkg/registry/apis/provisioning/files.go

@@ -10,7 +10,9 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/registry/rest"
 
+	authlib "github.com/grafana/authlib/types"
 	"github.com/grafana/grafana-app-sdk/logging"
+	"github.com/grafana/grafana/pkg/apimachinery/identity"
 	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/resources"
@@ -24,12 +26,13 @@ const (
 
 type filesConnector struct {
 	getter  RepoGetter
+	access  authlib.AccessChecker
 	parsers resources.ParserFactory
 	clients resources.ClientFactory
 }
 
-func NewFilesConnector(getter RepoGetter, parsers resources.ParserFactory, clients resources.ClientFactory) *filesConnector {
-	return &filesConnector{getter: getter, parsers: parsers, clients: clients}
+func NewFilesConnector(getter RepoGetter, parsers resources.ParserFactory, clients resources.ClientFactory, access authlib.AccessChecker) *filesConnector {
+	return &filesConnector{getter: getter, parsers: parsers, clients: clients, access: access}
 }
 
 func (*filesConnector) New() runtime.Object {
@@ -56,10 +59,10 @@ func (*filesConnector) NewConnectOptions() (runtime.Object, bool, string) {
 }
 
 // TODO: document the synchronous write and delete on the API Spec
-func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime.Object, responder rest.Responder) (http.Handler, error) {
+func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime.Object, responder rest.Responder) (http.Handler, error) {
 	logger := logging.FromContext(ctx).With("logger", "files-connector", "repository_name", name)
 	ctx = logging.Context(ctx, logger)
-	repo, err := s.getter.GetHealthyRepository(ctx, name)
+	repo, err := c.getter.GetHealthyRepository(ctx, name)
 	if err != nil {
 		logger.Debug("failed to find repository", "error", err)
 		return nil, err
@@ -70,12 +73,12 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime.
 		return nil, apierrors.NewBadRequest("repository does not support read-writing")
 	}
 
-	parser, err := s.parsers.GetParser(ctx, readWriter)
+	parser, err := c.parsers.GetParser(ctx, readWriter)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get parser: %w", err)
 	}
 
-	clients, err := s.clients.Clients(ctx, repo.Config().Namespace)
+	clients, err := c.clients.Clients(ctx, repo.Config().Namespace)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get clients: %w", err)
 	}
@@ -85,7 +88,7 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime.
 		return nil, fmt.Errorf("failed to get folder client: %w", err)
 	}
 	folders := resources.NewFolderManager(readWriter, folderClient, resources.NewEmptyFolderTree())
-	dualReadWriter := resources.NewDualReadWriter(readWriter, parser, folders)
+	dualReadWriter := resources.NewDualReadWriter(readWriter, parser, folders, c.access)
 
 	return withTimeout(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		query := r.URL.Query()
@@ -107,7 +110,7 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime.
 
 		isDir := safepath.IsDir(filePath)
 		if r.Method == http.MethodGet && isDir {
-			files, err := s.listFolderFiles(ctx, filePath, ref, readWriter)
+			files, err := c.listFolderFiles(ctx, filePath, ref, readWriter)
 			if err != nil {
 				responder.Error(err)
 				return
@@ -200,7 +203,18 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime.
 }
 
 // listFolderFiles returns a list of files in a folder
-func (s *filesConnector) listFolderFiles(ctx context.Context, filePath string, ref string, readWriter repository.ReaderWriter) (*provisioning.FileList, error) {
+func (c *filesConnector) listFolderFiles(ctx context.Context, filePath string, ref string, readWriter repository.ReaderWriter) (*provisioning.FileList, error) {
+	id, err := identity.GetRequester(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("missing auth info in context")
+	}
+
+	// TODO: replace with access check on the repo itself
+	if !id.GetOrgRole().Includes(identity.RoleAdmin) {
+		return nil, apierrors.NewForbidden(resources.DashboardResource.GroupResource(), "",
+			fmt.Errorf("requires admin role"))
+	}
+
 	// TODO: Implement folder navigation
 	if len(filePath) > 0 {
 		return nil, apierrors.NewBadRequest("folder navigation not yet supported")

pkg/registry/apis/provisioning/register.go

@@ -24,6 +24,7 @@ import (
 	"k8s.io/kube-openapi/pkg/spec3"
 	"k8s.io/kube-openapi/pkg/validation/spec"
 
+	authlib "github.com/grafana/authlib/types"
 	"github.com/grafana/grafana-app-sdk/logging"
 	dashboard "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v0alpha1"
 	"github.com/grafana/grafana/pkg/apimachinery/identity"
@@ -98,6 +99,7 @@ type APIBuilder struct {
 	unified          resource.ResourceClient
 	secrets          secrets.Service
 	client           client.ProvisioningV0alpha1Interface
+	access           authlib.AccessChecker
 }
 
 // NewAPIBuilder creates an API builder.
@@ -116,6 +118,7 @@ func NewAPIBuilder(
 	legacyMigrator legacy.LegacyMigrator,
 	storageStatus dualwrite.Service,
 	secrets secrets.Service,
+	access authlib.AccessChecker,
 ) *APIBuilder {
 	// HACK: Assume is only public if it is HTTPS
 	isPublic := strings.HasPrefix(urlProvider(""), "https://")
@@ -140,6 +143,7 @@ func NewAPIBuilder(
 		storageStatus:       storageStatus,
 		unified:             unified,
 		secrets:             secrets,
+		access:              access,
 		jobHistory:          jobs.NewJobHistoryCache(),
 	}
 }
@@ -156,6 +160,7 @@ func RegisterAPIService(
 	client resource.ResourceClient, // implements resource.RepositoryClient
 	configProvider apiserver.RestConfigProvider,
 	ghFactory *github.Factory,
+	access authlib.AccessClient,
 	legacyMigrator legacy.LegacyMigrator,
 	storageStatus dualwrite.Service,
 	usageStatsService usagestats.Service,
@@ -180,7 +185,7 @@ func RegisterAPIService(
 		filepath.Join(cfg.DataPath, "clone"), // where repositories are cloned (temporarialy for now)
 		configProvider, ghFactory,
 		legacyMigrator, storageStatus,
-		secrets.NewSingleTenant(secretsSvc),
+		secrets.NewSingleTenant(secretsSvc), access,
 	)
 	apiregistration.RegisterAPI(builder)
 	usageStatsService.RegisterMetricsFunc(builder.collectProvisioningStats)
@@ -217,21 +222,13 @@ func (b *APIBuilder) GetAuthorizer() authorizer.Authorizer {
 			case provisioning.RepositoryResourceInfo.GetName():
 				// TODO: Support more fine-grained permissions than the basic roles. Especially on Enterprise.
 				switch a.GetSubresource() {
-				case "":
+				case "", "test", "jobs":
 					// Doing something with the repository itself.
 					if id.GetOrgRole().Includes(identity.RoleAdmin) {
 						return authorizer.DecisionAllow, "", nil
 					}
 					return authorizer.DecisionDeny, "admin role is required", nil
 
-				case "test", "export", "migrate":
-					// Testing doesn't make sense for non-admins.
-					// Exporting and migrating are potentially dangerous.
-					if id.GetOrgRole().Includes(identity.RoleAdmin) {
-						return authorizer.DecisionAllow, "", nil
-					}
-					return authorizer.DecisionDeny, "admin role is required", nil
-
 				case "webhook":
 					// When the resource is a webhook, we'll deal with permissions manually by checking signatures or similar in the webhook handler.
 					// The user in this context is usually an anonymous user, but may also be an authenticated synthetic check by the Grafana instance's operator as well.
@@ -239,19 +236,8 @@ func (b *APIBuilder) GetAuthorizer() authorizer.Authorizer {
 					return authorizer.DecisionAllow, "", nil
 
 				case "files":
-					// Reading files is allowed for everyone, so as to allow code reviews and similar.
-					// Writing files is only allowed fo Editor and higher.
-					isViewer := id.GetOrgRole().Includes(identity.RoleViewer)
-					isEditor := id.GetOrgRole().Includes(identity.RoleEditor)
-					isReadOperation := a.GetVerb() == apiutils.VerbGet
-
-					if isEditor || (isViewer && isReadOperation) {
-						return authorizer.DecisionAllow, "", nil
-					} else if isReadOperation {
-						return authorizer.DecisionDeny, "viewer role is required for reads", nil
-					} else {
-						return authorizer.DecisionDeny, "editor role is required for edits", nil
-					}
+					// Access to files is controlled by the AccessClient
+					return authorizer.DecisionAllow, "", nil
 
 				case "render":
 					// This is used to read a blob from unified storage, for GitHub PR comments.
@@ -362,7 +348,7 @@ func (b *APIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupI
 	storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = &testConnector{
 		getter: b,
 	}
-	storage[provisioning.RepositoryResourceInfo.StoragePath("files")] = NewFilesConnector(b, b.parsers, b.clients)
+	storage[provisioning.RepositoryResourceInfo.StoragePath("files")] = NewFilesConnector(b, b.parsers, b.clients, b.access)
 	storage[provisioning.RepositoryResourceInfo.StoragePath("resources")] = &listConnector{
 		getter: b,
 		lister: b.resourceLister,

pkg/registry/apis/provisioning/resources/dualwriter.go

@@ -7,9 +7,13 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"github.com/google/uuid"
+
+	authlib "github.com/grafana/authlib/types"
 	"github.com/grafana/grafana-app-sdk/logging"
 	"github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1"
 	"github.com/grafana/grafana/pkg/apimachinery/identity"
+	"github.com/grafana/grafana/pkg/apimachinery/utils"
 	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/safepath"
@@ -21,10 +25,11 @@ type DualReadWriter struct {
 	repo    repository.ReaderWriter
 	parser  Parser
 	folders *FolderManager
+	access  authlib.AccessChecker
 }
 
-func NewDualReadWriter(repo repository.ReaderWriter, parser Parser, folders *FolderManager) *DualReadWriter {
-	return &DualReadWriter{repo: repo, parser: parser, folders: folders}
+func NewDualReadWriter(repo repository.ReaderWriter, parser Parser, folders *FolderManager, access authlib.AccessChecker) *DualReadWriter {
+	return &DualReadWriter{repo: repo, parser: parser, folders: folders, access: access}
 }
 
 func (r *DualReadWriter) Read(ctx context.Context, path string, ref string) (*ParsedResource, error) {
@@ -43,6 +48,11 @@ func (r *DualReadWriter) Read(ctx context.Context, path string, ref string) (*Pa
 		return nil, fmt.Errorf("parse file: %w", err)
 	}
 
+	// Authorize the parsed resource
+	if err = r.authorize(ctx, parsed, utils.VerbGet); err != nil {
+		return nil, err
+	}
+
 	// Fail as we use the dry run for this response and it's not about updating the resource
 	if err := parsed.DryRun(ctx); err != nil {
 		return nil, fmt.Errorf("run dry run: %w", err)
@@ -73,6 +83,10 @@ func (r *DualReadWriter) Delete(ctx context.Context, path string, ref string, me
 		return nil, fmt.Errorf("parse file: %w", err)
 	}
 
+	if err = r.authorize(ctx, parsed, utils.VerbDelete); err != nil {
+		return nil, err
+	}
+
 	parsed.Action = provisioning.ResourceActionDelete
 	err = r.repo.Delete(ctx, path, ref, message)
 	if err != nil {
@@ -112,6 +126,10 @@ func (r *DualReadWriter) CreateFolder(ctx context.Context, path string, ref stri
 		return nil, fmt.Errorf("not a folder path")
 	}
 
+	if err := r.authorizeCreateFolder(ctx, path); err != nil {
+		return nil, err
+	}
+
 	// Now actually create the folder
 	if err := r.repo.Create(ctx, path, ref, nil, message); err != nil {
 		return nil, fmt.Errorf("failed to create folder: %w", err)
@@ -167,6 +185,10 @@ func (r *DualReadWriter) CreateResource(ctx context.Context, path string, ref st
 		return nil, fmt.Errorf("parse file: %w", err)
 	}
 
+	if err = r.authorize(ctx, parsed, utils.VerbCreate); err != nil {
+		return nil, err
+	}
+
 	data, err = parsed.ToSaveBytes()
 	if err != nil {
 		return nil, err
@@ -218,6 +240,10 @@ func (r *DualReadWriter) UpdateResource(ctx context.Context, path string, ref st
 		return nil, fmt.Errorf("parse file: %w", err)
 	}
 
+	if err = r.authorize(ctx, parsed, utils.VerbUpdate); err != nil {
+		return nil, err
+	}
+
 	data, err = parsed.ToSaveBytes()
 	if err != nil {
 		return nil, err
@@ -250,3 +276,51 @@ func (r *DualReadWriter) UpdateResource(ctx context.Context, path string, ref st
 
 	return parsed, nil
 }
+
+func (r *DualReadWriter) authorize(ctx context.Context, parsed *ParsedResource, verb string) error {
+	auth, ok := authlib.AuthInfoFrom(ctx)
+	if !ok {
+		return fmt.Errorf("missing auth info in context")
+	}
+	rsp, err := r.access.Check(ctx, auth, authlib.CheckRequest{
+		Group:     parsed.GVR.Group,
+		Resource:  parsed.GVR.Resource,
+		Namespace: parsed.Obj.GetNamespace(),
+		Name:      parsed.Obj.GetName(),
+		Folder:    parsed.Meta.GetFolder(),
+		Verb:      verb,
+	})
+	if err != nil {
+		return err
+	}
+	if !rsp.Allowed {
+		return apierrors.NewForbidden(parsed.GVR.GroupResource(), parsed.Obj.GetName(),
+			fmt.Errorf("no access to see embedded file"))
+	}
+	return nil
+}
+
+func (r *DualReadWriter) authorizeCreateFolder(ctx context.Context, _ string) error {
+	auth, ok := authlib.AuthInfoFrom(ctx)
+	if !ok {
+		return fmt.Errorf("missing auth info in context")
+	}
+	rsp, err := r.access.Check(ctx, auth, authlib.CheckRequest{
+		Group:     FolderResource.Group,
+		Resource:  FolderResource.Resource,
+		Namespace: r.repo.Config().GetNamespace(),
+		Verb:      utils.VerbCreate,
+
+		// TODO: Currently this checks if you can create a new folder in root
+		// Ideally we should check the path and use the explicit parent and new id
+		Name: "f" + uuid.NewString(),
+	})
+	if err != nil {
+		return err
+	}
+	if !rsp.Allowed {
+		return apierrors.NewForbidden(FolderResource.GroupResource(), "",
+			fmt.Errorf("unable to create folder resource"))
+	}
+	return nil
+}

pkg/registry/apis/provisioning/resources/folders.go

@@ -10,6 +10,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/client-go/dynamic"
 
+	"github.com/grafana/grafana/pkg/apimachinery/identity"
 	"github.com/grafana/grafana/pkg/apimachinery/utils"
 	"github.com/grafana/grafana/pkg/apis/folder/v0alpha1"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
@@ -105,6 +106,12 @@ func (fm *FolderManager) EnsureFolderExists(ctx context.Context, folder Folder,
 		return fmt.Errorf("failed to check if folder exists: %w", err)
 	}
 
+	// Always use the provisioning identity when writing
+	ctx, _, err = identity.WithProvisioningIdentity(ctx, cfg.GetNamespace())
+	if err != nil {
+		return fmt.Errorf("unable to use provisioning identity %w", err)
+	}
+
 	obj = &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"spec": map[string]any{

pkg/registry/apis/provisioning/resources/parser.go

@@ -217,7 +217,12 @@ func (f *ParsedResource) DryRun(ctx context.Context) error {
 		return fmt.Errorf("no client configured")
 	}
 
-	var err error
+	// Use the same identity that would eventually write the resource (via Run)
+	ctx, _, err := identity.WithProvisioningIdentity(ctx, f.Obj.GetNamespace())
+	if err != nil {
+		return err
+	}
+
 	// FIXME: shouldn't we check for the specific error?
 	// Dry run CREATE or UPDATE
 	f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{})

pkg/tests/apis/provisioning/provisioning_test.go

@@ -76,7 +76,7 @@ func TestIntegrationProvisioning_CreatingAndGetting(t *testing.T) {
 			require.Equal(t, http.StatusForbidden, statusCode)
 
 			// Viewer can see file listing
-			rsp = helper.ViewerREST.Get().
+			rsp = helper.AdminREST.Get().
 				Namespace("default").
 				Resource("repositories").
 				Name(name).
@@ -300,10 +300,48 @@ func TestIntegrationProvisioning_RunLocalRepository(t *testing.T) {
 		require.Equal(t, repo, val, "should have repo annotations")
 
 		// Read the file we wrote
-		obj, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", targetPath)
+		wrapObj, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", targetPath)
 		require.NoError(t, err, "read value")
-		name, _, _ = unstructured.NestedString(obj.Object, "resource", "file", "metadata", "name")
-		require.Equal(t, allPanels, name, "read the name out of the saved file")
+
+		wrap := &unstructured.Unstructured{}
+		wrap.Object, _, err = unstructured.NestedMap(wrapObj.Object, "resource", "dryRun")
+		require.NoError(t, err)
+		meta, err := utils.MetaAccessor(wrap)
+		require.NoError(t, err)
+		require.Equal(t, allPanels, meta.GetName(), "read the name out of the saved file")
+
+		// Check that an admin can update
+		meta.SetAnnotation("test", "from-provisioning")
+		body, err := json.Marshal(wrap.Object)
+		require.NoError(t, err)
+		result = helper.AdminREST.Put().
+			Namespace("default").
+			Resource("repositories").
+			Name(repo).
+			SubResource("files", targetPath).
+			Body(body).
+			SetHeader("Content-Type", "application/json").
+			Do(ctx).StatusCode(&code)
+		require.Equal(t, 200, code)
+		require.NoError(t, result.Error(), "update as admin value")
+		raw, err = result.Raw()
+		require.NoError(t, err)
+		err = json.Unmarshal(raw, wrapper)
+		require.NoError(t, err)
+		anno, _, _ := unstructured.NestedString(wrapper.Resource.File.Object, "metadata", "annotations", "test")
+		require.Equal(t, "from-provisioning", anno, "should set the annotation")
+
+		// But a viewer can not
+		result = helper.ViewerREST.Put().
+			Namespace("default").
+			Resource("repositories").
+			Name(repo).
+			SubResource("files", targetPath).
+			Body(body).
+			SetHeader("Content-Type", "application/json").
+			Do(ctx).StatusCode(&code)
+		require.Equal(t, 403, code)
+		require.True(t, apierrors.IsForbidden(result.Error()), code)
 	})
 
 	t.Run("fail using invalid paths", func(t *testing.T) {