Problem/Motivation
We found out in #2248977: Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field that #2217749: Entity base class should provide standardized cache tags and built-in invalidation accidentally removed the call to delete the references.
This could possibly be a security related problem, if you delete a role and then create a new one with the same name, your users will still/again have it, and that won't be what you expected.
Proposed resolution
Add back a call to it. We also need to move the method to UserStorage, because it is just wrong on RoleStorage, as it is about stored users, not roles.
This is a generic problem that all entity reference fields have, but I don't know how to solve this in a generic way...
We also need to clear the cache with $this->resetCache().
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Bug because leaves orphan data in users after deletion of roles |
---|---|
Issue priority | Major because it is a potential security problem. |
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff-13-17.txt | 1.28 KB | Anonymous (not verified) |
#17 | user-role-permission-assignments-2390467-17.patch | 5.48 KB | Anonymous (not verified) |
#16 | interdiff-3-16.txt | 6.18 KB | Anonymous (not verified) |
#16 | user-role-permission-assignments-2390467-16.patch | 5.48 KB | Anonymous (not verified) |
#13 | user-role-permission-assignments-2390467-13-test-only.patch | 2.35 KB | Anonymous (not verified) |
Comments
Comment #1
BerdirComment #2
adamwhite CreditAttribution: adamwhite commentedIssue was reproduced and confirmed to still be happening. Looking at this for SprintWeekend2015.
Comment #3
adamwhite CreditAttribution: adamwhite commentedHere's a patch that calls RoleStorage::deleteRoleReferences() upon deletion of a role.
However for the first pass at this I've left it in RoleStorage rather than UserStorage. It could be argued that since cleaning up these references would be the responsibility of the Role the method should live there. Furthermore the methods in UserStorage seem to mostly be related so singular User entities and not the multiple as this method would address.
However it could absolutely run from UserStorage as well, you'd just swap:
for
$this->resetCache();
Comment #4
trevortwining CreditAttribution: trevortwining commentedI've successfully tested the patch in #3.
You can create a role, assign it to a user, delete the role, recreate the role, and it doesn't 're-inherit' the original assignment.
Comment #5
trevortwining CreditAttribution: trevortwining commentedComment #6
BerdirThanks for starting this.
The method really needs to move. roles are stored in configuration, user__roles is a field that belongs to the user. If you would store your users in MongoDB for example, then that method in RoleStorage would fail, so you would have to replace RoleStorage as well.
We have someone at our local sprint that will try to do this.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedI'll try it
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedI have moved the method from RoleStorage into the UserStorage.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #10
BerdirThis looks good, but we still need tests (which you are working on already as I can hear :))
Also, I'm wondering if Role::postDelete() should move to a user_role_delete() hook, as it the role entity should not have to know about it?
Comment #11
penyaskitoThanks @trevortwining, @adamwhite and @iPat for working on this!
We could use
$this->entityManager()
here, it is already injected.@berdir: Do you think this enough? Shouldn't the role cache be deleted too?
Comment #12
penyaskitoTalked with @Berdir on irc, and for #11.2 we don't need anything else.
Instead of #11.1, let's do
user_role_delete()
inuser.module as suggested in #10.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedaddressed #10, and move functionality to user_user_role_delete().
Also wrote a test and attached separate, should fail.
Comment #14
penyaskitoLooks great, let's see what the testbot thinks!
Only minor issues:
We shouldn't remove this space :/
"with same name"
Remove the "still"
Needs a blank line before the class end bracket per coding standards.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedI have corrected these points.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedFixed interdiff and also added a missing dot.
Comment #18
penyaskitoLooks good to me. Awesome work @iPat!
Filled beta evaluation form.
Comment #19
adamwhite CreditAttribution: adamwhite commentedThanks for taking the ball on this @iPat, I've tested on here and it looks great to me.
Comment #20
alexpottCommitted 99377f9 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
I discussed this with @Berdir in IRC - he pointed out that the general problem of entity reference cleanup is more a problem for config entities than content, because there is no risk of accidental re-assignment for them. I think we should open a followup to work on a generic fix.
Comment #23
jonathanshawThat follow-up now exists: #2723323: [META] Clean up references to deleted entities.