[AAP-50407] Refactor JWT claims handling to use gateway endpoint #789
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors JWT claims handling to use a gateway endpoint instead of embedded JWT token fields. It removes the deprecated objects, object_roles, and global_roles fields from JWT tokens and fetches permission data from the gateway's service-index/jwt_claims/<user_ansible_id> endpoint.
Key changes:
- Modified JWT authentication to fetch claims from gateway API instead of using embedded token fields
- Updated RBAC processing to exclusively use gateway claims with no fallback to deprecated JWT fields
- Added new
get_jwt_claims()method toResourceAPIClientfor gateway communication
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test_app/tests/conftest.py | Removed deprecated JWT token fields and added explanatory comments |
| ansible_base/resource_registry/rest_client.py | Added new get_jwt_claims() method and improved import formatting |
| ansible_base/jwt_consumer/common/cache.py | Added trailing newline |
| ansible_base/jwt_consumer/common/auth.py | Major refactoring to use gateway claims exclusively with comprehensive error handling |
Comments suppressed due to low confidence (1)
ansible_base/jwt_consumer/common/auth.py:165
- [nitpick] The empty line between method definition and the next method creates inconsistent spacing. Consider removing this extra blank line to maintain consistent code formatting.
def _fetch_jwt_claims_from_gateway(self, user_ansible_id):
| return resource, resource.content_object | ||
| else: | ||
| logger.error(f"build_resource_stub does not know how to build an object of type {type}") | ||
| logger.error(f"build_resource_stub does not know how to build an object of type {content_type}") |
There was a problem hiding this comment.
The error message references 'build_resource_stub' but this method is actually called 'get_or_create_resource'. The error message should be updated to reflect the correct method name for clarity.
| logger.error(f"build_resource_stub does not know how to build an object of type {content_type}") | |
| logger.error(f"get_or_create_resource does not know how to build an object of type {content_type}") |
Copilot uses AI. Check for mistakes.
TheRealHaoLiu
force-pushed
the
AAP-47811-update-jwt_consumer-to-load-user-claims-from-new-endpoint
branch
from
30d4e41 to
df0798d
Compare
JWT claims are now exclusively fetched from the gateway service-index API instead of being included in the JWT token. Deprecated fields (objects, object_roles, global_roles) are removed from token processing and all RBAC logic now relies on gateway claims. Added helper to ResourceAPIClient for fetching claims, and updated tests to reflect the new claims source.
TheRealHaoLiu
force-pushed
the
AAP-47811-update-jwt_consumer-to-load-user-claims-from-new-endpoint
branch
from
df0798d to
77db991
Compare
Switches to using the full service path from settings when retrieving JWT claims, adds robust JSON parsing with error handling, and enhances logging for invalid responses and failures. This improves reliability and debuggability when interacting with the gateway.
Replaces usage of the 'token' attribute with 'gateway_claims' in JWTCommonAuth tests to reflect recent code changes. Adds comprehensive tests for fetching JWT claims from the gateway, including success, invalid JSON, non-200 responses, and cache/hash logic.
Removed unused exception variable in JWTCommonAuth and improved formatting in related unit tests for clarity and consistency. No functional changes to logic.
Modified test_auth.py and hub/test_auth.py to reflect changes in JWT claims structure, replacing 'token' with 'gateway_claims' and updating role representations. Adjusted test parameters and assertions to align with the new claims format.
Update the condition to explicitly check for None when verifying the presence of gateway_claims. This prevents issues when gateway_claims is an empty value but not None.
Introduces tests to cover cache miss scenarios, exception handling during JWT claims fetch, and correct usage of RESOURCE_SERVICE_PATH. Also adds tests for caching behavior when claims hash or gateway claims are missing, improving coverage and reliability of JWTCommonAuth.
Cleaned up extra spaces and ensured consistent formatting in test_auth.py for better readability and code style adherence. No functional changes were made.
|
no matter how much test i add... sonar just saying i have 0% coverage for the new code! LIES! |
| logger.debug(f"Claims hash changed for user {user_ansible_id}: cached={cached_hash}, current={current_claims_hash}") | ||
| return True | ||
|
|
||
| # Hash matches cached value, try to get cached claims |
There was a problem hiding this comment.
No, don't do that.
If hashes match, do nothing. Don't do approximately nothing, do exactly nothing. Don't save the claims for later. If the hashes match that means the claims have already been saved.
There was a problem hiding this comment.
will update to do approximately nothing 🤣
| self.cache = JWTCache() | ||
| self.user = None | ||
| self.token = None | ||
| self.gateway_claims = None # Store claims from gateway |
There was a problem hiding this comment.
Very strongly convinced this shouldn't be on self. Let the de-referenced be garbage collected.
| for system_role_name in self.token.get("global_roles", []): | ||
| # Process global roles from gateway claims | ||
| global_roles = self.gateway_claims.get("global_roles", []) | ||
| for system_role_name in global_roles: |
There was a problem hiding this comment.
You're trying too hard to not change the existing code. And in this case it's not good code.
Make a new method, like save_claims(user, claims). See other stuff in ansible_base/rbac/claims.py, it should fit in with that crowd.
It should take the claims, and make them true for that user. You don't have to refactor the logic itself here (although you should, you don't have to), but you do really need to refactor the interface.
This method should be callable from unit tests, and it should be called from unit tests. We shouldn't need to see the JWT auth class within a mile of that logic.
| self.cache.set_claims_hash(user_ansible_id, claims_hash) | ||
| self.cache.set_cached_claims(user_ansible_id, self.gateway_claims) | ||
|
|
||
| def _fetch_jwt_claims_from_gateway(self, user_ansible_id): |
There was a problem hiding this comment.
| def _fetch_jwt_claims_from_gateway(self, user_ansible_id): | |
| def fetch_jwt_claims_from_gateway(self, user_ansible_id) -> dict[str,dict[dict[dict[str,list],list],dict]: |
There was a problem hiding this comment.
Just do what #583 does. I'm telling you right now, the reason this is a duplicated mess is because:
TODO - galaxy does not have an org admin roledef yet
and that this should no longer apply. If galaxy_ng errors because it doesn't have an Organization Admin role, add it there.
There was a problem hiding this comment.
good idea, we'll do this in a separate PR
| cached_hash = self.cache.get_claims_hash(user_ansible_id) | ||
| if cached_hash != current_claims_hash: | ||
| logger.debug(f"Claims hash changed for user {user_ansible_id}: cached={cached_hash}, current={current_claims_hash}") | ||
| return True |
There was a problem hiding this comment.
As a modifier to this, from another tangent conversation - if hashes mismatch, you can re-generate the local hash, and if that matches, replace the local cache value with it, and return True. But if you re-generate the local hash and it still doesn't match, then request has to be made.
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
AAP-50407 JWT Claims Gateway Integration - Complete Implementation
Description
This PR implements a complete migration from JWT token-embedded claims to gateway-sourced claims via the
service-index/jwt_claims/<user_ansible_id>endpoint.What is being changed?
objects,object_roles,global_roles) from required JWT token validationWhy is this change needed?
objects,object_roles, andglobal_rolesare being deprecated across the platformHow does this change address the issue?
get_jwt_claims()method inResourceAPIClientto call the gateway endpointJWTCommonAuthto fetch and store gateway claims inself.gateway_claimsprocess_rbac_permissions()andget_or_create_resource()to use gateway data exclusivelyType of Change
Self-Review Checklist
Testing Instructions
Prerequisites
service-index/jwt_claims/<user_ansible_id>endpointSteps to Test
Basic Authentication Test:
X-DAB-JW-TOKENheaderself.gateway_claimsis populatedRBAC Processing Test:
process_rbac_permissions()Resource Creation Test:
Error Handling Test:
Expected Results
Additional Context
Required Actions
Breaking Changes
This is a breaking change that affects systems relying on JWT token embedded claims:
service-index/jwt_claims/endpointMigration Impact
Before: JWT tokens contained full permission data
{ "sub": "user-id", "objects": {"organization": [...], "team": [...]}, "object_roles": {"Org Admin": {...}}, "global_roles": ["Platform Auditor"] }After: JWT tokens contain minimal auth data, permissions from gateway
{ "sub": "user-id", "user_data": {...} }Implementation Details
Files Modified
ansible_base/jwt_consumer/common/auth.py- Main JWT processing logicansible_base/jwt_consumer/common/cache.py- Cache methods (cleanup)ansible_base/resource_registry/rest_client.py- Gateway API clienttest_app/tests/conftest.py- Test fixtures updatedKey Methods Updated
JWTCommonAuth.parse_jwt_token()- Now fetches from gatewayJWTCommonAuth.process_rbac_permissions()- Uses gateway claims onlyJWTCommonAuth.get_or_create_resource()- Uses gateway claims onlyResourceAPIClient.get_jwt_claims()- New gateway endpoint methodError Handling Strategy
Performance Considerations
Security Impact
This implementation provides a clean, no-fallback solution that exclusively uses the gateway service for permission claims while maintaining robust error handling for operational scenarios.