[RORDEV-1388] New sytax of groups rules, refactor and common decoder #1080
Conversation
# Conflicts: # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/BlockContext.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/ImpersonationWarning.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/BaseGroupsRule.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsAndRule.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsOrRule.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/LdapAuthorizationRule.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/users/LocalUsersContext.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/domain/userAndGroups.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/common.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/ruleDecoders.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsLogicDecoder.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsRulesDecoder.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRuleDecoder.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/LdapRulesDecoders.scala # core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnAuthRuleDecoder.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/BaseGroupsRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/ExternalAuthorizationRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAllOfRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAnyOfRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAllOfRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAnyOfRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/LdapAuthRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/LdapAuthorizationRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/RorKbnAuthRuleTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/domain/GroupsLogicExecutorTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/ExternalAuthorizationRuleSettingsTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/GroupsRuleSettingsTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthRuleSettingsTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/LdapAuthRuleSettingsTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/LdapAuthorizationRuleSettingsTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthRuleSettingsTests.scala # gradle.properties
# Conflicts: # gradle.properties
mgoworko
changed the title
| @@ -54,8 +54,7 @@ object BlockValidator { | |||
| .map(_.rule) | |||
| .collect { case a: AuthenticationRule => a } | |||
| .filter { | |||
| case _: GroupsOrRule => false | |||
| case _: GroupsAndRule => false | |||
| case _: GroupsRule[_] => false | |||
There was a problem hiding this comment.
Here and in UserDefinitionsDecoder - the original implementation was created when there was only an "or" groups rule. I think that it is correct to match all groups rules here.
| import scala.compiletime.ops.string.Matches | ||
| import scala.compiletime.{constValue, error} | ||
|
|
||
| private[auth] class GroupsLogicRepresentationDecoder[ |
There was a problem hiding this comment.
Single, unified decoder implementation for all types of rules - GroupsRule, LDAP, JWT, external. Unified config structure and error handling. For GroupsRule the RuntimeResolvableGroupsLogic implementation is used, but for LDAP, JWT and External - the GroupsLogic. This decoder can work on both.
📝 WalkthroughWalkthroughThis pull request refactors and consolidates the group logic handling across the access control codebase. Key changes include renaming and replacing group rule types (e.g., from GroupsOrRule/GroupsAndRule to a unified GroupsRule with specific logic types like AnyOf and AllOf), streamlining implicit definitions, and updating pattern matches, imports, and method signatures. Significant refactoring updates the mechanism for decoding rules by introducing a generic GroupsLogicRepresentationDecoder, along with new decoders such as GroupsRuleDecoder. Furthermore, the refactor includes a new trait for runtime resolvable group logic with distinct Simple and Combined implementations. Test classes and helper functions are also updated to align with the new naming conventions and structures. Lastly, the gradle plugin version was updated from “1.63.0-pre5” to “1.63.0-pre6.” Sequence Diagram(s)sequenceDiagram
participant Client as Client/Application
participant RD as Rule Decoder
participant GRD as GroupsRuleDecoder
participant GLRD as GroupsLogicRepresentationDecoder
participant RDf as Rule Definition
Client->>RD: Submit rule JSON
RD->>GRD: Invoke groups rule decoding
GRD->>GLRD: Decode group logic from JSON
GLRD-->>GRD: Return GroupsLogicDecodingResult (Success/Error)
GRD->>GRD: Validate user definitions
GRD-->>RD: Create and return GroupsRule instance
RD-->>Client: Return final Rule Definition
sequenceDiagram
participant Caller as Runtime Resolver
participant RRG as RuntimeResolvableGroupsLogic
participant GLC as GroupsLogic.Creator
Caller->>RRG: Invoke resolve(groupIds)
alt Using Simple Logic
RRG->>GLC: Create GroupsLogic instance from groupIds
GLC-->>RRG: Return Simple GroupsLogic
else Using Combined Logic
RRG->>RRG: Resolve positive and negative Simple logics
RRG-->>GLC: Combine results to form Combined GroupsLogic
end
RRG-->>Caller: Return resolved GroupsLogic instance
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsLogicRepresentationDecoder.scala (4)
33-41: Consider reducing the complexity of type parameters.This constructor defines many type parameters for handling different logic representations, which can make it harder for newcomers to understand and maintain. If feasible, you might encapsulate these constraints or simplify the hierarchy to enhance readability.
68-82: Potential clarity improvement inwithGroupsSectionDecoder.This method decodes multiple fields (
all_of,any_of, etc.). Consider providing additional in-line documentation or a more explicit error message when malformed definitions occur, helping users quickly troubleshoot array-based configs.
83-110: Ensure legacy decoding logic remains maintainable.The
legacyWithoutGroupsSectionDecoderhandles older config fields (groups_and,roles, etc.). As the codebase evolves, there is a risk of overlooking changes for legacy paths. Consider centralizing any shared logic or tests so it remains synchronized.
111-139: Pattern match complexity inresultFromLogic.This comprehensive match handles many possible logic combinations. While it works well, consider splitting out nested logic or grouping repeated patterns for readability. This can help with maintainability and edge-case testing in the future.
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/GroupsRuleSettingsTests.scala (2)
306-337: Variable-driven group IDs are well tested.Verifying group variables (e.g.,
"group_@{header:test}") ensures that functionality is correct for dynamic scenarios. Adding corner-case tests (like empty headers or null values) could further strengthen confidence in production usage.
1389-1390: Future expansions for combined rules.This new "Combined GroupsRule" scenario is crucial. The initial test looks good, but watch for interplay with older rules. Adding a specific test for unexpected field combos might help reveal hidden friction.
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsRule.scala (2)
71-84: Consider extracting the pattern matching logic into a separate method.The nested pattern matching in
continueCheckingWithUserDefinitionscould be simplified for better readability and maintainability.- blockContext.userMetadata.loggedUser match { - case Some(user) => - NonEmptyList.fromFoldable(userDefinitionsMatching(user.id)) match { - case None => - Task.now(Rejected()) - case Some(filteredUserDefinitions) => - tryToAuthorizeAndAuthenticateUsing(filteredUserDefinitions, blockContext, permittedGroupsLogic) - } - case None => - tryToAuthorizeAndAuthenticateUsing(settings.usersDefinitions, blockContext, permittedGroupsLogic) - } + def getUserDefinitions(blockContext: B): NonEmptyList[UserDef] = { + blockContext.userMetadata.loggedUser + .flatMap(user => NonEmptyList.fromFoldable(userDefinitionsMatching(user.id))) + .getOrElse(settings.usersDefinitions) + } + tryToAuthorizeAndAuthenticateUsing(getUserDefinitions(blockContext), blockContext, permittedGroupsLogic)
106-138: Consider breaking down the complex authorization logic.The
authorizeAndAuthenticatemethod has multiple levels of nesting and pattern matching. Consider extracting the mode-specific logic into separate methods.+ private def handleUserMode[B <: BlockContext : BlockContextUpdater]( + userDef: UserDef, + blockContext: B, + allowedUserMatcher: GenericPatternMatcher[User.Id], + availableGroups: UniqueNonEmptyList[Group] + ): Task[Option[B]] = userDef.mode match { + case Mode.WithoutGroupsMapping(auth, _) => + authenticate(auth, blockContext, allowedUserMatcher, availableGroups, userDef.mode) + case Mode.WithGroupsMapping(Auth.SingleRule(auth), groupMappings) => + authenticateAndAuthorize( + auth, + groupMappings, + blockContext, + allowedUserMatcher, + availableGroups, + userDef.mode + ) + case Mode.WithGroupsMapping(Auth.SeparateRules(authn, authz), groupMappings) => + authenticateAndAuthorize( + authnRule = authn, + authzRule = authz, + groupMappings, + blockContext, + allowedUserMatcher, + availableGroups, + userDef.mode + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/BlockContext.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/ImpersonationWarning.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/RuleOrdering.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsAndRule.scala(0 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsNotAllOfRule.scala(0 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsNotAnyOfRule.scala(0 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsOrRule.scala(0 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsRule.scala(3 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/LdapAuthorizationRule.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/users/LocalUsersContext.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/userAndGroups.scala(4 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/BlockValidator.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/common.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/UsersDefinitionsDecoder.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/ruleDecoders.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/RuleBaseDecoder.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsLogicDecoder.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsLogicRepresentationDecoder.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsRuleDecoder.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsRulesDecoder.scala(0 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRuleDecoder.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/LdapRulesDecoders.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnAuthRuleDecoder.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/utils/ADecoder.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/BaseGroupsNegativeRuleTests.scala(2 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/BaseGroupsPositiveRuleTests.scala(2 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/ExternalAuthorizationRuleTests.scala(17 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAllOfRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAnyOfRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAllOfRuleTests.scala(2 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAnyOfRuleTests.scala(2 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsRuleTests.scala(3 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala(8 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/LdapAuthRuleTests.scala(17 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/LdapAuthorizationRuleTests.scala(14 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/RorKbnAuthRuleTests.scala(10 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/http/HeaderAnyOfRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/http/HeadersAllOfRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/domain/GroupsLogicExecutorTests.scala(14 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/ExternalAuthorizationRuleSettingsTests.scala(8 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/GroupsRuleSettingsTests.scala(49 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthRuleSettingsTests.scala(5 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/LdapAuthorizationRuleSettingsTests.scala(9 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthRuleSettingsTests.scala(5 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/http/HeadersAllOfRuleSettingsTests.scala(1 hunks)gradle.properties(1 hunks)
💤 Files with no reviewable changes (5)
- core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsNotAnyOfRule.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsNotAllOfRule.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsAndRule.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsRulesDecoder.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsOrRule.scala
✅ Files skipped from review due to trivial changes (7)
- gradle.properties
- core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/http/HeadersAllOfRuleSettingsTests.scala
- core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/http/HeaderAnyOfRuleTests.scala
- core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/http/HeadersAllOfRuleTests.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRuleDecoder.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnAuthRuleDecoder.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/BlockContext.scala
👮 Files not reviewed due to content moderation or server errors (2)
- core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/BaseGroupsPositiveRuleTests.scala
- core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/BaseGroupsNegativeRuleTests.scala
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests LICENSE)
🔇 Additional comments (92)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/users/LocalUsersContext.scala (2)
31-32: LGTM! Clean import restructuring.The imports have been consolidated to support the new unified group rules approach, improving code organization.
57-57: LGTM! Well-designed generic groups rule.The generic
groupsRuledefinition elegantly replaces multiple specific implicits, aligning with the PR's goal of simplifying group rules. The type parameterGL <: GroupsLogicensures type safety while providing flexibility.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/ExternalAuthorizationRuleTests.scala (1)
68-68: LGTM! The renaming improves code clarity.The consistent replacement of
Or/AndwithAnyOf/AllOfmakes the group matching logic more explicit and self-documenting. The test coverage remains comprehensive, ensuring the refactored logic maintains the same behavior.Also applies to: 92-92, 118-118, 142-142, 168-168, 192-192, 217-217, 247-247, 260-260, 278-278, 296-296, 314-314, 397-397, 414-414, 436-436, 458-458, 478-478
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala (1)
397-397: LGTM! Consistent renaming of group logic operators.The changes consistently replace
GroupsLogic.Or/AndwithGroupsLogic.AnyOf/AllOfacross all test cases, aligning with the PR's objective to introduce a unified and simplified syntax for group rules.Also applies to: 431-431, 465-465, 499-499, 641-641, 661-661, 681-681, 719-719
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala (2)
29-29: LGTM: Import aligns with new group rules architecture.The import of
GroupsLogicsupports the unified approach to group rules handling.
57-57: LGTM: Well-designed generic groups rule implementation.The new generic
groupsRuledefinition elegantly unifies the handling of different group rule types while maintaining type safety through the bounded type parameterGL <: GroupsLogic. This aligns perfectly with the PR's objective of simplifying group rules syntax.Key benefits:
- Reduces code duplication
- Maintains type safety
- Simplifies maintenance
- Provides flexibility for future group logic types
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthRuleSettingsTests.scala (2)
130-130: LGTM! Improved naming clarity.The renaming from
Or/AndtoAnyOf/AllOfmakes the group logic more explicit and self-documenting.Also applies to: 163-163
700-724: LGTM! Consistent error message updates.The error message updates correctly reflect the new group logic naming convention.
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/ExternalAuthorizationRuleSettingsTests.scala (1)
91-93: LGTM! Consistent renaming of group logic types.The changes consistently replace
GroupsLogic.OrwithGroupsLogic.AnyOfacross all test cases while maintaining the same functionality. The type assertions are correctly updated to match the new naming convention.Also applies to: 147-147, 201-201, 249-249, 299-301, 350-353, 409-413
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/LdapAuthorizationRuleTests.scala (1)
71-73: LGTM! Excellent refactoring of group logic naming.The consistent replacement of
Or/AndwithAnyOf/AllOfacross all test cases improves code clarity by making the group logic more explicit and self-documenting. The changes maintain the same test coverage while aligning with the new unified syntax.Also applies to: 93-95, 117-119, 139-141, 233-235, 262-264, 278-280, 290-292, 306-308, 402-404, 421-423, 443-445, 465-467, 484-486
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/BlockValidator.scala (2)
24-24: LGTM! Import change aligns with group rules unification.The consolidated import reflects the new unified group rules structure.
57-57: LGTM! Filtering logic correctly handles all group rules.The pattern match using
GroupsRule[_]appropriately excludes all group rules from the authentication rule uniqueness check, as intended per the past review comment.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/BaseGroupsPositiveRuleTests.scala (3)
25-25: LGTM! Good refactoring of the test hierarchy.The changes align well with the PR's goal of unifying group rules syntax. The renaming from
BaseGroupsRuletoGroupsRuleand the flattened inheritance hierarchy make the code structure clearer while preserving all test cases.Also applies to: 36-36
25-25: LGTM! Clean refactoring of group rule tests.The changes align well with the PR's objective of unifying group rules syntax. The trait inheritance and import updates maintain test functionality while adapting to the new structure.
Also applies to: 36-36
25-25: LGTM! Clean refactoring of imports and trait inheritance.The changes align with the broader refactoring effort to unify group rules:
- Import statement updated to use
GroupsRule.Settings- Trait now extends
GroupsRuleTestsinstead ofBaseGroupsRuleTestsThese changes maintain the same test coverage while simplifying the codebase structure.
Also applies to: 36-36
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/BaseGroupsNegativeRuleTests.scala (3)
25-25: LGTM! Consistent refactoring across test files.The changes mirror those in
BaseGroupsPositiveRuleTests.scala, maintaining symmetry between positive and negative test files while preserving test coverage.Also applies to: 36-36
25-25: LGTM! Consistent refactoring with positive test cases.The changes mirror those in
BaseGroupsPositiveRuleTests.scala, maintaining symmetry in the test structure while adapting to the new unified group rules syntax.Also applies to: 36-36
25-25: LGTM! Consistent refactoring with BaseGroupsPositiveRuleTests.The changes mirror those in BaseGroupsPositiveRuleTests.scala, maintaining consistency across the test suite:
- Import statement updated to use
GroupsRule.Settings- Trait now extends
GroupsRuleTestsinstead ofBaseGroupsRuleTestsAlso applies to: 36-36
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/ImpersonationWarning.scala (2)
33-34: LGTM! Clean import organization.The new imports for
GroupsLogicand its members are properly organized and specific, aligning with the PR's goal of unifying group rules.
134-134: LGTM! Well-designed generic implementation.The new generic implicit method
groupsRule[GL <: GroupsLogic]effectively consolidates multiple specific group rule implicits into a single, type-safe implementation. This simplification aligns well with the PR's objective of unifying group rules syntax while maintaining the same behavior.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/RorKbnAuthRuleTests.scala (7)
126-128: LGTM! Improved naming clarity.The change from
OrtoAnyOfmakes the group logic more descriptive and aligns with standard terminology.
153-155: LGTM! Consistent naming pattern.The change maintains consistency with the previous update, using
AnyOffor group logic.
182-184: LGTM! Clear test case description.The test case title "groups OR logic is used" aligns well with the
AnyOfimplementation, making the test's purpose clear.
209-211: LGTM! Pattern matching test case.Good test coverage showing that
AnyOfworks correctly with wildcard patterns in group IDs.
238-240: LGTM! Logical AND test case.The change from
AndtoAllOfimproves readability while maintaining the same logical behavior.
265-267: LGTM! Pattern matching with AllOf.Comprehensive test case showing that
AllOfworks correctly with wildcard patterns in group IDs.
339-341: LGTM! Consistent error case handling.The negative test cases have been updated consistently to use the new
AnyOf/AllOfterminology while maintaining the same error validation logic.Also applies to: 357-360, 377-379, 411-413
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthRuleSettingsTests.scala (4)
118-119: LGTM! Improved naming clarity.The renaming from
OrtoAnyOfmakes the group logic more explicit and self-documenting.
152-153: LGTM! Consistent naming improvement.The renaming from
AndtoAllOfaligns with the previous change and makes the group logic more descriptive.
394-406: LGTM! Variable names updated for consistency.The variable renaming from
groupsOrKey/groupsAndKeytogroupsAnyOfKey/groupsAllOfKeymaintains consistency with the new group logic naming.
416-416: LGTM! Error message updated.The error message has been updated to reflect the new naming convention.
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/UsersDefinitionsDecoder.scala (2)
32-32: LGTM! Import updated to use unified group rules.The change from
GroupsOrRuletoGroupsRulealigns with the PR's objective of unifying group rules into a single, more flexible type.
169-169: LGTM! Pattern matching updated for generic group rule type.The pattern matching now uses
GroupsRule[_], making it compatible with all group rule types while preserving the existing error handling behavior.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAllOfRuleTests.scala (4)
23-25: Clean imports using modern Scala syntax.The imports are well-organized and use the new Scala 3
askeyword for aliasing.
32-32: Clear class naming that reflects its purpose.The class name
GroupsAllOfRuleTestsbetter describes the logic being tested compared to the previousGroupsAndRuleTests.
34-38: Clean method overrides with improved type safety.The method signatures are well-defined and the implementation is concise. The addition of
ruleNameparameter improves traceability.
40-40: Test description matches the new naming convention.The test description accurately reflects what's being tested.
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/LdapAuthorizationRule.scala (1)
103-103: LGTM! Type rename maintains consistency.The change from
GroupsLogic.CombinedGroupsLogictoGroupsLogic.Combinedaligns with the simplified group logic naming convention.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/LdapRulesDecoders.scala (1)
225-225: LGTM! Pattern matching updated correctly.The pattern matching case update maintains consistency with the renamed
GroupsLogic.Combinedtype.core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/LdapAuthorizationRuleSettingsTests.scala (4)
59-60: LGTM! Test assertion updated to use new type.The assertion correctly uses
GroupsLogic.AnyOfinstead of the oldGroupsLogic.Ortype.
88-90: LGTM! Test assertion updated for AllOf logic.The assertion correctly uses
GroupsLogic.AllOfand properly verifies the pattern matching behavior.
408-413: LGTM! Combined group logic test updated.The test correctly validates the new
GroupsLogic.Combinedtype with proper composition ofAnyOfandNotAnyOflogic.
124-154: Well-structured test coverage for new syntax.The new test cases thoroughly validate both legacy and new syntax for group rules, ensuring backward compatibility while promoting the new cleaner syntax.
Also applies to: 416-450
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/LdapAuthRuleTests.scala (2)
66-66: LGTM! Consistent renaming of group logic types.The changes consistently replace
Or/AndwithAnyOf/AllOfacross all test cases, maintaining the same behavior while improving readability.Also applies to: 86-86, 108-108, 128-128, 215-215, 249-249, 263-263, 277-277, 291-291, 375-375, 390-390, 405-405, 429-429, 453-453, 477-477, 501-501, 527-527
1-671:✅ Verification successful
Verify test coverage for all group logic combinations.
The test suite covers the main group logic scenarios but let's verify we haven't missed any edge cases.
🏁 Script executed:
#!/bin/bash # Description: Search for all group logic types to ensure complete test coverage # Test: Search for all group logic types in test files rg -A 2 'GroupsLogic\.(AnyOf|AllOf|NotAnyOf|NotAllOf)' --type scalaLength of output: 72780
All group logic combinations are comprehensively tested.
The extensive output from our search confirms that tests exist for all scenarios (AnyOf, AllOf, NotAnyOf, NotAllOf, as well as combined logic handling in various decoders and rule settings). No missing edge cases were identified.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsLogicDecoder.scala (2)
20-20: Looks good.
Explicitly importingNegativeGroupsLogicandPositiveGroupsLogicenhances readability and makes the usage of specific subtypes more evident.
23-32: Streamlined decoder inheritance.
By extendingGroupsLogicRepresentationDecoder, you centralize the decoding logic and reduce duplication. This is a clean approach that promotes consistency.core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala (5)
21-21: Good use of domain-specific type aliases.
DeclaringNegativeGroupsLogicandPositiveGroupsLogicfosters clarity by explicitly distinguishing different group logic profiles.
26-30: Well-structured trait definition.
DefiningRuntimeResolvableGroupsLogicas a trait withresolveandusedVariablesclarifies the contract for group logic resolution. This design choice supports future extensions while keeping the code modular.
32-32: Namespace approach is clear.
EncapsulatingRuntimeResolvableGroupsLogicin anobjecthelps organize related classes (Simple,Combined) under a cohesive namespace.
34-45: Simple class ensures clarity.
UsingUniqueNonEmptyListforgroupIdsis a robust way to prevent empty groups at compile-time. Theresolvelogic is straightforward, and mapping throughGroupIdskeeps data transformations well-defined.
47-58: Combined logic gracefully merges positive and negative group sets.
Merging the resolutions inpositiveandnegativeclarifies the combined logic approach. The usage ofOptionchaining is exemplary, elegantly handling cases where either resolution might fail.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsRuleDecoder.scala (7)
1-16: License header recognized.
Maintaining the existing license header is consistent with the codebase’s open-source requirements.
17-22: Appropriate imports.
The chosen imports (cats,io.circe, custom project classes) form a well-structured foundation for defining and decoding group rules.
26-37: Decoding approach is clean.
ImplementingGroupsRuleDecoderby extendingRuleBaseDecoderWithoutAssociatedFieldskeeps logic cohesive within the codebase’s decoding pattern.
38-43: Constructor design.
Directly injectingusersDefinitions,globalSettings, and an implicitvariableCreatorfosters testability and allows straightforward mocking in potential tests.
44-66: Well-handled decoding results.
EmployingemapEto handle success vs. multiple or undefined logic ensures clear, typed error returns. This is an elegant solution that promotes explicit error messages.
67-75: Instantiating the combined runtime logic.
ParameterizingGroupsLogicRepresentationDecoderwith helper type constructors clarifies how positive and negative groups converge, reducing confusion in the decoding process.
77-81: Implicit decoder effectively abstracts variable resolution.
This approach converts user-supplied strings or lists into aRuntimeResolvableGroupsLogic.Simple, keeping the calling code consistent and concise.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsLogicRepresentationDecoder.scala (5)
43-46: Decoder method design is clear.Delegating to
syncDecoderis a good pattern, separating the synchronous decoding logic from high-level usage. The code is concise and follows functional best practices well.
47-55: Smooth use ofemapE.Using
emapEto handle the differentGroupsLogicDecodingResultvariants is elegant. Ensuring that this covers all error states makes error reporting consistent. Great approach for user-facing configuration errors.
57-66: Validate legacy fallback triggers.
syncDecoderfalls back tolegacyWithoutGroupsSectionDecoderwhenGroupsLogicNotDefinedoccurs. Double-check that all existing or legacy configs reliably flow through this path, avoiding situations where partial definitions might slip silently.
141-153:decodeAsOptionapproach is flexible and concise.Returning
(REPRESENTATION, String)pairs is handy for debug or error messaging. Ensure all error contexts (e.g., unknown fields) are properly surfaced to keep the troubleshooting experience straightforward.
156-182: Compile-time check for applied names is robust.The usage of compile-time patterns (
Matches) for validating applicable names is a neat approach. Confirm that newly introduced field names are always updated inapplicableNamesto avoid cryptic compile errors in future expansions.core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/GroupsRuleSettingsTests.scala (6)
22-23: Great addition of ScalaCheck properties.Including
ScalaCheckPropertyChecksexpands the testing scenarios and provides robust coverage for a wide variety of inputs. This reduces the risk of corner cases going unnoticed.
47-60: Table-driven tests enhance readability.Using a
Tablefor different group rule names is a clean approach, making the test intentions clear and covering multiple permutations in a single pass.
339-373:user_belongs_to_groupsintegration is nicely covered.These tests confirm that new syntax merges easily with older config patterns. It may be beneficial to add negative tests for incorrectly combined definitions to ensure robust error reporting.
379-453: Broad coverage of separate authentication and authorization.In-depth checks of separate rules (LDAP, external, token) confirm real-world usage. Good job ensuring each user definition is tested for nuances like local vs. external groups.
579-651: Advanced group mappings seem thoroughly validated.The scenarios with local vs. external group definitions are well handled, ensuring complex relationships (e.g., structured groups with detailed IDs) are tested. This helps maintain trust in real-world LDAP or external group providers.
1391-1517: Thorough combined logic coverage.Multiple scenarios test any_of/not_any_of, all_of/not_all_of. Great job. Keep an eye on code complexity in the tests: if it continues to grow, consider factoring out repeated logic to maintain clarity.
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/RuleBaseDecoder.scala (1)
37-42: Clean separation of cursor navigation.Introducing
decodingContextnicely abstracts away the directdownFieldcalls, making theapplymethod more readable. It's a small refactor with a positive impact on code clarity.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAnyOfRuleTests.scala (3)
23-25: LGTM! Clean import updates.The imports have been correctly updated to reflect the new
GroupsRulestructure.
32-32: LGTM! Clean transition to the new GroupsRule structure.The class and method signatures have been properly updated to use the new unified
GroupsRulestructure withAnyOflogic type.Also applies to: 34-36
32-32: LGTM! Improved naming and structure.The refactoring from
GroupsOrRuletoGroupsRule[GroupsLogic.AnyOf]with explicit rule naming enhances clarity and maintainability.Also applies to: 34-36
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/RuleOrdering.scala (3)
29-29: LGTM! Clean import addition.The import for
GroupsLogic.*has been correctly added to support the new group rule types.
69-73: LGTM! Well-structured group rule ordering.The group rules have been properly organized with a clear hierarchy: AnyOf, AllOf, NotAnyOf, NotAllOf, and Combined. This ordering ensures consistent rule evaluation.
69-73: LGTM! Enhanced type safety in rule ordering.The replacement of generic group rules with specialized
GroupsRulevariants (AnyOf,AllOf,NotAnyOf,NotAllOf,Combined) improves type safety while maintaining the critical ordering of authentication before authorization.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAnyOfRuleTests.scala (3)
23-25: LGTM! Clean import updates.The imports have been correctly updated to reflect the new
GroupsRulestructure.
34-36: LGTM! Clean transition to the new GroupsRule structure.The method signature has been properly updated to use the new unified
GroupsRulestructure withNotAnyOflogic type.
34-36: LGTM! Consistent test structure.The updated rule creation aligns with the unified group rule structure while maintaining comprehensive test coverage.
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAllOfRuleTests.scala (3)
23-25: LGTM! Clean import updates.The imports have been correctly updated to reflect the new
GroupsRulestructure.
34-36: LGTM! Clean transition to the new GroupsRule structure.The method signature has been properly updated to use the new unified
GroupsRulestructure withNotAllOflogic type.
34-36: LGTM! Well-structured test implementation.The test implementation maintains thorough coverage while aligning with the unified group rule structure.
core/src/main/scala/tech/beshu/ror/accesscontrol/utils/ADecoder.scala (1)
231-231: Great addition of thepuremethod!The
puremethod follows functional programming best practices by providing a way to lift pure values into the decoder context. This is a useful addition that enhances the composability of decoders.core/src/main/scala/tech/beshu/ror/accesscontrol/domain/userAndGroups.scala (2)
156-166: Well-designed Creator pattern implementation!The introduction of the
Creatortrait with type-safe factory methods improves the API design and makes group logic creation more explicit.
172-174: Improved naming clarity with AnyOf/AllOf!The renaming from
Or/AndtoAnyOf/AllOfmakes the intent clearer and follows better naming conventions for logical operations.Also applies to: 184-185
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/ruleDecoders.scala (1)
63-64: Excellent consolidation of group rule handling!The unified approach using
GroupsLogicRepresentationDecodersimplifies the codebase while maintaining flexibility to handle different group logic types.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsRuleTests.scala (2)
58-58: Enhanced type safety with context bound!The addition of the
GroupsLogic.Creatorcontext bound improves type safety and makes the requirements more explicit.
66-66: Improved rule creation and simplified logic resolution!The explicit
ruleNameparameter makes rule creation clearer, and the simplifiedresolvableGroupsLogicimplementation reduces complexity.Also applies to: 69-69
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsRule.scala (1)
28-28: LGTM! Clean refactoring of the class signature.The refactoring improves clarity by making the
nameanduserIdCaseSensitivityparameters explicit in the constructor, while maintaining the core functionality.Also applies to: 38-40
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/common.scala (1)
322-326: LGTM! Clean renaming of group logic decoders.The renaming from
And/OrtoAllOf/AnyOfimproves clarity and better represents the logical operations.core/src/test/scala/tech/beshu/ror/unit/acl/domain/GroupsLogicExecutorTests.scala (1)
30-31: LGTM! Comprehensive test coverage maintained.The test cases have been properly updated to use the new
AllOf/AnyOfnaming while maintaining the same logical test coverage.Also applies to: 77-78, 290-291, 305-306, 318-319, 333-334, 346-347, 361-362, 374-375, 389-390, 402-403, 417-418
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -35,7 +35,9 @@ import tech.beshu.ror.accesscontrol.matchers.GenericPatternMatcher | |||
| import tech.beshu.ror.implicits.* | |||
| import tech.beshu.ror.utils.uniquelist.{UniqueList, UniqueNonEmptyList} | |||
|
|
|||
| abstract class BaseGroupsRule[GL <: GroupsLogic](val settings: Settings[GL]) | |||
| class GroupsRule[GL <: GroupsLogic](override val name: Rule.Name, | |||
There was a problem hiding this comment.
Yes, it could be done like that. But we'd rather have a class per rule even if the class is trivial. I see the value in this approach.
There was a problem hiding this comment.
Ok, will restore classes per rule.
beshu-tech/readonlyrest-docs#246 <= docs, I can't assign reviewer there
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsRule.scala (3)
58-310: Consider breaking down the complex authentication logic.The base class handles multiple responsibilities. Consider extracting the authentication and authorization logic into separate traits or helper classes to improve maintainability.
+ trait GroupAuthenticationLogic { + protected def authenticate[B <: BlockContext](blockContext: B): Task[RuleResult[B]] + } + + trait GroupAuthorizationLogic { + protected def authorize[B <: BlockContext](blockContext: B): Task[RuleResult[B]] + } + - abstract class GroupsRule[+GL <: GroupsLogic] + abstract class GroupsRule[+GL <: GroupsLogic] extends GroupAuthenticationLogic with GroupAuthorizationLogic
73-86: Consider extracting authentication logic to a separate trait.The authentication logic is complex and could benefit from being extracted into a separate trait for better maintainability and testability.
+trait GroupsAuthenticationLogic { + protected def authenticate[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] +} + abstract class GroupsRule[+GL <: GroupsLogic](...) - extends AuthRule { + extends AuthRule with GroupsAuthenticationLogic {
58-310: Consider adding documentation for complex authentication flow.The authentication and authorization logic is complex. Consider adding documentation to explain:
- The flow between authentication and authorization
- The group mapping process
- The error handling strategy
+/** + * Base class for group-based rules that handle both authentication and authorization. + * + * Authentication flow: + * 1. Resolve groups logic + * 2. Check current group eligibility + * 3. Match user definitions + * 4. Authenticate and authorize using matched definitions + * + * Group mapping process: + * - For simple mappings: Direct local group assignment + * - For advanced mappings: Pattern-based external to local group mapping + * + * Error handling: + * - Authentication/authorization errors are logged at debug level + * - Failed matches return None to allow fallback to other definitions + */ abstract class GroupsRule[+GL <: GroupsLogic](override val name: Rule.Name,core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala (1)
36-45: Consider adding error handling for group resolution.The
resolvemethod silently fails by returningNone. Consider adding logging or error tracking for failed resolutions.override def resolve[B <: BlockContext](blockContext: B): Option[GL] = { + logger.debug(s"Resolving groups for context: ${blockContext.requestContext.id.show}") UniqueNonEmptyList .from(resolveAll(groupIds.toNonEmptyList, blockContext)) .map(GroupIds.apply) .map(GroupsLogic.Creator[GL].create) + .orElse { + logger.debug(s"Failed to resolve groups for context: ${blockContext.requestContext.id.show}") + None + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsRule.scala(3 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsRuleDecoder.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAllOfRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAnyOfRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAllOfRuleTests.scala(2 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAnyOfRuleTests.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/GroupsRuleDecoder.scala
- core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAnyOfRuleTests.scala
- core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsAllOfRuleTests.scala
👮 Files not reviewed due to content moderation or server errors (2)
- core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsRule.scala
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (CVE check Job)
🔇 Additional comments (18)
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAllOfRuleTests.scala (2)
23-25: LGTM! Clean import refactoring.The imports have been updated to use the new unified group rule structure, aligning with the broader codebase refactoring.
34-35: LGTM! Method signature improved.The
createRulemethod has been updated to use the new unifiedGroupsRulestructure with the factory pattern, making it more maintainable and consistent with the new architecture.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/GroupsNotAnyOfRuleTests.scala (2)
23-25: LGTM! Clean import refactoring.The imports have been updated to reflect the new unified group rules structure, removing base class dependencies in favor of more specific implementations.
34-36: LGTM! Well-structured factory pattern implementation.The changes improve the code by:
- Using dependency injection for rule name
- Implementing factory pattern for rule creation
- Using more specific type parameters
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala (8)
26-30: Well-designed trait with clear responsibilities!The trait provides a clean abstraction for resolving group logic at runtime with well-defined methods.
36-45: Excellent use of type class pattern!The
Simpleclass demonstrates a clean implementation using the type class pattern withGroupsLogic.Creator, making it extensible and type-safe.
47-58: Clean composition of positive and negative group logic!The
Combinedclass effectively composes twoRuntimeResolvableGroupsLogicinstances, maintaining separation of concerns while providing a unified interface.
26-30: LGTM! Well-structured trait definition.The trait is well-designed with covariant type parameter and clear abstract methods, providing a solid foundation for group logic implementations.
47-58: LGTM! Clean implementation of combined group logic.The Combined class effectively handles both positive and negative group logic, with proper variable concatenation.
36-45:❓ Verification inconclusive
Consider adding validation for empty group IDs resolution.
While the implementation is generally good, consider adding validation to handle cases where
resolveAllreturns an empty list, which could lead to unexpected behavior.
🏁 Script executed:
#!/bin/bash # Search for potential empty list handling in resolveAll implementations ast-grep --pattern 'def resolveAll($_, $_) { $$$ }'Length of output: 55
Attention: Validate empty group IDs behavior
The current implementation uses
UniqueNonEmptyList.from(resolveAll(...))so that an empty list naturally converts toNone, but this implicit handling might hide issues where an empty resolution is unexpected. Please consider an explicit check (e.g., logging a warning or raising an error) to clearly signal when no group IDs could be resolved, ensuring that downstream behavior can properly account for such cases. Verify that this meets your design goals for error reporting and observability.
- File:
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/RuntimeResolvableGroupsLogic.scala(lines 36–45).
26-30: Well-designed trait with clear abstractions!The trait is well-structured with covariant type parameter and clear abstract methods.
47-58: Clean implementation of combined group logic!The
Combinedclass effectively handles both positive and negative group logic with a clear and concise implementation.core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/GroupsRule.scala (6)
38-56: Clean implementation of rule-specific classes!Each rule type has its own class, following the team's preference while maintaining a clean inheritance hierarchy.
312-336: Excellent use of type classes for rule creation!The companion object provides a clean factory method and well-structured type class pattern for rule creation.
38-56: LGTM! Clean implementation following team conventions.The separate classes per rule type follow the team's preferred approach, as discussed in previous reviews, with a clean inheritance structure.
321-335: LGTM! Well-implemented type-safe creator pattern.The Creator trait and its companion object provide a clean, type-safe way to instantiate specific group rules.
38-56: Clean and consistent rule implementations!The rule implementations follow a consistent pattern and effectively leverage inheritance.
312-337: Well-structured companion object with clean type class pattern!The companion object provides a clean factory method and type class instances for each rule type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/RuleOrdering.scala (1)
29-29: Consider using explicit imports instead of wildcard.While the wildcard import works, explicit imports would make dependencies clearer and prevent potential naming conflicts.
-import tech.beshu.ror.accesscontrol.domain.GroupsLogic.* +import tech.beshu.ror.accesscontrol.domain.GroupsLogic.{AnyOf, AllOf, NotAnyOf, NotAllOf, Combined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/RuleOrdering.scala(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests LICENSE)
🔇 Additional comments (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/RuleOrdering.scala (1)
69-73:❓ Verification inconclusive
LGTM! Verify completion of group rules migration.
The ordering of the new group rules is logical, placed correctly after authentication rules and following a clear pattern (positive, negative, combined checks).
Let's verify that all old group rule references have been removed:
🏁 Script executed:
#!/bin/bash # Search for any remaining references to old group rules rg "GroupsOrRule|GroupsAndRule" --type scalaLength of output: 46
LGTM! Verify that the complete group rules migration is in place.
- The new group rules ordering (i.e., positive checks, negative checks, then combined) remains logical and correctly positioned after the authentication rules.
- The automated search for legacy references (e.g.,
GroupsOrRuleorGroupsAndRule) returned no output, but given the low-quality inference, please manually verify that no remnants of the old group rule implementations remain.
🚀New (ES) [New, unified and simplified syntax of groups rules]
Summary by CodeRabbit
Creatortrait for better handling of group logic instantiation.