Access Control (Authorization)

PoC (before fix) to demonstrate improper access in the files subresource: Prereqs: - Grafana provisioning API deployed with authentication enabled - A repository named my-repo with a folder path hierarchy, including an unsynced subfolder like repo/files/folder/README.md - A user with Viewer role (e.g., user: viewer) and a separate user with Editor role (e.g., user: editor) - Endpoint: /apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/{repo}/files/{path} Repro steps (pre-fix behavior): 1) As a viewer, attempt to GET a file under an unsynced subfolder: curl -u viewer:viewer http://<host>/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/my-repo/files/folder/README.md 2) Observe HTTP 403 Forbidden (read access denied) due to a blanket admin fallback being applied to all verbs. Expected post-fix behavior (as implemented by this commit): 1) Re-run the same GET as the viewer: curl -u viewer:viewer http://<host>/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/my-repo/files/folder/README.md 2) Observe HTTP 200 OK with the file content, indicating per-verb read access is granted to Viewer as intended. Notes: - The fix introduces a verb-aware access checker that dispatches read vs write checks based on req.Verb and composes the existing viewer/editor fallbacks at construction time. This prevents the previous Admin-wide fallback from restricting reads orWrites inappropriately. - This PoC is based on the behavior described in the commit and tests showing viewer GETs succeeding after the change.

