Skip to content

Commit fd7c364

Browse files
DrMaxNixwxiaoguangTheFox0x7
authored
Check user/org repo limit instead of doer (#34147)
This PR tries to finally fix the bug mentioned in #30011 and #15504, where the user repo limit is checked when creating a repo in an organization. Fix #30011 --------- Co-authored-by: wxiaoguang <[email protected]> Co-authored-by: TheFox0x7 <[email protected]>
1 parent a100ac3 commit fd7c364

File tree

15 files changed

+92
-71
lines changed

15 files changed

+92
-71
lines changed

‎models/organization/org.go

-6
Original file line numberDiff line numberDiff line change
@@ -178,12 +178,6 @@ func (org *Organization) HomeLink() string {
178178
return org.AsUser().HomeLink()
179179
}
180180

181-
// CanCreateRepo returns if user login can create a repository
182-
// NOTE: functions calling this assume a failure due to repository count limit; if new checks are added, those functions should be revised
183-
func (org *Organization) CanCreateRepo() bool {
184-
return org.AsUser().CanCreateRepo()
185-
}
186-
187181
// FindOrgMembersOpts represensts find org members conditions
188182
type FindOrgMembersOpts struct {
189183
db.ListOptions

‎models/repo/update.go

+8-8
Original file line numberDiff line numberDiff line change
@@ -111,31 +111,31 @@ func (err ErrRepoFilesAlreadyExist) Unwrap() error {
111111
return util.ErrAlreadyExist
112112
}
113113

114-
// CheckCreateRepository check if could created a repository
115-
func CheckCreateRepository(ctx context.Context, doer, u *user_model.User, name string, overwriteOrAdopt bool) error {
116-
if !doer.CanCreateRepo() {
117-
return ErrReachLimitOfRepo{u.MaxRepoCreation}
114+
// CheckCreateRepository check if doer could create a repository in new owner
115+
func CheckCreateRepository(ctx context.Context, doer, owner *user_model.User, name string, overwriteOrAdopt bool) error {
116+
if !doer.CanCreateRepoIn(owner) {
117+
return ErrReachLimitOfRepo{owner.MaxRepoCreation}
118118
}
119119

120120
if err := IsUsableRepoName(name); err != nil {
121121
return err
122122
}
123123

124-
has, err := IsRepositoryModelOrDirExist(ctx, u, name)
124+
has, err := IsRepositoryModelOrDirExist(ctx, owner, name)
125125
if err != nil {
126126
return fmt.Errorf("IsRepositoryExist: %w", err)
127127
} else if has {
128-
return ErrRepoAlreadyExist{u.Name, name}
128+
return ErrRepoAlreadyExist{owner.Name, name}
129129
}
130130

131-
repoPath := RepoPath(u.Name, name)
131+
repoPath := RepoPath(owner.Name, name)
132132
isExist, err := util.IsExist(repoPath)
133133
if err != nil {
134134
log.Error("Unable to check if %s exists. Error: %v", repoPath, err)
135135
return err
136136
}
137137
if !overwriteOrAdopt && isExist {
138-
return ErrRepoFilesAlreadyExist{u.Name, name}
138+
return ErrRepoFilesAlreadyExist{owner.Name, name}
139139
}
140140
return nil
141141
}

‎models/user/user.go

+11-11
Original file line numberDiff line numberDiff line change
@@ -247,19 +247,20 @@ func (u *User) MaxCreationLimit() int {
247247
return u.MaxRepoCreation
248248
}
249249

250-
// CanCreateRepo returns if user login can create a repository
251-
// NOTE: functions calling this assume a failure due to repository count limit; if new checks are added, those functions should be revised
252-
func (u *User) CanCreateRepo() bool {
250+
// CanCreateRepoIn checks whether the doer(u) can create a repository in the owner
251+
// NOTE: functions calling this assume a failure due to repository count limit; it ONLY checks the repo number LIMIT, if new checks are added, those functions should be revised
252+
func (u *User) CanCreateRepoIn(owner *User) bool {
253253
if u.IsAdmin {
254254
return true
255255
}
256-
if u.MaxRepoCreation <= -1 {
257-
if setting.Repository.MaxCreationLimit <= -1 {
256+
const noLimit = -1
257+
if owner.MaxRepoCreation == noLimit {
258+
if setting.Repository.MaxCreationLimit == noLimit {
258259
return true
259260
}
260-
return u.NumRepos < setting.Repository.MaxCreationLimit
261+
return owner.NumRepos < setting.Repository.MaxCreationLimit
261262
}
262-
return u.NumRepos < u.MaxRepoCreation
263+
return owner.NumRepos < owner.MaxRepoCreation
263264
}
264265

265266
// CanCreateOrganization returns true if user can create organisation.
@@ -272,13 +273,12 @@ func (u *User) CanEditGitHook() bool {
272273
return !setting.DisableGitHooks && (u.IsAdmin || u.AllowGitHook)
273274
}
274275

275-
// CanForkRepo returns if user login can fork a repository
276-
// It checks especially that the user can create repos, and potentially more
277-
func (u *User) CanForkRepo() bool {
276+
// CanForkRepoIn ONLY checks repository count limit
277+
func (u *User) CanForkRepoIn(owner *User) bool {
278278
if setting.Repository.AllowForkWithoutMaximumLimit {
279279
return true
280280
}
281-
return u.CanCreateRepo()
281+
return u.CanCreateRepoIn(owner)
282282
}
283283

284284
// CanImportLocal returns true if user can migrate repository by local path.

‎models/user/user_test.go

+35
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"code.gitea.io/gitea/modules/optional"
2020
"code.gitea.io/gitea/modules/setting"
2121
"code.gitea.io/gitea/modules/structs"
22+
"code.gitea.io/gitea/modules/test"
2223
"code.gitea.io/gitea/modules/timeutil"
2324

2425
"github.com./stretchr/testify/assert"
@@ -616,3 +617,37 @@ func TestGetInactiveUsers(t *testing.T) {
616617
assert.NoError(t, err)
617618
assert.Empty(t, users)
618619
}
620+
621+
func TestCanCreateRepo(t *testing.T) {
622+
defer test.MockVariableValue(&setting.Repository.MaxCreationLimit)()
623+
const noLimit = -1
624+
doerNormal := &user_model.User{}
625+
doerAdmin := &user_model.User{IsAdmin: true}
626+
t.Run("NoGlobalLimit", func(t *testing.T) {
627+
setting.Repository.MaxCreationLimit = noLimit
628+
629+
assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit}))
630+
assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0}))
631+
assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100}))
632+
633+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit}))
634+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0}))
635+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100}))
636+
})
637+
638+
t.Run("GlobalLimit50", func(t *testing.T) {
639+
setting.Repository.MaxCreationLimit = 50
640+
641+
assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit}))
642+
assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: noLimit})) // limited by global limit
643+
assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0}))
644+
assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100}))
645+
assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: 100}))
646+
647+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit}))
648+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: noLimit}))
649+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0}))
650+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100}))
651+
assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: 100}))
652+
})
653+
}

