mirror of
https://github.com/go-gitea/gitea.git
synced 2026-11-09 17:37:20 -06:00
fix(org): enforce unique team names in the database (#40228)
Concurrent requests could create teams with the same name in one
organization, because only the service checked the name. Editing such a
team, or renaming a team to a taken name, then failed with a 500.
- Add a unique index on `team (org_id, lower_name)`. Migration 368
renames every duplicate except the oldest to a free `<name>-<n>` and
logs each rename. IDs, members, repositories and permissions are kept,
but lookups by the old name (CODEOWNERS, group-to-team sync, mentions,
URLs) now reach only the kept team.
- Team writes report a name rejected by the index as
`ErrTeamAlreadyExist`, so duplicates, concurrent ones included, return
409 from `POST /orgs/{org}/teams` (previously 422) and `PATCH
/teams/{id}`, and a form error in the web UI.
Fixes https://github.com/go-gitea/gitea/issues/39632
---------
Co-authored-by: silverwind <me@silverwind.io>
This commit is contained in:
@@ -444,6 +444,7 @@ func prepareMigrationTasks() []*migration {
|
|||||||
newMigration(365, "Normalize legacy repository unit and login source JSON", v29.NormalizeLegacyConfigJSON),
|
newMigration(365, "Normalize legacy repository unit and login source JSON", v29.NormalizeLegacyConfigJSON),
|
||||||
newMigration(366, "Drop user_open_id table", v29.DropUserOpenIDTable),
|
newMigration(366, "Drop user_open_id table", v29.DropUserOpenIDTable),
|
||||||
newMigration(367, "Add latest_release_id to repository table", v29.AddLatestReleaseIDToRepository),
|
newMigration(367, "Add latest_release_id to repository table", v29.AddLatestReleaseIDToRepository),
|
||||||
|
newMigration(368, "Add unique index for team name", v29.AddUniqueTeamNameIndex),
|
||||||
}
|
}
|
||||||
return preparedMigrations
|
return preparedMigrations
|
||||||
}
|
}
|
||||||
|
|||||||
50
modelmigration/v29/v368.go
Normal file
50
modelmigration/v29/v368.go
Normal file
@@ -0,0 +1,50 @@
|
|||||||
|
// Copyright 2026 The Gitea Authors. All rights reserved.
|
||||||
|
// SPDX-License-Identifier: MIT
|
||||||
|
|
||||||
|
package v29
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"fmt"
|
||||||
|
"strings"
|
||||||
|
|
||||||
|
"gitea.dev/modelmigration/base"
|
||||||
|
"gitea.dev/modules/log"
|
||||||
|
"gitea.dev/modules/util"
|
||||||
|
|
||||||
|
"xorm.io/xorm"
|
||||||
|
)
|
||||||
|
|
||||||
|
func AddUniqueTeamNameIndex(_ctx context.Context, x base.EngineMigration) error {
|
||||||
|
type Team struct {
|
||||||
|
ID int64 `xorm:"pk autoincr"`
|
||||||
|
OrgID int64 `xorm:"INDEX UNIQUE(s)"`
|
||||||
|
LowerName string `xorm:"UNIQUE(s)"`
|
||||||
|
Name string
|
||||||
|
}
|
||||||
|
|
||||||
|
var teams []Team
|
||||||
|
if err := x.Where("id NOT IN (SELECT MIN(id) FROM team GROUP BY org_id, lower_name)").Asc("id").Find(&teams); err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
for _, team := range teams {
|
||||||
|
for suffix := team.ID; ; suffix++ {
|
||||||
|
ending := fmt.Sprintf("-%d", suffix)
|
||||||
|
name := util.TruncateStringBytes(team.Name, 255-len(ending)) + ending
|
||||||
|
exists, err := x.Where("org_id = ? AND lower_name = ?", team.OrgID, strings.ToLower(name)).Exist(new(Team))
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
if !exists {
|
||||||
|
if _, err := x.ID(team.ID).Cols("name", "lower_name").Update(&Team{Name: name, LowerName: strings.ToLower(name)}); err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
log.Warn("renamed duplicate team id=%d in org id=%d from %q to %q", team.ID, team.OrgID, team.Name, name)
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
_, err := x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, new(Team))
|
||||||
|
return err
|
||||||
|
}
|
||||||
52
modelmigration/v29/v368_test.go
Normal file
52
modelmigration/v29/v368_test.go
Normal file
@@ -0,0 +1,52 @@
|
|||||||
|
// Copyright 2026 The Gitea Authors. All rights reserved.
|
||||||
|
// SPDX-License-Identifier: MIT
|
||||||
|
|
||||||
|
package v29
|
||||||
|
|
||||||
|
import (
|
||||||
|
"strings"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"gitea.dev/modelmigration/migrationtest"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestAddUniqueTeamNameIndex(t *testing.T) {
|
||||||
|
type Team struct {
|
||||||
|
ID int64 `xorm:"pk autoincr"`
|
||||||
|
OrgID int64 `xorm:"INDEX"`
|
||||||
|
LowerName, Name string
|
||||||
|
}
|
||||||
|
|
||||||
|
x, cleanup := migrationtest.PrepareTestEnv(t, 0, new(Team))
|
||||||
|
defer cleanup()
|
||||||
|
if x == nil || t.Failed() {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
_, err := x.Insert(
|
||||||
|
&Team{OrgID: 1, Name: "Team", LowerName: "team"},
|
||||||
|
&Team{OrgID: 1, Name: "Team", LowerName: "team"},
|
||||||
|
&Team{OrgID: 1, Name: "Team-2", LowerName: "team-2"},
|
||||||
|
&Team{OrgID: 2, Name: "Team", LowerName: "team"},
|
||||||
|
&Team{OrgID: 1, Name: strings.Repeat("A", 255), LowerName: strings.Repeat("a", 255)},
|
||||||
|
&Team{OrgID: 1, Name: strings.Repeat("A", 255), LowerName: strings.Repeat("a", 255)},
|
||||||
|
)
|
||||||
|
require.NoError(t, err)
|
||||||
|
for range 2 {
|
||||||
|
require.NoError(t, AddUniqueTeamNameIndex(t.Context(), x))
|
||||||
|
var teams []Team
|
||||||
|
require.NoError(t, x.Asc("id").Find(&teams))
|
||||||
|
require.Equal(t, []Team{
|
||||||
|
{ID: 1, OrgID: 1, Name: "Team", LowerName: "team"},
|
||||||
|
{ID: 2, OrgID: 1, Name: "Team-3", LowerName: "team-3"},
|
||||||
|
{ID: 3, OrgID: 1, Name: "Team-2", LowerName: "team-2"},
|
||||||
|
{ID: 4, OrgID: 2, Name: "Team", LowerName: "team"},
|
||||||
|
{ID: 5, OrgID: 1, Name: strings.Repeat("A", 255), LowerName: strings.Repeat("a", 255)},
|
||||||
|
{ID: 6, OrgID: 1, Name: strings.Repeat("A", 253) + "-6", LowerName: strings.Repeat("a", 253) + "-6"},
|
||||||
|
}, teams)
|
||||||
|
}
|
||||||
|
_, err = x.Insert(&Team{OrgID: 1, Name: "TEAM", LowerName: "team"})
|
||||||
|
require.Error(t, err)
|
||||||
|
}
|
||||||
@@ -75,8 +75,8 @@ const OwnerTeamName = "Owners"
|
|||||||
// Team represents a organization team.
|
// Team represents a organization team.
|
||||||
type Team struct {
|
type Team struct {
|
||||||
ID int64 `xorm:"pk autoincr"`
|
ID int64 `xorm:"pk autoincr"`
|
||||||
OrgID int64 `xorm:"INDEX"`
|
OrgID int64 `xorm:"INDEX UNIQUE(s)"`
|
||||||
LowerName string
|
LowerName string `xorm:"UNIQUE(s)"`
|
||||||
Name string
|
Name string
|
||||||
Description string
|
Description string
|
||||||
AccessMode perm.AccessMode `xorm:"'authorize'"`
|
AccessMode perm.AccessMode `xorm:"'authorize'"`
|
||||||
|
|||||||
@@ -225,11 +225,7 @@ func CreateTeam(ctx *context.APIContext) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if err := org_service.NewTeam(ctx, team); err != nil {
|
if err := org_service.NewTeam(ctx, team); err != nil {
|
||||||
if organization.IsErrTeamAlreadyExist(err) {
|
ctx.APIErrorAuto(err)
|
||||||
ctx.APIError(http.StatusUnprocessableEntity, err.Error())
|
|
||||||
} else {
|
|
||||||
ctx.APIErrorInternal(err)
|
|
||||||
}
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -307,7 +303,7 @@ func EditTeam(ctx *context.APIContext) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if err := org_service.UpdateTeam(ctx, team, isAuthChanged, isIncludeAllChanged); err != nil {
|
if err := org_service.UpdateTeam(ctx, team, isAuthChanged, isIncludeAllChanged); err != nil {
|
||||||
ctx.APIErrorInternal(err)
|
ctx.APIErrorAuto(err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -38,6 +38,13 @@ func recordTeamMemberAudit(ctx context.Context, action audit_model.Action, team
|
|||||||
recordTeamAudit(ctx, action, team, "team", team.Name, "member", member.Name)
|
recordTeamAudit(ctx, action, team, "team", team.Name, "member", member.Name)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func teamWriteError(ctx context.Context, team *organization.Team, err error) error {
|
||||||
|
if existing, _ := organization.GetTeam(ctx, team.OrgID, team.LowerName); existing != nil && existing.ID != team.ID {
|
||||||
|
return organization.ErrTeamAlreadyExist{OrgID: team.OrgID, Name: team.LowerName}
|
||||||
|
}
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
// NewTeam creates a record of new team.
|
// NewTeam creates a record of new team.
|
||||||
// It's caller's responsibility to assign organization ID.
|
// It's caller's responsibility to assign organization ID.
|
||||||
func NewTeam(ctx context.Context, t *organization.Team) (err error) {
|
func NewTeam(ctx context.Context, t *organization.Team) (err error) {
|
||||||
@@ -58,17 +65,6 @@ func NewTeam(ctx context.Context, t *organization.Team) (err error) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
t.LowerName = strings.ToLower(t.Name)
|
t.LowerName = strings.ToLower(t.Name)
|
||||||
has, err = db.Exist[organization.Team](ctx, builder.Eq{
|
|
||||||
"org_id": t.OrgID,
|
|
||||||
"lower_name": t.LowerName,
|
|
||||||
})
|
|
||||||
if err != nil {
|
|
||||||
return err
|
|
||||||
}
|
|
||||||
if has {
|
|
||||||
return organization.ErrTeamAlreadyExist{OrgID: t.OrgID, Name: t.LowerName}
|
|
||||||
}
|
|
||||||
|
|
||||||
if err = db.WithTx(ctx, func(ctx context.Context) error {
|
if err = db.WithTx(ctx, func(ctx context.Context) error {
|
||||||
if err = db.Insert(ctx, t); err != nil {
|
if err = db.Insert(ctx, t); err != nil {
|
||||||
return err
|
return err
|
||||||
@@ -96,7 +92,7 @@ func NewTeam(ctx context.Context, t *organization.Team) (err error) {
|
|||||||
_, err = db.Exec(ctx, "UPDATE `user` SET num_teams=num_teams+1 WHERE id = ?", t.OrgID)
|
_, err = db.Exec(ctx, "UPDATE `user` SET num_teams=num_teams+1 WHERE id = ?", t.OrgID)
|
||||||
return err
|
return err
|
||||||
}); err != nil {
|
}); err != nil {
|
||||||
return err
|
return teamWriteError(ctx, t, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
recordTeamAudit(ctx, audit_model.OrganizationTeamAdd, t, "team", t.Name)
|
recordTeamAudit(ctx, audit_model.OrganizationTeamAdd, t, "team", t.Name)
|
||||||
@@ -114,17 +110,6 @@ func UpdateTeam(ctx context.Context, t *organization.Team, authChanged, includeA
|
|||||||
|
|
||||||
if err = db.WithTx(ctx, func(ctx context.Context) error {
|
if err = db.WithTx(ctx, func(ctx context.Context) error {
|
||||||
t.LowerName = strings.ToLower(t.Name)
|
t.LowerName = strings.ToLower(t.Name)
|
||||||
has, err := db.Exist[organization.Team](ctx, builder.Eq{
|
|
||||||
"org_id": t.OrgID,
|
|
||||||
"lower_name": t.LowerName,
|
|
||||||
}.And(builder.Neq{"id": t.ID}),
|
|
||||||
)
|
|
||||||
if err != nil {
|
|
||||||
return err
|
|
||||||
} else if has {
|
|
||||||
return organization.ErrTeamAlreadyExist{OrgID: t.OrgID, Name: t.LowerName}
|
|
||||||
}
|
|
||||||
|
|
||||||
sess := db.GetEngine(ctx)
|
sess := db.GetEngine(ctx)
|
||||||
if _, err = sess.ID(t.ID).Cols("name", "lower_name", "description",
|
if _, err = sess.ID(t.ID).Cols("name", "lower_name", "description",
|
||||||
"can_create_org_repo", "authorize", "includes_all_repositories", "visibility").Update(t); err != nil {
|
"can_create_org_repo", "authorize", "includes_all_repositories", "visibility").Update(t); err != nil {
|
||||||
@@ -173,7 +158,7 @@ func UpdateTeam(ctx context.Context, t *organization.Team, authChanged, includeA
|
|||||||
|
|
||||||
return nil
|
return nil
|
||||||
}); err != nil {
|
}); err != nil {
|
||||||
return err
|
return teamWriteError(ctx, t, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
recordTeamAudit(ctx, audit_model.OrganizationTeamUpdate, t, "team", t.Name)
|
recordTeamAudit(ctx, audit_model.OrganizationTeamUpdate, t, "team", t.Name)
|
||||||
|
|||||||
@@ -126,6 +126,9 @@ func TestAPITeam(t *testing.T) {
|
|||||||
checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories, api.AccessLevelNameNone, expectedTeamUnitsMap)
|
checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories, api.AccessLevelNameNone, expectedTeamUnitsMap)
|
||||||
teamID := apiTeam.ID
|
teamID := apiTeam.ID
|
||||||
|
|
||||||
|
MakeRequest(t, NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams", org.Name), teamToCreate).AddTokenAuth(token), http.StatusConflict)
|
||||||
|
MakeRequest(t, NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/teams/%d", teamID), api.EditTeamOption{Name: "team13NotCreators"}).AddTokenAuth(token), http.StatusConflict)
|
||||||
|
|
||||||
// Edit team.
|
// Edit team.
|
||||||
editDescription := "team 1"
|
editDescription := "team 1"
|
||||||
editFalse := false
|
editFalse := false
|
||||||
|
|||||||
Reference in New Issue
Block a user