From 8375ea1702fe4f0142d7e6f6e3e93f876f483522 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Sun, 9 Apr 2023 03:45:18 +0000 Subject: [PATCH 1/9] fix --- models/migrations/v1_20/v252.go | 47 +++++++++++++++++++++++++++++++++ routers/web/org/teams.go | 44 +++++++++++++++--------------- 2 files changed, 69 insertions(+), 22 deletions(-) create mode 100644 models/migrations/v1_20/v252.go diff --git a/models/migrations/v1_20/v252.go b/models/migrations/v1_20/v252.go new file mode 100644 index 0000000000000..ab61cd9b8b36e --- /dev/null +++ b/models/migrations/v1_20/v252.go @@ -0,0 +1,47 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_20 //nolint + +import ( + "code.gitea.io/gitea/modules/log" + + "xorm.io/xorm" +) + +func FixIncorrectAdminTeamUnitAccessMode(x *xorm.Engine) error { + type UnitType int + type AccessMode int + + type TeamUnit struct { + ID int64 `xorm:"pk autoincr"` + OrgID int64 `xorm:"INDEX"` + TeamID int64 `xorm:"UNIQUE(s)"` + Type UnitType `xorm:"UNIQUE(s)"` + AccessMode AccessMode + } + + const ( + // AccessModeAdmin admin access + AccessModeAdmin = 3 + ) + + sess := x.NewSession() + defer sess.Close() + + if err := sess.Begin(); err != nil { + return err + } + + count, err := sess.Table("team_unit"). + Where("team_id IN (SELECT id FROM team WHERE authorize = ?)", AccessModeAdmin). + Update(&TeamUnit{ + AccessMode: AccessModeAdmin, + }) + if err != nil { + return err + } + log.Debug("Updated %d admin team unit access mode to belong to admin instead of none", count) + + return sess.Commit() +} diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index 1ed7980145491..b789ea791ef5c 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -264,14 +264,18 @@ func NewTeam(ctx *context.Context) { ctx.HTML(http.StatusOK, tplTeamNew) } -func getUnitPerms(forms url.Values) map[unit_model.Type]perm.AccessMode { +func getUnitPerms(forms url.Values, teamPermission perm.AccessMode) map[unit_model.Type]perm.AccessMode { unitPerms := make(map[unit_model.Type]perm.AccessMode) for k, v := range forms { if strings.HasPrefix(k, "unit_") { t, _ := strconv.Atoi(k[5:]) if t > 0 { vv, _ := strconv.Atoi(v[0]) - unitPerms[unit_model.Type(t)] = perm.AccessMode(vv) + if teamPermission >= perm.AccessModeAdmin { + unitPerms[unit_model.Type(t)] = teamPermission + } else { + unitPerms[unit_model.Type(t)] = perm.AccessMode(vv) + } } } } @@ -282,8 +286,8 @@ func getUnitPerms(forms url.Values) map[unit_model.Type]perm.AccessMode { func NewTeamPost(ctx *context.Context) { form := web.GetForm(ctx).(*forms.CreateTeamForm) includesAllRepositories := form.RepoAccess == "all" - unitPerms := getUnitPerms(ctx.Req.Form) 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. @@ -299,17 +303,15 @@ func NewTeamPost(ctx *context.Context) { CanCreateOrgRepo: form.CanCreateOrgRepo, } - if t.AccessMode < perm.AccessModeAdmin { - units := make([]*org_model.TeamUnit, 0, len(unitPerms)) - for tp, perm := range unitPerms { - units = append(units, &org_model.TeamUnit{ - OrgID: ctx.Org.Organization.ID, - Type: tp, - AccessMode: perm, - }) - } - t.Units = units + units := make([]*org_model.TeamUnit, 0, len(unitPerms)) + for tp, perm := range unitPerms { + units = append(units, &org_model.TeamUnit{ + OrgID: ctx.Org.Organization.ID, + Type: tp, + AccessMode: perm, + }) } + t.Units = units ctx.Data["Title"] = ctx.Org.Organization.FullName ctx.Data["PageIsOrgTeams"] = true @@ -432,7 +434,13 @@ func EditTeam(ctx *context.Context) { func EditTeamPost(ctx *context.Context) { form := web.GetForm(ctx).(*forms.CreateTeamForm) t := ctx.Org.Team - unitPerms := getUnitPerms(ctx.Req.Form) + newAccessMode := perm.ParseAccessMode(form.Permission) + unitPerms := getUnitPerms(ctx.Req.Form, newAccessMode) + if newAccessMode < 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. + newAccessMode = unit_model.MinUnitAccessMode(unitPerms) + } isAuthChanged := false isIncludeAllChanged := false includesAllRepositories := form.RepoAccess == "all" @@ -443,14 +451,6 @@ func EditTeamPost(ctx *context.Context) { ctx.Data["Units"] = unit_model.Units if !t.IsOwnerTeam() { - // Validate permission level. - newAccessMode := perm.ParseAccessMode(form.Permission) - if newAccessMode < 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. - newAccessMode = unit_model.MinUnitAccessMode(unitPerms) - } - t.Name = form.TeamName if t.AccessMode != newAccessMode { isAuthChanged = true From 0003adba86854633d6aa8d823472c4935a490196 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Sun, 9 Apr 2023 03:56:13 +0000 Subject: [PATCH 2/9] fix --- models/migrations/migrations.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 07240c8e69ed5..35a18fb7f2bbc 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -481,6 +481,8 @@ var migrations = []Migration{ NewMigration("Change Container Metadata", v1_20.ChangeContainerMetadataMultiArch), // v251 -> v252 NewMigration("Fix incorrect owner team unit access mode", v1_20.FixIncorrectOwnerTeamUnitAccessMode), + // v252 -> v253 + NewMigration("Fix incorrect admin team unit access mode", v1_20.FixIncorrectAdminTeamUnitAccessMode), } // GetCurrentDBVersion returns the current db version From 13346a7e666abce0c9e8766b9e0ec7020405ae1a Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 10 Apr 2023 00:20:45 +0000 Subject: [PATCH 3/9] improve comment --- routers/web/org/teams.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index b789ea791ef5c..d9be202d11739 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -437,7 +437,7 @@ func EditTeamPost(ctx *context.Context) { newAccessMode := perm.ParseAccessMode(form.Permission) unitPerms := getUnitPerms(ctx.Req.Form, newAccessMode) if newAccessMode < perm.AccessModeAdmin { - // if p is less than admin accessmode, then it should be general accessmode, + // 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) } From df91859e9327d351a856b4cc8bc8925c277d0fc0 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 10 Apr 2023 00:22:25 +0000 Subject: [PATCH 4/9] always update unit permission --- routers/web/org/teams.go | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index d9be202d11739..3dfc41784e968 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -467,20 +467,18 @@ func EditTeamPost(ctx *context.Context) { } t.Description = form.Description - if t.AccessMode < perm.AccessModeAdmin { - 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, - }) - } - if err := org_model.UpdateTeamUnits(t, units); err != nil { - ctx.Error(http.StatusInternalServerError, "UpdateTeamUnits", err.Error()) - return - } + 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, + }) + } + if err := org_model.UpdateTeamUnits(t, units); err != nil { + ctx.Error(http.StatusInternalServerError, "UpdateTeamUnits", err.Error()) + return } if ctx.HasError() { From a26eaca62fe5c0eecd4e4adc0c3c7ec562955310 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 10 Apr 2023 01:17:30 +0000 Subject: [PATCH 5/9] improve getUnitPerms --- routers/web/org/teams.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index 3dfc41784e968..d4b3be6139242 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -5,6 +5,7 @@ package org import ( + "fmt" "net/http" "net/url" "path" @@ -16,6 +17,7 @@ import ( org_model "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" unit_model "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" @@ -266,16 +268,17 @@ func NewTeam(ctx *context.Context) { func getUnitPerms(forms url.Values, teamPermission perm.AccessMode) map[unit_model.Type]perm.AccessMode { unitPerms := make(map[unit_model.Type]perm.AccessMode) - for k, v := range forms { - if strings.HasPrefix(k, "unit_") { - t, _ := strconv.Atoi(k[5:]) - if t > 0 { - vv, _ := strconv.Atoi(v[0]) - if teamPermission >= perm.AccessModeAdmin { - unitPerms[unit_model.Type(t)] = teamPermission - } else { - unitPerms[unit_model.Type(t)] = perm.AccessMode(vv) - } + for _, ut := range unit.AllRepoUnitTypes { + // Default accessmode is none + unitPerms[ut] = perm.AccessModeNone + + v, ok := forms[fmt.Sprintf("unit_%d", ut)] + if ok { + vv, _ := strconv.Atoi(v[0]) + if teamPermission >= perm.AccessModeAdmin { + unitPerms[ut] = teamPermission + } else { + unitPerms[ut] = perm.AccessMode(vv) } } } From da83408a5acc1e9f9a5efb429c02f27dcaa3d511 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 10 Apr 2023 01:34:16 +0000 Subject: [PATCH 6/9] improve tmpl --- routers/web/org/teams.go | 20 ++++++++++++-------- templates/org/team/new.tmpl | 4 ++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index d4b3be6139242..129e593236a55 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -277,8 +277,15 @@ func getUnitPerms(forms url.Values, teamPermission perm.AccessMode) map[unit_mod vv, _ := strconv.Atoi(v[0]) if teamPermission >= perm.AccessModeAdmin { unitPerms[ut] = teamPermission + // Don't allow `TypeExternal{Tracker,Wiki}` to influence this as they can only be set to READ perms. + if ut == unit.TypeExternalTracker || ut == unit.TypeExternalWiki { + unitPerms[ut] = perm.AccessModeRead + } } else { unitPerms[ut] = perm.AccessMode(vv) + if unitPerms[ut] >= perm.AccessModeAdmin { + unitPerms[ut] = perm.AccessModeWrite + } } } } @@ -427,8 +434,8 @@ func SearchTeam(ctx *context.Context) { func EditTeam(ctx *context.Context) { ctx.Data["Title"] = ctx.Org.Organization.FullName ctx.Data["PageIsOrgTeams"] = true - ctx.Data["team_name"] = ctx.Org.Team.Name - ctx.Data["desc"] = ctx.Org.Team.Description + ctx.Org.Team.LoadUnits(ctx) + ctx.Data["Team"] = ctx.Org.Team ctx.Data["Units"] = unit_model.Units ctx.HTML(http.StatusOK, tplTeamNew) } @@ -470,19 +477,16 @@ func EditTeamPost(ctx *context.Context) { } t.Description = form.Description - units := make([]org_model.TeamUnit, 0, len(unitPerms)) + units := make([]*org_model.TeamUnit, 0, len(unitPerms)) for tp, perm := range unitPerms { - units = append(units, org_model.TeamUnit{ + units = append(units, &org_model.TeamUnit{ OrgID: t.OrgID, TeamID: t.ID, Type: tp, AccessMode: perm, }) } - if err := org_model.UpdateTeamUnits(t, units); err != nil { - ctx.Error(http.StatusInternalServerError, "UpdateTeamUnits", err.Error()) - return - } + t.Units = units if ctx.HasError() { ctx.HTML(http.StatusOK, tplTeamNew) diff --git a/templates/org/team/new.tmpl b/templates/org/team/new.tmpl index 195f8bdcd69d9..2e65d63580319 100644 --- a/templates/org/team/new.tmpl +++ b/templates/org/team/new.tmpl @@ -109,7 +109,7 @@
- +
@@ -137,7 +137,7 @@ {{else}} {{if not (eq .Team.LowerName "owners")}} - + {{end}} {{end}} From ff429da7cbe96ba4920026158ce51b4001b9c59b Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 10 Apr 2023 02:04:46 +0000 Subject: [PATCH 7/9] fix lint --- routers/web/org/teams.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index 129e593236a55..e2ec6d87858eb 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -17,7 +17,6 @@ import ( org_model "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" unit_model "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" @@ -268,7 +267,7 @@ func NewTeam(ctx *context.Context) { 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.AllRepoUnitTypes { + for _, ut := range unit_model.AllRepoUnitTypes { // Default accessmode is none unitPerms[ut] = perm.AccessModeNone @@ -278,7 +277,7 @@ func getUnitPerms(forms url.Values, teamPermission perm.AccessMode) map[unit_mod if teamPermission >= perm.AccessModeAdmin { unitPerms[ut] = teamPermission // Don't allow `TypeExternal{Tracker,Wiki}` to influence this as they can only be set to READ perms. - if ut == unit.TypeExternalTracker || ut == unit.TypeExternalWiki { + if ut == unit_model.TypeExternalTracker || ut == unit_model.TypeExternalWiki { unitPerms[ut] = perm.AccessModeRead } } else { @@ -434,7 +433,10 @@ func SearchTeam(ctx *context.Context) { func EditTeam(ctx *context.Context) { ctx.Data["Title"] = ctx.Org.Organization.FullName ctx.Data["PageIsOrgTeams"] = true - ctx.Org.Team.LoadUnits(ctx) + if err := ctx.Org.Team.LoadUnits(ctx); err != nil { + ctx.ServerError("LoadUnits", err) + return + } ctx.Data["Team"] = ctx.Org.Team ctx.Data["Units"] = unit_model.Units ctx.HTML(http.StatusOK, tplTeamNew) From 358ab99fcfc07f8595cc289d8cb363a3fd8ac62d Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Thu, 13 Apr 2023 06:02:40 +0000 Subject: [PATCH 8/9] fix api --- routers/api/v1/org/team.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index 50439251cc05f..024fee34693c6 100644 --- a/routers/api/v1/org/team.go +++ b/routers/api/v1/org/team.go @@ -166,6 +166,21 @@ func attachTeamUnitsMap(team *organization.Team, unitsMap map[string]string) { } } +func attachAdminTeamUnits(team *organization.Team) { + team.Units = make([]*organization.TeamUnit, 0, len(unit_model.AllRepoUnitTypes)) + for _, ut := range unit_model.AllRepoUnitTypes { + up := perm.AccessModeAdmin + if ut == unit_model.TypeExternalTracker || ut == unit_model.TypeExternalWiki { + up = perm.AccessModeRead + } + team.Units = append(team.Units, &organization.TeamUnit{ + OrgID: team.OrgID, + Type: ut, + AccessMode: up, + }) + } +} + // CreateTeam api for create a team func CreateTeam(ctx *context.APIContext) { // swagger:operation POST /orgs/{org}/teams organization orgCreateTeam @@ -213,6 +228,8 @@ func CreateTeam(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "getTeamUnits", errors.New("units permission should not be empty")) return } + } else { + attachAdminTeamUnits(team) } if err := models.NewTeam(team); err != nil { @@ -300,6 +317,8 @@ func EditTeam(ctx *context.APIContext) { } else if len(form.Units) > 0 { attachTeamUnits(team, form.Units) } + } else { + attachAdminTeamUnits(team) } if err := models.UpdateTeam(team, isAuthChanged, isIncludeAllChanged); err != nil { From 928b43073553b755ebc32b4f0150c3442913c847 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Thu, 13 Apr 2023 06:11:16 +0000 Subject: [PATCH 9/9] add test --- tests/integration/api_team_test.go | 31 ++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/integration/api_team_test.go b/tests/integration/api_team_test.go index 2801fdc98934e..934e6bf23046f 100644 --- a/tests/integration/api_team_test.go +++ b/tests/integration/api_team_test.go @@ -12,6 +12,7 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" @@ -189,6 +190,36 @@ func TestAPITeam(t *testing.T) { req = NewRequestf(t, "DELETE", "/api/v1/teams/%d?token="+token, teamID) MakeRequest(t, req, http.StatusNoContent) unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID}) + + // Create admin team + teamToCreate = &api.CreateTeamOption{ + Name: "teamadmin", + Description: "team admin", + IncludesAllRepositories: true, + Permission: "admin", + } + req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams?token=%s", org.Name, token), teamToCreate) + resp = MakeRequest(t, req, http.StatusCreated) + apiTeam = api.Team{} + DecodeJSON(t, resp, &apiTeam) + for _, ut := range unit.AllRepoUnitTypes { + up := perm.AccessModeAdmin + if ut == unit.TypeExternalTracker || ut == unit.TypeExternalWiki { + up = perm.AccessModeRead + } + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ + OrgID: org.ID, + TeamID: apiTeam.ID, + Type: ut, + AccessMode: up, + }) + } + teamID = apiTeam.ID + + // Delete team. + req = NewRequestf(t, "DELETE", "/api/v1/teams/%d?token="+token, teamID) + MakeRequest(t, req, http.StatusNoContent) + unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID}) } func checkTeamResponse(t *testing.T, testName string, apiTeam *api.Team, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) {