[GITEA] API commentAssignment() to verify the id belongs

Instead of repeating the tests that verify the ID of a comment
is related to the repository of the API endpoint, add the middleware
function commentAssignment() to assign ctx.Comment if the ID of the
comment is verified to be related to the repository.

There already are integration tests for cases of potential unrelated
comment IDs that cover some of the modified endpoints which covers the
commentAssignment() function logic.

* TestAPICommentReactions - GetIssueCommentReactions
* TestAPICommentReactions - PostIssueCommentReaction
* TestAPICommentReactions - DeleteIssueCommentReaction
* TestAPIEditComment - EditIssueComment
* TestAPIDeleteComment - DeleteIssueComment
* TestAPIGetCommentAttachment - GetIssueCommentAttachment

The other modified endpoints do not have tests to verify cases of
potential unrelated comment IDs. They no longer need to because they
no longer implement the logic to enforce this. They however all have
integration tests that verify the commentAssignment() they now rely on
does not introduce a regression.

* TestAPIGetComment - GetIssueComment
* TestAPIListCommentAttachments - ListIssueCommentAttachments
* TestAPICreateCommentAttachment - CreateIssueCommentAttachment
* TestAPIEditCommentAttachment - EditIssueCommentAttachment
* TestAPIDeleteCommentAttachment - DeleteIssueCommentAttachment

modules/context/api.go

@@ -11,6 +11,7 @@ import (
 	"net/url"
 	"strings"
 
+	issues_model "code.gitea.io/gitea/models/issues"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
@@ -38,6 +39,7 @@ type APIContext struct {
 	ContextUser *user_model.User // the user which is being visited, in most cases it differs from Doer
 
 	Repo    *Repository
+	Comment *issues_model.Comment
 	Org     *APIOrganization
 	Package *Package
 }

routers/api/v1/api.go

@@ -71,6 +71,7 @@ import (
 	actions_model "code.gitea.io/gitea/models/actions"
 	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/organization"
 	"code.gitea.io/gitea/models/perm"
 	access_model "code.gitea.io/gitea/models/perm/access"
@@ -228,6 +229,39 @@ func repoAssignment() func(ctx *context.APIContext) {
 	}
 }
 
+// must be used within a group with a call to repoAssignment() to set ctx.Repo
+func commentAssignment(idParam string) func(ctx *context.APIContext) {
+	return func(ctx *context.APIContext) {
+		comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(idParam))
+		if err != nil {
+			if issues_model.IsErrCommentNotExist(err) {
+				ctx.NotFound(err)
+			} else {
+				ctx.InternalServerError(err)
+			}
+			return
+		}
+
+		if err = comment.LoadIssue(ctx); err != nil {
+			ctx.InternalServerError(err)
+			return
+		}
+		if comment.Issue == nil || comment.Issue.RepoID != ctx.Repo.Repository.ID {
+			ctx.NotFound()
+			return
+		}
+
+		if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
+			ctx.NotFound()
+			return
+		}
+
+		comment.Issue.Repo = ctx.Repo.Repository
+
+		ctx.Comment = comment
+	}
+}
+
 func reqPackageAccess(accessMode perm.AccessMode) func(ctx *context.APIContext) {
 	return func(ctx *context.APIContext) {
 		if ctx.Package.AccessMode < accessMode && !ctx.IsUserSiteAdmin() {
@@ -1322,7 +1356,7 @@ func Routes() *web.Route {
 									Patch(reqToken(), mustNotBeArchived, bind(api.EditAttachmentOptions{}), repo.EditIssueCommentAttachment).
 									Delete(reqToken(), mustNotBeArchived, repo.DeleteIssueCommentAttachment)
 							}, mustEnableAttachments)
-						})
+						}, commentAssignment(":id"))
 					})
 					m.Group("/{index}", func() {
 						m.Combo("").Get(repo.GetIssue).

routers/api/v1/repo/issue_comment.go

@@ -450,29 +450,7 @@ func GetIssueComment(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id"))
+	comment := ctx.Comment
-	if err != nil {
-		if issues_model.IsErrCommentNotExist(err) {
-			ctx.NotFound(err)
-		} else {
-			ctx.Error(http.StatusInternalServerError, "GetCommentByID", err)
-		}
-		return
-	}
-
-	if err = comment.LoadIssue(ctx); err != nil {
-		ctx.InternalServerError(err)
-		return
-	}
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.Status(http.StatusNotFound)
-		return
-	}
-
-	if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
-		ctx.NotFound()
-		return
-	}
 
 	if comment.Type != issues_model.CommentTypeComment {
 		ctx.Status(http.StatusNoContent)
@@ -583,25 +561,7 @@ func EditIssueCommentDeprecated(ctx *context.APIContext) {
 }
 
 func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id"))
+	comment := ctx.Comment
-	if err != nil {
-		if issues_model.IsErrCommentNotExist(err) {
-			ctx.NotFound(err)
-		} else {
-			ctx.Error(http.StatusInternalServerError, "GetCommentByID", err)
-		}
-		return
-	}
-
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.Status(http.StatusNotFound)
-		return
-	}
 
 	if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
 		ctx.Status(http.StatusForbidden)
@@ -613,7 +573,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
 		return
 	}
 
