Fix admin team access mode value in team_unit table (#24012)

Same as https://github.com/go-gitea/gitea/pull/23675
Feedback:
https://github.com/go-gitea/gitea/pull/23879#issuecomment-1500923636

models/migrations/migrations.go

@@ -481,6 +481,8 @@ var migrations = []Migration{
 	NewMigration("Change Container Metadata", v1_20.ChangeContainerMetadataMultiArch),
 	// v251 -> v252
 	NewMigration("Fix incorrect owner team unit access mode", v1_20.FixIncorrectOwnerTeamUnitAccessMode),
+	// v252 -> v253
+	NewMigration("Fix incorrect admin team unit access mode", v1_20.FixIncorrectAdminTeamUnitAccessMode),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v1_20/v252.go

@@ -0,0 +1,47 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_20 //nolint
+
+import (
+	"code.gitea.io/gitea/modules/log"
+
+	"xorm.io/xorm"
+)
+
+func FixIncorrectAdminTeamUnitAccessMode(x *xorm.Engine) error {
+	type UnitType int
+	type AccessMode int
+
+	type TeamUnit struct {
+		ID         int64    `xorm:"pk autoincr"`
+		OrgID      int64    `xorm:"INDEX"`
+		TeamID     int64    `xorm:"UNIQUE(s)"`
+		Type       UnitType `xorm:"UNIQUE(s)"`
+		AccessMode AccessMode
+	}
+
+	const (
+		// AccessModeAdmin admin access
+		AccessModeAdmin = 3
+	)
+
+	sess := x.NewSession()
+	defer sess.Close()
+
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+
+	count, err := sess.Table("team_unit").
+		Where("team_id IN (SELECT id FROM team WHERE authorize = ?)", AccessModeAdmin).
+		Update(&TeamUnit{
+			AccessMode: AccessModeAdmin,
+		})
+	if err != nil {
+		return err
+	}
+	log.Debug("Updated %d admin team unit access mode to belong to admin instead of none", count)
+
+	return sess.Commit()
+}

routers/api/v1/org/team.go

@@ -166,6 +166,21 @@ func attachTeamUnitsMap(team *organization.Team, unitsMap map[string]string) {
 	}
 }
 
+func attachAdminTeamUnits(team *organization.Team) {
+	team.Units = make([]*organization.TeamUnit, 0, len(unit_model.AllRepoUnitTypes))
+	for _, ut := range unit_model.AllRepoUnitTypes {
+		up := perm.AccessModeAdmin
+		if ut == unit_model.TypeExternalTracker || ut == unit_model.TypeExternalWiki {
+			up = perm.AccessModeRead
+		}
+		team.Units = append(team.Units, &organization.TeamUnit{
+			OrgID:      team.OrgID,
+			Type:       ut,
+			AccessMode: up,
+		})
+	}
+}
+
 // CreateTeam api for create a team
 func CreateTeam(ctx *context.APIContext) {
 	// swagger:operation POST /orgs/{org}/teams organization orgCreateTeam
@@ -213,6 +228,8 @@ func CreateTeam(ctx *context.APIContext) {
 			ctx.Error(http.StatusInternalServerError, "getTeamUnits", errors.New("units permission should not be empty"))
 			return
 		}
+	} else {
+		attachAdminTeamUnits(team)
 	}
 
 	if err := models.NewTeam(team); err != nil {
@@ -300,6 +317,8 @@ func EditTeam(ctx *context.APIContext) {
 		} else if len(form.Units) > 0 {
 			attachTeamUnits(team, form.Units)
 		}
+	} else {
+		attachAdminTeamUnits(team)
 	}
 
 	if err := models.UpdateTeam(team, isAuthChanged, isIncludeAllChanged); err != nil {

routers/web/org/teams.go

@@ -5,6 +5,7 @@
 package org
 
 import (
+	"fmt"
 	"net/http"
 	"net/url"
 	"path"
@@ -264,14 +265,26 @@ func NewTeam(ctx *context.Context) {
 	ctx.HTML(http.StatusOK, tplTeamNew)
 }
 
-func getUnitPerms(forms url.Values) map[unit_model.Type]perm.AccessMode {
+func getUnitPerms(forms url.Values, teamPermission perm.AccessMode) map[unit_model.Type]perm.AccessMode {
 	unitPerms := make(map[unit_model.Type]perm.AccessMode)
-	for k, v := range forms {
-		if strings.HasPrefix(k, "unit_") {
-			t, _ := strconv.Atoi(k[5:])
-			if t > 0 {
-				vv, _ := strconv.Atoi(v[0])
-				unitPerms[unit_model.Type(t)] = perm.AccessMode(vv)
+	for _, ut := range unit_model.AllRepoUnitTypes {
+		// Default accessmode is none
+		unitPerms[ut] = perm.AccessModeNone
+
+		v, ok := forms[fmt.Sprintf("unit_%d", ut)]
+		if ok {
+			vv, _ := strconv.Atoi(v[0])
+			if teamPermission >= perm.AccessModeAdmin {
+				unitPerms[ut] = teamPermission
+				// Don't allow `TypeExternal{Tracker,Wiki}` to influence this as they can only be set to READ perms.
+				if ut == unit_model.TypeExternalTracker || ut == unit_model.TypeExternalWiki {
+					unitPerms[ut] = perm.AccessModeRead
+				}
+			} else {
+				unitPerms[ut] = perm.AccessMode(vv)
+				if unitPerms[ut] >= perm.AccessModeAdmin {
+					unitPerms[ut] = perm.AccessModeWrite
+				}
 			}
 		}
 	}
@@ -282,8 +295,8 @@ func getUnitPerms(forms url.Values) map[unit_model.Type]perm.AccessMode {
 func NewTeamPost(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.CreateTeamForm)
 	includesAllRepositories := form.RepoAccess == "all"
-	unitPerms := getUnitPerms(ctx.Req.Form)
 	p := perm.ParseAccessMode(form.Permission)
+	unitPerms := getUnitPerms(ctx.Req.Form, p)
 	if p < perm.AccessModeAdmin {
 		// if p is less than admin accessmode, then it should be general accessmode,
 		// so we should calculate the minial accessmode from units accessmodes.
@@ -299,17 +312,15 @@ func NewTeamPost(ctx *context.Context) {
 		CanCreateOrgRepo:        form.CanCreateOrgRepo,
 	}
 
-	if t.AccessMode < perm.AccessModeAdmin {
-		units := make([]*org_model.TeamUnit, 0, len(unitPerms))
-		for tp, perm := range unitPerms {
-			units = append(units, &org_model.TeamUnit{
-				OrgID:      ctx.Org.Organization.ID,
-				Type:       tp,
-				AccessMode: perm,
-			})
-		}
-		t.Units = units
+	units := make([]*org_model.TeamUnit, 0, len(unitPerms))
+	for tp, perm := range unitPerms {
+		units = append(units, &org_model.TeamUnit{
+			OrgID:      ctx.Org.Organization.ID,
+			Type:       tp,
+			AccessMode: perm,
+		})
 	}
+	t.Units = units
 
 	ctx.Data["Title"] = ctx.Org.Organization.FullName
 	ctx.Data["PageIsOrgTeams"] = true
@@ -422,8 +433,11 @@ func SearchTeam(ctx *context.Context) {
 func EditTeam(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Org.Organization.FullName
 	ctx.Data["PageIsOrgTeams"] = true
-	ctx.Data["team_name"] = ctx.Org.Team.Name
-	ctx.Data["desc"] = ctx.Org.Team.Description
+	if err := ctx.Org.Team.LoadUnits(ctx); err != nil {
+		ctx.ServerError("LoadUnits", err)
+		return
+	}
+	ctx.Data["Team"] = ctx.Org.Team
 	ctx.Data["Units"] = unit_model.Units
 	ctx.HTML(http.StatusOK, tplTeamNew)
 }
@@ -432,7 +446,13 @@ func EditTeam(ctx *context.Context) {
 func EditTeamPost(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.CreateTeamForm)
 	t := ctx.Org.Team
-	unitPerms := getUnitPerms(ctx.Req.Form)
+	newAccessMode := perm.ParseAccessMode(form.Permission)
+	unitPerms := getUnitPerms(ctx.Req.Form, newAccessMode)
+	if newAccessMode < perm.AccessModeAdmin {
+		// if newAccessMode is less than admin accessmode, then it should be general accessmode,
+		// so we should calculate the minial accessmode from units accessmodes.
+		newAccessMode = unit_model.MinUnitAccessMode(unitPerms)
+	}
 	isAuthChanged := false
 	isIncludeAllChanged := false
 	includesAllRepositories := form.RepoAccess == "all"
@@ -443,14 +463,6 @@ func EditTeamPost(ctx *context.Context) {
 	ctx.Data["Units"] = unit_model.Units
 
 	if !t.IsOwnerTeam() {
-		// Validate permission level.
-		newAccessMode := perm.ParseAccessMode(form.Permission)
-		if newAccessMode < perm.AccessModeAdmin {
-			// if p is less than admin accessmode, then it should be general accessmode,
-			// so we should calculate the minial accessmode from units accessmodes.
-			newAccessMode = unit_model.MinUnitAccessMode(unitPerms)
-		}
-
 		t.Name = form.TeamName
 		if t.AccessMode != newAccessMode {
 			isAuthChanged = true
@@ -467,21 +479,16 @@ func EditTeamPost(ctx *context.Context) {
 	}
 
 	t.Description = form.Description
-	if t.AccessMode < perm.AccessModeAdmin {
-		units := make([]org_model.TeamUnit, 0, len(unitPerms))
-		for tp, perm := range unitPerms {
-			units = append(units, org_model.TeamUnit{
-				OrgID:      t.OrgID,
-				TeamID:     t.ID,
-				Type:       tp,
-				AccessMode: perm,
-			})
-		}
-		if err := org_model.UpdateTeamUnits(t, units); err != nil {
-			ctx.Error(http.StatusInternalServerError, "UpdateTeamUnits", err.Error())
-			return
-		}
+	units := make([]*org_model.TeamUnit, 0, len(unitPerms))
+	for tp, perm := range unitPerms {
+		units = append(units, &org_model.TeamUnit{
+			OrgID:      t.OrgID,
+			TeamID:     t.ID,
+			Type:       tp,
+			AccessMode: perm,
+		})
 	}
+	t.Units = units
 
 	if ctx.HasError() {
 		ctx.HTML(http.StatusOK, tplTeamNew)

templates/org/team/new.tmpl

@@ -109,7 +109,7 @@
 													</td>
 													<td class="center aligned">
 														<div class="ui radio checkbox">
-															<input type="radio" name="unit_{{$unit.Type.Value}}" value="2"{{if (eq ($.Team.UnitAccessMode $.Context $unit.Type) 2)}} checked{{end}} {{if $unit.Type.UnitGlobalDisabled}}disabled{{end}} title="{{$.locale.Tr "org.teams.write_access"}}">
+															<input type="radio" name="unit_{{$unit.Type.Value}}" value="2"{{if (ge ($.Team.UnitAccessMode $.Context $unit.Type) 2)}} checked{{end}} {{if $unit.Type.UnitGlobalDisabled}}disabled{{end}} title="{{$.locale.Tr "org.teams.write_access"}}">
 														</div>
 													</td>
 												</tr>
@@ -137,7 +137,7 @@
 							{{else}}
 								<button class="ui green button">{{.locale.Tr "org.teams.update_settings"}}</button>
 								{{if not (eq .Team.LowerName "owners")}}
-									<button class="ui red button delete-button" data-url="{{.OrgLink}}/teams/{{.team_name | PathEscape}}/delete">{{.locale.Tr "org.teams.delete_team"}}</button>
+									<button class="ui red button delete-button" data-url="{{.OrgLink}}/teams/{{.Team.Name | PathEscape}}/delete">{{.locale.Tr "org.teams.delete_team"}}</button>
 								{{end}}
 							{{end}}
 						</div>

tests/integration/api_team_test.go

@@ -12,6 +12,7 @@ import (
 	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/organization"
+	"code.gitea.io/gitea/models/perm"
 	"code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/models/unittest"
@@ -189,6 +190,36 @@ func TestAPITeam(t *testing.T) {
 	req = NewRequestf(t, "DELETE", "/api/v1/teams/%d?token="+token, teamID)
 	MakeRequest(t, req, http.StatusNoContent)
 	unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID})
+
+	// Create admin team
+	teamToCreate = &api.CreateTeamOption{
+		Name:                    "teamadmin",
+		Description:             "team admin",
+		IncludesAllRepositories: true,
+		Permission:              "admin",
+	}
+	req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams?token=%s", org.Name, token), teamToCreate)
+	resp = MakeRequest(t, req, http.StatusCreated)
+	apiTeam = api.Team{}
+	DecodeJSON(t, resp, &apiTeam)
+	for _, ut := range unit.AllRepoUnitTypes {
+		up := perm.AccessModeAdmin
+		if ut == unit.TypeExternalTracker || ut == unit.TypeExternalWiki {
+			up = perm.AccessModeRead
+		}
+		unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{
+			OrgID:      org.ID,
+			TeamID:     apiTeam.ID,
+			Type:       ut,
+			AccessMode: up,
+		})
+	}
+	teamID = apiTeam.ID
+
+	// Delete team.
+	req = NewRequestf(t, "DELETE", "/api/v1/teams/%d?token="+token, teamID)
+	MakeRequest(t, req, http.StatusNoContent)
+	unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID})
 }
 
 func checkTeamResponse(t *testing.T, testName string, apiTeam *api.Team, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) {