From f09a125b708a48a6aaa6c2d854dcfbbc0ff70bb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Sat, 14 Oct 2017 01:37:33 +0300 Subject: [PATCH 1/7] Fix so that user can still fork his own repository to his organizations --- models/repo.go | 16 ++++++++++++++++ modules/context/repo.go | 5 +++++ routers/repo/pull.go | 22 ++++++++++++++++++---- templates/repo/header.tmpl | 2 +- templates/repo/pulls/fork.tmpl | 10 ++++++---- 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/models/repo.go b/models/repo.go index 7ad9ad4dc9139..a5c395c1ff026 100644 --- a/models/repo.go +++ b/models/repo.go @@ -634,6 +634,22 @@ func (repo *Repository) CanBeForked() bool { return !repo.IsBare && repo.UnitEnabled(UnitTypeCode) } +// CanUserFork returns true if specified user can fork repository. +func (repo *Repository) CanUserFork(user *User) (bool, error) { + if repo.OwnerID != user.ID && !user.HasForkedRepo(repo.ID) { + return true, nil + } + if err := user.GetOrganizations(true); err != nil { + return false, err + } + for _, org := range user.Orgs { + if !org.HasForkedRepo(repo.ID) { + return true, nil + } + } + return false, nil +} + // CanEnablePulls returns true if repository meets the requirements of accepting pulls. func (repo *Repository) CanEnablePulls() bool { return !repo.IsMirror && !repo.IsBare diff --git a/modules/context/repo.go b/modules/context/repo.go index ce65bfd04557e..2b3d483837f8c 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -337,6 +337,11 @@ func RepoAssignment() macaron.Handler { ctx.Data["IsRepositoryAdmin"] = ctx.Repo.IsAdmin() ctx.Data["IsRepositoryWriter"] = ctx.Repo.IsWriter() + if ctx.Data["CanSignedUserFork"], err = ctx.Repo.Repository.CanUserFork(ctx.User); err != nil { + ctx.Handle(500, "CanUserFork", err) + return + } + ctx.Data["DisableSSH"] = setting.SSH.Disabled ctx.Data["ExposeAnonSSH"] = setting.SSH.ExposeAnonymous ctx.Data["DisableHTTP"] = setting.Repository.DisableHTTPGit diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 48e17665dac3a..3d51284de9dce 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -61,6 +61,8 @@ func getForkRepository(ctx *context.Context) *models.Repository { ctx.Data["repo_name"] = forkRepo.Name ctx.Data["description"] = forkRepo.Description ctx.Data["IsPrivate"] = forkRepo.IsPrivate + canForkToUser := !ctx.User.HasForkedRepo(forkRepo.ID) + ctx.Data["CanForkToUser"] = canForkToUser if err = forkRepo.GetOwner(); err != nil { ctx.Handle(500, "GetOwner", err) @@ -73,7 +75,19 @@ func getForkRepository(ctx *context.Context) *models.Repository { ctx.Handle(500, "GetOrganizations", err) return nil } - ctx.Data["Orgs"] = ctx.User.Orgs + var orgs []*models.User + for _, org := range ctx.User.Orgs { + if !org.HasForkedRepo(forkRepo.ID) { + orgs = append(orgs, org) + } + } + ctx.Data["Orgs"] = orgs + + if canForkToUser { + ctx.Data["ContextUser"] = ctx.User + } else if len(orgs) > 0 { + ctx.Data["ContextUser"] = orgs[0] + } return forkRepo } @@ -87,7 +101,6 @@ func Fork(ctx *context.Context) { return } - ctx.Data["ContextUser"] = ctx.User ctx.HTML(200, tplFork) } @@ -95,15 +108,16 @@ func Fork(ctx *context.Context) { func ForkPost(ctx *context.Context, form auth.CreateRepoForm) { ctx.Data["Title"] = ctx.Tr("new_fork") - forkRepo := getForkRepository(ctx) + ctxUser := checkContextUser(ctx, form.UID) if ctx.Written() { return } - ctxUser := checkContextUser(ctx, form.UID) + forkRepo := getForkRepository(ctx) if ctx.Written() { return } + ctx.Data["ContextUser"] = ctxUser if ctx.HasError() { diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index 2ed93d1d930ad..15b871c1bb81c 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -32,7 +32,7 @@ {{if .CanBeForked}}
- + {{$.i18n.Tr "repo.fork"}} diff --git a/templates/repo/pulls/fork.tmpl b/templates/repo/pulls/fork.tmpl index 133cc92f7cb08..7d7f3476c1121 100644 --- a/templates/repo/pulls/fork.tmpl +++ b/templates/repo/pulls/fork.tmpl @@ -19,10 +19,12 @@
-
- - {{.SignedUser.ShortName 20}} -
+ {{if .CanForkToUser}} +
+ + {{.SignedUser.ShortName 20}} +
+ {{end}} {{range .Orgs}} {{if and (.IsOwnedBy $.SignedUser.ID) (ne .ID $.ForkFromOwnerID)}}
From d13427dd48d7fc20fd35be0bf6e8b2525bd9f803 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Sat, 14 Oct 2017 01:59:01 +0300 Subject: [PATCH 2/7] Add nil check --- models/repo.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/models/repo.go b/models/repo.go index a5c395c1ff026..10a4352388453 100644 --- a/models/repo.go +++ b/models/repo.go @@ -636,6 +636,9 @@ func (repo *Repository) CanBeForked() bool { // CanUserFork returns true if specified user can fork repository. func (repo *Repository) CanUserFork(user *User) (bool, error) { + if user == nil { + return false, nil + } if repo.OwnerID != user.ID && !user.HasForkedRepo(repo.ID) { return true, nil } From 0f894db6c78fff3ea6484d4162bfd91ec670e2aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Sat, 14 Oct 2017 22:00:50 +0300 Subject: [PATCH 3/7] Fix to only use owned organizations --- models/repo.go | 4 ++-- routers/repo/pull.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/models/repo.go b/models/repo.go index 10a4352388453..c58f5f13c7e17 100644 --- a/models/repo.go +++ b/models/repo.go @@ -642,10 +642,10 @@ func (repo *Repository) CanUserFork(user *User) (bool, error) { if repo.OwnerID != user.ID && !user.HasForkedRepo(repo.ID) { return true, nil } - if err := user.GetOrganizations(true); err != nil { + if err := user.GetOwnedOrganizations(); err != nil { return false, err } - for _, org := range user.Orgs { + for _, org := range user.OwnedOrgs { if !org.HasForkedRepo(repo.ID) { return true, nil } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 3d51284de9dce..a3a4ba184da60 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -61,7 +61,7 @@ func getForkRepository(ctx *context.Context) *models.Repository { ctx.Data["repo_name"] = forkRepo.Name ctx.Data["description"] = forkRepo.Description ctx.Data["IsPrivate"] = forkRepo.IsPrivate - canForkToUser := !ctx.User.HasForkedRepo(forkRepo.ID) + canForkToUser := forkRepo.OwnerID != ctx.User.ID && !ctx.User.HasForkedRepo(forkRepo.ID) ctx.Data["CanForkToUser"] = canForkToUser if err = forkRepo.GetOwner(); err != nil { @@ -71,12 +71,12 @@ func getForkRepository(ctx *context.Context) *models.Repository { ctx.Data["ForkFrom"] = forkRepo.Owner.Name + "/" + forkRepo.Name ctx.Data["ForkFromOwnerID"] = forkRepo.Owner.ID - if err := ctx.User.GetOrganizations(true); err != nil { - ctx.Handle(500, "GetOrganizations", err) + if err := ctx.User.GetOwnedOrganizations(); err != nil { + ctx.Handle(500, "GetOwnedOrganizations", err) return nil } var orgs []*models.User - for _, org := range ctx.User.Orgs { + for _, org := range ctx.User.OwnedOrgs { if !org.HasForkedRepo(forkRepo.ID) { orgs = append(orgs, org) } From 2b0d671a4059a86f2312aaf8e0dfa4c91f8ea17c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Sat, 14 Oct 2017 22:07:44 +0300 Subject: [PATCH 4/7] Small code improvement --- models/repo.go | 2 +- routers/repo/pull.go | 2 +- templates/repo/pulls/fork.tmpl | 10 ++++------ 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/models/repo.go b/models/repo.go index c58f5f13c7e17..1a7f65e7cfb83 100644 --- a/models/repo.go +++ b/models/repo.go @@ -646,7 +646,7 @@ func (repo *Repository) CanUserFork(user *User) (bool, error) { return false, err } for _, org := range user.OwnedOrgs { - if !org.HasForkedRepo(repo.ID) { + if repo.OwnerID != org.ID && !org.HasForkedRepo(repo.ID) { return true, nil } } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index a3a4ba184da60..f80e8cb1e532b 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -77,7 +77,7 @@ func getForkRepository(ctx *context.Context) *models.Repository { } var orgs []*models.User for _, org := range ctx.User.OwnedOrgs { - if !org.HasForkedRepo(forkRepo.ID) { + if forkRepo.OwnerID != org.ID && !org.HasForkedRepo(forkRepo.ID) { orgs = append(orgs, org) } } diff --git a/templates/repo/pulls/fork.tmpl b/templates/repo/pulls/fork.tmpl index 7d7f3476c1121..e9fbfdf2b61a5 100644 --- a/templates/repo/pulls/fork.tmpl +++ b/templates/repo/pulls/fork.tmpl @@ -26,12 +26,10 @@
{{end}} {{range .Orgs}} - {{if and (.IsOwnedBy $.SignedUser.ID) (ne .ID $.ForkFromOwnerID)}} -
- - {{.ShortName 20}} -
- {{end}} +
+ + {{.ShortName 20}} +
{{end}}
From f86bd84a97370a61caa73f89c3eed9711356a96a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Sun, 15 Oct 2017 00:04:45 +0300 Subject: [PATCH 5/7] Add integration test for forkin own repository to owned organization --- integrations/pull_create_test.go | 2 +- integrations/pull_merge_test.go | 4 ++-- integrations/repo_fork_test.go | 31 ++++++++++++++++++++++++------- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/integrations/pull_create_test.go b/integrations/pull_create_test.go index 7db1ce1ecfd01..730496c341e04 100644 --- a/integrations/pull_create_test.go +++ b/integrations/pull_create_test.go @@ -46,7 +46,7 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo, branch strin func TestPullCreate(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session) + testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md") testPullCreate(t, session, "user1", "repo1", "master") } diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index ddb18f80248d2..1ab6af88bfde1 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -48,7 +48,7 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str func TestPullMerge(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session) + testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md") resp := testPullCreate(t, session, "user1", "repo1", "master") @@ -61,7 +61,7 @@ func TestPullMerge(t *testing.T) { func TestPullCleanUpAfterMerge(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session) + testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md") resp := testPullCreate(t, session, "user1", "repo1", "feature/test") diff --git a/integrations/repo_fork_test.go b/integrations/repo_fork_test.go index bd6ec7915512a..91c332f002f8d 100644 --- a/integrations/repo_fork_test.go +++ b/integrations/repo_fork_test.go @@ -5,19 +5,20 @@ package integrations import ( + "fmt" "net/http" "testing" "github.com/stretchr/testify/assert" ) -func testRepoFork(t *testing.T, session *TestSession) *TestResponse { +func testRepoFork(t *testing.T, session *TestSession, ownerName, repoName, forkOwnerName, forkOwnerID, forkRepoName string) *TestResponse { // Step0: check the existence of the to-fork repo - req := NewRequest(t, "GET", "/user1/repo1") + req := NewRequestf(t, "GET", "/%s/%s", forkOwnerName, forkRepoName) resp := session.MakeRequest(t, req, http.StatusNotFound) // Step1: go to the main page of repo - req = NewRequest(t, "GET", "/user2/repo1") + req = NewRequestf(t, "GET", "/%s/%s", ownerName, repoName) resp = session.MakeRequest(t, req, http.StatusOK) // Step2: click the fork button @@ -31,15 +32,17 @@ func testRepoFork(t *testing.T, session *TestSession) *TestResponse { htmlDoc = NewHTMLParser(t, resp.Body) link, exists = htmlDoc.doc.Find("form.ui.form[action^=\"/repo/fork/\"]").Attr("action") assert.True(t, exists, "The template has changed") + _, exists = htmlDoc.doc.Find(fmt.Sprintf(".owner.dropdown .item[data-value=\"%s\"]", forkOwnerID)).Attr("data-value") + assert.True(t, exists, fmt.Sprintf("Fork owner '%s' is not present in select box", forkOwnerName)) req = NewRequestWithValues(t, "POST", link, map[string]string{ "_csrf": htmlDoc.GetCSRF(), - "uid": "1", - "repo_name": "repo1", + "uid": forkOwnerID, + "repo_name": forkRepoName, }) resp = session.MakeRequest(t, req, http.StatusFound) // Step4: check the existence of the forked repo - req = NewRequest(t, "GET", "/user1/repo1") + req = NewRequestf(t, "GET", "/%s/%s", forkOwnerName, forkRepoName) resp = session.MakeRequest(t, req, http.StatusOK) return resp @@ -48,5 +51,19 @@ func testRepoFork(t *testing.T, session *TestSession) *TestResponse { func TestRepoFork(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session) + testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") +} + +func TestRepoForkToOrg(t *testing.T) { + prepareTestEnv(t) + session := loginUser(t, "user2") + testRepoFork(t, session, "user2", "repo1", "user3", "3", "repo1") + + // Check that no more forking is allowed as user2 owns repository + // and user3 organization that owner user2 is also no has forked this repository + req := NewRequest(t, "GET", "/user2/repo1") + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + _, exists := htmlDoc.doc.Find("a.ui.button[href^=\"/repo/fork/\"]").Attr("href") + assert.False(t, exists, "Forking should not be allowed anymore") } From fa0d64ed85acaebc01869fc8663624634360c51a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Sun, 15 Oct 2017 00:07:27 +0300 Subject: [PATCH 6/7] Fix typo --- integrations/repo_fork_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrations/repo_fork_test.go b/integrations/repo_fork_test.go index 91c332f002f8d..20ecb68c6b9fd 100644 --- a/integrations/repo_fork_test.go +++ b/integrations/repo_fork_test.go @@ -60,7 +60,7 @@ func TestRepoForkToOrg(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user3", "3", "repo1") // Check that no more forking is allowed as user2 owns repository - // and user3 organization that owner user2 is also no has forked this repository + // and user3 organization that owner user2 is also now has forked this repository req := NewRequest(t, "GET", "/user2/repo1") resp := session.MakeRequest(t, req, http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) From f102aadba7314a7068f97f1c13038efc7701e84a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Sun, 15 Oct 2017 00:29:21 +0300 Subject: [PATCH 7/7] Fix fork tests to load user ID by user name --- integrations/pull_create_test.go | 2 +- integrations/pull_merge_test.go | 4 ++-- integrations/repo_fork_test.go | 14 +++++++++----- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/integrations/pull_create_test.go b/integrations/pull_create_test.go index 730496c341e04..a62144c613fba 100644 --- a/integrations/pull_create_test.go +++ b/integrations/pull_create_test.go @@ -46,7 +46,7 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo, branch strin func TestPullCreate(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md") testPullCreate(t, session, "user1", "repo1", "master") } diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index 1ab6af88bfde1..100298b083293 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -48,7 +48,7 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str func TestPullMerge(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md") resp := testPullCreate(t, session, "user1", "repo1", "master") @@ -61,7 +61,7 @@ func TestPullMerge(t *testing.T) { func TestPullCleanUpAfterMerge(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md") resp := testPullCreate(t, session, "user1", "repo1", "feature/test") diff --git a/integrations/repo_fork_test.go b/integrations/repo_fork_test.go index 20ecb68c6b9fd..f8cc2e3f3b4d3 100644 --- a/integrations/repo_fork_test.go +++ b/integrations/repo_fork_test.go @@ -9,10 +9,14 @@ import ( "net/http" "testing" + "code.gitea.io/gitea/models" + "github.com/stretchr/testify/assert" ) -func testRepoFork(t *testing.T, session *TestSession, ownerName, repoName, forkOwnerName, forkOwnerID, forkRepoName string) *TestResponse { +func testRepoFork(t *testing.T, session *TestSession, ownerName, repoName, forkOwnerName, forkRepoName string) *TestResponse { + forkOwner := models.AssertExistsAndLoadBean(t, &models.User{Name: forkOwnerName}).(*models.User) + // Step0: check the existence of the to-fork repo req := NewRequestf(t, "GET", "/%s/%s", forkOwnerName, forkRepoName) resp := session.MakeRequest(t, req, http.StatusNotFound) @@ -32,11 +36,11 @@ func testRepoFork(t *testing.T, session *TestSession, ownerName, repoName, forkO htmlDoc = NewHTMLParser(t, resp.Body) link, exists = htmlDoc.doc.Find("form.ui.form[action^=\"/repo/fork/\"]").Attr("action") assert.True(t, exists, "The template has changed") - _, exists = htmlDoc.doc.Find(fmt.Sprintf(".owner.dropdown .item[data-value=\"%s\"]", forkOwnerID)).Attr("data-value") + _, exists = htmlDoc.doc.Find(fmt.Sprintf(".owner.dropdown .item[data-value=\"%d\"]", forkOwner.ID)).Attr("data-value") assert.True(t, exists, fmt.Sprintf("Fork owner '%s' is not present in select box", forkOwnerName)) req = NewRequestWithValues(t, "POST", link, map[string]string{ "_csrf": htmlDoc.GetCSRF(), - "uid": forkOwnerID, + "uid": fmt.Sprintf("%d", forkOwner.ID), "repo_name": forkRepoName, }) resp = session.MakeRequest(t, req, http.StatusFound) @@ -51,13 +55,13 @@ func testRepoFork(t *testing.T, session *TestSession, ownerName, repoName, forkO func TestRepoFork(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user1") - testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") } func TestRepoForkToOrg(t *testing.T) { prepareTestEnv(t) session := loginUser(t, "user2") - testRepoFork(t, session, "user2", "repo1", "user3", "3", "repo1") + testRepoFork(t, session, "user2", "repo1", "user3", "repo1") // Check that no more forking is allowed as user2 owns repository // and user3 organization that owner user2 is also now has forked this repository