Skip to content

Commit 259cae5

Browse files
john-westcott-ivClaudeBrennanPaciorekAlanCoding
authored
[AAP-52133] Improve RoleUserAssignmentsCache.cache_existing method (#824)
**Changes to claims.py:** - Fix object_id type handling in cache_existing method to support UUID, int, and string types - Add proper error handling and logging for invalid object_id types and conversion failures - Replace simple int conversion with robust type checking and validation - Add UUID import and improve type safety for object_id processing - Replace direct role_assignments query with get_user_object_roles for better data handling **New comprehensive test suite:** - Create new TestRoleUserAssignmentsCache class with full test coverage - Add parameterized tests for different object_id types (None, int, UUID, string) - Test error handling for invalid object_id types and string conversion failures - Verify cache structure, status assignment, and role definition caching behavior - Test edge cases: empty lists, multiple assignments, existing cache preservation - Refactor similar tests into parameterized tests for better maintainability - All tests follow pytest best practices with descriptive IDs and proper fixtures ## Description <!-- Mandatory: Provide a clear, concise description of the changes and their purpose --> - What is being changed? Instead of using our own filter for getting user.role_assignments we now use the RBAC `get_user_object_roles` method. This should filter out any UUID permissions which were causing 500s before. Additionally we updated the `cache_existing` method so that it would now properly accept a UUID incase one ever made it through to avoid 500s in the future. - Why is this change needed? Login can fail if the user logging in has permissions to non-gateway items (which return a UUID instead of an int). - How does this change address the issue? This change should: 1) make the UUID objects not cached to start with 2) alter the cache logic to better tolerate invalid values. ## Type of Change <!-- Mandatory: Check one or more boxes that apply --> - [X] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update - [ ] Test update - [ ] Refactoring (no functional changes) - [ ] Development environment change - [ ] Configuration change ## Self-Review Checklist <!-- These items help ensure quality - they complement our automated CI checks --> - [X] I have performed a self-review of my code - [X] I have added relevant comments to complex code sections - [X] I have updated documentation where needed - [X] I have considered the security impact of these changes - [X] I have considered performance implications - [X] I have thought about error handling and edge cases - [X] I have tested the changes in my local environment ## Testing Instructions <!-- Optional for test-only changes. Mandatory for all other changes --> <!-- Must be detailed enough for reviewers to reproduce --> ### Prerequisites <!-- List any specific setup required --> ### Steps to Test 1. Configure gateway with a service 2. Login as a user from a source with authenticator maps 3. Grant the user permissions to objects in the service 4. Log out and login as the user, login should fail ### Expected Results <!-- Describe what should happen after following the steps --> ## Additional Context <!-- Optional but helpful information --> ### Required Actions <!-- Check if changes require work in other areas --> <!-- Remove section if no external actions needed --> - [ ] Requires documentation updates <!-- API docs, feature docs, deployment guides --> - [ ] Requires downstream repository changes <!-- Specify repos: django-ansible-base, eda-server, etc. --> - [ ] Requires infrastructure/deployment changes <!-- CI/CD, installer updates, new services --> - [ ] Requires coordination with other teams <!-- UI team, platform services, infrastructure --> - [ ] Blocked by PR/MR: #XXX <!-- Reference blocking PRs/MRs with brief context --> ### Screenshots/Logs <!-- Add if relevant to demonstrate the changes --> --------- Co-authored-by: Claude <[email protected]> Co-authored-by: Brennan Paciorek <[email protected]> Co-authored-by: Alan Rominger <[email protected]>
1 parent fe0b1d9 commit 259cae5

File tree

2 files changed

+545
-8
lines changed

2 files changed

+545
-8
lines changed

ansible_base/authentication/utils/claims.py

Lines changed: 89 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import logging
44
import re
55
from enum import Enum, auto
6-
from typing import Any, List, Optional, Union
6+
from typing import Any, Iterable, List, Optional, Union
77
from uuid import uuid4
88

99
from django.conf import settings
@@ -22,6 +22,7 @@
2222
from ansible_base.lib.utils.auth import get_organization_model, get_team_model
2323
from ansible_base.lib.utils.string import is_empty
2424
from ansible_base.rbac.models import DABContentType
25+
from ansible_base.rbac.remote import get_local_resource_prefix
2526

2627
from .trigger_definition import TRIGGER_DEFINITION
2728

@@ -894,22 +895,102 @@ def items(self):
894895
"""
895896
return self.cache.items()
896897

897-
def cache_existing(self, role_assignments):
898-
"""Caches given role_assignments associated with one user in form of dict (see method `items()`)"""
898+
def cache_existing(self, role_assignments: Iterable[models.Model]) -> None:
899+
"""
900+
Caches given role_assignments associated with one user in the internal cache dictionary.
901+
902+
This method processes role assignments and stores them in a nested dictionary structure
903+
for efficient lookup during permission reconciliation.
904+
905+
Args:
906+
role_assignments: An iterable of role assignment model instances (typically from
907+
user.role_assignments.all() QuerySet) that contain role_definition,
908+
content_type, content_object, and object_id attributes.
909+
910+
Cache Structure:
911+
The internal cache will be populated in the following format:
912+
{
913+
"System Auditor": { # role_name (str)
914+
None: { # content_type_id (None for system roles)
915+
None: { # object_id (None for system roles)
916+
'object': None, # content_object (None for system roles)
917+
'status': 'existing' # STATUS_EXISTING constant
918+
}
919+
}
920+
},
921+
"Organization Admin": { # role_name (str)
922+
15: { # content_type_id (int, e.g., Organization content type)
923+
42: { # object_id (int, specific organization ID)
924+
'object': <Organization>, # content_object (Organization instance)
925+
'status': 'existing' # STATUS_EXISTING constant
926+
},
927+
43: { # object_id (int, another organization ID)
928+
'object': <Organization>, # content_object (Organization instance)
929+
'status': 'existing' # STATUS_EXISTING constant
930+
}
931+
}
932+
},
933+
"Team Member": { # role_name (str)
934+
16: { # content_type_id (int, e.g., Team content type)
935+
7: { # object_id (int, specific team ID)
936+
'object': <Team>, # content_object (Team instance)
937+
'status': 'existing' # STATUS_EXISTING constant
938+
}
939+
}
940+
}
941+
}
942+
943+
Notes:
944+
- Caches both global/system roles and local object role assignments
945+
- Global/system roles have content_type_id=None and object_id=None
946+
- Local object roles are cached only if content_type.service is local or "shared"
947+
- Organization/Team roles have specific content_type_id and object_id values
948+
- All cached assignments are marked with STATUS_EXISTING status
949+
- Role definitions are also cached separately in self.role_definitions
950+
"""
899951
for role_assignment in role_assignments:
900952
# Cache role definition
901953
if (role_definition := self._rd_by_id(role_assignment)) is None:
902954
role_definition = role_assignment.role_definition
903955
self.role_definitions[role_definition.name] = role_definition
904956

905-
# Cache Role User Assignment
957+
# Skip role assignments that should not be cached
958+
if not (
959+
role_assignment.content_type is None # Global/system roles (e.g., System Auditor)
960+
or role_assignment.content_type.service in [get_local_resource_prefix(), "shared"]
961+
): # Local object roles
962+
continue
963+
964+
# Cache Role User Assignment - only initialize cache key for assignments we're actually caching
906965
self._init_cache_key(role_definition.name, content_type_id=role_assignment.content_type_id)
907966

908-
# object_id is TEXT db type
909-
object_id = int(role_assignment.object_id) if role_assignment.object_id is not None else None
910-
obj = role_assignment.content_object if object_id else None
967+
# Cache the role assignment
968+
self._cache_role_assignment(role_definition, role_assignment)
969+
970+
def _cache_role_assignment(self, role_definition: models.Model, role_assignment: models.Model) -> None:
971+
"""
972+
Cache a single role assignment.
973+
974+
Args:
975+
role_definition: The role definition associated with this assignment
976+
role_assignment: The role assignment to cache
977+
"""
978+
if role_assignment.content_type is None:
979+
# Global role - both object_id and content_object are None
980+
object_id = None
981+
obj = None
982+
else:
983+
# Object role - try to convert object_id to int
984+
try:
985+
object_id = int(role_assignment.object_id) if role_assignment.object_id is not None else None
986+
except (ValueError, TypeError):
987+
# Intended to catch any int casting errors, since we're assuming object_ids are text values cast-able to integers
988+
logger.exception(f'Unable to cache object_id {role_assignment.object_id}: Could not cast to type int')
989+
return # Skip this role assignment if we can't convert the object_id
990+
991+
obj = role_assignment.content_object if object_id is not None else None
911992

912-
self.cache[role_definition.name][role_assignment.content_type_id][object_id] = {'object': obj, 'status': self.STATUS_EXISTING}
993+
self.cache[role_definition.name][role_assignment.content_type_id][object_id] = {'object': obj, 'status': self.STATUS_EXISTING}
913994

914995
def rd_by_name(self, role_name: str) -> Optional[CommonModel]:
915996
"""Returns RoleDefinition by its name. Caches it if requested for first time"""

0 commit comments

Comments
 (0)