Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return 404 from Contents API when items don't exist #10323

Merged
merged 17 commits into from
Apr 15, 2020
Merged

Return 404 from Contents API when items don't exist #10323

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
c94671c
Return 404 on not exist
zeripath Feb 17, 2020
1ef8fdc
swagger update and use git.IsErrNotExist
zeripath Feb 17, 2020
b618883
Merge branch 'master' into contents-api-should-return-404-on-not-found
zeripath Feb 17, 2020
3b72c5c
Handle delete too
zeripath Feb 17, 2020
4297c11
Merge branch 'contents-api-should-return-404-on-not-found' of github.…
zeripath Feb 17, 2020
6326239
Handle delete too x2
zeripath Feb 17, 2020
6dd5071
Merge branch 'master' into contents-api-should-return-404-on-not-found
techknowlogick Feb 20, 2020
4d8dd1b
Merge branch 'master' into contents-api-should-return-404-on-not-found
sapk Feb 21, 2020
4a9d221
Fix pr 10323 (#3)
6543 Apr 1, 2020
2fc065d
Merge branch 'master' into contents-api-should-return-404-on-not-found
zeripath Apr 1, 2020
7132fc1
Merge branch 'master' into contents-api-should-return-404-on-not-found
zeripath Apr 6, 2020
dd35741
placate golangci-lint
zeripath Apr 6, 2020
21423db
Merge branch 'master' into contents-api-should-return-404-on-not-found
zeripath Apr 6, 2020
ecc9837
Update integrations/api_repo_file_delete_test.go
zeripath Apr 14, 2020
3bbf36f
Merge branch 'master' into contents-api-should-return-404-on-not-found
zeripath Apr 14, 2020
bb49646
Merge branch 'master' into contents-api-should-return-404-on-not-found
techknowlogick Apr 14, 2020
77ff95f
Merge branch 'master' into contents-api-should-return-404-on-not-found
guillep2k Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion routers/api/v1/repo/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,9 +362,15 @@ 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{
ctx.Error(http.StatusForbidden, "DeleteFile", models.ErrUserDoesNotHaveAccessToRepo{
Copy link
Member

@sapk sapk Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hesitate to think this should maybe kept as StatusInternalServerError to limit repo discovery.

In case you want to keep StatusForbidden you need to also update the others requests for consistency.
Like https://github.com/go-gitea/gitea/pull/10323/files#diff-f94444002d94f8c2766292e69cefbee2R466

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have read only access it should be forbidden status, in case you don't have any access it should be not found status but not internal server error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to get to DeleteFile you have to pass:

						m.Delete("", bind(api.DeleteFileOptions{}), repo.DeleteFile)
					}, reqRepoWriter(models.UnitTypeCode), reqToken())
				}, reqRepoReader(models.UnitTypeCode))
...
			}, repoAssignment())

CanWriteFiles does the following in addition:

// CanWriteFiles returns true if repository is editable and user has proper access level.
func CanWriteFiles(r *context.Repository) bool {
	return r.Permission.CanWrite(models.UnitTypeCode) && !r.Repository.IsMirror && !r.Repository.IsArchived
}

Therefore HttpStatusForbidden or HttpBadRequest are the correct statuses not 404 nor 500.

UserID: ctx.User.ID,
RepoName: ctx.Repo.Repository.LowerName,
})
Expand Down Expand Up @@ -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)
lafriks marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -439,6 +459,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{
Expand All @@ -452,6 +474,10 @@ func GetContents(ctx *context.APIContext) {
ref := ctx.QueryTrim("ref")

if fileList, err := repofiles.GetContentsOrList(ctx.Repo.Repository, treePath, ref); err != nil {
if git.IsErrNotExist(err) {
ctx.NotFound("GetContentsOrList", err)
return
}
ctx.Error(http.StatusInternalServerError, "GetContentsOrList", err)
} else {
ctx.JSON(http.StatusOK, fileList)
Expand Down Expand Up @@ -484,6 +510,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)
Expand Down
15 changes: 15 additions & 0 deletions templates/swagger/v1_json.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2412,6 +2412,9 @@
"responses": {
"200": {
"$ref": "#/responses/ContentsListResponse"
},
"404": {
"$ref": "#/responses/notFound"
}
}
}
Expand Down Expand Up @@ -2458,6 +2461,9 @@
"responses": {
"200": {
"$ref": "#/responses/ContentsResponse"
},
"404": {
"$ref": "#/responses/notFound"
}
}
},
Expand Down Expand Up @@ -2605,6 +2611,15 @@
"responses": {
"200": {
"$ref": "#/responses/FileDeleteResponse"
},
"400": {
"$ref": "#/responses/error"
},
"403": {
"$ref": "#/responses/error"
},
"404": {
"$ref": "#/responses/error"
}
}
}
Expand Down