Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow maintainers to view and edit files of private repos when "Allow maintainers to edit" is enabled #32215

Merged
merged 7 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -887,8 +887,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
}

if pull.HeadRepo != nil {
ctx.Data["SourcePath"] = pull.HeadRepo.Link() + "/src/commit/" + endCommitID
Copy link
Contributor Author

@Zettat123 Zettat123 Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to edit SourcePath here. Its value is already set by the setCompareContext.

setCompareContext(ctx, baseCommit, commit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)

And if we use the link of head repo here, the maintainer of the base repository won't be able to view the file.


if !pull.HasMerged && ctx.Doer != nil {
perm, err := access_model.GetUserRepoPermission(ctx, pull.HeadRepo, ctx.Doer)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions services/context/permission.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ func RequireRepoWriterOr(unitTypes ...unit.Type) func(ctx *Context) {
func RequireRepoReader(unitType unit.Type) func(ctx *Context) {
return func(ctx *Context) {
if !ctx.Repo.CanRead(unitType) {
if unitType == unit.TypeCode && canWriteAsMaintainer(ctx) {
return
}
if log.IsTrace() {
if ctx.IsSigned {
log.Trace("Permission Denied: User %-v cannot read %-v in Repo %-v\n"+
Expand Down
10 changes: 9 additions & 1 deletion services/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ func repoAssignment(ctx *Context, repo *repo_model.Repository) {
return
}

if !ctx.Repo.Permission.HasAnyUnitAccessOrEveryoneAccess() {
if !ctx.Repo.Permission.HasAnyUnitAccessOrEveryoneAccess() && !canWriteAsMaintainer(ctx) {
if ctx.FormString("go-get") == "1" {
EarlyResponseForGoGetMeta(ctx)
return
Expand Down Expand Up @@ -1058,3 +1058,11 @@ func GitHookService() func(ctx *Context) {
}
}
}

// canWriteAsMaintainer check if the doer can write to a branch as a maintainer
func canWriteAsMaintainer(ctx *Context) bool {
branchName := getRefNameFromPath(ctx.Repo, ctx.PathParam("*"), func(branchName string) bool {
return issues_model.CanMaintainerWriteToBranch(ctx, ctx.Repo.Permission, branchName, ctx.Doer)
})
return len(branchName) > 0
}
78 changes: 78 additions & 0 deletions tests/integration/pull_compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/test"
repo_service "code.gitea.io/gitea/services/repository"
"code.gitea.io/gitea/tests"

Expand Down Expand Up @@ -73,3 +74,80 @@ func TestPullCompare(t *testing.T) {
assert.EqualValues(t, editButtonCount, 0, "Expected not to find a button to edit a file in the PR diff view because head repository has been deleted")
})
}

func TestPullCompare_EnableAllowEditsFromMaintainer(t *testing.T) {
onGiteaRun(t, func(t *testing.T, u *url.URL) {
// repo3 is private
repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
assert.True(t, repo3.IsPrivate)

// user4 forks repo3
user4Session := loginUser(t, "user4")
forkedRepoName := "user4-forked-repo3"
testRepoFork(t, user4Session, repo3.OwnerName, repo3.Name, "user4", forkedRepoName, "")
forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user4", Name: forkedRepoName})
assert.True(t, forkedRepo.IsPrivate)

// user4 creates a new branch and a PR
testEditFileToNewBranch(t, user4Session, "user4", forkedRepoName, "master", "user4/update-readme", "README.md", "Hello, World\n(Edited by user4)\n")
resp := testPullCreateDirectly(t, user4Session, repo3.OwnerName, repo3.Name, "master", "user4", forkedRepoName, "user4/update-readme", "PR for user4 forked repo3")
prURL := test.RedirectURL(resp)

// user2 (admin of repo3) goes to the PR files page
user2Session := loginUser(t, "user2")
resp = user2Session.MakeRequest(t, NewRequest(t, "GET", fmt.Sprintf("%s/files", prURL)), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
nodes := htmlDoc.doc.Find(".diff-file-box[data-new-filename=\"README.md\"] .diff-file-header-actions .dropdown .menu a")
if assert.Equal(t, 1, nodes.Length()) {
// there is only "View File" button, no "Edit File" button
assert.Equal(t, "View File", nodes.First().Text())
viewFileLink, exists := nodes.First().Attr("href")
if assert.True(t, exists) {
user2Session.MakeRequest(t, NewRequest(t, "GET", viewFileLink), http.StatusOK)
}
}

// user4 goes to the PR page and enable "Allow maintainers to edit"
resp = user4Session.MakeRequest(t, NewRequest(t, "GET", prURL), http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)
dataURL, exists := htmlDoc.doc.Find("#allow-edits-from-maintainers").Attr("data-url")
assert.True(t, exists)
req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/set_allow_maintainer_edit", dataURL), map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"allow_maintainer_edit": "true",
})
user4Session.MakeRequest(t, req, http.StatusOK)

// user2 (admin of repo3) goes to the PR files page again
resp = user2Session.MakeRequest(t, NewRequest(t, "GET", fmt.Sprintf("%s/files", prURL)), http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)
nodes = htmlDoc.doc.Find(".diff-file-box[data-new-filename=\"README.md\"] .diff-file-header-actions .dropdown .menu a")
if assert.Equal(t, 2, nodes.Length()) {
// there are "View File" button and "Edit File" button
assert.Equal(t, "View File", nodes.First().Text())
viewFileLink, exists := nodes.First().Attr("href")
if assert.True(t, exists) {
user2Session.MakeRequest(t, NewRequest(t, "GET", viewFileLink), http.StatusOK)
}

assert.Equal(t, "Edit File", nodes.Last().Text())
editFileLink, exists := nodes.Last().Attr("href")
if assert.True(t, exists) {
// edit the file
resp := user2Session.MakeRequest(t, NewRequest(t, "GET", editFileLink), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
lastCommit := htmlDoc.GetInputValueByName("last_commit")
assert.NotEmpty(t, lastCommit)
req := NewRequestWithValues(t, "POST", editFileLink, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"last_commit": lastCommit,
"tree_path": "README.md",
"content": "File is edited by the maintainer user2",
"commit_summary": "user2 updated the file",
"commit_choice": "direct",
})
user2Session.MakeRequest(t, req, http.StatusSeeOther)
}
}
})
}
Loading