From 9de9034400db7d361b106b3b4a18e5a2c42f61aa Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 11 Oct 2024 14:48:47 +0200 Subject: [PATCH] [BUG] Don't allow owner team with incorrect unit access - On editting a team, only update the units if the team isn't the 'Owners' team. Otherwise the 'Owners' team end up having all of their unit access modes set to 'None'; because the request form doesn't send over any units, as it's simply not shown in the UI. - Adds a database inconstency check and fix for the case where the 'Owners' team is affected by this bug. - Adds unit test. - Adds integration test. - Resolves #5528 - Regression of https://github.com/go-gitea/gitea/pull/24012 --- .../TestInconsistentOwnerTeam/team.yml | 10 ++++ .../TestInconsistentOwnerTeam/team_unit.yml | 59 +++++++++++++++++++ models/organization/team.go | 40 +++++++++++++ models/organization/team_test.go | 50 ++++++++++++++++ routers/web/org/teams.go | 21 +++---- services/doctor/dbconsistency.go | 7 +++ tests/integration/org_test.go | 22 +++++++ 7 files changed, 199 insertions(+), 10 deletions(-) create mode 100644 models/organization/TestInconsistentOwnerTeam/team.yml create mode 100644 models/organization/TestInconsistentOwnerTeam/team_unit.yml diff --git a/models/organization/TestInconsistentOwnerTeam/team.yml b/models/organization/TestInconsistentOwnerTeam/team.yml new file mode 100644 index 0000000000..90e3ad43b0 --- /dev/null +++ b/models/organization/TestInconsistentOwnerTeam/team.yml @@ -0,0 +1,10 @@ +- + id: 1000 + org_id: 1000 + lower_name: owners + name: Owners + authorize: 4 # owner + num_repos: 0 + num_members: 0 + includes_all_repositories: true + can_create_org_repo: true diff --git a/models/organization/TestInconsistentOwnerTeam/team_unit.yml b/models/organization/TestInconsistentOwnerTeam/team_unit.yml new file mode 100644 index 0000000000..91e03d6a9a --- /dev/null +++ b/models/organization/TestInconsistentOwnerTeam/team_unit.yml @@ -0,0 +1,59 @@ +- + id: 1000 + team_id: 1000 + type: 1 + access_mode: 0 # None + +- + id: 1001 + team_id: 1000 + type: 2 + access_mode: 0 + +- + id: 1002 + team_id: 1000 + type: 3 + access_mode: 0 + +- + id: 1003 + team_id: 1000 + type: 4 + access_mode: 0 + +- + id: 1004 + team_id: 1000 + type: 5 + access_mode: 0 + +- + id: 1005 + team_id: 1000 + type: 6 + access_mode: 0 + +- + id: 1006 + team_id: 1000 + type: 7 + access_mode: 0 + +- + id: 1007 + team_id: 1000 + type: 8 + access_mode: 0 + +- + id: 1008 + team_id: 1000 + type: 9 + access_mode: 0 + +- + id: 1009 + team_id: 1000 + type: 10 + access_mode: 0 diff --git a/models/organization/team.go b/models/organization/team.go index 1b737c2d3d..ddff32cb8c 100644 --- a/models/organization/team.go +++ b/models/organization/team.go @@ -268,3 +268,43 @@ func IncrTeamRepoNum(ctx context.Context, teamID int64) error { _, err := db.GetEngine(ctx).Incr("num_repos").ID(teamID).Update(new(Team)) return err } + +// CountInconsistentOwnerTeams returns the amount of owner teams that have all of +// their access modes set to "None". +func CountInconsistentOwnerTeams(ctx context.Context) (int64, error) { + return db.GetEngine(ctx).Table("team"). + Join("INNER", "team_unit", "`team`.id = `team_unit`.team_id"). + Where("`team`.lower_name = ?", strings.ToLower(OwnerTeamName)). + GroupBy("`team_unit`.team_id"). + Having("SUM(`team_unit`.access_mode) = 0"). + Count() +} + +// FixInconsistentOwnerTeams fixes inconsistent owner teams that have all of +// their access modes set to "None", it sets it back to "Owner". +func FixInconsistentOwnerTeams(ctx context.Context) (int64, error) { + teamIDs := []int64{} + if err := db.GetEngine(ctx).Table("team"). + Select("`team`.id"). + Join("INNER", "team_unit", "`team`.id = `team_unit`.team_id"). + Where("`team`.lower_name = ?", strings.ToLower(OwnerTeamName)). + GroupBy("`team_unit`.team_id"). + Having("SUM(`team_unit`.access_mode) = 0"). + Find(&teamIDs); err != nil { + return 0, err + } + + if err := db.Iterate(ctx, builder.In("team_id", teamIDs), func(ctx context.Context, bean *TeamUnit) error { + if bean.Type == unit.TypeExternalTracker || bean.Type == unit.TypeExternalWiki { + bean.AccessMode = perm.AccessModeRead + } else { + bean.AccessMode = perm.AccessModeOwner + } + _, err := db.GetEngine(ctx).ID(bean.ID).Table("team_unit").Cols("access_mode").Update(bean) + return err + }); err != nil { + return 0, err + } + + return int64(len(teamIDs)), nil +} diff --git a/models/organization/team_test.go b/models/organization/team_test.go index c14c1f181d..601d136d87 100644 --- a/models/organization/team_test.go +++ b/models/organization/team_test.go @@ -4,11 +4,14 @@ package organization_test import ( + "path/filepath" "testing" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/setting" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -198,3 +201,50 @@ func TestUsersInTeamsCount(t *testing.T) { test([]int64{1, 2, 3, 4, 5}, []int64{2, 5}, 2) // userid 2,4 test([]int64{1, 2, 3, 4, 5}, []int64{2, 3, 5}, 3) // userid 2,4,5 } + +func TestInconsistentOwnerTeam(t *testing.T) { + defer unittest.OverrideFixtures( + unittest.FixturesOptions{ + Dir: filepath.Join(setting.AppWorkPath, "models/fixtures/"), + Base: setting.AppWorkPath, + Dirs: []string{"models/organization/TestInconsistentOwnerTeam/"}, + }, + )() + require.NoError(t, unittest.PrepareTestDatabase()) + + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1000, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1001, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1002, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1003, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1004, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1005, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1006, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1007, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1008, TeamID: 1000, AccessMode: perm.AccessModeNone}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1009, TeamID: 1000, AccessMode: perm.AccessModeNone}) + + count, err := organization.CountInconsistentOwnerTeams(db.DefaultContext) + require.NoError(t, err) + require.EqualValues(t, 1, count) + + count, err = organization.FixInconsistentOwnerTeams(db.DefaultContext) + require.NoError(t, err) + require.EqualValues(t, 1, count) + + count, err = organization.CountInconsistentOwnerTeams(db.DefaultContext) + require.NoError(t, err) + require.EqualValues(t, 0, count) + + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1000, AccessMode: perm.AccessModeOwner}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1001, AccessMode: perm.AccessModeOwner}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1002, AccessMode: perm.AccessModeOwner}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1003, AccessMode: perm.AccessModeOwner}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1004, AccessMode: perm.AccessModeOwner}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1007, AccessMode: perm.AccessModeOwner}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1008, AccessMode: perm.AccessModeOwner}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1009, AccessMode: perm.AccessModeOwner}) + + // External wiki and issue + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1005, AccessMode: perm.AccessModeRead}) + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ID: 1006, AccessMode: perm.AccessModeRead}) +} diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index fd7486cacd..45c36743e8 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -507,21 +507,22 @@ func EditTeamPost(ctx *context.Context) { t.IncludesAllRepositories = includesAllRepositories } t.CanCreateOrgRepo = form.CanCreateOrgRepo + + 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, + }) + } + t.Units = units } else { t.CanCreateOrgRepo = true } t.Description = form.Description - 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, - }) - } - t.Units = units if ctx.HasError() { ctx.HTML(http.StatusOK, tplTeamNew) diff --git a/services/doctor/dbconsistency.go b/services/doctor/dbconsistency.go index e3992a5ba5..7fce505d52 100644 --- a/services/doctor/dbconsistency.go +++ b/services/doctor/dbconsistency.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/migrations" + org_model "code.gitea.io/gitea/models/organization" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -177,6 +178,12 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er Fixer: auth_model.DeleteOrphanedOAuth2Applications, FixedMessage: "Removed", }, + { + Name: "Owner teams with no admin access", + Counter: org_model.CountInconsistentOwnerTeams, + Fixer: org_model.FixInconsistentOwnerTeams, + FixedMessage: "Fixed", + }, } // TODO: function to recalc all counters diff --git a/tests/integration/org_test.go b/tests/integration/org_test.go index f907b75c72..912ad0ca0b 100644 --- a/tests/integration/org_test.go +++ b/tests/integration/org_test.go @@ -10,6 +10,9 @@ import ( "testing" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" @@ -243,3 +246,22 @@ func TestOrgDashboardLabels(t *testing.T) { assert.True(t, ok) assert.Contains(t, labelFilterHref, "labels=3%2c-4") } + +func TestOwnerTeamUnit(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + session := loginUser(t, user.Name) + + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{TeamID: 1, Type: unit.TypeIssues, AccessMode: perm.AccessModeOwner}) + + req := NewRequestWithValues(t, "GET", fmt.Sprintf("/org/%s/teams/owners/edit", org.Name), map[string]string{ + "_csrf": GetCSRF(t, session, fmt.Sprintf("/org/%s/teams/owners/edit", org.Name)), + "team_name": "Owners", + "Description": "Just a description", + }) + session.MakeRequest(t, req, http.StatusOK) + + unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{TeamID: 1, Type: unit.TypeIssues, AccessMode: perm.AccessModeOwner}) +}