fix: Cmd K not working for viewer roles #8411
Conversation
WalkthroughWalkthroughThe updates focus on refining user permissions and data access methods in the NocoDB project. Changes include using a specific user model for project listings in the Command Palette service, removing token handling in the Organization Users service, and enhancing role permissions for the CREATOR role in the access control utility. These adjustments aim to improve security and data handling efficiency. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Out of diff range and nitpick comments (2)
packages/nocodb/src/services/command-palette.service.ts (1)
71-85: The view data structure is comprehensive, but ensure that theencodeURIComponentfunction is used correctly to avoid issues with special characters in URLs.Consider adding a comment explaining the use of
encodeURIComponentfor future maintainers.packages/nocodb/src/services/org-users.service.ts (1)
[!TIP]
Codebase VerificationThe search results indicate that
token_versionis used extensively across various files in the codebase, including services related to user authentication and authorization, JWT strategy validation, and database migrations. This suggests that thetoken_versionplays a significant role in the security and functionality of the system, particularly in managing session validity and preventing token misuse.Given this widespread usage and the critical nature of the functionality, the removal of
token_versionfrom theorg-users.service.tsupdate method could potentially lead to issues if not handled correctly elsewhere in the codebase.Action Required:
- Review the impact of removing
token_versionfrom theorg-users.service.tson related functionalities, especially in authentication and session management.- Ensure that any changes maintain the integrity and security of user sessions and authentication processes.
Files and locations to consider:
org-users.service.ts: Direct impact of the removal.users.service.ts,auth.service.ts,jwt.strategy.ts: Check how these services handletoken_versionafter its removal fromorg-users.service.ts.User.ts,nc_017_add_user_token_version_column.ts: Related to database schema and migrations.Analysis chain
Line range hint
89-89: Removal oftoken_versionsimplifies the update logic and reduces the risk of token misuse. Ensure that this change does not affect other functionalities wheretoken_versionmight be required.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of `token_version` does not affect other functionalities. # Test: Search for other usages of `token_version` in the codebase. rg --type typescript $'token_version'Length of output: 71
Script:
#!/bin/bash # Description: Verify that the removal of `token_version` does not affect other functionalities. # Correcting the previous script error by specifying the correct file type for ripgrep. rg --type ts 'token_version'Length of output: 3270
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/nocodb/src/services/command-palette.service.ts (2 hunks)
- packages/nocodb/src/services/org-users.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (1 hunks)
Additional comments not posted (5)
packages/nocodb/src/services/command-palette.service.ts (4)
3-3: Updated import fromBasetoBaseUseraligns with the new requirement to tailor data visibility based on user permissions.
25-27: Ensure thatBaseUser.getProjectsListmethod properly handles the case where no projects are available or the user has no accessible projects.
40-42: This filtering logic ensures that views are only added if the user has the appropriate role and the view is not disabled for that role. Good implementation of least privilege principle.
51-57: The construction of project data incmdDatais clear and well-structured. However, consider adding error handling for potential failures indeserializeJSON.packages/nocodb/src/utils/acl.ts (1)
279-279: Addition ofcommandPalettepermission for theCREATORrole expands their capabilities within the application. Ensure this aligns with the intended access control policies.
|
Uffizzi Preview |
There was a problem hiding this comment.
This is not exactly on same scope but requires a tiny fix so better to include it, whenever we open cmd+k from settings page it tries to load account_settings which is no longer a valid parent, we should load root instead.
Can you please get rid of the the logic related to account_settings from packages/nc-gui/composables/useCommandPalette/index.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nc-gui/composables/useCommandPalette/index.ts (1 hunks)
Additional comments not posted (3)
packages/nc-gui/composables/useCommandPalette/index.ts (3)
172-172: Early return when the active scope is 'root' is correctly implemented.
174-174: Setting the active scope to 'root' is correctly handled.
176-176: Correctly triggersloadScopewhen the active scope is set to 'root'.
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of