-
-
Notifications
You must be signed in to change notification settings - Fork 4k
feat(medusa): filter query fields #14588
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
…medusajs/medusa into feat/filter-query-fields
…medusajs/medusa into feat/filter-query-fields
…medusajs/medusa into feat/filter-query-fields
…medusajs/medusa into feat/filter-query-fields
…medusajs/medusa into feat/filter-query-fields
…medusajs/medusa into feat/filter-query-fields
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 8 Skipped Deployments
|
adrien2p
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.
🔥
| if (route.policies && !route.middlewares?.length) { | ||
| middlewares.push((_, __, next) => { | ||
| next() | ||
| }) | ||
| } |
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: any reason for pushing a noop there?
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.
Because we wrap the policy checks and run the original middleware, in this case, there were none.
Potentially I can check on the wrapper if there is any original function to be executed, but I was affraid of breaking something unexpected.
Will run some real tests and if I can have this removed, I'll do it in a next PR.
| * | ||
| * const readOp = PolicyOperation.read // "read" | ||
| * const writeOp = PolicyOperation.write // "write" | ||
| * const writeOp = PolicyOperation.create // "create" |
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: write op are used for create and update no?
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.
We can discuss that, if it makes sense to have create and update, or we just use write for everything.
cc: @olivermrbl
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.
yeah my concern here is that write is changed to create, but then what happen to update? could exist a case where someone cannot create something and only update existing objects?
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.
Hmm, a simple example that comes to mind is that customer service staff might not be allowed to create entire new orders, but only update them with e.g. shipping or customer info.
And if we think about this outside the context of commerce (e.g. on Cloud), I think these scenarios become even more realistic.
I would maybe go with create and update as separate actions for now, and see how that fits.
…t/filter-query-fields
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, will let @adrien2p review the filtering algo
integration-tests/http/__tests__/rbac/admin/rbac-field-filtering.spec.ts
Show resolved
Hide resolved
|
I am currently on the review @carlos-r-l-rodrigues, I might have some changes to propose but i ll let you know 👍 |
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.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Summary
This PR introduces RBAC-based field filtering for API responses and enhances the permission system to support wildcard resources ("*"), enabling super admin roles with full access.
🔧 RBAC Field Filtering
Added field filtering based on user permissions for API responses
Filters out requested fields (e.g., prices, tags) when users lack permissions
Configurable via new feature flag MEDUSA_FF_RBAC_FILTER_FIELDS
🎯 Wildcard Resource Support
Enhanced hasPermission function to support resource: "*" for all resources
Super admin roles can now access all resources and operations with : permissions
⚙️ Feature Flag
New feature flag: rbac_filter_fields (disabled by default)
Environment variable: MEDUSA_FF_RBAC_FILTER_FIELDS
Note
Introduces RBAC-aware field filtering for API queries and enables super-admin style wildcard permissions.
FieldParser,AllowedFieldFilter,RestrictedFieldFilter, and policy-backedRBACFieldFilter; updatesprepareListQuery/prepareRetrieveQueryto async, acceptreq, and filter disallowed fields whenrbac_filter_fieldsis enabledhas-permissionto support resource*and adjusts exports; updates middleware loader to attachpolicieseven without explicit handlers;check-permissionsnow uses new policy pathrbac_filter_fields(env:MEDUSA_FF_RBAC_FILTER_FIELDS) and comprehensive integration tests for field filtering and wildcard accessprices->price) and policy typings; JWT generation sets roles optionally; adapts affected routes/tests (e.g., cart complete) to async query utilsWritten by Cursor Bugbot for commit 51b295a. This will update automatically on new commits. Configure here.