mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2024-12-20 20:07:03 +01:00
[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
This commit is contained in:
parent
4cb01ba5da
commit
9de9034400
7 changed files with 199 additions and 10 deletions
10
models/organization/TestInconsistentOwnerTeam/team.yml
Normal file
10
models/organization/TestInconsistentOwnerTeam/team.yml
Normal file
|
@ -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
|
59
models/organization/TestInconsistentOwnerTeam/team_unit.yml
Normal file
59
models/organization/TestInconsistentOwnerTeam/team_unit.yml
Normal file
|
@ -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
|
|
@ -268,3 +268,43 @@ func IncrTeamRepoNum(ctx context.Context, teamID int64) error {
|
||||||
_, err := db.GetEngine(ctx).Incr("num_repos").ID(teamID).Update(new(Team))
|
_, err := db.GetEngine(ctx).Incr("num_repos").ID(teamID).Update(new(Team))
|
||||||
return err
|
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
|
||||||
|
}
|
||||||
|
|
|
@ -4,11 +4,14 @@
|
||||||
package organization_test
|
package organization_test
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"path/filepath"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"code.gitea.io/gitea/models/db"
|
"code.gitea.io/gitea/models/db"
|
||||||
"code.gitea.io/gitea/models/organization"
|
"code.gitea.io/gitea/models/organization"
|
||||||
|
"code.gitea.io/gitea/models/perm"
|
||||||
"code.gitea.io/gitea/models/unittest"
|
"code.gitea.io/gitea/models/unittest"
|
||||||
|
"code.gitea.io/gitea/modules/setting"
|
||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
"github.com/stretchr/testify/require"
|
"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, 5}, 2) // userid 2,4
|
||||||
test([]int64{1, 2, 3, 4, 5}, []int64{2, 3, 5}, 3) // userid 2,4,5
|
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})
|
||||||
|
}
|
||||||
|
|
|
@ -507,11 +507,7 @@ func EditTeamPost(ctx *context.Context) {
|
||||||
t.IncludesAllRepositories = includesAllRepositories
|
t.IncludesAllRepositories = includesAllRepositories
|
||||||
}
|
}
|
||||||
t.CanCreateOrgRepo = form.CanCreateOrgRepo
|
t.CanCreateOrgRepo = form.CanCreateOrgRepo
|
||||||
} else {
|
|
||||||
t.CanCreateOrgRepo = true
|
|
||||||
}
|
|
||||||
|
|
||||||
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 {
|
for tp, perm := range unitPerms {
|
||||||
units = append(units, &org_model.TeamUnit{
|
units = append(units, &org_model.TeamUnit{
|
||||||
|
@ -522,6 +518,11 @@ func EditTeamPost(ctx *context.Context) {
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
t.Units = units
|
t.Units = units
|
||||||
|
} else {
|
||||||
|
t.CanCreateOrgRepo = true
|
||||||
|
}
|
||||||
|
|
||||||
|
t.Description = form.Description
|
||||||
|
|
||||||
if ctx.HasError() {
|
if ctx.HasError() {
|
||||||
ctx.HTML(http.StatusOK, tplTeamNew)
|
ctx.HTML(http.StatusOK, tplTeamNew)
|
||||||
|
|
|
@ -12,6 +12,7 @@ import (
|
||||||
"code.gitea.io/gitea/models/db"
|
"code.gitea.io/gitea/models/db"
|
||||||
issues_model "code.gitea.io/gitea/models/issues"
|
issues_model "code.gitea.io/gitea/models/issues"
|
||||||
"code.gitea.io/gitea/models/migrations"
|
"code.gitea.io/gitea/models/migrations"
|
||||||
|
org_model "code.gitea.io/gitea/models/organization"
|
||||||
repo_model "code.gitea.io/gitea/models/repo"
|
repo_model "code.gitea.io/gitea/models/repo"
|
||||||
"code.gitea.io/gitea/modules/log"
|
"code.gitea.io/gitea/modules/log"
|
||||||
"code.gitea.io/gitea/modules/setting"
|
"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,
|
Fixer: auth_model.DeleteOrphanedOAuth2Applications,
|
||||||
FixedMessage: "Removed",
|
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
|
// TODO: function to recalc all counters
|
||||||
|
|
|
@ -10,6 +10,9 @@ import (
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
auth_model "code.gitea.io/gitea/models/auth"
|
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"
|
"code.gitea.io/gitea/models/unittest"
|
||||||
user_model "code.gitea.io/gitea/models/user"
|
user_model "code.gitea.io/gitea/models/user"
|
||||||
api "code.gitea.io/gitea/modules/structs"
|
api "code.gitea.io/gitea/modules/structs"
|
||||||
|
@ -243,3 +246,22 @@ func TestOrgDashboardLabels(t *testing.T) {
|
||||||
assert.True(t, ok)
|
assert.True(t, ok)
|
||||||
assert.Contains(t, labelFilterHref, "labels=3%2c-4")
|
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})
|
||||||
|
}
|
||||||
|
|
Loading…
Reference in a new issue