fix: Deleting a role does not invalidate the cached role closures - #10620
fix: Deleting a role does not invalidate the cached role closures#10620AdrianCurtin wants to merge 4 commits into
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe REST delete flow now clears the global role cache and cached LiveQuery roles when deleting ChangesRole deletion cache invalidation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This localized change invalidates cached role closures when roles are deleted, with focused tests covering the new behavior; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a security-relevant correctness gap in Parse Server’s role caching: deleting a _Role now invalidates cached role closures (and clears LiveQuery’s cached roles), matching the existing behavior on role create/update so access via role:<name> ACLs can’t linger until cache TTL expiry.
Changes:
- Clear
cacheController.role(andliveQueryController.clearCachedRoles) after_Roledeletion inrest.js. - Add regression coverage to ensure role deletes clear the role cache while non-role deletes do not.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/rest.js |
Clears role-related caches when deleting a _Role, aligning delete behavior with role writes. |
spec/ParseRole.spec.js |
Adds specs validating role-cache invalidation on role deletion and non-invalidation on other deletes. |
Suppressed comments (1)
spec/ParseRole.spec.js:704
- Like the previous spec, this uses a fixed sleep which can be flaky and slows the suite. Since the behavior you care about is that role cache clearing is not triggered on non-role deletes, you can assert that deterministically by spying on
cacheController.role.clear()and verifying it was not called (no timeout needed).
await object.destroy({ useMasterKey: true });
await new Promise(resolve => setTimeout(resolve, 200));
expect(await cacheController.role.get('someUser')).toEqual(['role:Admin']);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/ParseRole.spec.js (1)
679-705: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for LiveQuery role-cache invalidation.
These tests inspect only
Parse.Server.cacheController.role. They do not exerciseconfig.liveQueryController.clearCachedRoles(auth.user), which is part of this change. Add a LiveQuery-enabled case or a focused controller spy. Include the master-key deletion path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/ParseRole.spec.js` around lines 679 - 705, Extend the role-deletion cache tests around the existing role destroy case to cover LiveQuery invalidation through config.liveQueryController.clearCachedRoles(auth.user), not only cacheController.role. Enable LiveQuery or spy on the controller, verify clearCachedRoles is invoked for the deleted role’s user, and retain the useMasterKey deletion path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/ParseRole.spec.js`:
- Around line 689-690: Replace the fixed 200 ms sleeps in the role-deletion test
around the clear operation with a bounded polling or eventual assertion that
waits until the deleted role is no longer returned from the cache. Assert the
non-role cache immediately after deletion, preserving deterministic timeout
behavior without relying on elapsed time.
In `@src/rest.js`:
- Around line 244-255: Update the _Role deletion handling in RestRequest and the
corresponding RestWrite role-write path so LiveQuery invalidation does not
depend on auth.user and reaches every session affected by the deleted role,
including users inheriting it through child roles. Add or reuse a
role-id/role-orphan invalidation mechanism that propagates the deleted role’s
impact and clears all matching cached role sessions, while preserving the
existing global role cache clear.
---
Nitpick comments:
In `@spec/ParseRole.spec.js`:
- Around line 679-705: Extend the role-deletion cache tests around the existing
role destroy case to cover LiveQuery invalidation through
config.liveQueryController.clearCachedRoles(auth.user), not only
cacheController.role. Enable LiveQuery or spy on the controller, verify
clearCachedRoles is invoked for the deleted role’s user, and retain the
useMasterKey deletion path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4301386e-5393-485c-8189-877595d4d466
📒 Files selected for processing (2)
spec/ParseRole.spec.jssrc/rest.js
Refactors ParseRole cache invalidation specs to use the app config controllers instead of global server state, and adds explicit spies for both role cache and LiveQuery role cache clearing. The role-deletion test now waits on the actual clear promise rather than a fixed timeout, making it deterministic, while the non-role deletion test verifies no cache clear methods are called.
95e737c to
b43e6bb
Compare
Pull Request
Issue
Closes #10619.
Deleting a
_Roledid not invalidate the role cache, so every user who held that role kept it in their cached closure until the entry expired. ACLs granting access torole:<name>continued to be honored for a role that no longer existed.cacheController.role.clear()was called from only two places,RestWrite.js:1566(create and update) andPurgeRouter.js:22. Deletes do not go throughRestWrite; they go throughrest.jsdel(), which had no role cache handling at all.The asymmetry is what makes it a bug rather than a tradeoff: removing a user from a role is a
_Roleupdate and clears the cache correctly, while deleting the whole role revokes the same access from every member and cleared nothing.Approach
Clear the role cache in
rest.jsdel()whenclassName === '_Role', mirroringRestWrite#runDatabaseOperation, including the accompanyingliveQueryController.clearCachedRolescall that the delete path was also missing.Two details worth reviewer attention:
.then()followingdatabase.destroy, rather than before the write asRestWritedoes. Clearing before the write leaves a window in which a concurrent read repopulates the closure from pre-delete state, which would outlive the invalidation.hasTriggers || hasLiveQuery || className == '_Session'branch, so it runs for a_Rolewith no triggers registered. That branch is why the existingcacheAdapter.user.delatrest.js:199does not run for most classes.RestWrite.js:1566. Awaiting would make a Redis blip fail the delete, sinceRedisCacheAdapter#clearhas no internal error handling, and that seemed the worse trade. Happy to change it if maintainers prefer.The whole role cache is cleared rather than one user's entry, for the same reason
RestWritedoes it: the cached value is a flattened transitive closure, so deleting a parent role affects the members of every child role, and the delete does not identify which users are affected.This is independent of #10618 and applies with or without it. With that change merged,
role.clear()becomes a scopedSCANover<appId>:role:*instead of aFLUSHDB, which makes adding this call inexpensive.Tests
spec/ParseRole.spec.jsgains two specs:alphawithExpected [ 'role:Doomed' ] to equal nulland passes here.The full
spec/ParseRole.spec.js(20 specs) andspec/Auth.spec.js(11 specs), the latter being the main consumer of the role cache, both pass against MongoDB 8.Tasks
Summary by CodeRabbit