-	err = comment.LoadIssue(ctx)
+	err := comment.LoadIssue(ctx)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
 		return
@@ -707,25 +667,7 @@ func DeleteIssueCommentDeprecated(ctx *context.APIContext) {
 }
 
 func deleteIssueComment(ctx *context.APIContext) {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id"))
+	comment := ctx.Comment
-	if err != nil {
-		if issues_model.IsErrCommentNotExist(err) {
-			ctx.NotFound(err)
-		} else {
-			ctx.Error(http.StatusInternalServerError, "GetCommentByID", err)
-		}
-		return
-	}
-
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.Status(http.StatusNotFound)
-		return
-	}
 
 	if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
 		ctx.Status(http.StatusForbidden)
@@ -735,7 +677,7 @@ func deleteIssueComment(ctx *context.APIContext) {
 		return
 	}
 
-	if err = issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil {
+	if err := issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil {
 		ctx.Error(http.StatusInternalServerError, "DeleteCommentByID", err)
 		return
 	}

routers/api/v1/repo/issue_comment_attachment.go

@@ -55,11 +55,8 @@ func GetIssueCommentAttachment(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/error"
 
-	comment := getIssueCommentSafe(ctx)
+	comment := ctx.Comment
-	if comment == nil {
+	attachment := getIssueCommentAttachmentSafeRead(ctx)
-		return
-	}
-	attachment := getIssueCommentAttachmentSafeRead(ctx, comment)
 	if attachment == nil {
 		return
 	}
@@ -101,10 +98,7 @@ func ListIssueCommentAttachments(ctx *context.APIContext) {
 	//     "$ref": "#/responses/AttachmentList"
 	//   "404":
 	//     "$ref": "#/responses/error"
-	comment := getIssueCommentSafe(ctx)
+	comment := ctx.Comment
-	if comment == nil {
-		return
-	}
 
 	if err := comment.LoadAttachments(ctx); err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadAttachments", err)
@@ -166,14 +160,12 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/repoArchivedError"
 
 	// Check if comment exists and load comment
-	comment := getIssueCommentSafe(ctx)
+
-	if comment == nil {
+	if !canUserWriteIssueCommentAttachment(ctx) {
 		return
 	}
 
-	if !canUserWriteIssueCommentAttachment(ctx, comment) {
+	comment := ctx.Comment
-		return
-	}
 
 	updatedAt := ctx.Req.FormValue("updated_at")
 	if len(updatedAt) != 0 {
@@ -341,42 +333,17 @@ func DeleteIssueCommentAttachment(ctx *context.APIContext) {
 	ctx.Status(http.StatusNoContent)
 }
 
-func getIssueCommentSafe(ctx *context.APIContext) *issues_model.Comment {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64("id"))
-	if err != nil {
-		ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err)
-		return nil
-	}
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.Error(http.StatusInternalServerError, "comment.LoadIssue", err)
-		return nil
-	}
-	if comment.Issue == nil || comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.Error(http.StatusNotFound, "", "no matching issue comment found")
-		return nil
-	}
-
-	if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
-		return nil
-	}
-
-	comment.Issue.Repo = ctx.Repo.Repository
-
-	return comment
-}
-
 func getIssueCommentAttachmentSafeWrite(ctx *context.APIContext) *repo_model.Attachment {
-	comment := getIssueCommentSafe(ctx)
+	if !canUserWriteIssueCommentAttachment(ctx) {
-	if comment == nil {
 		return nil
 	}
-	if !canUserWriteIssueCommentAttachment(ctx, comment) {
+	return getIssueCommentAttachmentSafeRead(ctx)
-		return nil
-	}
-	return getIssueCommentAttachmentSafeRead(ctx, comment)
 }
 
-func canUserWriteIssueCommentAttachment(ctx *context.APIContext, comment *issues_model.Comment) bool {
+func canUserWriteIssueCommentAttachment(ctx *context.APIContext) bool {
+	// ctx.Comment is assumed to be set in a safe way via a middleware
+	comment := ctx.Comment
+
 	canEditComment := ctx.IsSigned && (ctx.Doer.ID == comment.PosterID || ctx.IsUserRepoAdmin() || ctx.IsUserSiteAdmin()) && ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)
 	if !canEditComment {
 		ctx.Error(http.StatusForbidden, "", "user should have permission to edit comment")
@@ -386,7 +353,10 @@ func canUserWriteIssueCommentAttachment(ctx *context.APIContext, comment *issues
 	return true
 }
 
-func getIssueCommentAttachmentSafeRead(ctx *context.APIContext, comment *issues_model.Comment) *repo_model.Attachment {
+func getIssueCommentAttachmentSafeRead(ctx *context.APIContext) *repo_model.Attachment {
+	// ctx.Comment is assumed to be set in a safe way via a middleware
+	comment := ctx.Comment
+
 	attachment, err := repo_model.GetAttachmentByID(ctx, ctx.ParamsInt64("attachment_id"))
 	if err != nil {
 		ctx.NotFoundOrServerError("GetAttachmentByID", repo_model.IsErrAttachmentNotExist, err)

routers/api/v1/repo/issue_reaction.go

@@ -49,30 +49,7 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id"))
+	comment := ctx.Comment
-	if err != nil {
-		if issues_model.IsErrCommentNotExist(err) {
-			ctx.NotFound(err)
-		} else {
-			ctx.Error(http.StatusInternalServerError, "GetCommentByID", err)
-		}
-		return
-	}
-
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.Error(http.StatusInternalServerError, "comment.LoadIssue", err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.NotFound()
-		return
-	}
-
-	if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
-		ctx.Error(http.StatusForbidden, "GetIssueCommentReactions", errors.New("no permission to get reactions"))
-		return
-	}
 
 	reactions, _, err := issues_model.FindCommentReactions(ctx, comment.IssueID, comment.ID)
 	if err != nil {
@@ -186,30 +163,7 @@ func DeleteIssueCommentReaction(ctx *context.APIContext) {
 }
 
 func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOption, isCreateType bool) {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id"))
+	comment := ctx.Comment
-	if err != nil {
-		if issues_model.IsErrCommentNotExist(err) {
-			ctx.NotFound(err)
-		} else {
-			ctx.Error(http.StatusInternalServerError, "GetCommentByID", err)
-		}
-		return
-	}
-
-	if err = comment.LoadIssue(ctx); err != nil {
-		ctx.Error(http.StatusInternalServerError, "comment.LoadIssue() failed", err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.NotFound()
-		return
-	}
-
-	if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
-		ctx.NotFound()
-		return
-	}
 
 	if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) {
 		ctx.Error(http.StatusForbidden, "ChangeIssueCommentReaction", errors.New("no permission to change reaction"))
@@ -241,7 +195,7 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp
 		})
 	} else {
 		// DeleteIssueCommentReaction part
-		err = issues_model.DeleteCommentReaction(ctx, ctx.Doer.ID, comment.Issue.ID, comment.ID, form.Reaction)
+		err := issues_model.DeleteCommentReaction(ctx, ctx.Doer.ID, comment.Issue.ID, comment.ID, form.Reaction)
 		if err != nil {
 			ctx.Error(http.StatusInternalServerError, "DeleteCommentReaction", err)
 			return