Merge pull request '[BUG] Don't allow owner team with incorrect unit access' (#5529) from gusted/forgejo-owners-team-description into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5529
Reviewed-by: Otto <otto@codeberg.org>

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

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

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
+}

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})
+}

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)

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

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})
+}