From 0696d430c9baa409297dc06ffa0ab84c0ea44a29 Mon Sep 17 00:00:00 2001
From: Unknwon <u@gogs.io>
Date: Fri, 24 Feb 2017 13:19:42 -0500
Subject: [PATCH] protect_branch: only list teams have write access

List teams without write access to the repository cause confusion
to make users think members of team could push to the branch.
---
 models/org.go                                 |  5 +++++
 models/org_team.go                            | 16 +++++++++++++---
 models/repo_branch.go                         | 10 ++++++----
 routers/repo/setting.go                       |  7 ++++---
 templates/repo/settings/protected_branch.tmpl | 12 +++++-------
 5 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/models/org.go b/models/org.go
index 3d08458b6..84bfd2f3e 100644
--- a/models/org.go
+++ b/models/org.go
@@ -59,6 +59,11 @@ func (org *User) GetTeams() error {
 	return org.getTeams(x)
 }
 
+// TeamsHaveAccessToRepo returns all teamsthat have given access level to the repository.
+func (org *User) TeamsHaveAccessToRepo(repoID int64, mode AccessMode) ([]*Team, error) {
+	return GetTeamsHaveAccessToRepo(org.ID, repoID, mode)
+}
+
 // GetMembers returns all members of organization.
 func (org *User) GetMembers() error {
 	ous, err := GetOrgUsersByOrgID(org.ID)
diff --git a/models/org_team.go b/models/org_team.go
index 9d2835cb0..d4a6b1e37 100644
--- a/models/org_team.go
+++ b/models/org_team.go
@@ -615,18 +615,18 @@ func RemoveTeamMember(orgID, teamID, uid int64) error {
 
 // TeamRepo represents an team-repository relation.
 type TeamRepo struct {
-	ID     int64 `xorm:"pk autoincr"`
+	ID     int64
 	OrgID  int64 `xorm:"INDEX"`
 	TeamID int64 `xorm:"UNIQUE(s)"`
 	RepoID int64 `xorm:"UNIQUE(s)"`
 }
 
 func hasTeamRepo(e Engine, orgID, teamID, repoID int64) bool {
-	has, _ := e.Where("org_id=?", orgID).And("team_id=?", teamID).And("repo_id=?", repoID).Get(new(TeamRepo))
+	has, _ := e.Where("org_id = ?", orgID).And("team_id = ?", teamID).And("repo_id = ?", repoID).Get(new(TeamRepo))
 	return has
 }
 
-// HasTeamRepo returns true if given repository belongs to team.
+// HasTeamRepo returns true if given team has access to the repository of the organization.
 func HasTeamRepo(orgID, teamID, repoID int64) bool {
 	return hasTeamRepo(x, orgID, teamID, repoID)
 }
@@ -657,3 +657,13 @@ func removeTeamRepo(e Engine, teamID, repoID int64) error {
 func RemoveTeamRepo(teamID, repoID int64) error {
 	return removeTeamRepo(x, teamID, repoID)
 }
+
+// GetTeamsHaveAccessToRepo returns all teams in an organization that have given access level to the repository.
+func GetTeamsHaveAccessToRepo(orgID, repoID int64, mode AccessMode) ([]*Team, error) {
+	teams := make([]*Team, 0, 5)
+	return teams, x.Where("team.authorize >= ?", mode).
+		Join("INNER", "team_repo", "team_repo.team_id = team.id").
+		And("team_repo.org_id = ?", orgID).
+		And("team_repo.repo_id = ?", repoID).
+		Find(&teams)
+}
diff --git a/models/repo_branch.go b/models/repo_branch.go
index 77e1db7f4..896847394 100644
--- a/models/repo_branch.go
+++ b/models/repo_branch.go
@@ -171,9 +171,9 @@ func UpdateOrgProtectBranch(repo *Repository, protectBranch *ProtectBranch, whit
 	if protectBranch.WhitelistTeamIDs != whitelistTeamIDs {
 		hasTeamsChanged = true
 		teamIDs := base.StringsToInt64s(strings.Split(whitelistTeamIDs, ","))
-		teams, err := GetTeamsByOrgID(repo.OwnerID)
+		teams, err := GetTeamsHaveAccessToRepo(repo.OwnerID, repo.ID, ACCESS_MODE_WRITE)
 		if err != nil {
-			return fmt.Errorf("GetTeamsByOrgID [org_id: %d]: %v", repo.OwnerID, err)
+			return fmt.Errorf("GetTeamsHaveAccessToRepo [org_id: %d, repo_id: %d]: %v", repo.OwnerID, repo.ID, err)
 		}
 		validTeamIDs = make([]int64, 0, len(teams))
 		for i := range teams {
@@ -190,7 +190,10 @@ func UpdateOrgProtectBranch(repo *Repository, protectBranch *ProtectBranch, whit
 	if hasUsersChanged || hasTeamsChanged {
 		mergedUserIDs := make(map[int64]bool)
 		for _, userID := range validUserIDs {
-			mergedUserIDs[userID] = true
+			// Empty whitelist users can cause an ID with 0
+			if userID != 0 {
+				mergedUserIDs[userID] = true
+			}
 		}
 
 		for _, teamID := range validTeamIDs {
@@ -225,7 +228,6 @@ func UpdateOrgProtectBranch(repo *Repository, protectBranch *ProtectBranch, whit
 		if _, err = sess.Insert(protectBranch); err != nil {
 			return fmt.Errorf("Insert: %v", err)
 		}
-		return
 	}
 
 	if _, err = sess.Id(protectBranch.ID).AllCols().Update(protectBranch); err != nil {
diff --git a/routers/repo/setting.go b/routers/repo/setting.go
index eb8048c50..7cafe3e3b 100644
--- a/routers/repo/setting.go
+++ b/routers/repo/setting.go
@@ -438,11 +438,12 @@ func SettingsProtectedBranch(ctx *context.Context) {
 		ctx.Data["Users"] = users
 		ctx.Data["whitelist_users"] = protectBranch.WhitelistUserIDs
 
-		if err = ctx.Repo.Owner.GetTeams(); err != nil {
-			ctx.Handle(500, "Repo.Owner.GetTeams", err)
+		teams, err := ctx.Repo.Owner.TeamsHaveAccessToRepo(ctx.Repo.Repository.ID, models.ACCESS_MODE_WRITE)
+		if err != nil {
+			ctx.Handle(500, "Repo.Owner.TeamsHaveAccessToRepo", err)
 			return
 		}
-		ctx.Data["Teams"] = ctx.Repo.Owner.Teams
+		ctx.Data["Teams"] = teams
 		ctx.Data["whitelist_teams"] = protectBranch.WhitelistTeamIDs
 	}
 
diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl
index 979939fa8..992d3e79d 100644
--- a/templates/repo/settings/protected_branch.tmpl
+++ b/templates/repo/settings/protected_branch.tmpl
@@ -46,7 +46,7 @@
 												{{range .Users}}
 													<div class="item" data-value="{{.ID}}">
 														<img class="ui mini image" src="{{.RelAvatarLink}}">
-														{{.Name}}
+														{{.DisplayName}}
 													</div>
 												{{end}}
 											</div>
@@ -60,12 +60,10 @@
 											<div class="default text">{{.i18n.Tr "repo.settings.protect_whitelist_search_teams"}}</div>
 											<div class="menu">
 												{{range .Teams}}
-													{{if and (not .IsOwnerTeam) .HasWriteAccess}}
-														<div class="item" data-value="{{.ID}}">
-															<i class="octicon octicon-jersey"></i>
-															{{.Name}}
-														</div>
-													{{end}}
+													<div class="item" data-value="{{.ID}}">
+														<i class="octicon octicon-jersey"></i>
+														{{.Name}}
+													</div>
 												{{end}}
 											</div>
 										</div>