[RORDEV-1639] Separate JWT authentication and authorization rules #1182
Conversation
|
|
||
| // Pseudo-authorization rule should be used exclusively as part of the JwtAuthRule, when there are is no groups logic defined. | ||
| // It preserves the kbn_auth rule behavior from before introducing separate authn and authz rules. | ||
| final class JwtPseudoAuthorizationRule(val settings: Settings) |
There was a problem hiding this comment.
The logic from JwtAuthRule is now extracted into 3 parts:
- the
JwtAuthenticationRule - the
JwtAuthorizationRule - and this
JwtPseudoAuthorizationRule
The JwtAuthRule until now handled the situation, when there was no groups rule defined in a very specific way. It was even more complicated than the RorKbnAuth rule, which we refactored recently.
When groups rule is not defined, then the "authorization" part checks, if the current, preferred group is eligible. When the preferred group is not defined, then it is simply accepted and passes.
But this "pseduo-authorization" is different in other ways too - it does not add available groups to BlockContext's user metadata.
So if we would want to change this behavior, it would change the output BlockContext after the rule matches. We can either:
- preserve this pseudo-authorization logic (the current implementation extracted 3 parts of logic from
JwtAuthRule, but did not change the behavior) - remove pseudo-authorization and change the behavior
But as I wrote - this "pseudo-authorization" is even more complex, than the one for RorKbnAuth was. It does not add groups to context and additionally it accepts the situation, when there are no groups in JWT token - something that was not allowed in RorKbnAuth.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually from my point of view, points 2 and 6 are most problematic, but let me summarize as I understand it:
The pseudo-authorization rule (the authorization part of old auth rule when no groups) is points 2,4,6 - all with Groups.NotDefined
- point 2.
- we did not have a similar case in ROR KBN rules, because we did not allow parsing token with missing groups
- in jwt_auth rule we allowed situation, where there was a token with missing groups claim
- it is now part of "pseudo-authorization" - because we don't want such behavior in "real-authorization" rule
- point 4.
- yes, exactly, that is the case similar to the one in the ROR KBN auth rule
- yes, we changed the behavior of the ROR KBN auth rule (and the new authz rule) to not allow it
- point 6.
- it does the pseudo - authorization when there are groups in JWT, but no groups rule
- but it is not equivalent to
allow all - "*"- it does not modify the user metadata, so the result is different - probably we can change that, but it is an incompatible change in behavior
So the changes in the jwt_auth rule will be (if we remove the "pseudo-authorization" and use only the newly created "real-authorization"):
- from point 2. - ROR will no longer accept JWT tokens without groups claim
- we did not have similar case in ROR KBN, because we did not allow tokens without groups claim, but for JWT it looks like a feature to me
- but it is similar to the point below, so I guess we can change that (with breaking change in behavior)
- from point 4. - ROR will no longer accept siaution with not found groups claims in token
- we fixed it in the same way for ROR KBN (with breaking change in behavior) so I guess we can do the same here
- from point 6. - the real pseudo-authorization
- we can use
allow all - "*" - bu the resulting context (user metadata) will be different that before - it will start to contain user groups (they were not added to context in old implementation)
- we can use
There are 14 failing tests when I switch from the pseudo to real authorization. Most of them fail, because before this change the result was ALLOWED, and now is FORBIDDEN, because of the breaking changes from those 3 points.
There was a problem hiding this comment.
Thanks for the analysis.
IMO, changing behaviour from ALLOWED to FORBIDDEN is not a big deal if two conditions are met:
- the change is justified and explanable (I assume all the changes we discuss here are, so don't focus on that)
- if the user's solution was built on the pseudo authorization logic, we will be able to advise him how to modify ACL to achieve the same (e.g., by changing the
jwt_auth, to thejwt_authenticationrule)
Could you please analyse these points against the above?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
ok, can i ask you to recap this?
For each of point I described here, let's write if this is achievable now and how. If not, why don't we allow that now.
I'd be nice to have it in one place.
Let's link this comment in our jira.
...cala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/UsersDefinitionsDecoder.scala
Show resolved
Hide resolved
core/src/test/scala/tech/beshu/ror/unit/acl/factory/LocalUsersTest.scala
Show resolved
Hide resolved
This comment was marked as spam.
This comment was marked as spam.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
...main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRuleDecoder.scala
Outdated
Show resolved
Hide resolved
...main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRuleDecoder.scala
Outdated
Show resolved
Hide resolved
.../tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala
Show resolved
Hide resolved
...tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala
Show resolved
Hide resolved
...tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala
Outdated
Show resolved
Hide resolved
|
|
||
| // Pseudo-authorization rule should be used exclusively as part of the JwtAuthRule, when there are is no groups logic defined. | ||
| // It preserves the kbn_auth rule behavior from before introducing separate authn and authz rules. | ||
| final class JwtPseudoAuthorizationRule(val settings: Settings) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This reverts commit 6ff6868.
...in/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala
Outdated
Show resolved
Hide resolved
...tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala
Outdated
Show resolved
Hide resolved
...h/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthenticationRuleSettingsTests.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthenticationRuleTests.scala (1)
466-482: Fix incorrect assertion method in negative test case.The test case is in the "not match" section but calls
assertMatchRuleinstead ofassertNotMatchRule. Additionally,assertMatchRulerequires ablockContextAssertionparameter (see line 486-490) which is not provided here, causing this code to either fail compilation or exhibit incorrect test behavior.Apply this diff:
- assertMatchRule( + assertNotMatchRule( configuredJwtDef = AuthenticationJwtDef( JwtDef.Name("test"), AuthorizationTokenDef(Header.Name.authorization, "Bearer "), SignatureCheckMethod.Hmac(key.getEncoded), userClaim = domain.Jwt.ClaimName(jsonPathFrom("userId")), ), tokenHeader = bearerHeader(jwt), preferredGroupId = Some(GroupId("group3")) )core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala (1)
124-175: Two tests look misleading: titles mention groups/group-names, but YAML doesn’t configure them.
Right now these are effectively duplicates of the minimal “HMAC + user_claim” decode tests, so they won’t catch regressions in groups-related decoding. Consider either (a) addinggroup_ids_claim/group_names_claim(and asserting the decodedJwtDefsubtype/fields), or (b) renaming/removing the tests.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala (1)
70-72: Consider making decoder classes protected.Per the past review comment,
AuthenticationRuleDecoder,AuthorizationRuleDecoder, andAuthRuleDecodercould beprotected classto limit visibility to trait implementations. Since they're only needed by concrete decoders mixing in this trait, tighter visibility would improve encapsulation.
🧹 Nitpick comments (4)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/base/BaseJwtRule.scala (1)
102-112: Strengthen token parsing for NoCheck signature method.The manual token splitting at lines 104-106 assumes a specific token structure without validating the input format first. While the pattern match at line 105 provides a fallback case at lines 110-111, consider adding explicit validation of the token structure (e.g., checking part count and format) before attempting to parse, to provide clearer error messages and prevent unexpected exceptions.
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/JwtDefinitionsDecoder.scala (1)
96-99: Error message could mention legacy aliases (roles_claim,groups_claim) since they’re still accepted.
Not required, but it may reduce confusion for users migrating older configs.Also applies to: 173-180
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala (1)
340-346:envVarsProviderappears unused in this suite.
Consider dropping it unless you plan to add RSA/EC/env resolution tests here.core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scala (1)
69-77: Minor:claimsDescriptionis already aString;${claimsDescription.show}is likely redundant.
Consider interpolatingclaimsDescriptiondirectly for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthRule.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthenticationRule.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/base/BaseJwtRule.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/DefinitionsPack.scala(2 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/JwtDefinitionsDecoder.scala(4 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/auth/JwtAuthRulesDecoders.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnRulesDecoders.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/utils/CirceOps.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala(22 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthenticationRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthorizationRuleTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/ImpersonationWarningsTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthRuleSettingsTests.scala(28 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthRuleSettingsTests.scala(4 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthenticationRuleSettingsTests.scala(6 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthorizationRuleSettingsTests.scala(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/DefinitionsPack.scala
- core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthRuleSettingsTests.scala
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mgoworko
Repo: sscarduzio/elasticsearch-readonlyrest-plugin PR: 1163
File: core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala:96-98
Timestamp: 2025-10-03T21:07:20.002Z
Learning: In the ROR KBN authentication refactoring (PR #1163), the VariableUsage implicit for rorKbnAuthRule uses a union type `VariableUsage[RorKbnAuthRule | RorKbnAuthenticationRule]` because the decoder explicitly uses this union type to represent the fallback behavior where `ror_kbn_auth` without groups falls back to `RorKbnAuthenticationRule`. A separate `VariableUsage[RorKbnAuthRule]` implicit is not needed since there are no direct usages of that specific type - all usage goes through the union type in the decoder.
📚 Learning: 2025-10-03T21:07:20.002Z
Learnt from: mgoworko
Repo: sscarduzio/elasticsearch-readonlyrest-plugin PR: 1163
File: core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala:96-98
Timestamp: 2025-10-03T21:07:20.002Z
Learning: In the ROR KBN authentication refactoring (PR #1163), the VariableUsage implicit for rorKbnAuthRule uses a union type `VariableUsage[RorKbnAuthRule | RorKbnAuthenticationRule]` because the decoder explicitly uses this union type to represent the fallback behavior where `ror_kbn_auth` without groups falls back to `RorKbnAuthenticationRule`. A separate `VariableUsage[RorKbnAuthRule]` implicit is not needed since there are no direct usages of that specific type - all usage goes through the union type in the decoder.
Applied to files:
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthenticationRuleSettingsTests.scalacore/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthRuleSettingsTests.scalacore/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scalacore/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthorizationRuleSettingsTests.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/ruleDecoders.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthRule.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/JwtDefinitionsDecoder.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRulesDecoders.scalacore/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnRulesDecoders.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala
📚 Learning: 2025-09-21T14:09:30.387Z
Learnt from: mgoworko
Repo: sscarduzio/elasticsearch-readonlyrest-plugin PR: 1163
File: core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/base/RorKbnRuleHelper.scala:168-172
Timestamp: 2025-09-21T14:09:30.387Z
Learning: RorKbnDef.SignatureCheckMethod sealed trait has only three cases: Hmac(key: Array[Byte]), Rsa(pubKey: PublicKey), and Ec(pubKey: PublicKey). There is no NoCheck case.
Applied to files:
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthenticationRuleSettingsTests.scalacore/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthRuleSettingsTests.scalacore/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scalacore/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scalacore/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala
🧬 Code graph analysis (12)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthenticationRuleSettingsTests.scala (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/RawRorConfigBasedCoreFactory.scala (2)
RulesLevelCreationError(480-480)Message(487-487)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala (3)
JwtDef(37-51)SignatureCheckMethod(41-46)Name(38-38)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthorizationRuleSettingsTests.scala (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/RawRorConfigBasedCoreFactory.scala (4)
CoreCreationError(475-504)DefinitionsLevelCreationError(478-478)RulesLevelCreationError(480-480)Message(487-487)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/ruleDecoders.scala (2)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRulesDecoders.scala (1)
JwtAuthRulesDecoders(25-54)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnRulesDecoders.scala (1)
RorKbnRulesDecoders(25-52)
core/src/main/scala/tech/beshu/ror/accesscontrol/utils/CirceOps.scala (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/VariableTransformationAliasesDefinitionsDecoder.scala (1)
error(65-68)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scala (2)
core/src/main/scala/tech/beshu/ror/accesscontrol/utils/ClaimsOps.scala (3)
ClaimSearchResult(177-185)toClaimsOps(174-174)groupsClaim(80-108)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/base/BaseJwtRule.scala (1)
processUsingJwtToken(39-72)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/base/BaseJwtRule.scala (3)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala (6)
JwtDef(37-51)SignatureCheckMethod(41-46)Ec(45-45)Hmac(43-43)NoCheck(42-42)Rsa(44-44)core/src/main/scala/tech/beshu/ror/accesscontrol/request/RequestContext.scala (2)
RequestContext(94-199)authorizationToken(235-249)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/security.scala (1)
Payload(100-100)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/ldap/LdapService.scala (1)
idShow(31-31)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/JwtDefinitionsDecoder.scala (4)
core/src/main/scala/tech/beshu/ror/accesscontrol/utils/CirceOps.scala (4)
CirceOps(42-423)DecodingFailureUtils(322-327)fromError(318-320)fromError(325-327)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/security.scala (1)
Jwt(95-101)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala (4)
AuthJwtDef(74-78)Name(38-38)AuthenticationJwtDef(64-67)AuthorizationJwtDef(69-72)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/http.scala (1)
AuthorizationTokenDef(276-277)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRulesDecoders.scala (4)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthenticationRule.scala (4)
rules(47-51)JwtAuthenticationRule(36-72)JwtAuthenticationRule(74-81)Settings(80-80)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scala (4)
rules(40-49)JwtAuthorizationRule(33-79)JwtAuthorizationRule(81-88)Settings(87-87)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/userAndGroups.scala (1)
GroupsLogic(154-261)core/src/test/scala/tech/beshu/ror/unit/acl/blocks/definitions/user/UserDefinitionsValidatorTests.scala (1)
globalSettings(120-129)
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala (7)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthenticationRule.scala (5)
rules(47-51)JwtAuthenticationRule(36-72)JwtAuthenticationRule(74-81)Name(76-78)Settings(80-80)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scala (5)
rules(40-49)JwtAuthorizationRule(33-79)JwtAuthorizationRule(81-88)Name(83-85)Settings(87-87)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/groups/GroupsLogicRepresentationDecoder.scala (3)
rules(153-155)rules(157-159)auth(37-161)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthRule.scala (3)
JwtAuthRule(25-36)JwtAuthRule(38-42)Name(39-41)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/userAndGroups.scala (13)
GroupIdLike(80-121)GroupId(81-82)GroupId(84-86)LoggedUser(33-39)DirectlyLoggedUser(34-34)User(41-60)Id(42-42)Id(44-53)GroupsLogic(154-261)AnyOf(172-172)GroupIds(123-125)GroupIds(127-150)AllOf(174-174)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/security.scala (3)
Jwt(95-101)ClaimName(96-96)Payload(100-100)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/ExternalAuthenticationService.scala (2)
Name(46-46)Name(47-49)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnRulesDecoders.scala (4)
core/src/main/scala/tech/beshu/ror/accesscontrol/domain/userAndGroups.scala (1)
GroupsLogic(154-261)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRulesDecoders.scala (6)
ruleTypePrefix(36-36)docsUrl(38-38)createAuthenticationRule(40-44)createAuthorizationRule(44-48)createAuthRule(48-51)serializeDefinitionId(51-54)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/RorKbnAuthenticationRule.scala (3)
RorKbnAuthenticationRule(33-76)RorKbnAuthenticationRule(78-85)Settings(84-84)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/RorKbnAuthorizationRule.scala (3)
Settings(73-73)RorKbnAuthorizationRule(32-65)RorKbnAuthorizationRule(67-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests WIN_IT_es717x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests WIN_IT_es818x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests WIN_IT_es92x)
- 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 (Required checks AUDIT_BUILD_CHECK)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Required checks COMPILE_CODEBASE_CHECK)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests IT_es92x)
- GitHub Check: ror (Optional checks CVE_CHECK)
- GitHub Check: ror (Required checks LICENSE_CHECK)
- GitHub Check: ror (Run all tests IT_es818x)
- GitHub Check: ror (Run all tests Unit tests)
🔇 Additional comments (23)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/ImpersonationWarningsTests.scala (1)
280-302: Test data correctly expanded for new JWT rule structure.The addition of
user_claimandgroup_ids_claimfields aligns with the new separated JWT authentication and authorization rules. This ensures the impersonation warnings test covers the explicit claim configuration introduced in the refactoring.core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthenticationRuleSettingsTests.scala (1)
262-262: Error message terminology correctly updated.The error message now uses "ror_kbn definition" instead of the previous "ROR Kibana definition" wording, maintaining consistency with the new naming conventions introduced across the codebase.
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthRuleSettingsTests.scala (1)
157-157: Error message terminology consistently updated.The error message correctly references "ror_kbn definition" in line with the project-wide terminology standardization.
core/src/main/scala/tech/beshu/ror/accesscontrol/utils/CirceOps.scala (1)
318-327: Error conversion logic well-centralized.The new
DecodingFailureUtilsobject provides a single source of truth for convertingCoreCreationErrortoDecodingFailure, while maintaining backward compatibility through theDecodingFailureOpsdelegation. This improves maintainability.core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthorizationRuleSettingsTests.scala (1)
156-156: Terminology correctly updated in error message.The error message properly references "ror_kbn definition" instead of the legacy "ROR Kibana definition" wording, maintaining consistency across the refactored codebase.
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthorizationRuleTests.scala (1)
48-300: Comprehensive test coverage for JWT authorization rule.The test suite thoroughly exercises the new
JwtAuthorizationRulewith both "or" and "and" group logic, pattern matching, and various error scenarios. The helper methods provide clear assertions on block context state (current group, available groups), ensuring the authorization logic behaves correctly.core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/base/BaseJwtRule.scala (1)
62-65: The hardcodedUser.Id(nes("jwt"))is intentional design for theNoCheckscenario. In this case, the JWT signature is not validated (using an unsecured parser), so the token is unverified. The external service call passes the raw token (token.value) as the secret for additional validation rather than trusting potentially malicious claims from an unverified token. Extracting and using the user ID from the payload would defeat the purpose of delegating validation to the external service.Likely an incorrect or invalid review comment.
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala (1)
176-359: Token/algorithm coverage (RSA/EC/NONE + external validator) is solid and nicely scoped to authn.
Good separation vs authz suite to avoid duplication.Also applies to: 579-590
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/ruleDecoders.scala (2)
131-165:lazy val optionalRuleDecoderchange looks safe and keeps control-flow intact.
131-143: JWT decoder wiring enforces required JWT definition fields via subtype filtering.
jwt_authrequires JWT definitions with bothuser_claimandgroup_ids_claim(or equivalentroles_claim/groups_claim). Tests confirm this validation is intentional and properly covered—definitions lacking either field are correctly rejected during config parsing.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/definitions/JwtDefinitionsDecoder.scala (1)
50-100: Subtype-based JwtDef decoding is a strong improvement (early config validation).
This makes the authn/authz split much harder to misconfigure at runtime.core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala (1)
43-248: Groups-logic alias coverage (roles/groups + and/or variants) is clear and thorough.Also applies to: 274-336
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scala (1)
40-67: Authz flow looks correct: early eligibility check + claim extraction + matched-groups intersection + metadata augmentation.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtAuthRulesDecoders.scala (2)
25-53: Decoder factory wiring is clean and consistent with the ROR-KBN counterpart.
36-39: Docs URL may not cover all three JWT rule types uniformly. The file contains a shared decoder for three distinct rules (jwt_authentication,jwt_authorization, andjwt_auth), each with different semantics (user_claim vs group_ids_claim requirements). Verify that the anchor#json-web-token-jwt-authand shared URL correctly document all three rule variants, or update to reference rule-specific anchors if they exist separately in the documentation.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnRulesDecoders.scala (1)
25-50: Nice symmetry with JWT decoders; should make future changes cheaper.core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthenticationRule.scala (2)
36-71: LGTM! Clean separation of authentication concerns.The rule correctly extracts the user ID from JWT claims and enriches the block context. The composition pattern with
BaseJwtRuleand thewithDisabledCallsToExternalAuthenticationServicemethod for use in composed rules aligns well with the PR objectives.
74-81: Companion object properly structured.The
Nameimplicit andSettingscase class correctly constrain the JWT definition toJwtDefForAuthentication, ensuring type safety for authentication-only scenarios.core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala (2)
27-35: Well-designed sealed trait hierarchy.The refactoring from a concrete class to a sealed trait with abstract members enables clean separation of authentication and authorization concerns while maintaining type safety.
53-78: Clean trait composition for the three use cases.The marker traits (
JwtDefForAuthentication,JwtDefForAuthorization,JwtDefForAuth) correctly partition the required fields, and the concrete case classes implement exactly what each rule type needs. This enables proper type constraints at the rule level.core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala (2)
790-808: Test helper correctly mirrors the decoder's fallback behavior.The
assertRulemethod constructs the compositeJwtAuthRulefrom separate authentication and authorization rules, defaulting togroups_any_of: ["*"]when no groups logic is specified. This matches the deprecation behavior documented inJwtLikeRulesDecoders.
313-316: Behavior change is documented appropriately.The comments (lines 313, 344, 375, 403) clearly document that
currentGroupis now added to context in scenarios where it wasn't before. This aligns with the PR objective of making rule semantics more consistent.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala (1)
148-167: Error messages are well-crafted.The
findDefinitionmethod provides actionable error messages that distinguish between "definition not found" and "definition exists but incompatible type", guiding users to documentation for resolution.
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthRule.scala
Show resolved
Hide resolved
| logger.warn( | ||
| s"""Missing groups logic settings in ${RuleName[AUTH_RULE].name.show} rule. | ||
| |For old configs, ROR treats this as `gr oups_any_of: ["*"]`. | ||
| |This syntax is deprecated. Add groups logic (https://github.com/beshu-tech/readonlyrest-docs/blob/master/details/authorization-rules-details.md#checking-groups-logic), | ||
| |or use ${RuleName[AUTHN_RULE].name.show} if you only need authentication. | ||
| |""".stripMargin | ||
| ) |
There was a problem hiding this comment.
Typo in deprecation warning message.
Line 131 has gr oups_any_of instead of groups_any_of.
- |For old configs, ROR treats this as `gr oups_any_of: ["*"]`.
+ |For old configs, ROR treats this as `groups_any_of: ["*"]`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.warn( | |
| s"""Missing groups logic settings in ${RuleName[AUTH_RULE].name.show} rule. | |
| |For old configs, ROR treats this as `gr oups_any_of: ["*"]`. | |
| |This syntax is deprecated. Add groups logic (https://github.com/beshu-tech/readonlyrest-docs/blob/master/details/authorization-rules-details.md#checking-groups-logic), | |
| |or use ${RuleName[AUTHN_RULE].name.show} if you only need authentication. | |
| |""".stripMargin | |
| ) | |
| logger.warn( | |
| s"""Missing groups logic settings in ${RuleName[AUTH_RULE].name.show} rule. | |
| |For old configs, ROR treats this as `groups_any_of: ["*"]`. | |
| |This syntax is deprecated. Add groups logic (https://github.com/beshu-tech/readonlyrest-docs/blob/master/details/authorization-rules-details.md#checking-groups-logic), | |
| |or use ${RuleName[AUTHN_RULE].name.show} if you only need authentication. | |
| |""".stripMargin | |
| ) |
🤖 Prompt for AI Agents
In
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala
around lines 129 to 135, the deprecation warning contains a typo "gr
oups_any_of" — change it to the correct token "groups_any_of" in the
interpolated string so the message reads correctly; keep the surrounding message
and stripMargin formatting unchanged.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| case GroupsLogicDecodingResult.MultipleGroupsLogicsDefined(_, fields) => | ||
| val fieldsStr = fields.map(f => s"'$f'").mkString(" or ") | ||
| Left(RulesLevelCreationError(Message( | ||
| s"Please specify either $fieldsStr for ${ruleTypePrefix}_authorization rule '$name'" | ||
| ))) |
There was a problem hiding this comment.
Error message incorrectly hardcodes "authorization" suffix.
The error at line 198 uses ${ruleTypePrefix}_authorization which is misleading when this decoder is used by AuthRuleDecoder (would say "jwt_authorization" instead of "jwt_auth").
- Left(RulesLevelCreationError(Message(
- s"Please specify either $fieldsStr for ${ruleTypePrefix}_authorization rule '$name'"
- )))
+ Left(RulesLevelCreationError(Message(
+ s"Please specify either $fieldsStr for ${RuleName[RULE].name.show} rule '$name'"
+ )))Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala
around lines 195 to 199, the error message hardcodes the "_authorization" suffix
which is incorrect for decoders like AuthRuleDecoder; change the string
interpolation to use the actual rule type suffix/name variable used elsewhere
(e.g. s"${ruleTypePrefix}_$ruleTypeSuffix" or the existing full rule-type
variable) instead of "_authorization" so the message matches the decoder's real
rule name and remains consistent.
There was a problem hiding this comment.
@coutoPL, CodeRabbit's observation is correct. The error message at line 198 hardcodes ${ruleTypePrefix}_authorization, which produces incorrect messages when this generic method is used by AuthRuleDecoder.
For example:
- When decoding
jwt_authwith multiple groups logic, the error would say "jwt_authorization" instead of "jwt_auth" - Same issue for
ror_kbn_auth
The suggested fix using ${RuleName[RULE].name.show} is the right approach because:
- It dynamically uses the actual rule type being decoded
- It matches the existing pattern already used at line 130 in this file
- It ensures error messages are accurate for all three decoder types (Authentication, Authorization, and Auth)
The fix should be applied as suggested.
🧠 Learnings used
Learnt from: mgoworko
Repo: sscarduzio/elasticsearch-readonlyrest-plugin PR: 1163
File: core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala:96-98
Timestamp: 2025-10-03T21:07:20.002Z
Learning: In the ROR KBN authentication refactoring (PR #1163), the VariableUsage implicit for rorKbnAuthRule uses a union type `VariableUsage[RorKbnAuthRule | RorKbnAuthenticationRule]` because the decoder explicitly uses this union type to represent the fallback behavior where `ror_kbn_auth` without groups falls back to `RorKbnAuthenticationRule`. A separate `VariableUsage[RorKbnAuthRule]` implicit is not needed since there are no direct usages of that specific type - all usage goes through the union type in the decoder.
...ain/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/RorKbnRulesDecoders.scala
Show resolved
Hide resolved
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala
Show resolved
Hide resolved
...tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala
Show resolved
Hide resolved
.../tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
...est/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthRuleSettingsTests.scala
Show resolved
Hide resolved
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala
Show resolved
Hide resolved
core/src/test/scala/tech/beshu/ror/unit/acl/blocks/rules/auth/JwtAuthRuleTests.scala
Show resolved
Hide resolved
...h/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthenticationRuleSettingsTests.scala
Outdated
Show resolved
Hide resolved
|
|
||
| // Pseudo-authorization rule should be used exclusively as part of the JwtAuthRule, when there are is no groups logic defined. | ||
| // It preserves the kbn_auth rule behavior from before introducing separate authn and authz rules. | ||
| final class JwtPseudoAuthorizationRule(val settings: Settings) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
...tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # 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/RorKbnRulesDecoders.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/RorKbnAuthRuleSettingsTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/RorKbnAuthorizationRuleSettingsTests.scala # tests-utils/build.gradle
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala (1)
233-261: UnusedSystem.setPropertycall remains.Line 235 sets
System.setProperty("SECRET_KEY", ...)but the YAML at line 251 uses@{env:SECRET_RSA}, which is resolved by theenvVarsProvideroverride (lines 579-585). TheSECRET_KEYproperty is never used.Remove line 235 to eliminate this dead code.
🔎 Proposed fix
- val pkey = KeyPairGenerator.getInstance("RSA").generateKeyPair().getPublic - System.setProperty("SECRET_KEY", Base64.getEncoder.encodeToString(pkey.getEncoded)) assertDecodingSuccess(core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala (1)
249-273: Test has malformed YAML and unclear intent.This test uses
jwt:underaccess_control_rules(line 258) instead ofjwt_authorization:, and- name jwt1(line 259) is missing a colon. The malformed YAML likely short-circuits decoding before reaching the "no JWT definition name" validation path that the test name suggests.To properly test "no JWT definition name is defined", the rule configuration should be valid but the JWT definition should be missing its
namefield.🔎 Proposed fix
"no JWT definition name is defined" in { assertDecodingFailure( yaml = """ |readonlyrest: | | access_control_rules: | | - name: test_block1 - | jwt: - | - name jwt1 + | jwt_authorization: + | name: "jwt1" + | groups: ["group1"] | | jwt: - | - signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" + | - group_ids_claim: groups + | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | |""".stripMargin, assertion = errors => { errors should have size 1 errors.head should be(DefinitionsLevelCreationError(MalformedValue.fromString( - """- signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" + """- group_ids_claim: "groups" + | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" |""".stripMargin ))) } ) }
🧹 Nitpick comments (1)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala (1)
340-346:envVarsProvideroverride appears unused.This suite's tests all use HMAC signature keys directly. No tests reference
SECRET_RSAor use RSA/EC algorithms, making this override unnecessary. It was likely copied fromJwtAuthenticationRuleSettingsTestsfor consistency.Consider removing it to reduce test noise, or retain it if you anticipate adding RSA-based tests to this suite in the future.
🔎 Proposed cleanup
- - override implicit protected def envVarsProvider: EnvVarsProvider = { - case EnvVarName(env) if env.value == "SECRET_RSA" => - val pkey = KeyPairGenerator.getInstance("RSA").generateKeyPair().getPublic - Some(Base64.getEncoder.encodeToString(pkey.getEncoded)) - case _ => - None - } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala(1 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/rules/auth/JwtLikeRulesDecoders.scala
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mgoworko
Repo: sscarduzio/elasticsearch-readonlyrest-plugin PR: 1163
File: core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala:96-98
Timestamp: 2025-10-03T21:07:20.002Z
Learning: In the ROR KBN authentication refactoring (PR #1163), the VariableUsage implicit for rorKbnAuthRule uses a union type `VariableUsage[RorKbnAuthRule | RorKbnAuthenticationRule]` because the decoder explicitly uses this union type to represent the fallback behavior where `ror_kbn_auth` without groups falls back to `RorKbnAuthenticationRule`. A separate `VariableUsage[RorKbnAuthRule]` implicit is not needed since there are no direct usages of that specific type - all usage goes through the union type in the decoder.
📚 Learning: 2025-10-03T21:07:20.002Z
Learnt from: mgoworko
Repo: sscarduzio/elasticsearch-readonlyrest-plugin PR: 1163
File: core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/variables/runtime/VariableContext.scala:96-98
Timestamp: 2025-10-03T21:07:20.002Z
Learning: In the ROR KBN authentication refactoring (PR #1163), the VariableUsage implicit for rorKbnAuthRule uses a union type `VariableUsage[RorKbnAuthRule | RorKbnAuthenticationRule]` because the decoder explicitly uses this union type to represent the fallback behavior where `ror_kbn_auth` without groups falls back to `RorKbnAuthenticationRule`. A separate `VariableUsage[RorKbnAuthRule]` implicit is not needed since there are no direct usages of that specific type - all usage goes through the union type in the decoder.
Applied to files:
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala
📚 Learning: 2025-09-21T14:09:30.387Z
Learnt from: mgoworko
Repo: sscarduzio/elasticsearch-readonlyrest-plugin PR: 1163
File: core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/base/RorKbnRuleHelper.scala:168-172
Timestamp: 2025-09-21T14:09:30.387Z
Learning: RorKbnDef.SignatureCheckMethod sealed trait has only three cases: Hmac(key: Array[Byte]), Rsa(pubKey: PublicKey), and Ec(pubKey: PublicKey). There is no NoCheck case.
Applied to files:
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala
🧬 Code graph analysis (2)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala (4)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala (3)
JwtDef(37-51)SignatureCheckMethod(41-46)Name(38-38)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/HttpClientsFactory.scala (2)
HttpClientsFactory(49-81)HttpClient(52-65)core/src/test/scala/tech/beshu/ror/mocks/MockHttpClientsFactory.scala (1)
MockHttpClientsFactoryWithFixedHttpClient(33-36)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthRuleSettingsTests.scala (1)
envVarsProvider(1092-1098)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala (5)
core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/rules/auth/JwtAuthorizationRule.scala (4)
rules(40-49)JwtAuthorizationRule(33-79)JwtAuthorizationRule(81-88)Name(83-85)core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/definitions/JwtDefForAuthRule.scala (4)
JwtDef(37-51)GroupsConfig(48-48)SignatureCheckMethod(41-46)Name(38-38)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/userAndGroups.scala (4)
GroupIdLike(80-121)GroupsLogic(154-261)AnyOf(172-172)AllOf(174-174)core/src/main/scala/tech/beshu/ror/accesscontrol/domain/http.scala (1)
AuthorizationTokenDef(276-277)core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala (1)
envVarsProvider(579-585)
🪛 Gitleaks (8.30.0)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala
[high] 55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 89-89: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ror (Warm up Gradle cache Download Gradle and toolchains)
🔇 Additional comments (5)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthenticationRuleSettingsTests.scala (3)
43-43: Test coverage appears comprehensive.The test suite covers the essential scenarios for JWT authentication rule loading: simplified/extended rule formats, various signature algorithms (HMAC, RSA, EC, NONE), environment variable resolution, and negative cases for malformed/missing configurations.
360-576: Comprehensive negative test coverage.The error case tests thoroughly validate:
- Missing or unknown JWT definitions
- Algorithm/signature key validation
- Environment variable resolution failures
- Appropriate error messages and types
587-590: Appropriate mocking for external validator tests.The mocked HTTP client factory correctly isolates external dependencies when testing NONE signature scenarios (lines 290-358).
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala (2)
43-112: Authorization-specific test coverage is appropriate.The positive tests correctly focus on groups logic (OR/AND) and authorization concerns, avoiding duplication of signature/algorithm tests from the authentication suite. The structure mirrors RorKbnAuthorizationRuleSettingsTests as noted in past reviews.
307-335: Replacegroups_anywith a valid groups logic key on line 318.The key
groups_anyis not valid. Usegroupsorgroups_orfor OR logic (orgroups_andfor AND logic) instead. If the invalid key prevents rule validation, the test may fail before reaching the duplicate JWT definition check it's meant to verify.
| "group names claim can be enabled in JWT definition" in { | ||
| assertDecodingSuccess( | ||
| yaml = | ||
| s""" | ||
| |readonlyrest: | ||
| | | ||
| | access_control_rules: | ||
| | | ||
| | - name: test_block1 | ||
| | jwt_authentication: jwt1 | ||
| | | ||
| | jwt: | ||
| | | ||
| | - name: jwt1 | ||
| | user_claim: user | ||
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | ||
| | | ||
| |""".stripMargin, | ||
| assertion = rule => { | ||
| rule.settings.jwt.id should be(JwtDef.Name("jwt1")) | ||
| rule.settings.jwt.authorizationTokenDef should be(AuthorizationTokenDef(Header.Name.authorization, "Bearer ")) | ||
| rule.settings.jwt.checkMethod shouldBe a[SignatureCheckMethod.Hmac] | ||
| rule.settings.jwt.userClaim should be(domain.Jwt.ClaimName(jsonPathFrom("user"))) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Test name and content mismatch.
The test is named "group names claim can be enabled in JWT definition" but the YAML configuration (lines 137-139) contains no group-related claims, and the assertions (lines 143-146) only verify userClaim. This test appears to be a duplicate of the previous test with a misleading name.
For JwtAuthenticationRule, group claims are not required and should not be the focus. Consider either removing this test as redundant or renaming it to match what it actually tests.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| "groups claim can be enabled in JWT definition and is a http address" in { | ||
| assertDecodingSuccess( | ||
| yaml = | ||
| """ | ||
| |readonlyrest: | ||
| | | ||
| | access_control_rules: | ||
| | | ||
| | - name: test_block1 | ||
| | jwt_authentication: jwt1 | ||
| | | ||
| | jwt: | ||
| | | ||
| | - name: jwt1 | ||
| | user_claim: user | ||
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | ||
| | | ||
| |""".stripMargin, | ||
| assertion = rule => { | ||
| rule.settings.jwt.id should be(JwtDef.Name("jwt1")) | ||
| rule.settings.jwt.authorizationTokenDef should be(AuthorizationTokenDef(Header.Name.authorization, "Bearer ")) | ||
| rule.settings.jwt.checkMethod shouldBe a [SignatureCheckMethod.Hmac] | ||
| rule.settings.jwt.userClaim should be(domain.Jwt.ClaimName(jsonPathFrom("user"))) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Another misleading test name.
The test claims "groups claim can be enabled in JWT definition and is a http address", but:
- The YAML (lines 163-165) contains no group claims
- There's no HTTP address in the configuration
- Assertions (lines 169-172) only verify basic
userClaim
This appears to be another copy-paste artifact. Consider removing this redundant test or clarifying what scenario it's meant to cover.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| "both 'groups or' key and 'groups and' key used" in { | ||
| List( | ||
| ("roles", "roles_and"), | ||
| ("groups", "groups_and") | ||
| ) | ||
| .foreach { case (groupsAnyOfKey, groupsAllOfKey) => | ||
| assertDecodingFailure( | ||
| yaml = | ||
| s""" | ||
| |readonlyrest: | ||
| | | ||
| | access_control_rules: | ||
| | | ||
| | - name: test_block1 | ||
| | jwt_authorization: | ||
| | name: "jwt1" | ||
| | $groupsAnyOfKey: ["group1", "group2"] | ||
| | $groupsAllOfKey: ["groups1", "groups2"] | ||
| | jwt: | ||
| | - name: jwt2 | ||
| | group_ids_claim: groups | ||
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | ||
| | | ||
| |""".stripMargin, | ||
| assertion = errors => { | ||
| errors should have size 1 | ||
| errors.head should be(RulesLevelCreationError(Message( | ||
| s"Please specify either '$groupsAnyOfKey' or '$groupsAllOfKey' for jwt_authorization rule 'jwt1'") | ||
| )) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Test may catch only one of two issues.
Lines 288-295 reference JWT definition jwt1 but only define jwt2. The test expects an error about mutual exclusivity of groups_or/groups_and (lines 300-302), but the configuration also has a missing definition issue that might be caught first depending on validation order.
This makes the test fragile - if validation order changes, the test might fail unexpectedly. Consider either:
- Fixing the JWT definition name to match (
jwt1instead ofjwt2at line 293) - Splitting into two separate tests for clarity
🔎 Proposed fix - align JWT definition name
| jwt:
- | - name: jwt2
+ | - name: jwt1
| group_ids_claim: groups
| signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "both 'groups or' key and 'groups and' key used" in { | |
| List( | |
| ("roles", "roles_and"), | |
| ("groups", "groups_and") | |
| ) | |
| .foreach { case (groupsAnyOfKey, groupsAllOfKey) => | |
| assertDecodingFailure( | |
| yaml = | |
| s""" | |
| |readonlyrest: | |
| | | |
| | access_control_rules: | |
| | | |
| | - name: test_block1 | |
| | jwt_authorization: | |
| | name: "jwt1" | |
| | $groupsAnyOfKey: ["group1", "group2"] | |
| | $groupsAllOfKey: ["groups1", "groups2"] | |
| | jwt: | |
| | - name: jwt2 | |
| | group_ids_claim: groups | |
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | |
| | | |
| |""".stripMargin, | |
| assertion = errors => { | |
| errors should have size 1 | |
| errors.head should be(RulesLevelCreationError(Message( | |
| s"Please specify either '$groupsAnyOfKey' or '$groupsAllOfKey' for jwt_authorization rule 'jwt1'") | |
| )) | |
| } | |
| ) | |
| } | |
| "both 'groups or' key and 'groups and' key used" in { | |
| List( | |
| ("roles", "roles_and"), | |
| ("groups", "groups_and") | |
| ) | |
| .foreach { case (groupsAnyOfKey, groupsAllOfKey) => | |
| assertDecodingFailure( | |
| yaml = | |
| s""" | |
| |readonlyrest: | |
| | | |
| | access_control_rules: | |
| | | |
| | - name: test_block1 | |
| | jwt_authorization: | |
| | name: "jwt1" | |
| | $groupsAnyOfKey: ["group1", "group2"] | |
| | $groupsAllOfKey: ["groups1", "groups2"] | |
| | jwt: | |
| | - name: jwt1 | |
| | group_ids_claim: groups | |
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | |
| | | |
| |""".stripMargin, | |
| assertion = errors => { | |
| errors should have size 1 | |
| errors.head should be(RulesLevelCreationError(Message( | |
| s"Please specify either '$groupsAnyOfKey' or '$groupsAllOfKey' for jwt_authorization rule 'jwt1'") | |
| )) | |
| } | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
core/src/test/scala/tech/beshu/ror/unit/acl/factory/decoders/rules/auth/JwtAuthorizationRuleSettingsTests.scala
around lines 274-305 the test YAML references a jwt named "jwt1" in the
jwt_authorization block but the JWT list defines "jwt2", which can cause a
missing-definition error to be raised before the mutual-exclusivity check and
make the test fragile; fix by aligning the JWT definition name to "jwt1" (change
the JWT entry name from jwt2 to jwt1) or alternatively split into two focused
tests (one asserting mutual-exclusivity of groups keys and another asserting
missing JWT definition) so validation order cannot cause spurious failures.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| payload: Jwt.Payload) = { | ||
| val result = payload.claims.userIdClaim(settings.jwt.userClaim) | ||
| logClaimSearchResults(blockContext, result) | ||
| (result match { |
There was a problem hiding this comment.
Can we do it like this?
result match {
case Found(userId) =>
Right(blockContext.withUserMetadata(
_.withLoggedUser(DirectlyLoggedUser(userId))
.withJwtToken(payload)
))
case NotFound =>
Left(())
}
| nonEmptyGroups <- UniqueNonEmptyList.from(groups) | ||
| matchedGroups <- groupsLogic.availableGroupsFrom(nonEmptyGroups) | ||
| if blockContext.isCurrentGroupEligible(GroupIds.from(matchedGroups)) | ||
| } yield blockContext.withUserMetadata(_.addAvailableGroups(matchedGroups))).toRight(()) |
There was a problem hiding this comment.
it seems, it'd be good to add withJwtToken also here
| B <: BlockContext, JWT_DEF <: JwtDef | ||
| ](blockContext: B, | ||
| jwt: JWT_DEF, | ||
| disabledCallsToExternalAuthenticationService: Boolean = false) |
There was a problem hiding this comment.
I would remove the call handling from this method.
BTW, I totally forgot about this undocumented feature ;) We have to obviously to call it in each JWT auth-related rule.
But let's do it in a different way.
Maybe we can modify the auth traits like that:
trait AuthenticationRule extends Rule {
this: AuthenticationImpersonationSupport =>
def eligibleUsers: AuthenticationRule.EligibleUsersSupport
implicit def userIdCaseSensitivity: CaseSensitivity
override def check[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[Rule.RuleResult[B]] = {
authenticate(blockContext)
.flatMap {
case Fulfilled(blockContext) => postAuthenticateAction(blockContext)
case r@Rejected(_) => Task.now(r)
}
}
protected [rules] def authenticate[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[Rule.RuleResult[B]]
protected def postAuthenticateAction[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] =
Task.now(Fulfilled(blockContext))
}
object AuthenticationRule {
sealed trait EligibleUsersSupport
object EligibleUsersSupport {
final case class Available(users: Set[User.Id]) extends EligibleUsersSupport
case object NotAvailable extends EligibleUsersSupport
}
}
trait AuthorizationRule extends Rule {
this: AuthorizationImpersonationSupport =>
override def check[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] = {
authorize(blockContext)
.flatMap {
case Fulfilled(blockContext) => postAuthorizeAction(blockContext)
case r@Rejected(_) => Task.now(r)
}
}
protected [rules] def authorize[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]]
protected def postAuthorizeAction[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] =
Task.now(Fulfilled(blockContext))
}
trait AuthRule extends AuthenticationRule with AuthorizationRule {
this: AuthenticationImpersonationSupport with AuthorizationImpersonationSupport =>
override def check[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] = {
authenticate(blockContext)
.flatMap {
case Fulfilled(newBlockContext) =>
authorize(newBlockContext)
case rejected@Rejected(_) =>
Task.now(rejected)
}
.flatMap {
case Fulfilled(blockContext) => postAuthAction(blockContext)
case r@Rejected(_) => Task.now(r)
}
}
protected def postAuthAction[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] =
Task.now(Fulfilled(blockContext))
}
and then BaseComposedAuthenticationAndAuthorizationRule would look like that:
private[auth] abstract class BaseComposedAuthenticationAndAuthorizationRule(authenticationRule: AuthenticationRule,
authorizationRule: AuthorizationRule)
extends AuthRule
with AuthenticationImpersonationCustomSupport
with AuthorizationImpersonationCustomSupport {
override protected [rules] def authenticate[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] =
authenticationRule.authenticate(blockContext)
override protected [rules] def authorize[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[RuleResult[B]] =
authorizationRule.authorize(blockContext)
}
then we will provide a post action handling in each JWT-related rule. E.g.:
final class JwtAuthRule(val authentication: JwtAuthenticationRule,
val authorization: JwtAuthorizationRule)
extends BaseComposedAuthenticationAndAuthorizationRule(authentication, authorization) {
override val name: Rule.Name = JwtAuthRule.Name.name
override val eligibleUsers: EligibleUsersSupport = EligibleUsersSupport.NotAvailable
override val userIdCaseSensitivity: CaseSensitivity = authentication.userIdCaseSensitivity
override protected def postAuthAction[B <: BlockContext : BlockContextUpdater](blockContext: B): Task[Rule.RuleResult[B]] = {
// if nocheck then call external validation service
???
}
}
WDYT?
| logger.debug(s"[${blockContext.requestContext.id.show}] Authorization header '${jwt.authorizationTokenDef.headerName.show}' is missing or does not contain a JWT token") | ||
| Task.now(Rejected()) | ||
| case Some(token) => | ||
| implicit val requestId: RequestId = blockContext.requestContext.id.toRequestId |
There was a problem hiding this comment.
let's try to refactor it to make it more readable
|
|
||
| private def claimsFrom[JWT_DEF <: JwtDef](token: Jwt.Token) | ||
| (implicit requestId: RequestId, | ||
| jwt: JWT_DEF) = { |
There was a problem hiding this comment.
I'm not sure if JWT_DEF should be passed as implicit.
| class JwtAuthorizationRuleTests | ||
| extends AnyWordSpec with MockFactory with Inside with BlockContextAssertion with WithDummyRequestIdSupport { | ||
|
|
||
| "A JwtAuthorizationRule" should { |
There was a problem hiding this comment.
For me, it's hard to tell if we tested it well or not (I think the same applies to RorKbnAuthorizationRuleTests).
- The tests focus on group logic (not fully, because we use
orandand). Every authz rule contains and uses GroupsLogic. We have tested it (don't we?). So, maybe we don't have to focus on it. - We could extract JWT token parsing to a separate class and test it separately.
Then we could focus only on the important things like:
- proper handling current groups
- proper handling JWT with:
a) no groups
b) groups with intersection
b) groups without intersection - non-parsable token?
- proper post-action handling
| "group names claim can be enabled in JWT definition" in { | ||
| assertDecodingSuccess( | ||
| yaml = | ||
| s""" | ||
| |readonlyrest: | ||
| | | ||
| | access_control_rules: | ||
| | | ||
| | - name: test_block1 | ||
| | jwt_authentication: jwt1 | ||
| | | ||
| | jwt: | ||
| | | ||
| | - name: jwt1 | ||
| | user_claim: user | ||
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | ||
| | | ||
| |""".stripMargin, | ||
| assertion = rule => { | ||
| rule.settings.jwt.id should be(JwtDef.Name("jwt1")) | ||
| rule.settings.jwt.authorizationTokenDef should be(AuthorizationTokenDef(Header.Name.authorization, "Bearer ")) | ||
| rule.settings.jwt.checkMethod shouldBe a[SignatureCheckMethod.Hmac] | ||
| rule.settings.jwt.userClaim should be(domain.Jwt.ClaimName(jsonPathFrom("user"))) | ||
| } | ||
| ) | ||
| } |
| "groups claim can be enabled in JWT definition and is a http address" in { | ||
| assertDecodingSuccess( | ||
| yaml = | ||
| """ | ||
| |readonlyrest: | ||
| | | ||
| | access_control_rules: | ||
| | | ||
| | - name: test_block1 | ||
| | jwt_authentication: jwt1 | ||
| | | ||
| | jwt: | ||
| | | ||
| | - name: jwt1 | ||
| | user_claim: user | ||
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | ||
| | | ||
| |""".stripMargin, | ||
| assertion = rule => { | ||
| rule.settings.jwt.id should be(JwtDef.Name("jwt1")) | ||
| rule.settings.jwt.authorizationTokenDef should be(AuthorizationTokenDef(Header.Name.authorization, "Bearer ")) | ||
| rule.settings.jwt.checkMethod shouldBe a [SignatureCheckMethod.Hmac] | ||
| rule.settings.jwt.userClaim should be(domain.Jwt.ClaimName(jsonPathFrom("user"))) | ||
| } | ||
| ) | ||
| } |
| "both 'groups or' key and 'groups and' key used" in { | ||
| List( | ||
| ("roles", "roles_and"), | ||
| ("groups", "groups_and") | ||
| ) | ||
| .foreach { case (groupsAnyOfKey, groupsAllOfKey) => | ||
| assertDecodingFailure( | ||
| yaml = | ||
| s""" | ||
| |readonlyrest: | ||
| | | ||
| | access_control_rules: | ||
| | | ||
| | - name: test_block1 | ||
| | jwt_authorization: | ||
| | name: "jwt1" | ||
| | $groupsAnyOfKey: ["group1", "group2"] | ||
| | $groupsAllOfKey: ["groups1", "groups2"] | ||
| | jwt: | ||
| | - name: jwt2 | ||
| | group_ids_claim: groups | ||
| | signature_key: "123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456.123456" | ||
| | | ||
| |""".stripMargin, | ||
| assertion = errors => { | ||
| errors should have size 1 | ||
| errors.head should be(RulesLevelCreationError(Message( | ||
| s"Please specify either '$groupsAnyOfKey' or '$groupsAllOfKey' for jwt_authorization rule 'jwt1'") | ||
| )) | ||
| } | ||
| ) | ||
| } |
| assertion = errors => { | ||
| errors should have size 1 | ||
| errors.head should be(RulesLevelCreationError(Message("Cannot find ROR Kibana definition with name: kbn1"))) | ||
| errors.head should be(RulesLevelCreationError(Message("Cannot find ror_kbn definition with name: kbn1"))) |
There was a problem hiding this comment.
let's add ticks:
Cannot find `ror_kbn` definition with name: kbn1
2 participants
🚀Added separate JWT authentication and authorization rules
Summary by CodeRabbit
New Features
Refactor
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.