-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Migrate roles and permissions to MetaDB #7877
base: develop
Are you sure you want to change the base?
Conversation
@o1lab P.S. The test failures seem unrelated to my changes, am I missing something? Maybe the tests are flaky? |
WalkthroughWalkthroughThe overall change introduces a more granular and flexible role-based permission system. It involves updating user and authentication models to include role permissions, enhancing the backend to handle permission checks more effectively, and migrating the database schema to support roles and permissions management. This change allows for a dynamic assignment of permissions to roles, improving the system's security and customizability. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
packages/nc-lib-gui/package-lock.json
is excluded by:!**/*.json
packages/nocodb/src/schema/swagger.json
is excluded by:!**/*.json
Files selected for processing (10)
- packages/nc-gui/composables/useRoles/index.ts (7 hunks)
- packages/nc-gui/lib/types.ts (1 hunks)
- packages/nocodb-sdk/src/lib/Api.ts (1 hunks)
- packages/nocodb/src/controllers/auth/auth.controller.ts (1 hunks)
- packages/nocodb/src/meta/migrations/XcMigrationSourcev2.ts (3 hunks)
- packages/nocodb/src/meta/migrations/v2/nc_043_roles_permissions.ts (1 hunks)
- packages/nocodb/src/middlewares/extract-ids/extract-ids.middleware.ts (4 hunks)
- packages/nocodb/src/services/users/users.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (2 hunks)
- packages/nocodb/src/utils/globals.ts (1 hunks)
Additional comments: 14
packages/nc-gui/composables/useRoles/index.ts (2)
- 6-16: The changes to the
hasPermission
function, including the addition of therolePermission
parameter and the updated logic for checking permissions, are correctly implemented and enhance the flexibility of permission checking.- 106-112: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [91-129]
The inclusion of
role_permissions
in the user object and the updated logic for checking permissions based on roles are correctly implemented. These changes enhance the system's ability to manage and check permissions in a centralized manner.Consider caching or optimizing the fetching of role permissions to improve performance, especially if there are many roles or if the permissions data is large.
packages/nc-gui/lib/types.ts (1)
- 16-16: The addition of the
role_permissions
field to theUser
interface is correctly implemented and necessary for storing the permissions associated with each role a user has.packages/nocodb/src/utils/globals.ts (1)
- 49-51: The addition of the
ROLES
,PERMISSIONS
, andROLES_PERMISSIONS
enum values to theMetaTable
enum is correctly implemented and aligns with the migration towards a more dynamic and customizable access control mechanism.packages/nocodb/src/meta/migrations/v2/nc_043_roles_permissions.ts (1)
- 20-178: The migration script
nc_043_roles_permissions.ts
is well-structured and correctly implements the creation of theROLES
,PERMISSIONS
, andROLES_PERMISSIONS
tables, along with the initial population of these tables with roles and permissions. The script follows best practices for database migrations and correctly handles different scopes and types for permissions, including special handling for thesuper
role.packages/nocodb/src/controllers/auth/auth.controller.ts (1)
- 126-131: The enhancements to the
AuthController
, specifically the logic to retrieve role permissions for all roles associated with a user and include these permissions in the returned user object, are correctly implemented. These changes provide a clearer view of the user's capabilities and access rights within the system.packages/nocodb/src/meta/migrations/XcMigrationSourcev2.ts (1)
- 32-32: The addition of the
nc_043_roles_permissions
migration to theXcMigrationSourcev2
class is correctly implemented, ensuring that the new migration script is integrated into the system's migration process.packages/nocodb/src/middlewares/extract-ids/extract-ids.middleware.ts (5)
- 27-28: The addition of imports for
Noco
andMetaTable
is noted. Ensure that these modules are used appropriately within the file and that their functionalities align with the intended changes in the middleware.- 41-47: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [44-54]
The modification in
getRolesLabels
to utilize theallRoles
parameter for role mapping introduces a more dynamic way of handling roles. This change aligns with the PR's objective of enhancing role and permission management. However, consider adding error handling for cases whereallRoles.find((item) => item.name === role)
returnsundefined
, which would lead toundefined
being included in the returned array.- .map((role) => allRoles.find((item) => item.name === role)?.label); + .map((role) => allRoles.find((item) => item.name === role)?.label).filter(label => label !== undefined);
- 261-263: Retrieving
allRoles
usingNoco.ncMeta.knex(MetaTable.ROLES).select('name', 'label')
is a critical change that supports the dynamic checking of roles against permissions. Ensure that this query is optimized for performance, especially ifAclMiddleware
is used frequently. Consider caching the roles if they don't change often to reduce database load.- 273-287: The retrieval and usage of
rolePermissions
to check if the user has the specific permission required for the action is a significant enhancement. This aligns with the PR's objective of centralizing and streamlining permission checks. Ensure that the permissions are correctly defined in the database and that the query efficiently retrieves the necessary information without causing performance issues.- 289-294: The final check for
isAllowed
and the subsequent error handling if the user is not allowed is crucial for enforcing permissions. The usage ofgetRolesLabels
to provide a detailed error message is a good practice. Ensure that the error messages are clear and informative to aid in debugging and user understanding.packages/nocodb/src/utils/acl.ts (2)
- 14-14: Exporting
permissionScopes
is a necessary change to make these scopes accessible outside the module, which aligns with the PR's objectives of enhancing role and permission management. Ensure that the permissions defined within are accurate and comprehensive.- 435-514: The addition of
notAssignedPermissionsBE
to define permissions for superadmins that are not assigned to any roles is a significant enhancement. This allows for a more granular control of permissions, especially for superadmins. Ensure that all permissions listed are necessary and correctly categorized underorg
andbase
scopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
packages/nc-lib-gui/package-lock.json
is excluded by:!**/*.json
packages/nocodb/src/schema/swagger.json
is excluded by:!**/*.json
Files selected for processing (10)
- packages/nc-gui/composables/useRoles/index.ts (7 hunks)
- packages/nc-gui/lib/types.ts (1 hunks)
- packages/nocodb-sdk/src/lib/Api.ts (1 hunks)
- packages/nocodb/src/controllers/auth/auth.controller.ts (1 hunks)
- packages/nocodb/src/meta/migrations/XcMigrationSourcev2.ts (3 hunks)
- packages/nocodb/src/meta/migrations/v2/nc_043_roles_permissions.ts (1 hunks)
- packages/nocodb/src/middlewares/extract-ids/extract-ids.middleware.ts (4 hunks)
- packages/nocodb/src/services/users/users.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (2 hunks)
- packages/nocodb/src/utils/globals.ts (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- packages/nc-gui/composables/useRoles/index.ts
- packages/nc-gui/lib/types.ts
- packages/nocodb-sdk/src/lib/Api.ts
- packages/nocodb/src/controllers/auth/auth.controller.ts
- packages/nocodb/src/meta/migrations/XcMigrationSourcev2.ts
- packages/nocodb/src/meta/migrations/v2/nc_043_roles_permissions.ts
- packages/nocodb/src/middlewares/extract-ids/extract-ids.middleware.ts
- packages/nocodb/src/services/users/users.service.ts
- packages/nocodb/src/utils/acl.ts
- packages/nocodb/src/utils/globals.ts
What is remaining for this to move forward? |
Change Summary
isUIAllowed
on frontend checking existing permissions in user.This is the first step to adding “Custom roles” functionality.
My plan is create page “Role management” in settings where user can see all roles in MetaDB and also create their own role from scratch or create own role based on an already existing one.
Next step I want to add page where user can manage permission for every role: add or remove some permissions from different roles.
This functionality is important for us and as far as we see for many other people too (#2712). We are open to your reviews, and proposals. We are ready to work on it.
Change type
Test/ Verification
I tested on Postgres and SQLite.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of
Summary by CodeRabbit
role_permissions
, allowing for more precise permission checks throughout the application.