diff --git a/apps/provisioning/pkg/apis/auth/verb_aware_access_checker.go b/apps/provisioning/pkg/apis/auth/verb_aware_access_checker.go new file mode 100644 index 0000000000000..257c80d7af707 --- /dev/null +++ b/apps/provisioning/pkg/apis/auth/verb_aware_access_checker.go @@ -0,0 +1,51 @@ +package auth + +import ( + "context" + + authlib "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +// verbAwareAccessChecker dispatches Check to a read or a write AccessChecker +// based on the request verb. It exists so a single endpoint that handles both +// read and write verbs (e.g. the files subresource) can apply different role +// fallbacks per operation: a reader fallback for get/list/watch and a writer +// fallback for everything else. +type verbAwareAccessChecker struct { + read AccessChecker + write AccessChecker +} + +// NewVerbAwareAccessChecker composes a read checker and a write checker into +// a single AccessChecker. The inner checkers are expected to already carry +// any desired fallback roles (e.g. accessWithViewer / accessWithEditor) — +// this wrapper does not configure fallbacks itself. +func NewVerbAwareAccessChecker(read, write AccessChecker) AccessChecker { + return &verbAwareAccessChecker{read: read, write: write} +} + +// WithFallbackRole is intentionally a no-op. Per-verb fallbacks are decided at +// construction by the caller; applying a single role across both inner checkers +// would defeat the purpose of the split. +func (c *verbAwareAccessChecker) WithFallbackRole(_ identity.RoleType) AccessChecker { + return c +} + +// Check dispatches to the read or write checker based on req.Verb. +func (c *verbAwareAccessChecker) Check(ctx context.Context, req authlib.CheckRequest, folder string) error { + if isReadVerb(req.Verb) { + return c.read.Check(ctx, req, folder) + } + return c.write.Check(ctx, req, folder) +} + +func isReadVerb(verb string) bool { + switch verb { + case utils.VerbGet, utils.VerbList, utils.VerbWatch: + return true + default: + return false + } +} diff --git a/apps/provisioning/pkg/apis/auth/verb_aware_access_checker_test.go b/apps/provisioning/pkg/apis/auth/verb_aware_access_checker_test.go new file mode 100644 index 0000000000000..9c4ac9ee44bc5 --- /dev/null +++ b/apps/provisioning/pkg/apis/auth/verb_aware_access_checker_test.go @@ -0,0 +1,84 @@ +package auth + +import ( + "context" + "errors" + "testing" + + authlib "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestVerbAwareAccessChecker_DispatchesByVerb(t *testing.T) { + tests := []struct { + name string + verb string + expectReader bool + }{ + {name: "get -> reader", verb: utils.VerbGet, expectReader: true}, + {name: "list -> reader", verb: utils.VerbList, expectReader: true}, + {name: "watch -> reader", verb: utils.VerbWatch, expectReader: true}, + {name: "create -> writer", verb: utils.VerbCreate, expectReader: false}, + {name: "update -> writer", verb: utils.VerbUpdate, expectReader: false}, + {name: "patch -> writer", verb: utils.VerbPatch, expectReader: false}, + {name: "delete -> writer", verb: utils.VerbDelete, expectReader: false}, + {name: "deletecollection -> writer", verb: utils.VerbDeleteCollection, expectReader: false}, + {name: "unknown verb -> writer (deny-by-default for safety)", verb: "weirdverb", expectReader: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + readErr := errors.New("read called") + writeErr := errors.New("write called") + checker := NewVerbAwareAccessChecker( + &recordingChecker{returnErr: readErr}, + &recordingChecker{returnErr: writeErr}, + ) + + err := checker.Check(context.Background(), authlib.CheckRequest{Verb: tt.verb}, "") + + if tt.expectReader { + assert.Same(t, readErr, err, "expected read checker to be called for verb %q", tt.verb) + } else { + assert.Same(t, writeErr, err, "expected write checker to be called for verb %q", tt.verb) + } + }) + } +} + +func TestVerbAwareAccessChecker_PassesThroughAllowed(t *testing.T) { + checker := NewVerbAwareAccessChecker( + &recordingChecker{returnErr: nil}, // reader allows + &recordingChecker{returnErr: errors.New("should not be called")}, + ) + + require.NoError(t, checker.Check(context.Background(), authlib.CheckRequest{Verb: utils.VerbGet}, "")) +} + +func TestVerbAwareAccessChecker_WithFallbackRoleIsNoOp(t *testing.T) { + original := NewVerbAwareAccessChecker( + &recordingChecker{}, + &recordingChecker{}, + ) + withFallback := original.WithFallbackRole(identity.RoleAdmin) + + assert.Same(t, original, withFallback, + "WithFallbackRole on verb-aware checker must not produce a new instance — fallbacks are configured on the inner checkers") +} + +// recordingChecker is a minimal AccessChecker that records whether it was +// invoked. Used to verify dispatch in TestVerbAwareAccessChecker_DispatchesByVerb. +type recordingChecker struct { + returnErr error +} + +func (r *recordingChecker) Check(_ context.Context, _ authlib.CheckRequest, _ string) error { + return r.returnErr +} + +func (r *recordingChecker) WithFallbackRole(_ identity.RoleType) AccessChecker { + return r +} diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 306dea092c4b5..c2cbe717ff52d 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -793,7 +793,16 @@ func (b *APIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupI // TODO: Remove this connector when we deprecate the test endpoint // We should use fieldErrors from status instead. storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = NewTestConnector(b, testTester) - storage[provisioning.RepositoryResourceInfo.StoragePath("files")] = NewFilesConnector(b, b.parsers, b.clients, b.accessWithAdmin, b.folderMetadataEnabled, b.folderAPIVersion) + // The files subresource handles GET/POST/PUT/DELETE in a single connector + // and the appropriate role fallback differs per verb: reads should fall + // back to Viewer, writes to Editor. A static accessWithEditor would over- + // restrict reads when the inner authz denies; a static accessWithViewer + // would over-permit writes. Compose a verb-aware checker so each operation + // gets the right fallback. Per-resource authz still happens inside the + // connector via ProvisioningAuthorizer.AuthorizeResource, and repository- + // level operations remain Admin-gated by authorizeRepositorySubresource. + filesAccess := auth.NewVerbAwareAccessChecker(b.accessWithViewer, b.accessWithEditor) + storage[provisioning.RepositoryResourceInfo.StoragePath("files")] = NewFilesConnector(b, b.parsers, b.clients, filesAccess, b.folderMetadataEnabled, b.folderAPIVersion) storage[provisioning.RepositoryResourceInfo.StoragePath("refs")] = NewRefsConnector(b) storage[provisioning.RepositoryResourceInfo.StoragePath("resources")] = &listConnector{ getter: b, diff --git a/pkg/tests/apis/provisioning/files_test.go b/pkg/tests/apis/provisioning/files_test.go index a2d6b2d731e86..e3b7816589184 100644 --- a/pkg/tests/apis/provisioning/files_test.go +++ b/pkg/tests/apis/provisioning/files_test.go @@ -14,6 +14,8 @@ import ( provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/tests/apis" "github.com/grafana/grafana/pkg/tests/apis/provisioning/common" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -762,11 +764,13 @@ func TestIntegrationProvisioning_ReadmeFiles(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode, "editor should be able to GET README.md") }) - // Negative case for the folder-scoped check: the nested 'folder/' directory - // exists in the repo file tree but has not been synced into Grafana, so the - // viewer has no permission grant on its hash-based folder UID. Admins still - // pass thanks to wildcard access. - t.Run("viewer is denied for README in an unsynced subfolder", func(t *testing.T) { + // Raw file reads on the files subresource fall back to the Viewer role for + // GET-family verbs, so an org Viewer can read raw files anywhere in a repo + // they can see — including paths whose containing directory has not been + // synced into Grafana as a real folder resource. Without this, the + // folder-scoped check is unsatisfiable on hash-based folder UIDs and the + // endpoint is effectively Admin-only for non-synced subfolders. + t.Run("viewer can GET README in an unsynced subfolder", func(t *testing.T) { addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() url := fmt.Sprintf("http://viewer:viewer@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/folder/README.md", addr, repo) req, err := http.NewRequest(http.MethodGet, url, nil) @@ -776,8 +780,8 @@ func TestIntegrationProvisioning_ReadmeFiles(t *testing.T) { // nolint:errcheck defer resp.Body.Close() - require.Equal(t, http.StatusForbidden, resp.StatusCode, - "viewer without folders:get on the parent folder should be denied") + require.Equal(t, http.StatusOK, resp.StatusCode, + "viewer should be able to read README in an unsynced subfolder") }) t.Run("admin can GET README in an unsynced subfolder", func(t *testing.T) { @@ -793,6 +797,90 @@ func TestIntegrationProvisioning_ReadmeFiles(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode, "admin should be able to GET README in any folder") }) + // The "viewer can GET README" subtest above relies on the Viewer role + // fallback. The next three subtests use users with org role None and + // fine-grained folder permissions to verify that the inner authz check + // is still authoritative when no role fallback applies — i.e. the + // relaxed read semantic is paid for by either basic role membership or + // an explicit grant, not given freely to any authenticated user. + rootReader := helper.CreateUser("ProvisioningRootReader", apis.Org1, org.RoleNone, []resourcepermissions.SetResourcePermissionCommand{ + { + Actions: []string{"folders:read"}, + Resource: "folders", + ResourceAttribute: "uid", + ResourceID: "general", + }, + }) + + wildcardReader := helper.CreateUser("ProvisioningFoldersReader", apis.Org1, org.RoleNone, []resourcepermissions.SetResourcePermissionCommand{ + { + Actions: []string{"folders:read"}, + Resource: "folders", + ResourceAttribute: "uid", + ResourceID: "*", + }, + }) + + noGrants := helper.CreateUser("ProvisioningNoGrants", apis.Org1, org.RoleNone, nil) + + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + + t.Run("RoleNone with folders:read on general can GET README at the repo root", func(t *testing.T) { + url := fmt.Sprintf("http://%s:%s@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/README.md", + rootReader.Identity.GetLogin(), "ProvisioningRootReader", addr, repo) + req, err := http.NewRequest(http.MethodGet, url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + // nolint:errcheck + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, + "explicit folders:read on general should permit reading README at repo root") + }) + + t.Run("RoleNone with folders:read on general is denied for README in unsynced subfolder", func(t *testing.T) { + url := fmt.Sprintf("http://%s:%s@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/folder/README.md", + rootReader.Identity.GetLogin(), "ProvisioningRootReader", addr, repo) + req, err := http.NewRequest(http.MethodGet, url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + // nolint:errcheck + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode, + "folders:read on general should not extend to a hash-based UID for an unsynced subfolder, and there is no role fallback for org-None") + }) + + t.Run("RoleNone with folders:read wildcard can GET README in an unsynced subfolder", func(t *testing.T) { + url := fmt.Sprintf("http://%s:%s@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/folder/README.md", + wildcardReader.Identity.GetLogin(), "ProvisioningFoldersReader", addr, repo) + req, err := http.NewRequest(http.MethodGet, url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + // nolint:errcheck + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, + "folders:read wildcard should permit reading README in any (including unsynced) subfolder without role fallback") + }) + + t.Run("RoleNone with no grants is denied for README at the repo root", func(t *testing.T) { + url := fmt.Sprintf("http://%s:%s@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/README.md", + noGrants.Identity.GetLogin(), "ProvisioningNoGrants", addr, repo) + req, err := http.NewRequest(http.MethodGet, url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + // nolint:errcheck + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode, + "a user without any folder grants and no basic role should be denied") + }) + _ = ctx }