-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat(rbac): role-based access control module #14310
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
base: develop
Are you sure you want to change the base?
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 8 Skipped Deployments
|
olivermrbl
left a comment
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.
Overall LGTM, a few suggestions and comments
| .define("rbac_role_policy", { | ||
| id: model.id({ prefix: "rlpl" }).primaryKey(), | ||
| role: model.belongsTo(() => RbacRole), | ||
| scope: model.belongsTo(() => RbacPolicy), |
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.
suggestion: wouldn't it make more sense to call this policy?
| scope: model.belongsTo(() => RbacPolicy), | |
| policy: model.belongsTo(() => RbacPolicy), |
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.
resurfacing this one
| .define("rbac_role_inheritance", { | ||
| id: model.id({ prefix: "rlin" }).primaryKey(), | ||
| role: model.belongsTo(() => RbacRole, { mappedBy: "inherited_roles" }), | ||
| inherited_role: model.belongsTo(() => RbacRole, { |
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.
suggestion: we have historically used parent_ for inheritance relationships; would it make sense to use it here too? not a strong opinion, so also fine to keep as is
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.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 17
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
| ) | ||
|
|
||
| createRbacRolePoliciesStep(policiesUpdateData) |
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.
Update workflow only adds policies, never removes existing
The updateRbacRolesWorkflow handles policy_ids inconsistently with inherited_role_ids. For inheritance, setRoleInheritanceStep properly syncs by fetching existing records, comparing, removing old ones, and adding new ones. However, for policies, it uses createRbacRolePoliciesStep which only creates new associations without removing existing ones. When a user updates a role with policy_ids, they expect it to replace all policies, but instead it only appends new ones. This means clearing policies with an empty array won't work, and updating with a subset of policies will accumulate rather than replace.
| q: z.string().optional(), | ||
| id: z.union([z.string(), z.array(z.string())]).optional(), | ||
| name: z.union([z.string(), z.array(z.string())]).optional(), | ||
| parent_id: z.union([z.string(), z.array(z.string())]).optional(), |
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.
Validators reference nonexistent parent_id model field
The API validators (AdminGetRbacRolesParamsFields, AdminCreateRbacRole, AdminUpdateRbacRole) and defaultAdminRbacRoleFields query config all reference a parent_id field that doesn't exist in the RbacRole model. The model only defines id, name, description, and metadata. Role inheritance is handled through the separate RbacRoleInheritance table, not a parent_id column. This will cause failures when filtering by parent_id, creating/updating with parent_id, or when the default query fields are requested.
Additional Locations (2)
olivermrbl
left a comment
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.
Overall LGTM, few comments and suggestions
| role_id: string | ||
| previousInheritedRoleIds: string[] |
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.
super-nit: think we should align casing 😅
| import { adminRbacRolePolicyRoutesMiddlewares } from "./role-policies/middlewares" | ||
| import { adminRbacRoleRoutesMiddlewares } from "./roles/middlewares" | ||
|
|
||
| export const adminRbacRoutesMiddlewares: MiddlewareRoute[] = [ |
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.
q: should we omit this file if the feature flag is not enabled?
| AdminUpdateRbacPolicy, | ||
| } from "./validators" | ||
|
|
||
| export const adminRbacPolicyRoutesMiddlewares: MiddlewareRoute[] = [ |
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.
Same as other comment
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.
separately: I am leaning toward removing the rbac resource from the API; I think this goes against our general conventions, and I doubt we will have other features needing the routes without it
| AdminUpdateRbacPolicy, | ||
| } from "./validators" | ||
|
|
||
| export const adminRbacPolicyRoutesMiddlewares: MiddlewareRoute[] = [ |
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.
Same as other comment
| AdminUpdateRbacRolePolicy, | ||
| } from "./validators" | ||
|
|
||
| export const adminRbacRolePolicyRoutesMiddlewares: MiddlewareRoute[] = [ |
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.
Same as other comment
| .define("rbac_role_policy", { | ||
| id: model.id({ prefix: "rlpl" }).primaryKey(), | ||
| role: model.belongsTo(() => RbacRole), | ||
| scope: model.belongsTo(() => RbacPolicy), |
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.
resurfacing this one
What
Note
Introduce RBAC module with roles, policies, role-policies, and inheritance, exposing workflows and Admin API CRUD, plus integration tests and wiring across types/utils/config.
@medusajs/rbac):rbac_role,rbac_policy,rbac_role_policy,rbac_role_inheritancewith indexes and migrations.RbacModuleServiceandRbacRepository(policy listing with inheritance; augment role listings to includepolicies).Modulesconstants; addIRbacModuleServiceand RBAC DTOs/types.rbac_roles,rbac_policies,rbac_role_policies; manage role inheritance (set-role-inheritance); expose viacore-flows.GET/POST /admin/rbac/roles,GET/POST/DELETE /admin/rbac/roles/:id.GET/POST /admin/rbac/policies,GET/POST/DELETE /admin/rbac/policies/:id.GET/POST /admin/rbac/role-policies,GET/POST/DELETE /admin/rbac/role-policies/:id.common,mutations,service), extend container typings; addModules.RBACmappings.Written by Cursor Bugbot for commit 3d63ef0. This will update automatically on new commits. Configure here.