diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 19b02ccab9..55bbe6938c 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -518,7 +518,7 @@ func updateTeamWhitelist(ctx context.Context, repo *repo_model.Repository, curre return currentWhitelist, nil } - teams, err := organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) + teams, err := organization.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypeCode, unit.TypePullRequests) if err != nil { return nil, fmt.Errorf("GetTeamsWithAccessToRepo [org_id: %d, repo_id: %d]: %v", repo.OwnerID, repo.ID, err) } diff --git a/models/organization/org.go b/models/organization/org.go index dc889ea17f..0f3aef146c 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -602,8 +602,3 @@ func getUserTeamIDsQueryBuilder(orgID, userID int64) *builder.Builder { "team_user.uid": userID, }) } - -// TeamsWithAccessToRepo returns all teams that have given access level to the repository. -func (org *Organization) TeamsWithAccessToRepo(ctx context.Context, repoID int64, mode perm.AccessMode) ([]*Team, error) { - return GetTeamsWithAccessToRepo(ctx, org.ID, repoID, mode) -} diff --git a/models/organization/team_repo.go b/models/organization/team_repo.go index 53edd203a8..b3e266dbc7 100644 --- a/models/organization/team_repo.go +++ b/models/organization/team_repo.go @@ -9,6 +9,8 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/unit" + + "xorm.io/builder" ) // TeamRepo represents an team-repository relation. @@ -48,26 +50,27 @@ func RemoveTeamRepo(ctx context.Context, teamID, repoID int64) error { return err } -// GetTeamsWithAccessToRepo returns all teams in an organization that have given access level to the repository. -func GetTeamsWithAccessToRepo(ctx context.Context, orgID, repoID int64, mode perm.AccessMode) ([]*Team, 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) ([]*Team, error) { teams := make([]*Team, 0, 5) - return teams, db.GetEngine(ctx).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). - OrderBy("name"). - Find(&teams) -} -// GetTeamsWithAccessToRepoUnit returns all teams in an organization that have given access level to the repository special unit. -func GetTeamsWithAccessToRepoUnit(ctx context.Context, orgID, repoID int64, mode perm.AccessMode, unitType unit.Type) ([]*Team, error) { - teams := make([]*Team, 0, 5) - return teams, db.GetEngine(ctx).Where("team_unit.access_mode >= ?", mode). + sub := builder.Select("team_id").From("team_unit"). + Where(builder.Expr("team_unit.team_id = team.id")). + And(builder.In("team_unit.type", append([]unit.Type{unitType}, unitTypesMore...))). + And(builder.Expr("team_unit.access_mode >= ?", mode)) + + err := db.GetEngine(ctx). Join("INNER", "team_repo", "team_repo.team_id = team.id"). - Join("INNER", "team_unit", "team_unit.team_id = team.id"). And("team_repo.org_id = ?", orgID). And("team_repo.repo_id = ?", repoID). - And("team_unit.type = ?", unitType). + And(builder.Or( + builder.Expr("team.authorize >= ?", mode), + builder.In("team.id", sub), + )). OrderBy("name"). Find(&teams) + + return teams, err } diff --git a/models/organization/team_repo_test.go b/models/organization/team_repo_test.go index c0d6750df9..73a06a93c2 100644 --- a/models/organization/team_repo_test.go +++ b/models/organization/team_repo_test.go @@ -22,7 +22,7 @@ func TestGetTeamsWithAccessToRepoUnit(t *testing.T) { org41 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 41}) repo61 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 61}) - teams, err := organization.GetTeamsWithAccessToRepoUnit(db.DefaultContext, org41.ID, repo61.ID, perm.AccessModeRead, unit.TypePullRequests) + teams, err := organization.GetTeamsWithAccessToAnyRepoUnit(db.DefaultContext, org41.ID, repo61.ID, perm.AccessModeRead, unit.TypePullRequests) assert.NoError(t, err) if assert.Len(t, teams, 2) { assert.EqualValues(t, 21, teams[0].ID) diff --git a/models/perm/access/repo_permission.go b/models/perm/access/repo_permission.go index 45efb192c8..fe429b1f97 100644 --- a/models/perm/access/repo_permission.go +++ b/models/perm/access/repo_permission.go @@ -267,7 +267,6 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use perm.units = repo.Units // anonymous user visit private repo. - // TODO: anonymous user visit public unit of private repo??? if user == nil && repo.IsPrivate { perm.AccessMode = perm_model.AccessModeNone return perm, nil @@ -286,7 +285,8 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use } // Prevent strangers from checking out public repo of private organization/users - // Allow user if they are collaborator of a repo within a private user or a private organization but not a member of the organization itself + // Allow user if they are a collaborator of a repo within a private user or a private organization but not a member of the organization itself + // TODO: rename it to "IsOwnerVisibleToDoer" if !organization.HasOrgOrUserVisible(ctx, repo.Owner, user) && !isCollaborator { perm.AccessMode = perm_model.AccessModeNone return perm, nil @@ -304,7 +304,7 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use return perm, nil } - // plain user + // plain user TODO: this check should be replaced, only need to check collaborator access mode perm.AccessMode, err = accessLevel(ctx, user, repo) if err != nil { return perm, err @@ -314,6 +314,19 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use return perm, nil } + // now: the owner is visible to doer, if the repo is public, then the min access mode is read + minAccessMode := util.Iif(!repo.IsPrivate && !user.IsRestricted, perm_model.AccessModeRead, perm_model.AccessModeNone) + perm.AccessMode = max(perm.AccessMode, minAccessMode) + + // get units mode from teams + teams, err := organization.GetUserRepoTeams(ctx, repo.OwnerID, user.ID, repo.ID) + if err != nil { + return perm, err + } + if len(teams) == 0 { + return perm, nil + } + perm.unitsMode = make(map[unit.Type]perm_model.AccessMode) // Collaborators on organization @@ -323,12 +336,6 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use } } - // get units mode from teams - teams, err := organization.GetUserRepoTeams(ctx, repo.OwnerID, user.ID, repo.ID) - if err != nil { - return perm, err - } - // if user in an owner team for _, team := range teams { if team.HasAdminAccess() { @@ -339,19 +346,12 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use } for _, u := range repo.Units { - var found bool for _, team := range teams { + unitAccessMode := minAccessMode if teamMode, exist := team.UnitAccessModeEx(ctx, u.Type); exist { - perm.unitsMode[u.Type] = max(perm.unitsMode[u.Type], teamMode) - found = true - } - } - - // for a public repo on an organization, a non-restricted user has read permission on non-team defined units. - if !found && !repo.IsPrivate && !user.IsRestricted { - if _, ok := perm.unitsMode[u.Type]; !ok { - perm.unitsMode[u.Type] = perm_model.AccessModeRead + unitAccessMode = max(perm.unitsMode[u.Type], unitAccessMode, teamMode) } + perm.unitsMode[u.Type] = unitAccessMode } } diff --git a/models/perm/access/repo_permission_test.go b/models/perm/access/repo_permission_test.go index 024f4400b3..c8675b1ded 100644 --- a/models/perm/access/repo_permission_test.go +++ b/models/perm/access/repo_permission_test.go @@ -6,12 +6,16 @@ package access import ( "testing" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" perm_model "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" + "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestHasAnyUnitAccess(t *testing.T) { @@ -152,3 +156,45 @@ func TestUnitAccessMode(t *testing.T) { } assert.Equal(t, perm_model.AccessModeRead, perm.UnitAccessMode(unit.TypeWiki), "has unit, and map, use map") } + +func TestGetUserRepoPermission(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + ctx := t.Context() + repo32 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 32}) // org public repo + require.NoError(t, repo32.LoadOwner(ctx)) + require.True(t, repo32.Owner.IsOrganization()) + + require.NoError(t, db.TruncateBeans(ctx, &organization.Team{}, &organization.TeamUser{}, &organization.TeamRepo{}, &organization.TeamUnit{})) + org := repo32.Owner + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + team := &organization.Team{OrgID: org.ID, LowerName: "test_team"} + require.NoError(t, db.Insert(ctx, team)) + + t.Run("DoerInTeamWithNoRepo", func(t *testing.T) { + require.NoError(t, db.Insert(ctx, &organization.TeamUser{OrgID: org.ID, TeamID: team.ID, UID: user.ID})) + perm, err := GetUserRepoPermission(ctx, repo32, user) + require.NoError(t, err) + assert.Equal(t, perm_model.AccessModeRead, perm.AccessMode) + assert.Nil(t, perm.unitsMode) // doer in the team, but has no access to the repo + }) + + require.NoError(t, db.Insert(ctx, &organization.TeamRepo{OrgID: org.ID, TeamID: team.ID, RepoID: repo32.ID})) + require.NoError(t, db.Insert(ctx, &organization.TeamUnit{OrgID: org.ID, TeamID: team.ID, Type: unit.TypeCode, AccessMode: perm_model.AccessModeNone})) + t.Run("DoerWithTeamUnitAccessNone", func(t *testing.T) { + perm, err := GetUserRepoPermission(ctx, repo32, user) + require.NoError(t, err) + assert.Equal(t, perm_model.AccessModeRead, perm.AccessMode) + assert.Equal(t, perm_model.AccessModeRead, perm.unitsMode[unit.TypeCode]) + assert.Equal(t, perm_model.AccessModeRead, perm.unitsMode[unit.TypeIssues]) + }) + + require.NoError(t, db.TruncateBeans(ctx, &organization.TeamUnit{})) + require.NoError(t, db.Insert(ctx, &organization.TeamUnit{OrgID: org.ID, TeamID: team.ID, Type: unit.TypeCode, AccessMode: perm_model.AccessModeWrite})) + t.Run("DoerWithTeamUnitAccessWrite", func(t *testing.T) { + perm, err := GetUserRepoPermission(ctx, repo32, user) + require.NoError(t, err) + assert.Equal(t, perm_model.AccessModeRead, perm.AccessMode) + assert.Equal(t, perm_model.AccessModeWrite, perm.unitsMode[unit.TypeCode]) + assert.Equal(t, perm_model.AccessModeRead, perm.unitsMode[unit.TypeIssues]) + }) +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 0efc9d04c2..3d3bafbffd 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2163,6 +2163,7 @@ settings.collaboration.write = Write settings.collaboration.read = Read settings.collaboration.owner = Owner settings.collaboration.undefined = Undefined +settings.collaboration.per_unit = Unit Permissions settings.hooks = Webhooks settings.githooks = Git Hooks settings.basic_settings = Basic Settings diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index 676c6d0c63..0ec7cfddc5 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -283,11 +283,22 @@ func NewTeam(ctx *context.Context) { } // 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. +// The existing teams won't inherit the correct admin permission for the new unit. +// The full history is like this: +// 1. There was only "team", no "team unit", so "team.authorize" was used to determine the team permission. +// 2. Later, "team unit" was introduced, then the usage of "team.authorize" became inconsistent, and causes various bugs. +// - Sometimes, "team.authorize" is used to determine the team permission, e.g. admin, owner +// - Sometimes, "team unit" is used not really used and "team unit" is used. +// - Some functions like `GetTeamsWithAccessToAnyRepoUnit` use both. +// +// 3. After introducing "team unit" and more unclear changes, it becomes difficult to maintain team permissions. +// - Org owner need to click the permission for each unit, but can't just set a common "write" permission for all units. +// +// Ideally, "team.authorize=write" should mean the team has write access to all units including newly (future) added ones. 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 { - // Default accessmode is none + // Default access mode is none unitPerms[ut] = perm.AccessModeNone v, ok := forms[fmt.Sprintf("unit_%d", ut)] diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index f241242f02..0eea5e3f34 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -17,6 +17,7 @@ import ( "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/web" @@ -89,7 +90,7 @@ func SettingsProtectedBranch(c *context.Context) { c.Data["recent_status_checks"] = contexts if c.Repo.Owner.IsOrganization() { - teams, err := organization.OrgFromUser(c.Repo.Owner).TeamsWithAccessToRepo(c, c.Repo.Repository.ID, perm.AccessModeRead) + teams, err := organization.GetTeamsWithAccessToAnyRepoUnit(c, c.Repo.Owner.ID, c.Repo.Repository.ID, perm.AccessModeRead, unit.TypeCode, unit.TypePullRequests) if err != nil { c.ServerError("Repo.Owner.TeamsWithAccessToRepo", err) return diff --git a/routers/web/repo/setting/protected_tag.go b/routers/web/repo/setting/protected_tag.go index 33692778d5..50f5a28c4c 100644 --- a/routers/web/repo/setting/protected_tag.go +++ b/routers/web/repo/setting/protected_tag.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" + "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/templates" @@ -156,7 +157,7 @@ func setTagsContext(ctx *context.Context) error { ctx.Data["Users"] = users if ctx.Repo.Owner.IsOrganization() { - teams, err := organization.OrgFromUser(ctx.Repo.Owner).TeamsWithAccessToRepo(ctx, ctx.Repo.Repository.ID, perm.AccessModeRead) + teams, err := organization.GetTeamsWithAccessToAnyRepoUnit(ctx, ctx.Repo.Owner.ID, ctx.Repo.Repository.ID, perm.AccessModeRead, unit.TypeCode, unit.TypePullRequests) if err != nil { ctx.ServerError("Repo.Owner.TeamsWithAccessToRepo", err) return err diff --git a/services/convert/convert.go b/services/convert/convert.go index 0723b0d34d..0de3822140 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -149,7 +149,7 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch, repo mergeWhitelistUsernames := getWhitelistEntities(readers, bp.MergeWhitelistUserIDs) approvalsWhitelistUsernames := getWhitelistEntities(readers, bp.ApprovalsWhitelistUserIDs) - teamReaders, err := organization.OrgFromUser(repo.Owner).TeamsWithAccessToRepo(ctx, repo.ID, perm.AccessModeRead) + teamReaders, err := organization.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.Owner.ID, repo.ID, perm.AccessModeRead, unit.TypeCode, unit.TypePullRequests) if err != nil { log.Error("Repo.Owner.TeamsWithAccessToRepo: %v", err) } @@ -727,7 +727,7 @@ func ToTagProtection(ctx context.Context, pt *git_model.ProtectedTag, repo *repo whitelistUsernames := getWhitelistEntities(readers, pt.AllowlistUserIDs) - teamReaders, err := organization.OrgFromUser(repo.Owner).TeamsWithAccessToRepo(ctx, repo.ID, perm.AccessModeRead) + teamReaders, err := organization.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.Owner.ID, repo.ID, perm.AccessModeRead, unit.TypeCode, unit.TypePullRequests) if err != nil { log.Error("Repo.Owner.TeamsWithAccessToRepo: %v", err) } diff --git a/services/issue/assignee.go b/services/issue/assignee.go index a9494b7ab4..ba9c91e0ed 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -304,7 +304,7 @@ func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, rep // If the repo's owner is an organization, members of teams with read permission on pull requests can change reviewers if repo.Owner.IsOrganization() { - teams, err := organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) + teams, err := organization.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) if err != nil { log.Error("GetTeamsWithAccessToRepo: %v", err) return false diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go index bf0d8cb298..52f2f3401c 100644 --- a/services/pull/reviewer.go +++ b/services/pull/reviewer.go @@ -85,5 +85,5 @@ func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*orga return nil, nil } - return organization.GetTeamsWithAccessToRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) + return organization.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) } diff --git a/templates/repo/settings/collaboration.tmpl b/templates/repo/settings/collaboration.tmpl index 7064b4c7ba..e941a2c4b6 100644 --- a/templates/repo/settings/collaboration.tmpl +++ b/templates/repo/settings/collaboration.tmpl @@ -63,13 +63,33 @@ {{.Name}}