Some improvements
This commit is contained in:
Roberto Jimenez Sanchez
@@ -112,16 +112,16 @@ func (r *DualReadWriter) Delete(ctx context.Context, opts DualWriteOptions) (*Pa
|
||||
return nil, fmt.Errorf("parse file: %w", err)
|
||||
}
|
||||
|
||||
// Authorize after simple checks but before performing the operation
|
||||
// This ensures authorization is validated for all branches
|
||||
if err = r.authorize(ctx, parsed, utils.VerbDelete); err != nil {
|
||||
// Populate the existing resource to ensure we check permissions in the correct folder
|
||||
if err = r.ensureExisting(ctx, parsed); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Check if an existing resource with this UID is managed by a different repository
|
||||
// This prevents unauthorized deletion of resources owned by other repositories
|
||||
if err = r.authorizeForExistingResource(ctx, parsed, utils.VerbDelete); err != nil {
|
||||
return nil, fmt.Errorf("not authorized to delete existing resource: %w", err)
|
||||
// Authorize after simple checks but before performing the operation
|
||||
// This checks against the existing resource's folder (if it exists) to ensure
|
||||
// we validate permissions where the resource actually lives
|
||||
if err = r.authorize(ctx, parsed, utils.VerbDelete); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
parsed.Action = provisioning.ResourceActionDelete
|
||||
@@ -160,7 +160,7 @@ func (r *DualReadWriter) CreateFolder(ctx context.Context, opts DualWriteOptions
|
||||
return nil, fmt.Errorf("not a folder path")
|
||||
}
|
||||
|
||||
if err := r.authorizeCreateFolder(ctx, opts.Path); err != nil {
|
||||
if err := r.authorizeFolder(ctx, opts.Path, utils.VerbCreate); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -250,20 +250,17 @@ func (r *DualReadWriter) createOrUpdate(ctx context.Context, create bool, opts D
|
||||
return nil, fmt.Errorf("errors while parsing file [%v]", parsed.Errors)
|
||||
}
|
||||
|
||||
// Check if an existing resource with this UID exists
|
||||
// If it does, we need permission to update/delete it
|
||||
// This prevents unauthorized overwrites of resources managed by other repositories
|
||||
// Note: This check applies to ALL operations, not just configured branch operations
|
||||
if parsed.Action == provisioning.ResourceActionCreate {
|
||||
// For creates, check if we can overwrite an existing resource with this UID
|
||||
if err = r.authorizeForExistingResource(ctx, parsed, utils.VerbUpdate); err != nil {
|
||||
return nil, fmt.Errorf("not authorized to overwrite existing resource: %w", err)
|
||||
}
|
||||
// Populate existing resource if it exists
|
||||
if err = r.ensureExisting(ctx, parsed); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Verify that we can create (or update) the referenced resource
|
||||
// Authorization check:
|
||||
// - If resource exists (parsed.Existing != nil): Check if we can update it in its current folder
|
||||
// - If resource doesn't exist: Check if we can create it in the folder from the file
|
||||
// This prevents unauthorized overwrites of existing resources and ensures proper folder permissions
|
||||
verb := utils.VerbUpdate
|
||||
if parsed.Action == provisioning.ResourceActionCreate {
|
||||
if parsed.Existing == nil && parsed.Action == provisioning.ResourceActionCreate {
|
||||
verb = utils.VerbCreate
|
||||
}
|
||||
if err = r.authorize(ctx, parsed, verb); err != nil {
|
||||
@@ -347,6 +344,16 @@ func (r *DualReadWriter) moveDirectory(ctx context.Context, opts DualWriteOption
|
||||
}
|
||||
}
|
||||
|
||||
// Check permissions to delete the original folder
|
||||
if err := r.authorizeFolder(ctx, opts.OriginalPath, utils.VerbDelete); err != nil {
|
||||
return nil, fmt.Errorf("not authorized to move from original folder: %w", err)
|
||||
}
|
||||
|
||||
// Check permissions to create at the new folder location
|
||||
if err := r.authorizeFolder(ctx, opts.Path, utils.VerbCreate); err != nil {
|
||||
return nil, fmt.Errorf("not authorized to move to new folder: %w", err)
|
||||
}
|
||||
|
||||
// For branch operations, we just perform the repository move without updating Grafana DB
|
||||
// Always use the provisioning identity when writing
|
||||
ctx, _, err := identity.WithProvisioningIdentity(ctx, r.repo.Config().Namespace)
|
||||
@@ -397,7 +404,12 @@ func (r *DualReadWriter) moveFile(ctx context.Context, opts DualWriteOptions) (*
|
||||
return nil, fmt.Errorf("parse original file: %w", err)
|
||||
}
|
||||
|
||||
// Authorize delete on the original path
|
||||
// Populate existing resource to check delete permission in the correct folder
|
||||
if err = r.ensureExisting(ctx, parsed); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Authorize delete on the original path (checks existing resource's folder if it exists)
|
||||
if err = r.authorize(ctx, parsed, utils.VerbDelete); err != nil {
|
||||
return nil, fmt.Errorf("not authorized to delete original file: %w", err)
|
||||
}
|
||||
@@ -436,23 +448,20 @@ func (r *DualReadWriter) moveFile(ctx context.Context, opts DualWriteOptions) (*
|
||||
return nil, fmt.Errorf("errors while parsing moved file [%v]", newParsed.Errors)
|
||||
}
|
||||
|
||||
// Authorize for the target resource
|
||||
// If the UID changed or this is a new file, check if an existing resource with the target UID exists
|
||||
// and verify we have permission to modify/delete it
|
||||
if newParsed.Obj.GetName() != parsed.Obj.GetName() {
|
||||
// UID changed - we need permission to delete any existing resource with the new UID
|
||||
if err = r.authorizeForExistingResource(ctx, newParsed, utils.VerbDelete); err != nil {
|
||||
return nil, fmt.Errorf("not authorized to overwrite existing resource at target: %w", err)
|
||||
}
|
||||
// Populate existing resource at destination to check if we're overwriting something
|
||||
if err = r.ensureExisting(ctx, newParsed); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Authorize create or update on the new path
|
||||
verb := utils.VerbCreate
|
||||
if newParsed.Action == provisioning.ResourceActionUpdate {
|
||||
verb = utils.VerbUpdate
|
||||
// Authorize for the target resource
|
||||
// - If resource exists at destination: Check if we can update it in its folder
|
||||
// - If no resource at destination: Check if we can create in the new folder
|
||||
verb := utils.VerbUpdate
|
||||
if newParsed.Existing == nil && newParsed.Action == provisioning.ResourceActionCreate {
|
||||
verb = utils.VerbCreate
|
||||
}
|
||||
if err = r.authorize(ctx, newParsed, verb); err != nil {
|
||||
return nil, fmt.Errorf("not authorized to create new file: %w", err)
|
||||
return nil, fmt.Errorf("not authorized for destination: %w", err)
|
||||
}
|
||||
|
||||
data, err := newParsed.ToSaveBytes()
|
||||
@@ -512,78 +521,105 @@ func (r *DualReadWriter) moveFile(ctx context.Context, opts DualWriteOptions) (*
|
||||
|
||||
// authorize checks if the requester has permission to perform the specified verb on the resource.
|
||||
// The provisioning service operates with admin privileges and validates based on resource-level permissions.
|
||||
//
|
||||
// IMPORTANT: If parsed.Existing is set, this checks permissions against the EXISTING resource's folder.
|
||||
// Otherwise, it checks against the NEW resource's folder (from parsed.Meta).
|
||||
// This ensures we validate permissions in the correct folder where the resource actually exists or will be created.
|
||||
func (r *DualReadWriter) authorize(ctx context.Context, parsed *ParsedResource, verb string) error {
|
||||
id, err := identity.GetRequester(ctx)
|
||||
if err != nil {
|
||||
return apierrors.NewUnauthorized(err.Error())
|
||||
}
|
||||
|
||||
// Determine which resource name to use for authorization
|
||||
var name string
|
||||
// Determine which resource and folder to use for authorization
|
||||
var resourceName string
|
||||
var folder string
|
||||
|
||||
if parsed.Existing != nil {
|
||||
name = parsed.Existing.GetName()
|
||||
// Check against existing resource in its current folder
|
||||
resourceName = parsed.Existing.GetName()
|
||||
if meta, err := utils.MetaAccessor(parsed.Existing); err == nil {
|
||||
folder = meta.GetFolder()
|
||||
} else {
|
||||
// Fallback to file metadata if we can't extract from existing
|
||||
folder = parsed.Meta.GetFolder()
|
||||
}
|
||||
} else {
|
||||
name = parsed.Obj.GetName()
|
||||
// Check against new resource in the folder from the file
|
||||
resourceName = parsed.Obj.GetName()
|
||||
folder = parsed.Meta.GetFolder()
|
||||
}
|
||||
|
||||
// Check resource-level permissions via access checker
|
||||
// The provisioning service is treated as admin-level for resource type operations
|
||||
rsp, err := r.access.Check(ctx, id, authlib.CheckRequest{
|
||||
Group: parsed.GVR.Group,
|
||||
Resource: parsed.GVR.Resource,
|
||||
Namespace: id.GetNamespace(),
|
||||
Name: name,
|
||||
Name: resourceName,
|
||||
Verb: verb,
|
||||
}, parsed.Meta.GetFolder())
|
||||
}, folder)
|
||||
if err != nil || !rsp.Allowed {
|
||||
return apierrors.NewForbidden(parsed.GVR.GroupResource(), parsed.Obj.GetName(),
|
||||
fmt.Errorf("no access to perform %s on the resource", verb))
|
||||
return apierrors.NewForbidden(parsed.GVR.GroupResource(), resourceName,
|
||||
fmt.Errorf("no access to perform %s on the resource in folder '%s'", verb, folder))
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// authorizeForExistingResource checks if we have permission to modify/delete an existing resource
|
||||
// with the given name. If no resource exists, this returns nil (no authorization needed).
|
||||
// This is used to prevent unauthorized overwrites of existing resources managed by other repositories.
|
||||
func (r *DualReadWriter) authorizeForExistingResource(ctx context.Context, parsed *ParsedResource, verb string) error {
|
||||
// Try to fetch the existing resource with this name/UID
|
||||
if parsed.Client == nil {
|
||||
return nil // No client means we can't check for existing resources
|
||||
// ensureExisting populates parsed.Existing if a resource with the given name exists in storage.
|
||||
// Returns nil if no resource exists, if Client is nil, or if Existing is already populated.
|
||||
// This is used before authorization checks to ensure we validate permissions against the actual
|
||||
// existing resource's folder, not just the folder specified in the file.
|
||||
func (r *DualReadWriter) ensureExisting(ctx context.Context, parsed *ParsedResource) error {
|
||||
if parsed.Client == nil || parsed.Existing != nil {
|
||||
return nil // Already populated or can't check
|
||||
}
|
||||
|
||||
existing, err := parsed.Client.Get(ctx, parsed.Obj.GetName(), metav1.GetOptions{})
|
||||
if err != nil {
|
||||
if apierrors.IsNotFound(err) {
|
||||
return nil // No existing resource, no authorization needed
|
||||
return nil // No existing resource
|
||||
}
|
||||
return fmt.Errorf("failed to check for existing resource: %w", err)
|
||||
}
|
||||
|
||||
// Resource exists - create a ParsedResource for authorization check
|
||||
existingParsed := &ParsedResource{
|
||||
Obj: existing,
|
||||
Existing: existing,
|
||||
GVR: parsed.GVR,
|
||||
GVK: parsed.GVK,
|
||||
Meta: parsed.Meta,
|
||||
Client: parsed.Client,
|
||||
}
|
||||
|
||||
// Authorize the operation on the existing resource
|
||||
return r.authorize(ctx, existingParsed, verb)
|
||||
parsed.Existing = existing
|
||||
return nil
|
||||
}
|
||||
|
||||
// authorizeCreateFolder checks if the requester has permission to create folders.
|
||||
// The provisioning service operates with admin privileges for folder creation.
|
||||
func (r *DualReadWriter) authorizeCreateFolder(ctx context.Context, _ string) error {
|
||||
_, err := identity.GetRequester(ctx)
|
||||
// authorizeFolder validates that the requester has permission to perform the specified verb on a folder.
|
||||
// This is used for folder operations (create, delete, update) where we don't have a ParsedResource.
|
||||
func (r *DualReadWriter) authorizeFolder(ctx context.Context, path string, verb string) error {
|
||||
id, err := identity.GetRequester(ctx)
|
||||
if err != nil {
|
||||
return apierrors.NewUnauthorized(err.Error())
|
||||
}
|
||||
|
||||
// Provisioning service is treated as admin-level for folder operations
|
||||
// No additional authorization checks needed beyond identity validation
|
||||
// For folder operations, we need to determine the parent folder for permission checks
|
||||
parentFolder := ""
|
||||
// TODO: Extract parent folder from path if needed for hierarchical permission checks
|
||||
|
||||
// For create operations, use empty name (checking if we can create in the parent)
|
||||
// For other operations, the folder path is the resource name
|
||||
name := path
|
||||
if verb == utils.VerbCreate {
|
||||
name = "" // Empty name for creation checks
|
||||
}
|
||||
|
||||
// Check folder permissions via access checker
|
||||
rsp, err := r.access.Check(ctx, id, authlib.CheckRequest{
|
||||
Group: FolderResource.Group,
|
||||
Resource: FolderResource.Resource,
|
||||
Namespace: id.GetNamespace(),
|
||||
Name: name,
|
||||
Verb: verb,
|
||||
}, parentFolder)
|
||||
|
||||
if err != nil || !rsp.Allowed {
|
||||
return apierrors.NewForbidden(FolderResource.GroupResource(), path,
|
||||
fmt.Errorf("no access to perform %s on folder", verb))
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -600,6 +636,11 @@ func (r *DualReadWriter) deleteFolder(ctx context.Context, opts DualWriteOptions
|
||||
}
|
||||
}
|
||||
|
||||
// Check permissions to delete the folder
|
||||
if err := r.authorizeFolder(ctx, opts.Path, utils.VerbDelete); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// For branch operations, just delete from the repository without updating Grafana DB
|
||||
err := r.repo.Delete(ctx, opts.Path, opts.Ref, opts.Message)
|
||||
if err != nil {
|
||||
|
||||
@@ -828,4 +828,142 @@ func TestIntegrationProvisioning_FilesAuthorization(t *testing.T) {
|
||||
require.Error(t, result.Error(), "viewer should not be able to delete files")
|
||||
require.True(t, apierrors.IsForbidden(result.Error()), "should return Forbidden error")
|
||||
})
|
||||
|
||||
// Folder Authorization Tests
|
||||
t.Run("POST folder (create) - Admin role should succeed", func(t *testing.T) {
|
||||
addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String()
|
||||
url := fmt.Sprintf("http://admin:admin@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/test-folder/", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, 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, "admin should be able to create folders")
|
||||
})
|
||||
|
||||
t.Run("POST folder (create) - Editor role should succeed", func(t *testing.T) {
|
||||
addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String()
|
||||
url := fmt.Sprintf("http://editor:editor@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/editor-folder/", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, 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, "editor should be able to create folders via access checker")
|
||||
})
|
||||
|
||||
t.Run("POST folder (create) - Viewer role should fail", 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/viewer-folder/", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, 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, "viewer should not be able to create folders")
|
||||
})
|
||||
|
||||
t.Run("DELETE folder on branch - Editor role should succeed", func(t *testing.T) {
|
||||
// Create a folder first
|
||||
addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String()
|
||||
createUrl := fmt.Sprintf("http://admin:admin@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/folder-to-delete/", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, createUrl, nil)
|
||||
require.NoError(t, err)
|
||||
resp, err := http.DefaultClient.Do(req)
|
||||
require.NoError(t, err)
|
||||
resp.Body.Close()
|
||||
|
||||
// Delete on a branch (delete on configured branch is not allowed for folders)
|
||||
deleteUrl := fmt.Sprintf("http://editor:editor@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/folder-to-delete/?ref=test-delete-folder-branch", addr, repo)
|
||||
req, err = http.NewRequest(http.MethodDelete, deleteUrl, 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, "editor should be able to delete folders on branches via access checker")
|
||||
})
|
||||
|
||||
t.Run("DELETE folder on branch - Viewer role should fail", 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/test-folder/?ref=test-delete-branch", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodDelete, 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, "viewer should not be able to delete folders")
|
||||
})
|
||||
|
||||
// Move File Authorization Tests
|
||||
t.Run("POST file (move) - Editor role should succeed", func(t *testing.T) {
|
||||
// Create a file first
|
||||
dashboardContent := helper.LoadFile("testdata/timeline-demo.json")
|
||||
result := helper.AdminREST.Post().
|
||||
Namespace("default").
|
||||
Resource("repositories").
|
||||
Name(repo).
|
||||
SubResource("files", "file-to-move.json").
|
||||
Body(dashboardContent).
|
||||
SetHeader("Content-Type", "application/json").
|
||||
Do(ctx)
|
||||
require.NoError(t, result.Error())
|
||||
|
||||
// Move using editor role
|
||||
addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String()
|
||||
targetUrl := fmt.Sprintf("http://editor:editor@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/moved-by-editor.json?originalPath=file-to-move.json", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, targetUrl, 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, "editor should be able to move files via access checker")
|
||||
})
|
||||
|
||||
t.Run("POST file (move) - Viewer role should fail", func(t *testing.T) {
|
||||
// Try to move using viewer role
|
||||
addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String()
|
||||
targetUrl := fmt.Sprintf("http://viewer:viewer@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/moved-by-viewer.json?originalPath=dashboard1.json", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, targetUrl, 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, "viewer should not be able to move files")
|
||||
})
|
||||
|
||||
// Move Folder Authorization Tests (on branches only, since configured branch is not allowed)
|
||||
t.Run("POST folder (move) on branch - Editor role should succeed", func(t *testing.T) {
|
||||
// Create a folder with files first
|
||||
helper.CopyToProvisioningPath(t, "testdata/text-options.json", "folder-to-move/file1.json")
|
||||
helper.SyncAndWait(t, repo, nil)
|
||||
|
||||
addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String()
|
||||
targetUrl := fmt.Sprintf("http://editor:editor@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/moved-folder/?originalPath=folder-to-move/&ref=test-move-folder-branch", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, targetUrl, 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, "editor should be able to move folders on branches via access checker")
|
||||
})
|
||||
|
||||
t.Run("POST folder (move) on branch - Viewer role should fail", func(t *testing.T) {
|
||||
addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String()
|
||||
targetUrl := fmt.Sprintf("http://viewer:viewer@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/moved-by-viewer-folder/?originalPath=test-folder/&ref=test-move-branch", addr, repo)
|
||||
req, err := http.NewRequest(http.MethodPost, targetUrl, 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, "viewer should not be able to move folders")
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user