enhance: inherit team access for all units (#38938)

Admin and write team authorize now grant that mode on every unit,
including units added later, instead of only rows present in
`team_unit`. Granular teams keep `authorize=none` and explicit unit
rows.

Closes the `TEAM-UNIT-PERMISSION` design gap from
https://github.com/go-gitea/gitea/pull/34128.

Maybe also fix #15962 (actually maybe it had been fixed before, the root
cause is out-of-sync "access" table)


## Screenshots

only writing selected:
<img width="1399" height="1007" alt="image"
src="https://github.com/user-attachments/assets/1d1b4c49-a59a-47b6-998f-0464a067395b"
/>


_Created with the help of AI_

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/issues/issue_search.go

@@ -11,6 +11,7 @@ import (
 
 	"gitea.dev/models/db"
 	"gitea.dev/models/organization"
+	"gitea.dev/models/perm"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unit"
 	user_model "gitea.dev/models/user"
@@ -287,8 +288,8 @@ func applyConditions(sess db.Session, opts *IssuesOptions) {
 	}
 }
 
-// teamUnitsRepoCond returns query condition for those repo id in the special org team with special units access
-func teamUnitsRepoCond(id string, userID, orgID, teamID int64, units ...unit.Type) builder.Cond {
+// teamUnitsRepoReaderCond returns query condition for those repo id in the special org team with special units access
+func teamUnitsRepoReaderCond(id string, userID, orgID, teamID int64, units ...unit.Type) builder.Cond {
 	return builder.In(id,
 		builder.Select("repo_id").From("team_repo").Where(
 			builder.Eq{
@@ -316,12 +317,19 @@ func teamUnitsRepoCond(id string, userID, orgID, teamID int64, units ...unit.Typ
 						}),
 					),
 				)).And(
-				builder.In(
-					"team_id", builder.Select("team_id").From("team_unit").Where(
-						builder.Eq{
-							"`team_unit`.org_id": orgID,
-						}.And(
-							builder.In("`team_unit`.type", units),
+				builder.Or(
+					builder.In(
+						"team_id", builder.Select("id").From("team").Where(
+							builder.Eq{"id": teamID}.And(builder.Gt{"authorize": perm.AccessModeNone}),
+						),
+					),
+					builder.In(
+						"team_id", builder.Select("team_id").From("team_unit").Where(
+							builder.Eq{
+								"`team_unit`.org_id": orgID,
+							}.And(
+								builder.In("`team_unit`.type", units),
+							),
 						),
 					),
 				),
@@ -338,7 +346,7 @@ func issuePullAccessibleRepoCond(repoIDstr string, userID int64, owner *user_mod
 	}
 	if owner != nil && owner.IsOrganization() {
 		if team != nil {
-			cond = cond.And(teamUnitsRepoCond(repoIDstr, userID, owner.ID, team.ID, unitType)) // special team member repos
+			cond = cond.And(teamUnitsRepoReaderCond(repoIDstr, userID, owner.ID, team.ID, unitType)) // special team member repos
 		} else {
 			cond = cond.And(
 				builder.Or(

models/issues/issue_update.go

@@ -626,16 +626,7 @@ func ResolveIssueMentionsByVisibility(ctx context.Context, issue *Issue, doer *u
 				unittype = unit.TypePullRequests
 			}
 			for _, team := range teams {
-				if team.HasAdminAccess() {
-					checked = append(checked, team.ID)
-					resolved[issue.Repo.Owner.LowerName+"/"+team.LowerName] = true
-					continue
-				}
-				has, err := db.Exist[organization.TeamUnit](ctx, builder.Eq{"org_id": issue.Repo.Owner.ID, "team_id": team.ID, "`type`": unittype})
-				if err != nil {
-					return nil, fmt.Errorf("get team units (%d): %w", team.ID, err)
-				}
-				if has {
+				if team.UnitEnabled(ctx, unittype) {
 					checked = append(checked, team.ID)
 					resolved[issue.Repo.Owner.LowerName+"/"+team.LowerName] = true
 				}

models/organization/org.go

@@ -284,8 +284,8 @@ func (org *Organization) CustomAvatarRelativePath() string {
 	return org.Avatar
 }
 
-// UnitPermission returns unit permission
-func (org *Organization) UnitPermission(ctx context.Context, doer *user_model.User, unitType unit.Type) perm.AccessMode {
+func (org *Organization) AnyRepoUnitPermission(ctx context.Context, doer *user_model.User, unitType unit.Type) perm.AccessMode {
+	// FIXME: ORG-TEAM-UNIT-MAX-PERMISSION: this function is not right, team can access repo1's code doesn't mean it can access repo2's code
 	if doer != nil {
 		teams, err := GetUserOrgTeams(ctx, org.ID, doer.ID)
 		if err != nil {
@@ -299,7 +299,7 @@ func (org *Organization) UnitPermission(ctx context.Context, doer *user_model.Us
 		}
 
 		if len(teams) > 0 {
-			return teams.UnitMaxAccess(unitType)
+			return teams.AnyRepoUnitMaxAccess(ctx, unitType)
 		}
 	}
 

models/organization/team.go

@@ -149,29 +149,14 @@ func (t *Team) LoadUnits(ctx context.Context) (err error) {
 	return err
 }
 
-// GetUnitNames returns the team units names
-func (t *Team) GetUnitNames() (res []string) {
-	if t.HasAdminAccess() {
-		return unit.AllUnitKeyNames()
-	}
-
-	for _, u := range t.Units {
-		res = append(res, unit.Units[u.Type].NameKey)
-	}
-	return res
-}
-
 // GetUnitsMap returns the team units permissions
 func (t *Team) GetUnitsMap() map[string]string {
+	if len(t.Units) == 0 {
+		return nil
+	}
 	m := make(map[string]string)
-	if t.HasAdminAccess() {
-		for _, u := range unit.Units {
-			m[u.NameKey] = t.AccessMode.ToString()
-		}
-	} else {
-		for _, u := range t.Units {
-			m[u.Unit().NameKey] = u.AccessMode.ToString()
-		}
+	for _, u := range t.Units {
+		m[u.Unit().NameKey] = u.AccessMode.ToString()
 	}
 	return m
 }
@@ -214,16 +199,21 @@ func (t *Team) UnitAccessMode(ctx context.Context, tp unit.Type) perm.AccessMode
 	return accessMode
 }
 
-func (t *Team) UnitAccessModeEx(ctx context.Context, tp unit.Type) (accessMode perm.AccessMode, exist bool) {
+func (t *Team) UnitAccessModeEx(ctx context.Context, tp unit.Type) (mode perm.AccessMode, exist bool) {
 	if err := t.LoadUnits(ctx); err != nil {
-		log.Warn("Error loading team (ID: %d) units: %s", t.ID, err.Error())
+		log.Error("Error loading team (ID: %d) units: %v", t.ID, err)
 	}
 	for _, u := range t.Units {
 		if u.Type == tp {
-			return u.AccessMode, true
+			mode, exist = u.AccessMode, true
+			break
 		}
 	}
-	return perm.AccessModeNone, false
+	mode = max(mode, t.AccessMode)
+	if unitDef, ok := unit.Units[tp]; ok {
+		mode = min(mode, unitDef.MaxPerm())
+	}
+	return mode, exist || t.AccessMode > perm.AccessModeNone
 }
 
 // IsUsableTeamName tests if a name could be as team name

models/organization/team_list.go

@@ -27,20 +27,14 @@ func (t TeamList) LoadUnits(ctx context.Context) error {
 	return nil
 }
 
-func (t TeamList) UnitMaxAccess(tp unit.Type) perm.AccessMode {
+func (t TeamList) AnyRepoUnitMaxAccess(ctx context.Context, tp unit.Type) perm.AccessMode {
+	// FIXME: ORG-TEAM-UNIT-MAX-PERMISSION: this function is not right, team can access repo1's code doesn't mean it can access repo2's code
 	maxAccess := perm.AccessModeNone
 	for _, team := range t {
 		if team.IsOwnerTeam() {
 			return perm.AccessModeOwner
 		}
-		for _, teamUnit := range team.Units {
-			if teamUnit.Type != tp {
-				continue
-			}
-			if teamUnit.AccessMode > maxAccess {
-				maxAccess = teamUnit.AccessMode
-			}
-		}
+		maxAccess = max(maxAccess, team.UnitAccessMode(ctx, tp))
 	}
 	return maxAccess
 }

models/organization/team_list_test.go

@@ -7,6 +7,8 @@ import (
 	"testing"
 
 	org_model "gitea.dev/models/organization"
+	"gitea.dev/models/perm"
+	"gitea.dev/models/unit"
 	"gitea.dev/models/unittest"
 
 	"github.com/stretchr/testify/assert"
@@ -22,3 +24,21 @@ func Test_GetTeamsByIDs(t *testing.T) {
 	assert.Equal(t, "Owners", teams[1].Name)
 	assert.Equal(t, "team1", teams[2].Name)
 }
+
+func TestTeamList_UnitMaxAccess(t *testing.T) {
+	ctx := t.Context()
+	adminTeam := &org_model.Team{AccessMode: perm.AccessModeAdmin, Units: nil}
+	writeTeam := &org_model.Team{AccessMode: perm.AccessModeWrite, Units: nil}
+	granularTeam := &org_model.Team{
+		AccessMode: perm.AccessModeNone,
+		Units: []*org_model.TeamUnit{
+			{Type: unit.TypeCode, AccessMode: perm.AccessModeWrite},
+		},
+	}
+
+	assert.Equal(t, perm.AccessModeAdmin, org_model.TeamList{adminTeam}.AnyRepoUnitMaxAccess(ctx, unit.TypeActions))
+	assert.Equal(t, perm.AccessModeWrite, org_model.TeamList{writeTeam}.AnyRepoUnitMaxAccess(ctx, unit.TypeActions))
+	assert.Equal(t, perm.AccessModeWrite, org_model.TeamList{granularTeam}.AnyRepoUnitMaxAccess(ctx, unit.TypeCode))
+	assert.Equal(t, perm.AccessModeNone, org_model.TeamList{granularTeam}.AnyRepoUnitMaxAccess(ctx, unit.TypeActions))
+	assert.Equal(t, perm.AccessModeAdmin, org_model.TeamList{granularTeam, adminTeam}.AnyRepoUnitMaxAccess(ctx, unit.TypeActions))
+}

models/organization/team_repo.go

@@ -52,7 +52,6 @@ func RemoveTeamRepo(ctx context.Context, teamID, repoID int64) error {
 
 // GetTeamsWithAccessToAnyRepoUnit returns all teams in an organization that have given access level to the repository special unit.
 // This function is only used for finding some teams that can be used as branch protection allowlist or reviewers, it isn't really used for access control.
-// FIXME: TEAM-UNIT-PERMISSION this logic is not complete, search the fixme keyword to see more details
 func GetTeamsWithAccessToAnyRepoUnit(ctx context.Context, orgID, repoID int64, mode perm.AccessMode, unitType unit.Type, unitTypesMore ...unit.Type) (teams []*Team, err error) {
 	teamIDs, err := getTeamIDsWithAccessToAnyRepoUnit(ctx, orgID, repoID, mode, unitType, unitTypesMore...)
 	if err != nil {

models/organization/team_test.go

@@ -8,7 +8,9 @@ import (
 
 	"gitea.dev/models/db"
 	"gitea.dev/models/organization"
+	"gitea.dev/models/perm"
 	repo_model "gitea.dev/models/repo"
+	"gitea.dev/models/unit"
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/structs"
@@ -291,3 +293,28 @@ func TestIsUsableTeamName(t *testing.T) {
 	assert.NoError(t, organization.IsUsableTeamName("usable"))
 	assert.True(t, db.IsErrNameReserved(organization.IsUsableTeamName("new")))
 }
+
+func TestTeam_UnitAccessModeEx(t *testing.T) {
+	team := &organization.Team{
+		AccessMode: perm.AccessModeWrite, Units: []*organization.TeamUnit{
+			{Type: unit.TypeIssues, AccessMode: perm.AccessModeRead}, // team mode wins
+			{Type: unit.TypeWiki, AccessMode: perm.AccessModeAdmin},  // unit mode wins
+		},
+	}
+	mode, exist := team.UnitAccessModeEx(t.Context(), unit.TypeActions)
+	assert.True(t, exist)
+	assert.Equal(t, perm.AccessModeWrite, mode)
+	assert.Equal(t, perm.AccessModeWrite, team.UnitAccessMode(t.Context(), unit.TypeIssues))
+	assert.Equal(t, perm.AccessModeAdmin, team.UnitAccessMode(t.Context(), unit.TypeWiki))
+	assert.Equal(t, perm.AccessModeRead, team.UnitAccessMode(t.Context(), unit.TypeExternalWiki)) // limited by unit definition
+
+	team = &organization.Team{AccessMode: perm.AccessModeOwner, Units: []*organization.TeamUnit{}}
+	mode, exist = team.UnitAccessModeEx(t.Context(), unit.TypePackages)
+	assert.True(t, exist)
+	assert.Equal(t, perm.AccessModeAdmin, mode)
+
+	team = &organization.Team{AccessMode: perm.AccessModeNone, Units: []*organization.TeamUnit{}}
+	mode, exist = team.UnitAccessModeEx(t.Context(), unit.TypeActions)
+	assert.False(t, exist)
+	assert.Equal(t, perm.AccessModeNone, mode)
+}

models/repo/collaboration.go

@@ -111,16 +111,21 @@ func IsCollaborator(ctx context.Context, repoID, userID int64) (bool, error) {
 	return db.Exist[Collaboration](ctx, builder.Eq{"repo_id": repoID, "user_id": userID})
 }
 
-// IsOwnerMemberCollaborator checks if a provided user is the owner, a collaborator or a member of a team in a repository
-func IsOwnerMemberCollaborator(ctx context.Context, repo *Repository, userID int64) (bool, error) {
+func HasAccessToRepoCodeUnit(ctx context.Context, repo *Repository, userID int64) (bool, error) {
 	if repo.OwnerID == userID {
 		return true, nil
 	}
-	teamMember, err := db.GetEngine(ctx).Join("INNER", "team_repo", "team_repo.team_id = team_user.team_id").
-		Join("INNER", "team_unit", "team_unit.team_id = team_user.team_id").
+	teamMember, err := db.GetEngine(ctx).Table("team_user").
+		Join("INNER", "team_repo", "team_repo.team_id = team_user.team_id").
+		Join("INNER", "team", "team.id = team_user.team_id").
+		Join("LEFT", "team_unit", "team_unit.team_id = team_user.team_id AND team_unit.`type` = ?", unit.TypeCode).
 		Where("team_repo.repo_id = ?", repo.ID).
-		And("team_unit.`type` = ?", unit.TypeCode).
-		And("team_user.uid = ?", userID).Table("team_user").Exist()
+		And("team_user.uid = ?", userID).
+		And(builder.Or(
+			builder.Gt{"team.authorize": perm.AccessModeNone},
+			builder.Gt{"team_unit.access_mode": perm.AccessModeNone},
+		)).
+		Exist()
 	if err != nil {
 		return false, err
 	}

models/repo/collaboration_test.go

@@ -73,31 +73,31 @@ func TestRepository_IsOwnerMemberCollaborator(t *testing.T) {
 	repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
 
 	// Organisation owner.
-	actual, err := repo_model.IsOwnerMemberCollaborator(t.Context(), repo1, 2)
+	actual, err := repo_model.HasAccessToRepoCodeUnit(t.Context(), repo1, 2)
 	assert.NoError(t, err)
 	assert.True(t, actual)
 
 	// Team member.
-	actual, err = repo_model.IsOwnerMemberCollaborator(t.Context(), repo1, 4)
+	actual, err = repo_model.HasAccessToRepoCodeUnit(t.Context(), repo1, 4)
 	assert.NoError(t, err)
 	assert.True(t, actual)
 
 	// Normal user.
-	actual, err = repo_model.IsOwnerMemberCollaborator(t.Context(), repo1, 1)
+	actual, err = repo_model.HasAccessToRepoCodeUnit(t.Context(), repo1, 1)
 	assert.NoError(t, err)
 	assert.False(t, actual)
 
 	repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
 
 	// Collaborator.
-	actual, err = repo_model.IsOwnerMemberCollaborator(t.Context(), repo2, 4)
+	actual, err = repo_model.HasAccessToRepoCodeUnit(t.Context(), repo2, 4)
 	assert.NoError(t, err)
 	assert.True(t, actual)
 
 	repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 15})
 
 	// Repository owner.
-	actual, err = repo_model.IsOwnerMemberCollaborator(t.Context(), repo3, 2)
+	actual, err = repo_model.HasAccessToRepoCodeUnit(t.Context(), repo3, 2)
 	assert.NoError(t, err)
 	assert.True(t, actual)
 }

models/repo/repo_list.go

@@ -310,15 +310,12 @@ func userOrgTeamRepoBuilder(userID int64) *builder.Builder {
 }
 
 // userOrgTeamUnitRepoBuilder returns repo ids where user's teams can access the special unit.
-// A team grants the unit either through an explicit team_unit row (access_mode > none) or by being an
-// admin/owner team (team.authorize >= admin), which grants every unit regardless of team_unit rows —
-// mirroring the HasAdminAccess() short-circuit in access.GetIndividualUserRepoPermission.
 func userOrgTeamUnitRepoBuilder(userID int64, unitType unit.Type) *builder.Builder {
 	return userOrgTeamRepoBuilder(userID).
 		Join("INNER", "team", "`team`.id = `team_repo`.team_id").
 		Join("LEFT", "team_unit", builder.Expr("`team_unit`.team_id = `team_repo`.team_id AND `team_unit`.`type` = ?", unitType)).
 		Where(builder.Or(
-			builder.Gte{"`team`.authorize": int(perm.AccessModeAdmin)},
+			builder.Gt{"`team`.authorize": int(perm.AccessModeNone)},
 			builder.Gt{"`team_unit`.`access_mode`": int(perm.AccessModeNone)},
 		))
 }

models/repo/repo_list_test.go

@@ -486,10 +486,7 @@ func TestFindUserActionsAccessibleOwnerRepoIDs(t *testing.T) {
 	assert.Contains(t, publicOnly, int64(32), "a public repo under a public owner stays listed")
 }
 
-// TestUserOrgUnitRepoCondTeamAuthorize pins the team.authorize behavior of userOrgTeamUnitRepoBuilder
-// (exercised through UserOrgUnitRepoCond): an admin/owner team grants every unit even without an explicit
-// team_unit row, while a non-admin team only grants a unit it has an explicit row for. This guards both
-// directions — hiding repos from admin-team members, and over-broadening a plain team's access.
+// TestUserOrgUnitRepoCondTeamAuthorize pins team.authorize vs team_unit.access_mode
 func TestUserOrgUnitRepoCondTeamAuthorize(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())
 
@@ -500,17 +497,16 @@ func TestUserOrgUnitRepoCondTeamAuthorize(t *testing.T) {
 		return ids
 	}
 
-	// Case A: user18 is only on org17's owner team (team5, authorize=owner), linked to the private repo24
-	// but with no Actions team_unit row. The owner authorize must still grant it, mirroring the runtime
-	// HasAdminAccess() short-circuit in access.GetIndividualUserRepoPermission.
-	assert.Contains(t, accessibleRepoIDs(18, 17, unit.TypeActions), int64(24),
-		"an owner team grants a unit it has no explicit team_unit row for")
+	// Owner team5 has no Actions team_unit row but still grants via authorize=owner.
+	assert.Contains(t, accessibleRepoIDs(18, 17, unit.TypeActions), int64(24))
 
-	// Cases B and C share one subject so the team_unit row is the only difference: user4 is only on org3's
-	// write team (team2, authorize=write, non-admin), linked to the private repo3. team2 has an explicit
-	// Projects row but none for Actions.
-	assert.Contains(t, accessibleRepoIDs(4, 3, unit.TypeProjects), int64(3),
-		"a non-admin team grants a unit it has an explicit team_unit row for")
-	assert.NotContains(t, accessibleRepoIDs(4, 3, unit.TypeActions), int64(3),
-		"a non-admin team must NOT grant a unit it has no team_unit row for")
+	// team2 is "authorize=write" with Projects team_unit but no Actions row.
+	assert.Contains(t, accessibleRepoIDs(4, 3, unit.TypeProjects), int64(3))
+	assert.Contains(t, accessibleRepoIDs(4, 3, unit.TypeActions), int64(3))
+
+	// now team2 is "authorize=none", no Actions row.
+	_, err := db.GetEngine(t.Context()).Exec("UPDATE team SET authorize=0 WHERE id=2")
+	assert.NoError(t, err)
+	assert.Contains(t, accessibleRepoIDs(4, 3, unit.TypeProjects), int64(3))
+	assert.NotContains(t, accessibleRepoIDs(4, 3, unit.TypeActions), int64(3))
 }

models/unit/unit.go

@@ -33,9 +33,6 @@ const (
 	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)

models/unittest/unit_tests.go

@@ -27,6 +27,7 @@ type TestingT interface {
 	require.TestingT
 	assert.TestingT
 	Context() context.Context
+	Helper()
 }
 
 type testCond struct {
@@ -77,6 +78,7 @@ func GetBean[T any](t TestingT, bean T, conditions ...any) (ret T) {
 
 // AssertExistsAndLoadBean assert that a bean exists and load it from the test database
 func AssertExistsAndLoadBean[T any](t TestingT, bean T, conditions ...any) T {
+	t.Helper()
 	exists, err := getBeanIfExists(t, bean, conditions...)
 	require.NoError(t, err)
 	require.True(t, exists,
@@ -87,6 +89,7 @@ func AssertExistsAndLoadBean[T any](t TestingT, bean T, conditions ...any) T {
 
 // AssertExistsAndLoadMap assert that a row exists and load it from the test database
 func AssertExistsAndLoadMap(t TestingT, table string, conditions ...any) map[string]string {
+	t.Helper()
 	e := db.GetEngine(t.Context()).Table(table)
 	res, err := whereOrderConditions(e, conditions).Query()
 	assert.NoError(t, err)
@@ -123,6 +126,7 @@ func GetCount(t TestingT, bean any, conditions ...any) int {
 
 // AssertNotExistsBean assert that a bean does not exist in the test database
 func AssertNotExistsBean(t TestingT, bean any, conditions ...any) {
+	t.Helper()
 	exists, err := getBeanIfExists(t, bean, conditions...)
 	assert.NoError(t, err)
 	assert.False(t, exists)
@@ -130,17 +134,20 @@ func AssertNotExistsBean(t TestingT, bean any, conditions ...any) {
 
 // AssertCount assert the count of a bean
 func AssertCount(t TestingT, bean, expected any) bool {
+	t.Helper()
 	return assert.EqualValues(t, expected, GetCount(t, bean))
 }
 
 // AssertInt64InRange assert value is in range [low, high]
-func AssertInt64InRange(t assert.TestingT, low, high, value int64) {
+func AssertInt64InRange(t TestingT, low, high, value int64) {
+	t.Helper()
 	assert.True(t, value >= low && value <= high,
 		"Expected value in range [%d, %d], found %d", low, high, value)
 }
 
 // GetCountByCond get the count of database entries matching bean
 func GetCountByCond(t TestingT, tableName string, cond builder.Cond) int64 {
+	t.Helper()
 	e := db.GetEngine(t.Context())
 	count, err := e.Table(tableName).Where(cond).Count()
 	assert.NoError(t, err)