From fdda4da461fa4de1bc65d99a3e7182c58b134e22 Mon Sep 17 00:00:00 2001 From: maliming Date: Fri, 6 Mar 2026 17:11:42 +0800 Subject: [PATCH] fix: update RetryAfter handling to return null for ban policies and add custom resolver null check --- .../infrastructure/operation-rate-limiting.md | 6 ++++-- .../Rules/FixedWindowOperationRateLimitingRule.cs | 14 +++++++++++++- .../DistributedCacheOperationRateLimitingStore.cs | 4 ++-- .../AbpOperationRateLimitingTestModule.cs | 7 +++++++ ...ibutedCacheOperationRateLimitingStore_Tests.cs | 4 ++-- .../OperationRateLimitingChecker_Tests.cs | 15 +++++++++++++++ 6 files changed, 43 insertions(+), 7 deletions(-) diff --git a/docs/en/framework/infrastructure/operation-rate-limiting.md b/docs/en/framework/infrastructure/operation-rate-limiting.md index 45139b1313..bdc55f1c7e 100644 --- a/docs/en/framework/infrastructure/operation-rate-limiting.md +++ b/docs/en/framework/infrastructure/operation-rate-limiting.md @@ -205,7 +205,9 @@ policy.WithFixedWindow(TimeSpan.FromHours(1), maxCount: 100) ## Multi-Tenancy -By default, rate limit counters are **shared across all tenants**. You can enable tenant isolation for a rule by calling `WithMultiTenancy()`: +By default, partition keys do not include tenant information — for partition types like `PartitionByParameter`, `PartitionByCurrentUser`, `PartitionByClientIp`, etc., counters are shared across tenants unless you call `WithMultiTenancy()`. Note that `PartitionByCurrentTenant()` is inherently per-tenant since the partition key is the tenant ID itself, and `PartitionByClientIp()` is typically kept global since the same IP should share a counter regardless of tenant. + +You can enable tenant isolation for a rule by calling `WithMultiTenancy()`: ````csharp policy.AddRule(rule => rule @@ -326,7 +328,7 @@ public override void ConfigureServices(ServiceConfigurationContext context) ### Ban Policy (maxCount: 0) -Setting `maxCount` to `0` creates a ban policy that blocks all requests for the specified duration: +Setting `maxCount` to `0` creates a ban policy that permanently denies all requests regardless of the window duration. The `RetryAfter` value will be `null` since there is no window to wait for: ````csharp options.AddPolicy("BlockedUser", policy => diff --git a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs index 7df79a0052..cd2dd4db32 100644 --- a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs +++ b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs @@ -99,12 +99,24 @@ public class FixedWindowOperationRateLimitingRule : IOperationRateLimitingRule $"Phone number is required for policy '{PolicyName}' (PartitionByPhoneNumber). Provide it via context.Parameter or ensure the user has a phone number."), OperationRateLimitingPartitionType.Custom => - await Definition.CustomPartitionKeyResolver!(context), + await ResolveCustomPartitionKeyAsync(context), _ => throw new AbpException($"Unknown partition type: {Definition.PartitionType}") }; } + protected virtual async Task ResolveCustomPartitionKeyAsync(OperationRateLimitingContext context) + { + var key = await Definition.CustomPartitionKeyResolver!(context); + if (string.IsNullOrEmpty(key)) + { + throw new AbpException( + $"Custom partition key resolver returned null or empty for policy '{PolicyName}'. " + + "The resolver must return a non-empty string."); + } + return key; + } + protected virtual string BuildStoreKey(string partitionKey) { // Stable rule descriptor based on content so reordering rules does not change the key. diff --git a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Store/DistributedCacheOperationRateLimitingStore.cs b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Store/DistributedCacheOperationRateLimitingStore.cs index ca64981e3b..d9f13b41d1 100644 --- a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Store/DistributedCacheOperationRateLimitingStore.cs +++ b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Store/DistributedCacheOperationRateLimitingStore.cs @@ -38,7 +38,7 @@ public class DistributedCacheOperationRateLimitingStore : IOperationRateLimiting IsAllowed = false, CurrentCount = 0, MaxCount = maxCount, - RetryAfter = duration + RetryAfter = null }; } @@ -111,7 +111,7 @@ public class DistributedCacheOperationRateLimitingStore : IOperationRateLimiting IsAllowed = false, CurrentCount = 0, MaxCount = maxCount, - RetryAfter = duration + RetryAfter = null }; } diff --git a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingTestModule.cs b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingTestModule.cs index 7a43716927..cd436546f1 100644 --- a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingTestModule.cs +++ b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingTestModule.cs @@ -161,6 +161,13 @@ public class AbpOperationRateLimitingTestModule : AbpModule .PartitionBy(ctx => Task.FromResult($"action:{ctx.Parameter}")); }); + // Custom resolver returning null - should throw + options.AddPolicy("TestCustomResolverNull", policy => + { + policy.WithFixedWindow(TimeSpan.FromHours(1), maxCount: 2) + .PartitionBy(ctx => Task.FromResult(null!)); + }); + // Multi-tenant: ByParameter with tenant isolation - same param, different tenants = different counters options.AddPolicy("TestMultiTenantByParameter", policy => { diff --git a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/DistributedCacheOperationRateLimitingStore_Tests.cs b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/DistributedCacheOperationRateLimitingStore_Tests.cs index aa21159613..b612419e48 100644 --- a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/DistributedCacheOperationRateLimitingStore_Tests.cs +++ b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/DistributedCacheOperationRateLimitingStore_Tests.cs @@ -118,7 +118,7 @@ public class DistributedCacheOperationRateLimitingStore_Tests : OperationRateLim result.IsAllowed.ShouldBeFalse(); result.CurrentCount.ShouldBe(0); result.MaxCount.ShouldBe(0); - result.RetryAfter.ShouldNotBeNull(); + result.RetryAfter.ShouldBeNull(); } [Fact] @@ -130,6 +130,6 @@ public class DistributedCacheOperationRateLimitingStore_Tests : OperationRateLim result.IsAllowed.ShouldBeFalse(); result.CurrentCount.ShouldBe(0); result.MaxCount.ShouldBe(0); - result.RetryAfter.ShouldNotBeNull(); + result.RetryAfter.ShouldBeNull(); } } diff --git a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs index 23dcfb6799..8a967077c5 100644 --- a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs +++ b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs @@ -653,6 +653,7 @@ public class OperationRateLimitingChecker_Tests : OperationRateLimitingTestBase exception.Result.IsAllowed.ShouldBeFalse(); exception.Result.MaxCount.ShouldBe(0); + exception.Result.RetryAfter.ShouldBeNull(); exception.HttpStatusCode.ShouldBe(429); } @@ -674,6 +675,7 @@ public class OperationRateLimitingChecker_Tests : OperationRateLimitingTestBase status.IsAllowed.ShouldBeFalse(); status.MaxCount.ShouldBe(0); status.RemainingCount.ShouldBe(0); + status.RetryAfter.ShouldBeNull(); } [Fact] @@ -701,6 +703,19 @@ public class OperationRateLimitingChecker_Tests : OperationRateLimitingTestBase (await _checker.IsAllowedAsync("TestCustomResolver", ctx2)).ShouldBeTrue(); } + [Fact] + public async Task Should_Throw_When_Custom_Resolver_Returns_Null() + { + var context = new OperationRateLimitingContext { Parameter = "test" }; + + var exception = await Assert.ThrowsAsync(async () => + { + await _checker.CheckAsync("TestCustomResolverNull", context); + }); + + exception.Message.ShouldContain("Custom partition key resolver returned null or empty"); + } + [Fact] public void Should_Throw_When_Policy_Has_Duplicate_Rules() {