From 9354efceb1f90eeb7c256ee62ea79c0d0054c9a2 Mon Sep 17 00:00:00 2001 From: Brook Miles Date: Mon, 8 Sep 2025 01:05:55 +0200 Subject: [PATCH] fix: quota evaluation rules not working properly (#9033) This patch is mainly intended to fix forgejo/forgejo#7721, and to fix forgejo/forgejo#9019. It also changes the evaluation of 0 limits to prevent all writes, instead of allowing one write and then failing on subsequent writes after the limit has been exceeded. This matches the expectation of the existing tests, and I believe it will better match the expectations of users. Tests have been updated accordingly where necessary, and some additional test coverage added. The fixes in this PR depend on each other in order for the quota system to function correctly, so I'm submitting them as a single PR instead of individually. ## Test Cases ### Quota subjects not covered by their parent subjects Before enabling quotas, create a test user and test repository for that user. Enable quotas, and set a default total to some large value. (Do not use unit suffixes forgejo/forgejo#8996) ```ini [quota] ENABLED = true [quota.default] TOTAL = 1073741824 ``` With the test user, navigate to "Storage overview" and verify that the quota group "Global quota" is the only group listed, containing the rule "Default", and displays the configured limit, and that the limit has not been exceeded (eg. `42 MiB / 1 GiB`). The default quota rule has the subject `size:all`, so any write action should be allowed. #### Attempt to create a new repository. Expected result: Repository is created. Actual result: Error 413, You have exhausted your quota. #### Attempt to create a new file in the existing repository. Expected result: File is created. Actual result: Error 413, You have exhausted your quota. #### Create an issue on the test repository, and attempt to upload an image to the issue. Expected result: Image is uploaded. Actual Result: Quota exceeded. Displays error message: `JavaScript promise rejection: can't access property "submitted", oi[ji.uuid] is undefined. Open browser console to see more details.` ### Unlimited quota rules incorrectly allow all writes With quotas enabled, [Use the API](https://forgejo.org/docs/latest/admin/advanced/quota/#advanced-usage-via-api) to create a quota group containing a single rule with a subject of `size:git:lfs`, and a limit of `-1` (Unlimited). Add the test user to this group. ```json { "name": "git-lfs-unlimited", "rules": [ { "name": "git-lfs-unlimited", "limit": -1, "subjects": ["size:git:lfs"] } ] } ``` With the test user, navigate to "Storage overview" and verify that the user has been added to this group, that it is the only group the user is assigned to, and that the rule limit displays as "Unlimited". The user should only have the ability to write to Git LFS storage, all other writes should be denied. #### Attempt to create a new repository. Expected result: Error 413, You have exhausted your quota. Actual result: Repository is created. #### Attempt to create a new file in the test repository. Expected result: Error 413, You have exhausted your quota. Actual result: File is created. #### Create an issue on the test repository, and attempt to upload an image to the issue. Expected Result: Quota exceeded. Actual result: Image is uploaded. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9033 Reviewed-by: Gusted Co-authored-by: Brook Miles Co-committed-by: Brook Miles --- models/quota/default.go | 2 +- models/quota/group.go | 77 ++---- models/quota/quota.go | 4 +- models/quota/quota_group_test.go | 121 ++++----- models/quota/quota_rule_test.go | 235 +++++++++--------- models/quota/rule.go | 38 ++- routers/api/v1/api.go | 2 +- tests/e2e/e2e_test.go | 7 +- .../e2e/fixtures/quota_group_rule_mapping.yml | 2 +- tests/e2e/fixtures/quota_rule.yml | 4 +- tests/integration/api_quota_use_test.go | 14 -- 11 files changed, 249 insertions(+), 257 deletions(-) diff --git a/models/quota/default.go b/models/quota/default.go index 9f655d7847..37b23739ad 100644 --- a/models/quota/default.go +++ b/models/quota/default.go @@ -7,7 +7,7 @@ import ( "forgejo.org/modules/setting" ) -func EvaluateDefault(used Used, forSubject LimitSubject) (bool, int64) { +func EvaluateDefault(used Used, forSubject LimitSubject) bool { groups := GroupList{ &Group{ Name: "builtin-default-group", diff --git a/models/quota/group.go b/models/quota/group.go index 7ddc20b2d6..a4ec8d0e14 100644 --- a/models/quota/group.go +++ b/models/quota/group.go @@ -5,7 +5,6 @@ package quota import ( "context" - "math" "forgejo.org/models/db" user_model "forgejo.org/models/user" @@ -179,78 +178,40 @@ func (g *Group) RemoveRuleByName(ctx context.Context, ruleName string) error { return committer.Commit() } -var affectsMap = map[LimitSubject]LimitSubjects{ - LimitSubjectSizeAll: { - LimitSubjectSizeReposAll, - LimitSubjectSizeGitLFS, - LimitSubjectSizeAssetsAll, - }, - LimitSubjectSizeReposAll: { - LimitSubjectSizeReposPublic, - LimitSubjectSizeReposPrivate, - }, - LimitSubjectSizeAssetsAll: { - LimitSubjectSizeAssetsAttachmentsAll, - LimitSubjectSizeAssetsArtifacts, - LimitSubjectSizeAssetsPackagesAll, - }, - LimitSubjectSizeAssetsAttachmentsAll: { - LimitSubjectSizeAssetsAttachmentsIssues, - LimitSubjectSizeAssetsAttachmentsReleases, - }, -} - -// Evaluate returns whether the size used is acceptable for the topic if a rule -// was found, and returns the smallest limit of all applicable rules or the -// first limit found to be unacceptable for the size used. -func (g *Group) Evaluate(used Used, forSubject LimitSubject) (bool, bool, int64) { - var found bool - foundLimit := int64(math.MaxInt64) +// Group.Evaluate returns whether the group contains a matching rule for the subject +// and if so, whether the group allows the action given the size used +func (g *Group) Evaluate(used Used, forSubject LimitSubject) (match, allow bool) { for _, rule := range g.Rules { - ok, has := rule.Evaluate(used, forSubject) - if has { - if !ok { - return false, true, rule.Limit + ruleMatch, ruleAllow := rule.Evaluate(used, forSubject) + if ruleMatch { + // evaluation stops as soon as we find a matching rule that denies the action + if !ruleAllow { + return true, false } - found = true - foundLimit = min(foundLimit, rule.Limit) + + match = true + allow = true } } - if !found { - // If Evaluation for forSubject did not succeed, try evaluating against - // subjects below - - for _, subject := range affectsMap[forSubject] { - ok, has, limit := g.Evaluate(used, subject) - if has { - if !ok { - return false, true, limit - } - found = true - foundLimit = min(foundLimit, limit) - } - } - } - - return true, found, foundLimit + return match, allow } -// Evaluate returns if the used size is acceptable for the subject and the -// lowest limit that is acceptable for the subject. -func (gl *GroupList) Evaluate(used Used, forSubject LimitSubject) (bool, int64) { +// GroupList.Evaluate returns whether the grouplist allows the action given the size used +func (gl *GroupList) Evaluate(used Used, forSubject LimitSubject) (pass bool) { // If there are no groups, use the configured defaults: if gl == nil || len(*gl) == 0 { return EvaluateDefault(used, forSubject) } for _, group := range *gl { - ok, has, limit := group.Evaluate(used, forSubject) - if has && ok { - return true, limit + groupMatch, groupAllow := group.Evaluate(used, forSubject) + if groupMatch && groupAllow { + // evaluation stops as soon as we find a matching group that allows the action + return true } } - return false, 0 + return false } func GetGroupByName(ctx context.Context, name string) (*Group, error) { diff --git a/models/quota/quota.go b/models/quota/quota.go index 9f1c3ca949..9869e9acab 100644 --- a/models/quota/quota.go +++ b/models/quota/quota.go @@ -32,6 +32,6 @@ func EvaluateForUser(ctx context.Context, userID int64, subject LimitSubject) (b return false, err } - acceptable, _ := groups.Evaluate(*used, subject) - return acceptable, nil + allow := groups.Evaluate(*used, subject) + return allow, nil } diff --git a/models/quota/quota_group_test.go b/models/quota/quota_group_test.go index 7f693b391b..7085682bfe 100644 --- a/models/quota/quota_group_test.go +++ b/models/quota/quota_group_test.go @@ -4,15 +4,16 @@ package quota_test import ( - "math" "testing" quota_model "forgejo.org/models/quota" + "forgejo.org/modules/setting" + "forgejo.org/modules/test" "github.com/stretchr/testify/assert" ) -func TestQuotaGroupAllRulesMustPass(t *testing.T) { +func TestQuotaGroupAllRulesMustAllow(t *testing.T) { unlimitedRule := quota_model.Rule{ Limit: -1, Subjects: quota_model.LimitSubjects{ @@ -35,12 +36,11 @@ func TestQuotaGroupAllRulesMustPass(t *testing.T) { used := quota_model.Used{} used.Size.Repos.Public = 1024 - // Within a group, *all* rules must pass. Thus, if we have a deny-all rule, - // and an unlimited rule, that will always fail. - ok, has, limit := group.Evaluate(used, quota_model.LimitSubjectSizeAll) - assert.True(t, has) - assert.False(t, ok) - assert.EqualValues(t, 0, limit) + // Within a group, *all* matching rules must allow. Thus, if we have a deny-all rule, + // and an unlimited rule, the deny rule wins. + match, allow := group.Evaluate(used, quota_model.LimitSubjectSizeAll) + assert.True(t, match) + assert.False(t, allow) } func TestQuotaGroupRuleScenario1(t *testing.T) { @@ -68,25 +68,21 @@ func TestQuotaGroupRuleScenario1(t *testing.T) { used.Size.Assets.Packages.All = 256 used.Size.Git.LFS = 16 - ok, has, limit := group.Evaluate(used, quota_model.LimitSubjectSizeAssetsAttachmentsReleases) - assert.True(t, has, "size:assets:attachments:releases is covered") - assert.True(t, ok, "size:assets:attachments:releases passes") - assert.EqualValues(t, 1024, limit) + match, allow := group.Evaluate(used, quota_model.LimitSubjectSizeAssetsAttachmentsReleases) + assert.True(t, match, "size:assets:attachments:releases is covered") + assert.True(t, allow, "size:assets:attachments:releases is allowed") - ok, has, limit = group.Evaluate(used, quota_model.LimitSubjectSizeAssetsPackagesAll) - assert.True(t, has, "size:assets:packages:all is covered") - assert.True(t, ok, "size:assets:packages:all passes") - assert.EqualValues(t, 1024, limit) + match, allow = group.Evaluate(used, quota_model.LimitSubjectSizeAssetsPackagesAll) + assert.True(t, match, "size:assets:packages:all is covered") + assert.True(t, allow, "size:assets:packages:all is allowed") - ok, has, limit = group.Evaluate(used, quota_model.LimitSubjectSizeGitLFS) - assert.True(t, has, "size:git:lfs is covered") - assert.False(t, ok, "size:git:lfs fails") - assert.EqualValues(t, 0, limit) + match, allow = group.Evaluate(used, quota_model.LimitSubjectSizeGitLFS) + assert.True(t, match, "size:git:lfs is covered") + assert.False(t, allow, "size:git:lfs is denied") - ok, has, limit = group.Evaluate(used, quota_model.LimitSubjectSizeAll) - assert.True(t, has, "size:all is covered") - assert.False(t, ok, "size:all fails") - assert.EqualValues(t, 0, limit) + match, allow = group.Evaluate(used, quota_model.LimitSubjectSizeAll) + assert.False(t, match, "size:all is not covered") + assert.False(t, allow, "size:all is not allowed (not covered)") } func TestQuotaGroupRuleCombination(t *testing.T) { @@ -114,31 +110,23 @@ func TestQuotaGroupRuleCombination(t *testing.T) { }, } - // Git LFS isn't covered by any rule - _, has, limit := group.Evaluate(used, quota_model.LimitSubjectSizeGitLFS) - assert.False(t, has) - assert.EqualValues(t, math.MaxInt, limit) + // Git LFS does not match any rule + match, allow := group.Evaluate(used, quota_model.LimitSubjectSizeGitLFS) + assert.False(t, match) + assert.False(t, allow) - // repos:all is covered, and is passing - ok, has, limit := group.Evaluate(used, quota_model.LimitSubjectSizeReposAll) - assert.True(t, has) - assert.True(t, ok) - assert.EqualValues(t, 4096, limit) + // repos:all has a matching rule and is allowed + match, allow = group.Evaluate(used, quota_model.LimitSubjectSizeReposAll) + assert.True(t, match) + assert.True(t, allow) - // packages:all is covered, and is failing - ok, has, limit = group.Evaluate(used, quota_model.LimitSubjectSizeAssetsPackagesAll) - assert.True(t, has) - assert.False(t, ok) - assert.EqualValues(t, 0, limit) - - // size:all is covered, and is failing (due to packages:all being over quota) - ok, has, limit = group.Evaluate(used, quota_model.LimitSubjectSizeAll) - assert.True(t, has, "size:all should be covered") - assert.False(t, ok, "size:all should fail") - assert.EqualValues(t, 0, limit) + // packages:all has a matching rule and is denied + match, allow = group.Evaluate(used, quota_model.LimitSubjectSizeAssetsPackagesAll) + assert.True(t, match) + assert.False(t, allow) } -func TestQuotaGroupListsRequireOnlyOnePassing(t *testing.T) { +func TestQuotaGroupListsRequireOnlyOneAllow(t *testing.T) { unlimitedRule := quota_model.Rule{ Limit: -1, Subjects: quota_model.LimitSubjects{ @@ -168,13 +156,12 @@ func TestQuotaGroupListsRequireOnlyOnePassing(t *testing.T) { used := quota_model.Used{} used.Size.Repos.Public = 1024 - // In a group list, if any group passes, the entire evaluation passes. - ok, limit := groups.Evaluate(used, quota_model.LimitSubjectSizeAll) - assert.True(t, ok) - assert.EqualValues(t, -1, limit) + // In a group list, an action is allowed if any group matches and allows it. + allow := groups.Evaluate(used, quota_model.LimitSubjectSizeAll) + assert.True(t, allow) } -func TestQuotaGroupListAllFailing(t *testing.T) { +func TestQuotaGroupListAllDeny(t *testing.T) { denyRule := quota_model.Rule{ Limit: 0, Subjects: quota_model.LimitSubjects{ @@ -204,18 +191,38 @@ func TestQuotaGroupListAllFailing(t *testing.T) { used := quota_model.Used{} used.Size.Repos.Public = 2048 - ok, limit := groups.Evaluate(used, quota_model.LimitSubjectSizeAll) - assert.False(t, ok) - assert.EqualValues(t, 0, limit) + allow := groups.Evaluate(used, quota_model.LimitSubjectSizeAll) + assert.False(t, allow) } -func TestQuotaGroupListEmpty(t *testing.T) { +// An empty group list should result in the use of the built in Default +// group: size:all defaulting to unlimited +func TestQuotaDefaultGroup(t *testing.T) { groups := quota_model.GroupList{} used := quota_model.Used{} used.Size.Repos.Public = 2048 - ok, limit := groups.Evaluate(used, quota_model.LimitSubjectSizeAll) - assert.True(t, ok) - assert.EqualValues(t, -1, limit) + testSets := []struct { + name string + limit int64 + expectAllow bool + }{ + {"unlimited", -1, true}, + {"limit-allow", 1024 * 1024, true}, + {"limit-deny", 1024, false}, + } + + for _, testSet := range testSets { + t.Run(testSet.name, func(t *testing.T) { + defer test.MockVariableValue(&setting.Quota.Default.Total, testSet.limit)() + + for subject := quota_model.LimitSubjectFirst; subject <= quota_model.LimitSubjectLast; subject++ { + t.Run(subject.String(), func(t *testing.T) { + allow := groups.Evaluate(used, subject) + assert.Equal(t, testSet.expectAllow, allow) + }) + } + }) + } } diff --git a/models/quota/quota_rule_test.go b/models/quota/quota_rule_test.go index 59c05563f0..c4605fd58e 100644 --- a/models/quota/quota_rule_test.go +++ b/models/quota/quota_rule_test.go @@ -83,28 +83,43 @@ func assertEvaluation(t *testing.T, rule quota_model.Rule, used quota_model.Used t.Helper() t.Run(subject.String(), func(t *testing.T) { - ok, has := rule.Evaluate(used, subject) - assert.True(t, has) - assert.Equal(t, expected, ok) + match, allow := rule.Evaluate(used, subject) + assert.True(t, match) + assert.Equal(t, expected, allow) }) } -func TestQuotaRuleNoEvaluation(t *testing.T) { - rule := quota_model.Rule{ - Limit: 1024, - Subjects: quota_model.LimitSubjects{ - quota_model.LimitSubjectSizeAssetsAttachmentsAll, - }, +func TestQuotaRuleNoMatch(t *testing.T) { + testSets := []struct { + name string + limit int64 + }{ + {"unlimited", -1}, + {"limit-0", 0}, + {"limit-1k", 1024}, + {"limit-1M", 1024 * 1024}, } - used := quota_model.Used{} - used.Size.Repos.Public = 4096 - _, has := rule.Evaluate(used, quota_model.LimitSubjectSizeReposAll) + for _, testSet := range testSets { + t.Run(testSet.name, func(t *testing.T) { + rule := quota_model.Rule{ + Limit: testSet.limit, + Subjects: quota_model.LimitSubjects{ + quota_model.LimitSubjectSizeAssetsAttachmentsAll, + }, + } + used := quota_model.Used{} + used.Size.Repos.Public = 4096 - // We have a rule for "size:assets:attachments:all", and query for - // "size:repos:all". We don't cover that subject, so the evaluation returns - // with no rules found. - assert.False(t, has) + match, allow := rule.Evaluate(used, quota_model.LimitSubjectSizeReposAll) + + // We have a rule for "size:assets:attachments:all", and query for + // "size:repos:all". We don't cover that subject, so the rule does not match + // regardless of the limit. + assert.False(t, match) + assert.False(t, allow) + }) + } } func TestQuotaRuleDirectEvaluation(t *testing.T) { @@ -129,13 +144,12 @@ func TestQuotaRuleDirectEvaluation(t *testing.T) { } t.Run("limit:0", func(t *testing.T) { - // With limit:0, nothing used is fine. + // With limit:0, any usage will fail evaluation, including 0 t.Run("used:0", func(t *testing.T) { for subject := quota_model.LimitSubjectFirst; subject <= quota_model.LimitSubjectLast; subject++ { - runTest(t, subject, 0, 0, true) + runTest(t, subject, 0, 0, false) } }) - // With limit:0, any usage will fail evaluation t.Run("used:512", func(t *testing.T) { for subject := quota_model.LimitSubjectFirst; subject <= quota_model.LimitSubjectLast; subject++ { runTest(t, subject, 0, 512, false) @@ -170,14 +184,6 @@ func TestQuotaRuleDirectEvaluation(t *testing.T) { } func TestQuotaRuleCombined(t *testing.T) { - rule := quota_model.Rule{ - Limit: 1024, - Subjects: quota_model.LimitSubjects{ - quota_model.LimitSubjectSizeGitLFS, - quota_model.LimitSubjectSizeAssetsAttachmentsReleases, - quota_model.LimitSubjectSizeAssetsPackagesAll, - }, - } used := quota_model.Used{ Size: quota_model.UsedSize{ Repos: quota_model.UsedSizeRepos{ @@ -198,107 +204,112 @@ func TestQuotaRuleCombined(t *testing.T) { }, } - expectationMap := map[quota_model.LimitSubject]bool{ - quota_model.LimitSubjectSizeGitLFS: false, - quota_model.LimitSubjectSizeAssetsAttachmentsReleases: false, - quota_model.LimitSubjectSizeAssetsPackagesAll: false, + expectMatch := map[quota_model.LimitSubject]bool{ + quota_model.LimitSubjectSizeGitLFS: true, + quota_model.LimitSubjectSizeAssetsAttachmentsReleases: true, + quota_model.LimitSubjectSizeAssetsPackagesAll: true, } - for subject := quota_model.LimitSubjectFirst; subject <= quota_model.LimitSubjectLast; subject++ { - t.Run(subject.String(), func(t *testing.T) { - evalOk, evalHas := rule.Evaluate(used, subject) - expected, expectedHas := expectationMap[subject] + testSets := []struct { + name string + limit int64 + expectAllow bool + }{ + {"unlimited", -1, true}, + {"limit-allow", 1024 * 1024, true}, + {"limit-deny", 1024, false}, + } - assert.Equal(t, expectedHas, evalHas) - if expectedHas { - assert.Equal(t, expected, evalOk) + for _, testSet := range testSets { + t.Run(testSet.name, func(t *testing.T) { + rule := quota_model.Rule{ + Limit: testSet.limit, + Subjects: quota_model.LimitSubjects{ + quota_model.LimitSubjectSizeGitLFS, + quota_model.LimitSubjectSizeAssetsAttachmentsReleases, + quota_model.LimitSubjectSizeAssetsPackagesAll, + }, + } + + for subject := quota_model.LimitSubjectFirst; subject <= quota_model.LimitSubjectLast; subject++ { + t.Run(subject.String(), func(t *testing.T) { + match, allow := rule.Evaluate(used, subject) + + assert.Equal(t, expectMatch[subject], match) + if expectMatch[subject] { + assert.Equal(t, testSet.expectAllow, allow) + } else { + assert.False(t, allow) + } + }) } }) } } func TestQuotaRuleSizeAll(t *testing.T) { - runTests := func(t *testing.T, rule quota_model.Rule, expected bool) { - t.Helper() - - subject := quota_model.LimitSubjectSizeAll - - t.Run("used:0", func(t *testing.T) { - used := quota_model.Used{} - - assertEvaluation(t, rule, used, subject, true) - }) - - t.Run("used:some-each", func(t *testing.T) { - used := makeFullyUsed() - - assertEvaluation(t, rule, used, subject, expected) - }) - - t.Run("used:some", func(t *testing.T) { - used := makePartiallyUsed() - - assertEvaluation(t, rule, used, subject, expected) - }) + type Test struct { + name string + limit int64 + expectAllow bool } - // With all limits set to 0, evaluation always fails if usage > 0 - t.Run("rule:0", func(t *testing.T) { - rule := quota_model.Rule{ - Limit: 0, - Subjects: quota_model.LimitSubjects{ - quota_model.LimitSubjectSizeAll, + usedSets := []struct { + name string + used quota_model.Used + testSets []Test + }{ + { + "empty", + quota_model.Used{}, + []Test{ + {"unlimited", -1, true}, + {"limit-1M", 1024 * 1024, true}, + {"limit-5k", 5 * 1024, true}, + {"limit-0", 0, false}, }, - } - - runTests(t, rule, false) - }) - - // With no limits, evaluation always succeeds - t.Run("rule:unlimited", func(t *testing.T) { - rule := quota_model.Rule{ - Limit: -1, - Subjects: quota_model.LimitSubjects{ - quota_model.LimitSubjectSizeAll, + }, + { + "partial", + makePartiallyUsed(), + []Test{ + {"unlimited", -1, true}, + {"limit-1M", 1024 * 1024, true}, + {"limit-5k", 5 * 1024, true}, + {"limit-0", 0, false}, }, - } - - runTests(t, rule, true) - }) - - // With a specific, very generous limit, evaluation succeeds if the limit isn't exhausted - t.Run("rule:generous", func(t *testing.T) { - rule := quota_model.Rule{ - Limit: 102400, - Subjects: quota_model.LimitSubjects{ - quota_model.LimitSubjectSizeAll, + }, + { + "full", + makeFullyUsed(), + []Test{ + {"unlimited", -1, true}, + {"limit-1M", 1024 * 1024, true}, + {"limit-5k", 5 * 1024, false}, + {"limit-0", 0, false}, }, - } + }, + } - runTests(t, rule, true) + for _, usedSet := range usedSets { + t.Run(usedSet.name, func(t *testing.T) { + testSets := usedSet.testSets + used := usedSet.used - t.Run("limit exhaustion", func(t *testing.T) { - used := quota_model.Used{ - Size: quota_model.UsedSize{ - Repos: quota_model.UsedSizeRepos{ - Public: 204800, - }, - }, + for _, testSet := range testSets { + t.Run(testSet.name, func(t *testing.T) { + rule := quota_model.Rule{ + Limit: testSet.limit, + Subjects: quota_model.LimitSubjects{ + quota_model.LimitSubjectSizeAll, + }, + } + + match, allow := rule.Evaluate(used, quota_model.LimitSubjectSizeAll) + assert.True(t, match) + assert.Equal(t, testSet.expectAllow, allow) + }) } - - assertEvaluation(t, rule, used, quota_model.LimitSubjectSizeAll, false) }) - }) - - // With a specific, small limit, evaluation fails - t.Run("rule:limited", func(t *testing.T) { - rule := quota_model.Rule{ - Limit: 512, - Subjects: quota_model.LimitSubjects{ - quota_model.LimitSubjectSizeAll, - }, - } - - runTests(t, rule, false) - }) + } } diff --git a/models/quota/rule.go b/models/quota/rule.go index 89cb57cace..98959e0a91 100644 --- a/models/quota/rule.go +++ b/models/quota/rule.go @@ -16,6 +16,21 @@ type Rule struct { Subjects LimitSubjects `json:"subjects,omitempty"` } +var subjectToParent = map[LimitSubject]LimitSubject{ + LimitSubjectSizeGitAll: LimitSubjectSizeAll, + LimitSubjectSizeGitLFS: LimitSubjectSizeGitAll, + LimitSubjectSizeReposAll: LimitSubjectSizeGitAll, + LimitSubjectSizeReposPublic: LimitSubjectSizeReposAll, + LimitSubjectSizeReposPrivate: LimitSubjectSizeReposAll, + LimitSubjectSizeAssetsAll: LimitSubjectSizeAll, + LimitSubjectSizeAssetsAttachmentsAll: LimitSubjectSizeAssetsAll, + LimitSubjectSizeAssetsAttachmentsIssues: LimitSubjectSizeAssetsAttachmentsAll, + LimitSubjectSizeAssetsAttachmentsReleases: LimitSubjectSizeAssetsAttachmentsAll, + LimitSubjectSizeAssetsArtifacts: LimitSubjectSizeAssetsAll, + LimitSubjectSizeAssetsPackagesAll: LimitSubjectSizeAssetsAll, + LimitSubjectSizeWiki: LimitSubjectSizeAssetsAll, +} + func (r *Rule) TableName() string { return "quota_rule" } @@ -36,18 +51,25 @@ func (r Rule) Sum(used Used) int64 { return sum } -func (r Rule) Evaluate(used Used, forSubject LimitSubject) (bool, bool) { - // If there's no limit, short circuit out - if r.Limit == -1 { - return true, true - } - - // If the rule does not cover forSubject, bail out early +func (r Rule) Evaluate(used Used, forSubject LimitSubject) (match, allow bool) { if !slices.Contains(r.Subjects, forSubject) { + // this rule does not match the subject being tested + parent := subjectToParent[forSubject] + if parent != LimitSubjectNone { + return r.Evaluate(used, parent) + } return false, false } - return r.Sum(used) <= r.Limit, true + match = true + + if r.Limit == -1 { + // Unlimited, any value is allowed + allow = true + } else { + allow = r.Sum(used) < r.Limit + } + return match, allow } func (r *Rule) Edit(ctx context.Context, limit *int64, subjects *LimitSubjects) (*Rule, error) { diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 17bd0a246e..8c9eabc613 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1193,7 +1193,7 @@ func Routes() *web.Route { m.Get("", repo.ListBranches) m.Get("/*", repo.GetBranch) m.Delete("/*", reqToken(), reqRepoWriter(unit.TypeCode), mustNotBeArchived, repo.DeleteBranch) - m.Post("", reqToken(), reqRepoWriter(unit.TypeCode), mustNotBeArchived, bind(api.CreateBranchRepoOption{}), context.EnforceQuotaAPI(quota_model.LimitSubjectSizeGitAll, context.QuotaTargetRepo), repo.CreateBranch) + m.Post("", reqToken(), reqRepoWriter(unit.TypeCode), mustNotBeArchived, bind(api.CreateBranchRepoOption{}), context.EnforceQuotaAPI(quota_model.LimitSubjectSizeReposAll, context.QuotaTargetRepo), repo.CreateBranch) m.Patch("/*", reqToken(), reqRepoWriter(unit.TypeCode), mustNotBeArchived, bind(api.UpdateBranchRepoOption{}), repo.UpdateBranch) }, context.ReferencesGitRepo(), reqRepoReader(unit.TypeCode)) m.Group("/branch_protections", func() { diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 1961a4b68a..7fad9d1834 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -20,6 +20,7 @@ import ( "forgejo.org/modules/graceful" "forgejo.org/modules/log" "forgejo.org/modules/setting" + "forgejo.org/modules/test" "forgejo.org/modules/testlogger" "forgejo.org/modules/util" "forgejo.org/modules/web" @@ -37,7 +38,6 @@ func TestMain(m *testing.M) { defer cancel() tests.InitTest(true) - setting.Quota.Enabled = true initChangedFiles() testE2eWebRoutes = routers.NormalRoutes() @@ -103,6 +103,11 @@ func TestE2e(t *testing.T) { } t.Run(testname, func(t *testing.T) { + if testname == "user-settings.test.e2e" { + defer test.MockVariableValue(&setting.Quota.Enabled, true)() + defer test.MockVariableValue(&testE2eWebRoutes, routers.NormalRoutes())() + } + // Default 2 minute timeout onForgejoRun(t, func(*testing.T, *url.URL) { defer DeclareGitRepos(t)() diff --git a/tests/e2e/fixtures/quota_group_rule_mapping.yml b/tests/e2e/fixtures/quota_group_rule_mapping.yml index 1144888f21..4dfb9d4ba5 100644 --- a/tests/e2e/fixtures/quota_group_rule_mapping.yml +++ b/tests/e2e/fixtures/quota_group_rule_mapping.yml @@ -6,7 +6,7 @@ - id: 1002 group_name: trusted-user - rule_name: "all:assets" + rule_name: "repos:all" - id: 1003 diff --git a/tests/e2e/fixtures/quota_rule.yml b/tests/e2e/fixtures/quota_rule.yml index f6170cbdaa..309c6f0b70 100644 --- a/tests/e2e/fixtures/quota_rule.yml +++ b/tests/e2e/fixtures/quota_rule.yml @@ -4,9 +4,9 @@ subjects: [6] - - name: "all:assets" + name: "repos:all" limit: -1 - subjects: [7] + subjects: [2] - name: "Multi subjects" limit: 5000000000 diff --git a/tests/integration/api_quota_use_test.go b/tests/integration/api_quota_use_test.go index 1cac2d8d76..1e3d41428a 100644 --- a/tests/integration/api_quota_use_test.go +++ b/tests/integration/api_quota_use_test.go @@ -187,16 +187,6 @@ func (e *quotaEnv) SetupWithMultipleQuotaRules(t *testing.T) { cleaner = createQuotaGroup(t, "default") e.cleanups = append(e.cleanups, cleaner) - // Create three rules: all, repo-size, and asset-size - zero := int64(0) - ruleAll := api.CreateQuotaRuleOptions{ - Name: "all", - Limit: &zero, - Subjects: []string{"size:all"}, - } - cleaner = createQuotaRule(t, ruleAll) - e.cleanups = append(e.cleanups, cleaner) - fifteenMb := int64(1024 * 1024 * 15) ruleRepoSize := api.CreateQuotaRuleOptions{ Name: "repo-size", @@ -215,8 +205,6 @@ func (e *quotaEnv) SetupWithMultipleQuotaRules(t *testing.T) { e.cleanups = append(e.cleanups, cleaner) // Add these rules to the group - cleaner = e.AddRuleToGroup(t, "default", "all") - e.cleanups = append(e.cleanups, cleaner) cleaner = e.AddRuleToGroup(t, "default", "repo-size") e.cleanups = append(e.cleanups, cleaner) cleaner = e.AddRuleToGroup(t, "default", "asset-size") @@ -1414,7 +1402,6 @@ func TestAPIQuotaUserBasics(t *testing.T) { // Temporarily disable quota checking defer env.SetRuleLimit(t, "repo-size", -1)() - defer env.SetRuleLimit(t, "all", -1)() // Create a branch req := NewRequestWithJSON(t, "POST", env.APIPathForRepo("/branches"), api.CreateBranchRepoOption{ @@ -1424,7 +1411,6 @@ func TestAPIQuotaUserBasics(t *testing.T) { // Set the limit back. No need to defer, the first one will set it // back to the correct value. - env.SetRuleLimit(t, "all", 0) env.SetRuleLimit(t, "repo-size", 0) // Deleting a branch does not incur quota enforcement