fix(content-manager): use RBAC-aware populate in countDraftRelations#25977
Conversation
|
@Akash504-ai is attempting to deploy a commit to the Strapi Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @Akash504-ai thanks for this work! It looks like the right fix. I'd just like to improve the test coverage slightly, would you be able to apply this .diff and update the branch ? |
|
Hi @jhoward1994, I have applied the suggested changes and updated the tests to improve RBAC coverage. All tests are passing locally. Please let me know if anything else is needed . |
innerdvations
left a comment
There was a problem hiding this comment.
LGTM!
@jhoward1994 since you've been working on admin performance, do you think this will have any significant impact?
|
No major performance concern from me. It's a findOne, not a list so I think it's one extra join. We also definitely need this in order to fix the 403 issue. |
What does it do?
Fixes an issue in
countDraftRelationswhere the entity was loaded withpopulate: {}, preventing RBAC conditions from being evaluated correctly.The controller now uses the RBAC-aware populate builder (
populate-builder) based onpermissionChecker.sanitizedQuery, aligning it with other controllers likefindOneandupdate.Also adds a test to ensure the endpoint does not return a 403 when RBAC conditions such as "same as creator" or "same role" are enabled.
Why is it needed?
When RBAC conditions like "same as creator" or "same role" are enabled, the controller failed to populate required relational fields (e.g.,
createdBy), causing permission checks to fail and return a 403 error.This resulted in a user-facing error:
How to test it?
Create a role with:
Create an entry as that user
Open / Save / Publish the entry
Before fix:
countDraftRelationsreturns 403After fix:
Also covered by added unit test:
countDraftRelations.test.tsRelated issue(s)/PR(s)
Fixes #25965