diff --git a/apps/provisioning/pkg/apis/provisioning/v0alpha1/types.go b/apps/provisioning/pkg/apis/provisioning/v0alpha1/types.go index 9ddfcd3897f..fc24ac84869 100644 --- a/apps/provisioning/pkg/apis/provisioning/v0alpha1/types.go +++ b/apps/provisioning/pkg/apis/provisioning/v0alpha1/types.go @@ -159,6 +159,66 @@ func (r *Repository) Branch() string { return "" } +// URL returns the URL for git-based repositories +// or an empty string for local repositories +func (r *Repository) URL() string { + if !r.Spec.Type.IsGit() { + return "" + } + + switch r.Spec.Type { + case GitHubRepositoryType: + if r.Spec.GitHub != nil { + return r.Spec.GitHub.URL + } + case GitRepositoryType: + if r.Spec.Git != nil { + return r.Spec.Git.URL + } + case BitbucketRepositoryType: + if r.Spec.Bitbucket != nil { + return r.Spec.Bitbucket.URL + } + case GitLabRepositoryType: + if r.Spec.GitLab != nil { + return r.Spec.GitLab.URL + } + default: + return "" + } + + return "" +} + +func (r *Repository) Path() string { + switch r.Spec.Type { + case GitHubRepositoryType: + if r.Spec.GitHub != nil { + return r.Spec.GitHub.Path + } + case GitRepositoryType: + if r.Spec.Git != nil { + return r.Spec.Git.Path + } + case BitbucketRepositoryType: + if r.Spec.Bitbucket != nil { + return r.Spec.Bitbucket.Path + } + case GitLabRepositoryType: + if r.Spec.GitLab != nil { + return r.Spec.GitLab.Path + } + case LocalRepositoryType: + if r.Spec.Local != nil { + return r.Spec.Local.Path + } + default: + return "" + } + + return "" +} + type RepositorySpec struct { // The repository display name (shown in the UI) Title string `json:"title"` diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 920c8680b21..ffe315f63b6 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -2,9 +2,11 @@ package provisioning import ( "context" + "errors" "fmt" "net/http" "net/url" + "path/filepath" "slices" "strings" "time" @@ -75,6 +77,11 @@ var ( _ builder.OpenAPIPostProcessor = (*APIBuilder)(nil) ) +var ( + ErrRepositoryParentFolderConflict = errors.New("parent folder conflict") + ErrRepositoryDuplicatePath = errors.New("duplicate repository path") +) + // JobHistoryConfig holds configuration for job history backends type JobHistoryConfig struct { Loki *loki.Config `json:"loki,omitempty"` @@ -626,18 +633,32 @@ func invalidRepositoryError(name string, list field.ErrorList) error { } func (b *APIBuilder) getRepositoriesInNamespace(ctx context.Context) ([]provisioning.Repository, error) { - obj, err := b.store.List(ctx, &internalversion.ListOptions{ - Limit: 100, - }) - if err != nil { - return nil, err + var allRepositories []provisioning.Repository + continueToken := "" + + for { + obj, err := b.store.List(ctx, &internalversion.ListOptions{ + Limit: 100, + Continue: continueToken, + }) + if err != nil { + return nil, err + } + + repositoryList, ok := obj.(*provisioning.RepositoryList) + if !ok { + return nil, fmt.Errorf("expected repository list") + } + + allRepositories = append(allRepositories, repositoryList.Items...) + + continueToken = repositoryList.GetContinue() + if continueToken == "" { + break + } } - all, ok := obj.(*provisioning.RepositoryList) - if !ok { - return nil, fmt.Errorf("expected repository list") - } - return all.Items, nil + return allRepositories, nil } // TODO: move this to a more appropriate place. Probably controller/validation.go @@ -670,6 +691,33 @@ func (b *APIBuilder) verifyAgainstExistingRepositories(cfg *provisioning.Reposit } } + // If repo is git, ensure no other repository is defined with a child path + if cfg.Spec.Type.IsGit() { + for _, v := range all { + // skip itself + if cfg.Name == v.Name { + continue + } + if v.URL() == cfg.URL() { + if v.Path() == cfg.Path() { + return field.Forbidden(field.NewPath("spec", string(cfg.Spec.Type), "path"), + fmt.Sprintf("%s: %s", ErrRepositoryDuplicatePath.Error(), v.Name)) + } + + relPath, err := filepath.Rel(v.Path(), cfg.Path()) + if err != nil { + return field.Forbidden(field.NewPath("spec", string(cfg.Spec.Type), "path"), "failed to evaluate path: "+err.Error()) + } + // https://pkg.go.dev/path/filepath#Rel + // Rel will return "../" if the relative paths are not related + if !strings.HasPrefix(relPath, "../") { + return field.Forbidden(field.NewPath("spec", string(cfg.Spec.Type), "path"), + fmt.Sprintf("%s: %s", ErrRepositoryParentFolderConflict.Error(), v.Name)) + } + } + } + } + // Count repositories excluding the current one being created/updated count := 0 for _, v := range all { diff --git a/pkg/tests/apis/provisioning/repository_test.go b/pkg/tests/apis/provisioning/repository_test.go index 8d3e7daa089..4c49ba76814 100644 --- a/pkg/tests/apis/provisioning/repository_test.go +++ b/pkg/tests/apis/provisioning/repository_test.go @@ -3,12 +3,14 @@ package provisioning import ( "context" "encoding/json" + "errors" "fmt" "net/http" "strings" "testing" "time" + provisioningAPIServer "github.com/grafana/grafana/pkg/registry/apis/provisioning" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -213,6 +215,71 @@ func TestIntegrationProvisioning_RepositoryValidation(t *testing.T) { } }) } + + // Test Git repository path validation - ensure child paths are rejected + t.Run("Git repository path validation", func(t *testing.T) { + baseURL := "https://github.com/grafana/test-repo-path-validation" + + pathTests := []struct { + name string + path string + expectError error + }{ + { + name: "first repo with path 'demo/nested' should succeed", + path: "demo/nested", + expectError: nil, + }, + { + name: "second repo with child path 'demo/nested/again' should fail", + path: "demo/nested/again", + expectError: provisioningAPIServer.ErrRepositoryParentFolderConflict, + }, + { + name: "third repo with parent path 'demo' should fail", + path: "demo", + expectError: provisioningAPIServer.ErrRepositoryParentFolderConflict, + }, + { + name: "fourth repo with nested child path 'demo/nested/nested-second' should fail", + path: "demo/nested/again/two", + expectError: provisioningAPIServer.ErrRepositoryParentFolderConflict, + }, + { + name: "fifth repo with duplicate path 'demo/nested' should fail", + path: "demo/nested", + expectError: provisioningAPIServer.ErrRepositoryDuplicatePath, + }, + } + + for i, test := range pathTests { + t.Run(test.name, func(t *testing.T) { + repoName := fmt.Sprintf("git-path-test-%d", i+1) + gitRepo := helper.RenderObject(t, "testdata/github-readonly.json.tmpl", map[string]any{ + "Name": repoName, + "URL": baseURL, + "Path": test.path, + "SyncEnabled": false, // Disable sync to avoid external dependencies + "SyncTarget": "folder", + }) + + _, err := helper.Repositories.Resource.Create(ctx, gitRepo, metav1.CreateOptions{FieldValidation: "Strict"}) + + if test.expectError != nil { + require.Error(t, err, "Expected error for repository with path: %s", test.path) + + require.ErrorContains(t, err, test.expectError.Error(), "Error should contain expected message for path: %s", test.path) + var statusError *apierrors.StatusError + if errors.As(err, &statusError) { + require.Equal(t, metav1.StatusReasonInvalid, statusError.ErrStatus.Reason, "Should be a validation error") + require.Equal(t, http.StatusUnprocessableEntity, int(statusError.ErrStatus.Code), "Should return 422 status code") + } + } else { + require.NoError(t, err, "Expected success for repository with path: %s", test.path) + } + }) + } + }) } func TestIntegrationProvisioning_FailInvalidSchema(t *testing.T) { @@ -364,7 +431,8 @@ func TestIntegrationProvisioning_CreatingGitHubRepository(t *testing.T) { "Name": test.name, "URL": test.input, "SyncTarget": "folder", - "SyncEnabled": false, // Disable sync since we're just testing URL cleanup + "SyncEnabled": false, // Disable sync since we're just testing URL cleanup, + "Path": fmt.Sprintf("grafana-%s/", test.name), }) _, err := helper.Repositories.Resource.Create(ctx, input, metav1.CreateOptions{})