From bed7e0b528faab1c29ff72702c25ca5751ec5ac5 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 5 Sep 2021 11:00:57 +0800 Subject: [PATCH 01/12] Add repo_id for attachment --- models/attachment.go | 25 ++-------- models/attachment_test.go | 1 + models/context.go | 6 +++ models/migrations/migrations.go | 2 + models/migrations/v193.go | 33 +++++++++++++ models/repo.go | 56 ++++++++++++++++++----- routers/api/v1/repo/release_attachment.go | 4 +- routers/web/repo/attachment.go | 10 ++-- routers/web/repo/setting.go | 3 +- services/attachment/attachment.go | 36 +++++++++++++++ services/wiki/wiki.go | 10 ++++ 11 files changed, 146 insertions(+), 40 deletions(-) create mode 100644 models/migrations/v193.go create mode 100644 services/attachment/attachment.go diff --git a/models/attachment.go b/models/attachment.go index e12609f8108f4..ee46f3b3a193e 100644 --- a/models/attachment.go +++ b/models/attachment.go @@ -5,16 +5,13 @@ package models import ( - "bytes" "fmt" - "io" "path" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/timeutil" - gouuid "github.com/google/uuid" "xorm.io/xorm" ) @@ -22,8 +19,9 @@ import ( type Attachment struct { ID int64 `xorm:"pk autoincr"` UUID string `xorm:"uuid UNIQUE"` - IssueID int64 `xorm:"INDEX"` - ReleaseID int64 `xorm:"INDEX"` + 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 Name string @@ -81,23 +79,6 @@ func (a *Attachment) LinkedRepository() (*Repository, UnitType, error) { return nil, -1, nil } -// NewAttachment creates a new attachment object. -func NewAttachment(attach *Attachment, buf []byte, file io.Reader) (_ *Attachment, err error) { - attach.UUID = gouuid.New().String() - - size, err := storage.Attachments.Save(attach.RelativePath(), io.MultiReader(bytes.NewReader(buf), file), -1) - if err != nil { - return nil, fmt.Errorf("Create: %v", err) - } - attach.Size = size - - if _, err := x.Insert(attach); err != nil { - return nil, err - } - - return attach, nil -} - // GetAttachmentByID returns attachment by given id func GetAttachmentByID(id int64) (*Attachment, error) { return getAttachmentByID(x, id) diff --git a/models/attachment_test.go b/models/attachment_test.go index 700b7c09dbb11..0b05752a419e9 100644 --- a/models/attachment_test.go +++ b/models/attachment_test.go @@ -28,6 +28,7 @@ func TestUploadAttachment(t *testing.T) { buf = buf[:n] attach, err := NewAttachment(&Attachment{ + RepoID: 1, UploaderID: user.ID, Name: filepath.Base(fPath), }, buf, f) diff --git a/models/context.go b/models/context.go index 1221ab7dede9d..a074d068343f1 100644 --- a/models/context.go +++ b/models/context.go @@ -66,3 +66,9 @@ func Iterate(ctx DBContext, tableBean interface{}, cond builder.Cond, fun func(i BufferSize(setting.Database.IterateBufferSize). Iterate(tableBean, fun) } + +// Insert inserts records into database +func Insert(ctx DBContext, beans ...interface{}) error { + _, err := ctx.e.Insert(beans...) + return err +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 79b1e90ecd734..7960edc80bf3e 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -338,6 +338,8 @@ var migrations = []Migration{ NewMigration("Alter issue/comment table TEXT fields to LONGTEXT", alterIssueAndCommentTextFieldsToLongText), // v192 -> v193 NewMigration("RecreateIssueResourceIndexTable to have a primary key instead of an unique index", recreateIssueResourceIndexTable), + // v193 -> v194 + NewMigration("Add repo id column for attachment table", addRepoIDForAttachment), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v193.go b/models/migrations/v193.go new file mode 100644 index 0000000000000..5c1f27f1d8f1f --- /dev/null +++ b/models/migrations/v193.go @@ -0,0 +1,33 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +func addRepoIDForAttachment(x *xorm.Engine) error { + 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"` + } + if err := x.Sync2(new(Attachment)); err != nil { + return err + } + + if _, err := x.Exec("UPDATE attachment set repo_id = (SELECT repo_id FROM issue WHERE issue.id = issue_id) WHERE issue_id > 0"); err != nil { + return err + } + + if _, err := x.Exec("UPDATE attachment set repo_id = (SELECT repo_id FROM release WHERE release.id = release_id) WHERE release_id > 0"); err != nil { + return err + } + + return nil +} diff --git a/models/repo.go b/models/repo.go index 94af3187891d0..db1f978b9d51c 100644 --- a/models/repo.go +++ b/models/repo.go @@ -530,12 +530,7 @@ func (repo *Repository) DeleteWiki() error { } func (repo *Repository) deleteWiki(e Engine) error { - wikiPaths := []string{repo.WikiPath()} - for _, wikiPath := range wikiPaths { - removeAllWithNotice(e, "Delete repository wiki", wikiPath) - } - - _, err := e.Where("repo_id = ?", repo.ID).And("type = ?", UnitTypeWiki).Delete(new(RepoUnit)) + _, err := x.Where("repo_id = ?", repo.ID).And("type = ?", UnitTypeWiki).Delete(new(RepoUnit)) return err } @@ -1579,10 +1574,6 @@ func DeleteRepository(doer *User, uid, repoID int64) error { } } - // FIXME: Remove repository files should be executed after transaction succeed. - repoPath := repo.RepoPath() - removeAllWithNotice(sess, "Delete repository files", repoPath) - err = repo.deleteWiki(sess) if err != nil { return err @@ -1594,6 +1585,7 @@ func DeleteRepository(doer *User, uid, repoID int64) error { return err } + var lfsPaths = make([]string, 0, len(lfsObjects)) for _, v := range lfsObjects { count, err := sess.Count(&LFSMetaObject{Pointer: lfs.Pointer{Oid: v.Oid}}) if err != nil { @@ -1603,7 +1595,7 @@ func DeleteRepository(doer *User, uid, repoID int64) error { continue } - removeStorageWithNotice(sess, storage.LFS, "Delete orphaned LFS file", v.RelativePath()) + lfsPaths = append(lfsPaths, v.RelativePath()) } if _, err := sess.Delete(&LFSMetaObject{RepositoryID: repoID}); err != nil { @@ -1616,10 +1608,11 @@ func DeleteRepository(doer *User, uid, repoID int64) error { return err } + var archivePaths = make([]string, 0, len(archives)) for _, v := range archives { v.Repo = repo p, _ := v.RelativePath() - removeStorageWithNotice(sess, storage.RepoArchives, "Delete repo archive file", p) + archivePaths = append(archivePaths, p) } if _, err := sess.Delete(&RepoArchiver{RepoID: repoID}); err != nil { @@ -1632,6 +1625,21 @@ func DeleteRepository(doer *User, uid, repoID int64) error { } } + // Get all attachments with both issue_id and release_id are zero + var newAttachments []*Attachment + if err := sess.Where(builder.Eq{ + "repo_id": repo.ID, + "issue_id": 0, + "release_id": 0, + }).Find(&newAttachments); err != nil { + return err + } + + var newAttachmentPaths = make([]string, 0, len(newAttachments)) + for _, attach := range newAttachments { + newAttachmentPaths = append(newAttachmentPaths, attach.RelativePath()) + } + if err = sess.Commit(); err != nil { return err } @@ -1641,6 +1649,25 @@ func DeleteRepository(doer *User, uid, repoID int64) error { // We should always delete the files after the database transaction succeed. If // we delete the file but the database rollback, the repository will be broken. + // Remove repository files. + repoPath := repo.RepoPath() + removeAllWithNotice(x, "Delete repository files", repoPath) + + // Remove wiki files + if repo.HasWiki() { + removeAllWithNotice(x, "Delete repository wiki", repo.WikiPath()) + } + + // Remove archives + for i := range archivePaths { + removeStorageWithNotice(x, storage.RepoArchives, "Delete repo archive file", archivePaths[i]) + } + + // Remove lfs objects + for i := range lfsPaths { + removeStorageWithNotice(x, storage.LFS, "Delete orphaned LFS file", lfsPaths[i]) + } + // Remove issue attachment files. for i := range attachmentPaths { RemoveStorageWithNotice(storage.Attachments, "Delete issue attachment", attachmentPaths[i]) @@ -1651,6 +1678,11 @@ func DeleteRepository(doer *User, uid, repoID int64) error { RemoveStorageWithNotice(storage.Attachments, "Delete release attachment", releaseAttachments[i]) } + // Remove attachment with no issue_id and release_id. + for i := range newAttachmentPaths { + RemoveStorageWithNotice(storage.Attachments, "Delete issue attachment", attachmentPaths[i]) + } + if len(repo.Avatar) > 0 { if err := storage.RepoAvatars.Delete(repo.CustomAvatarRelativePath()); err != nil { return fmt.Errorf("Failed to remove %s: %v", repo.Avatar, err) diff --git a/routers/api/v1/repo/release_attachment.go b/routers/api/v1/repo/release_attachment.go index 0834667657cfa..77a2cd190889d 100644 --- a/routers/api/v1/repo/release_attachment.go +++ b/routers/api/v1/repo/release_attachment.go @@ -15,6 +15,7 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/upload" "code.gitea.io/gitea/modules/web" + "code.gitea.io/gitea/services/attachment" ) // GetReleaseAttachment gets a single attachment of the release @@ -195,7 +196,8 @@ func CreateReleaseAttachment(ctx *context.APIContext) { } // Create a new attachment and save the file - attach, err := models.NewAttachment(&models.Attachment{ + attach, err := attachment.NewAttachment(&models.Attachment{ + RepoID: release.RepoID, UploaderID: ctx.User.ID, Name: filename, ReleaseID: release.ID, diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index 1a25384792c24..e5e4f6915613e 100644 --- a/routers/web/repo/attachment.go +++ b/routers/web/repo/attachment.go @@ -16,20 +16,21 @@ import ( "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/upload" "code.gitea.io/gitea/routers/common" + "code.gitea.io/gitea/services/attachment" ) // UploadIssueAttachment response for Issue/PR attachments func UploadIssueAttachment(ctx *context.Context) { - uploadAttachment(ctx, setting.Attachment.AllowedTypes) + uploadAttachment(ctx, ctx.Repo.Repository.ID, setting.Attachment.AllowedTypes) } // UploadReleaseAttachment response for uploading release attachments func UploadReleaseAttachment(ctx *context.Context) { - uploadAttachment(ctx, setting.Repository.Release.AllowedTypes) + uploadAttachment(ctx, ctx.Repo.Repository.ID, setting.Repository.Release.AllowedTypes) } // UploadAttachment response for uploading attachments -func uploadAttachment(ctx *context.Context, allowedTypes string) { +func uploadAttachment(ctx *context.Context, repoID int64, allowedTypes string) { if !setting.Attachment.Enabled { ctx.Error(http.StatusNotFound, "attachment is not enabled") return @@ -54,7 +55,8 @@ func uploadAttachment(ctx *context.Context, allowedTypes string) { return } - attach, err := models.NewAttachment(&models.Attachment{ + attach, err := attachment.NewAttachment(&models.Attachment{ + RepoID: repoID, UploaderID: ctx.User.ID, Name: header.Filename, }, buf, file) diff --git a/routers/web/repo/setting.go b/routers/web/repo/setting.go index 136e08cb4702e..5f71a77659dea 100644 --- a/routers/web/repo/setting.go +++ b/routers/web/repo/setting.go @@ -34,6 +34,7 @@ import ( "code.gitea.io/gitea/services/mailer" mirror_service "code.gitea.io/gitea/services/mirror" repo_service "code.gitea.io/gitea/services/repository" + wiki_service "code.gitea.io/gitea/services/wiki" ) const ( @@ -666,7 +667,7 @@ func SettingsPost(ctx *context.Context) { return } - err := repo.DeleteWiki() + err := wiki_service.DeleteWiki(repo) if err != nil { log.Error("Delete Wiki: %v", err.Error()) } diff --git a/services/attachment/attachment.go b/services/attachment/attachment.go new file mode 100644 index 0000000000000..d258c451f93cd --- /dev/null +++ b/services/attachment/attachment.go @@ -0,0 +1,36 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package attachment + +import ( + "bytes" + "fmt" + "io" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/storage" + + "github.com/google/uuid" +) + +// NewAttachment creates a new attachment object. +func NewAttachment(attach *models.Attachment, buf []byte, file io.Reader) (*models.Attachment, error) { + if attach.RepoID == 0 { + return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name) + } + + err := models.WithTx(func(ctx models.DBContext) error { + attach.UUID = uuid.New().String() + size, err := storage.Attachments.Save(attach.RelativePath(), io.MultiReader(bytes.NewReader(buf), file), -1) + if err != nil { + return fmt.Errorf("Create: %v", err) + } + attach.Size = size + + return models.Insert(ctx, attach) + }) + + return attach, err +} diff --git a/services/wiki/wiki.go b/services/wiki/wiki.go index e1590f461ef23..7ddc69e4281e4 100644 --- a/services/wiki/wiki.go +++ b/services/wiki/wiki.go @@ -366,3 +366,13 @@ func DeleteWikiPage(doer *models.User, repo *models.Repository, wikiName string) return nil } + +// DeleteWiki removes the actual and local copy of repository wiki. +func DeleteWiki(repo *models.Repository) error { + if err := repo.DeleteWiki(); err != nil { + return err + } + + models.RemoveAllWithNotice("Delete repository wiki", repo.WikiPath()) + return nil +} From 06601cecea06686e216e1774f54af6d5c8e7154f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 5 Sep 2021 11:09:39 +0800 Subject: [PATCH 02/12] deleting attachments only have repo_id --- models/repo.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/models/repo.go b/models/repo.go index db1f978b9d51c..90863f9911f12 100644 --- a/models/repo.go +++ b/models/repo.go @@ -1492,11 +1492,6 @@ func DeleteRepository(doer *User, uid, repoID int64) error { releaseAttachments = append(releaseAttachments, attachments[i].RelativePath()) } - if _, err = sess.In("release_id", builder.Select("id").From("`release`").Where(builder.Eq{"`release`.repo_id": repoID})). - Delete(&Attachment{}); err != nil { - return err - } - if _, err := sess.Exec("UPDATE `user` SET num_stars=num_stars-1 WHERE id IN (SELECT `uid` FROM `star` WHERE repo_id = ?)", repo.ID); err != nil { return err } @@ -1640,6 +1635,10 @@ func DeleteRepository(doer *User, uid, repoID int64) error { newAttachmentPaths = append(newAttachmentPaths, attach.RelativePath()) } + if _, err := sess.Where("repo_id=?", repo.ID).Delete(new(Attachment)); err != nil { + return err + } + if err = sess.Commit(); err != nil { return err } From 9ad3cb0232d76ca198a2bf2f5ff86e9c4ebab189 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 12:27:37 +0200 Subject: [PATCH 03/12] refactor & fix DeleteWiki --- models/repo.go | 15 --------------- services/wiki/wiki.go | 2 +- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/models/repo.go b/models/repo.go index 90863f9911f12..23287067e2a40 100644 --- a/models/repo.go +++ b/models/repo.go @@ -524,16 +524,6 @@ func (repo *Repository) ComposeDocumentMetas() map[string]string { return repo.DocumentRenderingMetas } -// DeleteWiki removes the actual and local copy of repository wiki. -func (repo *Repository) DeleteWiki() error { - return repo.deleteWiki(x) -} - -func (repo *Repository) deleteWiki(e Engine) error { - _, err := x.Where("repo_id = ?", repo.ID).And("type = ?", UnitTypeWiki).Delete(new(RepoUnit)) - return err -} - func (repo *Repository) getAssignees(e Engine) (_ []*User, err error) { if err = repo.getOwner(e); err != nil { return nil, err @@ -1569,11 +1559,6 @@ func DeleteRepository(doer *User, uid, repoID int64) error { } } - err = repo.deleteWiki(sess) - if err != nil { - return err - } - // Remove LFS objects var lfsObjects []*LFSMetaObject if err = sess.Where("repository_id=?", repoID).Find(&lfsObjects); err != nil { diff --git a/services/wiki/wiki.go b/services/wiki/wiki.go index 7ddc69e4281e4..5acb23ac78a24 100644 --- a/services/wiki/wiki.go +++ b/services/wiki/wiki.go @@ -369,7 +369,7 @@ func DeleteWikiPage(doer *models.User, repo *models.Repository, wikiName string) // DeleteWiki removes the actual and local copy of repository wiki. func DeleteWiki(repo *models.Repository) error { - if err := repo.DeleteWiki(); err != nil { + if err := models.UpdateRepositoryUnits(repo, nil, []models.UnitType{models.UnitTypeWiki}); err != nil { return err } From a6ca8deea5ca9d411025b68ab885aaa3e38384ba Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 12:32:13 +0200 Subject: [PATCH 04/12] Fix tests --- models/attachment_test.go | 30 ------------------ services/attachment/attachment_test.go | 43 ++++++++++++++++++++++++++ services/release/release_test.go | 7 +++-- 3 files changed, 48 insertions(+), 32 deletions(-) create mode 100644 services/attachment/attachment_test.go diff --git a/models/attachment_test.go b/models/attachment_test.go index 0b05752a419e9..4f6eb0a5edd21 100644 --- a/models/attachment_test.go +++ b/models/attachment_test.go @@ -5,41 +5,11 @@ package models import ( - "os" - "path/filepath" "testing" "github.com/stretchr/testify/assert" ) -func TestUploadAttachment(t *testing.T) { - assert.NoError(t, PrepareTestDatabase()) - - user := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User) - - fPath := "./attachment_test.go" - f, err := os.Open(fPath) - assert.NoError(t, err) - defer f.Close() - - buf := make([]byte, 1024) - n, err := f.Read(buf) - assert.NoError(t, err) - buf = buf[:n] - - attach, err := NewAttachment(&Attachment{ - RepoID: 1, - UploaderID: user.ID, - Name: filepath.Base(fPath), - }, buf, f) - assert.NoError(t, err) - - attachment, err := GetAttachmentByUUID(attach.UUID) - assert.NoError(t, err) - assert.EqualValues(t, user.ID, attachment.UploaderID) - assert.Equal(t, int64(0), attachment.DownloadCount) -} - func TestIncreaseDownloadCount(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) diff --git a/services/attachment/attachment_test.go b/services/attachment/attachment_test.go new file mode 100644 index 0000000000000..0691f72cf9de8 --- /dev/null +++ b/services/attachment/attachment_test.go @@ -0,0 +1,43 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package attachment + +import ( + "os" + "path/filepath" + "testing" + + "code.gitea.io/gitea/models" + + "github.com/stretchr/testify/assert" +) + +func TestUploadAttachment(t *testing.T) { + assert.NoError(t, models.PrepareTestDatabase()) + + user := models.AssertExistsAndLoadBean(t, &models.User{ID: 1}).(*models.User) + + fPath := "./attachment_test.go" + f, err := os.Open(fPath) + assert.NoError(t, err) + defer f.Close() + + buf := make([]byte, 1024) + n, err := f.Read(buf) + assert.NoError(t, err) + buf = buf[:n] + + attach, err := NewAttachment(&models.Attachment{ + RepoID: 1, + UploaderID: user.ID, + Name: filepath.Base(fPath), + }, buf, f) + assert.NoError(t, err) + + attachment, err := models.GetAttachmentByUUID(attach.UUID) + assert.NoError(t, err) + assert.EqualValues(t, user.ID, attachment.UploaderID) + assert.Equal(t, int64(0), attachment.DownloadCount) +} diff --git a/services/release/release_test.go b/services/release/release_test.go index 9f665fabab6f5..8467b12c1d6f2 100644 --- a/services/release/release_test.go +++ b/services/release/release_test.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/services/attachment" "github.com/stretchr/testify/assert" ) @@ -101,7 +102,8 @@ func TestRelease_Create(t *testing.T) { IsTag: false, }, nil, "")) - attach, err := models.NewAttachment(&models.Attachment{ + attach, err := attachment.NewAttachment(&models.Attachment{ + RepoID: repo.ID, UploaderID: user.ID, Name: "test.txt", }, []byte{}, strings.NewReader("testtest")) @@ -233,7 +235,8 @@ func TestRelease_Update(t *testing.T) { assert.Equal(t, tagName, release.TagName) // Add new attachments - attach, err := models.NewAttachment(&models.Attachment{ + attach, err := attachment.NewAttachment(&models.Attachment{ + RepoID: repo.RepoID, UploaderID: user.ID, Name: "test.txt", }, []byte{}, strings.NewReader("testtest")) From 5d687f3bc124f8b5c1397d8f98d6f5430a5dc348 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 13:06:35 +0200 Subject: [PATCH 05/12] move upload.Verify into attachment_service --- routers/api/v1/repo/release_attachment.go | 24 +++++--------------- routers/web/repo/attachment.go | 22 +++++------------- services/attachment/attachment.go | 27 ++++++++++++++++++++--- services/attachment/attachment_test.go | 7 +----- services/release/release_test.go | 4 ++-- 5 files changed, 37 insertions(+), 47 deletions(-) diff --git a/routers/api/v1/repo/release_attachment.go b/routers/api/v1/repo/release_attachment.go index 77a2cd190889d..d1533e2b5ae03 100644 --- a/routers/api/v1/repo/release_attachment.go +++ b/routers/api/v1/repo/release_attachment.go @@ -177,32 +177,18 @@ func CreateReleaseAttachment(ctx *context.APIContext) { } defer file.Close() - buf := make([]byte, 1024) - n, _ := file.Read(buf) - if n > 0 { - buf = buf[:n] - } - - // Check if the filetype is allowed by the settings - err = upload.Verify(buf, header.Filename, setting.Repository.Release.AllowedTypes) - if err != nil { - ctx.Error(http.StatusBadRequest, "DetectContentType", err) - return - } - var filename = header.Filename if query := ctx.FormString("name"); query != "" { filename = query } // Create a new attachment and save the file - attach, err := attachment.NewAttachment(&models.Attachment{ - RepoID: release.RepoID, - UploaderID: ctx.User.ID, - Name: filename, - ReleaseID: release.ID, - }, buf, file) + attach, err := attachment.UploadAttachment(file, ctx.User.ID, release.RepoID, releaseID, filename, setting.Repository.Release.AllowedTypes) if err != nil { + if upload.IsErrFileTypeForbidden(err) { + ctx.Error(http.StatusBadRequest, "DetectContentType", err) + return + } ctx.Error(http.StatusInternalServerError, "NewAttachment", err) return } diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index e5e4f6915613e..3968d2765266c 100644 --- a/routers/web/repo/attachment.go +++ b/routers/web/repo/attachment.go @@ -43,24 +43,12 @@ func uploadAttachment(ctx *context.Context, repoID int64, allowedTypes string) { } defer file.Close() - buf := make([]byte, 1024) - n, _ := file.Read(buf) - if n > 0 { - buf = buf[:n] - } - - err = upload.Verify(buf, header.Filename, allowedTypes) - if err != nil { - ctx.Error(http.StatusBadRequest, err.Error()) - return - } - - attach, err := attachment.NewAttachment(&models.Attachment{ - RepoID: repoID, - UploaderID: ctx.User.ID, - Name: header.Filename, - }, buf, file) + attach, err := attachment.UploadAttachment(file, ctx.User.ID, repoID, 0, header.Filename, allowedTypes) if err != nil { + if upload.IsErrFileTypeForbidden(err) { + ctx.Error(http.StatusBadRequest, err.Error()) + return + } ctx.Error(http.StatusInternalServerError, fmt.Sprintf("NewAttachment: %v", err)) return } diff --git a/services/attachment/attachment.go b/services/attachment/attachment.go index d258c451f93cd..4c356cd079109 100644 --- a/services/attachment/attachment.go +++ b/services/attachment/attachment.go @@ -11,19 +11,20 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/storage" + "code.gitea.io/gitea/modules/upload" "github.com/google/uuid" ) -// NewAttachment creates a new attachment object. -func NewAttachment(attach *models.Attachment, buf []byte, file io.Reader) (*models.Attachment, error) { +// NewAttachment creates a new attachment object, but do not verify. +func NewAttachment(attach *models.Attachment, file io.Reader) (*models.Attachment, error) { if attach.RepoID == 0 { return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name) } err := models.WithTx(func(ctx models.DBContext) error { attach.UUID = uuid.New().String() - size, err := storage.Attachments.Save(attach.RelativePath(), io.MultiReader(bytes.NewReader(buf), file), -1) + size, err := storage.Attachments.Save(attach.RelativePath(), file, -1) if err != nil { return fmt.Errorf("Create: %v", err) } @@ -34,3 +35,23 @@ func NewAttachment(attach *models.Attachment, buf []byte, file io.Reader) (*mode return attach, err } + +// UploadAttachment upload new attachment into storage and update database +func UploadAttachment(file io.Reader, actorID, repoID, releaseID int64, fileName string, allowedTypes string) (*models.Attachment, error) { + buf := make([]byte, 1024) + n, _ := file.Read(buf) + if n > 0 { + buf = buf[:n] + } + + if err := upload.Verify(buf, fileName, allowedTypes); err != nil { + return nil, err + } + + return NewAttachment(&models.Attachment{ + RepoID: repoID, + UploaderID: actorID, + ReleaseID: releaseID, + Name: fileName, + }, io.MultiReader(bytes.NewReader(buf), file)) +} diff --git a/services/attachment/attachment_test.go b/services/attachment/attachment_test.go index 0691f72cf9de8..2b0127738733a 100644 --- a/services/attachment/attachment_test.go +++ b/services/attachment/attachment_test.go @@ -24,16 +24,11 @@ func TestUploadAttachment(t *testing.T) { assert.NoError(t, err) defer f.Close() - buf := make([]byte, 1024) - n, err := f.Read(buf) - assert.NoError(t, err) - buf = buf[:n] - attach, err := NewAttachment(&models.Attachment{ RepoID: 1, UploaderID: user.ID, Name: filepath.Base(fPath), - }, buf, f) + }, f) assert.NoError(t, err) attachment, err := models.GetAttachmentByUUID(attach.UUID) diff --git a/services/release/release_test.go b/services/release/release_test.go index 8467b12c1d6f2..b160f77c4e816 100644 --- a/services/release/release_test.go +++ b/services/release/release_test.go @@ -106,7 +106,7 @@ func TestRelease_Create(t *testing.T) { RepoID: repo.ID, UploaderID: user.ID, Name: "test.txt", - }, []byte{}, strings.NewReader("testtest")) + }, strings.NewReader("testtest")) assert.NoError(t, err) var release = models.Release{ @@ -239,7 +239,7 @@ func TestRelease_Update(t *testing.T) { RepoID: repo.RepoID, UploaderID: user.ID, Name: "test.txt", - }, []byte{}, strings.NewReader("testtest")) + }, strings.NewReader("testtest")) assert.NoError(t, err) assert.NoError(t, UpdateRelease(user, gitRepo, release, []string{attach.UUID}, nil, nil)) From c139a49bcb04588376139ea07eb6fe377b2db68e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 5 Sep 2021 19:49:14 +0800 Subject: [PATCH 06/12] Fix test --- services/release/release_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/release/release_test.go b/services/release/release_test.go index b160f77c4e816..936f2ab71c753 100644 --- a/services/release/release_test.go +++ b/services/release/release_test.go @@ -236,7 +236,7 @@ func TestRelease_Update(t *testing.T) { // Add new attachments attach, err := attachment.NewAttachment(&models.Attachment{ - RepoID: repo.RepoID, + RepoID: repo.ID, UploaderID: user.ID, Name: "test.txt", }, strings.NewReader("testtest")) From 7d9cfda019598fc24cb4f1edc5b6eb0a0cb58665 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 15:50:31 +0200 Subject: [PATCH 07/12] test: make db prepared --- services/attachment/attachment_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/attachment/attachment_test.go b/services/attachment/attachment_test.go index 2b0127738733a..c11204b22109a 100644 --- a/services/attachment/attachment_test.go +++ b/services/attachment/attachment_test.go @@ -14,6 +14,10 @@ import ( "github.com/stretchr/testify/assert" ) +func TestMain(m *testing.M) { + models.MainTest(m, filepath.Join("..", "..")) +} + func TestUploadAttachment(t *testing.T) { assert.NoError(t, models.PrepareTestDatabase()) From 083b4a42a3434a0c3140c09af06f8be1cdf342cc Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 5 Sep 2021 21:56:18 +0800 Subject: [PATCH 08/12] Add test for migration v193 --- models/migrations/v193_test.go | 72 ++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 models/migrations/v193_test.go diff --git a/models/migrations/v193_test.go b/models/migrations/v193_test.go new file mode 100644 index 0000000000000..1172173a15dbd --- /dev/null +++ b/models/migrations/v193_test.go @@ -0,0 +1,72 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_addRepoIDForAttachment(t *testing.T) { + 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"` + } + + // Prepare and load the testing database + x, deferable := prepareTestEnv(t, 0, new(Attachment)) + if x == nil || t.Failed() { + defer deferable() + return + } + defer deferable() + + // Run the migration + if err := addRepoIDForAttachment(x); err != nil { + assert.NoError(t, err) + return + } + + type Issue struct { + ID int64 + RepoID int64 + } + + var issueAttachments []*Attachment + err := x.Where("issue_id > 0").Find(&issueAttachments) + assert.NoError(t, err) + for _, attach := range issueAttachments { + assert.Greater(t, attach.RepoID, 0) + assert.Greater(t, attach.IssueID, 0) + var issue Issue + has, err := x.ID(attach.IssueID).Get(&issue) + assert.NoError(t, err) + assert.True(t, has) + assert.EqualValues(t, attach.RepoID, issue.RepoID) + } + + type Release struct { + ID int64 + RepoID int64 + } + + var releaseAttachments []*Attachment + err = x.Where("release_id > 0").Find(&releaseAttachments) + assert.NoError(t, err) + for _, attach := range releaseAttachments { + assert.Greater(t, attach.RepoID, 0) + assert.Greater(t, attach.IssueID, 0) + var release Release + has, err := x.ID(attach.ReleaseID).Get(&release) + assert.NoError(t, err) + assert.True(t, has) + assert.EqualValues(t, attach.RepoID, release.RepoID) + } +} From b02329f9a1e7387bee6e2bf51c61c103ab9ed0fa Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 16:06:46 +0200 Subject: [PATCH 09/12] Apply suggestions from code review --- models/migrations/v193.go | 4 ++-- models/migrations/v193_test.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/models/migrations/v193.go b/models/migrations/v193.go index 5c1f27f1d8f1f..c8244a1b3de9f 100644 --- a/models/migrations/v193.go +++ b/models/migrations/v193.go @@ -21,11 +21,11 @@ func addRepoIDForAttachment(x *xorm.Engine) error { return err } - if _, err := x.Exec("UPDATE attachment set repo_id = (SELECT repo_id FROM issue WHERE issue.id = issue_id) WHERE issue_id > 0"); err != nil { + if _, err := x.Exec("UPDATE `attachment` set repo_id = (SELECT repo_id FROM `issue` WHERE `issue`.id = `attachment`.issue_id) WHERE `attachment`.issue_id > 0"); err != nil { return err } - if _, err := x.Exec("UPDATE attachment set repo_id = (SELECT repo_id FROM release WHERE release.id = release_id) WHERE release_id > 0"); err != nil { + if _, err := x.Exec("UPDATE `attachment` set repo_id = (SELECT repo_id FROM `release` WHERE `release`.id = `attachment`.release_id) WHERE `attachment`.release_id > 0"); err != nil { return err } diff --git a/models/migrations/v193_test.go b/models/migrations/v193_test.go index 1172173a15dbd..3211072d0fdbc 100644 --- a/models/migrations/v193_test.go +++ b/models/migrations/v193_test.go @@ -22,11 +22,10 @@ func Test_addRepoIDForAttachment(t *testing.T) { // Prepare and load the testing database x, deferable := prepareTestEnv(t, 0, new(Attachment)) + defer deferable() if x == nil || t.Failed() { - defer deferable() return } - defer deferable() // Run the migration if err := addRepoIDForAttachment(x); err != nil { From 2ae23cd454af904e8abfd2d7705da3fcdb40487b Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 16:12:04 +0200 Subject: [PATCH 10/12] update fixtures --- models/fixtures/attachment.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/models/fixtures/attachment.yml b/models/fixtures/attachment.yml index 2606d52b47166..3a7843f89c874 100644 --- a/models/fixtures/attachment.yml +++ b/models/fixtures/attachment.yml @@ -1,6 +1,7 @@ - id: 1 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11 + repo_id: 1 issue_id: 1 comment_id: 0 name: attach1 @@ -10,6 +11,7 @@ - id: 2 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12 + repo_id: 2 issue_id: 4 comment_id: 0 name: attach2 @@ -19,6 +21,7 @@ - id: 3 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a13 + repo_id: 1 issue_id: 2 comment_id: 1 name: attach1 @@ -28,6 +31,7 @@ - id: 4 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14 + repo_id: 1 issue_id: 3 comment_id: 1 name: attach2 @@ -37,6 +41,7 @@ - id: 5 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a15 + repo_id: 2 issue_id: 4 comment_id: 0 name: attach1 @@ -46,6 +51,7 @@ - id: 6 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a16 + repo_id: 1 issue_id: 5 comment_id: 2 name: attach1 @@ -55,6 +61,7 @@ - id: 7 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17 + repo_id: 1 issue_id: 5 comment_id: 2 name: attach1 @@ -64,6 +71,7 @@ - id: 8 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18 + repo_id: 3 issue_id: 6 comment_id: 0 name: attach1 @@ -73,6 +81,7 @@ - id: 9 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19 + repo_id: 1 release_id: 1 name: attach1 download_count: 0 @@ -81,6 +90,8 @@ - id: 10 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20 + repo_id: 1 + release_id: 5 uploader_id: 8 name: attach1 download_count: 0 @@ -89,6 +100,7 @@ - id: 11 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a21 + repo_id: 40 release_id: 2 name: attach1 download_count: 0 From e3f06adbbc1602228654907e76188ffb56dd2535 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 16:16:16 +0200 Subject: [PATCH 11/12] fix TestGetAttachment/NotLinked --- models/fixtures/attachment.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/models/fixtures/attachment.yml b/models/fixtures/attachment.yml index 3a7843f89c874..8612f6ece7088 100644 --- a/models/fixtures/attachment.yml +++ b/models/fixtures/attachment.yml @@ -90,8 +90,7 @@ - id: 10 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20 - repo_id: 1 - release_id: 5 + repo_id: 0 # TestGetAttachment/NotLinked uploader_id: 8 name: attach1 download_count: 0 From d4db913a657bc00440a58ddf410cfa8812d34f9e Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 5 Sep 2021 17:17:22 +0200 Subject: [PATCH 12/12] fix v193_test.go --- models/migrations/v193_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/models/migrations/v193_test.go b/models/migrations/v193_test.go index 3211072d0fdbc..b250d154f782c 100644 --- a/models/migrations/v193_test.go +++ b/models/migrations/v193_test.go @@ -20,9 +20,19 @@ func Test_addRepoIDForAttachment(t *testing.T) { UploaderID int64 `xorm:"INDEX DEFAULT 0"` } + type Issue struct { + ID int64 + RepoID int64 + } + + type Release struct { + ID int64 + RepoID int64 + } + // Prepare and load the testing database - x, deferable := prepareTestEnv(t, 0, new(Attachment)) - defer deferable() + x, deferrable := prepareTestEnv(t, 0, new(Attachment), new(Issue), new(Release)) + defer deferrable() if x == nil || t.Failed() { return } @@ -33,11 +43,6 @@ func Test_addRepoIDForAttachment(t *testing.T) { return } - type Issue struct { - ID int64 - RepoID int64 - } - var issueAttachments []*Attachment err := x.Where("issue_id > 0").Find(&issueAttachments) assert.NoError(t, err) @@ -51,11 +56,6 @@ func Test_addRepoIDForAttachment(t *testing.T) { assert.EqualValues(t, attach.RepoID, issue.RepoID) } - type Release struct { - ID int64 - RepoID int64 - } - var releaseAttachments []*Attachment err = x.Where("release_id > 0").Find(&releaseAttachments) assert.NoError(t, err)