From c94671c312b619aa491ff741010609831d05883d Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 17 Feb 2020 21:32:05 +0000 Subject: [PATCH 1/7] Return 404 on not exist --- routers/api/v1/repo/file.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 14923984bd765..40f7d6ddb2880 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -439,6 +439,8 @@ func GetContents(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/ContentsResponse" + // "404": + // "$ref": "#/responses/notFound" if !CanReadFiles(ctx.Repo) { ctx.Error(http.StatusInternalServerError, "GetContentsOrList", models.ErrUserDoesNotHaveAccessToRepo{ @@ -452,6 +454,10 @@ func GetContents(ctx *context.APIContext) { ref := ctx.QueryTrim("ref") if fileList, err := repofiles.GetContentsOrList(ctx.Repo.Repository, treePath, ref); err != nil { + if models.IsErrNotExist(err) { + ctx.NotFound("GetContentsOrList", err) + return + } ctx.Error(http.StatusInternalServerError, "GetContentsOrList", err) } else { ctx.JSON(http.StatusOK, fileList) From 1ef8fdc847761c0a07a2b367ec30cae48d41057e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 17 Feb 2020 21:42:09 +0000 Subject: [PATCH 2/7] swagger update and use git.IsErrNotExist --- routers/api/v1/repo/file.go | 4 +++- templates/swagger/v1_json.tmpl | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 40f7d6ddb2880..b2ebb1b37bf00 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -454,7 +454,7 @@ func GetContents(ctx *context.APIContext) { ref := ctx.QueryTrim("ref") if fileList, err := repofiles.GetContentsOrList(ctx.Repo.Repository, treePath, ref); err != nil { - if models.IsErrNotExist(err) { + if git.IsErrNotExist(err) { ctx.NotFound("GetContentsOrList", err) return } @@ -490,6 +490,8 @@ func GetContentsList(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/ContentsListResponse" + // "404": + // "$ref": "#/responses/notFound" // same as GetContents(), this function is here because swagger fails if path is empty in GetContents() interface GetContents(ctx) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index b52145a0a9b6f..61304dcafbb87 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -2412,6 +2412,9 @@ "responses": { "200": { "$ref": "#/responses/ContentsListResponse" + }, + "404": { + "$ref": "#/responses/notFound" } } } @@ -2458,6 +2461,9 @@ "responses": { "200": { "$ref": "#/responses/ContentsResponse" + }, + "404": { + "$ref": "#/responses/notFound" } } }, From 3b72c5c0fb744c65c44a71ebc65e79fc91e43a6f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 17 Feb 2020 21:59:26 +0000 Subject: [PATCH 3/7] Handle delete too --- routers/api/v1/repo/file.go | 20 ++++++++++++++++++++ templates/swagger/v1_json.tmpl | 9 +++++++++ 2 files changed, 29 insertions(+) diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index b2ebb1b37bf00..5656a37360382 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -362,6 +362,12 @@ func DeleteFile(ctx *context.APIContext, apiOpts api.DeleteFileOptions) { // responses: // "200": // "$ref": "#/responses/FileDeleteResponse" + // "400": + // "$ref": "#/responses/error" + // "403": + // "$ref": "#/responses/error" + // "404": + // "$ref": "#/responses/error" if !CanWriteFiles(ctx.Repo) { ctx.Error(http.StatusInternalServerError, "DeleteFile", models.ErrUserDoesNotHaveAccessToRepo{ @@ -402,6 +408,20 @@ func DeleteFile(ctx *context.APIContext, apiOpts api.DeleteFileOptions) { } if fileResponse, err := repofiles.DeleteRepoFile(ctx.Repo.Repository, ctx.User, opts); err != nil { + if git.IsErrBranchNotExist(err) || models.IsErrRepoFileDoesNotExist(err) || git.IsErrNotExist(err) { + ctx.Error(http.StatusNotFound, "DeleteFile", err) + return + } else if models.IsErrBranchAlreadyExists(err) || + models.IsErrFilenameInvalid(err) || + models.IsErrSHADoesNotMatch(err) || + models.IsErrCommitIDDoesNotMatch(err) || + models.IsErrSHAOrCommitIDNotProvided(err) { + ctx.Error(http.StatusBadRequest, "DeleteFile", err) + return + } else if models.IsErrUserCannotCommit(err) { + ctx.Error(http.StatusForbidden, "DeleteFile", err) + return + } ctx.Error(http.StatusInternalServerError, "DeleteFile", err) } else { ctx.JSON(http.StatusOK, fileResponse) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 61304dcafbb87..e17cf05a74e7c 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -2611,6 +2611,15 @@ "responses": { "200": { "$ref": "#/responses/FileDeleteResponse" + }, + "400": { + "$ref": "#/responses/error" + }, + "403": { + "$ref": "#/responses/error" + }, + "404": { + "$ref": "#/responses/error" } } } From 632623935f68199265d10e3f20013290fe391b24 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 17 Feb 2020 22:13:21 +0000 Subject: [PATCH 4/7] Handle delete too x2 --- routers/api/v1/repo/file.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 5656a37360382..54dc43093ee2b 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -370,7 +370,7 @@ func DeleteFile(ctx *context.APIContext, apiOpts api.DeleteFileOptions) { // "$ref": "#/responses/error" if !CanWriteFiles(ctx.Repo) { - ctx.Error(http.StatusInternalServerError, "DeleteFile", models.ErrUserDoesNotHaveAccessToRepo{ + ctx.Error(http.StatusForbidden, "DeleteFile", models.ErrUserDoesNotHaveAccessToRepo{ UserID: ctx.User.ID, RepoName: ctx.Repo.Repository.LowerName, }) From 4a9d221487ee59f2aaf00ac988cdd7a5c9a5a03f Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 1 Apr 2020 21:37:02 +0200 Subject: [PATCH 5/7] Fix pr 10323 (#3) * fix TESTS * leafe a note for fututre --- integrations/api_repo_file_delete_test.go | 9 +-------- integrations/api_repo_get_contents_list_test.go | 9 +-------- integrations/api_repo_get_contents_test.go | 9 +-------- routers/api/v1/repo/file.go | 2 +- 4 files changed, 4 insertions(+), 25 deletions(-) diff --git a/integrations/api_repo_file_delete_test.go b/integrations/api_repo_file_delete_test.go index 59b3ff91b68ec..c5e11eb164748 100644 --- a/integrations/api_repo_file_delete_test.go +++ b/integrations/api_repo_file_delete_test.go @@ -113,14 +113,7 @@ func TestAPIDeleteFile(t *testing.T) { deleteFileOptions.SHA = "badsha" url = fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo1.Name, treePath, token2) req = NewRequestWithJSON(t, "DELETE", url, &deleteFileOptions) - resp = session.MakeRequest(t, req, http.StatusInternalServerError) - expectedAPIError := context.APIError{ - Message: "sha does not match [given: " + deleteFileOptions.SHA + ", expected: " + correctSHA + "]", - URL: setting.API.SwaggerURL, - } - var apiError context.APIError - DecodeJSON(t, resp, &apiError) - assert.Equal(t, expectedAPIError, apiError) + resp = session.MakeRequest(t, req, http.StatusNotFound) // Test creating a file in repo16 by user4 who does not have write access fileID++ diff --git a/integrations/api_repo_get_contents_list_test.go b/integrations/api_repo_get_contents_list_test.go index abc4f42cf4d3e..d9593151cbd5c 100644 --- a/integrations/api_repo_get_contents_list_test.go +++ b/integrations/api_repo_get_contents_list_test.go @@ -139,14 +139,7 @@ func testAPIGetContentsList(t *testing.T, u *url.URL) { // Test file contents a file with a bad ref ref = "badref" req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?ref=%s", user2.Name, repo1.Name, treePath, ref) - resp = session.MakeRequest(t, req, http.StatusInternalServerError) - expectedAPIError := context.APIError{ - Message: "object does not exist [id: " + ref + ", rel_path: ]", - URL: setting.API.SwaggerURL, - } - var apiError context.APIError - DecodeJSON(t, resp, &apiError) - assert.Equal(t, expectedAPIError, apiError) + resp = session.MakeRequest(t, req, http.StatusNotFound) // Test accessing private ref with user token that does not have access - should fail req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo16.Name, treePath, token4) diff --git a/integrations/api_repo_get_contents_test.go b/integrations/api_repo_get_contents_test.go index 184e76831bbd5..651a763bb04c9 100644 --- a/integrations/api_repo_get_contents_test.go +++ b/integrations/api_repo_get_contents_test.go @@ -141,14 +141,7 @@ func testAPIGetContents(t *testing.T, u *url.URL) { // Test file contents a file with a bad ref ref = "badref" req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?ref=%s", user2.Name, repo1.Name, treePath, ref) - resp = session.MakeRequest(t, req, http.StatusInternalServerError) - expectedAPIError := context.APIError{ - Message: "object does not exist [id: " + ref + ", rel_path: ]", - URL: setting.API.SwaggerURL, - } - var apiError context.APIError - DecodeJSON(t, resp, &apiError) - assert.Equal(t, expectedAPIError, apiError) + resp = session.MakeRequest(t, req, http.StatusNotFound) // Test accessing private ref with user token that does not have access - should fail req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo16.Name, treePath, token4) diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 54dc43093ee2b..90a84cd54c30f 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -424,7 +424,7 @@ func DeleteFile(ctx *context.APIContext, apiOpts api.DeleteFileOptions) { } ctx.Error(http.StatusInternalServerError, "DeleteFile", err) } else { - ctx.JSON(http.StatusOK, fileResponse) + ctx.JSON(http.StatusOK, fileResponse) // FIXME on APIv2: return http.StatusNoContent } } From dd357411266b8c55dd81d94b6a1b983277938773 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 6 Apr 2020 19:32:14 +0100 Subject: [PATCH 6/7] placate golangci-lint Signed-off-by: Andrew Thornton --- integrations/api_repo_file_delete_test.go | 3 --- integrations/api_repo_get_contents_list_test.go | 1 - integrations/api_repo_get_contents_test.go | 1 - 3 files changed, 5 deletions(-) diff --git a/integrations/api_repo_file_delete_test.go b/integrations/api_repo_file_delete_test.go index c5e11eb164748..12425605831a6 100644 --- a/integrations/api_repo_file_delete_test.go +++ b/integrations/api_repo_file_delete_test.go @@ -11,8 +11,6 @@ import ( "testing" "code.gitea.io/gitea/models" - "code.gitea.io/gitea/modules/context" - "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" @@ -109,7 +107,6 @@ func TestAPIDeleteFile(t *testing.T) { treePath = fmt.Sprintf("delete/file%d.txt", fileID) createFile(user2, repo1, treePath) deleteFileOptions = getDeleteFileOptions() - correctSHA := deleteFileOptions.SHA deleteFileOptions.SHA = "badsha" url = fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo1.Name, treePath, token2) req = NewRequestWithJSON(t, "DELETE", url, &deleteFileOptions) diff --git a/integrations/api_repo_get_contents_list_test.go b/integrations/api_repo_get_contents_list_test.go index d9593151cbd5c..ddbd877a40e80 100644 --- a/integrations/api_repo_get_contents_list_test.go +++ b/integrations/api_repo_get_contents_list_test.go @@ -11,7 +11,6 @@ import ( "testing" "code.gitea.io/gitea/models" - "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" diff --git a/integrations/api_repo_get_contents_test.go b/integrations/api_repo_get_contents_test.go index 651a763bb04c9..3d4a31be81140 100644 --- a/integrations/api_repo_get_contents_test.go +++ b/integrations/api_repo_get_contents_test.go @@ -10,7 +10,6 @@ import ( "testing" "code.gitea.io/gitea/models" - "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" From ecc98378ea42225b4519a5df64fc407dd9c39835 Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 14 Apr 2020 14:26:14 +0100 Subject: [PATCH 7/7] Update integrations/api_repo_file_delete_test.go Co-Authored-By: 6543 <6543@obermui.de> --- integrations/api_repo_file_delete_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrations/api_repo_file_delete_test.go b/integrations/api_repo_file_delete_test.go index 12425605831a6..178aec6f5bc3e 100644 --- a/integrations/api_repo_file_delete_test.go +++ b/integrations/api_repo_file_delete_test.go @@ -110,7 +110,7 @@ func TestAPIDeleteFile(t *testing.T) { deleteFileOptions.SHA = "badsha" url = fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo1.Name, treePath, token2) req = NewRequestWithJSON(t, "DELETE", url, &deleteFileOptions) - resp = session.MakeRequest(t, req, http.StatusNotFound) + resp = session.MakeRequest(t, req, http.StatusBadRequest) // Test creating a file in repo16 by user4 who does not have write access fileID++