fix: properly reflect role permissions in the UI #1609
Conversation
WalkthroughThis pull request introduces a new backend method for retrieving folder-based permissions and expands the API response to include these permissions along with the root folder ID. On the frontend, the permission-checking logic across various components and routes has been centralized with the new Changes
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nas-tabchiche
changed the title
nas-tabchiche
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/lib/utils/access-control.ts (1)
10-12: Clean implementation of centralized permission checking logicThis implementation efficiently checks if a user can perform a specific action on a model within a domain context. The function is concise yet powerful, correctly handling the permission format of
${action}_${model}and safely handling undefined domain permissions with the|| []fallback.Consider adding a basic validation check for empty or null domain values:
- export function canPerformAction({ user, action, model, domain }: CanPerformActionParams): boolean { - return (user.domain_permissions[domain] || []).includes(`${action}_${model}`); - } + export function canPerformAction({ user, action, model, domain }: CanPerformActionParams): boolean { + if (!domain) { + return false; + } + return (user.domain_permissions[domain] || []).includes(`${action}_${model}`); + }frontend/src/lib/components/ModelTable/ModelTable.svelte (1)
203-214: Improved domain-aware permission check implementationThe new implementation correctly determines if a user can create an object by considering the folder context. The change from direct permission checks to using the
canPerformActionfunction enhances the permission system by making it domain-aware.Two suggestions to improve robustness:
- Consider adding a fallback priority comment to clarify the order in which domain sources are checked:
$: canCreateObject = model ? canPerformAction({ user, action: 'add', model: model.name, domain: + // Priority: current folder > data folder > ID param > user root folder $page.data?.data?.folder?.id || $page.data?.data?.folder || $page.params.id || user.root_folder_id }) : false;
- Consider extracting the domain determination logic to a helper function to ensure consistency with other components that perform similar checks.
frontend/src/lib/components/TableRowActions/TableRowActions.svelte (2)
99-106: Enhanced delete permission check with domain awarenessGood implementation of delete permission checks that considers the domain context. The special handling for folder models (using the folder's own ID as domain) versus other models (using the parent folder's ID) is a good approach for hierarchical permissions.
Consider adding null/undefined checks on
row.meta.folder?.idto prevent potential runtime errors:$: canDeleteObject = model ? canPerformAction({ user, action: 'delete', model: model.name, domain: model.name === 'folder' ? row.meta.id - : (row.meta.folder?.id ?? row.meta.folder) + : (row.meta?.folder?.id ?? row.meta?.folder ?? user.root_folder_id) }) && !preventDelete : false;
107-114: Enhanced edit permission check with domain awarenessThe implementation correctly handles edit permissions based on folder hierarchy, with appropriate special handling for folder models.
Similar to the delete permission check, consider adding null/undefined checks for
row.meta:$: canEditObject = model ? canPerformAction({ user, action: 'change', model: model.name, domain: model.name === 'folder' ? row.meta.id - : (row.meta.folder?.id ?? row.meta.folder) + : (row.meta?.folder?.id ?? row.meta?.folder ?? user.root_folder_id) }) : false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/iam/models.py(1 hunks)backend/iam/views.py(1 hunks)frontend/src/lib/components/DetailView/DetailView.svelte(2 hunks)frontend/src/lib/components/ModelTable/ModelTable.svelte(2 hunks)frontend/src/lib/components/TableRowActions/TableRowActions.svelte(2 hunks)frontend/src/lib/utils/access-control.ts(1 hunks)frontend/src/lib/utils/types.ts(1 hunks)frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte(3 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
backend/iam/views.py (1)
89-92:❓ Verification inconclusive
Domain permissions implementation correctly enhances user context
The addition of domain permissions and root folder ID to the user data response is well implemented. This supports the PR objective of exposing folder permissions for users.
Let's ensure that this implementation doesn't cause excessive data loading:
🏁 Script executed:
#!/bin/bash # Check implementation of get_permissions_per_folder method to understand its performance characteristics # Find the implementation of the get_permissions_per_folder method ast-grep --pattern $'class RoleAssignment($$$) { $$$ @classmethod def get_permissions_per_folder($$$) { $$$ } $$$ }'Length of output: 131
Let's further investigate the implementation to ensure we correctly evaluate its performance characteristics. I'm going to run a new shell script that searches the repository for the definition of
get_permissions_per_folderusingrg(ripgrep). This should help us locate the method even if theast-greppattern didn't match.Please run this script so we can review its output and determine if any performance concerns remain.
🏁 Script executed:
#!/bin/bash # Search for the definition of get_permissions_per_folder in the repository rg "def get_permissions_per_folder" -n .Length of output: 106
Domain permissions and folder ID addition appear correct, but please verify potential performance impacts.
I've confirmed that the
get_permissions_per_foldermethod exists in the repository (located at backend/iam/models.py:795). However, the automated search did not reveal details about its internal implementation. As such, while the overall approach supports exposing folder permissions for users, please double-check that the recursive loading performed whenrecursive=Truedoes not introduce excessive data loading or performance concerns.
- Location:
backend/iam/models.pyat line 795- Action: Manually verify that the recursion and data fetching in
get_permissions_per_folderefficiently handle large data sets.frontend/src/lib/utils/types.ts (1)
18-19:✅ Verification successful
User interface extends correctly to support new permission model
The new properties
domain_permissionsandroot_folder_idalign well with the backend changes, providing a type-safe representation of the folder-based permissions data structure. This ensures proper typing for role-based access control throughout the application.Let's check if the type is used consistently across the codebase:
🏁 Script executed:
#!/bin/bash # Check for any places where domain_permissions might be accessed without proper type checking # Look for accesses to domain_permissions that might need checking rg "\.domain_permissions(?!\s*(\?\.))" -g "*.ts" -g "*.svelte"Length of output: 321
UI Type Safety for Folder-based Permissions (Manual Verification Recommended)
The addition of the
domain_permissionsandroot_folder_idproperties infrontend/src/lib/utils/types.tsappears to correctly align with the updated backend changes and ensures a type-safe interface for handling folder-based permissions. However, the initial verification command encountered a regex parsing issue due to unsupported lookahead syntax; please re-run the revised command above to manually confirm that all usages ofdomain_permissionsinclude proper type checking (e.g., via optional chaining) where necessary.
- Action Required: Manually verify the output of the revised script to ensure no unsafe access patterns exist for
domain_permissions.--- 🏁 Script executed: ```shell #!/bin/bash # Re-run the search with PCRE2 enabled to support lookahead assertions. rg --pcre2 "\.domain_permissions(?!\s*(\?\.))" -g "*.ts" -g "*.svelte" -A 1Length of output: 363
New Permission Model Type Usage Verified
The extension in
frontend/src/lib/utils/types.ts—addingdomain_permissionsandroot_folder_id—correctly aligns with the backend changes. A verification run showed the one usage infrontend/src/lib/utils/access-control.tsemploys a safe fallback (user.domain_permissions[domain] || []), ensuring proper type safety across the application.frontend/src/lib/components/DetailView/DetailView.svelte (2)
29-29: Import for canPerformAction centralizes access control logicThe import statement correctly brings in the new centralized permission checking utility.
169-174:❓ Verification inconclusive
Enhanced permission checking with domain context
The refactored permission logic now properly considers the domain context when determining if a user can edit an object. This is a significant improvement as it provides granular, folder-based access control.
Key improvements:
- Domain-specific permissions (vs global permissions)
- Clear action intent ('change')
- Proper handling of both folder objects and folder-related objects
Let's verify that this doesn't break existing functionality by checking where else edit permissions are checked:
🏁 Script executed:
#!/bin/bash # Find any other permission checks that might need similar updates # Look for similar user permission checks in other places that might need to be updated rg "user\.permissions.*\.includes\(" -g "*.svelte"Length of output: 52
Permission Logic Update: Further Manual Verification Required
The updated permission logic in
frontend/src/lib/components/DetailView/DetailView.svelte(lines 169–174) now leverages domain-specific checks, which is a notable improvement for granular, folder-based access control. Our initial grep using the commandrg "user\.permissions.*\.includes\(" -g "*.svelte"did not yield any additional instances of such permission checks. However, as the output was empty and might not fully capture all patterns in the codebase, please verify manually that no other parts of the application rely on the older permission-checking pattern.
Action points:
- Review other instances: Double-check any usage of similar permission logic (e.g., calls to
canPerformAction(or similar constructs) across the codebase to ensure consistency.- Confirm app behavior: Ensure that the new folder-based access control does not inadvertently affect any unrelated functionality.
If everything checks out in your manual verification, this update should be safe to merge.
frontend/src/lib/utils/access-control.ts (1)
3-8: Well-defined interface for permission checking parametersThe
CanPerformActionParamsinterface provides a clear contract for the permission checking function, specifying required parameters with appropriate types and helpful documentation comments.frontend/src/lib/components/ModelTable/ModelTable.svelte (1)
40-40: LGTM: Good utility importAdding the
canPerformActionutility function centralizes permission checking logic.frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (3)
43-43: LGTM: Good utility importAdding the
canPerformActionutility function centralizes permission checking logic.
51-56: Improved folder-based permission checkThe implementation correctly uses the compliance assessment's folder ID as the domain for permission checking. This ensures that permissions are properly enforced at the folder level.
58-63: Consistent permission checking approachGood consistency in using the same permission checking approach for requirement assessments, ensuring that folder-level permissions are properly enforced here as well.
frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte (2)
17-17: LGTM: Good utility importAdding the
canPerformActionutility function centralizes permission checking logic.
22-27: Folder-based permission check for risk scenariosThe implementation correctly uses the risk scenario's perimeter folder ID as the domain for permission checking. This ensures proper access control based on folder hierarchy.
frontend/src/lib/components/TableRowActions/TableRowActions.svelte (1)
14-14: LGTM: Good utility importAdding the
canPerformActionutility function centralizes permission checking logic.backend/iam/models.py (1)
794-814: Well-structured implementation of folder-based permissions retrieval.This method is a good addition that aligns with the PR objectives of exposing permissions for user folders and enhancing role permission visibility. The implementation is clean and efficient, using a defaultdict to group permissions by folder ID, which will help the UI accurately reflect what actions users can perform.
A few observations:
- Good use of type hints for the principal parameter
- Effective use of the
|=operator to aggregate permission sets- Proper handling of recursive permissions for subfolders when requested
frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte (5)
6-6: Import path simplified correctly.The removal of the
.jsextension improves import consistency.
18-18: Import path simplified correctly.The removal of the
.jsextension improves import consistency.
21-21: Good introduction of the access control utility.This new import introduces the centralized permission checking mechanism, which aligns with the PR objectives of properly reflecting role permissions in the UI.
32-37: Excellent implementation of folder-based permission checking.This is a significant improvement over the previous implementation. Instead of a simple object property check, the code now uses the
canPerformActionutility function that takes into account:
- The user object
- The specific action ('change')
- The model name
- The domain (folder) context
This new approach ensures that permissions are properly enforced based on the folder structure and role assignments, which directly fulfills the PR objective of "enforcing proper access control in the user interface specifically for edit operations."
209-219: Verify conditional rendering based on permissions.The edit button is conditionally rendered based on the
canEditObjectpermission check, which now uses the new folder-based permission system. This ensures that only users with the appropriate permissions for this specific risk assessment's folder can see the edit button.The implementation looks correct, but you should verify that this works correctly across different user roles and folder structures to ensure the edit button visibility matches the expected permissions.
nas-tabchiche
force-pushed
the
fix/properly-reflect-role-permissions-in-the-ui
branch
from
c56083e to
7b16747
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/routes/(app)/(internal)/my-profile/+page.svelte (1)
28-33: Consider adding a comment explaining the permission logic.The switch to a folder-based permission model represents a significant architectural change. Adding a brief comment explaining how this works would help future developers understand the permission system.
+// Check if user can edit their profile based on folder permissions const canEditObject: boolean = canPerformAction({ user, action: 'delete', model: 'user', domain: user.root_folder_id });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/components/DetailView/DetailView.svelte(2 hunks)frontend/src/lib/components/TableRowActions/TableRowActions.svelte(2 hunks)frontend/src/routes/(app)/(internal)/my-profile/+page.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/components/DetailView/DetailView.svelte
- frontend/src/lib/components/TableRowActions/TableRowActions.svelte
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
frontend/src/routes/(app)/(internal)/my-profile/+page.svelte (3)
8-8: Good addition of the centralized permission checking utility.The import of
canPerformActionfrom the shared access control utility is a positive step toward centralizing permission logic across the application.
43-49:❓ Verification inconclusive
Verify that edit button visibility matches permission intent.
The conditional rendering of the edit button based on
canEditObjectneeds to align with the correct permission check for editing users. Ensure that fixing the action parameter in the previous comment correctly enables the button for authorized users only.
🏁 Script executed:
#!/bin/bash # Find the implementation of canPerformAction to understand how it maps actions to permissions echo "Searching for canPerformAction implementation..." rg -A 20 "export function canPerformAction" --glob "*.ts" --glob "*.js" # Check if there are other places using similar pattern to confirm correct usage echo "Checking for similar usage patterns..." rg "canPerformAction.*action: 'change'" --glob "*.svelte" --glob "*.ts" --glob "*.js"Length of output: 667
Ensure the edit button’s permission check is correctly implemented
Based on our verification:
- The
canPerformActionfunction infrontend/src/lib/utils/access-control.tsbuilds permission strings using the template${action}_${model}.- A search for usage patterns with
action: 'change'showed no conflicting instances.Please double-check that the logic used to compute
canEditObjectin+page.sveltecorrectly applies the intended action parameter (as suggested in the previous comment) so that only users with the appropriate permissions see the edit button.
28-33:✅ Verification successful
Validate that folder-based permission model is correctly implemented.
The new permission model now includes a domain parameter with the user's root folder ID, which aligns with the PR objective of "exposing permissions for user folders." Ensure this approach is consistently applied throughout the application for all user-related operations.
🏁 Script executed:
#!/bin/bash # Check how domains/folders are used in permission checks across the application echo "Examining how root_folder_id is used in permission checks..." rg "root_folder_id.*canPerformAction" --glob "*.svelte" --glob "*.ts" --glob "*.js" # Also check the backend API to confirm it's returning the necessary folder permissions echo "Checking related backend API endpoints..." rg -B 5 -A 10 "def get_permissions" --glob "*.py"Length of output: 2348
Folder-based permission model appears correctly integrated.
The updated permission check in
frontend/src/routes/(app)/(internal)/my-profile/+page.sveltenow correctly passesuser.root_folder_idas the domain, and the backend confirms folder-specific permission handling viaget_permissions_per_folderinbackend/iam/models.py. Please double-check that all other user-related operations also consistently apply the new domain parameter.
- Verify that similar permission checks elsewhere incorporate the folder context.
- Ensure that the backend’s folder permissions are consistently enforced across API endpoints.
| const canEditObject: boolean = canPerformAction({ | ||
| user, | ||
| action: 'delete', | ||
| model: 'user', | ||
| domain: user.root_folder_id | ||
| }); |
There was a problem hiding this comment.
Action parameter inconsistency detected - 'delete' vs expected 'edit'.
There appears to be a mismatch between the action parameter and the intended functionality. The variable canEditObject controls the display of an edit button, but the action being checked is 'delete'. This inconsistency could lead to incorrect permission enforcement.
const canEditObject: boolean = canPerformAction({
user,
- action: 'delete',
+ action: 'change',
model: 'user',
domain: user.root_folder_id
});According to Django's permission naming conventions (which appears to be used based on PR description), 'change_user' would be the appropriate permission for edit operations, so the action should be 'change' rather than 'delete'.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const canEditObject: boolean = canPerformAction({ | |
| user, | |
| action: 'delete', | |
| model: 'user', | |
| domain: user.root_folder_id | |
| }); | |
| const canEditObject: boolean = canPerformAction({ | |
| user, | |
| action: 'change', | |
| model: 'user', | |
| domain: user.root_folder_id | |
| }); |
nas-tabchiche
force-pushed
the
fix/properly-reflect-role-permissions-in-the-ui
branch
from
67e29a3 to
8d7dd91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/lib/components/TableRowActions/TableRowActions.svelte (1)
99-111: Consider extracting common permission logic to reduce duplication.Both
canDeleteObjectandcanEditObjectreactive declarations contain very similar logic with only minor differences in the action parameter. This represents an opportunity to extract this logic into a helper function to improve maintainability.-$: canDeleteObject = model - ? $page.params.id - ? canPerformAction({ - user, - action: 'delete', - model: model.name, - domain: - model.name === 'folder' - ? row.meta.id - : (row.meta.folder?.id ?? row.meta.folder ?? user.root_folder_id) - }) && !preventDelete - : Object.hasOwn(user.permissions, `delete_${model.name}`) - : false; -$: canEditObject = model - ? $page.params.id - ? canPerformAction({ - user, - action: 'change', - model: model.name, - domain: - model.name === 'folder' - ? row.meta.id - : (row.meta.folder?.id ?? row.meta.folder ?? user.root_folder_id) - }) - : Object.hasOwn(user.permissions, `change_${model.name}`) - : false; +// Helper function to check permissions with common logic +function checkObjectPermission(action, includePreventDelete = false) { + if (!model) return false; + + if ($page.params.id) { + const hasPermission = canPerformAction({ + user, + action, + model: model.name, + domain: + model.name === 'folder' + ? row.meta.id + : (row.meta.folder?.id ?? row.meta.folder ?? user.root_folder_id) + }); + return includePreventDelete ? hasPermission && !preventDelete : hasPermission; + } + + return Object.hasOwn(user.permissions, `${action}_${model.name}`); +} + +$: canDeleteObject = checkObjectPermission('delete', true); +$: canEditObject = checkObjectPermission('change');Also applies to: 112-124
frontend/src/lib/components/ModelTable/ModelTable.svelte (2)
203-216: Consider simplifying permission check for readability.The nested ternary expressions make the code harder to read and maintain. Consider extracting this logic into a separate function or using a more explicit approach with if/else statements.
-$: canCreateObject = model - ? $page.params.id - ? canPerformAction({ - user, - action: 'add', - model: model.name, - domain: - $page.data?.data?.folder?.id || - $page.data?.data?.folder || - $page.params.id || - user.root_folder_id - }) - : Object.hasOwn(user.permissions, `add_${model.name}`) - : false; +function checkCreatePermission() { + if (!model) return false; + + if ($page.params.id) { + const domain = $page.data?.data?.folder?.id || + $page.data?.data?.folder || + $page.params.id || + user.root_folder_id; + + return canPerformAction({ + user, + action: 'add', + model: model.name, + domain + }); + } + + return Object.hasOwn(user.permissions, `add_${model.name}`); +} + +$: canCreateObject = checkCreatePermission();
209-214: Document domain resolution fallback mechanism.The domain resolution logic is complex and critical for proper permission checking. Consider adding code comments to explain the fallback mechanism and why each step is necessary.
domain: - $page.data?.data?.folder?.id || - $page.data?.data?.folder || - $page.params.id || - user.root_folder_id + // Try to find the folder ID in the following order: + // 1. From the folder object in the page data (if it's a structured object) + // 2. From the direct folder ID in the page data + // 3. From the URL parameter (which might be a folder ID in some contexts) + // 4. Fall back to the user's root folder ID as a last resort + $page.data?.data?.folder?.id || + $page.data?.data?.folder || + $page.params.id || + user.root_folder_id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/ModelTable/ModelTable.svelte(2 hunks)frontend/src/lib/components/TableRowActions/TableRowActions.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
frontend/src/lib/components/TableRowActions/TableRowActions.svelte (4)
14-14: New dependency for access control.The import of
canPerformActionfrom$lib/utils/access-controlis in line with the PR objective to properly reflect role permissions in the UI.
99-111: Improved permission checking for delete operations.The updated permission logic for
canDeleteObjectnow correctly considers folder-based permissions, which is a significant improvement over the previous implementation. The code now handles three scenarios:
- If no model is defined, permissions are denied
- If viewing a specific object (with ID in params), it uses folder-based permissions
- Otherwise, it falls back to global permissions
This is a key enhancement that fulfills the PR objective of properly reflecting role permissions.
112-124: Enhanced edit permission verification.Similar to the delete permission logic, the edit permission check now properly handles folder-based access control, which aligns with the PR objective.
99-111:❓ Verification inconclusive
Verify folder fallback mechanism is consistent across the application.
The domain fallback mechanism (
row.meta.folder?.id ?? row.meta.folder ?? user.root_folder_id) is critical for proper permission checking. Ensure this pattern is consistently applied in all permission-checking code throughout the application.
🏁 Script executed:
#!/bin/bash # Description: Check for consistent use of user.root_folder_id across the codebase # Look for other instances of root_folder_id usage echo "Searching for root_folder_id usage patterns:" rg "root_folder_id" --type svelte --type ts # Look for domain determination patterns in canPerformAction usage echo "Searching for domain determination patterns in canPerformAction usage:" rg "domain:.*folder" --type svelte --type tsLength of output: 402
Here's a revised shell script that bypasses the unrecognized file type issue by using glob patterns to search in both
.svelteand.tsfiles:Please run this script to verify that the folder fallback mechanism (
row.meta.folder?.id ?? row.meta.folder ?? user.root_folder_id) is applied consistently across the codebase. Once you’ve confirmed the output, let’s move on to finalizing the review comment.
🏁 Script executed:
#!/bin/bash echo "Searching for root_folder_id usage patterns in .svelte and .ts files:" rg "root_folder_id" -g "*.svelte" -g "*.ts" echo "Searching for domain determination patterns in canPerformAction usage in .svelte and .ts files:" rg "domain:.*folder" -g "*.svelte" -g "*.ts" echo "Searching for folder fallback mechanism pattern (row.meta.folder?.id fallback) in .svelte and .ts files:" rg "row\.meta\.folder\?\.\w+" -g "*.svelte" -g "*.ts"Length of output: 2346
Action: Verify Consistency of Folder Fallback Mechanism Across Permissions
The fallback mechanism in TableRowActions.svelte is implemented as
row.meta.folder?.id ?? row.meta.folder ?? user.root_folder_id
and is consistent with a similar pattern in DetailView.svelte. However, note that in other parts of the application, such as in the my-profile page and risk/compliance assessment pages, the domain is determined differently (e.g., directly usinguser.root_folder_idor properties likerisk_assessment.folder.id). Please manually verify that these differences are intentional based on the model requirements and that all permission-checking logic is handling folder fallbacks appropriately.
- TableRowActions.svelte & DetailView.svelte: Fallback with optional chaining and multiple fallbacks is used.
- Other Pages: Direct references (e.g.,
user.root_folder_idorrisk_assessment.folder.id) are observed.frontend/src/lib/components/ModelTable/ModelTable.svelte (3)
40-40: New dependency for access control.The import of
canPerformActionfrom$lib/utils/access-controlaligns with the PR objective to properly reflect role permissions in the UI.
203-216: Enhanced permission checking for object creation.The updated permission logic for
canCreateObjectnow correctly accounts for folder-based permissions, which is a significant improvement. The code now:
- Checks if a model is defined
- For specific contexts (when ID is in params), uses folder-based permissions with appropriate domain fallbacks
- Otherwise, falls back to global permissions
This implementation properly enforces access control in the UI as intended by the PR.
205-214:❓ Verification inconclusive
Verify domain resolution fallback chain.
The domain resolution uses multiple fallbacks:
domain: $page.data?.data?.folder?.id || $page.data?.data?.folder || $page.params.id || user.root_folder_idThis complex chain of fallbacks might be difficult to debug if issues arise. Ensure this logic is consistently applied across the application and well-tested with different data scenarios.
🏁 Script executed:
#!/bin/bash # Description: Check how domains are resolved across permission checks in the codebase echo "Searching for similar domain resolution patterns:" rg "domain:.*\?.data\?.folder" --type svelte --type ts # Check all usages of canPerformAction to verify domain resolution consistency echo "All canPerformAction usages with domain resolution:" rg "canPerformAction.*domain:" -A 6 --type svelte --type tsLength of output: 402
Action Required: Verify Consistency of Domain Resolution Fallback Chain
- The fallback chain in
frontend/src/lib/components/ModelTable/ModelTable.svelte(lines 205–214) is implemented as follows:
$page.data?.data?.folder?.id$page.data?.data?.folder$page.params.iduser.root_folder_id- This chain appears to be designed to provide multiple fallback options, but our initial automated search (using
rgwith the--type svelteflag) was unsuccessful due to unrecognized file type errors. As a result, we couldn’t automatically verify if similar patterns are applied consistently elsewhere.- Next Steps:
- Manual Verification: Please manually verify that the same domain resolution logic is consistently used across all permission checks or components.
- Consideration: It might be helpful to add inline documentation or tests to clarify and validate this fallback chain.
| $: canCreateObject = model | ||
| ? $page.params.id | ||
| ? canPerformAction({ | ||
| user, | ||
| action: 'add', | ||
| model: model.name, | ||
| domain: | ||
| $page.data?.data?.folder?.id || | ||
| $page.data?.data?.folder || | ||
| $page.params.id || | ||
| user.root_folder_id | ||
| }) | ||
| : Object.hasOwn(user.permissions, `add_${model.name}`) | ||
| : false; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that the access control mechanism has comprehensive test coverage.
The permission checking logic is critical for application security. Ensure that the new canPerformAction utility and its integration with these components are thoroughly tested, especially with edge cases like missing folder IDs or undefined models.
🏁 Script executed:
#!/bin/bash
# Description: Check for test coverage of the access control logic
# Check if there are tests for the access-control utility
echo "Looking for tests for access-control.ts:"
fd -t f -e ts -e js "access-control.*test"
# Check the implementation of canPerformAction
echo "canPerformAction implementation:"
rg -A 20 "export function canPerformAction" --type tsLength of output: 572
Enhance Test Coverage for Access Control Logic
The implementation of canPerformAction in frontend/src/lib/utils/access-control.ts currently lacks direct test coverage. Please add comprehensive tests that cover:
- Standard and Edge Cases: Validate behavior when the user has the required permissions, as well as scenarios with missing or undefined folder IDs and models.
- Integration Testing: Ensure that the access control logic in components like
frontend/src/lib/components/ModelTable/ModelTable.svelte(lines 203–216) behaves correctly under all conditions. - Permission String Generation: Test that the utility accurately checks permission strings based on varying
user.domain_permissions.
Addressing these points will help secure the permission checking logic and prevent potential regressions.
If the authenticated user has the permission in at lease a single folder, they will be able to create an object from its global model table, the domain choices in the create form will already be filtered only top include those they have access to.
nas-tabchiche
force-pushed
the
fix/properly-reflect-role-permissions-in-the-ui
branch
from
8d7dd91 to
afcd641
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/lib/components/ModelTable/ModelTable.svelte (2)
40-40: Consider centralizing all permission checks viacanPerformAction.While importing
canPerformActionis an excellent step, double-check that all permission checks (including those currently implemented withObject.hasOwn(...)) are eventually unified under this utility. This ensures consistent logic across the codebase.
203-216: Unify fallback permission checks.When
$page.params.idis not available, the code currently falls back toObject.hasOwn(user.permissions, \add_${model.name}`). Consider usingcanPerformActionfor both cases to ensure consistent authorization logic. You can pass an empty or default domain identifier when$page.params.id` is absent.$: canCreateObject = model ? canPerformAction({ user, action: 'add', model: model.name, domain: $page.params.id || $page.data?.data?.folder?.id || $page.data?.data?.folder || user.root_folder_id }) : false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/components/ModelTable/ModelTable.svelte(2 hunks)frontend/src/lib/components/TableRowActions/TableRowActions.svelte(2 hunks)frontend/src/routes/(app)/(internal)/my-profile/+page.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/components/TableRowActions/TableRowActions.svelte
- frontend/src/routes/(app)/(internal)/my-profile/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
Summary by CodeRabbit
New Features
Refactor