fix: add missing checks to several handlers and services (#40120)

Several code paths skipped checks their siblings perform, or cleaned up
more than they validated.

- Official team review requests now count, and removing one no longer
marks an unrelated review official
- Release edits only delete files of validated attachments
- Rejected container uploads no longer delete an existing blob
- Attachment removal only removes the uploader's own pending uploads
- OpenID sign-in checks the allow and block lists against the normalized
identifier
- HTTP git access is rejected while a repository is being created, like
SSH

---------

Signed-off-by: silverwind <me@silverwind.io>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/issues/review.go

@@ -289,23 +289,12 @@ func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewer *user_model.
 	}
 	if rule == nil {
 		// if no rule is found, then user with write access can make official reviews
-		err := pr.LoadBaseRepo(ctx)
-		if err != nil {
+		if err := pr.LoadBaseRepo(ctx); err != nil {
 			return false, err
 		}
-		writeAccess, err := access_model.HasAccessUnit(ctx, reviewer, pr.BaseRepo, unit.TypeCode, perm.AccessModeWrite)
-		if err != nil {
-			return false, err
-		}
-		return writeAccess, nil
+		return access_model.HasAccessUnit(ctx, reviewer, pr.BaseRepo, unit.TypeCode, perm.AccessModeWrite)
 	}
-
-	official, err := git_model.IsUserOfficialReviewer(ctx, rule, reviewer)
-	if official || err != nil {
-		return official, err
-	}
-
-	return false, nil
+	return git_model.IsUserOfficialReviewer(ctx, rule, reviewer)
 }
 
 // IsOfficialReviewerTeam check if reviewer in this team can make official reviews in issue (counts towards required approvals)
@@ -317,66 +306,82 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio
 	if err != nil {
 		return false, err
 	}
-	if pb == nil {
-		return false, nil
+	if pb != nil && pb.EnableApprovalsWhitelist {
+		return slices.Contains(pb.ApprovalsWhitelistTeamIDs, team.ID), nil
 	}
-
-	if !pb.EnableApprovalsWhitelist {
-		return team.UnitAccessMode(ctx, unit.TypeCode) >= perm.AccessModeWrite, nil
-	}
-
-	return slices.Contains(pb.ApprovalsWhitelistTeamIDs, team.ID), nil
+	return team.UnitAccessMode(ctx, unit.TypeCode) >= perm.AccessModeWrite, nil
 }
 
