Browse Source

fix: update RetryAfter handling to return null for ban policies and add custom resolver null check

pull/25024/head
maliming 3 weeks ago
parent
commit
fdda4da461
No known key found for this signature in database GPG Key ID: A646B9CB645ECEA4
  1. 6
      docs/en/framework/infrastructure/operation-rate-limiting.md
  2. 14
      framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs
  3. 4
      framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Store/DistributedCacheOperationRateLimitingStore.cs
  4. 7
      framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingTestModule.cs
  5. 4
      framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/DistributedCacheOperationRateLimitingStore_Tests.cs
  6. 15
      framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs

6
docs/en/framework/infrastructure/operation-rate-limiting.md

@ -205,7 +205,9 @@ policy.WithFixedWindow(TimeSpan.FromHours(1), maxCount: 100)
## Multi-Tenancy ## 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 ````csharp
policy.AddRule(rule => rule policy.AddRule(rule => rule
@ -326,7 +328,7 @@ public override void ConfigureServices(ServiceConfigurationContext context)
### Ban Policy (maxCount: 0) ### 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 ````csharp
options.AddPolicy("BlockedUser", policy => options.AddPolicy("BlockedUser", policy =>

14
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."), $"Phone number is required for policy '{PolicyName}' (PartitionByPhoneNumber). Provide it via context.Parameter or ensure the user has a phone number."),
OperationRateLimitingPartitionType.Custom => OperationRateLimitingPartitionType.Custom =>
await Definition.CustomPartitionKeyResolver!(context), await ResolveCustomPartitionKeyAsync(context),
_ => throw new AbpException($"Unknown partition type: {Definition.PartitionType}") _ => throw new AbpException($"Unknown partition type: {Definition.PartitionType}")
}; };
} }
protected virtual async Task<string> 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) protected virtual string BuildStoreKey(string partitionKey)
{ {
// Stable rule descriptor based on content so reordering rules does not change the key. // Stable rule descriptor based on content so reordering rules does not change the key.

4
framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Store/DistributedCacheOperationRateLimitingStore.cs

@ -38,7 +38,7 @@ public class DistributedCacheOperationRateLimitingStore : IOperationRateLimiting
IsAllowed = false, IsAllowed = false,
CurrentCount = 0, CurrentCount = 0,
MaxCount = maxCount, MaxCount = maxCount,
RetryAfter = duration RetryAfter = null
}; };
} }
@ -111,7 +111,7 @@ public class DistributedCacheOperationRateLimitingStore : IOperationRateLimiting
IsAllowed = false, IsAllowed = false,
CurrentCount = 0, CurrentCount = 0,
MaxCount = maxCount, MaxCount = maxCount,
RetryAfter = duration RetryAfter = null
}; };
} }

7
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}")); .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<string>(null!));
});
// Multi-tenant: ByParameter with tenant isolation - same param, different tenants = different counters // Multi-tenant: ByParameter with tenant isolation - same param, different tenants = different counters
options.AddPolicy("TestMultiTenantByParameter", policy => options.AddPolicy("TestMultiTenantByParameter", policy =>
{ {

4
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.IsAllowed.ShouldBeFalse();
result.CurrentCount.ShouldBe(0); result.CurrentCount.ShouldBe(0);
result.MaxCount.ShouldBe(0); result.MaxCount.ShouldBe(0);
result.RetryAfter.ShouldNotBeNull(); result.RetryAfter.ShouldBeNull();
} }
[Fact] [Fact]
@ -130,6 +130,6 @@ public class DistributedCacheOperationRateLimitingStore_Tests : OperationRateLim
result.IsAllowed.ShouldBeFalse(); result.IsAllowed.ShouldBeFalse();
result.CurrentCount.ShouldBe(0); result.CurrentCount.ShouldBe(0);
result.MaxCount.ShouldBe(0); result.MaxCount.ShouldBe(0);
result.RetryAfter.ShouldNotBeNull(); result.RetryAfter.ShouldBeNull();
} }
} }

15
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.IsAllowed.ShouldBeFalse();
exception.Result.MaxCount.ShouldBe(0); exception.Result.MaxCount.ShouldBe(0);
exception.Result.RetryAfter.ShouldBeNull();
exception.HttpStatusCode.ShouldBe(429); exception.HttpStatusCode.ShouldBe(429);
} }
@ -674,6 +675,7 @@ public class OperationRateLimitingChecker_Tests : OperationRateLimitingTestBase
status.IsAllowed.ShouldBeFalse(); status.IsAllowed.ShouldBeFalse();
status.MaxCount.ShouldBe(0); status.MaxCount.ShouldBe(0);
status.RemainingCount.ShouldBe(0); status.RemainingCount.ShouldBe(0);
status.RetryAfter.ShouldBeNull();
} }
[Fact] [Fact]
@ -701,6 +703,19 @@ public class OperationRateLimitingChecker_Tests : OperationRateLimitingTestBase
(await _checker.IsAllowedAsync("TestCustomResolver", ctx2)).ShouldBeTrue(); (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<AbpException>(async () =>
{
await _checker.CheckAsync("TestCustomResolverNull", context);
});
exception.Message.ShouldContain("Custom partition key resolver returned null or empty");
}
[Fact] [Fact]
public void Should_Throw_When_Policy_Has_Duplicate_Rules() public void Should_Throw_When_Policy_Has_Duplicate_Rules()
{ {

Loading…
Cancel
Save