Commit 259cae5
[AAP-52133] Improve RoleUserAssignmentsCache.cache_existing method (ansible#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- ansible_base/authentication/utils
- test_app/tests/authentication/utils
2 files changed
+545
-8
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| 25 | + | |
25 | 26 | | |
26 | 27 | | |
27 | 28 | | |
| |||
894 | 895 | | |
895 | 896 | | |
896 | 897 | | |
897 | | - | |
898 | | - | |
| 898 | + | |
| 899 | + | |
| 900 | + | |
| 901 | + | |
| 902 | + | |
| 903 | + | |
| 904 | + | |
| 905 | + | |
| 906 | + | |
| 907 | + | |
| 908 | + | |
| 909 | + | |
| 910 | + | |
| 911 | + | |
| 912 | + | |
| 913 | + | |
| 914 | + | |
| 915 | + | |
| 916 | + | |
| 917 | + | |
| 918 | + | |
| 919 | + | |
| 920 | + | |
| 921 | + | |
| 922 | + | |
| 923 | + | |
| 924 | + | |
| 925 | + | |
| 926 | + | |
| 927 | + | |
| 928 | + | |
| 929 | + | |
| 930 | + | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
| 935 | + | |
| 936 | + | |
| 937 | + | |
| 938 | + | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
| 942 | + | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
| 947 | + | |
| 948 | + | |
| 949 | + | |
| 950 | + | |
899 | 951 | | |
900 | 952 | | |
901 | 953 | | |
902 | 954 | | |
903 | 955 | | |
904 | 956 | | |
905 | | - | |
| 957 | + | |
| 958 | + | |
| 959 | + | |
| 960 | + | |
| 961 | + | |
| 962 | + | |
| 963 | + | |
| 964 | + | |
906 | 965 | | |
907 | 966 | | |
908 | | - | |
909 | | - | |
910 | | - | |
| 967 | + | |
| 968 | + | |
| 969 | + | |
| 970 | + | |
| 971 | + | |
| 972 | + | |
| 973 | + | |
| 974 | + | |
| 975 | + | |
| 976 | + | |
| 977 | + | |
| 978 | + | |
| 979 | + | |
| 980 | + | |
| 981 | + | |
| 982 | + | |
| 983 | + | |
| 984 | + | |
| 985 | + | |
| 986 | + | |
| 987 | + | |
| 988 | + | |
| 989 | + | |
| 990 | + | |
| 991 | + | |
911 | 992 | | |
912 | | - | |
| 993 | + | |
913 | 994 | | |
914 | 995 | | |
915 | 996 | | |
| |||
0 commit comments