diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 746a59c6fd..5e9c012c05 100644 --- a/models/issues/issue_update.go +++ b/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 diff --git a/models/organization/org_user.go b/models/organization/org_user.go index 08d936d922..4d7527c15f 100644 --- a/models/organization/org_user.go +++ b/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 } } diff --git a/models/organization/team.go b/models/organization/team.go index 96666da39a..7f3a9b3829 100644 --- a/models/organization/team.go +++ b/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)) diff --git a/models/perm/access/repo_permission.go b/models/perm/access/repo_permission.go index f42c96bbe2..9d0c9f0077 100644 --- a/models/perm/access/repo_permission.go +++ b/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 } } diff --git a/models/unit/unit.go b/models/unit/unit.go index c816fc6c68..4ca676802f 100644 --- a/models/unit/unit.go +++ b/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 -} diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index f70e5dd235..71c21f2dde 100644 --- a/routers/api/v1/org/team.go +++ b/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) diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index a54225f0fd..d1652c1d51 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/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 { diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index aeea3708b2..b1b0dd2c49 100644 --- a/routers/web/org/teams.go +++ b/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 { diff --git a/services/context/org.go b/services/context/org.go index 992a48afa0..c8b6ed09b7 100644 --- a/services/context/org.go +++ b/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) diff --git a/services/org/team.go b/services/org/team.go index ee3bd898ea..6890dafd90 100644 --- a/services/org/team.go +++ b/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 { diff --git a/services/org/team_test.go b/services/org/team_test.go index a7070fadb0..831fe1669f 100644 --- a/services/org/team_test.go +++ b/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()) diff --git a/templates/org/team/new.tmpl b/templates/org/team/new.tmpl index b67c18dd7d..67529ddfba 100644 --- a/templates/org/team/new.tmpl +++ b/templates/org/team/new.tmpl @@ -56,7 +56,7 @@
- + {{ctx.Locale.Tr "org.teams.general_access_helper"}}
diff --git a/templates/org/team/sidebar.tmpl b/templates/org/team/sidebar.tmpl index c4acd8da24..8390bf0acd 100644 --- a/templates/org/team/sidebar.tmpl +++ b/templates/org/team/sidebar.tmpl @@ -42,10 +42,12 @@
  • {{ctx.Locale.Tr "org.teams.can_create_org_repo"}}
  • {{end}} - {{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)*/}}

    {{ctx.Locale.Tr "org.settings.permission"}}

    {{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*/}}

    {{ctx.Locale.Tr "org.settings.permission"}}

    {{ctx.Locale.Tr "org.teams.admin_permission_desc"}} {{else}} diff --git a/tests/integration/api_team_test.go b/tests/integration/api_team_test.go index a2a2f388fd..e54203ed50 100644 --- a/tests/integration/api_team_test.go +++ b/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}) diff --git a/tests/integration/org_test.go b/tests/integration/org_test.go index b1376fe1bf..9a93455858 100644 --- a/tests/integration/org_test.go +++ b/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