fix(access): updates no longer mistakenly blocked in some scenarios #10103
Conversation
|
If a developer turns off access control to give you an entity, and canEdit() is true, you should be allowed to update. It doesn't matter if the entity would be invisible to you under other circumstances. Right? |
|
All that's really important is that caches (like memcache) check |
|
Looks right. We need to audit cache habdling. |
|
Actually, has_access_to_entity() need only be checked if access is not ignored. The only case for has access check without ignored access check is in notifications, IMO |
|
LGTM I just came across this last week. This bug forced me to make a direct database query to update the data instead of using the API. I agree that user should be allowed to update an entity if access is explicitly being ignored. |
|
I would really like some tests on this - they would make it clearer what the "scenarios" are |
| @@ -379,4 +379,25 @@ public function testCreateWithContainerGuidEqualsZero() { | |||
| $user->delete(); | |||
|
|
|||
| } | |||
|
|
|||
| public function testUpdateAbilityDependsOnCanEdit() { | |||
There was a problem hiding this comment.
@hypeJunction anything else we need to test here?
There was a problem hiding this comment.
Perhaps the reverse. Revoking edit permissions via hook for an owner
There was a problem hiding this comment.
Can't think of anything else.
There was a problem hiding this comment.
Is ignored access tested elsewhere?
There was a problem hiding this comment.
I went ahead and just added it.
There was a problem hiding this comment.
I will take another look tomorrow
mrclay
force-pushed
the
10098_update
branch
2 times, most recently
from
02032b6
to
d2acd02
Compare
In order to fix `canEdit()` in 1.9, `update()` needed to load a fresh copy of entity from the DB to check the persisted attributes. In that fix, Elgg@66eb9e6, a corner case was checked out of my paranoia that a dev would load an invisible entity with access control on, but try to delete it with access off. I now believe this check was unnecessary (we never did similar checks for `delete()`). When the original `canEdit()` attributes issue was resolved in
|
LGTM |
In order to fix
canEdit()in 1.9,update()needed to load a fresh copy of entity from the DB to check the persisted attributes. In that fix, 66eb9e6, a corner case was checked out of my paranoia that a dev would load an invisible entity with access control on, but try to delete it with access off.I now believe this check was unnecessary (we never did similar checks for
delete()). When the originalcanEdit()attributes issue was resolved in #9434, I should've made this change.(replaces #10098)