Regex operator for JWT role extraction #483
Conversation
WalkthroughRefactors JwtRolesResolver to pass the full JwtRoleRule to its operator evaluator, consolidates operator dispatch and negation there, and adds a new MATCH operator with regex validation and a compiled_pattern on JwtRoleRule. Unit tests updated to exercise MATCH semantics and related validations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Resolver as JwtRolesResolver
participant Rule as JwtRoleRule
participant Claims as JWT Claims
Caller->>Resolver: evaluate_role_rules(rules, token)
Resolver->>Claims: extract values via JSONPath
loop for each extracted match
Resolver->>Resolver: _evaluate_operator(rule, match)
alt rule.operator == MATCH
Resolver->>Rule: access compiled_regex
alt compiled_regex exists
Note right of Resolver #DFF2E1: iterate or test single value
Resolver->>Resolver: apply compiled_regex.match (any true => result=true)
else
Resolver->>Resolver: result = False
end
else Other operators
Resolver->>Resolver: evaluate EQUALS / CONTAINS / IN semantics
end
alt rule.negate
Resolver->>Resolver: result = not result
end
end
Resolver-->>Caller: final boolean decision
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Draft because ran into some issues with some recent changes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/authorization/resolvers.py(2 hunks)src/models/config.py(4 hunks)tests/unit/authorization/test_resolvers.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/authorization/test_resolvers.py (2)
src/models/config.py (3)
JwtRoleRule(241-299)JsonPathOperator(232-238)compiled_regex(295-299)src/authorization/resolvers.py (4)
JwtRolesResolver(53-122)resolve_roles(29-30)resolve_roles(36-39)resolve_roles(60-67)
src/authorization/resolvers.py (1)
src/models/config.py (3)
JwtRoleRule(241-299)JsonPathOperator(232-238)compiled_regex(295-299)
🪛 GitHub Actions: Unit tests
tests/unit/authorization/test_resolvers.py
[error] 141-144: pytest step failed: tests/unit/authorization/test_resolvers.py::TestJwtRolesResolver::test_resolve_roles_match_operator_non_string_pattern - expected ValueError with message containing 'MATCH operator requires a string regex pattern', but got ValidationError: 'MATCH operator requires a string pattern, int'.
🪛 GitHub Actions: Pyright
src/authorization/resolvers.py
[error] 115-115: Pyright error during 'uv run pyright src': Cannot access attribute 'search' for class 'MethodType'. Attribute 'search' is unknown (reportAttributeAccessIssue).
⏰ 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: build-pr
🔇 Additional comments (11)
src/models/config.py (2)
4-6: Imports for regex support — LGTM.Also applies to: 15-15
238-239: New JsonPathOperator.MATCH — LGTM.src/authorization/resolvers.py (2)
74-77: Passing the full rule into the evaluator — LGTM.
119-122: Negation after evaluation — LGTM.tests/unit/authorization/test_resolvers.py (7)
5-8: Test imports — LGTM.
81-103: MATCH positive path — LGTM.
104-126: MATCH negative path — LGTM.
127-138: Invalid regex validation — LGTM.
151-172: Non-string match value — LGTM.
173-194: Use correct commands to verify compiled_regex implementation
EnsureJwtRoleRule.compiled_regexis a property returningPattern[str]so theisinstance(..., re.Pattern)assertions pass, then run:pyright src pytest -q tests/unit/authorization/test_resolvers.py::TestJwtRolesResolver
139-150: Non-string pattern expectation — message updated to “MATCH operator requires a string regex pattern”. After applying the config.py change, re-run:pytest -q tests/unit/authorization/test_resolvers.py::TestJwtRolesResolver::test_resolve_roles_match_operator_non_string_pattern
When we originally added the JWT role extraction operators, we made some basic ones which thought would be enough, but we found that there's too much complexity and variance in the various fields we find in Red Hat SSO tokens, so we realized we're going to need a regex operator to handle more complex matching scenarios This commit simply adds a new `MATCH` operator to the `JsonPathOperator` enum, and implements the logic to handle it in the `JwtRolesResolver`. `_evaluate_operator` had to be modified to accept the entire `JwtRoleRule` so that it can access the pre-compiled regex pattern for better performance.
|
Undrafted |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/authorization/resolvers.py (1)
100-111: Fix list-handling semantics for EQUALS/IN; keep CONTAINS robust.
matchis a list of JSONPath values; current EQUALS compares list-to-scalar and IN checks list-in-container. Evaluate per-item; also make CONTAINS support substring checks for string items. (Re-raising a prior point.)Apply:
- rule: JwtRoleRule, match: Any + rule: JwtRoleRule, matches: Any ) -> bool: # pylint: disable=too-many-branches """Evaluate an operator against a match and rule.""" - result = False - match rule.operator: + result = False + items = matches if isinstance(matches, list) else [matches] + match rule.operator: case JsonPathOperator.EQUALS: - result = match == rule.value + # Any JSONPath value equals the rule value + result = any(item == rule.value for item in items) case JsonPathOperator.CONTAINS: - result = rule.value in match + # List membership or substring containment (strings only) + result = rule.value in items + if isinstance(rule.value, str): + result = result or any( + isinstance(item, str) and rule.value in item for item in items + ) case JsonPathOperator.IN: - result = match in rule.value + # Any JSONPath value is contained in the rule's container + result = hasattr(rule.value, "__contains__") and any( + item in rule.value for item in items + )
🧹 Nitpick comments (2)
src/authorization/resolvers.py (2)
111-118: Quiet Pyright error and make MATCH branch type-safe; also iterate items consistently.Cast the cached property and reuse per-item iteration.
Apply:
- case JsonPathOperator.MATCH: - # Use the pre-compiled regex pattern for better performance - if rule.compiled_regex is not None: - result = any( - isinstance(item, str) and bool(rule.compiled_regex.search(item)) - for item in match - ) + case JsonPathOperator.MATCH: + # Use the pre-compiled regex pattern for better performance + pattern = cast(Optional[Pattern[str]], getattr(rule, "compiled_regex", None)) + if pattern is not None: + result = any( + isinstance(item, str) and bool(pattern.search(item)) + for item in (matches if isinstance(matches, list) else [matches]) + )Add to imports at the top of the file:
from typing import Any, Optional, Pattern, castAlso consider ReDoS hardening for untrusted patterns: prefer RE2 (e.g., via
re2/regexwith timeouts) or require anchored, length-bounded patterns for MATCH.
119-120: Negation after a non-applicable operator could grant roles on misconfig.If MATCH is misconfigured and produces no decision, negation flips False→True. Consider short-circuiting to False on invalid operator config (log and return) before applying negation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/authorization/resolvers.py(2 hunks)src/models/config.py(3 hunks)tests/unit/authorization/test_resolvers.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/models/config.py
- tests/unit/authorization/test_resolvers.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/authorization/resolvers.py (1)
src/models/config.py (3)
JwtRoleRule(241-299)JsonPathOperator(232-238)compiled_regex(295-299)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
src/authorization/resolvers.py (1)
73-77: Passing the full rule into _evaluate_operator is a good change.This simplifies dispatch and enables reuse of precompiled regex.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
the new match rules looks nice!
Bump to lightspeed-core/lightspeed-stack#483 Also include changes from rh-ecosystem-edge#171 so we only have one bump PR
2 participants
Description
When we originally added the JWT role extraction operators, we made some basic ones which thought would be enough, but we found that there's too much complexity and variance in the various fields we find in Red Hat SSO tokens, so we realized we're going to need a regex operator to handle more complex matching scenarios
This commit simply adds a new
MATCHoperator to theJsonPathOperatorenum, and implements the logic to handle it in theJwtRolesResolver._evaluate_operatorhad to be modified to accept the entireJwtRoleRuleso that it can access the pre-compiled regex pattern for better performance.Type of change
Related Tickets & Documents
MGMT-21654
Checklist before requesting a review
Testing
Tested manually, wrote unit-tests
Summary by CodeRabbit