From 6b153142d5c9d1ce675c3d34337fdd92767d5d7c Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 00:08:32 +0100 Subject: [PATCH 01/17] test: add current attachement responses --- integrations/attachement_test.go | 37 ++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/integrations/attachement_test.go b/integrations/attachement_test.go index 8d709a376ec9..ac469d7b4dc3 100644 --- a/integrations/attachement_test.go +++ b/integrations/attachement_test.go @@ -9,10 +9,14 @@ import ( "image" "image/png" "io" + "io/ioutil" "mime/multipart" "net/http" + "os" + "path" "testing" + "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" ) @@ -86,3 +90,36 @@ func TestCreateIssueAttachement(t *testing.T) { req = NewRequest(t, "GET", "/attachments/"+uuid) session.MakeRequest(t, req, http.StatusOK) } + +func TestGetAttachement(t *testing.T) { + prepareTestEnv(t) + session := loginUser(t, "user2") + testCases := []struct { + name string + uuid string + createFile bool + want int + }{ + {"LinkedIssueUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, http.StatusOK}, + {"LinkedCommentUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", true, http.StatusOK}, + {"linked_release_uuid", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19", true, http.StatusOK}, + {"NotExistingUUID", "not-existing-uuid", false, http.StatusNotFound}, + {"FileMissing", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, http.StatusInternalServerError}, + {"NotLinked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, http.StatusOK}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + //Write empty file to be available for response + if tc.createFile { + localPath := models.AttachmentLocalPath(tc.uuid) + err := os.MkdirAll(path.Dir(localPath), os.ModePerm) + assert.NoError(t, err) + err = ioutil.WriteFile(localPath, []byte("hello world"), 0644) + assert.NoError(t, err) + } + //Actual test + req := NewRequest(t, "GET", "/attachments/"+tc.uuid) + session.MakeRequest(t, req, tc.want) + }) + } +} From 595f5bb023c87b0b9cdd4ccedae191e368d9f103 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 01:13:03 +0100 Subject: [PATCH 02/17] refactor: check if attachement is linked and accessible by user --- integrations/attachement_test.go | 25 +++++++++++------ models/fixtures/attachment.yml | 2 +- models/fixtures/repo_unit.yml | 6 ++++ routers/routes/routes.go | 47 ++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 9 deletions(-) diff --git a/integrations/attachement_test.go b/integrations/attachement_test.go index ac469d7b4dc3..11627a8a2d07 100644 --- a/integrations/attachement_test.go +++ b/integrations/attachement_test.go @@ -93,19 +93,28 @@ func TestCreateIssueAttachement(t *testing.T) { func TestGetAttachement(t *testing.T) { prepareTestEnv(t) - session := loginUser(t, "user2") + adminSession := loginUser(t, "user1") + user2Session := loginUser(t, "user2") + user8Session := loginUser(t, "user8") + emptySession := emptyTestSession(t) testCases := []struct { name string uuid string createFile bool + session *TestSession want int }{ - {"LinkedIssueUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, http.StatusOK}, - {"LinkedCommentUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", true, http.StatusOK}, - {"linked_release_uuid", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19", true, http.StatusOK}, - {"NotExistingUUID", "not-existing-uuid", false, http.StatusNotFound}, - {"FileMissing", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, http.StatusInternalServerError}, - {"NotLinked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, http.StatusOK}, + {"LinkedIssueUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, user2Session, http.StatusOK}, + {"LinkedCommentUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", true, user2Session, http.StatusOK}, + {"linked_release_uuid", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19", true, user2Session, http.StatusOK}, + {"NotExistingUUID", "not-existing-uuid", false, user2Session, http.StatusNotFound}, + {"FileMissing", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, user2Session, http.StatusInternalServerError}, + {"NotLinked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, user2Session, http.StatusNotFound}, + {"PublicByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, emptySession, http.StatusOK}, + {"PrivateByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, emptySession, http.StatusNotFound}, + {"PrivateAccessibleByAdmin", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, adminSession, http.StatusOK}, + {"PrivateAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user2Session, http.StatusOK}, //TODO + {"NotAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user8Session, http.StatusForbidden}, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -119,7 +128,7 @@ func TestGetAttachement(t *testing.T) { } //Actual test req := NewRequest(t, "GET", "/attachments/"+tc.uuid) - session.MakeRequest(t, req, tc.want) + tc.session.MakeRequest(t, req, tc.want) }) } } diff --git a/models/fixtures/attachment.yml b/models/fixtures/attachment.yml index 289d4d0efd6c..062856d2c12b 100644 --- a/models/fixtures/attachment.yml +++ b/models/fixtures/attachment.yml @@ -10,7 +10,7 @@ - id: 2 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12 - issue_id: 1 + issue_id: 4 comment_id: 0 name: attach2 download_count: 1 diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index f7522d303116..5ced38b003fe 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -472,4 +472,10 @@ repo_id: 48 type: 7 config: "{\"ExternalTrackerURL\":\"https://tracker.com\",\"ExternalTrackerFormat\":\"https://tracker.com/{user}/{repo}/issues/{index}\",\"ExternalTrackerStyle\":\"alphanumeric\"}" + created_unix: 946684810 +- + id: 69 + repo_id: 2 + type: 2 + config: "{}" created_unix: 946684810 \ No newline at end of file diff --git a/routers/routes/routes.go b/routers/routes/routes.go index cfd4a60974f3..7c12ebc30112 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -495,6 +495,53 @@ func RegisterRoutes(m *macaron.Macaron) { return } + if attach.IssueID != 0 { + iss, err := models.GetIssueByID(attach.IssueID) + if err != nil { + ctx.ServerError("GetAttachmentByUUID.GetIssueByID", err) + return + } + repo, err := models.GetRepositoryByID(iss.RepoID) + if err != nil { + ctx.ServerError("GetAttachmentByUUID.GetRepositoryByID", err) + return + } + if repo.IsPrivate { + if !ctx.IsSigned { + ctx.Error(http.StatusNotFound) + return + } + if !repo.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, models.UnitTypeIssues) { + ctx.Error(http.StatusForbidden) + return + } + } + } else if attach.ReleaseID != 0 { + rel, err := models.GetReleaseByID(attach.ReleaseID) + if err != nil { + ctx.ServerError("GetAttachmentByUUID.GetReleaseByID", err) + return + } + repo, err := models.GetRepositoryByID(rel.RepoID) + if err != nil { + ctx.ServerError("GetAttachmentByUUID.GetRepositoryByID", err) + return + } + if repo.IsPrivate { + if !ctx.IsSigned { + ctx.Error(http.StatusNotFound) + return + } + if !repo.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, models.UnitTypeReleases) { + ctx.Error(http.StatusForbidden) + return + } + } + } else { + ctx.Error(http.StatusNotFound) + } + + //If we have matched and access to release or issue fr, err := os.Open(attach.LocalPath()) if err != nil { ctx.ServerError("Open", err) From cd494ee4c2cbc221d06dd2c9e591331680046d5e Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 01:14:40 +0100 Subject: [PATCH 03/17] chore: clean TODO --- integrations/attachement_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrations/attachement_test.go b/integrations/attachement_test.go index 11627a8a2d07..1d36f1b8a138 100644 --- a/integrations/attachement_test.go +++ b/integrations/attachement_test.go @@ -113,7 +113,7 @@ func TestGetAttachement(t *testing.T) { {"PublicByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, emptySession, http.StatusOK}, {"PrivateByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, emptySession, http.StatusNotFound}, {"PrivateAccessibleByAdmin", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, adminSession, http.StatusOK}, - {"PrivateAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user2Session, http.StatusOK}, //TODO + {"PrivateAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user2Session, http.StatusOK}, {"NotAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user8Session, http.StatusForbidden}, } for _, tc := range testCases { From a87f112a108e6158d01a8166259b2579febe6518 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 01:21:10 +0100 Subject: [PATCH 04/17] fix: typo attachement -> attachment --- go.sum | 1 + integrations/{attachement_test.go => attachment_test.go} | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) rename integrations/{attachement_test.go => attachment_test.go} (96%) diff --git a/go.sum b/go.sum index dd4ee71ac044..f668dcd07bbb 100644 --- a/go.sum +++ b/go.sum @@ -527,6 +527,7 @@ github.com/steveyen/gtreap v0.0.0-20150807155958-0abe01ef9be2 h1:JNEGSiWg6D3lcBC github.com/steveyen/gtreap v0.0.0-20150807155958-0abe01ef9be2/go.mod h1:mjqs7N0Q6m5HpR7QfXVBZXZWSqTjQLeTujjA/xUp2uw= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= diff --git a/integrations/attachement_test.go b/integrations/attachment_test.go similarity index 96% rename from integrations/attachement_test.go rename to integrations/attachment_test.go index 1d36f1b8a138..f8a4e25a05be 100644 --- a/integrations/attachement_test.go +++ b/integrations/attachment_test.go @@ -62,7 +62,7 @@ func TestCreateAnonymousAttachment(t *testing.T) { createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusFound) } -func TestCreateIssueAttachement(t *testing.T) { +func TestCreateIssueAttachment(t *testing.T) { prepareTestEnv(t) const repoURL = "user2/repo1" session := loginUser(t, "user2") @@ -77,7 +77,7 @@ func TestCreateIssueAttachement(t *testing.T) { postData := map[string]string{ "_csrf": htmlDoc.GetCSRF(), - "title": "New Issue With Attachement", + "title": "New Issue With Attachment", "content": "some content", "files[0]": uuid, } @@ -86,12 +86,12 @@ func TestCreateIssueAttachement(t *testing.T) { resp = session.MakeRequest(t, req, http.StatusFound) test.RedirectURL(resp) // check that redirect URL exists - //Validate that attachement is available + //Validate that attachment is available req = NewRequest(t, "GET", "/attachments/"+uuid) session.MakeRequest(t, req, http.StatusOK) } -func TestGetAttachement(t *testing.T) { +func TestGetAttachment(t *testing.T) { prepareTestEnv(t) adminSession := loginUser(t, "user1") user2Session := loginUser(t, "user2") From 3a905768a6b4a65f569d27d3f6e74cae97d0acc6 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 01:25:00 +0100 Subject: [PATCH 05/17] revert un-needed go.sum change --- go.sum | 1 - 1 file changed, 1 deletion(-) diff --git a/go.sum b/go.sum index f668dcd07bbb..dd4ee71ac044 100644 --- a/go.sum +++ b/go.sum @@ -527,7 +527,6 @@ github.com/steveyen/gtreap v0.0.0-20150807155958-0abe01ef9be2 h1:JNEGSiWg6D3lcBC github.com/steveyen/gtreap v0.0.0-20150807155958-0abe01ef9be2/go.mod h1:mjqs7N0Q6m5HpR7QfXVBZXZWSqTjQLeTujjA/xUp2uw= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= From 67551c499a5bf2ee4448332edcbd345efa326544 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 01:52:52 +0100 Subject: [PATCH 06/17] refactor: move models logic to models --- models/attachment.go | 20 ++++++++++++++ routers/routes/routes.go | 56 ++++++++++++---------------------------- 2 files changed, 36 insertions(+), 40 deletions(-) diff --git a/models/attachment.go b/models/attachment.go index 487ddd4ad5b4..6cfa6cb64ec3 100644 --- a/models/attachment.go +++ b/models/attachment.go @@ -71,6 +71,26 @@ func (a *Attachment) DownloadURL() string { return fmt.Sprintf("%sattachments/%s", setting.AppURL, a.UUID) } +// LinkedRepository returns the linked repo if any +func (a *Attachment) LinkedRepository() (*Repository, UnitType, error) { + if a.IssueID != 0 { + iss, err := GetIssueByID(a.IssueID) + if err != nil { + return nil, UnitTypeIssues, err + } + repo, err := GetRepositoryByID(iss.RepoID) + return repo, UnitTypeIssues, err + } else if a.ReleaseID != 0 { + rel, err := GetReleaseByID(a.ReleaseID) + if err != nil { + return nil, UnitTypeReleases, err + } + repo, err := GetRepositoryByID(rel.RepoID) + return repo, UnitTypeReleases, err + } + 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.NewV4().String() diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 7c12ebc30112..9901f578e32e 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -495,50 +495,26 @@ func RegisterRoutes(m *macaron.Macaron) { return } - if attach.IssueID != 0 { - iss, err := models.GetIssueByID(attach.IssueID) - if err != nil { - ctx.ServerError("GetAttachmentByUUID.GetIssueByID", err) - return - } - repo, err := models.GetRepositoryByID(iss.RepoID) - if err != nil { - ctx.ServerError("GetAttachmentByUUID.GetRepositoryByID", err) - return - } - if repo.IsPrivate { - if !ctx.IsSigned { - ctx.Error(http.StatusNotFound) - return - } - if !repo.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, models.UnitTypeIssues) { - ctx.Error(http.StatusForbidden) - return - } - } - } else if attach.ReleaseID != 0 { - rel, err := models.GetReleaseByID(attach.ReleaseID) - if err != nil { - ctx.ServerError("GetAttachmentByUUID.GetReleaseByID", err) + repository, unitType, err := attach.LinkedRepository() + if err != nil { + ctx.ServerError("LinkedRepository", err) + return + } + + if repository == nil { + ctx.Error(http.StatusNotFound) + return + } + + if repository.IsPrivate { + if !ctx.IsSigned { + ctx.Error(http.StatusNotFound) return } - repo, err := models.GetRepositoryByID(rel.RepoID) - if err != nil { - ctx.ServerError("GetAttachmentByUUID.GetRepositoryByID", err) + if !repository.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, unitType) { + ctx.Error(http.StatusForbidden) return } - if repo.IsPrivate { - if !ctx.IsSigned { - ctx.Error(http.StatusNotFound) - return - } - if !repo.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, models.UnitTypeReleases) { - ctx.Error(http.StatusForbidden) - return - } - } - } else { - ctx.Error(http.StatusNotFound) } //If we have matched and access to release or issue From 4740e0c67e36b67a252d8e6b7233661d4903e8ee Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 02:25:40 +0100 Subject: [PATCH 07/17] fix TestCreateIssueAttachment which was wrongly successful --- integrations/attachment_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integrations/attachment_test.go b/integrations/attachment_test.go index f8a4e25a05be..80a4e411a566 100644 --- a/integrations/attachment_test.go +++ b/integrations/attachment_test.go @@ -76,10 +76,10 @@ func TestCreateIssueAttachment(t *testing.T) { assert.True(t, exists, "The template has changed") postData := map[string]string{ - "_csrf": htmlDoc.GetCSRF(), - "title": "New Issue With Attachment", - "content": "some content", - "files[0]": uuid, + "_csrf": htmlDoc.GetCSRF(), + "title": "New Issue With Attachment", + "content": "some content", + "files": uuid, } req = NewRequestWithValues(t, "POST", link, postData) From fbddeac6a46a5e5285b4f3f335a83d8a58242197 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 02:33:25 +0100 Subject: [PATCH 08/17] fix unit tests with unittype added --- routers/user/home_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/user/home_test.go b/routers/user/home_test.go index 9d4136ac8c95..c85a0ea3e255 100644 --- a/routers/user/home_test.go +++ b/routers/user/home_test.go @@ -26,8 +26,8 @@ func TestIssues(t *testing.T) { Issues(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) - assert.EqualValues(t, map[int64]int64{1: 1}, ctx.Data["Counts"]) + assert.EqualValues(t, map[int64]int64{1: 1, 2: 1}, ctx.Data["Counts"]) assert.EqualValues(t, true, ctx.Data["IsShowClosed"]) assert.Len(t, ctx.Data["Issues"], 1) - assert.Len(t, ctx.Data["Repos"], 1) + assert.Len(t, ctx.Data["Repos"], 2) } From 7ebe2c31de09b7bff096bef54c4302e586bbf733 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 02:38:24 +0100 Subject: [PATCH 09/17] fix unit tests with changes --- models/attachment_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/attachment_test.go b/models/attachment_test.go index f38a5beeee13..0dad1b8bba1c 100644 --- a/models/attachment_test.go +++ b/models/attachment_test.go @@ -61,7 +61,7 @@ func TestGetByCommentOrIssueID(t *testing.T) { // count of attachments from issue ID attachments, err := GetAttachmentsByIssueID(1) assert.NoError(t, err) - assert.Equal(t, 2, len(attachments)) + assert.Equal(t, 1, len(attachments)) attachments, err = GetAttachmentsByCommentID(1) assert.NoError(t, err) @@ -73,7 +73,7 @@ func TestDeleteAttachments(t *testing.T) { count, err := DeleteAttachmentsByIssue(4, false) assert.NoError(t, err) - assert.Equal(t, 1, count) + assert.Equal(t, 2, count) count, err = DeleteAttachmentsByComment(2, false) assert.NoError(t, err) From b488f1db88563d79f6712b0a2a339a69dc83efa1 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 13 Dec 2019 03:20:01 +0100 Subject: [PATCH 10/17] use a valid uuid format for pgsql int. test --- integrations/attachment_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrations/attachment_test.go b/integrations/attachment_test.go index 80a4e411a566..3fe349d49f85 100644 --- a/integrations/attachment_test.go +++ b/integrations/attachment_test.go @@ -107,7 +107,7 @@ func TestGetAttachment(t *testing.T) { {"LinkedIssueUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, user2Session, http.StatusOK}, {"LinkedCommentUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", true, user2Session, http.StatusOK}, {"linked_release_uuid", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19", true, user2Session, http.StatusOK}, - {"NotExistingUUID", "not-existing-uuid", false, user2Session, http.StatusNotFound}, + {"NotExistingUUID", "b0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, user2Session, http.StatusNotFound}, {"FileMissing", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, user2Session, http.StatusInternalServerError}, {"NotLinked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, user2Session, http.StatusNotFound}, {"PublicByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, emptySession, http.StatusOK}, From ce4993ae4569d589b874ead98411dd067660c744 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Wed, 18 Dec 2019 02:18:55 +0100 Subject: [PATCH 11/17] test: add unit test TestLinkedRepository --- models/attachment_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/models/attachment_test.go b/models/attachment_test.go index 0dad1b8bba1c..8d27918fa6b7 100644 --- a/models/attachment_test.go +++ b/models/attachment_test.go @@ -128,3 +128,35 @@ func TestGetAttachmentsByUUIDs(t *testing.T) { assert.Equal(t, int64(1), attachList[0].IssueID) assert.Equal(t, int64(5), attachList[1].IssueID) } + +func TestLinkedRepository(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + testCases := []struct { + name string + attachID int64 + expectedErr error + expectedRepo *Repository + expectedUnitType UnitType + }{ + {"LinkedIssue", 1, nil, &Repository{ID: 1}, UnitTypeIssues}, + {"LinkedComment", 3, nil, &Repository{ID: 1}, UnitTypeIssues}, + {"LinkedRelease", 9, nil, &Repository{ID: 1}, UnitTypeReleases}, + {"Notlinked", 10, nil, nil, -1}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + attach, err := GetAttachmentByID(tc.attachID) + assert.NoError(t, err) + repo, unitType, err := attach.LinkedRepository() + if tc.expectedErr == nil { + assert.NoError(t, err) + if tc.expectedRepo != nil { + assert.Equal(t, tc.expectedRepo.ID, repo.ID) + } + assert.Equal(t, tc.expectedUnitType, unitType) + } else { + assert.Equal(t, tc.expectedErr, repo.ID) + } + }) + } +} From 49a18a226ce54736c21bdcbfb43ddfbbf5a249c0 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Thu, 19 Dec 2019 00:55:31 +0100 Subject: [PATCH 12/17] refactor: allow uploader to access unlinked attachement --- integrations/attachment_test.go | 1 + models/fixtures/attachment.yml | 1 + routers/routes/routes.go | 22 ++++++++++++---------- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/integrations/attachment_test.go b/integrations/attachment_test.go index 3fe349d49f85..d5ad19cc42e3 100644 --- a/integrations/attachment_test.go +++ b/integrations/attachment_test.go @@ -110,6 +110,7 @@ func TestGetAttachment(t *testing.T) { {"NotExistingUUID", "b0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, user2Session, http.StatusNotFound}, {"FileMissing", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, user2Session, http.StatusInternalServerError}, {"NotLinked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, user2Session, http.StatusNotFound}, + {"NotLinkedAccessibleByUploader", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, user8Session, http.StatusOK}, {"PublicByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, emptySession, http.StatusOK}, {"PrivateByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, emptySession, http.StatusNotFound}, {"PrivateAccessibleByAdmin", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, adminSession, http.StatusOK}, diff --git a/models/fixtures/attachment.yml b/models/fixtures/attachment.yml index 062856d2c12b..d23599c44c42 100644 --- a/models/fixtures/attachment.yml +++ b/models/fixtures/attachment.yml @@ -81,6 +81,7 @@ - id: 10 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20 + uploader_id: 8 name: attach1 download_count: 0 created_unix: 946684800 \ No newline at end of file diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 2c340ba21869..47c507d806a4 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -509,19 +509,21 @@ func RegisterRoutes(m *macaron.Macaron) { return } - if repository == nil { - ctx.Error(http.StatusNotFound) - return - } - - if repository.IsPrivate { - if !ctx.IsSigned { + if repository == nil { //If not linked + if !(ctx.IsSigned && attach.UploaderID == ctx.User.ID) { //We block if not the uploader ctx.Error(http.StatusNotFound) return } - if !repository.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, unitType) { - ctx.Error(http.StatusForbidden) - return + } else { //If we have the repository we check acces + if repository.IsPrivate { + if !ctx.IsSigned { + ctx.Error(http.StatusNotFound) + return + } + if !repository.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, unitType) { + ctx.Error(http.StatusForbidden) + return + } } } From 06162e364d47547da48b02957c4246b7ddbf6dff Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Thu, 2 Jan 2020 05:09:06 +0100 Subject: [PATCH 13/17] add missing blank line --- integrations/attachment_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integrations/attachment_test.go b/integrations/attachment_test.go index d5ad19cc42e3..216e83d81702 100644 --- a/integrations/attachment_test.go +++ b/integrations/attachment_test.go @@ -18,6 +18,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/test" + "github.com/stretchr/testify/assert" ) From 19b9eb98420e57bd668c153d883c1371ae3d7a0f Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Thu, 2 Jan 2020 05:41:14 +0100 Subject: [PATCH 14/17] refactor: move to a separate function repo.GetAttachment --- routers/repo/attachment.go | 57 ++++++++++++++++++++++++++++++++++++++ routers/routes/routes.go | 55 +----------------------------------- 2 files changed, 58 insertions(+), 54 deletions(-) diff --git a/routers/repo/attachment.go b/routers/repo/attachment.go index 0d496230e16a..e49fc1c8e123 100644 --- a/routers/repo/attachment.go +++ b/routers/repo/attachment.go @@ -6,6 +6,8 @@ package repo import ( "fmt" + "net/http" + "os" "strings" "code.gitea.io/gitea/models" @@ -85,3 +87,58 @@ func DeleteAttachment(ctx *context.Context) { "uuid": attach.UUID, }) } + +// GetAttachment serve attachements +func GetAttachment(ctx *context.Context) { + attach, err := models.GetAttachmentByUUID(ctx.Params(":uuid")) + if err != nil { + if models.IsErrAttachmentNotExist(err) { + ctx.Error(404) + } else { + ctx.ServerError("GetAttachmentByUUID", err) + } + return + } + + repository, unitType, err := attach.LinkedRepository() + if err != nil { + ctx.ServerError("LinkedRepository", err) + return + } + + if repository == nil { //If not linked + if !(ctx.IsSigned && attach.UploaderID == ctx.User.ID) { //We block if not the uploader + ctx.Error(http.StatusNotFound) + return + } + } else { //If we have the repository we check acces + if repository.IsPrivate { + if !ctx.IsSigned { + ctx.Error(http.StatusNotFound) + return + } + if !repository.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, unitType) { + ctx.Error(http.StatusForbidden) + return + } + } + } + + //If we have matched and access to release or issue + fr, err := os.Open(attach.LocalPath()) + if err != nil { + ctx.ServerError("Open", err) + return + } + defer fr.Close() + + if err := attach.IncreaseDownloadCount(); err != nil { + ctx.ServerError("Update", err) + return + } + + if err = ServeData(ctx, attach.Name, fr); err != nil { + ctx.ServerError("ServeData", err) + return + } +} diff --git a/routers/routes/routes.go b/routers/routes/routes.go index e27c07f62f88..888c92ac4aec 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -8,7 +8,6 @@ import ( "bytes" "encoding/gob" "net/http" - "os" "path" "text/template" "time" @@ -474,59 +473,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/following", user.Following) }) - m.Get("/attachments/:uuid", func(ctx *context.Context) { - attach, err := models.GetAttachmentByUUID(ctx.Params(":uuid")) - if err != nil { - if models.IsErrAttachmentNotExist(err) { - ctx.Error(404) - } else { - ctx.ServerError("GetAttachmentByUUID", err) - } - return - } - - repository, unitType, err := attach.LinkedRepository() - if err != nil { - ctx.ServerError("LinkedRepository", err) - return - } - - if repository == nil { //If not linked - if !(ctx.IsSigned && attach.UploaderID == ctx.User.ID) { //We block if not the uploader - ctx.Error(http.StatusNotFound) - return - } - } else { //If we have the repository we check acces - if repository.IsPrivate { - if !ctx.IsSigned { - ctx.Error(http.StatusNotFound) - return - } - if !repository.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, unitType) { - ctx.Error(http.StatusForbidden) - return - } - } - } - - //If we have matched and access to release or issue - fr, err := os.Open(attach.LocalPath()) - if err != nil { - ctx.ServerError("Open", err) - return - } - defer fr.Close() - - if err := attach.IncreaseDownloadCount(); err != nil { - ctx.ServerError("Update", err) - return - } - - if err = repo.ServeData(ctx, attach.Name, fr); err != nil { - ctx.ServerError("ServeData", err) - return - } - }) + m.Get("/attachments/:uuid", repo.GetAttachment) }, ignSignIn) m.Group("/attachments", func() { From 219a4d0199bdfbb4ae55b1bd7c5ba91dbf98d818 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Thu, 2 Jan 2020 05:46:39 +0100 Subject: [PATCH 15/17] typo --- routers/repo/attachment.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/repo/attachment.go b/routers/repo/attachment.go index e49fc1c8e123..f83cdb2b75c8 100644 --- a/routers/repo/attachment.go +++ b/routers/repo/attachment.go @@ -111,7 +111,7 @@ func GetAttachment(ctx *context.Context) { ctx.Error(http.StatusNotFound) return } - } else { //If we have the repository we check acces + } else { //If we have the repository we check access if repository.IsPrivate { if !ctx.IsSigned { ctx.Error(http.StatusNotFound) From dc6856906dffce4a808ad9a2e0c98acfe3295ff9 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Sat, 4 Jan 2020 14:42:56 +0100 Subject: [PATCH 16/17] test: remove err test return --- models/attachment_test.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/models/attachment_test.go b/models/attachment_test.go index 8d27918fa6b7..ddb6abad3212 100644 --- a/models/attachment_test.go +++ b/models/attachment_test.go @@ -134,29 +134,25 @@ func TestLinkedRepository(t *testing.T) { testCases := []struct { name string attachID int64 - expectedErr error expectedRepo *Repository expectedUnitType UnitType }{ - {"LinkedIssue", 1, nil, &Repository{ID: 1}, UnitTypeIssues}, - {"LinkedComment", 3, nil, &Repository{ID: 1}, UnitTypeIssues}, - {"LinkedRelease", 9, nil, &Repository{ID: 1}, UnitTypeReleases}, - {"Notlinked", 10, nil, nil, -1}, + {"LinkedIssue", 1, &Repository{ID: 1}, UnitTypeIssues}, + {"LinkedComment", 3, &Repository{ID: 1}, UnitTypeIssues}, + {"LinkedRelease", 9, &Repository{ID: 1}, UnitTypeReleases}, + {"Notlinked", 10, nil, -1}, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { attach, err := GetAttachmentByID(tc.attachID) assert.NoError(t, err) repo, unitType, err := attach.LinkedRepository() - if tc.expectedErr == nil { - assert.NoError(t, err) - if tc.expectedRepo != nil { - assert.Equal(t, tc.expectedRepo.ID, repo.ID) - } - assert.Equal(t, tc.expectedUnitType, unitType) - } else { - assert.Equal(t, tc.expectedErr, repo.ID) + assert.NoError(t, err) + if tc.expectedRepo != nil { + assert.Equal(t, tc.expectedRepo.ID, repo.ID) } + assert.Equal(t, tc.expectedUnitType, unitType) + }) } } From 149d2f6c5c7ffdb5588317131bc4b1ba972ea100 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Sat, 4 Jan 2020 23:22:15 +0100 Subject: [PATCH 17/17] refactor: use repo perm for access checking generally + 404 for all reject --- integrations/attachment_test.go | 3 ++- models/fixtures/attachment.yml | 10 +++++++++- models/fixtures/release.yml | 17 ++++++++++++++++- routers/repo/attachment.go | 17 ++++++++--------- 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/integrations/attachment_test.go b/integrations/attachment_test.go index 216e83d81702..746256df9554 100644 --- a/integrations/attachment_test.go +++ b/integrations/attachment_test.go @@ -116,7 +116,8 @@ func TestGetAttachment(t *testing.T) { {"PrivateByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, emptySession, http.StatusNotFound}, {"PrivateAccessibleByAdmin", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, adminSession, http.StatusOK}, {"PrivateAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user2Session, http.StatusOK}, - {"NotAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user8Session, http.StatusForbidden}, + {"RepoNotAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user8Session, http.StatusNotFound}, + {"OrgNotAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a21", true, user8Session, http.StatusNotFound}, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { diff --git a/models/fixtures/attachment.yml b/models/fixtures/attachment.yml index d23599c44c42..2606d52b4716 100644 --- a/models/fixtures/attachment.yml +++ b/models/fixtures/attachment.yml @@ -84,4 +84,12 @@ uploader_id: 8 name: attach1 download_count: 0 - created_unix: 946684800 \ No newline at end of file + created_unix: 946684800 + +- + id: 11 + uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a21 + release_id: 2 + name: attach1 + download_count: 0 + created_unix: 946684800 diff --git a/models/fixtures/release.yml b/models/fixtures/release.yml index db9a6b503d45..f95eb048be38 100644 --- a/models/fixtures/release.yml +++ b/models/fixtures/release.yml @@ -11,4 +11,19 @@ is_draft: false is_prerelease: false is_tag: false - created_unix: 946684800 \ No newline at end of file + created_unix: 946684800 + +- + id: 2 + repo_id: 40 + publisher_id: 2 + tag_name: "v1.1" + lower_tag_name: "v1.1" + target: "master" + title: "testing-release" + sha1: "65f1bf27bc3bf70f64657658635e66094edbcb4d" + num_commits: 10 + is_draft: false + is_prerelease: false + is_tag: false + created_unix: 946684800 diff --git a/routers/repo/attachment.go b/routers/repo/attachment.go index f83cdb2b75c8..96dc28a23a9d 100644 --- a/routers/repo/attachment.go +++ b/routers/repo/attachment.go @@ -112,15 +112,14 @@ func GetAttachment(ctx *context.Context) { return } } else { //If we have the repository we check access - if repository.IsPrivate { - if !ctx.IsSigned { - ctx.Error(http.StatusNotFound) - return - } - if !repository.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, unitType) { - ctx.Error(http.StatusForbidden) - return - } + perm, err := models.GetUserRepoPermission(repository, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err.Error()) + return + } + if !perm.CanRead(unitType) { + ctx.Error(http.StatusNotFound) + return } }