From 1ec4dc6c1dec3814d8956bcd1157a51bfe53eb0e Mon Sep 17 00:00:00 2001 From: Lauris BH Date: Sun, 15 Oct 2017 18:06:07 +0300 Subject: [PATCH] Fix so that user can still fork his own repository to owned organizations (#2699) * Fix so that user can still fork his own repository to his organizations * Fix to only use owned organizations * Add integration test for forking own repository to owned organization --- integrations/pull_create_test.go | 2 +- integrations/pull_merge_test.go | 4 ++-- integrations/repo_fork_test.go | 35 +++++++++++++++++++++++++------- models/repo.go | 19 +++++++++++++++++ modules/context/repo.go | 5 +++++ routers/repo/pull.go | 30 +++++++++++++++++++-------- templates/repo/header.tmpl | 2 +- templates/repo/pulls/fork.tmpl | 20 +++++++++--------- 8 files changed, 88 insertions(+), 29 deletions(-) diff --git a/integrations/pull_create_test.go b/integrations/pull_create_test.go index 7db1ce1ec..a62144c61 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", "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 ddb18f802..100298b08 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", "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", "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 bd6ec7915..f8cc2e3f3 100644 --- a/integrations/repo_fork_test.go +++ b/integrations/repo_fork_test.go @@ -5,19 +5,24 @@ package integrations import ( + "fmt" "net/http" "testing" + "code.gitea.io/gitea/models" + "github.com/stretchr/testify/assert" ) -func testRepoFork(t *testing.T, session *TestSession) *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 := 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 +36,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=\"%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": "1", - "repo_name": "repo1", + "uid": fmt.Sprintf("%d", forkOwner.ID), + "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 +55,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", "repo1") +} + +func TestRepoForkToOrg(t *testing.T) { + prepareTestEnv(t) + session := loginUser(t, "user2") + 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 + 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") } diff --git a/models/repo.go b/models/repo.go index a6817f651..9819d4b77 100644 --- a/models/repo.go +++ b/models/repo.go @@ -651,6 +651,25 @@ 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 user == nil { + return false, nil + } + if repo.OwnerID != user.ID && !user.HasForkedRepo(repo.ID) { + return true, nil + } + if err := user.GetOwnedOrganizations(); err != nil { + return false, err + } + for _, org := range user.OwnedOrgs { + if repo.OwnerID != org.ID && !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 788588f8e..88208922c 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 48e17665d..f80e8cb1e 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 := forkRepo.OwnerID != ctx.User.ID && !ctx.User.HasForkedRepo(forkRepo.ID) + ctx.Data["CanForkToUser"] = canForkToUser if err = forkRepo.GetOwner(); err != nil { ctx.Handle(500, "GetOwner", err) @@ -69,11 +71,23 @@ 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 } - ctx.Data["Orgs"] = ctx.User.Orgs + var orgs []*models.User + for _, org := range ctx.User.OwnedOrgs { + if forkRepo.OwnerID != org.ID && !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") + ctxUser := checkContextUser(ctx, form.UID) + if ctx.Written() { + return + } + forkRepo := getForkRepository(ctx) if ctx.Written() { return } - ctxUser := checkContextUser(ctx, form.UID) - if ctx.Written() { - return - } ctx.Data["ContextUser"] = ctxUser if ctx.HasError() { diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index 6baacd537..ee6fcb388 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 133cc92f7..e9fbfdf2b 100644 --- a/templates/repo/pulls/fork.tmpl +++ b/templates/repo/pulls/fork.tmpl @@ -19,17 +19,17 @@
-
- - {{.SignedUser.ShortName 20}} -
+ {{if .CanForkToUser}} +
+ + {{.SignedUser.ShortName 20}} +
+ {{end}} {{range .Orgs}} - {{if and (.IsOwnedBy $.SignedUser.ID) (ne .ID $.ForkFromOwnerID)}} -
- - {{.ShortName 20}} -
- {{end}} +
+ + {{.ShortName 20}} +
{{end}}