-// RecalculateReviewsOfficial re-evaluates the "official" flag of the latest approve
-// and reject reviews of an issue against its pull request's current base branch.
-// It must be called whenever the target branch changes, otherwise an approval that
-// was official on the previous (possibly unprotected) branch would keep satisfying
-// the new branch's protection rules.
-func RecalculateReviewsOfficial(ctx context.Context, issue *Issue) error {
-	if err := issue.LoadPullRequest(ctx); err != nil {
+// updateReviewerOfficial keeps the official flag only on the reviewer's latest approval, rejection or review request, and only if the reviewer is official
+func updateReviewerOfficial(ctx context.Context, issueID, reviewerID int64, official bool) error {
+	if _, err := db.GetEngine(ctx).Where(builder.Eq{"issue_id": issueID, "reviewer_id": reviewerID}).
+		NoAutoTime().Cols("official").Update(&Review{}); err != nil {
 		return err
 	}
+	if !official {
+		return nil
+	}
+	latest, err := GetReviewByIssueIDAndUserID(ctx, issueID, reviewerID)
+	if IsErrReviewNotExist(err) {
+		return nil
+	} else if err != nil {
+		return err
+	}
+	_, err = db.GetEngine(ctx).ID(latest.ID).NoAutoTime().Cols("official").Update(&Review{Official: true})
+	return err
+}
 
-	// Clearing and restoring the official flags must happen atomically, otherwise a
-	// failure in between would leave the reviews without any official flag set.
+// RecalculateReviewsOfficial re-evaluates the "official" flag of an issue's reviews and review requests
+// against its pull request's current base branch. It must be called whenever the target branch changes,
+// otherwise an approval that was official on the previous (possibly unprotected) branch would keep
+// satisfying the new branch's protection rules.
+func RecalculateReviewsOfficial(ctx context.Context, issue *Issue) error {
 	return db.WithTx(ctx, func(ctx context.Context) error {
-		// Only the latest approve/reject review of each reviewer counts as official, so
-		// clear the flag on all of them first and restore it only where it still applies.
-		if _, err := db.GetEngine(ctx).
-			Where("issue_id = ?", issue.ID).
-			In("type", ReviewTypeApprove, ReviewTypeReject).
-			Cols("official").
-			Update(&Review{Official: false}); err != nil {
+		if _, err := db.GetEngine(ctx).Where("issue_id = ?", issue.ID).NoAutoTime().Cols("official").Update(&Review{}); err != nil {
 			return err
 		}
 
-		reviews, err := FindLatestReviews(ctx, FindReviewOptions{
-			Types:   []ReviewType{ReviewTypeApprove, ReviewTypeReject},
-			IssueID: issue.ID,
-		})
-		if err != nil {
+		reviewerIDs := make([]int64, 0, 5)
+		if err := db.GetEngine(ctx).Table("review").Cols("reviewer_id").
+			Where(builder.Eq{"issue_id": issue.ID, "original_author_id": 0}).
+			And(builder.In("type", ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest)).
+			And(builder.Neq{"reviewer_id": 0}).
+			Distinct("reviewer_id").Find(&reviewerIDs); err != nil {
 			return err
 		}
-
-		for _, review := range reviews {
-			if err := review.LoadReviewer(ctx); err != nil {
-				return err
-			}
-			if review.Reviewer == nil {
-				continue
-			}
-			official, err := IsOfficialReviewer(ctx, issue, review.Reviewer)
+		for _, reviewerID := range reviewerIDs {
+			_, reviewer, err := user_model.GetPossibleUserByID(ctx, reviewerID)
 			if err != nil {
 				return err
 			}
-			if official {
-				if _, err := db.GetEngine(ctx).ID(review.ID).Cols("official").Update(&Review{Official: true}); err != nil {
-					return err
-				}
+			official, err := IsOfficialReviewer(ctx, issue, reviewer)
+			if err != nil {
+				return err
+			}
+			if err := updateReviewerOfficial(ctx, issue.ID, reviewerID, official); err != nil {
+				return err
 			}
 		}
 
+		teamRequests := make([]*Review, 0, 5)
+		if err := db.GetEngine(ctx).Where(builder.Eq{"issue_id": issue.ID, "type": ReviewTypeRequest}).
+			And(builder.Gt{"reviewer_team_id": 0}).Find(&teamRequests); err != nil {
+			return err
+		}
+		for _, request := range teamRequests {
+			if err := request.LoadReviewerTeam(ctx); organization.IsErrTeamNotExist(err) {
+				continue
+			} else if err != nil {
+				return err
+			}
+			official, err := IsOfficialReviewerTeam(ctx, issue, request.ReviewerTeam)
+			if err != nil {
+				return err
+			}
+			if _, err := db.GetEngine(ctx).ID(request.ID).NoAutoTime().Cols("official").Update(&Review{Official: official}); err != nil {
+				return err
+			}
+		}
 		return nil
 	})
 }
@@ -476,7 +481,13 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 	defer committer.Close()
 	sess := db.GetEngine(ctx)
 
+	isApproveOrReject := reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject
 	official := false
+	if isApproveOrReject {
+		if official, err = IsOfficialReviewer(ctx, issue, doer); err != nil {
+			return nil, nil, err
+		}
+	}
 
 	review, err := GetCurrentReview(ctx, doer, issue)
 	if err != nil {
@@ -488,16 +499,6 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 			return nil, nil, ContentEmptyErr{}
 		}
 
-		if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject {
-			// Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared
-			if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
-				return nil, nil, err
-			}
-			if official, err = IsOfficialReviewer(ctx, issue, doer); err != nil {
-				return nil, nil, err
-			}
-		}
-
 		// No current review. Create a new one!
 		if review, err = CreateReview(ctx, CreateReviewOptions{
 			Type:     reviewType,
@@ -518,16 +519,6 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 			return nil, nil, ContentEmptyErr{}
 		}
 
-		if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject {
-			// Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared
-			if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
-				return nil, nil, err
-			}
-			if official, err = IsOfficialReviewer(ctx, issue, doer); err != nil {
-				return nil, nil, err
-			}
-		}
-
 		review.Official = official
 		review.Issue = issue
 		review.Content = content
@@ -548,6 +539,12 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 		}
 	}
 
+	if isApproveOrReject {
+		if err := updateReviewerOfficial(ctx, issue.ID, doer.ID, official); err != nil {
+			return nil, nil, err
+		}
+	}
+
 	comm, err := CreateComment(ctx, &CreateCommentOptions{
 		Type:        CommentTypeReview,
 		Doer:        doer,
@@ -562,7 +559,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 	}
 
 	// try to remove team review request if need
-	if issue.Repo.Owner.IsOrganization() && (reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject) {
+	if issue.Repo.Owner.IsOrganization() && isApproveOrReject {
 		teamReviewRequests := make([]*Review, 0, 10)
 		if err := sess.SQL("SELECT * FROM review WHERE issue_id = ? AND reviewer_team_id > 0 AND type = ?", issue.ID, ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
 			return nil, nil, err
@@ -700,8 +697,6 @@ func InsertReviews(ctx context.Context, reviews []*Review) error {
 // AddReviewRequest add a review request from one reviewer
 func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User, isCodeOwners bool) (*Comment, error) {
 	return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
-		sess := db.GetEngine(ctx)
-
 		review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID)
 		if err != nil && !IsErrReviewNotExist(err) {
 			return nil, err
@@ -727,15 +722,9 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 			}
 		}
 
-		// if the reviewer is an official reviewer,
-		// remove the official flag in the all previous reviews
 		official, err := IsOfficialReviewer(ctx, issue, reviewer)
 		if err != nil {
 			return nil, err
-		} else if official {
-			if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, reviewer.ID); err != nil {
-				return nil, err
-			}
 		}
 
 		review, err = CreateReview(ctx, CreateReviewOptions{
@@ -749,6 +738,10 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 			return nil, err
 		}
 
+		if err := updateReviewerOfficial(ctx, issue.ID, reviewer.ID, official); err != nil {
+			return nil, err
+		}
+
 		comment, err := CreateComment(ctx, &CreateCommentOptions{
 			Type:            CommentTypeReviewRequest,
 			Doer:            doer,
@@ -789,10 +782,9 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user
 		official, err := IsOfficialReviewer(ctx, issue, reviewer)
 		if err != nil {
 			return nil, err
-		} else if official {
-			if err := restoreLatestOfficialReview(ctx, issue.ID, reviewer.ID); err != nil {
-				return nil, err
-			}
+		}
+		if err := updateReviewerOfficial(ctx, issue.ID, reviewer.ID, official); err != nil {
+			return nil, err
 		}
 
 		return CreateComment(ctx, &CreateCommentOptions{
@@ -806,22 +798,6 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user
 	})
 }
 
-// Recalculate the latest official review for reviewer
-func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64) error {
-	review, err := GetReviewByIssueIDAndUserID(ctx, issueID, reviewerID)
-	if err != nil && !IsErrReviewNotExist(err) {
-		return err
-	}
-
-	if review != nil {
-		if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE id=?", true, review.ID); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
 // AddTeamReviewRequest add a review request from one team
 func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User, isCodeOwners bool) (*Comment, error) {
 	return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
@@ -838,10 +814,6 @@ func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organizat
 		official, err := IsOfficialReviewerTeam(ctx, issue, reviewer)
 		if err != nil {
 			return nil, fmt.Errorf("isOfficialReviewerTeam(): %w", err)
-		} else if !official {
-			if official, err = IsOfficialReviewer(ctx, issue, doer); err != nil {
-				return nil, fmt.Errorf("isOfficialReviewer(): %w", err)
-			}
 		}
 
 		if review, err = CreateReview(ctx, CreateReviewOptions{
@@ -854,12 +826,6 @@ func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organizat
 			return nil, err
 		}
 
-		if official {
-			if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id=?", false, issue.ID, reviewer.ID); err != nil {
-				return nil, err
-			}
-		}
-
 		comment, err := CreateComment(ctx, &CreateCommentOptions{
 			Type:            CommentTypeReviewRequest,
 			Doer:            doer,
@@ -894,25 +860,6 @@ func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organi
 			return nil, err
 		}
 
-		official, err := IsOfficialReviewerTeam(ctx, issue, reviewer)
-		if err != nil {
-			return nil, fmt.Errorf("isOfficialReviewerTeam(): %w", err)
-		}
-
-		if official {
-			// recalculate which is the latest official review from that team
-			review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, -reviewer.ID)
-			if err != nil && !IsErrReviewNotExist(err) {
-				return nil, err
-			}
-
-			if review != nil {
-				if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE id=?", true, review.ID); err != nil {
-					return nil, err
-				}
-			}
-		}
-
 		if doer == nil {
 			return nil, nil
 		}
@@ -1019,10 +966,8 @@ func DeleteReview(ctx context.Context, r *Review) error {
 			return err
 		}
 
-		if r.Official {
-			if err := restoreLatestOfficialReview(ctx, r.IssueID, r.ReviewerID); err != nil {
-				return err
-			}
+		if r.Official && r.OriginalAuthorID == 0 {
+			return updateReviewerOfficial(ctx, r.IssueID, r.ReviewerID, true)
 		}
 		return nil
 	})

models/issues/review_test.go

@@ -9,6 +9,7 @@ import (
 	"gitea.dev/models/db"
 	git_model "gitea.dev/models/git"
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/organization"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
@@ -434,3 +435,17 @@ func TestRecalculateReviewsOfficial(t *testing.T) {
 	review = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: review.ID})
 	assert.True(t, review.Official)
 }
+
+func TestTeamReviewRequestOfficial(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 12})
+	assert.NoError(t, issue.LoadRepo(t.Context()))
+	comment, err := issues_model.AddTeamReviewRequest(t.Context(), issue, unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2}), &user_model.User{ID: 2}, false)
+	assert.NoError(t, err)
+	assert.True(t, unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}).Official)
+
+	assert.NoError(t, db.Insert(t.Context(), &git_model.ProtectedBranch{RepoID: issue.RepoID, RuleName: "master", EnableApprovalsWhitelist: true}))
+	assert.NoError(t, issues_model.RecalculateReviewsOfficial(t.Context(), issue))
+	assert.False(t, unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}).Official)
+}

