Fix team permission (#34128)

The `team.access_mode` should be either `none` or `admin/owner`.
For non-admin team, the real permissions are provided by `team_unit`.

models/issues/issue_update.go

@@ -11,7 +11,6 @@ import (
 
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/organization"
-	"code.gitea.io/gitea/models/perm"
 	access_model "code.gitea.io/gitea/models/perm/access"
 	project_model "code.gitea.io/gitea/models/project"
 	repo_model "code.gitea.io/gitea/models/repo"
@@ -612,7 +611,7 @@ func ResolveIssueMentionsByVisibility(ctx context.Context, issue *Issue, doer *u
 				unittype = unit.TypePullRequests
 			}
 			for _, team := range teams {
-				if team.AccessMode >= perm.AccessModeAdmin {
+				if team.HasAdminAccess() {
 					checked = append(checked, team.ID)
 					resolved[issue.Repo.Owner.LowerName+"/"+team.LowerName] = true
 					continue

models/organization/org_user.go

@@ -78,7 +78,7 @@ func IsOrganizationAdmin(ctx context.Context, orgID, uid int64) (bool, error) {
 		return false, err
 	}
 	for _, t := range teams {
-		if t.AccessMode >= perm.AccessModeAdmin {
+		if t.HasAdminAccess() {
 			return true, nil
 		}
 	}

models/organization/team.go

@@ -113,7 +113,7 @@ func (t *Team) LoadUnits(ctx context.Context) (err error) {
 
 // GetUnitNames returns the team units names
 func (t *Team) GetUnitNames() (res []string) {
-	if t.AccessMode >= perm.AccessModeAdmin {
+	if t.HasAdminAccess() {
 		return unit.AllUnitKeyNames()
 	}
 
@@ -126,7 +126,7 @@ func (t *Team) GetUnitNames() (res []string) {
 // GetUnitsMap returns the team units permissions
 func (t *Team) GetUnitsMap() map[string]string {
 	m := make(map[string]string)
-	if t.AccessMode >= perm.AccessModeAdmin {
+	if t.HasAdminAccess() {
 		for _, u := range unit.Units {
 			m[u.NameKey] = t.AccessMode.ToString()
 		}
@@ -153,6 +153,10 @@ func (t *Team) IsMember(ctx context.Context, userID int64) bool {
 	return isMember
 }
 
+func (t *Team) HasAdminAccess() bool {
+	return t.AccessMode >= perm.AccessModeAdmin
+}
+
 // LoadMembers returns paginated members in team of organization.
 func (t *Team) LoadMembers(ctx context.Context) (err error) {
 	t.Members, err = GetTeamMembers(ctx, &SearchMembersOptions{
@@ -238,22 +242,6 @@ func GetTeamByID(ctx context.Context, teamID int64) (*Team, error) {
 	return t, nil
 }
 
-// GetTeamNamesByID returns team's lower name from a list of team ids.
-func GetTeamNamesByID(ctx context.Context, teamIDs []int64) ([]string, error) {
-	if len(teamIDs) == 0 {
-		return []string{}, nil
-	}
-
-	var teamNames []string
-	err := db.GetEngine(ctx).Table("team").
-		Select("lower_name").
-		In("id", teamIDs).
-		Asc("name").
-		Find(&teamNames)
-
-	return teamNames, err
-}
-
 // IncrTeamRepoNum increases the number of repos for the given team by 1
 func IncrTeamRepoNum(ctx context.Context, teamID int64) error {
 	_, err := db.GetEngine(ctx).Incr("num_repos").ID(teamID).Update(new(Team))

models/perm/access/repo_permission.go

@@ -331,7 +331,7 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use
 
 	// if user in an owner team
 	for _, team := range teams {
-		if team.AccessMode >= perm_model.AccessModeAdmin {
+		if team.HasAdminAccess() {
 			perm.AccessMode = perm_model.AccessModeOwner
 			perm.unitsMode = nil
 			return perm, nil
@@ -399,7 +399,7 @@ func IsUserRepoAdmin(ctx context.Context, repo *repo_model.Repository, user *use
 	}
 
 	for _, team := range teams {
-		if team.AccessMode >= perm_model.AccessModeAdmin {
+		if team.HasAdminAccess() {
 			return true, nil
 		}
 	}

models/unit/unit.go

@@ -20,17 +20,21 @@ type Type int
 
 // Enumerate all the unit types
 const (
-	TypeInvalid         Type = iota // 0 invalid
-	TypeCode                        // 1 code
-	TypeIssues                      // 2 issues
-	TypePullRequests                // 3 PRs
-	TypeReleases                    // 4 Releases
-	TypeWiki                        // 5 Wiki
-	TypeExternalWiki                // 6 ExternalWiki
-	TypeExternalTracker             // 7 ExternalTracker
-	TypeProjects                    // 8 Projects
-	TypePackages                    // 9 Packages
-	TypeActions                     // 10 Actions
+	TypeInvalid Type = iota // 0 invalid
+
+	TypeCode            // 1 code
+	TypeIssues          // 2 issues
+	TypePullRequests    // 3 PRs
+	TypeReleases        // 4 Releases
+	TypeWiki            // 5 Wiki
+	TypeExternalWiki    // 6 ExternalWiki
+	TypeExternalTracker // 7 ExternalTracker
+	TypeProjects        // 8 Projects
+	TypePackages        // 9 Packages
+	TypeActions         // 10 Actions
+
+	// FIXME: TEAM-UNIT-PERMISSION: the team unit "admin" permission's design is not right, when a new unit is added in the future,
+	// admin team won't inherit the correct admin permission for the new unit, need to have a complete fix before adding any new unit.
 )
 
 // Value returns integer value for unit type (used by template)
@@ -380,20 +384,3 @@ func AllUnitKeyNames() []string {
 	}
 	return res
 }
-
-// MinUnitAccessMode returns the minial permission of the permission map
-func MinUnitAccessMode(unitsMap map[Type]perm.AccessMode) perm.AccessMode {
-	res := perm.AccessModeNone
-	for t, mode := range unitsMap {
-		// Don't allow `TypeExternal{Tracker,Wiki}` to influence this as they can only be set to READ perms.
-		if t == TypeExternalTracker || t == TypeExternalWiki {
-			continue
-		}
-
-		// get the minial permission great than AccessModeNone except all are AccessModeNone
-		if mode > perm.AccessModeNone && (res == perm.AccessModeNone || mode < res) {
-			res = mode
-		}
-	}
-	return res
-}

routers/api/v1/org/team.go

@@ -141,26 +141,18 @@ func GetTeam(ctx *context.APIContext) {
 	ctx.JSON(http.StatusOK, apiTeam)
 }
 
-func attachTeamUnits(team *organization.Team, units []string) {
+func attachTeamUnits(team *organization.Team, defaultAccessMode perm.AccessMode, units []string) {
 	unitTypes, _ := unit_model.FindUnitTypes(units...)
 	team.Units = make([]*organization.TeamUnit, 0, len(units))
 	for _, tp := range unitTypes {
 		team.Units = append(team.Units, &organization.TeamUnit{
 			OrgID:      team.OrgID,
 			Type:       tp,
-			AccessMode: team.AccessMode,
+			AccessMode: defaultAccessMode,
 		})
 	}
 }
 
-func convertUnitsMap(unitsMap map[string]string) map[unit_model.Type]perm.AccessMode {
-	res := make(map[unit_model.Type]perm.AccessMode, len(unitsMap))
-	for unitKey, p := range unitsMap {
-		res[unit_model.TypeFromKey(unitKey)] = perm.ParseAccessMode(p)
-	}
-	return res
-}
-
 func attachTeamUnitsMap(team *organization.Team, unitsMap map[string]string) {
 	team.Units = make([]*organization.TeamUnit, 0, len(unitsMap))
 	for unitKey, p := range unitsMap {
@@ -214,24 +206,22 @@ func CreateTeam(ctx *context.APIContext) {
 	//   "422":
 	//     "$ref": "#/responses/validationError"
 	form := web.GetForm(ctx).(*api.CreateTeamOption)
-	p := perm.ParseAccessMode(form.Permission)
-	if p < perm.AccessModeAdmin && len(form.UnitsMap) > 0 {
-		p = unit_model.MinUnitAccessMode(convertUnitsMap(form.UnitsMap))
-	}
+	teamPermission := perm.ParseAccessMode(form.Permission, perm.AccessModeNone, perm.AccessModeAdmin)
 	team := &organization.Team{
 		OrgID:                   ctx.Org.Organization.ID,
 		Name:                    form.Name,
 		Description:             form.Description,
 		IncludesAllRepositories: form.IncludesAllRepositories,
 		CanCreateOrgRepo:        form.CanCreateOrgRepo,
-		AccessMode:              p,
+		AccessMode:              teamPermission,
 	}
 
 	if team.AccessMode < perm.AccessModeAdmin {
 		if len(form.UnitsMap) > 0 {
 			attachTeamUnitsMap(team, form.UnitsMap)
 		} else if len(form.Units) > 0 {
-			attachTeamUnits(team, form.Units)
+			unitPerm := perm.ParseAccessMode(form.Permission, perm.AccessModeRead, perm.AccessModeWrite)
+			attachTeamUnits(team, unitPerm, form.Units)
 		} else {
 			ctx.APIErrorInternal(errors.New("units permission should not be empty"))
 			return
@@ -304,15 +294,10 @@ func EditTeam(ctx *context.APIContext) {
 	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 {
+		teamPermission := perm.ParseAccessMode(form.Permission, perm.AccessModeNone, perm.AccessModeAdmin)
+		if team.AccessMode != teamPermission {
 			isAuthChanged = true
-			team.AccessMode = p
+			team.AccessMode = teamPermission
 		}
 
 		if form.IncludesAllRepositories != nil {
@@ -325,7 +310,8 @@ func EditTeam(ctx *context.APIContext) {
 		if len(form.UnitsMap) > 0 {
 			attachTeamUnitsMap(team, form.UnitsMap)
 		} else if len(form.Units) > 0 {
-			attachTeamUnits(team, form.Units)
+			unitPerm := perm.ParseAccessMode(form.Permission, perm.AccessModeRead, perm.AccessModeWrite)
+			attachTeamUnits(team, unitPerm, form.Units)
 		}
 	} else {
 		attachAdminTeamUnits(team)

routers/api/v1/repo/collaborators.go

@@ -181,7 +181,7 @@ func AddOrUpdateCollaborator(ctx *context.APIContext) {
 
 	p := perm.AccessModeWrite
 	if form.Permission != nil {
-		p = perm.ParseAccessMode(*form.Permission)
+		p = perm.ParseAccessMode(*form.Permission, perm.AccessModeRead, perm.AccessModeWrite, perm.AccessModeAdmin)
 	}
 
 	if err := repo_service.AddOrUpdateCollaborator(ctx, ctx.Repo.Repository, collaborator, p); err != nil {

routers/web/org/teams.go

@@ -284,6 +284,8 @@ func NewTeam(ctx *context.Context) {
 	ctx.HTML(http.StatusOK, tplTeamNew)
 }
 
+// FIXME: TEAM-UNIT-PERMISSION: this design is not right, when a new unit is added in the future,
+// admin team won't inherit the correct admin permission for the new unit.
 func getUnitPerms(forms url.Values, teamPermission perm.AccessMode) map[unit_model.Type]perm.AccessMode {
 	unitPerms := make(map[unit_model.Type]perm.AccessMode)
 	for _, ut := range unit_model.AllRepoUnitTypes {
@@ -314,19 +316,14 @@ func getUnitPerms(forms url.Values, teamPermission perm.AccessMode) map[unit_mod
 func NewTeamPost(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.CreateTeamForm)
 	includesAllRepositories := form.RepoAccess == "all"
-	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.
-		p = unit_model.MinUnitAccessMode(unitPerms)
-	}
+	teamPermission := perm.ParseAccessMode(form.Permission, perm.AccessModeNone, perm.AccessModeAdmin)
+	unitPerms := getUnitPerms(ctx.Req.Form, teamPermission)
 
 	t := &org_model.Team{
 		OrgID:                   ctx.Org.Organization.ID,
 		Name:                    form.TeamName,
 		Description:             form.Description,
-		AccessMode:              p,
+		AccessMode:              teamPermission,
 		IncludesAllRepositories: includesAllRepositories,
 		CanCreateOrgRepo:        form.CanCreateOrgRepo,
 	}
@@ -485,13 +482,8 @@ 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)
-	}
+	teamPermission := perm.ParseAccessMode(form.Permission, perm.AccessModeNone, perm.AccessModeAdmin)
+	unitPerms := getUnitPerms(ctx.Req.Form, teamPermission)
 	isAuthChanged := false
 	isIncludeAllChanged := false
 	includesAllRepositories := form.RepoAccess == "all"
@@ -503,9 +495,9 @@ func EditTeamPost(ctx *context.Context) {
 
 	if !t.IsOwnerTeam() {
 		t.Name = form.TeamName
-		if t.AccessMode != newAccessMode {
+		if t.AccessMode != teamPermission {
 			isAuthChanged = true
-			t.AccessMode = newAccessMode
+			t.AccessMode = teamPermission
 		}
 
 		if t.IncludesAllRepositories != includesAllRepositories {

services/context/org.go

@@ -182,7 +182,7 @@ func OrgAssignment(opts OrgAssignmentOptions) func(ctx *Context) {
 					return
 				}
 				for _, team := range teams {
-					if team.IncludesAllRepositories && team.AccessMode >= perm.AccessModeAdmin {
+					if team.IncludesAllRepositories && team.HasAdminAccess() {
 						shouldSeeAllTeams = true
 						break
 					}
@@ -228,7 +228,7 @@ func OrgAssignment(opts OrgAssignmentOptions) func(ctx *Context) {
 				return
 			}
 
-			ctx.Org.IsTeamAdmin = ctx.Org.Team.IsOwnerTeam() || ctx.Org.Team.AccessMode >= perm.AccessModeAdmin
+			ctx.Org.IsTeamAdmin = ctx.Org.Team.IsOwnerTeam() || ctx.Org.Team.HasAdminAccess()
 			ctx.Data["IsTeamAdmin"] = ctx.Org.IsTeamAdmin
 			if opts.RequireTeamAdmin && !ctx.Org.IsTeamAdmin {
 				ctx.NotFound(err)

services/org/team.go

@@ -259,37 +259,6 @@ func AddTeamMember(ctx context.Context, team *organization.Team, user *user_mode
 		}
 
 		team.NumMembers++
-
-		// Give access to team repositories.
-		// update exist access if mode become bigger
-		subQuery := builder.Select("repo_id").From("team_repo").
-			Where(builder.Eq{"team_id": team.ID})
-
-		if _, err := sess.Where("user_id=?", user.ID).
-			In("repo_id", subQuery).
-			And("mode < ?", team.AccessMode).
-			SetExpr("mode", team.AccessMode).
-			Update(new(access_model.Access)); err != nil {
-			return fmt.Errorf("update user accesses: %w", err)
-		}
-
-		// for not exist access
-		var repoIDs []int64
-		accessSubQuery := builder.Select("repo_id").From("access").Where(builder.Eq{"user_id": user.ID})
-		if err := sess.SQL(subQuery.And(builder.NotIn("repo_id", accessSubQuery))).Find(&repoIDs); err != nil {
-			return fmt.Errorf("select id accesses: %w", err)
-		}
-
-		accesses := make([]*access_model.Access, 0, 100)
-		for i, repoID := range repoIDs {
-			accesses = append(accesses, &access_model.Access{RepoID: repoID, UserID: user.ID, Mode: team.AccessMode})
-			if (i%100 == 0 || i == len(repoIDs)-1) && len(accesses) > 0 {
-				if err = db.Insert(ctx, accesses); err != nil {
-					return fmt.Errorf("insert new user accesses: %w", err)
-				}
-				accesses = accesses[:0]
-			}
-		}
 		return nil
 	})
 	if err != nil {

services/org/team_test.go

@@ -166,24 +166,6 @@ func TestRemoveTeamMember(t *testing.T) {
 	assert.True(t, organization.IsErrLastOrgOwner(err))
 }
 
-func TestRepository_RecalculateAccesses3(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-	team5 := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 5})
-	user29 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 29})
-
-	has, err := db.GetEngine(db.DefaultContext).Get(&access_model.Access{UserID: user29.ID, RepoID: 23})
-	assert.NoError(t, err)
-	assert.False(t, has)
-
-	// adding user29 to team5 should add an explicit access row for repo 23
-	// even though repo 23 is public
-	assert.NoError(t, AddTeamMember(db.DefaultContext, team5, user29))
-
-	has, err = db.GetEngine(db.DefaultContext).Get(&access_model.Access{UserID: user29.ID, RepoID: 23})
-	assert.NoError(t, err)
-	assert.True(t, has)
-}
-
 func TestIncludesAllRepositoriesTeams(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 

templates/org/team/new.tmpl

@@ -56,7 +56,7 @@
 								<br>
 								<div class="field">
 									<div class="ui radio checkbox">
-										<input type="radio" name="permission" value="read" {{if or .PageIsOrgTeamsNew (eq .Team.AccessMode 1) (eq .Team.AccessMode 2)}}checked{{end}}>
+										<input type="radio" name="permission" value="read" {{if or .PageIsOrgTeamsNew (eq .Team.AccessMode 0) (eq .Team.AccessMode 1) (eq .Team.AccessMode 2)}}checked{{end}}>
 										<label>{{ctx.Locale.Tr "org.teams.general_access"}}</label>
 										<span class="help">{{ctx.Locale.Tr "org.teams.general_access_helper"}}</span>
 									</div>

templates/org/team/sidebar.tmpl

@@ -42,10 +42,12 @@
 						<li>{{ctx.Locale.Tr "org.teams.can_create_org_repo"}}</li>
 					{{end}}
 				</ul>
-				{{if (eq .Team.AccessMode 2)}}
+				{{/* the AccessMode should be either none or admin/owner, the real permissions are provided by each team unit */}}
+				{{if false}}{{/*(eq .Team.AccessMode 2)*/}}
 					<h3>{{ctx.Locale.Tr "org.settings.permission"}}</h3>
 					{{ctx.Locale.Tr "org.teams.write_permission_desc"}}
 				{{else if (eq .Team.AccessMode 3)}}
+					{{/* FIXME: here might not right, see "FIXME: TEAM-UNIT-PERMISSION", new units might not have correct admin permission*/}}
 					<h3>{{ctx.Locale.Tr "org.settings.permission"}}</h3>
 					{{ctx.Locale.Tr "org.teams.admin_permission_desc"}}
 				{{else}}

tests/integration/api_team_test.go

@@ -78,9 +78,9 @@ func TestAPITeam(t *testing.T) {
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
 	checkTeamResponse(t, "CreateTeam1", &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
-		teamToCreate.Permission, teamToCreate.Units, nil)
+		"none", teamToCreate.Units, nil)
 	checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
-		teamToCreate.Permission, teamToCreate.Units, nil)
+		"none", teamToCreate.Units, nil)
 	teamID := apiTeam.ID
 
 	// Edit team.
@@ -149,9 +149,9 @@ func TestAPITeam(t *testing.T) {
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
 	checkTeamResponse(t, "CreateTeam2", &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
-		"read", nil, teamToCreate.UnitsMap)
+		"none", nil, teamToCreate.UnitsMap)
 	checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
-		"read", nil, teamToCreate.UnitsMap)
+		"none", nil, teamToCreate.UnitsMap)
 	teamID = apiTeam.ID
 
 	// Edit team.
@@ -171,9 +171,9 @@ func TestAPITeam(t *testing.T) {
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
 	checkTeamResponse(t, "EditTeam2", &apiTeam, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
-		"read", nil, teamToEdit.UnitsMap)
+		"none", nil, teamToEdit.UnitsMap)
 	checkTeamBean(t, apiTeam.ID, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
-		"read", nil, teamToEdit.UnitsMap)
+		"none", nil, teamToEdit.UnitsMap)
 
 	// Edit team Description only
 	editDescription = "second team"
@@ -184,9 +184,9 @@ func TestAPITeam(t *testing.T) {
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
 	checkTeamResponse(t, "EditTeam2_DescOnly", &apiTeam, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
-		"read", nil, teamToEdit.UnitsMap)
+		"none", nil, teamToEdit.UnitsMap)
 	checkTeamBean(t, apiTeam.ID, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
-		"read", nil, teamToEdit.UnitsMap)
+		"none", nil, teamToEdit.UnitsMap)
 
 	// Read team.
 	teamRead = unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: teamID})

tests/integration/org_test.go

@@ -177,9 +177,9 @@ func TestOrgRestrictedUser(t *testing.T) {
 	resp := adminSession.MakeRequest(t, req, http.StatusCreated)
 	DecodeJSON(t, resp, &apiTeam)
 	checkTeamResponse(t, "CreateTeam_codereader", &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
-		teamToCreate.Permission, teamToCreate.Units, nil)
+		"none", teamToCreate.Units, nil)
 	checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
-		teamToCreate.Permission, teamToCreate.Units, nil)
+		"none", teamToCreate.Units, nil)
 	// teamID := apiTeam.ID
 
 	// Now we need to add the restricted user to the team