From 59557fa21f58891db0f9faf7afb6dcca229b6265 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Tue, 6 Jun 2023 10:26:09 +0200 Subject: [PATCH] Handle UserLogin properly when creating repositories - GitHub/Gitea/GitLab providers: The `UserRepositoriesClient.Create` method's `UserRepositoryRef` parameter contains a `UserLogin` field which is not taken into account for this call. Instead, the repository is actually created in the namespace of the user that is tied to the token used by the respective `gitprovider.Client` implementation. If a user now passed in a `UserLogin` that is different from the one derived from the token, the return value of `Create` would still contain the wrong `UserLogin` passed in to `Create` and methods like `GetCloneURL` would return the wrong URL. With this commit, the returned `UserRepository` now points to the correct owner of the repository. - Stash/Bitbucket Server: The Stash provider actually took the field into account and would fail to create the repository even when the token was correct. To align this provider's behaviour with the others, it now also creates the repository under the token user's account and ignores the provided `UserRepositoryRef.UserLogin` field. Signed-off-by: Max Jonas Werner --- gitea/client_repositories_org.go | 2 +- gitea/client_repositories_user.go | 7 +++++++ gitea/client_repository_commit.go | 3 --- gitea/integration_repositories_org_test.go | 4 ++-- gitea/integration_repositories_user_test.go | 23 ++++++++++++++++----- gitea/integration_suite_test.go | 4 ++-- gitea/resource_repository.go | 6 +++--- gitea/util.go | 7 ------- github/client_repositories_user.go | 8 +++++++ github/integration_test.go | 12 +++++++++++ gitlab/client_repositories_user.go | 8 +++++++ gitlab/integration_test.go | 12 +++++++++++ stash/client_repositories_user.go | 3 ++- stash/integration_repositories_user_test.go | 12 +++++++++++ stash/repositories.go | 2 +- 15 files changed, 88 insertions(+), 25 deletions(-) diff --git a/gitea/client_repositories_org.go b/gitea/client_repositories_org.go index f5bcba55..734745e9 100644 --- a/gitea/client_repositories_org.go +++ b/gitea/client_repositories_org.go @@ -77,7 +77,7 @@ func (c *OrgRepositoriesClient) List(ctx context.Context, ref gitprovider.Organi // apiObj is already validated at ListOrgRepos repos = append(repos, newOrgRepository(c.clientContext, apiObj, gitprovider.OrgRepositoryRef{ OrganizationRef: ref, - RepositoryName: *&apiObj.Name, + RepositoryName: apiObj.Name, })) } return repos, nil diff --git a/gitea/client_repositories_user.go b/gitea/client_repositories_user.go index f045d159..229968b2 100644 --- a/gitea/client_repositories_user.go +++ b/gitea/client_repositories_user.go @@ -19,6 +19,7 @@ package gitea import ( "context" "errors" + "fmt" "code.gitea.io/sdk/gitea" "github.com/fluxcd/go-git-providers/gitprovider" @@ -111,6 +112,12 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, if err != nil { return nil, err } + + if apiObj.Owner == nil { + return nil, fmt.Errorf("returned API object doesn't have an owner") + } + ref.UserLogin = apiObj.Owner.UserName + return newUserRepository(c.clientContext, apiObj, ref), nil } diff --git a/gitea/client_repository_commit.go b/gitea/client_repository_commit.go index 625f6372..e8c2e735 100644 --- a/gitea/client_repository_commit.go +++ b/gitea/client_repository_commit.go @@ -24,9 +24,6 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -var giteaNewFileMode = "100644" -var giteaBlobTypeFile = "blob" - // CommitClient implements the gitprovider.CommitClient interface. var _ gitprovider.CommitClient = &CommitClient{} diff --git a/gitea/integration_repositories_org_test.go b/gitea/integration_repositories_org_test.go index 9d82ce54..32a42321 100644 --- a/gitea/integration_repositories_org_test.go +++ b/gitea/integration_repositories_org_test.go @@ -61,9 +61,9 @@ var _ = Describe("Gitea Provider", func() { Expect(err).ToNot(HaveOccurred()) // Generate a repository name which doesn't exist already - testOrgRepoName = fmt.Sprintf("test-repo-%03d", rand.Intn(1000)) + testOrgRepoName = fmt.Sprintf("test-org-repo-%03d", rand.Intn(1000)) for findOrgRepo(repos, testOrgRepoName) != nil { - testOrgRepoName = fmt.Sprintf("test-repo-%03d", rand.Intn(1000)) + testOrgRepoName = fmt.Sprintf("test-org-repo-%03d", rand.Intn(1000)) } // We know that a repo with this name doesn't exist in the organization, let's verify we get an diff --git a/gitea/integration_repositories_user_test.go b/gitea/integration_repositories_user_test.go index a8b648b0..85a324a8 100644 --- a/gitea/integration_repositories_user_test.go +++ b/gitea/integration_repositories_user_test.go @@ -70,7 +70,7 @@ var _ = Describe("Gitea Provider", func() { } fmt.Print("Creating repository ", testRepoName, "...") - repoRef := newUserRepoRef(giteaUser, testRepoName) + repoRef := newUserRepoRef(testRepoName) // Check that the repository doesn't exist _, err = c.UserRepositories().Get(ctx, repoRef) @@ -100,8 +100,21 @@ var _ = Describe("Gitea Provider", func() { Expect(getSpec.Equals(postSpec)).To(BeTrue()) }) + It("should return correct repo info when creating a repository with wrong UserLogin", func() { + repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000)) + repoRef := newUserRepoRef(repoName) + repoRef.UserLogin = "yadda-yadda-yada" + + repo, err := c.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{}) + + Expect(err).To(BeNil()) + Expect( + repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)). + To(Equal(fmt.Sprintf("%s/%s/%s.git", giteaBaseUrl, giteaUser, repoName))) + }) + It("should error at creation time if the repo already does exist", func() { - repoRef := newUserRepoRef(giteaUser, testRepoName) + repoRef := newUserRepoRef(testRepoName) _, err := c.UserRepositories().Get(ctx, repoRef) Expect(err).ToNot(HaveOccurred()) @@ -110,7 +123,7 @@ var _ = Describe("Gitea Provider", func() { }) It("should update if the repository already exists when reconciling", func() { - repoRef := newUserRepoRef(giteaUser, testRepoName) + repoRef := newUserRepoRef(testRepoName) // No-op reconcile resp, actionTaken, err := c.UserRepositories().Reconcile(ctx, repoRef, gitprovider.RepositoryInfo{ Description: gitprovider.StringVar(defaultDescription), @@ -189,7 +202,7 @@ var _ = Describe("Gitea Provider", func() { It("should be possible to create a pr for a user repository", func() { testRepoName = fmt.Sprintf("test-user-repo2-%03d", rand.Intn(1000)) - repoRef := newUserRepoRef(giteaUser, testRepoName) + repoRef := newUserRepoRef(testRepoName) description := "test description" // Create a new repo userRepo, err := c.UserRepositories().Create(ctx, repoRef, @@ -281,7 +294,7 @@ var _ = Describe("Gitea Provider", func() { It("should be possible to download files from path and branch specified", func() { testRepoName = fmt.Sprintf("test-repo-tree-%03d", rand.Intn(1000)) - userRepoRef := newUserRepoRef(giteaUser, testRepoName) + userRepoRef := newUserRepoRef(testRepoName) repo, err := c.UserRepositories().Create(ctx, userRepoRef, gitprovider.RepositoryInfo{ DefaultBranch: gitprovider.StringVar(defaultBranch), Description: gitprovider.StringVar(defaultDescription), diff --git a/gitea/integration_suite_test.go b/gitea/integration_suite_test.go index 950f26e7..0545f1fd 100644 --- a/gitea/integration_suite_test.go +++ b/gitea/integration_suite_test.go @@ -243,9 +243,9 @@ func newUserRef(userLogin string) gitprovider.UserRef { } } -func newUserRepoRef(userLogin, repoName string) gitprovider.UserRepositoryRef { +func newUserRepoRef(repoName string) gitprovider.UserRepositoryRef { return gitprovider.UserRepositoryRef{ - UserRef: newUserRef(userLogin), + UserRef: newUserRef(giteaUser), RepositoryName: repoName, } } diff --git a/gitea/resource_repository.go b/gitea/resource_repository.go index 59391599..1fecd9c3 100644 --- a/gitea/resource_repository.go +++ b/gitea/resource_repository.go @@ -164,7 +164,7 @@ func (r *userRepository) Update(ctx context.Context) error { Archived: &r.r.Archived, DefaultMergeStyle: &r.r.DefaultMergeStyle, } - if r.r.Mirror == true { + if r.r.Mirror { opts.MirrorInterval = &r.r.MirrorInterval } apiObj, err := updateRepo(r.c, r.ref.GetIdentity(), r.ref.GetRepository(), &opts) @@ -261,7 +261,7 @@ func validateRepositoryAPI(apiObj *gitea.Repository) error { validator.Required("Name") } // Make sure visibility is valid if set - if apiObj.Private != true { + if !apiObj.Private { v := gitprovider.RepositoryVisibility("public") validator.Append(gitprovider.ValidateRepositoryVisibility(v), v, "Visibility") } else { @@ -276,7 +276,7 @@ func repositoryFromAPI(apiObj *gitea.Repository) gitprovider.RepositoryInfo { Description: &apiObj.Description, DefaultBranch: &apiObj.DefaultBranch, } - if apiObj.Private != true { + if !apiObj.Private { repo.Visibility = gitprovider.RepositoryVisibilityVar(gitprovider.RepositoryVisibility("public")) } else { repo.Visibility = gitprovider.RepositoryVisibilityVar(gitprovider.RepositoryVisibility("private")) diff --git a/gitea/util.go b/gitea/util.go index bbb4cb6f..7c900787 100644 --- a/gitea/util.go +++ b/gitea/util.go @@ -26,13 +26,6 @@ import ( "github.com/fluxcd/go-git-providers/validation" ) -const ( - alreadyExistsMagicString = "name already exists on this account" -) - -// TODO: Guard better against nil pointer dereference panics in this package, also -// validate data coming from the server - // validateUserRepositoryRef makes sure the UserRepositoryRef is valid for Gitea's usage. func validateUserRepositoryRef(ref gitprovider.UserRepositoryRef, expectedDomain string) error { // Make sure the RepositoryRef fields are valid diff --git a/github/client_repositories_user.go b/github/client_repositories_user.go index 42b853de..572bc58c 100644 --- a/github/client_repositories_user.go +++ b/github/client_repositories_user.go @@ -19,6 +19,7 @@ package github import ( "context" "errors" + "fmt" "github.com/fluxcd/go-git-providers/gitprovider" ) @@ -91,6 +92,13 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, if err != nil { return nil, err } + + owner := apiObj.GetOwner() + if owner == nil { + return nil, fmt.Errorf("returned API object doesn't have an owner") + } + ref.UserLogin = *owner.Login + return newUserRepository(c.clientContext, apiObj, ref), nil } diff --git a/github/integration_test.go b/github/integration_test.go index c24689f5..ea0acc9d 100644 --- a/github/integration_test.go +++ b/github/integration_test.go @@ -345,6 +345,18 @@ var _ = Describe("GitHub Provider", func() { Expect(getSpec.Equals(postSpec)).To(BeTrue()) }) + It("should return correct repo info when creating a repository with wrong UserLogin", func() { + repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000)) + repoRef := newUserRepoRef("yadda-yadda-yada", repoName) + + repo, err := c.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{}) + + Expect(err).To(BeNil()) + Expect( + repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)). + To(Equal(fmt.Sprintf("https://%s/%s/%s.git", githubDomain, testUser, repoName))) + }) + It("should error at creation time if the repo already does exist", func() { repoRef := newOrgRepoRef(testOrgName, testOrgRepoName) _, err := c.OrgRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{}) diff --git a/gitlab/client_repositories_user.go b/gitlab/client_repositories_user.go index 3a643961..0f8ffc85 100644 --- a/gitlab/client_repositories_user.go +++ b/gitlab/client_repositories_user.go @@ -19,6 +19,7 @@ package gitlab import ( "context" "errors" + "fmt" "github.com/fluxcd/go-git-providers/gitprovider" ) @@ -82,6 +83,7 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption, ) (gitprovider.UserRepository, error) { + // Make sure the RepositoryRef is valid if err := validateUserRepositoryRef(ref, c.domain); err != nil { return nil, err @@ -91,6 +93,12 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, if err != nil { return nil, err } + + if apiObj.Owner == nil { + return nil, fmt.Errorf("returned API object doesn't have an owner") + } + ref.UserLogin = apiObj.Owner.Username + return newUserProject(c.clientContext, apiObj, ref), nil } diff --git a/gitlab/integration_test.go b/gitlab/integration_test.go index 71ec9033..bda91f36 100644 --- a/gitlab/integration_test.go +++ b/gitlab/integration_test.go @@ -796,6 +796,18 @@ var _ = Describe("GitLab Provider", func() { Expect(errors.Is(err, gitprovider.ErrAlreadyExists)).To(BeTrue()) }) + It("should return correct repo info when creating a repository with wrong UserLogin", func() { + repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000)) + repoRef := newUserRepoRef(testBaseUrl, "yadda-yadda-yada", repoName) + + repo, err := c.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{}) + + Expect(err).To(BeNil()) + Expect( + repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)). + To(Equal(fmt.Sprintf("%s/%s/%s.git", testBaseUrl, testUserName, repoName))) + }) + It("should update if the user repo already exists when reconciling", func() { repoRef := newUserRepoRef(testBaseUrl, testUserName, testRepoName) // No-op reconcile diff --git a/stash/client_repositories_user.go b/stash/client_repositories_user.go index 3a1b241e..4b7693dd 100644 --- a/stash/client_repositories_user.go +++ b/stash/client_repositories_user.go @@ -128,7 +128,7 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, return nil, err } - apiObj, err := createRepository(ctx, c.client, addTilde(ref.UserLogin), ref, req, opts...) + apiObj, err := createRepository(ctx, c.client, addTilde(c.client.username), ref, req, opts...) if err != nil { if errors.Is(err, ErrAlreadyExists) { return nil, gitprovider.ErrAlreadyExists @@ -137,6 +137,7 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, } ref.SetSlug(apiObj.Slug) + ref.UserLogin = apiObj.UserName return newUserRepository(c.clientContext, apiObj, ref), nil } diff --git a/stash/integration_repositories_user_test.go b/stash/integration_repositories_user_test.go index 4fa217f7..7c755026 100644 --- a/stash/integration_repositories_user_test.go +++ b/stash/integration_repositories_user_test.go @@ -107,6 +107,18 @@ var _ = Describe("Stash Provider", func() { Expect(getSpec.Equals(postSpec)).To(BeTrue()) }) + It("should return correct repo info when creating a repository with wrong UserLogin", func() { + repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000)) + repoRef := newUserRepoRef("yadda-yadda-yadda", repoName) + + repo, err := client.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{}) + + Expect(err).To(BeNil(), "repository with different UserLogin failed") + Expect( + repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)). + To(Equal(fmt.Sprintf("https://%s/%s/%s.git", stashDomain, stashUser, repoName))) + }) + It("should error at creation time if the user repo already does exist", func() { repoRef := newUserRepoRef(stashUser, testRepoName) _, err := client.UserRepositories().Get(ctx, repoRef) diff --git a/stash/repositories.go b/stash/repositories.go index f2ce67b7..1acaecdc 100644 --- a/stash/repositories.go +++ b/stash/repositories.go @@ -229,7 +229,7 @@ func (s *RepositoriesService) Create(ctx context.Context, projectKey string, rep return nil, fmt.Errorf("create respository failed: %w", err) } - if resp != nil && resp.StatusCode == http.StatusBadRequest { + if resp != nil && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("create repository failed: %s", resp.Status) }