models/repo/attachment.go

@@ -42,6 +42,10 @@ func init() {
 	db.RegisterModel(new(Attachment))
 }
 
+func (a *Attachment) IsLinked() bool {
+	return a.IssueID > 0 || a.ReleaseID > 0 || a.CommentID > 0
+}
+
 // IncreaseDownloadCount is update download count + 1
 func (a *Attachment) IncreaseDownloadCount(ctx context.Context) error {
 	// Update download count.

routers/api/packages/container/blob.go

@@ -36,7 +36,7 @@ func saveAsPackageBlob(ctx context.Context, hsr packages_module.HashedSizeReader
 }
 
 func saveAsPackageBlobInternal(ctx context.Context, hsr packages_module.HashedSizeReader, pci *packages_service.PackageCreationInfo, pb *packages_model.PackageBlob) (*packages_model.PackageBlob, error) {
-	exists := false
+	blobCreated := false
 
 	contentStore := packages_module.NewContentStore()
 
@@ -49,14 +49,16 @@ func saveAsPackageBlobInternal(ctx context.Context, hsr packages_module.HashedSi
 		if err := packages_service.CheckSizeQuotaExceeded(ctx, pci.Creator, pci.Owner, packages_model.TypeContainer, hsr.Size()); err != nil {
 			return err
 		}
+		var exists bool
 		pb, exists, err = packages_service.GetOrSavePackageBlob(ctx, contentStore, pb, hsr)
 		if err != nil {
 			return err
 		}
+		blobCreated = !exists
 		return createFileForBlob(ctx, uploadVersion, pb)
 	})
 	if err != nil {
-		if !exists && pb != nil { // pb can be nil if GetOrInsertBlob failed
+		if blobCreated {
 			if err := contentStore.Delete(packages_module.BlobHash256Key(pb.HashSHA256)); err != nil {
 				log.Error("Error deleting package blob from content store: %v", err)
 			}

routers/web/repo/attachment.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 
 	auth_model "gitea.dev/models/auth"
-	issues_model "gitea.dev/models/issues"
 	access_model "gitea.dev/models/perm/access"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unit"
@@ -78,49 +77,24 @@ func uploadAttachment(ctx *context.Context, repoID int64, uploadFunc attachment.
 	})
 }
 
-// DeleteAttachment response for deleting issue's attachment
-func DeleteAttachment(ctx *context.Context) {
+func DeletePendingAttachment(ctx *context.Context) {
 	file := ctx.FormString("file")
 	attach, err := repo_model.GetAttachmentByUUID(ctx, file)
 	if err != nil {
-		ctx.HTTPError(http.StatusBadRequest, err.Error())
+		ctx.ServerError("GetAttachmentByUUID", err)
 		return
 	}
 
-	if !ctx.IsSigned {
+	// only uploader can delete their own pending (unlinked) attachment,
+	// linked attachments are managed by the linked entity (issue, release, etc.) and cannot be deleted directly
+	canDelete := attach.RepoID == ctx.Repo.Repository.ID &&
+		attach.UploaderID == ctx.Doer.ID &&
+		!attach.IsLinked()
+	if !canDelete {
 		ctx.HTTPError(http.StatusForbidden)
 		return
 	}
 
-	if attach.RepoID != ctx.Repo.Repository.ID {
-		ctx.HTTPError(http.StatusBadRequest, "attachment does not belong to this repository")
-		return
-	}
-
-	if ctx.Doer.ID != attach.UploaderID {
-		if attach.IssueID > 0 {
-			issue, err := issues_model.GetIssueByID(ctx, attach.IssueID)
-			if err != nil {
-				ctx.ServerError("GetIssueByID", err)
-				return
-			}
-			if !ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull) {
-				ctx.HTTPError(http.StatusForbidden)
-				return
-			}
-		} else if attach.ReleaseID > 0 {
-			if !ctx.Repo.Permission.CanWrite(unit.TypeReleases) {
-				ctx.HTTPError(http.StatusForbidden)
-				return
-			}
-		} else {
-			if !ctx.Repo.Permission.IsAdmin() && !ctx.Repo.Permission.IsOwner() {
-				ctx.HTTPError(http.StatusForbidden)
-				return
-			}
-		}
-	}
-
 	err = repo_model.DeleteAttachment(ctx, attach, true)
 	if err != nil {
 		ctx.ServerError("DeleteAttachment", err)

routers/web/repo/githttp.go

@@ -206,6 +206,11 @@ func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler {
 		}
 	}
 
+	if repoExist && repo.IsBeingCreated() {
+		ctx.PlainText(http.StatusForbidden, "Repository is still being created, please retry later")
+		return nil
+	}
+
 	if !repoExist {
 		if !receivePack {
 			ctx.PlainText(http.StatusNotFound, "Repository not found")

routers/web/web.go

@@ -1366,7 +1366,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 			})
 
 			m.Post("/attachments", repo.UploadIssueAttachment)
-			m.Post("/attachments/remove", repo.DeleteAttachment)
+			m.Post("/attachments/remove", repo.DeletePendingAttachment)
 
 			m.Post("/labels", repo.UpdateIssueLabel)
 			m.Post("/milestone", repo.UpdateIssueMilestone)
@@ -1495,7 +1495,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 			m.Post("/generate-notes", web.Bind[*forms.GenerateReleaseNotesForm](), repo.GenerateReleaseNotes)
 			m.Post("/delete", repo.DeleteRelease)
 			m.Post("/attachments", repo.UploadReleaseAttachment)
-			m.Post("/attachments/remove", repo.DeleteAttachment)
+			m.Post("/attachments/remove", repo.DeletePendingAttachment)
 		}, reqSignIn, context.RepoMustNotBeArchived(), reqRepoReleaseWriter)
 	}, optSignIn, context.RepoAssignment, repo.MustBeNotEmpty, reqRepoReleaseReader)
 	// end "/{username}/{reponame}": repo releases

