Fix stale dynamic permission presets after editing current account#27899
Conversation
a4db98a to
5d3d334
Compare
AI Assisted contributions are OK. You'll also want to read the Contribution Guidelines. I don't understand the relevance of what you've outlined in the 'tested scenarios' section, you might want to review the Common Commands to simplify your efforts there. |
|
Appreciate the pointers. I simplified the Tested Scenarios section to align with the common command wording, and removed the now-unneeded AI policy caveat from the disclosure. |
|
Your changes can't be merged until you add yourself to the contributors list; I don't think the Directus Team will review your code until you do that. |
|
Added |
ComfortablyCoding
left a comment
There was a problem hiding this comment.
Thanks for the PR! Unfortunately I am unable to reproduce the original issue on the latest version, so I can't confirm whether this fixes it or not. Could you share the steps to reproduce the issue?
Reproduction Attempt
Attempt was done with sqlite, db should not play a role here. No cache enabled.
Screen.Recording.2026-07-17.at.11.35.58.AM.mov
Schema
|
Thanks, that helped clarify the gap. My current patch covers the stale I’m going to broaden the patch so the PR covers the full reported user-save path rather than only the permissions rehydration side. I’ll update the tests/repro notes accordingly so it’s clear which behavior is covered. |
|
Thanks again for the reproduction note. I pushed an update in The PR now covers both app-side paths from the original report:
I also added focused coverage for the broadened relation-display path in Checks run with Node 22:
Those focused checks passed locally. |
|
Appreciate the updates, but neither of those addresses my review. I am unable to reproduce the original issue and therefor cannot verify whether the proposed fix resolves it. |
|
Thanks, you’re right. My previous updates did not give the actual reproduction path you asked for. I can reproduce the original report when I use the version/environment from the issue rather than a current unlicensed setup:
{
"tenant_id": "$CURRENT_USER.builder_tenant_id"
}Steps that reproduce it:
Observed result on
I also checked the backend side separately: after updating One note about the latest/sqlite reproduction attempt: on The permissions-store refresh in this PR is the change tied to the reproduced create failure above: after the current user is hydrated from the save, the permissions store is hydrated again so |
|
Thanks for the reproduction steps, I can reproduce the issue now. 🙂 That said, I don't think refreshing permissions is the correct solution here. It addresses the symptom rather than the root cause, since the permissions themselves are not actually stale in this case. The stale value is the preset. I believe a better approach would be to remove Thoughts? |
|
Thanks, that makes sense. I updated the PR in The permissions store now keeps the raw preset payload from I also removed the route-level permissions refresh and its regression test, and added store-level regression coverage that verifies the preset updates after the current user changes without another Happy to adjust further if you would prefer this shaped differently. |
|
Thanks for the updates! This is definitely an improvement over the re-fetch approach, but I don't think this is the right place for it.
I believe the better approach would be to leave This would keep the preset values in sync with runtime changes without introducing side effects or unnecessary work into Thoughts? |
|
Thanks, that makes sense. I updated this in
The updated tests now cover both sides of that boundary:
The existing relation-display fallback from the broader report remains in place. Local focused checks passed:
The current PR check rollup is green as well. Happy to adjust further if you would prefer the helper placement or naming shaped differently. |
The /permissions/me endpoint already resolves $CURRENT_USER and $CURRENT_ROLE preset variables server-side, so the client-side parsePreset and related operations are redundant.
|
After further investigation, the Apologies for the confusion, the original solution appears to be the right approach given the current state. I’ve reverted the changes back to that implementation and removed the outdated code 🫡 |
ComfortablyCoding
left a comment
There was a problem hiding this comment.
LGTM! Thanks and congratulations on your first contribution to directus! 🥳
|
Woohoo! Thank you! Glad we could help. 🙏 |
Record directus/directus#27899 merge and issue closure. Note the public maintainer correction confirming the original refresh-permissions approach was the right one for the current implementation. Update validation, review status, public links, and changed-file coverage for the merged PR.
What's Changed
Tested Scenarios
Review Notes / Questions / Concerns
Checklist
@directus/specs)@directus/sdk) updated to reflect the changes@directus/types) updated to reflect the changes@directus/system-data) updated for changes to system collections/fields/relationsFixes #24029
Disclosure: I used AI-assisted coding tools while preparing this PR. I reviewed the changes myself, tested them, and take responsibility for the implementation and any follow-up revisions needed.