Improve middleware and resolvers coverage #650
Conversation
The coverage for the resolvers module and the authz middleware module was a bit low. This commit adds tests to improve that. Also fixes a minor issue in the `JwtRolesResolver` where it would raise an error if the JWT has no claims, which shouldn't be an problem.
WalkthroughAdjusted claim handling in src/authorization/resolvers.py to return empty JWT claims without raising an error. Added extensive unit tests for authorization middleware and resolvers, covering resolver selection, authorization checks, role resolution behaviors, operators, admin and multi-role scenarios, and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API Endpoint
participant Middleware as Authorization Middleware
participant Resolver as Roles/Access Resolvers
participant Claims as _get_claims
Client->>API: Request
API->>Middleware: authorize(...)
Middleware->>Claims: Extract JWT claims
note right of Claims: Change: empty claims return {} (no error)
Claims-->>Middleware: claims ({} or populated)
alt Claims present or {}
Middleware->>Resolver: resolve roles/actions from claims
Resolver-->>Middleware: roles/actions (may be empty)
alt Access allowed
Middleware-->>API: proceed (state updated if actions)
API-->>Client: Response
else Access denied
Middleware-->>Client: HTTP 403
end
else Missing auth arg
Middleware-->>Client: HTTP 500
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/authorization/test_resolvers.py (1)
26-28: Add type annotations to the helper function.The coding guidelines require complete type annotations for all function parameters and return types. This helper function should specify types for clarity and type-checking benefits.
Apply this diff:
-def claims_to_auth_tuple(claims: dict) -> tuple: +def claims_to_auth_tuple(claims: dict) -> tuple[str, str, bool, str]: """Convert JWT claims dictionary to an auth tuple.""" return ("user", "token", False, claims_to_token(claims))As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/authorization/resolvers.py(0 hunks)tests/unit/authorization/test_middleware.py(1 hunks)tests/unit/authorization/test_resolvers.py(5 hunks)
💤 Files with no reviewable changes (1)
- src/authorization/resolvers.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/authorization/test_middleware.pytests/unit/authorization/test_resolvers.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/authorization/test_middleware.pytests/unit/authorization/test_resolvers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/authorization/test_middleware.pytests/unit/authorization/test_resolvers.py
🧬 Code graph analysis (2)
tests/unit/authorization/test_middleware.py (4)
src/authorization/middleware.py (3)
get_authorization_resolvers(25-65)_perform_authorization_check(68-108)authorize(111-122)src/authorization/resolvers.py (13)
NoopRolesResolver(33-39)NoopAccessResolver(131-143)JwtRolesResolver(53-116)GenericAccessResolver(146-202)resolve_roles(29-30)resolve_roles(36-39)resolve_roles(60-67)check_access(123-124)check_access(134-138)check_access(171-188)get_actions(127-128)get_actions(140-143)get_actions(190-202)src/models/config.py (6)
config(139-145)Action(328-374)JwtRoleRule(267-325)AccessRule(377-381)JsonPathOperator(258-264)jwk_configuration(437-445)src/configuration.py (2)
authorization_configuration(108-116)authentication_configuration(100-105)
tests/unit/authorization/test_resolvers.py (2)
src/authorization/resolvers.py (11)
JwtRolesResolver(53-116)GenericAccessResolver(146-202)resolve_roles(29-30)resolve_roles(36-39)resolve_roles(60-67)check_access(123-124)check_access(134-138)check_access(171-188)get_actions(127-128)get_actions(140-143)get_actions(190-202)src/models/config.py (5)
config(139-145)JwtRoleRule(267-325)AccessRule(377-381)JsonPathOperator(258-264)Action(328-374)
⏰ 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 (ci)
🔇 Additional comments (8)
tests/unit/authorization/test_middleware.py (4)
1-20: LGTM! Module setup is clean and complete.The module docstring is present and descriptive. All necessary imports are included, and the file correctly uses pytest as required by the coding guidelines.
28-151: Excellent test coverage for resolver selection logic.The test class comprehensively covers:
- Resolver selection for different auth modules (noop, K8s, JWK)
- Fallback behavior when JWK rules are empty
- Proper resolver instantiation when rules are present
- Error handling for unknown auth modules
The use of
get_authorization_resolvers.cache_clear()at lines 91, 121, and 142 is good practice to ensure test isolation when the function being tested is cached.
153-232: LGTM! Comprehensive testing of authorization check logic.The tests effectively verify:
- Error handling when the
authkeyword argument is missing (HTTP 500)- Access denial raising HTTP 403 with appropriate detail message
- Correct propagation of
authorized_actionstorequest.statein various scenarios (kwargs, args, or absent)- The wildcard "*" role is always included in role checks
The use of
pytest-mockwithAsyncMockfor the resolvers follows the coding guidelines correctly.
235-269: LGTM! Decorator tests verify both success and failure paths.The tests appropriately:
- Mock
_perform_authorization_checkto control authorization outcome- Verify successful authorization allows the endpoint to execute
- Verify authorization failure raises HTTP 403
tests/unit/authorization/test_resolvers.py (4)
34-75: LGTM! Well-structured fixtures improve test maintainability.The new fixtures (
employee_role_rule,employee_resolver,employee_claims,non_employee_claims) effectively:
- Reduce code duplication across tests
- Provide clear, reusable test data
- Follow pytest best practices with descriptive names and docstrings
107-182: LGTM! Comprehensive operator testing.The new tests effectively cover:
- MATCH operator with email domain regex
- EQUALS operator with exact matching
- IN operator with list membership
- Both successful matches and non-matches for each operator
This expands test coverage to validate the various
JsonPathOperatorbehaviors.
252-264: LGTM! Good edge-case coverage for guest tokens.The test correctly verifies that
NO_USER_TOKENis handled gracefully without raising exceptions. The use ofdoes_not_raise()context manager (fromcontextlib.nullcontext) is a good pattern for explicitly testing that no exception occurs.This aligns with the resolver changes that return empty claims for tokens without claims instead of raising errors.
270-342: LGTM! Comprehensive admin and multi-role testing.The new tests effectively verify:
- Admin role validation (admin action cannot coexist with other actions in a single rule)
- Admin role grants access to all actions via recursive check
get_actionscorrectly excludes the ADMIN action itself from returned actions- Multi-role users receive combined permissions from all their roles
This provides thorough coverage of the
GenericAccessResolverbehavior for edge cases and special roles.
| async def test_negate_operator(self, employee_role_rule, non_employee_claims): | ||
| """Test role extraction with negated operator.""" | ||
| negated_rule = employee_role_rule | ||
| negated_rule.negate = True | ||
|
|
||
| auth = ("user", "token", False, claims_to_token(jwt_claims)) | ||
| roles = await jwt_resolver.resolve_roles(auth) | ||
| assert "redhat_employee" in roles | ||
| resolver = JwtRolesResolver([negated_rule]) | ||
|
|
||
| async def test_resolve_roles_match_operator_no_match(self): | ||
| """Test role extraction using MATCH operator with no match.""" | ||
| role_rules = [ | ||
| JwtRoleRule( | ||
| jsonpath="$.email", | ||
| operator=JsonPathOperator.MATCH, | ||
| value=r"@redhat\.com$", | ||
| roles=["redhat_employee"], | ||
| ) | ||
| ] | ||
| jwt_resolver = JwtRolesResolver(role_rules) | ||
| assert "employee" in await resolver.resolve_roles( | ||
| claims_to_auth_tuple(non_employee_claims) | ||
| ) |
There was a problem hiding this comment.
Avoid mutating fixtures; create a new rule instance instead.
Lines 98-99 mutate the employee_role_rule fixture by setting negated_rule.negate = True. Mutating fixtures can cause test isolation issues and violates the principle that fixtures should provide fresh, immutable test data.
Apply this diff to create a new rule instance instead:
- async def test_negate_operator(self, employee_role_rule, non_employee_claims):
+ async def test_negate_operator(self, non_employee_claims):
"""Test role extraction with negated operator."""
- negated_rule = employee_role_rule
- negated_rule.negate = True
+ negated_rule = JwtRoleRule(
+ jsonpath="$.realm_access.roles[*]",
+ operator=JsonPathOperator.CONTAINS,
+ value="redhat:employees",
+ roles=["employee"],
+ negate=True,
+ )
resolver = JwtRolesResolver([negated_rule])📝 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.
| async def test_negate_operator(self, employee_role_rule, non_employee_claims): | |
| """Test role extraction with negated operator.""" | |
| negated_rule = employee_role_rule | |
| negated_rule.negate = True | |
| auth = ("user", "token", False, claims_to_token(jwt_claims)) | |
| roles = await jwt_resolver.resolve_roles(auth) | |
| assert "redhat_employee" in roles | |
| resolver = JwtRolesResolver([negated_rule]) | |
| async def test_resolve_roles_match_operator_no_match(self): | |
| """Test role extraction using MATCH operator with no match.""" | |
| role_rules = [ | |
| JwtRoleRule( | |
| jsonpath="$.email", | |
| operator=JsonPathOperator.MATCH, | |
| value=r"@redhat\.com$", | |
| roles=["redhat_employee"], | |
| ) | |
| ] | |
| jwt_resolver = JwtRolesResolver(role_rules) | |
| assert "employee" in await resolver.resolve_roles( | |
| claims_to_auth_tuple(non_employee_claims) | |
| ) | |
| async def test_negate_operator(self, non_employee_claims): | |
| """Test role extraction with negated operator.""" | |
| negated_rule = JwtRoleRule( | |
| jsonpath="$.realm_access.roles[*]", | |
| operator=JsonPathOperator.CONTAINS, | |
| value="redhat:employees", | |
| roles=["employee"], | |
| negate=True, | |
| ) | |
| resolver = JwtRolesResolver([negated_rule]) | |
| assert "employee" in await resolver.resolve_roles( | |
| claims_to_auth_tuple(non_employee_claims) | |
| ) |
🤖 Prompt for AI Agents
In tests/unit/authorization/test_resolvers.py around lines 96 to 105, the test
mutates the shared fixture by setting negated_rule.negate = True; instead,
create a new rule instance (e.g., via copying/deepcopy or using the rule's
constructor/ dataclass replace utility) with negate=True and use that new
instance for JwtRolesResolver so the original fixture remains unchanged and test
isolation is preserved.
2 participants
Description
The coverage for the resolvers module and the authz middleware module
was a bit low. This commit adds tests to improve that.
Also fixes a minor issue in the
JwtRolesResolverwhere it would raisean error if the JWT has no claims, which shouldn't be an problem.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Bug Fixes
Tests