services/release/release.go

@@ -383,16 +383,6 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo
 		return err
 	}
 
-	for _, uuid := range delAttachmentUUIDs {
-		if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(uuid)); err != nil {
-			// Even delete files failed, but the attachments has been removed from database, so we
-			// should not return error but only record the error on logs.
-			// users have to delete this attachments manually or we should have a
-			// synchronize between database attachment table and attachment storage
-			log.Error("delete attachment[uuid: %s] failed: %v", uuid, err)
-		}
-	}
-
 	if !rel.IsDraft {
 		if !isTagCreated && !isConvertedFromTag {
 			notify_service.UpdateRelease(ctx, doer, rel)

tests/integration/attachment_test.go

@@ -66,11 +66,13 @@ func testCreateAttachment(t *testing.T, session *TestSession, repoURL, issueOrRe
 }
 
 func testDeleteIssueAttachment(t *testing.T, session *TestSession, repoURL, uuid string, expectedStatus int) {
+	t.Helper()
 	req := NewRequestWithValues(t, "POST", repoURL+"/issues/attachments/remove", map[string]string{"file": uuid})
 	session.MakeRequest(t, req, expectedStatus)
 }
 
 func testDeleteReleaseAttachment(t *testing.T, session *TestSession, repoURL, uuid string, expectedStatus int) {
+	t.Helper()
 	req := NewRequestWithValues(t, "POST", repoURL+"/releases/attachments/remove", map[string]string{"file": uuid})
 	session.MakeRequest(t, req, expectedStatus)
 }
@@ -294,17 +296,24 @@ func testDeleteAttachmentPermissions(t *testing.T) {
 	testDeleteIssueAttachment(t, readonlySession, repoURL, issueFromOwner, http.StatusForbidden)
 
 	issueFromReader := testCreateIssueAttachment(t, readonlySession, repoURL, "reader-issue.png", testGeneratePngBytes(), http.StatusOK)
-	testDeleteIssueAttachment(t, ownerSession, repoURL, issueFromReader, http.StatusOK)
+	testDeleteIssueAttachment(t, ownerSession, repoURL, issueFromReader, http.StatusForbidden)
+	testDeleteIssueAttachment(t, readonlySession, repoURL, issueFromReader, http.StatusOK)
 
 	testCreateReleaseAttachment(t, readonlySession, repoURL, "reader-release.png", testGeneratePngBytes(), http.StatusNotFound)
 
 	crossRepoUUID := testCreateIssueAttachment(t, ownerSession, repoURL, "cross-repo.png", testGeneratePngBytes(), http.StatusOK)
-	testDeleteIssueAttachment(t, ownerSession, "/user2/repo2", crossRepoUUID, http.StatusBadRequest)
+	testDeleteIssueAttachment(t, ownerSession, "/user2/repo2", crossRepoUUID, http.StatusForbidden)
+	testDeleteReleaseAttachment(t, ownerSession, "/user2/repo2", crossRepoUUID, http.StatusForbidden)
 	testDeleteIssueAttachment(t, ownerSession, repoURL, crossRepoUUID, http.StatusOK)
+	testDeleteIssueAttachment(t, ownerSession, repoURL, crossRepoUUID, http.StatusNotFound)
 
 	releaseUUID := testCreateReleaseAttachment(t, ownerSession, repoURL, "reader-release.png", testGeneratePngBytes(), http.StatusOK)
 	testDeleteReleaseAttachment(t, ownerSession, repoURL, releaseUUID, http.StatusOK)
 
-	// test deleting release attachment from another repo
-	testDeleteReleaseAttachment(t, ownerSession, "/user2/repo2", crossRepoUUID, http.StatusBadRequest)
+	linkedUUID := testCreateIssueAttachment(t, ownerSession, repoURL, "linked.png", testGeneratePngBytes(), http.StatusOK)
+	linked, err := repo_model.GetAttachmentByUUID(t.Context(), linkedUUID)
+	require.NoError(t, err)
+	linked.CommentID = 1
+	require.NoError(t, repo_model.UpdateAttachmentByUUID(t.Context(), linked, "comment_id"))
+	testDeleteIssueAttachment(t, ownerSession, repoURL, linkedUUID, http.StatusForbidden)
 }