‎routers/web/repo/fork.go

+6-11
Original file line numberDiff line numberDiff line change
@@ -91,12 +91,17 @@ func getForkRepository(ctx *context.Context) *repo_model.Repository {
9191
ctx.Data["CanForkToUser"] = canForkToUser
9292
ctx.Data["Orgs"] = orgs
9393

94+
// TODO: this message should only be shown for the "current doer" when it is selected, just like the "new repo" page.
95+
// msg := ctx.TrN(maxCreationLimit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", ctx.Doer.MaxCreationLimit())
96+
9497
if canForkToUser {
9598
ctx.Data["ContextUser"] = ctx.Doer
99+
ctx.Data["CanForkRepoInNewOwner"] = true
96100
} else if len(orgs) > 0 {
97101
ctx.Data["ContextUser"] = orgs[0]
102+
ctx.Data["CanForkRepoInNewOwner"] = true
98103
} else {
99-
ctx.Data["CanForkRepo"] = false
104+
ctx.Data["CanForkRepoInNewOwner"] = false
100105
ctx.Flash.Error(ctx.Tr("repo.fork_no_valid_owners"), true)
101106
return nil
102107
}
@@ -120,15 +125,6 @@ func getForkRepository(ctx *context.Context) *repo_model.Repository {
120125
// Fork render repository fork page
121126
func Fork(ctx *context.Context) {
122127
ctx.Data["Title"] = ctx.Tr("new_fork")
123-
124-
if ctx.Doer.CanForkRepo() {
125-
ctx.Data["CanForkRepo"] = true
126-
} else {
127-
maxCreationLimit := ctx.Doer.MaxCreationLimit()
128-
msg := ctx.TrN(maxCreationLimit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", maxCreationLimit)
129-
ctx.Flash.Error(msg, true)
130-
}
131-
132128
getForkRepository(ctx)
133129
if ctx.Written() {
134130
return
@@ -141,7 +137,6 @@ func Fork(ctx *context.Context) {
141137
func ForkPost(ctx *context.Context) {
142138
form := web.GetForm(ctx).(*forms.CreateRepoForm)
143139
ctx.Data["Title"] = ctx.Tr("new_fork")
144-
ctx.Data["CanForkRepo"] = true
145140

146141
ctxUser := checkContextUser(ctx, form.UID)
147142
if ctx.Written() {

‎routers/web/repo/repo.go

+6-10
Original file line numberDiff line numberDiff line change
@@ -87,17 +87,13 @@ func checkContextUser(ctx *context.Context, uid int64) *user_model.User {
8787
return nil
8888
}
8989

90-
if !ctx.Doer.IsAdmin {
91-
orgsAvailable := []*organization.Organization{}
92-
for i := 0; i < len(orgs); i++ {
93-
if orgs[i].CanCreateRepo() {
94-
orgsAvailable = append(orgsAvailable, orgs[i])
95-
}
90+
var orgsAvailable []*organization.Organization
91+
for i := 0; i < len(orgs); i++ {
92+
if ctx.Doer.CanCreateRepoIn(orgs[i].AsUser()) {
93+
orgsAvailable = append(orgsAvailable, orgs[i])
9694
}
97-
ctx.Data["Orgs"] = orgsAvailable
98-
} else {
99-
ctx.Data["Orgs"] = orgs
10095
}
96+
ctx.Data["Orgs"] = orgsAvailable
10197

10298
// Not equal means current user is an organization.
10399
if uid == ctx.Doer.ID || uid == 0 {
@@ -154,7 +150,7 @@ func createCommon(ctx *context.Context) {
154150
ctx.Data["Licenses"] = repo_module.Licenses
155151
ctx.Data["Readmes"] = repo_module.Readmes
156152
ctx.Data["IsForcedPrivate"] = setting.Repository.ForcePrivate
157-
ctx.Data["CanCreateRepoInDoer"] = ctx.Doer.CanCreateRepo()
153+
ctx.Data["CanCreateRepoInDoer"] = ctx.Doer.CanCreateRepoIn(ctx.Doer)
158154
ctx.Data["MaxCreationLimitOfDoer"] = ctx.Doer.MaxCreationLimit()
159155
ctx.Data["SupportedObjectFormats"] = git.DefaultFeatures().SupportedObjectFormats
160156
ctx.Data["DefaultObjectFormat"] = git.Sha1ObjectFormat

‎routers/web/repo/setting/setting.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ func SettingsCtxData(ctx *context.Context) {
5959
ctx.Data["DisableNewPushMirrors"] = setting.Mirror.DisableNewPush
6060
ctx.Data["DefaultMirrorInterval"] = setting.Mirror.DefaultInterval
6161
ctx.Data["MinimumMirrorInterval"] = setting.Mirror.MinInterval
62+
ctx.Data["CanConvertFork"] = ctx.Repo.Repository.IsFork && ctx.Doer.CanCreateRepoIn(ctx.Repo.Repository.Owner)
6263

6364
signing, _ := asymkey_service.SigningKey(ctx, ctx.Repo.Repository.RepoPath())
6465
ctx.Data["SigningKeyAvailable"] = len(signing) > 0
@@ -786,7 +787,7 @@ func handleSettingsPostConvertFork(ctx *context.Context) {
786787
return
787788
}
788789

789-
if !ctx.Repo.Owner.CanCreateRepo() {
790+
if !ctx.Doer.CanForkRepoIn(ctx.Repo.Owner) {
790791
maxCreationLimit := ctx.Repo.Owner.MaxCreationLimit()
791792
msg := ctx.TrN(maxCreationLimit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", maxCreationLimit)
792793
ctx.Flash.Error(msg)

‎services/repository/adopt.go

+8-8
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,17 @@ func deleteFailedAdoptRepository(repoID int64) error {
4040
}
4141

4242
// AdoptRepository adopts pre-existing repository files for the user/organization.
43-
func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateRepoOptions) (*repo_model.Repository, error) {
44-
if !doer.IsAdmin && !u.CanCreateRepo() {
43+
func AdoptRepository(ctx context.Context, doer, owner *user_model.User, opts CreateRepoOptions) (*repo_model.Repository, error) {
44+
if !doer.CanCreateRepoIn(owner) {
4545
return nil, repo_model.ErrReachLimitOfRepo{
46-
Limit: u.MaxRepoCreation,
46+
Limit: owner.MaxRepoCreation,
4747
}
4848
}
4949

5050
repo := &repo_model.Repository{
51-
OwnerID: u.ID,
52-
Owner: u,
53-
OwnerName: u.Name,
51+
OwnerID: owner.ID,
52+
Owner: owner,
53+
OwnerName: owner.Name,
5454
Name: opts.Name,
5555
LowerName: strings.ToLower(opts.Name),
5656
Description: opts.Description,
@@ -65,7 +65,7 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR
6565

6666
// 1 - create the repository database operations first
6767
err := db.WithTx(ctx, func(ctx context.Context) error {
68-
return createRepositoryInDB(ctx, doer, u, repo, false)
68+
return createRepositoryInDB(ctx, doer, owner, repo, false)
6969
})
7070
if err != nil {
7171
return nil, err
@@ -104,7 +104,7 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR
104104
return nil, fmt.Errorf("UpdateRepositoryCols: %w", err)
105105
}
106106

107-
notify_service.AdoptRepository(ctx, doer, u, repo)
107+
notify_service.AdoptRepository(ctx, doer, owner, repo)
108108

109109
return repo, nil
110110
}

‎services/repository/create.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -204,10 +204,10 @@ func initRepository(ctx context.Context, u *user_model.User, repo *repo_model.Re
204204
}
205205

206206
// CreateRepositoryDirectly creates a repository for the user/organization.
207-
func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opts CreateRepoOptions) (*repo_model.Repository, error) {
208-
if !doer.IsAdmin && !u.CanCreateRepo() {
207+
func CreateRepositoryDirectly(ctx context.Context, doer, owner *user_model.User, opts CreateRepoOptions) (*repo_model.Repository, error) {
208+
if !doer.CanCreateRepoIn(owner) {
209209
return nil, repo_model.ErrReachLimitOfRepo{
210-
Limit: u.MaxRepoCreation,
210+
Limit: owner.MaxRepoCreation,
211211
}
212212
}
213213

@@ -227,9 +227,9 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt
227227
}
228228

229229
repo := &repo_model.Repository{
230-
OwnerID: u.ID,
231-
Owner: u,
232-
OwnerName: u.Name,
230+
OwnerID: owner.ID,
231+
Owner: owner,
232+
OwnerName: owner.Name,
233233
Name: opts.Name,
234234
LowerName: strings.ToLower(opts.Name),
235235
Description: opts.Description,
@@ -252,7 +252,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt
252252

253253
// 1 - create the repository database operations first
254254
err := db.WithTx(ctx, func(ctx context.Context) error {
255-
return createRepositoryInDB(ctx, doer, u, repo, false)
255+
return createRepositoryInDB(ctx, doer, owner, repo, false)
256256
})
257257
if err != nil {
258258
return nil, err

‎services/repository/fork.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork
6565
}
6666

6767
// Fork is prohibited, if user has reached maximum limit of repositories
68-
if !owner.CanForkRepo() {
68+
if !doer.CanForkRepoIn(owner) {
6969
return nil, repo_model.ErrReachLimitOfRepo{
7070
Limit: owner.MaxRepoCreation,
7171
}

‎services/repository/template.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func GenerateProtectedBranch(ctx context.Context, templateRepo, generateRepo *re
6868

6969
// GenerateRepository generates a repository from a template
7070
func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templateRepo *repo_model.Repository, opts GenerateRepoOptions) (_ *repo_model.Repository, err error) {
71-
if !doer.IsAdmin && !owner.CanCreateRepo() {
71+
if !doer.CanCreateRepoIn(owner) {
7272
return nil, repo_model.ErrReachLimitOfRepo{
7373
Limit: owner.MaxRepoCreation,
7474
}

‎services/repository/transfer.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, d
6161
return err
6262
}
6363

64-
if !doer.IsAdmin && !repoTransfer.Recipient.CanCreateRepo() {
64+
if !doer.CanCreateRepoIn(repoTransfer.Recipient) {
6565
limit := util.Iif(repoTransfer.Recipient.MaxRepoCreation >= 0, repoTransfer.Recipient.MaxRepoCreation, setting.Repository.MaxCreationLimit)
6666
return LimitReachedError{Limit: limit}
6767
}
@@ -416,7 +416,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use
416416
return err
417417
}
418418

419-
if !doer.IsAdmin && !newOwner.CanCreateRepo() {
419+
if !doer.CanForkRepoIn(newOwner) {
420420
limit := util.Iif(newOwner.MaxRepoCreation >= 0, newOwner.MaxRepoCreation, setting.Repository.MaxCreationLimit)
421421
return LimitReachedError{Limit: limit}
422422
}

‎services/repository/transfer_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func TestRepositoryTransferRejection(t *testing.T) {
144144
require.NotNil(t, transfer)
145145
require.NoError(t, transfer.LoadRecipient(db.DefaultContext))
146146

147-
require.True(t, transfer.Recipient.CanCreateRepo()) // admin is not subject to limits
147+
require.True(t, doerAdmin.CanCreateRepoIn(transfer.Recipient)) // admin is not subject to limits
148148

149149
// Administrator should not be affected by the limits so transfer should be successful
150150
assert.NoError(t, AcceptTransferOwnership(db.DefaultContext, repo, doerAdmin))
@@ -158,7 +158,7 @@ func TestRepositoryTransferRejection(t *testing.T) {
158158
require.NotNil(t, transfer)
159159
require.NoError(t, transfer.LoadRecipient(db.DefaultContext))
160160

161-
require.False(t, transfer.Recipient.CanCreateRepo()) // regular user is subject to limits
161+
require.False(t, doer.CanCreateRepoIn(transfer.Recipient)) // regular user is subject to limits
162162

163163
// Cannot accept because of the limit
164164
err = AcceptTransferOwnership(db.DefaultContext, repo, doer)

‎templates/repo/pulls/fork.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@
7575

7676
<div class="inline field">
7777
<label></label>
78-
<button class="ui primary button{{if not .CanForkRepo}} disabled{{end}}">
78+
<button class="ui primary button{{if not .CanForkRepoInNewOwner}} disabled{{end}}">
7979
{{ctx.Locale.Tr "repo.fork_repo"}}
8080
</button>
8181
</div>

‎templates/repo/settings/options.tmpl

+2-2
Original file line numberDiff line numberDiff line change
@@ -802,7 +802,7 @@
802802
</div>
803803
</div>
804804
{{end}}
805-
{{if and .Repository.IsFork .Repository.Owner.CanCreateRepo}}
805+
{{if .CanConvertFork}}
806806
<div class="flex-item">
807807
<div class="flex-item-main">
808808
<div class="flex-item-title">{{ctx.Locale.Tr "repo.settings.convert_fork"}}</div>
@@ -916,7 +916,7 @@
916916
</div>
917917
</div>
918918
{{end}}
919-
{{if and .Repository.IsFork .Repository.Owner.CanCreateRepo}}
919+
{{if .CanConvertFork}}
920920
<div class="ui small modal" id="convert-fork-repo-modal">
921921
<div class="header">
922922
{{ctx.Locale.Tr "repo.settings.convert_fork"}}

0 commit comments

Comments
 (0)