diff --git a/models/issues/comment.go b/models/issues/comment.go index c0a13ef03d..325fcbe30b 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -611,9 +611,13 @@ func (c *Comment) UpdateAttachments(ctx context.Context, uuids []string) error { } defer committer.Close() - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) + if err := c.LoadIssue(ctx); err != nil { + return fmt.Errorf("LoadIssue: %w", err) + } + + attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, c.Issue.RepoID, uuids, repo_model.FindAttachmentOptions{}) if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", uuids, c.Issue.RepoID, err) } for i := 0; i < len(attachments); i++ { attachments[i].IssueID = c.IssueID @@ -889,7 +893,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment // Check comment type. switch opts.Type { case CommentTypeCode: - if err = updateAttachments(ctx, opts, comment); err != nil { + if err := comment.UpdateAttachments(ctx, opts.Attachments); err != nil { return err } if comment.ReviewID != 0 { @@ -909,7 +913,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment } fallthrough case CommentTypeReview: - if err = updateAttachments(ctx, opts, comment); err != nil { + if err := comment.UpdateAttachments(ctx, opts.Attachments); err != nil { return err } case CommentTypeReopen, CommentTypeClose: @@ -921,23 +925,6 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment return UpdateIssueCols(ctx, opts.Issue, "updated_unix") } -func updateAttachments(ctx context.Context, opts *CreateCommentOptions, comment *Comment) error { - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err) - } - for i := range attachments { - attachments[i].IssueID = opts.Issue.ID - attachments[i].CommentID = comment.ID - // No assign value could be 0, so ignore AllCols(). - if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil { - return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err) - } - } - comment.Attachments = attachments - return nil -} - func createDeadlineComment(ctx context.Context, doer *user_model.User, issue *Issue, newDeadlineUnix timeutil.TimeStamp) (*Comment, error) { var content string var commentType CommentType diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 84a8820cd1..22e6fcb8d4 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -234,18 +234,18 @@ func AddDeletePRBranchComment(ctx context.Context, doer *user_model.User, repo * } // UpdateIssueAttachments update attachments by UUIDs for the issue -func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string) (err error) { +func UpdateIssueAttachments(ctx context.Context, issue *Issue, uuids []string) (err error) { ctx, committer, err := db.TxContext(ctx) if err != nil { return err } defer committer.Close() - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) + attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, issue.RepoID, uuids, repo_model.FindAttachmentOptions{}) if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", uuids, issue.RepoID, err) } for i := 0; i < len(attachments); i++ { - attachments[i].IssueID = issueID + attachments[i].IssueID = issue.ID if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil { return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) } @@ -394,18 +394,8 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue return err } - if len(opts.Attachments) > 0 { - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err) - } - - for i := 0; i < len(attachments); i++ { - attachments[i].IssueID = opts.Issue.ID - if _, err = e.ID(attachments[i].ID).Update(attachments[i]); err != nil { - return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) - } - } + if err := UpdateIssueAttachments(ctx, opts.Issue, opts.Attachments); err != nil { + return fmt.Errorf("UpdateIssueAttachments: %w", err) } if err = opts.Issue.LoadAttributes(ctx); err != nil { return err diff --git a/models/repo/attachment.go b/models/repo/attachment.go index 6d903be5f8..1b6af572dd 100644 --- a/models/repo/attachment.go +++ b/models/repo/attachment.go @@ -1,4 +1,5 @@ // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2026 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package repo @@ -16,17 +17,30 @@ import ( "forgejo.org/modules/timeutil" "forgejo.org/modules/util" "forgejo.org/modules/validation" + + "xorm.io/builder" ) // Attachment represent a attachment of issue/comment/release. type Attachment struct { - ID int64 `xorm:"pk autoincr"` - UUID string `xorm:"uuid UNIQUE"` - RepoID int64 `xorm:"INDEX"` // this should not be zero - IssueID int64 `xorm:"INDEX"` // maybe zero when creating - ReleaseID int64 `xorm:"INDEX"` // maybe zero when creating - UploaderID int64 `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added - CommentID int64 `xorm:"INDEX"` + ID int64 `xorm:"pk autoincr"` + // UUID is the public identifier of the attachment, and is used during HTTP + // requests to refer to a specific attachment. + UUID string `xorm:"uuid UNIQUE"` + // UploaderID is always set and non-zero and refers to the user that has + // uploaded this attachment. + UploaderID int64 `xorm:"INDEX DEFAULT 0"` + // RepoID is always set and non-zero and refers to the repository where this + // attachment was uploaded to. + RepoID int64 `xorm:"INDEX"` + // IssueID, ReleaseID and CommentID have multiple possible states: + // - ReleaseID != 0 && IssueID == 0 && CommentID == 0: attached to release with id `ReleaseID`. + // - ReleaseID == 0 && IssueID != 0 && CommentID == 0: attached to the issue with id `IssueID`. + // - ReleaseID == 0 && IssueID != 0 && CommentID != 0: attached to comment with id `CommentID` that is in issue with id `IssueID`. + // All other states should be considered invalid. + IssueID int64 `xorm:"INDEX"` + ReleaseID int64 `xorm:"INDEX"` + CommentID int64 `xorm:"INDEX"` Name string DownloadCount int64 `xorm:"DEFAULT 0"` Size int64 `xorm:"DEFAULT 0"` @@ -73,6 +87,12 @@ func (a *Attachment) DownloadURL() string { return setting.AppURL + "attachments/" + url.PathEscape(a.UUID) } +// IsAttachedToResource returns true if this attachment is attached to a release, +// issue or comment. +func (a *Attachment) IsAttachedToResource() bool { + return a.ReleaseID != 0 || a.IssueID != 0 || a.CommentID != 0 +} + // ErrAttachmentNotExist represents a "AttachmentNotExist" kind of error. type ErrAttachmentNotExist struct { ID int64 @@ -133,15 +153,37 @@ func GetAttachmentByUUID(ctx context.Context, uuid string) (*Attachment, error) return attach, nil } -// GetAttachmentsByUUIDs returns attachment by given UUID list. -func GetAttachmentsByUUIDs(ctx context.Context, uuids []string) ([]*Attachment, error) { +type FindAttachmentOptions struct { + ReleaseID int64 + IssueID int64 + CommentID int64 +} + +func (opts FindAttachmentOptions) ToConds() builder.Cond { + return builder.Eq{"release_id": opts.ReleaseID, "issue_id": opts.IssueID, "comment_id": opts.CommentID} +} + +// FindRepoAttachmentsByUUID always returns attachment that has a UUID that is +// in the given `uuids` argument and is attached to the repository. +// +// The values in `opts` are always as a condition even if they are zero, this +// allows to search for attachments that are not yet attached to any resource by +// specifying a empty struct. +func FindRepoAttachmentsByUUID(ctx context.Context, repoID int64, uuids []string, opts FindAttachmentOptions) ([]*Attachment, error) { + // Nothing to match anyway. if len(uuids) == 0 { return []*Attachment{}, nil } - // Silently drop invalid uuids. + // At maximum nothing is filtered and we get all attachments via the UUID. attachments := make([]*Attachment, 0, len(uuids)) - return attachments, db.GetEngine(ctx).In("uuid", uuids).Find(&attachments) + + err := db.GetEngine(ctx). + Where("repo_id = ?", repoID). + In("uuid", uuids). + And(opts.ToConds()). + Find(&attachments) + return attachments, err } // ExistAttachmentsByUUID returns true if attachment exists with the given UUID diff --git a/models/repo/release.go b/models/repo/release.go index 4efbfb352d..edd628fa0f 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -224,18 +224,14 @@ func UpdateRelease(ctx context.Context, rel *Release) error { } // AddReleaseAttachments adds a release attachments -func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs []string) (err error) { - // Check attachments - attachments, err := GetAttachmentsByUUIDs(ctx, attachmentUUIDs) +func AddReleaseAttachments(ctx context.Context, release *Release, attachmentUUIDs []string) (err error) { + attachments, err := FindRepoAttachmentsByUUID(ctx, release.RepoID, attachmentUUIDs, FindAttachmentOptions{}) if err != nil { - return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", attachmentUUIDs, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", attachmentUUIDs, release.RepoID, err) } for i := range attachments { - if attachments[i].ReleaseID != 0 { - return util.NewPermissionDeniedErrorf("release permission denied") - } - attachments[i].ReleaseID = releaseID + attachments[i].ReleaseID = release.ID // No assign value could be 0, so ignore AllCols(). if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil { return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 10d3e6651b..cc09651e97 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3682,7 +3682,7 @@ func updateAttachments(ctx *context.Context, item any, files []string) error { if len(files) > 0 { switch content := item.(type) { case *issues_model.Issue: - err = issues_model.UpdateIssueAttachments(ctx, content.ID, files) + err = issues_model.UpdateIssueAttachments(ctx, content, files) case *issues_model.Comment: err = content.UpdateAttachments(ctx, files) default: diff --git a/services/attachment/attachment.go b/services/attachment/attachment.go index b6f763842b..420e95d1eb 100644 --- a/services/attachment/attachment.go +++ b/services/attachment/attachment.go @@ -24,6 +24,9 @@ func NewAttachment(ctx context.Context, attach *repo_model.Attachment, file io.R if attach.RepoID == 0 { return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name) } + if attach.UploaderID == 0 { + return nil, fmt.Errorf("attachment %s should have a uploader", attach.Name) + } err := db.WithTx(ctx, func(ctx context.Context) error { attach.UUID = uuid.New().String() @@ -48,6 +51,9 @@ func NewExternalAttachment(ctx context.Context, attach *repo_model.Attachment) ( if attach.RepoID == 0 { return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name) } + if attach.UploaderID == 0 { + return nil, fmt.Errorf("attachment %s should have a uploader", attach.Name) + } if attach.ExternalURL == "" { return nil, fmt.Errorf("attachment %s should have a external url", attach.Name) } diff --git a/services/release/release.go b/services/release/release.go index 69889a1638..9536ebf6dd 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -192,7 +192,7 @@ func CreateRelease(gitRepo *git.Repository, rel *repo_model.Release, msg string, } } - if err = repo_model.AddReleaseAttachments(gitRepo.Ctx, rel.ID, addAttachmentUUIDs.Values()); err != nil { + if err = repo_model.AddReleaseAttachments(gitRepo.Ctx, rel, addAttachmentUUIDs.Values()); err != nil { return err } @@ -314,44 +314,37 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo } } - if err = repo_model.AddReleaseAttachments(ctx, rel.ID, addAttachmentUUIDs.Values()); err != nil { + if err = repo_model.AddReleaseAttachments(ctx, rel, addAttachmentUUIDs.Values()); err != nil { return fmt.Errorf("AddReleaseAttachments: %w", err) } deletedUUIDs := make(container.Set[string]) if len(delAttachmentUUIDs) > 0 { - // Check attachments - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, delAttachmentUUIDs.Values()) + // Check delAttachments + delAttachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, rel.RepoID, delAttachmentUUIDs.Values(), repo_model.FindAttachmentOptions{ReleaseID: rel.ID}) if err != nil { - return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", delAttachmentUUIDs, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", delAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err) } - for _, attach := range attachments { - if attach.ReleaseID != rel.ID { - return util.SilentWrap{ - Message: "delete attachment of release permission denied", - Err: util.ErrPermissionDenied, - } - } + for _, attach := range delAttachments { deletedUUIDs.Add(attach.UUID) } - if _, err := repo_model.DeleteAttachments(ctx, attachments, true); err != nil { + if _, err := repo_model.DeleteAttachments(ctx, delAttachments, true); err != nil { return fmt.Errorf("DeleteAttachments [uuids: %v]: %w", delAttachmentUUIDs, err) } } if len(updateAttachmentUUIDs) > 0 { - // Check attachments - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, updateAttachmentUUIDs.Values()) + // Check that attachments actually belong to repository and release. + attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, rel.RepoID, updateAttachmentUUIDs.Values(), repo_model.FindAttachmentOptions{ReleaseID: rel.ID}) if err != nil { - return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", updateAttachmentUUIDs, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", updateAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err) } - for _, attach := range attachments { - if attach.ReleaseID != rel.ID { - return util.SilentWrap{ - Message: "update attachment of release permission denied", - Err: util.ErrPermissionDenied, - } + + if len(attachments) != len(updateAttachments) { + return util.SilentWrap{ + Message: "update attachment of release permission denied", + Err: util.ErrPermissionDenied, } } }