From 8393ff78fce165f39d62b14f2f38c8e1345cc364 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 14 May 2024 15:15:36 +0800 Subject: [PATCH] Protected tag is no internal server error (#30962) (#30970) Backport #30962 by @KN4CK3R Fixes #30959 Adds an API test for protected tags. Fix existing tag in combination with fixtures. Co-authored-by: KN4CK3R --- models/fixtures/protected_tag.yml | 24 ++++++++++++++++++++++++ routers/api/v1/repo/release.go | 11 ++++++++--- routers/api/v1/repo/release_tags.go | 6 +++--- routers/api/v1/repo/tag.go | 8 ++++++-- templates/swagger/v1_json.tmpl | 17 +++++++++++++---- tests/integration/api_releases_test.go | 25 +++++++++++++++++++++++++ tests/integration/repo_tag_test.go | 21 ++++----------------- 7 files changed, 83 insertions(+), 29 deletions(-) create mode 100644 models/fixtures/protected_tag.yml diff --git a/models/fixtures/protected_tag.yml b/models/fixtures/protected_tag.yml new file mode 100644 index 000000000..dbec52c0c --- /dev/null +++ b/models/fixtures/protected_tag.yml @@ -0,0 +1,24 @@ +- + id: 1 + repo_id: 4 + name_pattern: /v.+/ + allowlist_user_i_ds: [] + allowlist_team_i_ds: [] + created_unix: 1715596037 + updated_unix: 1715596037 +- + id: 2 + repo_id: 1 + name_pattern: v-* + allowlist_user_i_ds: [] + allowlist_team_i_ds: [] + created_unix: 1715596037 + updated_unix: 1715596037 +- + id: 3 + repo_id: 1 + name_pattern: v-1.1 + allowlist_user_i_ds: [2] + allowlist_team_i_ds: [] + created_unix: 1715596037 + updated_unix: 1715596037 diff --git a/routers/api/v1/repo/release.go b/routers/api/v1/repo/release.go index f0f3c0bbc..f92fb86f5 100644 --- a/routers/api/v1/repo/release.go +++ b/routers/api/v1/repo/release.go @@ -215,6 +215,9 @@ func CreateRelease(ctx *context.APIContext) { // "$ref": "#/responses/notFound" // "409": // "$ref": "#/responses/error" + // "422": + // "$ref": "#/responses/validationError" + form := web.GetForm(ctx).(*api.CreateReleaseOption) if ctx.Repo.Repository.IsEmpty { ctx.Error(http.StatusUnprocessableEntity, "RepoIsEmpty", fmt.Errorf("repo is empty")) @@ -246,6 +249,8 @@ func CreateRelease(ctx *context.APIContext) { if err := release_service.CreateRelease(ctx.Repo.GitRepo, rel, nil, ""); err != nil { if repo_model.IsErrReleaseAlreadyExist(err) { ctx.Error(http.StatusConflict, "ReleaseAlreadyExist", err) + } else if models.IsErrProtectedTagName(err) { + ctx.Error(http.StatusUnprocessableEntity, "ProtectedTagName", err) } else { ctx.Error(http.StatusInternalServerError, "CreateRelease", err) } @@ -386,8 +391,8 @@ func DeleteRelease(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/notFound" - // "405": - // "$ref": "#/responses/empty" + // "422": + // "$ref": "#/responses/validationError" id := ctx.ParamsInt64(":id") rel, err := repo_model.GetReleaseForRepoByID(ctx, ctx.Repo.Repository.ID, id) @@ -401,7 +406,7 @@ func DeleteRelease(ctx *context.APIContext) { } if err := release_service.DeleteReleaseByID(ctx, ctx.Repo.Repository, rel, ctx.Doer, false); err != nil { if models.IsErrProtectedTagName(err) { - ctx.Error(http.StatusMethodNotAllowed, "delTag", "user not allowed to delete protected tag") + ctx.Error(http.StatusUnprocessableEntity, "delTag", "user not allowed to delete protected tag") return } ctx.Error(http.StatusInternalServerError, "DeleteReleaseByID", err) diff --git a/routers/api/v1/repo/release_tags.go b/routers/api/v1/repo/release_tags.go index fec91164a..f845fad53 100644 --- a/routers/api/v1/repo/release_tags.go +++ b/routers/api/v1/repo/release_tags.go @@ -92,8 +92,8 @@ func DeleteReleaseByTag(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/notFound" - // "405": - // "$ref": "#/responses/empty" + // "422": + // "$ref": "#/responses/validationError" tag := ctx.Params(":tag") @@ -114,7 +114,7 @@ func DeleteReleaseByTag(ctx *context.APIContext) { if err = releaseservice.DeleteReleaseByID(ctx, ctx.Repo.Repository, release, ctx.Doer, false); err != nil { if models.IsErrProtectedTagName(err) { - ctx.Error(http.StatusMethodNotAllowed, "delTag", "user not allowed to delete protected tag") + ctx.Error(http.StatusUnprocessableEntity, "delTag", "user not allowed to delete protected tag") return } ctx.Error(http.StatusInternalServerError, "DeleteReleaseByID", err) diff --git a/routers/api/v1/repo/tag.go b/routers/api/v1/repo/tag.go index a6908f361..8577a0e89 100644 --- a/routers/api/v1/repo/tag.go +++ b/routers/api/v1/repo/tag.go @@ -184,6 +184,8 @@ func CreateTag(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "409": // "$ref": "#/responses/conflict" + // "422": + // "$ref": "#/responses/validationError" // "423": // "$ref": "#/responses/repoArchivedError" form := web.GetForm(ctx).(*api.CreateTagOption) @@ -205,7 +207,7 @@ func CreateTag(ctx *context.APIContext) { return } if models.IsErrProtectedTagName(err) { - ctx.Error(http.StatusMethodNotAllowed, "CreateNewTag", "user not allowed to create protected tag") + ctx.Error(http.StatusUnprocessableEntity, "CreateNewTag", "user not allowed to create protected tag") return } @@ -253,6 +255,8 @@ func DeleteTag(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "409": // "$ref": "#/responses/conflict" + // "422": + // "$ref": "#/responses/validationError" // "423": // "$ref": "#/responses/repoArchivedError" tagName := ctx.Params("*") @@ -274,7 +278,7 @@ func DeleteTag(ctx *context.APIContext) { if err = releaseservice.DeleteReleaseByID(ctx, ctx.Repo.Repository, tag, ctx.Doer, true); err != nil { if models.IsErrProtectedTagName(err) { - ctx.Error(http.StatusMethodNotAllowed, "delTag", "user not allowed to delete protected tag") + ctx.Error(http.StatusUnprocessableEntity, "delTag", "user not allowed to delete protected tag") return } ctx.Error(http.StatusInternalServerError, "DeleteReleaseByID", err) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 163490108..1c23c72ac 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -12771,6 +12771,9 @@ }, "409": { "$ref": "#/responses/error" + }, + "422": { + "$ref": "#/responses/validationError" } } } @@ -12889,8 +12892,8 @@ "404": { "$ref": "#/responses/notFound" }, - "405": { - "$ref": "#/responses/empty" + "422": { + "$ref": "#/responses/validationError" } } } @@ -12975,8 +12978,8 @@ "404": { "$ref": "#/responses/notFound" }, - "405": { - "$ref": "#/responses/empty" + "422": { + "$ref": "#/responses/validationError" } } }, @@ -13826,6 +13829,9 @@ "409": { "$ref": "#/responses/conflict" }, + "422": { + "$ref": "#/responses/validationError" + }, "423": { "$ref": "#/responses/repoArchivedError" } @@ -13919,6 +13925,9 @@ "409": { "$ref": "#/responses/conflict" }, + "422": { + "$ref": "#/responses/validationError" + }, "423": { "$ref": "#/responses/repoArchivedError" } diff --git a/tests/integration/api_releases_test.go b/tests/integration/api_releases_test.go index 49aa4c4e1..8b7d67450 100644 --- a/tests/integration/api_releases_test.go +++ b/tests/integration/api_releases_test.go @@ -154,6 +154,31 @@ func TestAPICreateAndUpdateRelease(t *testing.T) { assert.EqualValues(t, rel.Note, newRelease.Note) } +func TestAPICreateProtectedTagRelease(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + writer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + session := loginUser(t, writer.LowerName) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + gitRepo, err := gitrepo.OpenRepository(git.DefaultContext, repo) + assert.NoError(t, err) + defer gitRepo.Close() + + commit, err := gitRepo.GetBranchCommit("master") + assert.NoError(t, err) + + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/releases", repo.OwnerName, repo.Name), &api.CreateReleaseOption{ + TagName: "v0.0.1", + Title: "v0.0.1", + IsDraft: false, + IsPrerelease: false, + Target: commit.ID.String(), + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusUnprocessableEntity) +} + func TestAPICreateReleaseToDefaultBranch(t *testing.T) { defer tests.PrepareTestEnv(t)() diff --git a/tests/integration/repo_tag_test.go b/tests/integration/repo_tag_test.go index 7e7790647..6d3d85532 100644 --- a/tests/integration/repo_tag_test.go +++ b/tests/integration/repo_tag_test.go @@ -26,22 +26,10 @@ func TestCreateNewTagProtected(t *testing.T) { repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) - t.Run("API", func(t *testing.T) { + t.Run("Code", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - err := release.CreateNewTag(git.DefaultContext, owner, repo, "master", "v-1", "first tag") - assert.NoError(t, err) - - err = git_model.InsertProtectedTag(db.DefaultContext, &git_model.ProtectedTag{ - RepoID: repo.ID, - NamePattern: "v-*", - }) - assert.NoError(t, err) - err = git_model.InsertProtectedTag(db.DefaultContext, &git_model.ProtectedTag{ - RepoID: repo.ID, - NamePattern: "v-1.1", - AllowlistUserIDs: []int64{repo.OwnerID}, - }) + err := release.CreateNewTag(git.DefaultContext, owner, repo, "master", "t-first", "first tag") assert.NoError(t, err) err = release.CreateNewTag(git.DefaultContext, owner, repo, "master", "v-2", "second tag") @@ -54,13 +42,12 @@ func TestCreateNewTagProtected(t *testing.T) { t.Run("Git", func(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { - username := "user2" - httpContext := NewAPITestContext(t, username, "repo1") + httpContext := NewAPITestContext(t, owner.Name, repo.Name) dstPath := t.TempDir() u.Path = httpContext.GitPath() - u.User = url.UserPassword(username, userPassword) + u.User = url.UserPassword(owner.Name, userPassword) doGitClone(dstPath, u)(t)