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

@@ -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)

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)
+}

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 {

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 {
+		teams, err := ctx.Repo.Owner.TeamsHaveAccessToRepo(ctx.Repo.Repository.ID, models.ACCESS_MODE_WRITE)
-			ctx.Handle(500, "Repo.Owner.GetTeams", err)
+		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
 	}
 

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}}">
-														<div class="item" data-value="{{.ID}}">
+														<i class="octicon octicon-jersey"></i>
-															<i class="octicon octicon-jersey"></i>
+														{{.Name}}
-															{{.Name}}
+													</div>
-														</div>
-													{{end}}
 												{{end}}
 											</div>
 										</div>