Fix SimpleStateCheckerManager overwriting result between batch checkers#25417
Conversation
When a state has multiple batch checkers (e.g. RequirePermissions plus RequireFeatures), the per-state result was overwritten on every checker pass, so a later 'true' silently masked an earlier 'false'. AND-combine the results to require all checkers to pass.
There was a problem hiding this comment.
Pull request overview
Fixes an incorrect result aggregation in SimpleStateCheckerManager.IsEnabledAsync(TState[]) where per-state results from multiple batch checkers were previously overwritten instead of combined, allowing a later true to mask an earlier false. This aligns batch-checker behavior with the expected “all checkers must pass” (AND) semantics and prevents incorrect enablement/visibility decisions (e.g., menu items requiring both features and permissions).
Changes:
- AND-combine per-state results across multiple batch state checkers (
result[state] = result[state] && checkerResult). - Add regression tests covering false→true, true→false, and all-true batch-checker orderings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| framework/src/Volo.Abp.Core/Volo/Abp/SimpleStateChecking/SimpleStateCheckerManager.cs | Fixes batch-checker aggregation to AND-combine results instead of overwriting. |
| framework/test/Volo.Abp.Core.Tests/Volo/Abp/SimpleStateChecking/SimpleStateChecker_AndCombineResults_Tests.cs | Adds regression tests validating correct aggregation across multiple batch checkers and ordering scenarios. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rel-10.4 #25417 +/- ##
============================================
- Coverage 49.32% 49.30% -0.03%
============================================
Files 3667 3668 +1
Lines 123186 123235 +49
Branches 9409 9412 +3
============================================
- Hits 60765 60757 -8
- Misses 60596 60666 +70
+ Partials 1825 1812 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When a state has multiple batch checkers (e.g.
RequirePermissionsplusRequireFeatures),SimpleStateCheckerManager.IsEnabledAsync(TState[])overwrites the per-state result on every checker pass, so a latertruesilently masks an earlierfalse. AND-combine the results so all checkers must pass.This shows up in practice on menu items that chain
.RequireFeatures(...).RequirePermissions(...)(e.g.Language management,Text templates,Audit logs): when a role grants the feature but not the permission, the menu item still becomes visible because the feature checker'strueoverwrites the permission checker'sfalse.Added regression tests under
framework/test/Volo.Abp.Core.Tests/Volo/Abp/SimpleStateChecking/SimpleStateChecker_AndCombineResults_Tests.cscovering false-then-true, true-then-false and all-true orderings. Reverse-verified: with the fix removed, the false-then-true test fails.