Fix edit team

models/organization/team.go

@@ -272,3 +272,8 @@ func IncrTeamRepoNum(ctx context.Context, teamID int64) error {
 	_, err := db.GetEngine(ctx).Incr("num_repos").ID(teamID).Update(new(Team))
 	return err
 }
+
+func UpdateTeam(ctx context.Context, t *Team, cols ...string) error {
+	_, err := db.GetEngine(ctx).ID(t.ID).Cols(cols...).Update(t)
+	return err
+}

models/organization/team_fix.go

@@ -0,0 +1,72 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package organization
+
+import (
+	"context"
+	"strings"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/models/perm"
+	"code.gitea.io/gitea/models/unit"
+
+	"xorm.io/builder"
+)
+
+// CountInconsistentOwnerTeams returns the amount of owner teams that any of
+// their access modes not set to the right value. For external trackers and external wikis
+// it should be Read and otherwise it should be Owner.
+func CountInconsistentOwnerTeams(ctx context.Context) (int64, error) {
+	cnt1, err := db.GetEngine(ctx).Table("team").
+		Join("INNER", "team_unit", "`team`.id = `team_unit`.team_id").
+		Where("`team`.lower_name = ?", strings.ToLower(OwnerTeamName)).
+		And(builder.Or(builder.Eq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Eq{"`team_unit`.`type`": unit.TypeExternalWiki})).
+		And("`team_unit`.`access_mode` <> ?", perm.AccessModeRead).
+		GroupBy("`team`.`id`").
+		Count()
+	if err != nil {
+		return 0, err
+	}
+
+	cnt2, err := db.GetEngine(ctx).Table("team").
+		Join("INNER", "team_unit", "`team`.id = `team_unit`.team_id").
+		Where("`team`.lower_name = ?", strings.ToLower(OwnerTeamName)).
+		And(builder.And(builder.Neq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Neq{"`team_unit`.`type`": unit.TypeExternalWiki})).
+		And("`team_unit`.`access_mode` <> ?", perm.AccessModeOwner).
+		GroupBy("`team_unit`.team_id").
+		Count()
+	if err != nil {
+		return 0, err
+	}
+	return cnt1 + cnt2, nil
+}
+
+// FixInconsistentOwnerTeams fixes inconsistent owner teams that  of
+// their access modes not set to the right value. For external trackers and external wikis
+// it should be Read and otherwise it should be Owner.
+func FixInconsistentOwnerTeams(ctx context.Context) (int64, error) {
+	subQuery := builder.Select("id").From("team").Where(builder.Eq{"lower_name": strings.ToLower(OwnerTeamName)})
+	updated1, err := db.GetEngine(ctx).Table("team_unit").
+		Where(builder.Or(builder.Eq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Eq{"`team_unit`.`type`": unit.TypeExternalWiki})).
+		In("team_id", subQuery).
+		Cols("access_mode").
+		Update(&TeamUnit{
+			AccessMode: perm.AccessModeRead,
+		})
+	if err != nil {
+		return 0, err
+	}
+
+	updated2, err := db.GetEngine(ctx).Table("team_unit").
+		Where(builder.And(builder.Neq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Neq{"`team_unit`.`type`": unit.TypeExternalWiki})).
+		In("team_id", subQuery).
+		Cols("access_mode").
+		Update(&TeamUnit{
+			AccessMode: perm.AccessModeOwner,
+		})
+	if err != nil {
+		return 0, err
+	}
+	return updated1 + updated2, nil
+}

routers/api/v1/org/team.go

@@ -293,45 +293,46 @@ func EditTeam(ctx *context.APIContext) {
 		team.CanCreateOrgRepo = team.IsOwnerTeam() || *form.CanCreateOrgRepo
 	}
 
+	teamName := team.Name
 	if len(form.Name) > 0 {
-		team.Name = form.Name
+		teamName = form.Name
 	}
 
+	description := team.Description
 	if form.Description != nil {
-		team.Description = *form.Description
+		description = *form.Description
 	}
 
