-
Notifications
You must be signed in to change notification settings - Fork 81
[AAP-50407] Refactor JWT claims handling to use gateway endpoint #789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devel
Are you sure you want to change the base?
Changes from 8 commits
77db991
bbd74c1
cac9bf2
bd16d2f
7f5e2ee
2d379b6
68ebe9c
cd32782
a626222
0384836
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,6 +19,7 @@ | |||||
from ansible_base.lib.utils.auth import get_user_by_ansible_id | ||||||
from ansible_base.lib.utils.translations import translatableConditionally as _ | ||||||
from ansible_base.resource_registry.models import Resource, ResourceType | ||||||
from ansible_base.resource_registry.rest_client import get_resource_server_client | ||||||
from ansible_base.resource_registry.signals.handlers import no_reverse_sync | ||||||
|
||||||
logger = logging.getLogger("ansible_base.jwt_consumer.common.auth") | ||||||
|
@@ -52,6 +53,7 @@ def __init__(self, user_fields=default_mapped_user_fields) -> None: | |||||
self.cache = JWTCache() | ||||||
self.user = None | ||||||
self.token = None | ||||||
self.gateway_claims = None # Store claims from gateway | ||||||
|
||||||
@log_excess_runtime(logger, debug_cutoff=0.01) | ||||||
def parse_jwt_token(self, request): | ||||||
|
@@ -142,10 +144,94 @@ def parse_jwt_token(self, request): | |||||
resource.service_id = self.token['service_id'] | ||||||
resource.save(update_fields=['ansible_id', 'service_id']) | ||||||
|
||||||
# Check if claims need to be refreshed from gateway based on claims_hash | ||||||
user_ansible_id = self.token['sub'] | ||||||
current_claims_hash = self.token.get('claims_hash') | ||||||
|
||||||
if self._should_fetch_claims_from_gateway(user_ansible_id, current_claims_hash): | ||||||
logger.debug(f"Claims hash changed or not cached, fetching claims from gateway for user {user_ansible_id}") | ||||||
jwt_claims = self._fetch_jwt_claims_from_gateway(user_ansible_id) | ||||||
|
||||||
if jwt_claims: | ||||||
self.gateway_claims = jwt_claims | ||||||
self._cache_claims_hash(user_ansible_id, current_claims_hash) | ||||||
logger.debug(f"Successfully loaded and cached gateway claims for user {user_ansible_id}") | ||||||
else: | ||||||
logger.error(f"Failed to fetch claims from gateway for user {user_ansible_id}. RBAC processing will not be available.") | ||||||
# Note: We don't raise an exception here to allow basic authentication to succeed | ||||||
# RBAC processing will fail gracefully with appropriate error messages | ||||||
else: | ||||||
logger.debug(f"Using cached claims for user {user_ansible_id} (claims_hash unchanged)") | ||||||
setattr(self.user, "resource_api_actions", self.token.get("resource_api_actions", None)) | ||||||
|
||||||
logger.info(f"User {self.user.username} authenticated from JWT auth") | ||||||
|
||||||
def _should_fetch_claims_from_gateway(self, user_ansible_id, current_claims_hash): | ||||||
""" | ||||||
Determine if claims should be fetched from gateway based on claims_hash comparison. | ||||||
Returns True if claims need to be fetched (hash changed or not cached). | ||||||
""" | ||||||
if not current_claims_hash: | ||||||
logger.debug(f"No claims_hash in token for user {user_ansible_id}, will fetch claims") | ||||||
return True | ||||||
|
||||||
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 | ||||||
|
||||||
|
||||||
# Hash matches cached value, try to get cached claims | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will update to do approximately nothing 🤣 |
||||||
cached_claims = self.cache.get_cached_claims(user_ansible_id) | ||||||
if cached_claims: | ||||||
self.gateway_claims = cached_claims | ||||||
return False | ||||||
else: | ||||||
logger.debug(f"Claims hash matches but no cached claims found for user {user_ansible_id}") | ||||||
return True | ||||||
|
||||||
def _cache_claims_hash(self, user_ansible_id, claims_hash): | ||||||
"""Cache the claims hash and gateway claims for future comparisons.""" | ||||||
if claims_hash and self.gateway_claims: | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
""" | ||||||
Fetch JWT claims for a user from the gateway service-index API. | ||||||
Returns None if claims cannot be retrieved. | ||||||
""" | ||||||
try: | ||||||
# Use the full service path from settings, not just "service-index" | ||||||
# This should be something like "/api/gateway/v1/service-index/" | ||||||
service_path = getattr(settings, "RESOURCE_SERVICE_PATH", "/api/gateway/v1/service-index/") | ||||||
client = get_resource_server_client(service_path) | ||||||
response = client.get_jwt_claims(user_ansible_id) | ||||||
|
||||||
if response.status_code == 200: | ||||||
# Try to parse JSON, but handle empty or invalid responses | ||||||
try: | ||||||
claims = response.json() | ||||||
logger.debug(f"Retrieved JWT claims from gateway for user {user_ansible_id}") | ||||||
return claims | ||||||
except ValueError: | ||||||
# Log the actual response content for debugging | ||||||
logger.error( | ||||||
f"Invalid JSON response from gateway for user {user_ansible_id}. " | ||||||
f"Status: {response.status_code}, " | ||||||
f"Content-Type: {response.headers.get('Content-Type', 'unknown')}, " | ||||||
f"Response text: {response.text[:500]}" # First 500 chars to avoid huge logs | ||||||
) | ||||||
return None | ||||||
else: | ||||||
logger.warning( | ||||||
f"Failed to retrieve JWT claims from gateway for user {user_ansible_id}: " | ||||||
f"Status: {response.status_code}, Response: {response.text[:500]}" | ||||||
) | ||||||
return None | ||||||
except Exception as e: | ||||||
logger.error(f"Error fetching JWT claims from gateway for user {user_ansible_id}: {e}") | ||||||
return None | ||||||
|
||||||
def log_and_raise(self, conditional_translate_object, expand_values={}, error_code=None): | ||||||
logger.error(conditional_translate_object.not_translated() % expand_values) | ||||||
translated_error_message = conditional_translate_object.translated() % expand_values | ||||||
|
@@ -226,7 +312,8 @@ def validate_token(self, unencrypted_token, decryption_key, request_id=None): | |||||
return validated_body | ||||||
|
||||||
def decode_jwt_token(self, unencrypted_token, decryption_key, additional_options={}): | ||||||
local_required_field = ["sub", "user_data", "exp", "objects", "object_roles", "global_roles", "version"] | ||||||
# Core required fields - claims_hash is now required to track permission changes | ||||||
local_required_field = ["sub", "user_data", "exp", "version", "claims_hash"] | ||||||
options = {"require": local_required_field} | ||||||
options.update(additional_options) | ||||||
return jwt.decode( | ||||||
|
@@ -259,16 +346,23 @@ def get_role_definition(self, name: str) -> Optional[Model]: | |||||
def process_rbac_permissions(self): | ||||||
""" | ||||||
This is a default process_permissions which should be usable if you are using RBAC from DAB | ||||||
Uses gateway claims data exclusively - no fallback to JWT token fields | ||||||
""" | ||||||
if self.token is None or self.user is None: | ||||||
logger.error("Unable to process rbac permissions because user or token is not defined, please call authenticate first") | ||||||
if self.user is None: | ||||||
logger.error("Unable to process rbac permissions because user is not defined, please call authenticate first") | ||||||
return | ||||||
|
||||||
if self.gateway_claims is None: | ||||||
logger.error("Unable to process rbac permissions because gateway claims are not available. Ensure gateway jwt_claims endpoint is accessible.") | ||||||
return | ||||||
|
||||||
from ansible_base.rbac.models import RoleUserAssignment | ||||||
|
||||||
role_diff = RoleUserAssignment.objects.filter(user=self.user, role_definition__name__in=settings.ANSIBLE_BASE_JWT_MANAGED_ROLES) | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||||||
logger.debug(f"Processing system role {system_role_name} for {self.user.username}") | ||||||
rd = self.get_role_definition(system_role_name) | ||||||
if rd: | ||||||
|
@@ -282,7 +376,11 @@ def process_rbac_permissions(self): | |||||
logger.error(f"Unable to grant {self.user.username} system level role {system_role_name} because it does not exist") | ||||||
continue | ||||||
|
||||||
for object_role_name in self.token.get('object_roles', {}).keys(): | ||||||
# Process object roles from gateway claims | ||||||
object_roles = self.gateway_claims.get('object_roles', {}) | ||||||
objects = self.gateway_claims.get('objects', {}) | ||||||
|
||||||
for object_role_name in object_roles.keys(): | ||||||
rd = self.get_role_definition(object_role_name) | ||||||
if rd is None: | ||||||
logger.error(f"Unable to grant {self.user.username} object role {object_role_name} because it does not exist") | ||||||
|
@@ -291,11 +389,11 @@ def process_rbac_permissions(self): | |||||
logger.error(f"Unable to grant {self.user.username} object role {object_role_name} because it is not a JWT managed role") | ||||||
continue | ||||||
|
||||||
object_type = self.token['object_roles'][object_role_name]['content_type'] | ||||||
object_indexes = self.token['object_roles'][object_role_name]['objects'] | ||||||
object_type = object_roles[object_role_name]['content_type'] | ||||||
object_indexes = object_roles[object_role_name]['objects'] | ||||||
|
||||||
for index in object_indexes: | ||||||
object_data = self.token['objects'][object_type][index] | ||||||
object_data = objects[object_type][index] | ||||||
try: | ||||||
resource, obj = self.get_or_create_resource(object_type, object_data) | ||||||
except IntegrityError as e: | ||||||
|
@@ -310,7 +408,7 @@ def process_rbac_permissions(self): | |||||
role_diff = role_diff.exclude(pk=assignment.pk) | ||||||
logger.info(f"Granted user {self.user.username} role {object_role_name} to object {obj.name} with ansible_id {object_data['ansible_id']}") | ||||||
|
||||||
# Remove all permissions not authorized by the JWT | ||||||
# Remove all permissions not authorized by the gateway claims | ||||||
for role_assignment in role_diff: | ||||||
rd = role_assignment.role_definition | ||||||
content_object = role_assignment.content_object | ||||||
|
@@ -322,9 +420,17 @@ def process_rbac_permissions(self): | |||||
def get_or_create_resource(self, content_type: str, data: dict) -> Tuple[Optional[Resource], Optional[Model]]: | ||||||
""" | ||||||
Gets or creates a resource from a content type and its default data | ||||||
Uses gateway claims exclusively - no fallback to JWT token fields | ||||||
|
||||||
This can only build or get organizations or teams | ||||||
Args: | ||||||
content_type: Type of content ('team', 'organization') | ||||||
data: Resource data dictionary | ||||||
""" | ||||||
if self.gateway_claims is None: | ||||||
logger.error("Unable to create resource because gateway claims are not available") | ||||||
return None, None | ||||||
|
||||||
object_ansible_id = data['ansible_id'] | ||||||
try: | ||||||
resource = Resource.objects.get(ansible_id=object_ansible_id) | ||||||
|
@@ -337,7 +443,7 @@ def get_or_create_resource(self, content_type: str, data: dict) -> Tuple[Optiona | |||||
if content_type == 'team': | ||||||
# For a team we first have to make sure the org is there | ||||||
org_id = data['org'] | ||||||
organization_data = self.token['objects']["organization"][org_id] | ||||||
organization_data = self.gateway_claims['objects']["organization"][org_id] | ||||||
|
||||||
# Now that we have the org we can build a team | ||||||
org_resource, _ = self.get_or_create_resource("organization", organization_data) | ||||||
|
@@ -359,7 +465,7 @@ def get_or_create_resource(self, content_type: str, data: dict) -> Tuple[Optiona | |||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
return None, None | ||||||
|
||||||
|
||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just do what #583 does. I'm telling you right now, the reason this is a duplicated mess is because:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea, we'll do this in a separate PR |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -542,9 +542,8 @@ def __init__(self): | |
"email": "[email protected]", | ||
"is_superuser": False, | ||
}, | ||
"objects": {}, | ||
"object_roles": {}, | ||
"global_roles": [], | ||
# claims_hash is required to track permission changes | ||
"claims_hash": "test_hash_123", | ||
} | ||
|
||
def encrypt_token(self): | ||
|
@@ -595,6 +594,31 @@ def mocked_gateway_view_get_request(self, *args, **kwargs): | |
return MockedHttp() | ||
|
||
|
||
@pytest.fixture | ||
def mock_gateway_jwt_claims(): | ||
"""Mock for gateway JWT claims endpoint.""" | ||
mock_claims = { | ||
"global_roles": ["Platform Auditor"], | ||
"object_roles": {"Organization Admin": {"content_type": "organization", "objects": [0]}}, | ||
"objects": {"organization": [{"ansible_id": "test-org-id", "name": "Test Organization"}], "team": []}, | ||
} | ||
|
||
class MockResponse: | ||
def __init__(self, json_data, status_code=200): | ||
self.json_data = json_data | ||
self.status_code = status_code | ||
|
||
def json(self): | ||
return self.json_data | ||
|
||
class MockResourceAPIClient: | ||
def get_jwt_claims(self, user_ansible_id): | ||
return MockResponse(mock_claims) | ||
|
||
with mock.patch('ansible_base.jwt_consumer.common.auth.get_resource_server_client', return_value=MockResourceAPIClient()): | ||
yield mock_claims | ||
|
||
|
||
@pytest.fixture | ||
def system_user(db, no_log_messages): | ||
with no_log_messages(): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very strongly convinced this shouldn't be on
self
. Let the de-referenced be garbage collected.