From b23c70d439cde90039fabb19683cd6cc7a5e1040 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sat, 7 Nov 2026 09:43:39 +0000 Subject: [PATCH] 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 Co-authored-by: delvh Co-authored-by: wxiaoguang --- models/issues/review.go | 221 ++++++++++--------------- models/issues/review_test.go | 15 ++ models/repo/attachment.go | 4 + routers/api/packages/container/blob.go | 6 +- routers/web/repo/attachment.go | 42 +---- routers/web/repo/githttp.go | 5 + routers/web/web.go | 4 +- services/release/release.go | 10 -- tests/integration/attachment_test.go | 17 +- 9 files changed, 134 insertions(+), 190 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index a32b0ef4f1d..c3e58015e05 100644 --- a/models/issues/review.go +++ b/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 }) diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 29aff876df1..dcf6dc2cf7d 100644 --- a/models/issues/review_test.go +++ b/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) +} diff --git a/models/repo/attachment.go b/models/repo/attachment.go index 09adcd4c9ea..c16e9c98866 100644 --- a/models/repo/attachment.go +++ b/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. diff --git a/routers/api/packages/container/blob.go b/routers/api/packages/container/blob.go index cb93e5c2f18..ceaecd2a621 100644 --- a/routers/api/packages/container/blob.go +++ b/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) } diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index c4b74c0832d..07d13a6d36b 100644 --- a/routers/web/repo/attachment.go +++ b/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) diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go index 5042e8f8516..04cde5c474c 100644 --- a/routers/web/repo/githttp.go +++ b/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") diff --git a/routers/web/web.go b/routers/web/web.go index caedfc55658..d6edfe1dbf9 100644 --- a/routers/web/web.go +++ b/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 diff --git a/services/release/release.go b/services/release/release.go index 628da6d9f76..e87a99f7496 100644 --- a/services/release/release.go +++ b/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) diff --git a/tests/integration/attachment_test.go b/tests/integration/attachment_test.go index ac8d653e198..3f61029163b 100644 --- a/tests/integration/attachment_test.go +++ b/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) }