-	isAuthChanged := false
-	isIncludeAllChanged := false
-	if !team.IsOwnerTeam() && len(form.Permission) != 0 {
-		// Validate permission level.
-		p := perm.ParseAccessMode(form.Permission)
-		if p < perm.AccessModeAdmin && len(form.UnitsMap) > 0 {
-			p = unit_model.MinUnitAccessMode(convertUnitsMap(form.UnitsMap))
-		}
-
-		if team.AccessMode != p {
-			isAuthChanged = true
-			team.AccessMode = p
-		}
+	includeAllRepos := team.IncludesAllRepositories
+	if form.IncludesAllRepositories != nil {
+		includeAllRepos = *form.IncludesAllRepositories
+	}
 
-		if form.IncludesAllRepositories != nil {
-			isIncludeAllChanged = true
-			team.IncludesAllRepositories = *form.IncludesAllRepositories
-		}
+	canCreateOrgRepo := team.CanCreateOrgRepo
+	if form.CanCreateOrgRepo != nil {
+		canCreateOrgRepo = *form.CanCreateOrgRepo
 	}
 
+	unitPerms := make(map[unit_model.Type]perm.AccessMode)
 	if team.AccessMode < perm.AccessModeAdmin {
 		if len(form.UnitsMap) > 0 {
-			attachTeamUnitsMap(team, form.UnitsMap)
-		} else if len(form.Units) > 0 {
-			attachTeamUnits(team, form.Units)
+			unitPerms = convertUnitsMap(form.UnitsMap)
 		}
-	} else {
-		attachAdminTeamUnits(team)
 	}
 
-	if err := org_service.UpdateTeam(ctx, team, isAuthChanged, isIncludeAllChanged); err != nil {
+	isAdmin := team.AccessMode == perm.AccessModeAdmin
+	if form.Permission != "" && form.Permission == "admin" {
+		isAdmin = true
+	}
+
+	if err := org_service.UpdateTeam(ctx, team, org_service.UpdateTeamOptions{
+		TeamName:                teamName,
+		Description:             description,
+		IsAdmin:                 isAdmin,
+		IncludesAllRepositories: includeAllRepos,
+		CanCreateOrgRepo:        canCreateOrgRepo,
+		UnitPerms:               unitPerms,
+	}); err != nil {
 		ctx.Error(http.StatusInternalServerError, "EditTeam", err)
 		return
 	}

routers/web/org/teams.go

@@ -481,61 +481,26 @@ func EditTeam(ctx *context.Context) {
 func EditTeamPost(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.CreateTeamForm)
 	t := ctx.Org.Team
-	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"
+	unitPerms := getUnitPerms(ctx.Req.Form, perm.ParseAccessMode(form.Permission))
 
 	ctx.Data["Title"] = ctx.Org.Organization.FullName
 	ctx.Data["PageIsOrgTeams"] = true
 	ctx.Data["Team"] = t
 	ctx.Data["Units"] = unit_model.Units
 
-	if !t.IsOwnerTeam() {
-		t.Name = form.TeamName
-		if t.AccessMode != newAccessMode {
-			isAuthChanged = true
-			t.AccessMode = newAccessMode
-		}
-
-		if t.IncludesAllRepositories != includesAllRepositories {
-			isIncludeAllChanged = true
-			t.IncludesAllRepositories = includesAllRepositories
-		}
-		t.CanCreateOrgRepo = form.CanCreateOrgRepo
-	} else {
-		t.CanCreateOrgRepo = true
-	}
-
-	t.Description = form.Description
-	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)
-		return
-	}
-
 	if t.AccessMode < perm.AccessModeAdmin && len(unitPerms) == 0 {
 		ctx.RenderWithErr(ctx.Tr("form.team_no_units_error"), tplTeamNew, &form)
 		return
 	}
 
-	if err := org_service.UpdateTeam(ctx, t, isAuthChanged, isIncludeAllChanged); err != nil {
+	if err := org_service.UpdateTeam(ctx, t, org_service.UpdateTeamOptions{
+		TeamName:                form.TeamName,
+		Description:             form.Description,
+		IsAdmin:                 form.Permission == "admin",
+		IncludesAllRepositories: form.RepoAccess == "all",
+		CanCreateOrgRepo:        form.CanCreateOrgRepo,
+		UnitPerms:               unitPerms,
+	}); err != nil {
 		ctx.Data["Err_TeamName"] = true
 		switch {
 		case org_model.IsErrTeamAlreadyExist(err):

services/doctor/dbconsistency.go

@@ -11,6 +11,7 @@ import (
 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/migrations"
+	org_model "code.gitea.io/gitea/models/organization"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -164,6 +165,12 @@ func prepareDBConsistencyChecks() []consistencyCheck {
 			Fixer:        repo_model.DeleteOrphanedTopics,
 			FixedMessage: "Removed",
 		},
+		{
+			Name:         "Owner teams with wrong admin access unit",
+			Counter:      org_model.CountInconsistentOwnerTeams,
+			Fixer:        org_model.FixInconsistentOwnerTeams,
+			FixedMessage: "Fixed",
+		},
 	}
 
 	// TODO: function to recalc all counters

services/doctor/fix8312.go

@@ -10,7 +10,6 @@ import (
 	org_model "code.gitea.io/gitea/models/organization"
 	"code.gitea.io/gitea/models/perm"
 	"code.gitea.io/gitea/modules/log"
-	org_service "code.gitea.io/gitea/services/org"
 
 	"xorm.io/builder"
 )
@@ -29,7 +28,7 @@ func fixOwnerTeamCreateOrgRepo(ctx context.Context, logger log.Logger, autofix b
 				return nil
 			}
 
-			return org_service.UpdateTeam(ctx, team, false, false)
+			return org_model.UpdateTeam(ctx, team, "can_create_org_repo")
 		},
 	)
 	if err != nil {

services/forms/org.go

@@ -53,19 +53,12 @@ func (f *UpdateOrgSettingForm) Validate(req *http.Request, errs binding.Errors)
 	return middleware.Validate(errs, ctx.Data, f, ctx.Locale)
 }
 
-// ___________
-// \__    ___/___ _____    _____
-//   |    |_/ __ \\__  \  /     \
-//   |    |\  ___/ / __ \|  Y Y  \
-//   |____| \___  >____  /__|_|  /
-//              \/     \/      \/
-
 // CreateTeamForm form for creating team
 type CreateTeamForm struct {
 	TeamName         string `binding:"Required;AlphaDashDot;MaxSize(255)"`
 	Description      string `binding:"MaxSize(255)"`
-	Permission       string
-	RepoAccess       string
+	Permission       string `binding:"Required;In(,read,write,admin)"`
+	RepoAccess       string `binding:"Required;In(specified, all)"`
 	CanCreateOrgRepo bool
 }