feat: add cache for IAM #3124
feat: add cache for IAM #3124
Conversation
cache folders, roles, role assignments and user_groups
📝 WalkthroughWalkthroughAdds a versioned, DB-backed snapshot cache and registry, DB-free cache builders for IAM/folder state, signal handlers wired in AppConfig to invalidate/bump caches on m2m changes, model/view updates to consume caches, and a migration adding a CacheVersion table. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(235,243,255,0.7)
participant Signal as m2m Signal Handler
participant Registry as CacheRegistry
participant Store as VersionStore
participant DB as Database
Note over Signal,DB: m2m_changed → cache invalidation triggers a version bump
end
Signal->>Registry: invalidate(key)
activate Registry
Registry->>Store: bump(key)
activate Store
Store->>DB: SELECT ... FOR UPDATE (cache_version row)
DB-->>Store: current_version
Store->>DB: UPDATE cache_version SET version = current_version + 1
DB-->>Store: new_version
Store-->>Registry: new_version
deactivate Store
Registry->>Registry: clear local snapshot for key
Registry-->>Signal: invalidation complete
deactivate Registry
sequenceDiagram
rect rgba(250,245,235,0.7)
participant Request as Hydration Caller
participant Registry as CacheRegistry
participant Store as VersionStore
participant Cache as VersionedSnapshotCache
participant Builder as Builder Function
participant DB as Database
Note over Request,Builder: hydrate_all → ensure versions, then build per-cache snapshots
end
Request->>Registry: hydrate_all(force_reload?)
activate Registry
Registry->>Store: ensure_and_get_versions(keys)
activate Store
Store->>DB: SELECT key, version FROM cache_version
DB-->>Store: versions
Store-->>Registry: VersionSnapshot
deactivate Store
par per-registered-key
Registry->>Cache: get(versions, force_reload)
activate Cache
alt version changed or force_reload
Cache->>Builder: build snapshot (builder may query DB)
activate Builder
Builder-->>Cache: snapshot_value
deactivate Builder
Cache->>Cache: store snapshot
end
Cache-->>Registry: snapshot_value
deactivate Cache
end
Registry-->>Request: { key: snapshot_value, ... }
deactivate Registry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
add missing imports
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/iam/models.py (1)
982-1023: Return type mismatch: returns UUIDs, notFolderobjects.The method signature declares
-> list[Folder]but the implementation returns a list ofuuid.UUID(line 1021 appendsfolder_id, notfolder_obj).Fix: Either update the return type or return Folder objects
Option 1: Fix return type annotation
def get_accessible_folders( folder: Folder, user: User, content_type: Folder.ContentType, codename: str = "view_folder", - ) -> list[Folder]: + ) -> list[uuid.UUID]:Option 2: Return Folder objects (matches original behavior)
- result: list[uuid.UUID] = [] + result: list[Folder] = [] for folder_id in accessible_ids: folder_obj = state.folders[folder_id] if content_type and folder_obj.content_type != content_type: continue if folder_id in perimeter_ids: - result.append(folder_id) + result.append(folder_obj)
🧹 Nitpick comments (3)
backend/iam/snapshot_cache.py (1)
102-132: Thread-safety consideration for process-local snapshot.In multi-threaded WSGI deployments, concurrent requests may race on
self._snapshotreads/writes. While Python's GIL provides some protection, the read-check-write pattern inget()(lines 123-132) isn't atomic. A request could read a stale snapshot reference while another thread is rebuilding.For most Django deployments this is low-risk (worst case: one extra rebuild), but consider adding a threading lock if strict consistency is required.
Optional: Add a lock for thread-safe access
+import threading + class VersionedSnapshotCache(Generic[T]): def __init__(self, *, key: str, builder: Callable[[], T]): self.key = key self._builder = builder self._snapshot: Optional[_Snapshot[T]] = None + self._lock = threading.Lock() def get(self, versions: Mapping[str, int], *, force_reload: bool = False) -> T: # ... validation ... v = int(versions[self.key]) - if ( - not force_reload - and self._snapshot is not None - and self._snapshot.version == v - ): - return self._snapshot.value - - value = self._builder() - self._snapshot = _Snapshot(version=v, value=value) - return value + with self._lock: + if ( + not force_reload + and self._snapshot is not None + and self._snapshot.version == v + ): + return self._snapshot.value + + value = self._builder() + self._snapshot = _Snapshot(version=v, value=value) + return valuebackend/iam/cache_builders.py (2)
338-344: Redundant registration patterns.The file performs import-time registration (lines 341-344) AND provides
register_all_caches()for explicit registration. SinceCacheRegistry.register()is a no-op for existing keys, this is safe but potentially confusing. Consider documenting which pattern is canonical or removing one.The comment on lines 338-340 suggests choosing one approach — following through on that would reduce confusion.
347-367: Each accessor triggers a fullhydrate_all()call.When a request needs multiple cache states (e.g.,
get_folder_state()+get_roles_state()+get_assignments_state()), each call invokeshydrate_all(), which:
- Calls
ensure_keys()(DB query if not cached)- Fetches all versions via
get_versions()- Potentially rebuilds all registered caches
Consider either:
- Caching the version snapshot per-request (e.g., using Django's request or thread-local storage)
- Providing a batch accessor that returns multiple states at once
For now this is acceptable since
ensure_keysis mostly a no-op and version checks are cheap when snapshots are fresh.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/iam/apps.pybackend/iam/cache_builders.pybackend/iam/migrations/0017_cacheversion.pybackend/iam/models.pybackend/iam/snapshot_cache.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (3)
backend/iam/cache_builders.py (1)
backend/iam/snapshot_cache.py (6)
CacheRegistry(153-227)get(107-132)invalidate(134-147)invalidate(215-216)register(163-180)hydrate_all(183-205)
backend/iam/apps.py (2)
backend/iam/cache_builders.py (2)
invalidate_groups_cache(247-248)invalidate_assignments_cache(301-302)backend/iam/models.py (1)
User(533-827)
backend/iam/models.py (1)
backend/iam/cache_builders.py (11)
get_folder_state(347-352)get_roles_state(355-357)get_groups_state(360-362)get_assignments_state(365-367)get_parent_folders_cached(164-170)get_folder_path(173-184)invalidate_folders_cache(118-119)invalidate_roles_cache(218-219)invalidate_groups_cache(247-248)invalidate_assignments_cache(301-302)iter_descendant_ids(141-154)
🔇 Additional comments (17)
backend/iam/migrations/0017_cacheversion.py (1)
1-27: LGTM!The migration correctly creates the
CacheVersionmodel that backs the versioned snapshot cache system. Using aCharFieldas primary key for cache keys andPositiveBigIntegerFieldfor version counters is appropriate.backend/iam/snapshot_cache.py (3)
26-41: LGTM!The
CacheVersionmodel provides a simple and effective schema for tracking cache versions. Using the cache key as the primary key ensures uniqueness and efficient lookups.
76-85: Atomic version bump implementation is correct.The use of
select_for_update()withtransaction.atomic()ensures serializable updates. TheF("version") + 1pattern followed byrefresh_from_db()correctly retrieves the new value.
153-227: LGTM!The
CacheRegistryprovides a clean API for managing multiple versioned caches. Thehydrate_all()method efficiently batches version checks into a single query, and the registration is safely idempotent.backend/iam/cache_builders.py (4)
69-115: LGTM!The folder cache builder efficiently constructs an immutable tree representation. The recursive
_depth()function uses memoization to avoid redundant computation. UsingMappingProxyTypeenforces immutability at the API level.
140-154: LGTM!The iterative DFS implementation in
iter_descendant_idsavoids recursion limits and provides stable ordering viareversed()on children.
195-215: LGTM!The roles cache builder efficiently prefetches permissions and stores them as frozen sets keyed by role ID. This avoids N+1 queries when checking role permissions.
267-298: LGTM!The assignments cache builder creates lightweight
AssignmentLiteobjects that capture only the fields needed for permission checks. This minimizes memory usage while enabling efficient lookups.backend/iam/models.py (9)
151-159: LGTM!Cache invalidation is correctly placed after
super().save()/super().delete(), ensuring the cache is invalidated only after the database change succeeds.
161-168: LGTM!The folder traversal methods now leverage the cached tree structure, avoiding repeated database queries during permission checks.
411-421: LGTM!Invalidating both groups and assignments caches when a
UserGroupchanges is correct, since role assignments reference groups.
858-879: LGTM!The helper efficiently collects assignments from both direct user assignments and group-based assignments using only cached data.
921-964: LGTM!The permission check now uses cached state for folder ancestry, role permissions, and assignments. The logic correctly walks up the folder tree to check if any ancestor is in the assignment's perimeter.
1025-1174: LGTM!The method correctly uses cached state for permission resolution while still querying the database for actual object IDs. The logic for published object inheritance is preserved from the original implementation.
1182-1225: LGTM!The
get_permissionsmethod efficiently uses cached role permissions and only hits the database once at the end to fetch permission display names.
1247-1295: LGTM!The
get_permissions_per_foldermethod correctly handles both explicit perimeter folders and recursive expansion to descendants whenrecursive=True.
1298-1306: LGTM!The
FolderDisplayContextdataclass provides a clean, immutable structure for folder rendering context.
bump cache on m2m change Reduce db calls in get_accessible_object_ids
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/iam/apps.py (1)
1-14: Remove duplicate class definition.Lines 1-6 duplicate lines 9-14, creating two identical
IamConfigclass definitions in the same file. This appears to be a merge error or copy-paste mistake.🔎 Proposed fix
-from django.apps import AppConfig - - -class IamConfig(AppConfig): - default_auto_field = "django.db.models.BigAutoField" - name = "iam" - - from django.apps import AppConfig class IamConfig(AppConfig): default_auto_field = "django.db.models.BigAutoField" name = "iam" def ready(self):
♻️ Duplicate comments (1)
backend/iam/apps.py (1)
16-48: Missing signal handler forRole.permissionsM2M changes remains unaddressed.The previous review correctly identified that
Role.permissionsM2M changes aren't being invalidated. WhileRoleAssignment.perimeter_folderswas added (lines 33-48),Role.permissionsstill lacks a handler.When
Role.permissionsis modified via.add()/.remove()/.clear(), theroles_cache(which stores role→permission mappings percache_builders.pylines 195-220) will become stale, causing permission checks to use outdated data.🔎 Proposed fix
Add after line 23:
from iam.cache_builders import ( invalidate_groups_cache, invalidate_assignments_cache, + invalidate_roles_cache, )Add after line 26:
User = apps.get_model("iam", "User") RoleAssignment = apps.get_model("iam", "RoleAssignment") +Role = apps.get_model("iam", "Role")Add after line 35:
def _ra_perimeters_changed(sender, instance, action, **kwargs): if action in {"post_add", "post_remove", "post_clear"}: invalidate_assignments_cache() +def _role_permissions_changed(sender, instance, action, **kwargs): + if action in {"post_add", "post_remove", "post_clear"}: + invalidate_roles_cache()Add after line 48:
m2m_changed.connect( _ra_perimeters_changed, sender=RoleAssignment.perimeter_folders.through, dispatch_uid="iam.roleassignment.perimeter_folders.m2m.invalidate_assignments_cache", weak=False, ) +m2m_changed.connect( + _role_permissions_changed, + sender=Role.permissions.through, + dispatch_uid="iam.role.permissions.m2m.invalidate_roles_cache", + weak=False, +)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/iam/apps.pybackend/iam/models.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (2)
backend/iam/apps.py (2)
backend/iam/cache_builders.py (2)
invalidate_groups_cache(247-248)invalidate_assignments_cache(301-302)backend/iam/models.py (2)
User(533-827)RoleAssignment(882-1292)
backend/iam/models.py (1)
backend/iam/cache_builders.py (12)
get_folder_state(347-352)get_roles_state(355-357)get_groups_state(360-362)get_assignments_state(365-367)get_sub_folders_cached(157-161)get_parent_folders_cached(164-170)get_folder_path(173-184)invalidate_folders_cache(118-119)invalidate_roles_cache(218-219)invalidate_groups_cache(247-248)invalidate_assignments_cache(301-302)iter_descendant_ids(141-154)
⏰ 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). (9)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (13)
backend/iam/models.py (13)
4-64: LGTM!The import additions properly support the cache-driven refactor, including cache state accessors, cache helpers, and invalidation functions.
109-176: LGTM!The Folder model changes correctly:
- Return
Optional[uuid.UUID]forget_root_folder_id()to handle missing root- Invalidate
folders_cacheon save/delete to maintain cache coherence- Delegate navigation methods to cached implementations while preserving the public API
411-421: LGTM!UserGroup save/delete correctly invalidate both
groups_cacheandassignments_cache, since role assignments reference user groups.
840-848: Approve with dependency on apps.py fix.Role save/delete correctly invalidate
roles_cache. However, cache coherence also depends on the missingRole.permissionsM2M signal handler flagged inapps.py.
858-879: LGTM!The
_iter_assignment_lites_for_userhelper efficiently yields assignments using caches only, properly handling unauthenticated users and missing IDs.
896-904: LGTM!RoleAssignment save/delete correctly invalidate
assignments_cache. The M2M fieldperimeter_foldersis handled by the signal handler inapps.py.
922-964: LGTM!The
is_access_allowedrefactor correctly uses cached state for:
- Role permissions from
roles_state- Assignments from
_iter_assignment_lites_for_user- Folder ancestry from cached
parent_mapThe folder ancestry walk (lines 957-962) correctly checks if the target folder or any ancestor is in the assignment's perimeter.
983-1023: LGTM!The
get_accessible_foldersrefactor correctly:
- Computes the perimeter using cached folder tree traversal
- Expands recursive assignment perimeters to include descendants
- Returns only folders matching the content_type within the perimeter
1026-1171: LGTM!The
get_accessible_object_idsrefactor significantly reduces DB calls by:
- Computing folder→permissions mappings entirely from caches (lines 1074-1110)
- Fetching object IDs only for folders with permissions (lines 1117-1144)
- Collecting all ancestor folder IDs first, then executing a single query for published objects (lines 1149-1169)
The published inheritance logic correctly excludes ENCLAVE folders and uses the cached parent map for ancestry traversal.
1180-1222: LGTM!The
get_permissionsrefactor correctly:
- Uses cached assignments (via helper for User, or by_group for UserGroup)
- Aggregates role permissions from
roles_state- Preserves the original output format with a single DB query for permission names
1225-1242: LGTM!The
has_rolerefactor correctly checks role membership using cached assignments from_iter_assignment_lites_for_user.
1245-1292: LGTM!The new
get_permissions_per_foldermethod correctly:
- Groups permissions by folder ID using cached state
- Propagates to descendants only when both
recursiveparameter andassignment.is_recursiveare True (lines 1275-1279)- Handles both User and UserGroup principals
1295-1303: LGTM!The
FolderDisplayContextdataclass is well-designed as an immutable, memory-efficient container for folder rendering context.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/iam/apps.py (1)
1-14: Remove duplicate class definition.Lines 1-6 define
IamConfig, and lines 9-14 redefine the same class. Python will only use the second definition, making lines 1-6 dead code. This appears to be a merge artifact or copy-paste error.🔎 Proposed fix
-from django.apps import AppConfig - - -class IamConfig(AppConfig): - default_auto_field = "django.db.models.BigAutoField" - name = "iam" - - from django.apps import AppConfig
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/iam/apps.pybackend/iam/models.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (2)
backend/iam/apps.py (2)
backend/iam/cache_builders.py (3)
invalidate_groups_cache(247-248)invalidate_assignments_cache(301-302)invalidate_roles_cache(218-219)backend/iam/models.py (2)
RoleAssignment(882-1293)Role(830-855)
backend/iam/models.py (1)
backend/iam/cache_builders.py (5)
get_folder_state(347-352)get_roles_state(355-357)get_groups_state(360-362)get_assignments_state(365-367)iter_descendant_ids(141-154)
⏰ 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). (9)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
backend/iam/apps.py (1)
16-60: M2M signal handlers correctly implemented.The
ready()method properly wires up all three M2M relationships to invalidate the appropriate caches:
User.user_groupsinvalidates groups and assignmentsRoleAssignment.perimeter_foldersinvalidates assignmentsRole.permissionsinvalidates rolesThe handlers use correct action filters (
post_add,post_remove,post_clear), unique dispatch UIDs, andweak=Falseto prevent premature garbage collection. This addresses the gap identified in the past review comment.backend/iam/models.py (7)
4-64: LGTM! Cache infrastructure imports properly integrated.The new imports support the cache refactoring:
from __future__ import annotationsenables modern type syntax- Cache builder functions for state access, invalidation, and tree traversal
- Additional typing imports for improved type annotations
109-175: LGTM! Folder cache integration correctly implemented.The changes properly integrate caching:
get_root_folder_id()provides a clean ID accessor used as a default valuesave()/delete()invalidate the folders cache after successful DB operations- Folder traversal methods (
get_sub_folders,get_parent_folders,get_folder_full_path) now use cached implementationsThe pattern of invalidating caches after the DB operation ensures consistency—if the save/delete fails, the cache remains valid.
411-421: LGTM! UserGroup cache invalidation correctly handles dependencies.Both
save()anddelete()properly invalidate both the groups cache and the assignments cache, reflecting thatRoleAssignmententities referenceUserGroupinstances. This is consistent with the M2M signal handlers inapps.py.
840-848: LGTM! Role cache invalidation correctly implemented.The
save()anddelete()methods properly invalidate the roles cache after successful DB operations. This aligns with the M2M signal handler forRole.permissionsinapps.py.
858-880: LGTM! Helper function correctly aggregates user assignments from caches.
_iter_assignment_lites_for_user()properly:
- Guards against unauthenticated or ID-less users
- Retrieves user assignments directly and via group membership
- Uses cached state exclusively (no DB queries)
- Returns an iterator for memory efficiency
896-1293: LGTM! Permission methods successfully refactored to use caching.The extensive refactoring of
RoleAssignmentmethods correctly integrates the cache system:
save()/delete(): Invalidate assignments cache after DB operationsis_access_allowed(): Uses cached folder ancestry and role permissions; walks the parent tree efficientlyget_accessible_folders(): Leverages cached tree traversal withiter_descendant_idsget_accessible_object_ids(): Resolves permissions via caches, queries DB only for object IDsget_permissions(): Aggregates permissions from cached assignments, hydrates names in one DB queryhas_role(): Simple cache-based role membership checkget_permissions_per_folder(): Computes folder-level permissions using caches, correctly handles recursive assignmentsThe refactoring maintains correctness while moving IAM resolution logic to in-memory caches.
1296-1304: LGTM! Clean data container for folder display context.
FolderDisplayContextis properly defined as a frozen dataclass with slots for immutability and memory efficiency. The fields provide comprehensive path information for UI rendering.
eric-intuitem
force-pushed
the
folders_cache
branch
from
36d188a to
8e14d74
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/iam/cache_builders.py (2)
103-112: Recursive depth computation could stack overflow on very deep folder trees.The
_depthfunction uses recursion with memoization. While unlikely for typical folder hierarchies, extremely deep trees could hit Python's recursion limit. Consider an iterative approach if folder depth could exceed ~1000 levels.Iterative alternative
def compute_depths(parent_map: Dict[uuid.UUID, Optional[uuid.UUID]]) -> Dict[uuid.UUID, int]: depth_map: Dict[uuid.UUID, int] = {} for folder_id in parent_map: depth = 0 current = folder_id ancestors = [] while current is not None and current not in depth_map: ancestors.append(current) current = parent_map.get(current) base = depth_map.get(current, -1) + 1 if current else 0 for i, ancestor in enumerate(reversed(ancestors)): depth_map[ancestor] = base + i return depth_map
346-352: Duplicate registration at import time and via function.Caches are registered both at import time (lines 349-352) and in
register_all_caches(). Ifregister_all_caches(allow_replace=False)is called after import, it may fail or duplicate. Consider removing import-time registration and relying solely onregister_all_caches()called fromAppConfig.ready(), or guard the import-time calls with a module-level flag.Suggested approach: single registration path
-# Import-time registration (DB-free). -# If you prefer explicit registration, delete these lines and call register_all_caches() -# in iam/apps.py -> IamConfig.ready(). -CacheRegistry.register(FOLDER_CACHE_KEY, build_folder_cache_state) -CacheRegistry.register(IAM_ROLES_KEY, build_roles_cache_state) -CacheRegistry.register(IAM_GROUPS_KEY, build_groups_cache_state) -CacheRegistry.register(IAM_ASSIGNMENTS_KEY, build_assignments_cache_state) +# Registration happens in iam/apps.py -> IamConfig.ready() via register_all_caches()backend/iam/models.py (1)
1021-1030: Redundant check in the result filtering.Line 1027 checks
if folder_id in perimeter_ids, butfolder_idcomes fromaccessible_idswhich is already the intersection ofperimeter_ids & ra_perimeter_ids. This check is always true and can be removed.Simplified filtering
# Filter by content_type and keep only within perimeter_ids result: list[uuid.UUID] = [] for folder_id in accessible_ids: folder_obj = state.folders[folder_id] if content_type and folder_obj.content_type != content_type: continue - if folder_id in perimeter_ids: - result.append(folder_id) + result.append(folder_id) return result
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/iam/cache_builders.pybackend/iam/models.pybackend/iam/views.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/views.pybackend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/views.pybackend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (2)
backend/iam/views.py (1)
backend/iam/models.py (1)
get_accessible_folder_ids(989-1030)
backend/iam/models.py (1)
backend/iam/cache_builders.py (12)
get_folder_state(355-360)get_roles_state(363-365)get_groups_state(368-370)get_assignments_state(373-375)get_sub_folders_cached(165-169)get_parent_folders_cached(172-178)get_folder_path(181-192)invalidate_folders_cache(126-127)invalidate_roles_cache(226-227)invalidate_groups_cache(255-256)invalidate_assignments_cache(309-310)iter_descendant_ids(149-162)
🪛 GitHub Actions: Backend Linters
backend/iam/cache_builders.py
[error] 1-1: ruff format check failed. 1 file would be reformatted. Run 'ruff format' to fix code style issues.
⏰ 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). (8)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (17)
backend/iam/cache_builders.py (5)
181-192: LGTM!The path logic correctly includes the requested folder while optionally excluding the root. The edge case where only the root exists is handled by returning it (the requested folder itself).
203-223: LGTM!Good use of
Prefetchto avoid N+1 queries, and proper immutability withfrozensetandMappingProxyType.
239-252: LGTM!Efficient direct query on the M2M through table. The
type: ignoreis acceptable for Django's dynamically generated through models.
275-306: LGTM!Efficient prefetching of
perimeter_foldersand clean separation of user vs group assignments into lightweightAssignmentLitestructures.
355-375: LGTM!The state accessors provide a clean API. Note that calling multiple accessors sequentially will invoke
hydrate_all()multiple times, but assumingCacheRegistryhandles this efficiently (returns cached result), this is acceptable.backend/iam/views.py (1)
214-216: LGTM!The switch to
get_accessible_folder_idsaligns with the cache-driven refactor. The returned UUIDs are correctly serialized to strings on line 238.backend/iam/models.py (11)
104-113: LGTM!Good defensive approach with fallback to direct ORM query when cache is unavailable (e.g., during migrations or early startup).
157-165: LGTM!Correct pattern: persist changes first via
super(), then invalidate the cache.
417-427: LGTM!Correctly invalidates both
groups_cacheandassignments_cachesince role assignments reference groups.
864-885: LGTM!Clean helper that aggregates assignments from both direct and group-based sources using cached state only. Good early return for unauthenticated users.
928-970: LGTM!Permission check correctly uses cached state for role permissions and folder ancestry. The special case for
add_filteringlabelis preserved. Parent traversal viastate.parent_mapis efficient.
1065-1070: Early return if any of the three permissions is missing.This returns empty results if
view_,change_, ordelete_permissions don't all exist in the database for this model. For non-standard models with partial permissions, this could unexpectedly return no results. Verify this is the intended behavior.
1186-1229: LGTM!Correctly uses cached role permissions for resolution while maintaining the original output format. The single DB query for permission names is an acceptable tradeoff.
1251-1299: LGTM!Clean implementation using a nested
apply_assignmentfunction. The recursive permission propagation viaiter_descendant_idsis efficient. Output format (string folder_id keys) maintains API compatibility.
1302-1310: LGTM!Clean dataclass for folder display context. Immutable by design with
frozen=Trueand memory-efficient withslots=True.
846-854: M2M signal handlers for Role.permissions are already in place—no action needed.The concern is unfounded. The codebase already registers a
_role_permissions_changedhandler inapps.py(lines 39-41) that connects toRole.permissions.throughand correctly callsinvalidate_roles_cache()onpost_add,post_remove, andpost_clearactions.Likely an incorrect or invalid review comment.
902-910: The M2M signal handlers forperimeter_foldersare properly registered inapps.py. The_ra_perimeters_changedhandler (line 35–37) callsinvalidate_assignments_cache()for all relevant M2M actions (post_add,post_remove,post_clear). M2M changes are correctly handled through signals, so no issue exists here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/iam/models.py (1)
1022-1030: Minor: redundant perimeter check.The check
if folder_id in perimeter_idson line 1027 is redundant sinceaccessible_idsis already the intersection ofperimeter_idsand role assignment perimeters.🔎 Proposed simplification
result: list[uuid.UUID] = [] for folder_id in accessible_ids: folder_obj = state.folders[folder_id] if content_type and folder_obj.content_type != content_type: continue - if folder_id in perimeter_ids: - result.append(folder_id) + result.append(folder_id) return result
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/iam/cache_builders.pybackend/iam/models.pybackend/iam/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/iam/views.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (1)
backend/iam/models.py (1)
backend/iam/cache_builders.py (12)
get_folder_state(352-357)get_roles_state(360-362)get_groups_state(365-367)get_assignments_state(370-372)get_sub_folders_cached(162-166)get_parent_folders_cached(169-175)get_folder_path(178-189)invalidate_folders_cache(123-124)invalidate_roles_cache(223-224)invalidate_groups_cache(252-253)invalidate_assignments_cache(306-307)iter_descendant_ids(146-159)
⏰ 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). (6)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
🔇 Additional comments (20)
backend/iam/models.py (11)
51-64: LGTM on cache imports.The imports from
iam.cache_buildersare well-organized and provide all necessary cache state accessors, invalidation functions, and tree navigation helpers.
104-113: Cache-first with DB fallback is a good resilience pattern.The fallback to
_get_root_folder()ensures the system works during initial startup or if the cache isn't yet hydrated. This is well-designed.
157-165: Cache invalidation on save/delete looks correct.The invalidation happens after the database operation completes. One consideration: if wrapped in a transaction that later rolls back, the cache will still be invalidated. This is generally acceptable (causes a cache rebuild on next access) but worth noting.
417-427: LGTM - dual cache invalidation is correct.Invalidating both groups and assignments caches when a UserGroup changes is appropriate since assignments reference groups.
864-886: LGTM - well-structured helper for iterating user assignments.The function correctly handles anonymous/unauthenticated users and combines both direct user assignments and assignments via group membership using cached state.
927-970: Cache-based permission check implementation looks correct.The logic correctly:
- Checks authentication
- Iterates user assignments (direct and via groups)
- Checks role permissions via cached state
- Walks folder ancestry via
parent_mapto find matching perimeterThe special case for
add_filteringlabel(line 958-959) preserving the original behavior is good.
1032-1178: LGTM - comprehensive cache-based object access resolution.The implementation correctly:
- Resolves permissions via cached role/assignment state
- Minimizes DB queries to just fetching object IDs
- Handles all object type patterns (folder, risk_assessment, entity, provider_entity)
- Preserves published object inheritance logic
- Uses sets for efficient deduplication
1186-1229: LGTM - efficient permission retrieval.The implementation collects permission codenames via cached state and then performs a single DB query to fetch the human-readable names. The output format
{codename: {"str": name}}is preserved for backwards compatibility.
1251-1299: LGTM - cache-based permissions per folder.The implementation correctly handles permission aggregation per folder with proper support for recursive assignments. Using
str(folder_id)as dictionary keys ensures JSON serialization compatibility.
1302-1310: LGTM - clean immutable data container.
FolderDisplayContextis well-designed withfrozen=Truefor immutability andslots=Truefor memory efficiency.
167-174: Generator return type is compatible with all existing callers.
get_sub_foldersandget_parent_foldersnow return generators instead of lists. All usages in the codebase are compatible with this change—they use generator-compatible patterns like iteration, list comprehensions, theinoperator, and set updates. No callers rely on list-specific operations likelen(), indexing, or slicing. The data-model.md file does not document these methods, so no documentation update is needed.Likely an incorrect or invalid review comment.
backend/iam/cache_builders.py (9)
1-18: Good module documentation.The docstring clearly explains the design goals: no circular imports, DB-free registration, immutable snapshots, and the expectation of manual invalidation.
70-121: LGTM - well-structured folder cache builder.The implementation:
- Uses
apps.get_model()to avoid circular imports- Builds efficient lookup structures (parent_map, children_map, depth_map)
- Uses
MappingProxyTypefor immutability- Sorts children by name for stable traversal order
The recursive
_depthfunction is fine for typical folder hierarchies which are shallow.
146-159: LGTM - iterative DFS avoids recursion limits.The stack-based implementation handles arbitrarily deep trees without stack overflow. Using
reversed()withstack.pop()maintains the natural ordering fromchildren_map.
200-220: LGTM - efficient role permissions cache.Uses prefetch to minimize queries and builds an immutable mapping of role IDs to permission codenames.
236-249: LGTM - direct through-table access is efficient.Querying the M2M through table directly (
User.user_groups.through) is more efficient than iterating users with prefetched groups.
272-303: LGTM - assignment cache builder handles all cases.The implementation correctly handles assignments that may have a user, a user_group, or both, storing them in the appropriate lookup dictionaries.
352-372: LGTM - consistent state accessor pattern.All accessors delegate to
CacheRegistry.hydrate_all()and return the specific cache. This is fine assuminghydrate_all()uses an in-memory cache and doesn't rebuild on every call.
375-403: LGTM - comprehensive exports.The
__all__list appropriately includes all public APIs while keeping internal helpers likeensure_cached_folderandpath_ids_from_rootprivate.
313-349: No issue with duplicate registration. TheCacheRegistry.register()method is explicitly designed to handle multiple registrations gracefully: whenallow_replace=False(the default), it returns early as a no-op if the key already exists (seesnapshot_cache.pylines 177-178). Both import-time registration (lines 346-349) and callingregister_all_caches()later can safely coexist—the second call will simply be a harmless no-op. The method's docstring confirms it is "Safe to call multiple times."Likely an incorrect or invalid review comment.
new fix attempt
to avoid early db calls
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/iam/apps.py (1)
1-14: Critical: Remove duplicate class definition.Lines 1-6 contain the old
IamConfigclass definition, and lines 9-14 redefine it. Python will ignore the first definition, making lines 1-6 dead code. This appears to be a merge artifact or incomplete replacement.Remove lines 1-8 entirely, keeping only the version starting at line 9.
🔎 Proposed fix
-from django.apps import AppConfig - - -class IamConfig(AppConfig): - default_auto_field = "django.db.models.BigAutoField" - name = "iam" - - from django.apps import AppConfig class IamConfig(AppConfig):
♻️ Duplicate comments (1)
backend/iam/cache_builders.py (1)
1-1: Formatting issue remains unresolved.The pipeline failure indicates
ruff formatcheck failed. Please runruff format backend/iam/cache_builders.pyto resolve this before merging.
🧹 Nitpick comments (2)
backend/iam/cache_builders.py (2)
61-77: Consider thread-safe cache readiness flag.The global
cache_readyflag lacks synchronization. While Python bool operations are atomic and Django's app initialization is single-threaded, there's a theoretical race during theFalse→Truetransition where a thread might read stale state and raiseCacheNotReadyErrorunnecessarily.For increased robustness, consider using
threading.Lockaround the flag or Python'sthreading.Event. However, given Django's threading model, the current implementation is likely safe in practice.🔎 Optional refactor using threading.Event
-cache_ready: bool = False +import threading +_cache_ready_event = threading.Event() def set_cache_ready(*, ready: bool = True) -> None: """Allow callers to toggle whether caches may touch the DB.""" - global cache_ready - cache_ready = bool(ready) + if ready: + _cache_ready_event.set() + else: + _cache_ready_event.clear() def is_cache_ready() -> bool: - return cache_ready + return _cache_ready_event.is_set() def _ensure_cache_ready() -> None: - if not cache_ready: + if not _cache_ready_event.is_set(): raise CacheNotReadyError("IAM caches are not ready to run DB queries")
335-371: Backward-compatible registration pattern works but could be clearer.The dual registration approach (import-time at lines 368-371 and optional explicit via
register_all_caches()) is intentional for backward compatibility. However, the interaction between them could be confusing for maintainers:
- Import-time registration (lines 368-371) runs without
allow_replace.- Later calls to
register_all_caches()try withallow_replace, falling back to without.- Per
CacheRegistry.register()behavior, no-ops if key exists andallow_replace=False.This works correctly but adds cognitive overhead. Consider adding a comment explaining why both patterns exist and when each is used.
📝 Suggested clarifying comment
# Import-time registration (DB-free). -# If you prefer explicit registration, delete these lines and call register_all_caches() -# in iam/apps.py -> IamConfig.ready(). +# These calls ensure caches are registered even if apps.py ready() hasn't run yet. +# If register_all_caches() is later called from apps.py with allow_replace=True, +# these registrations will be replaced. If allow_replace is not supported, +# register() no-ops when keys already exist. CacheRegistry.register(FOLDER_CACHE_KEY, build_folder_cache_state)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/iam/apps.pybackend/iam/cache_builders.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/iam/apps.py (2)
backend/iam/cache_builders.py (4)
invalidate_groups_cache(274-275)invalidate_assignments_cache(328-329)invalidate_roles_cache(245-246)set_cache_ready(64-67)backend/iam/models.py (3)
User(539-833)RoleAssignment(888-1299)Role(836-861)
backend/iam/cache_builders.py (3)
backend/iam/snapshot_cache.py (6)
CacheRegistry(153-227)get(107-132)invalidate(134-147)invalidate(215-216)register(163-180)hydrate_all(183-205)backend/iam/models.py (7)
Folder(97-347)ContentType(119-124)Role(836-861)permissions(430-431)permissions(777-778)User(539-833)RoleAssignment(888-1299)backend/iam/apps.py (1)
ready(16-71)
⏰ 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). (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
backend/iam/cache_builders.py (7)
92-142: LGTM with minor observations.The folder cache builder correctly constructs an immutable snapshot with efficient querying (select_related, only) and stable ordering. The depth computation uses memoization to avoid redundant work.
Minor notes:
- Line 113 assumes a single root folder (first encountered with
ContentType.ROOT). If multiple root folders exist, only one is cached. Verify this aligns with your data model constraints.- The recursive depth computation could theoretically hit Python's recursion limit for extremely deep hierarchies (1000+ levels), though this is unlikely for organizational folders.
217-246: LGTM!The roles cache builder efficiently prefetches permissions and constructs an immutable mapping. The defensive check
if p.codenameon line 239 correctly filters out any null/empty codenames.
253-275: LGTM!The groups cache builder efficiently queries the M2M through table and constructs an immutable user→groups mapping.
281-329: LGTM!The assignments cache builder correctly handles both user-based and group-based role assignments, efficiently prefetches M2M relations, and constructs immutable snapshots.
374-398: LGTM with performance note already flagged.The state getter functions correctly enforce cache readiness before hydrating. The
force_reloadparameter allows callers to bypass version checks when needed. Note that each call to these functions triggersCacheRegistry.hydrate_all(), which was flagged earlier for potential DB overhead.
401-432: LGTM!The
__all__list comprehensively exports the module's public API, making the interface explicit and discoverable.
184-211: Multiplehydrate_all()calls trigger repeated version queries.Each call to
get_sub_folders_cached(),get_parent_folders_cached(), orget_folder_path()invokesget_folder_state(), which callsCacheRegistry.hydrate_all(). This in turn executesVersionStore.get_versions()to fetch all CacheVersion rows:rows = CacheVersion.objects.all().values_list("key", "version")While the snapshot is reused if the version hasn't changed (avoiding expensive rebuilds), the version-check query still runs on every call. Usage patterns show these helpers are sometimes called multiple times per request context (e.g., list comprehensions in
core/helpers.py:1874-1875).That said, the CacheVersion table is small (4 rows) and the query is read-only, so practical impact depends on database query caching. Consider whether this warrants optimization, or profile actual request patterns first.
backend/iam/apps.py (1)
16-71: LGTM!The
ready()method correctly wires M2M signal handlers to invalidate the appropriate caches and sets cache readiness after migrations complete. The approach of importing insideready()avoids import-time issues, and usingweak=Falsewith uniquedispatch_uidvalues ensures the handlers remain registered.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/iam/apps.py (1)
1-14: Remove duplicate class definition.The
IamConfigclass is defined twice (lines 1-6 and lines 9-14). This duplication will cause a syntax error or unexpected behavior. Remove the first definition (lines 1-6) and keep only the complete definition starting at line 9.🔎 Proposed fix
-from django.apps import AppConfig - - -class IamConfig(AppConfig): - default_auto_field = "django.db.models.BigAutoField" - name = "iam" - - from django.apps import AppConfig class IamConfig(AppConfig): default_auto_field = "django.db.models.BigAutoField" name = "iam"
🧹 Nitpick comments (1)
backend/iam/cache_builders.py (1)
383-390: Clarify the dual registration strategy.Import-time registration (lines 386-389) always occurs, while
register_all_caches()provides an explicit alternative withallow_replacesupport. The comment on line 384 suggests these are mutually exclusive, but both are active. If this dual-path design is intentional (import-time for convenience, explicit for re-registration), clarify the comment to explain when each should be used.🔎 Suggested clarification
-# Import-time registration (DB-free). -# If you prefer explicit registration, delete these lines and call register_all_caches() -# in iam/apps.py -> IamConfig.ready(). +# Import-time registration (DB-free) for default convenience. +# Call register_all_caches(allow_replace=True) in iam/apps.py if you need to +# re-register or override caches at runtime. CacheRegistry.register(FOLDER_CACHE_KEY, build_folder_cache_state) CacheRegistry.register(IAM_ROLES_KEY, build_roles_cache_state) CacheRegistry.register(IAM_GROUPS_KEY, build_groups_cache_state) CacheRegistry.register(IAM_ASSIGNMENTS_KEY, build_assignments_cache_state)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/iam/apps.pybackend/iam/cache_builders.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/iam/apps.py (2)
backend/iam/cache_builders.py (3)
invalidate_groups_cache(292-293)invalidate_assignments_cache(346-347)invalidate_roles_cache(263-264)backend/iam/models.py (2)
RoleAssignment(888-1299)Role(836-861)
⏰ 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). (9)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
backend/iam/apps.py (1)
16-60: LGTM! Signal handlers correctly wire M2M changes to cache invalidation.The
ready()method properly:
- Uses
apps.get_modelto avoid circular imports- Defines handlers for all M2M relationships that affect cached state
- Filters for mutating actions (
post_add,post_remove,post_clear)- Uses unique
dispatch_uidvalues to prevent duplicate registrations- Sets
weak=Falseto ensure handlers persistThe invalidation logic is sound: user_groups changes invalidate both groups and assignments caches, perimeter_folders changes invalidate assignments, and role permissions changes invalidate roles.
backend/iam/cache_builders.py (4)
110-160: LGTM! Cache builders follow consistent patterns and properly construct immutable snapshots.All four cache builders (
build_folder_cache_state,build_roles_cache_state,build_groups_cache_state,build_assignments_cache_state) correctly:
- Use
apps.get_modelto avoid circular imports- Apply appropriate
select_related/prefetch_relatedoptimizations- Build mutable structures first, then convert to immutable proxies
- Return frozen dataclasses with
MappingProxyType,frozenset, andtupleThe folder builder's recursive depth calculation and stable child ordering are well-designed.
Also applies to: 240-260, 276-289, 312-343
392-417: LGTM! State accessors provide clean, consistent API with proper readiness checks.All accessor functions (
get_folder_state,get_roles_state,get_groups_state,get_assignments_state) follow a uniform pattern:
- Call
_ensure_cache_ready()to prevent DB queries during migrations- Delegate to
CacheRegistry.hydrate_all()withforce_reloadsupport- Return the specific state snapshot
The design provides a clean public API for cache consumers.
167-229: LGTM! Navigation helpers provide efficient cache-based folder traversal.The helper functions correctly implement common folder navigation patterns:
ensure_cached_foldervalidates existence with clear error messagespath_ids_from_rootbuilds ancestor paths via parent chain traversaliter_descendant_idsuses stack-based DFS (avoids recursion depth limits)- Public APIs (
get_sub_folders_cached,get_parent_folders_cached,get_folder_path) provide intuitive interfacesThe iterative DFS approach in
iter_descendant_idsis well-suited for potentially deep folder hierarchies.
59-95: LGTM! Cache readiness infrastructure properly guards against premature DB access.The readiness system correctly:
- Maintains a global
cache_readyflag to skip checks after initialization- Implements
_cache_tables_ready()to detect whetherCacheVersiontable exists- Provides
_ensure_cache_ready()that lazily enables caching when tables are available- Raises
CacheNotReadyErrorduring migrations/startup to prevent DB queriesThe lazy initialization pattern is appropriate for Django's lifecycle, and the minor thread-safety consideration (redundant checks during startup) is acceptable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/iam/cache_builders.py (1)
359-379: Narrow the exception handling scope.The
try/except TypeErrorblock catches anyTypeErrorfrom the builder functions or registry, not just missingallow_replaceparameter. This was flagged in a previous review.
🧹 Nitpick comments (3)
backend/iam/cache_builders.py (3)
266-269: Remove internal developer note from code.Line 268 contains a leftover conversation note (
"NOTE: you said you'll manage M2M later...") that should be removed or converted to a proper documentation comment explaining the M2M handling strategy.🔎 Proposed fix
# -------------------------------------------------------------------- # IAM Groups cache: user_id -> group_ids (M2M) -# NOTE: you said you'll manage M2M later; keeping the builder here anyway. # --------------------------------------------------------------------
382-388: Redundant registration at import-time whenregister_all_caches()exists.Lines 385-388 register caches at import time, but
register_all_caches()(lines 352-379) provides the same functionality with additionalallow_replacesupport. This creates two registration paths that could lead to confusion or double-registration issues ifCacheRegistrydoesn't handle duplicates gracefully.Consider removing import-time registration and documenting that
register_all_caches()should be called inIamConfig.ready(), or keep only import-time registration and removeregister_all_caches().
391-415: Multiple DB queries when calling different state getters—consider memoization for efficiency.Each
get_*_state()function callsCacheRegistry.hydrate_all(), which executes a DB query to fetch all cache versions. Whilecache.get()does short-circuit when versions are unchanged (lines 123–128), calling different state getters sequentially triggers multiple redundant DB queries. For example, callingget_folder_state()thenget_roles_state()results in two DB queries even though the first call already fetched all versions.Consider memoizing
hydrate_all()results or providing individual cache hydration methods if frequent sequential calls to different state getters are common in your codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/cache_builders.py
⏰ 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). (9)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
Chasing db calls
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/iam/models.py (1)
371-375: Same bug asget_folder_path: returns root wheninclude_root=False.This method has the same logic issue as the cached version—when the path contains only the root folder and
include_root=False, it returns[root]instead of[].🔎 Proposed fix
def get_folder_full_path(self, *, include_root: bool = False) -> list[Folder]: folders = ([self.folder] + [f for f in self.folder.get_parent_folders()])[::-1] if include_root: return folders - return folders[1:] if len(folders) > 1 else folders + return folders[1:]
♻️ Duplicate comments (3)
backend/iam/cache_builders.py (3)
1-17: Update outdated module docstring regarding signal-based invalidation.The docstring omits the signal-based invalidation strategy. Per the AI summary,
backend/iam/apps.pyimplementsm2m_changedsignal handlers that automatically invalidate caches. Update line 11 to reflect this.
217-228: Bug:get_folder_pathreturns root folder wheninclude_root=Falsefor root folder.When
folder_idis the root andinclude_root=False, the function returns[root_folder]instead of[]. The condition on line 228 returnsfolderswhenlen(folders) <= 1, which includes the root.
361-388: Narrow the exception handling scope.The
try/except TypeErrorblock is too broad—it will catchTypeErrorraised by incorrect arguments to the builder functions, not just a missingallow_replaceparameter.
🧹 Nitpick comments (5)
backend/iam/cache_builders.py (2)
139-148: Consider adding cycle detection to prevent stack overflow on corrupted data.The recursive
_depthfunction will cause aRecursionErrorif the folder hierarchy contains a cycle (e.g., corrupted data where folder A's parent is folder B, and B's parent is A). While cycles should be prevented at the model level, defensive coding could log an error and break instead.🔎 Proposed fix with cycle detection
def _depth(current_id: uuid.UUID) -> int: if current_id in depth_map: return depth_map[current_id] parent_id = parent_map[current_id] - depth_val = 0 if parent_id is None else _depth(parent_id) + 1 + if parent_id is None: + depth_val = 0 + elif parent_id == current_id: + # Self-referential - treat as root + depth_val = 0 + else: + depth_val = _depth(parent_id) + 1 depth_map[current_id] = depth_val return depth_val
400-424: Add explicit return type annotations for better type safety.The state accessor functions return typed dataclasses but lack return type annotations. This could cause type checker issues when consumers rely on the specific state types.
🔎 Proposed fix
-def get_folder_state(*, force_reload: bool = False) -> FolderCacheState: +def get_folder_state(*, force_reload: bool = False) -> FolderCacheState: """ Convenience accessor for folder cache state. """ _ensure_cache_ready() state_map = CacheRegistry.hydrate_all(force_reload=force_reload) - return state_map[FOLDER_CACHE_KEY] + return state_map[FOLDER_CACHE_KEY] # type: ignore[return-value]The functions already have return type annotations, but the dictionary lookup returns
object. Consider adding a type-narrowing cast or# type: ignoreto suppress type checker warnings.backend/iam/snapshot_cache.py (2)
242-247: Clear version cache state inclear()for complete reset.The
clear()method resets_cachesbut leaves_last_versionsand_last_fetched_atintact. For a clean test reset, these should also be cleared.🔎 Proposed fix
@classmethod def clear(cls) -> None: """ Useful for tests: clears registered caches. """ cls._caches.clear() + cls._last_versions = None + cls._last_fetched_at = None
85-94: Theselect_for_update().get_or_create()pattern is an anti-pattern, but safe here due to the PRIMARY KEY constraint.The combination of
select_for_update()withget_or_create()is a known Django anti-pattern (Django ticket #29499) becauseselect_for_update()only locks existing rows—concurrentget_or_create()calls can still race on the insert. However, this code is defensible: thekeyfield is a PRIMARY KEY, so the database enforces uniqueness. When concurrent calls race to insert, one succeeds and the other triggers anIntegrityError, which Django'sget_or_create()handles by retrying the get. This makes the code safe in practice.That said, the pattern is not ideal for clarity. A cleaner approach would explicitly handle the IntegrityError or use a separate existence check—but the current code works correctly.
backend/iam/models.py (1)
1031-1039: Minor: Redundant membership check.Line 1036 checks
if folder_id in perimeter_ids, butaccessible_idsis already computed asperimeter_ids & ra_perimeter_ids, so all IDs inaccessible_idsare guaranteed to be inperimeter_ids.🔎 Proposed simplification
# Filter by content_type and keep only within perimeter_ids result: list[uuid.UUID] = [] for folder_id in accessible_ids: folder_obj = state.folders[folder_id] if content_type and folder_obj.content_type != content_type: continue - if folder_id in perimeter_ids: - result.append(folder_id) + result.append(folder_id) return result
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/ciso_assistant/settings.pybackend/core/views.pybackend/iam/cache_builders.pybackend/iam/models.pybackend/iam/snapshot_cache.py
✅ Files skipped from review due to trivial changes (1)
- backend/ciso_assistant/settings.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/cache_builders.pybackend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (1)
backend/iam/cache_builders.py (1)
backend/iam/snapshot_cache.py (7)
CacheRegistry(161-247)CacheVersion(27-42)get(116-140)invalidate(142-155)invalidate(235-236)register(174-191)hydrate_all(194-225)
⏰ 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). (9)
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (18)
backend/core/views.py (1)
660-664: Dynamic related-object optimization inget_querysetlooks goodConditionally adding
select_related("parent_folder")andprefetch_related("filtering_labels")based on model fields is safe and should reduce N+1 queries for models that use these relations, without impacting others.backend/iam/cache_builders.py (3)
62-93: LGTM!The cache readiness logic is well-structured with a two-phase check: first the fast in-memory flag, then the DB table introspection. The graceful fallback for
OperationalError/ProgrammingErrorduring migrations is appropriate.
240-268: LGTM!The roles cache correctly builds both the role-to-permissions mapping and a complete permission ID lookup table. The separate
Permission.objects.all()query is necessary since some permissions may not be assigned to any role yet.
320-351: LGTM!The assignments cache correctly handles both user-direct and group-based role assignments. The prefetch optimization for perimeter folders minimizes DB queries during hydration.
backend/iam/snapshot_cache.py (3)
27-42: LGTM!The
CacheVersionmodel is well-designed with a simple schema. Using the cache key as the primary key ensures uniqueness and efficient lookups.
58-83: LGTM!The version store correctly handles concurrent access:
bulk_createwithignore_conflicts=Truegracefully handles race conditions when multiple processes create the same key.- The re-query after insert ensures correct version values are returned.
106-158: LGTM!The
VersionedSnapshotCachecorrectly implements version-based invalidation. The lack of thread locking is acceptable here since:
- Builders are idempotent (same input → same output)
- Snapshots are immutable once created
- Worst case is redundant rebuilds, not incorrect data
backend/iam/models.py (11)
51-66: LGTM!The cache utility imports are well-organized and avoid circular import issues through the TYPE_CHECKING pattern in
cache_builders.py.
106-122: LGTM with a minor observation.The graceful fallback to DB queries when the cache is unavailable is appropriate for reliability. The broad
except Exceptionon line 112 is acceptable here since logging captures the details and the fallback is safe.
166-174: LGTM!The save/delete hooks correctly persist changes before invalidating the cache. The
invalidate()method insnapshot_cache.pygracefully handles DB errors during migrations.
426-436: LGTM!Invalidating both the groups and assignments caches on UserGroup changes is correct since role assignments reference user groups.
873-894: LGTM!The helper correctly aggregates role assignments from both direct user assignments and indirect group memberships, using cached state exclusively.
960-979: Verifyis_recursivehandling in permission check.The code checks if the folder or its ancestors are in
perimeter_folder_ids, but doesn't expand recursive assignments to include descendant folders. If an assignment hasis_recursive=Trueandperimeter_folder_ids={root_id}, a user should have access to child folders, but this code only walks up the tree.Actually, on re-reading: walking up from the target folder to check if any ancestor is in the perimeter is the correct approach for recursive permissions—if the perimeter is a parent of the target folder, access is granted.
The ancestor walk correctly handles recursive semantics: if
is_recursive=Trueand a perimeter folder is an ancestor of the target folder, access is granted.
1078-1123: LGTM!The cache-based permission computation correctly:
- Expands recursive assignment perimeters to descendants
- Intersects with the requested perimeter scope
- Builds per-folder permission sets efficiently
The separation of cache reads (permissions) from DB reads (object IDs) is a good optimization pattern.
1191-1234: LGTM!The permissions retrieval correctly uses cached role-to-codename mappings while still fetching human-readable names from the DB. The backward-compatible return format
{codename: {"str": name}}is preserved.
1236-1254: LGTM!The cache-based role check is simple and efficient, correctly using the shared assignment iteration helper.
1256-1304: LGTM!The cache-based folder permission aggregation correctly handles both explicit perimeter folders and recursive expansion. Using
str(folder_id)as keys enables JSON serialization for API responses.
1307-1315: LGTM!The
FolderDisplayContextdataclass is well-designed with immutable fields and clear purpose for UI rendering context.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/iam/models.py (1)
90-96: Replace bareexcept:with specific exception types.The bare
except:clause catches all exceptions, includingSystemExit,KeyboardInterrupt, and other critical exceptions that should not be suppressed. This can make debugging difficult and may hide serious issues.🔎 Proposed fix
def _get_root_folder(): """helper function outside of class to facilitate serialization to be used only in Folder class""" try: return Folder.objects.get(content_type=Folder.ContentType.ROOT) - except: + except Folder.DoesNotExist: return None
🧹 Nitpick comments (1)
backend/iam/models.py (1)
937-1300: Consider adding integration tests for cache-driven permission logic.The permission resolution logic has been completely rewritten to use caches. Given the security-critical nature of IAM code, ensure comprehensive integration tests cover:
- Cache warmup and invalidation scenarios
- Permission checks across folder hierarchies
- Recursive vs. non-recursive role assignments
- Edge cases like cache failures and fallback paths
- Published object inheritance
- Anonymous and authenticated user scenarios
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/models.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (1)
backend/iam/models.py (1)
backend/iam/cache_builders.py (13)
CacheNotReadyError(58-59)FolderCacheState(100-106)get_folder_state(400-406)get_roles_state(409-412)get_groups_state(415-418)get_assignments_state(421-424)get_sub_folders_cached(201-205)get_parent_folders_cached(208-214)get_folder_path(217-228)invalidate_folders_cache(162-163)invalidate_roles_cache(271-272)invalidate_groups_cache(300-301)invalidate_assignments_cache(354-355)
⏰ 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). (9)
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
backend/iam/models.py (3)
1303-1312: Well-designed dataclass for folder display context.The use of
frozen=Trueandslots=Trueis excellent for an immutable, memory-efficient data structure. This provides a clear API contract for folder UI rendering.
166-174: Cache invalidation for m2m relationships is properly handled via signal handlers.M2m relationship changes (e.g.,
RoleAssignment.perimeter_folders,Role.permissions,User.user_groups) are already covered by Django signal handlers configured inbackend/iam/apps.py. These handlers invalidate the appropriate caches when relationships are modified via.add(),.remove(),.clear(), or.set()operations.
1038-1179: No action required. Special Permission filtering bycontent_type.app_labelis preserved in the refactored code at the serializer layer (backend/core/serializers.py), not inget_accessible_object_ids(). Theget_accessible_object_ids()method is not designed to be called with Permission objects; Permission access control happens through serializer-level filters that enforce the ALLOWED_PERMISSION_APPS constraint, maintaining the security properties described in the learning.Likely an incorrect or invalid review comment.
restore permissions query for get_accessible_objects_ids
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/iam/models.py (2)
937-976: Cache-based permission logic looks correct, but lacks error handling.The implementation correctly:
- Iterates user assignments (direct + via groups)
- Checks role permissions from cache
- Walks folder ancestry via cached parent map
- Handles special case for
add_filteringlabelHowever, if any cache operation fails, the entire method will raise an exception, potentially denying all access. Consider adding error handling with safe defaults (deny access on cache failure).
🔎 Suggested defensive error handling
+ try: + state = get_folder_state() + roles_state = get_roles_state() + except Exception: + logger.exception("Cache failure in is_access_allowed, denying access") + return False
1038-1194: Multiple issues: KeyError risk, complexity, and mixed cache/DB consistency.Issue 1: KeyError risk
Line 1178 accessesstate.folders[folder_id]without checking existence, similar to the previous method.Issue 2: Cache/DB consistency
The method reads folder permissions from cache (lines 1074-1110) but then queries the database for object IDs (lines 1140-1192). If cache is stale or DB has been updated after cache load, the results may be inconsistent.Issue 3: Method complexity
This 150+ line method handles multiple concerns: permission computation, object queries, published inheritance. Consider refactoring into smaller, focused methods.🔎 Quick fix for KeyError
- folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + continue
🧹 Nitpick comments (4)
backend/iam/models.py (4)
1203-1245: LGTM with minor observations.The cache-based implementation correctly:
- Distinguishes between User and UserGroup principals
- Uses cached role permissions for IAM logic
- Fetches permission names from DB only for display
Note: The output format
{codename: {"str": permission_name}}is unusual. If this is legacy compatibility, consider documenting it or refactoring to a simpler structure like{codename: permission_name}in a future change.
1268-1315: LGTM! Recursive permission propagation is correctly implemented.The method properly:
- Handles recursive assignments by walking descendants
- Uses cached folder tree for efficient traversal
- Distinguishes User vs UserGroup principals
Consider extracting the nested
apply_assignmentfunction to module level if it could be reused or tested independently.
1318-1326: Consider moving UI-related dataclass to serializers or views module.
FolderDisplayContextis well-designed (frozen, slots), but it appears to be UI/presentation-specific rather than a core model concern. Consider moving it toserializers.pyor a dedicateddisplay.pymodule for better separation of concerns.
176-190: Consider standardizing error handling for consistency withget_root_folder().These methods delegate to cache functions that call
get_folder_state(), which internally calls_ensure_cache_ready()and will raiseCacheNotReadyErrorif cache tables don't exist. While the lazy initialization approach provides implicit protection in normal deployments,get_root_folder()explicitly handles this case with a DB fallback—an inconsistency worth addressing:
get_root_folder()(lines 106–122): Explicitly catchesCacheNotReadyErrorand falls back to_get_root_folder()get_sub_folders(),get_parent_folders(),get_folder_full_path()(lines 176–190): No explicit error handlingFor consistency and defensive programming, either:
- Add matching try-except blocks with appropriate fallbacks
- Document that these methods require
_ensure_cache_ready()to pass (implicit expectation)- Consolidate error handling at the cache layer level
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/models.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
⏰ 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). (9)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
backend/iam/models.py (6)
51-66: LGTM! Cache integration imports are comprehensive.The imports cover all necessary cache operations: state getters, invalidation functions, and tree navigation helpers.
106-122: LGTM! Proper cache integration with fallback handling.The implementation correctly:
- Attempts cache lookup first for performance
- Falls back to DB query when cache isn't ready or fails
- Logs errors for observability
- Handles None cases safely
1248-1265: LGTM! Clean cache-based implementation.The logic correctly checks both direct and group-based role assignments. As with other methods, consider adding error handling for cache failures.
911-919: > Likely an incorrect or invalid review comment.
426-436: M2M cache invalidation is already implemented via signal handlers.The review comment incorrectly assumes that M2M changes to user_groups bypass cache invalidation. In fact,
backend/iam/apps.pyregisters am2m_changedsignal handler forUser.user_groups.throughthat explicitly callsinvalidate_groups_cache()andinvalidate_assignments_cache()when users are added, removed, or cleared from groups.The handler correctly covers all relevant actions (
post_add,post_remove,post_clear), so cached permissions are properly invalidated when users change group membership.Likely an incorrect or invalid review comment.
166-174: The review comment is based on incorrect assumptions about the codebase.Error handling IS present:
VersionedSnapshotCache.invalidate()(inbackend/iam/snapshot_cache.pylines 147-152) already wraps the version bump in a try-except block that catchesOperationalErrorandProgrammingError. On failure, it clears the local snapshot and returnsNonesafely. This prevents transaction rollback and allows the cache to rebuild on the next access.M2M signal handlers for
filtering_labelsare not needed: While it's true thatFolder.filtering_labelshas no M2M signal handlers, this is not a bug. The folder cache snapshot (built bybuild_folder_cache_state()) only storesid,name,parent_folder_id,content_type, andbuiltinfields. Thefiltering_labelsfield is intentionally not cached, so changes to it don't require cache invalidation.Likely an incorrect or invalid review comment.
get_root_folder is not deemed to return None anymore, as this occurs only in exceptional situation of initial migration, and we want to keep type checking for all other cases
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/iam/models.py (1)
942-981: CacheNotReadyError handling remains unaddressed.As noted in previous review comments, this permission-checking method can raise
CacheNotReadyErrorif caches aren't ready. The lack of fallback differs fromFolder.get_root_folder()which handles this gracefully. This remains a robustness concern during application startup or testing.
🧹 Nitpick comments (3)
backend/iam/models.py (3)
181-195: Consider adding CacheNotReadyError fallbacks for consistency.These folder navigation methods rely on cache helpers that can raise
CacheNotReadyError. While the cache should be ready during normal operations, adding fallbacks similar toget_root_folder()(lines 114-126) would make the behavior more predictable during edge cases like early startup or testing.Example fallback pattern
def get_sub_folders(self) -> Generator[Self, None, None]: """Return the list of subfolders through the cached tree.""" try: yield from get_sub_folders_cached(self.id) except CacheNotReadyError: # Fallback to DB query if cache not ready yield from Folder.objects.filter(parent_folder=self)
878-899: Add CacheNotReadyError fallback for robustness.This helper is called by
is_access_allowed,get_permissions,has_role, and other permission methods. Without error handling, aCacheNotReadyErrorfromget_assignments_state()orget_groups_state()will propagate to all callers. Consider adding a DB fallback similar toFolder.get_root_folder().🔎 Example fallback implementation
def _iter_assignment_lites_for_user(user: AbstractBaseUser | AnonymousUser): """ Yield AssignmentLite for a user, including via groups, using caches only. Returns empty iterator for AnonymousUser / unauthenticated. """ if not getattr(user, "is_authenticated", False) or not getattr(user, "id", None): return iter(()) + try: assignments_state = get_assignments_state() groups_state = get_groups_state() + except CacheNotReadyError: + # Fallback: query DB for user's direct assignments and group assignments + from collections import namedtuple + AssignmentLite = namedtuple('AssignmentLite', ['role_id', 'perimeter_folder_ids', 'is_recursive']) + + direct_qs = RoleAssignment.objects.filter(user=user).prefetch_related('perimeter_folders') + group_qs = RoleAssignment.objects.filter(user_group__user=user).prefetch_related('perimeter_folders') + + result = [] + for ra in direct_qs.union(group_qs): + result.append(AssignmentLite( + role_id=ra.role_id, + perimeter_folder_ids=[f.id for f in ra.perimeter_folders.all()], + is_recursive=ra.is_recursive + )) + return iter(result) user_id = user.id group_ids = groups_state.user_group_ids.get(user_id, frozenset())Based on learnings, the codebase uses a custom permission system accessed via
RoleAssignmentmethods.
999-1042: Defensive check recommended for cache consistency robustness.Line 1036 accesses
state.folders[folder_id]directly. While the input folder is now type-guaranteed to be non-null (commit 94ed9db), and the cache is built from all database Folders, a defensive access pattern would improve resilience against cache inconsistencies. This is especially relevant sinceget_sub_folders_cached(line 189 in cache_builders.py) uses this defensive pattern: checking key existence before access.for folder_id in accessible_ids: - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + continue
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/models.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (1)
backend/iam/models.py (2)
backend/iam/cache_builders.py (14)
CacheNotReadyError(58-59)FolderCacheState(91-97)get_folder_state(389-395)get_roles_state(398-401)get_groups_state(404-407)get_assignments_state(410-413)get_sub_folders_cached(187-192)get_parent_folders_cached(195-202)get_folder_path(205-216)invalidate_folders_cache(153-154)invalidate_roles_cache(259-260)invalidate_groups_cache(288-289)invalidate_assignments_cache(343-344)iter_descendant_ids(171-184)backend/iam/views.py (2)
get(154-162)get(203-243)
⏰ 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). (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
backend/iam/models.py (11)
91-102: LGTM!The explicit
Folder | Nonereturn type and comprehensive exception handling properly support the migration/initialization fallback path. The docstring clearly explains whenNoneis expected.
171-179: LGTM!Cache invalidation is correctly placed after the database mutation in both
saveanddeletemethods.
224-252: LGTM!Raising
RuntimeErrorwhen folder resolution fails (line 252) is clearer and more debuggable than returningNone. This makes the failure explicit rather than allowing it to propagate as an AttributeError downstream.
431-441: LGTM!Correctly invalidates both
groups_cacheandassignments_cachesince role assignments reference user groups. The inline comment clearly explains the dependency.
860-868: LGTM!Cache invalidation is properly placed and consistent with the pattern used throughout the file.
916-924: LGTM!Cache invalidation is correctly implemented.
1176-1199: Same cache consistency concern applies here.Line 1185 also performs direct dictionary access to
state.folders[folder_id]. The same defensive pattern suggested forget_accessible_folder_idswould apply here.
1210-1252: LGTM!The cache-based implementation correctly handles both
UserandUserGroupprincipals, preserving the original output format. The single DB query for permission names (line 1243) is reasonable since it's retrieving display strings, not IAM logic.
1255-1272: LGTM!Clean, straightforward implementation leveraging the cached assignment iterator.
1275-1322: LGTM!The cache-based implementation correctly handles recursive permission propagation through folder hierarchies. The nested
apply_assignmentfunction keeps the logic clean and avoids duplication between user and group principal handling.
1325-1333: LGTM!Well-designed immutable dataclass for folder UI context. The use of
frozen=Trueandslots=Trueis appropriate for this display-oriented data structure.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
backend/iam/cache_builders.py (3)
1-17: Update the docstring to reflect automatic signal-based invalidation.Line 11 states "Manual invalidation is expected (no signals)," but
backend/iam/apps.pyimplementsm2m_changedsignal handlers that automatically invalidate caches forUser.user_groups,RoleAssignment.perimeter_folders, andRole.permissions.🔎 Proposed update
Design goals: - No circular imports: DO NOT import iam.models at runtime. - Import-time registration is DB-free (only CacheRegistry.register calls). - Actual DB work happens only when CacheRegistry.hydrate_all() rebuilds a snapshot. - Snapshots are immutable-ish via MappingProxyType + frozenset/tuple. -- Manual invalidation is expected (no signals). Be careful with M2M changes. +- M2M signal handlers in iam/apps.py automatically invalidate caches. +- Model save/delete methods call invalidate_*_cache() directly.
211-222: Fix:get_folder_pathreturns root folder when it should return empty list.When
folder_idis the root andinclude_root=False, line 222 returns[root_folder]instead of[]. The conditionif len(folders) > 1fails when folders contains only the root, causing it to return the root folder even though the caller explicitly excluded it.🔎 Proposed fix
def get_folder_path( folder_id: uuid.UUID, *, include_root: bool = False ) -> List["Folder"]: """ Return the ordered list of folders from root to the requested folder. """ state = get_folder_state() ids = path_ids_from_root(state, folder_id) folders = [state.folders[fid] for fid in ids] if include_root: return folders - return folders[1:] if len(folders) > 1 else folders + return folders[1:]
356-383: Narrow the exception handling scope.The
try/except TypeErrorblock (lines 363-378) is too broad and will catchTypeErrorraised by incorrect arguments to the builder functions or other unexpected issues, not just a missingallow_replaceparameter inCacheRegistry.register.Consider checking the signature before calling, or catching a more specific exception.
backend/iam/models.py (3)
890-914: Consider adding CacheNotReadyError handling for consistency.This function calls
get_assignments_state()andget_groups_state()which raiseCacheNotReadyErrorif caches aren't ready. UnlikeFolder.get_root_folder()which has a fallback, permission-checking methods will fail during early startup.For consistency, consider adding a try/except fallback to DB queries during migrations/startup.
115-132: Type annotation doesn't reflect None possibility during migrations.Line 116 declares
-> "Folder"but line 124 can returnNonefrom_get_root_folder()during initial migrations. Thecastandtype: ignoresuppress this, but calling code may not handleNonesafely.Consider either:
- Update return type to
"Folder" | Noneand handle None in callers- Add runtime assertion:
if folder is None: raise RuntimeError("Root folder not initialized")
1050-1059: Potential KeyError if folder is missing from cache.Line 1053 accesses
state.folders[folder_id]without checking if the key exists. If the cache is incomplete oraccessible_idscontains a folder_id not in the cache state, this will raiseKeyError.Add a defensive check:
🔎 Suggested fix
result: list[uuid.UUID] = [] for folder_id in accessible_ids: - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + continue if content_type and folder_obj.content_type != content_type: continue if folder_id in perimeter_ids: result.append(folder_id)
🧹 Nitpick comments (2)
backend/core/views.py (2)
616-616: Type annotation onmodelis fine, but consider aligning related type hintsDeclaring
model: type[models.Model] | None = Nonematches the actual usage (a model class or None). However,BaseModelViewSet.get_querysetstill returnsNonewhenself.modelis falsy while its return type is annotated as justQuerySet, which is slightly inconsistent for type‑checkers. If you care about strict typing, consider either:
- Updating the return annotation to
QuerySet | None, or- Raising in the
if not self.modelbranch instead of returningNone.
660-665: Dynamicselect_related/prefetch_relatedis safe; optionally cache field namesThe new field‑introspection is a nice, generic way to avoid hard‑coding per‑model optimizations and is guarded so it only touches models that actually have
parent_folder/filtering_labels. It’s also compatible with subclasses that further refine the queryset.If this turns into a hot path, you might optionally cache the computed
field_namesper model (e.g., on the class or via a small LRU) to avoid callingself.model._meta.get_fields()on every request, but that’s purely a micro‑optimization.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/core/views.pybackend/iam/apps.pybackend/iam/cache_builders.pybackend/iam/models.pybackend/iam/sso/errors.pybackend/iam/sso/oidc/views.pybackend/iam/sso/saml/defaults.pybackend/iam/tests/test_models.pybackend/iam/urls.pybackend/mypy.ini
✅ Files skipped from review due to trivial changes (1)
- backend/iam/sso/oidc/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/iam/apps.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/cache_builders.pybackend/iam/models.py
📚 Learning: 2025-12-26T12:14:38.283Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 3095
File: backend/iam/utils.py:4-4
Timestamp: 2025-12-26T12:14:38.283Z
Learning: In django-allauth 65.13.1 or later, the correct import path for SessionTokenStrategy is `from allauth.headless.tokens.strategies.sessions import SessionTokenStrategy` (with the `strategies` submodule), not `from allauth.headless.tokens.sessions import SessionTokenStrategy`. The old path may emit deprecation warnings.
Applied to files:
backend/iam/sso/errors.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
🧬 Code graph analysis (3)
backend/iam/cache_builders.py (2)
backend/iam/snapshot_cache.py (7)
CacheRegistry(161-247)CacheVersion(27-42)get(116-140)invalidate(142-155)invalidate(235-236)register(174-191)hydrate_all(194-225)backend/iam/models.py (7)
Folder(109-367)ContentType(139-144)Role(862-887)permissions(450-451)permissions(807-808)User(562-859)RoleAssignment(917-1341)
backend/iam/tests/test_models.py (1)
backend/iam/models.py (1)
Folder(109-367)
backend/core/views.py (2)
backend/ebios_rm/models.py (1)
name(1080-1085)backend/metrology/serializers.py (1)
get_fields(181-187)
⏰ 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). (9)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (12)
backend/iam/urls.py (1)
1-1: LGTM!The
type: ignore[import-untyped]directive appropriately suppresses mypy warnings for the knox library, which lacks type stubs.backend/iam/sso/saml/defaults.py (1)
1-3: LGTM! Type annotation cleanup.The multi-line import format with
type: ignore[import-untyped]appropriately suppresses mypy warnings for the untyped django-allauth import. No functional changes.backend/iam/sso/errors.py (1)
1-3: LGTM! Type annotation cleanup.The multi-line import format with
type: ignore[import-untyped]appropriately suppresses mypy warnings for the untyped django-allauth import. No functional changes.backend/mypy.ini (1)
2-4: Configuration strategy is sound.The consolidation of plugins and new top-level settings (
ignore_missing_imports = False,show_error_codes = True) establish a strict, informative type-checking baseline. This is a sensible approach for a codebase expanding IAM-related type coverage.backend/iam/tests/test_models.py (1)
4-4: LGTM: Import simplification aligns with cache refactor.The direct import of
Folderis appropriate and simplifies the test dependencies.backend/iam/models.py (7)
56-71: LGTM: Cache integration imports are well-structured.The use of
TYPE_CHECKINGguards and the comprehensive cache builder imports properly support the cache-driven IAM implementation.
177-185: LGTM: Cache invalidation on save/delete ensures consistency.The cache invalidation hooks properly maintain cache coherence when folder data changes.
187-201: LGTM: Folder traversal methods properly use cached data.The delegation to cache-backed implementations is clean and maintains the same API while improving performance.
437-447: LGTM: Correct cache invalidation for dependent caches.The dual invalidation (groups + assignments) is necessary because
RoleAssignmentreferences user groups. The comment on line 440 helpfully explains this dependency.
1228-1271: LGTM: Efficient cache-backed permission retrieval.The implementation correctly uses cached state for IAM logic and only queries the database for
Permissionnames, which is a good optimization.
1273-1341: LGTM: Cache-backed role and permission methods are well-implemented.Both
has_roleandget_permissions_per_foldercorrectly leverage cached state for efficient permission resolution.
1344-1352: LGTM: Clean dataclass for folder UI context.The
FolderDisplayContextdataclass is well-structured with appropriate use offrozen=Trueandslots=Truefor immutability and memory efficiency.
simplify all_register
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/iam/cache_builders.py (2)
1-1: Fix formatting to pass CI.The pipeline indicates
ruff formatcheck failed. Runruff format backend/iam/cache_builders.pyto resolve the formatting issues and ensure CI passes.
211-222: Bug:get_folder_pathreturns root folder wheninclude_root=Falsefor root folder.When
folder_idis the root andinclude_root=False, the function returns[root_folder]instead of[]. The condition on line 222 returns the fullfolderslist whenlen(folders) <= 1, which includes the root even though the caller explicitly requested to exclude it.🔎 Proposed fix
def get_folder_path( folder_id: uuid.UUID, *, include_root: bool = False ) -> List["Folder"]: """ Return the ordered list of folders from root to the requested folder. """ state = get_folder_state() ids = path_ids_from_root(state, folder_id) folders = [state.folders[fid] for fid in ids] if include_root: return folders - return folders[1:] if len(folders) > 1 else folders + return folders[1:]
🧹 Nitpick comments (1)
backend/iam/cache_builders.py (1)
376-382: Consider removing duplicate import-time registration.The import-time registration (lines 379-382) duplicates the functionality of
register_all_caches()(lines 356-374). The comment suggests choosing one approach. Since the explicitregister_all_caches()inAppConfig.ready()is more conventional and flexible, consider removing the import-time registration and callingregister_all_caches()fromiam/apps.pyinstead.🔎 Proposed refactor
Remove import-time registration:
-# Import-time registration (DB-free). -# If you prefer explicit registration, delete these lines and call register_all_caches() -# in iam/apps.py -> IamConfig.ready(). -CacheRegistry.register(FOLDER_CACHE_KEY, build_folder_cache_state) -CacheRegistry.register(IAM_ROLES_KEY, build_roles_cache_state) -CacheRegistry.register(IAM_GROUPS_KEY, build_groups_cache_state) -CacheRegistry.register(IAM_ASSIGNMENTS_KEY, build_assignments_cache_state)Then call from
backend/iam/apps.py:from iam.cache_builders import register_all_caches class IamConfig(AppConfig): def ready(self): # ... existing code ... register_all_caches()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/cache_builders.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/cache_builders.py
🪛 GitHub Actions: Backend Linters
backend/iam/cache_builders.py
[error] 1-1: ruff format --check . failed: 1 file would be reformatted (iam/cache_builders.py). Run 'ruff format iam/cache_builders.py' or fix formatting to pass the check.
⏰ 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). (9)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
backend/iam/cache_builders.py (9)
66-86: LGTM!The cache readiness checks properly detect whether required tables exist before attempting cache hydration, preventing crashes during early application startup or before migrations run.
101-156: LGTM!The folder cache builder correctly constructs an immutable snapshot with optimized queries, stable sorting for deterministic traversal, and proper depth tracking. The recursive depth computation is acceptable for typical folder hierarchy depths.
163-174: LGTM!The path resolution correctly walks the parent chain and reverses to produce root-to-folder ordering, with appropriate error handling for uncached folders.
177-190: LGTM!The iterative depth-first traversal avoids recursion limits and preserves stable ordering by reversing the already-sorted children list.
193-208: LGTM!Both helper functions correctly leverage the cached tree structure with appropriate error handling for uncached folders.
234-262: LGTM!The roles cache builder efficiently prefetches permissions and constructs immutable mappings with appropriate null filtering.
278-291: LGTM!The groups cache builder efficiently queries the M2M through table and constructs an immutable user-to-groups mapping.
314-346: LGTM!The assignments cache builder efficiently prefetches perimeter folders and correctly constructs immutable mappings for both user-based and group-based assignments.
385-409: LGTM!The state accessor functions follow a consistent pattern with proper readiness checks and cache hydration, providing type-safe access to cached state.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/pyproject.toml
🧰 Additional context used
🪛 GitHub Actions: Backend code coverage
backend/pyproject.toml
[error] 1-1: poetry install failed: pyproject.toml changed significantly since poetry.lock was last generated. Run 'poetry lock' to fix the lock file.
🪛 GitHub Actions: Backend migrations check
backend/pyproject.toml
[error] 1-1: poetry install failed: pyproject.toml changed significantly since poetry.lock was last generated. Run 'poetry lock' to fix the lock file.
⏰ 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). (5)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/pyproject.toml (1)
57-57: The mypy constraint is reasonable and requires no changes.The semantic versioning constraint
^1.9.0permits versions up to 1.19.1 (the latest stable release as of December 2025), and there are no published security advisories for mypy. No action needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/iam/models.py (4)
1196-1216: Potential KeyError in published inheritance logic.Line 1202 accesses
state.folders[folder_id]without checking existence. Iffolder_perm_codescontains a folder_id not in the cache state, this will raiseKeyError.🔎 Suggested defensive fix
for folder_id, perms in folder_perm_codes.items(): if view_code not in perms: continue - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + logger.warning("Folder missing from cache during published inheritance", folder_id=str(folder_id)) + continue if folder_obj.content_type == Folder.ContentType.ENCLAVE: continue parent_id = state.parent_map.get(folder_id) while parent_id: + if parent_id not in state.folders: + break ancestor_ids.add(parent_id) parent_id = state.parent_map.get(parent_id)
474-493: Critical: M2M relationship set before instance is saved.Line 492 calls
user.user_groups.set(...)before line 493user.save(...). Django M2M relationships require the instance to have a primary key before M2M fields can be set, which will raiseValueError.🔎 Proposed fix
user = cast( "User", self.model( email=email, first_name=extra_fields.get("first_name", ""), last_name=extra_fields.get("last_name", ""), is_superuser=extra_fields.get("is_superuser", False), is_active=extra_fields.get("is_active", True), observation=extra_fields.get("observation"), folder=_get_root_folder(), keep_local_login=extra_fields.get("keep_local_login", False), expiry_date=extra_fields.get("expiry_date"), ), ) if password: user.password = make_password(password) else: user.set_unusable_password() - user.user_groups.set(extra_fields.get("user_groups", [])) user.save(using=self._db) + user.user_groups.set(extra_fields.get("user_groups", [])) if initial_group: initial_group.user_set.add(user)
1016-1059: Potential KeyError if cached folder data is incomplete.Line 1053 accesses
state.folders[folder_id]without checking if the key exists. If the cache is partially loaded or inconsistent, this will raiseKeyErrorand cause the method to fail.🔎 Suggested defensive fix
result: list[uuid.UUID] = [] for folder_id in accessible_ids: - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + logger.warning("Folder missing from cache", folder_id=str(folder_id)) + continue if content_type and folder_obj.content_type != content_type: continue if folder_id in perimeter_ids: result.append(folder_id)
116-132: Type inconsistency: return type doesn't account for None during migrations.The method signature declares
-> "Folder"but line 123-124 can returnNonefrom_get_root_folder()during initial migrations (cast withtype: ignoresuppresses the error). While the docstring in_get_root_folderexplains this occurs only "before IAM tables/migrations are ready," the type system doesn't capture this constraint, potentially causing issues in calling code that assumes a non-None return.🔎 Suggested fix options
Option 1: Make the return type explicit:
@staticmethod - def get_root_folder() -> "Folder": + def get_root_folder() -> "Folder | None": """class function for general use"""Option 2: Add a runtime guard if None should never occur in production:
except CacheNotReadyError: # During initial migrations the cache cannot hydrate yet; fall back to the # direct lookup which may still be None until the schema is ready. folder = _get_root_folder() - return cast("Folder", folder) # type: ignore[return-value] + if folder is None: + raise RuntimeError("Root folder not yet initialized; database schema may not be ready") + return folder
🧹 Nitpick comments (1)
backend/iam/models.py (1)
890-914: Add defensive error handling forCacheNotReadyErrorin cache-backed helper or verify initialization assumptions.The helper calls
get_assignments_state()andget_groups_state()which can raiseCacheNotReadyErrorif caches are not ready, but has no error handling. While the docstring states it uses "caches only" (suggesting intentional design to work only when caches are ready), note that all direct callers—is_access_allowed,has_role,get_permissions—also call cache getters directly without error handling.If these methods can be invoked before cache initialization (as suggested by the
Folder.get_root_folder()pattern at lines 116-124), add try/except with fallback DB queries. Otherwise, verify that application initialization guarantees caches are ready before these permission methods are called, which would make this a non-issue.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
backend/iam/models.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-09-19T08:44:33.744Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/views.py:3753-3762
Timestamp: 2025-09-19T08:44:33.744Z
Learning: In the domain export/import functionality in backend/core/views.py, Evidence.owner M2M relationships are intentionally not set during import because User objects are not exported. The owner_ids are collected in _process_model_relationships only to prevent M2M field validation errors during import, but they are deliberately not applied in _set_many_to_many_relations since users are environment-specific.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
⏰ 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). (9)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/iam/cache_builders.py (2)
1-17: Update outdated comment about signals.Line 11 describes the invalidation strategy but does not mention the signal-based invalidation. According to past reviews and the AI summary,
backend/iam/apps.pyimplementsm2m_changedsignal handlers that automatically invalidate caches forUser.user_groups,RoleAssignment.perimeter_folders, andRole.permissions. Update the comment to reflect this.🔎 Proposed fix
Design goals: - No circular imports: DO NOT import iam.models at runtime. - Import-time registration is DB-free (only CacheRegistry.register calls). - Actual DB work happens only when CacheRegistry.hydrate_all() rebuilds a snapshot. - Snapshots are immutable-ish via MappingProxyType + frozenset/tuple. +- M2M signal handlers in iam/apps.py automatically invalidate caches. +- Model save/delete methods call invalidate_*_cache() directly.
211-222: Bug:get_folder_pathreturns root folder wheninclude_root=Falsefor root folder.When
folder_idis the root andinclude_root=False, the function returns[root_folder]instead of[]. The condition on line 222 returnsfolderswhenlen(folders) <= 1, which includes the root even though the caller explicitly requested to exclude it.🔎 Proposed fix
def get_folder_path( folder_id: uuid.UUID, *, include_root: bool = False ) -> List["Folder"]: """ Return the ordered list of folders from root to the requested folder. """ state = get_folder_state() ids = path_ids_from_root(state, folder_id) folders = [state.folders[fid] for fid in ids] if include_root: return folders - return folders[1:] if len(folders) > 1 else folders + return folders[1:]
🧹 Nitpick comments (2)
backend/iam/cache_builders.py (2)
63-85: Consider thread safety for the cache readiness flag.The global
_cache_readyflag and_ensure_cache_ready()function are not thread-safe. If multiple threads call_ensure_cache_ready()concurrently during initialization, they might all check_cache_tables_ready()simultaneously before any sets the flag.While the impact is minor (redundant table checks), consider using a threading lock for strict correctness:
🔎 Optional refactor with threading.Lock
+import threading + +_cache_ready: bool = False +_cache_ready_lock = threading.Lock() def _ensure_cache_ready() -> None: global _cache_ready if _cache_ready: return - if _cache_tables_ready(): - _cache_ready = True - return - raise CacheNotReadyError("IAM caches are not ready to run DB queries") + with _cache_ready_lock: + if _cache_ready: + return + if _cache_tables_ready(): + _cache_ready = True + return + raise CacheNotReadyError("IAM caches are not ready to run DB queries")
269-295: Clarify or remove the stale comment.Line 271 states: "NOTE: you said you'll manage M2M later; keeping the builder here anyway." This comment is unclear—who is "you," and what does "manage M2M later" mean? Based on the AI summary and context, M2M signal handlers for
User.user_groupsare already implemented iniam/apps.py. Consider removing or clarifying this comment.🔎 Proposed fix
# -------------------------------------------------------------------- # IAM Groups cache: user_id -> group_ids (M2M) -# NOTE: you said you'll manage M2M later; keeping the builder here anyway. # --------------------------------------------------------------------
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/cache_builders.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/cache_builders.py
🧬 Code graph analysis (1)
backend/iam/cache_builders.py (2)
backend/iam/snapshot_cache.py (7)
CacheRegistry(161-247)CacheVersion(27-42)get(116-140)invalidate(142-155)invalidate(235-236)register(174-191)hydrate_all(194-225)backend/iam/models.py (2)
Folder(109-367)ContentType(139-144)
⏰ 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). (9)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
backend/iam/cache_builders.py (8)
19-56: LGTM!The imports are well-organized, and the
TYPE_CHECKINGpattern correctly avoids circular imports. Cache key constants are clear and follow naming conventions.
88-156: LGTM!The folder cache builder is well-designed:
- Efficient queries with
.only()to minimize data fetched.- Stable child ordering using
casefold().- Memoized depth calculation prevents redundant traversals.
- Immutable state using
MappingProxyTypeand tuples prevents accidental mutations.
163-208: LGTM!The helper functions
path_ids_from_root,iter_descendant_ids,get_sub_folders_cached, andget_parent_folders_cachedare correctly implemented with clear logic for tree traversal.
225-266: LGTM!The roles cache builder efficiently uses
prefetch_relatedto minimize queries and returns immutable state. The dual mappings (role→permissions and codename→permission_id) provide flexible lookup options.
298-350: LGTM!The assignments cache builder efficiently prefetches M2M relations and correctly handles both user and group assignments. The use of
AssignmentLiteas a lightweight, immutable representation is a good design choice.
353-383: LGTM!The registration logic is clear and well-documented. The comment explains the choice between import-time registration (lines 380-383) and explicit registration via
register_all_caches(). The past concern about broad exception handling has been addressed—the current code is clean and straightforward.
386-410: LGTM!The accessor functions follow a consistent pattern: ensure readiness, hydrate all caches once, and return the specific state. Supporting
force_reloadprovides flexibility for testing and manual cache refresh scenarios.
413-442: LGTM!The
__all__list comprehensively documents the public API and includes a helpful comment indicating which helpers are used frommodels.py. This makes the module's interface clear and maintainable.
fix regression
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/iam/models.py (5)
1016-1059: Potential KeyError if cached folder data is incomplete.Line 1053 accesses
state.folders[folder_id]without checking if the key exists. If the cache is partially loaded or inconsistent, this will raiseKeyError. The past review comment indicated this was addressed in commit 94ed9db, but the code still uses direct dictionary access.🔎 Suggested fix for missing folder data
# Filter by content_type and keep only within perimeter_ids result: list[uuid.UUID] = [] for folder_id in accessible_ids: - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + logger.warning("Folder missing from cache", folder_id=str(folder_id)) + continue if content_type and folder_obj.content_type != content_type: continue if folder_id in perimeter_ids: result.append(folder_id) return result
1193-1216: Potential KeyError in published inheritance logic.Line 1202 accesses
state.folders[folder_id]without checking existence. Iffolder_perm_codescontains a folder_id not in cache state, this will raiseKeyError.🔎 Suggested fix
# Published inheritance: published parents for local-view folders # PERF: collect all ancestor folder_ids first, then do ONE query. if hasattr(object_type, "is_published") and hasattr(object_type, "folder"): ancestor_ids: set[uuid.UUID] = set() for folder_id, perms in folder_perm_codes.items(): if view_code not in perms: continue - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + logger.warning("Folder missing from cache in published inheritance", folder_id=str(folder_id)) + continue if folder_obj.content_type == Folder.ContentType.ENCLAVE: continue parent_id = state.parent_map.get(folder_id) while parent_id: ancestor_ids.add(parent_id) parent_id = state.parent_map.get(parent_id) if ancestor_ids: result_view.update( object_type.objects.filter( folder_id__in=ancestor_ids, is_published=True ).values_list("id", flat=True) ) return (list(result_view), list(result_change), list(result_delete))
474-493: Critical: M2M relationship set before instance is saved.Line 492 calls
user.user_groups.set(...)before line 493user.save(...). Django M2M relationships require the instance to have a primary key (i.e., be saved first). This will raiseValueError: "<User instance>" needs to have a value for field "id" before this many-to-many relationship can be used.🔎 Proposed fix
Move the M2M set operation after the save:
user = cast( "User", self.model( email=email, first_name=extra_fields.get("first_name", ""), last_name=extra_fields.get("last_name", ""), is_superuser=extra_fields.get("is_superuser", False), is_active=extra_fields.get("is_active", True), observation=extra_fields.get("observation"), folder=_get_root_folder(), keep_local_login=extra_fields.get("keep_local_login", False), expiry_date=extra_fields.get("expiry_date"), ), ) if password: user.password = make_password(password) else: user.set_unusable_password() - user.user_groups.set(extra_fields.get("user_groups", [])) user.save(using=self._db) + user.user_groups.set(extra_fields.get("user_groups", [])) if initial_group: initial_group.user_set.add(user)
116-132: Unsafe cast masks potential None return value.Line 124 casts the result of
_get_root_folder()to"Folder"with atype: ignorecomment, but_get_root_folder()can returnNoneduring migrations. This cast bypasses type safety and could cause runtime errors if callers assume a non-None return.🔎 Proposed fix
Update the return type annotation to reflect reality and add a runtime guard:
@staticmethod - def get_root_folder() -> "Folder": + def get_root_folder() -> "Folder | None": """class function for general use""" try: state = get_folder_state() except CacheNotReadyError: # During initial migrations the cache cannot hydrate yet; fall back to the # direct lookup which may still be None until the schema is ready. - folder = _get_root_folder() - return cast("Folder", folder) # type: ignore[return-value] + return _get_root_folder() # Cache is ready, so root folder is already existing # But if the cache is stale, we call the db if state.root_folder_id: cached_root = state.folders.get(state.root_folder_id) if cached_root is not None: return cached_root return Folder.objects.get(content_type=Folder.ContentType.ROOT)Alternatively, if None should truly never occur in production, add a runtime assertion:
except CacheNotReadyError: - folder = _get_root_folder() - return cast("Folder", folder) # type: ignore[return-value] + folder = _get_root_folder() + if folder is None: + raise RuntimeError("Root folder not yet initialized") + return folder
890-914: Add error handling forCacheNotReadyErrorin_iter_assignment_lites_for_user.This function calls
get_assignments_state()andget_groups_state()which can raiseCacheNotReadyErrorif caches aren't ready. Since this helper is used by all permission-checking methods (is_access_allowed,get_permissions,has_role, etc.), an unhandledCacheNotReadyErrorwill propagate and potentially fail authentication/authorization during app startup or cache invalidation windows.🔎 Proposed fix
Add a try/except block with DB fallback similar to
Folder.get_root_folder():def _iter_assignment_lites_for_user(user: AbstractBaseUser | AnonymousUser): """ Yield AssignmentLite for a user, including via groups, using caches only. Returns empty iterator for AnonymousUser / unauthenticated. """ if isinstance(user, AnonymousUser) or not getattr(user, "is_authenticated", False): return iter(()) user_id_opt = cast(Optional[uuid.UUID], getattr(user, "id", None)) if user_id_opt is None: return iter(()) + try: - assignments_state = get_assignments_state() - groups_state = get_groups_state() + assignments_state = get_assignments_state() + groups_state = get_groups_state() + except CacheNotReadyError: + # Fallback to DB queries during cache unavailability + from iam.cache_builders import AssignmentLite + user_id = cast(uuid.UUID, user_id_opt) + + # Direct assignments + direct_qs = RoleAssignment.objects.filter(user_id=user_id).select_related('role') + direct_lites = [ + AssignmentLite( + role_id=ra.role_id, + perimeter_folder_ids=tuple(ra.perimeter_folders.values_list('id', flat=True)), + is_recursive=ra.is_recursive + ) + for ra in direct_qs + ] + + # Group assignments + group_ids = user.user_groups.values_list('id', flat=True) + group_qs = RoleAssignment.objects.filter(user_group_id__in=group_ids).select_related('role') + group_lites = [ + AssignmentLite( + role_id=ra.role_id, + perimeter_folder_ids=tuple(ra.perimeter_folders.values_list('id', flat=True)), + is_recursive=ra.is_recursive + ) + for ra in group_qs + ] + + return iter((*direct_lites, *group_lites)) user_id = cast(uuid.UUID, user_id_opt) group_ids = groups_state.user_group_ids.get(user_id, frozenset()) # user direct direct = assignments_state.by_user.get(user_id, ()) # via groups via_groups: list["AssignmentLite"] = [] for gid in group_ids: via_groups.extend(assignments_state.by_group.get(gid, ())) return iter((*direct, *via_groups))
🧹 Nitpick comments (3)
backend/core/views.py (2)
616-616: Python version requirement formodeltype annotation
model: type[models.Model] | None = Noneuses PEP 604 (|) andtype[...], which requires Python 3.10+ at runtime (nofrom __future__ import annotationsworkaround for this). If you still support 3.9, this line will break import; in that case, switch toOptional[type[models.Model]]/Union[...]fromtypinginstead.Also note that
get_queryset()is annotated to returnmodels.query.QuerySetbut can returnNonewhenself.modelis unset; if you want typing to be accurate, consider returningQuerySet | Noneor raising whenmodelis missing.
660-664: Dynamic eager loading looks good; consider caching field introspectionThe new block:
field_names = {f.name for f in self.model._meta.get_fields()} if "parent_folder" in field_names: queryset = queryset.select_related("parent_folder") if "filtering_labels" in field_names: queryset = queryset.prefetch_related("filtering_labels")is functionally safe and will reduce N+1s for models that expose
parent_folder/filtering_labels. It runs only after theself.modelguard, so_metaaccess is safe, and chaining with downstream.select_related()/.prefetch_related()in subclasses is fine.If this path becomes hot, you could avoid recomputing
field_nameson every request by caching the booleans per view class (e.g.,@cached_propertyflags or class-level attributes computed once), but that’s an optional micro-optimization.backend/iam/models.py (1)
959-998: Consider adding defensive error handling for cache failures.This critical permission check calls
get_folder_state()andget_roles_state()which can raiseCacheNotReadyError. While fixing_iter_assignment_lites_for_user(per previous comment) helps, adding a try/except block here provides defense-in-depth for this security-critical code path.🔎 Proposed pattern
@staticmethod def is_access_allowed( user: AbstractBaseUser | AnonymousUser, perm: Permission, folder: Folder ) -> bool: """ Determines if a user has specified permission on a specified folder. Cached path: - role permissions: Roles cache - assignments: Assignments cache (+ groups cache) - folder ancestry: Folder cache """ if not getattr(user, "is_authenticated", False): return False perm_codename = perm.codename if not perm_codename: return False + try: - state = get_folder_state() - roles_state = get_roles_state() + state = get_folder_state() + roles_state = get_roles_state() + except CacheNotReadyError: + # Fallback to DB-based permission check + logger.warning("Cache not ready in is_access_allowed, using DB fallback") + # Implement DB-based check or return False during startup + return False for a in _iter_assignment_lites_for_user(user): role_perms = roles_state.role_permissions.get(a.role_id, frozenset()) if perm_codename not in role_perms: continue # allow any folder if user has add_filteringlabel in role if perm_codename == "add_filteringlabel": return True perimeter_ids = set(a.perimeter_folder_ids) # walk up folder parents via cached parent_map current_id: Optional[uuid.UUID] = folder.id while current_id is not None: if current_id in perimeter_ids: return True current_id = state.parent_map.get(current_id) return False
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/views.pybackend/iam/models.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-09-19T08:44:33.744Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/views.py:3753-3762
Timestamp: 2025-09-19T08:44:33.744Z
Learning: In the domain export/import functionality in backend/core/views.py, Evidence.owner M2M relationships are intentionally not set during import because User objects are not exported. The owner_ids are collected in _process_model_relationships only to prevent M2M field validation errors during import, but they are deliberately not applied in _set_many_to_many_relations since users are environment-specific.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
⏰ 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). (8)
- GitHub Check: build (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: test (3.12)
🔇 Additional comments (17)
backend/iam/models.py (17)
4-71: LGTM: Imports are well-organized.The TYPE_CHECKING guard properly prevents circular imports, and all cache-related imports are cleanly organized.
95-106: LGTM: Proper error handling for migration scenarios.The function correctly handles pre-migration states by catching both
DoesNotExistand database operation errors, with clear documentation.
135-137: LGTM: Properly handles None case.The function correctly uses
getattrwith a default and has an accurate return type annotation.
177-185: LGTM: Cache invalidation properly implemented.Both
saveanddeletecorrectly invalidate the folders cache.
187-201: LGTM: Methods properly delegate to cache.The folder traversal methods correctly delegate to cached implementations with appropriate generator return types.
306-367: LGTM: Type annotations updated.Only cosmetic changes to type annotations; logic remains correct.
437-447: LGTM: Correct cache invalidation.Both caches are properly invalidated with a clear comment explaining why assignments cache must also be cleared.
627-666: LGTM: Defensive handling of user PK.The code properly handles the case where
user.pkmight be None with appropriate guards.
798-852: LGTM: Type annotations updated.Only cosmetic improvements to type hints; logic is unchanged.
872-880: LGTM: Cache invalidation properly implemented.Both
saveanddeletecorrectly invalidate the roles cache.
933-941: LGTM: Cache invalidation properly implemented.Both
saveanddeletecorrectly invalidate the assignments cache.
1001-1013: LGTM: Type annotation updated.Only cosmetic type hint update; logic is correct.
1220-1226: LGTM: Logic is correct.Method properly checks both direct user assignment and group membership.
1229-1271: Cache-based implementation looks correct.The method properly collects permissions from cached assignment state. Note: depends on error handling in
_iter_assignment_lites_for_user(flagged separately).
1274-1291: LGTM: Cache-based role check is correct.Logic correctly checks role assignments via the helper iterator. Depends on error handling in
_iter_assignment_lites_for_user(flagged separately).
1294-1341: LGTM: Permissions per folder logic is sound.The cache-based implementation correctly groups permissions by folder. Depends on cache availability and error handling in helper functions (flagged separately).
1344-1352: LGTM: Well-designed dataclass.The
FolderDisplayContextdataclass is properly defined withfrozen=Trueandslots=Truefor immutability and performance.
Was not implemented in is_access_allowed
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/iam/models.py (3)
474-493: Critical: M2M relationship still set before instance is saved.Line 492 calls
user.user_groups.set(...)before line 493user.save(...). Despite the past review comment being marked as addressed, the code still has this ordering. Django M2M relationships require the instance to have a primary key before M2M operations.This will raise:
ValueError: "<User instance>" needs to have a value for field "id" before this many-to-many relationship can be used.🔎 Proposed fix
user = cast( "User", self.model( email=email, first_name=extra_fields.get("first_name", ""), last_name=extra_fields.get("last_name", ""), is_superuser=extra_fields.get("is_superuser", False), is_active=extra_fields.get("is_active", True), observation=extra_fields.get("observation"), folder=_get_root_folder(), keep_local_login=extra_fields.get("keep_local_login", False), expiry_date=extra_fields.get("expiry_date"), ), ) if password: user.password = make_password(password) else: user.set_unusable_password() - user.user_groups.set(extra_fields.get("user_groups", [])) user.save(using=self._db) + user.user_groups.set(extra_fields.get("user_groups", [])) if initial_group: initial_group.user_set.add(user)
1018-1061: Potential KeyError: direct cache lookup without existence check.Line 1055 accesses
state.folders[folder_id]without checking if the key exists. Ifaccessible_idscontains a folder_id that's missing from the cache state (due to cache staleness or inconsistency), this will raiseKeyErrorand deny all access.The past review comment suggested this was addressed, but the direct indexing is still present.
🔎 Suggested defensive fix
# Filter by content_type and keep only within perimeter_ids result: list[uuid.UUID] = [] for folder_id in accessible_ids: - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + logger.warning("Folder missing from cache", folder_id=str(folder_id)) + continue if content_type and folder_obj.content_type != content_type: continue
1195-1218: Potential KeyError in published inheritance logic.Line 1204 accesses
state.folders[folder_id]without checking existence. Iffolder_perm_codescontains a folder_id not present in the cache state, this will raiseKeyError.This is the same issue flagged in the past review comment for this section.
🔎 Suggested defensive fix
for folder_id, perms in folder_perm_codes.items(): if view_code not in perms: continue - folder_obj = state.folders[folder_id] + folder_obj = state.folders.get(folder_id) + if folder_obj is None: + logger.warning("Folder missing from cache during published inheritance check", folder_id=str(folder_id)) + continue if folder_obj.content_type == Folder.ContentType.ENCLAVE: continue parent_id = state.parent_map.get(folder_id) while parent_id: ancestor_ids.add(parent_id) parent_id = state.parent_map.get(parent_id)
🧹 Nitpick comments (3)
backend/iam/models.py (3)
116-132: Type safety workaround withtype: ignoreshould be reconsidered.Line 124 uses
cast("Folder", folder)with# type: ignore[return-value]whenfoldermight beNonefrom_get_root_folder(). This suppresses the type checker but doesn't prevent runtime issues if calling code assumes a non-None result.Consider either:
- Update the return type to
Folder | Noneand have callers handle None, or- Add a runtime guard that raises a clear exception if None is returned
🔎 Option 1: Update return type
@staticmethod -def get_root_folder() -> "Folder": +def get_root_folder() -> "Folder | None": """class function for general use""" try: state = get_folder_state() except CacheNotReadyError: # During initial migrations the cache cannot hydrate yet; fall back to the # direct lookup which may still be None until the schema is ready. folder = _get_root_folder() - return cast("Folder", folder) # type: ignore[return-value] + return folder🔎 Option 2: Add runtime guard
except CacheNotReadyError: # During initial migrations the cache cannot hydrate yet; fall back to the # direct lookup which may still be None until the schema is ready. folder = _get_root_folder() - return cast("Folder", folder) # type: ignore[return-value] + if folder is None: + raise RuntimeError("Root folder not yet initialized - database schema may not be ready") + return folder
890-914: Consider addingCacheNotReadyErrorfallback for consistency.This helper calls
get_assignments_state()andget_groups_state(), which can raiseCacheNotReadyErrorduring early application startup. The past review comment suggested adding error handling similar toFolder.get_root_folder().While permission checks during migrations are edge cases, adding a fallback would provide consistency and robustness. Consider wrapping cache access in try/except and falling back to direct DB queries for assignments and group memberships.
🔎 Example fallback pattern
def _iter_assignment_lites_for_user(user: AbstractBaseUser | AnonymousUser): """ Yield AssignmentLite for a user, including via groups, using caches only. Returns empty iterator for AnonymousUser / unauthenticated. """ if isinstance(user, AnonymousUser) or not getattr(user, "is_authenticated", False): return iter(()) user_id_opt = cast(Optional[uuid.UUID], getattr(user, "id", None)) if user_id_opt is None: return iter(()) + try: assignments_state = get_assignments_state() groups_state = get_groups_state() + except CacheNotReadyError: + # Fallback to DB queries during early startup + # (Implementation would query RoleAssignment and UserGroup tables) + return _iter_assignment_lites_for_user_db_fallback(user_id_opt) user_id = cast(uuid.UUID, user_id_opt) group_ids = groups_state.user_group_ids.get(user_id, frozenset())
958-1000: AddCacheNotReadyErrorhandling for robustness.This method calls cache getters (
get_folder_state(),get_roles_state()) and_iter_assignment_lites_for_user(), all of which can raiseCacheNotReadyErrorduring early application startup. Since this is a core permission check called by views, unhandled exceptions could cause request failures.Add try/except with DB fallback similar to
get_root_folder()pattern.Based on learnings, the custom permission system relies on RoleAssignment methods for all access control.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/models.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".
Applied to files:
backend/iam/models.py
📚 Learning: 2025-09-19T08:44:33.744Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/views.py:3753-3762
Timestamp: 2025-09-19T08:44:33.744Z
Learning: In the domain export/import functionality in backend/core/views.py, Evidence.owner M2M relationships are intentionally not set during import because User objects are not exported. The owner_ids are collected in _process_model_relationships only to prevent M2M field validation errors during import, but they are deliberately not applied in _set_many_to_many_relations since users are environment-specific.
Applied to files:
backend/iam/models.py
📚 Learning: 2025-01-22T15:55:06.417Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
Applied to files:
backend/iam/models.py
⏰ 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). (9)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
backend/iam/models.py (11)
4-71: LGTM!The import structure is well-organized with proper use of
TYPE_CHECKINGto avoid circular dependencies and comprehensive cache builder imports.
95-106: LGTM!The helper function properly handles migration scenarios with targeted exception handling and clear return type annotation.
177-185: LGTM!Cache invalidation is properly triggered on both save and delete operations.
187-201: LGTM!Proper delegation to cache-backed implementations for folder traversal.
437-447: LGTM!Proper cache invalidation for both groups and assignments, with clear comment explaining the dependency.
872-880: LGTM!Proper cache invalidation on Role save and delete operations.
933-941: LGTM!Proper cache invalidation on RoleAssignment save and delete operations.
1222-1228: LGTM!Simple and correct logic for checking user assignment.
1231-1273: LGTM!Cache-driven permission retrieval logic is correct for both User and UserGroup principals. The single DB query at the end efficiently maps codenames to permission names.
Note: This method can raise
CacheNotReadyErrorvia_iter_assignment_lites_for_user(), consistent with other methods reviewed.
1276-1293: LGTM!Efficient cache-driven role membership check.
1296-1354: LGTM!The cache-driven
get_permissions_per_foldercorrectly handles both direct and recursive permission propagation. TheFolderDisplayContextdataclass is well-structured with appropriate use offrozen=Trueandslots=Truefor immutability and memory efficiency.
save before m2m
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/iam/cache_builders.py (1)
211-222: Fix edge case: root folder withinclude_root=Falsereturns[root]instead of[].When
folder_idis the root folder andinclude_root=False, the function currently returns[root_folder]instead of an empty list. Line 222's conditionalif len(folders) > 1prevents the slice from returning an empty list in this case.🔎 Proposed fix
def get_folder_path( folder_id: uuid.UUID, *, include_root: bool = False ) -> List["Folder"]: """ Return the ordered list of folders from root to the requested folder. """ state = get_folder_state() ids = path_ids_from_root(state, folder_id) folders = [state.folders[fid] for fid in ids] if include_root: return folders - return folders[1:] if len(folders) > 1 else folders + return folders[1:]
🧹 Nitpick comments (1)
backend/iam/cache_builders.py (1)
1-17: Consider documenting the invalidation strategy.The docstring describes the design goals but does not mention how caches are invalidated. Since
backend/iam/apps.pyimplementsm2m_changedsignal handlers that automatically invalidate caches (forUser.user_groups,RoleAssignment.perimeter_folders, andRole.permissions), consider adding a brief note about this in the docstring to help future maintainers understand the complete picture.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/cache_builders.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.
Applied to files:
backend/iam/cache_builders.py
⏰ 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). (9)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
backend/iam/cache_builders.py (9)
59-86: LGTM!The cache readiness mechanism appropriately gates access to caches until the required database tables exist, preventing premature queries during migrations or initial setup.
101-156: LGTM!The folder cache builder correctly constructs an immutable snapshot with parent/child relationships, stable ordering, and depth computation. The use of
only()for field selection andMappingProxyTypefor immutability are excellent optimizations.
163-209: LGTM!The folder traversal utilities correctly implement path computation, descendant iteration, and parent walking using the cached state. The use of a stack for iteration and appropriate KeyError handling are well done.
228-262: LGTM!The roles cache builder efficiently prefetches permissions and constructs immutable mappings for role permissions and permission codename lookups.
273-291: LGTM!The groups cache builder correctly queries the M2M through table and builds an immutable user-to-groups mapping.
301-350: LGTM!The assignments cache builder efficiently prefetches perimeter folders and constructs immutable per-user and per-group assignment mappings using the frozen
AssignmentLitedataclass.
353-357: LGTM!Import-time registration is clean and DB-free as designed.
360-384: LGTM!The state accessor functions follow a consistent pattern: check readiness, hydrate caches, and return the correctly typed state. The
force_reloadparameter is properly threaded through.
387-415: LGTM!The
__all__export list is comprehensive and well-organized, exposing all the necessary public APIs for cache management and state access.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/iam/snapshot_cache.py (1)
230-232: The invalidation race condition from the previous review is still present.As previously noted, when
invalidate()is called, the DB version is bumped but_last_versionsand_last_fetched_atare not cleared. Ifhydrate_all()is called within the 500ms window (line 208), it will use the stale cached versions and not reflect the invalidation. This can lead to serving stale data until the timeout expires.🔎 Recommended fix from previous review
@classmethod def invalidate(cls, key: str) -> Optional[int]: - return cls.get_cache(key).invalidate() + result = cls.get_cache(key).invalidate() + # Clear cached versions so next hydrate_all fetches fresh versions + cls._last_versions = None + cls._last_fetched_at = None + return result
🧹 Nitpick comments (1)
backend/iam/snapshot_cache.py (1)
167-167: Consider documenting the fetch interval rationale.The 500ms minimal fetch interval is a reasonable default balancing cache freshness against database load. Adding a comment explaining this tradeoff would help future maintainers understand the design choice.
📝 Optional documentation addition
- _MIN_FETCH_INTERVAL_MS = 500.0 + # Minimal fetch interval to reduce DB load while maintaining reasonable freshness. + # 500ms balances between cache staleness and query frequency. + _MIN_FETCH_INTERVAL_MS = 500.0
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/snapshot_cache.py
⏰ 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). (9)
- GitHub Check: build (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
backend/iam/snapshot_cache.py (6)
1-17: LGTM!The imports are appropriate for the versioned caching system. Using
from __future__ import annotationsenables cleaner type hints.
23-38: LGTM!The
CacheVersionmodel is well-designed with appropriate field types. Usingkeyas the primary key andPositiveBigIntegerFieldfor version numbers provides a solid foundation for the versioning system.
44-46: LGTM!Simple, immutable dataclass for wrapping version data. The frozen and slots attributes optimize memory and ensure immutability.
54-79: LGTM!The version fetching and creation logic is correct. Using
ignore_conflicts=Trueinbulk_createensures safe concurrent access, and re-querying after creation guarantees all versions are returned.
102-154: LGTM!The cache implementation is well-designed:
- The
get()method fails fast with a clear error message if the cache key hasn't been hydrated- The
invalidate()method gracefully handles database unavailability during migrations while still clearing the local snapshot- Version comparison logic ensures rebuilds only when necessary
82-90: Atomic version bump implementation is correct.The use of
select_for_update()insidetransaction.atomic()withget_or_create()is a supported Django pattern. TheF("version") + 1ensures the database executes the increment atomically, andrefresh_from_db()correctly retrieves the updated value. This is thread-safe on PostgreSQL (the primary database) with proper row-level locking.
cache folders, roles, role assignments and user_groups
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
API Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.