fix(permissions): All permissions functions handle user fetches consistently #8944
Conversation
mrclay
force-pushed
the
permissions_check_user_8941
branch
3 times, most recently
from
efe5899
to
09d49f1
Compare
| $result = true; | ||
| } | ||
|
|
||
| // If the user can edit the entity this is attached to, they can edit. | ||
| $entity = $this->getEntity(); |
There was a problem hiding this comment.
I moved this line above the $user check so $entity is defined either way.
mrclay
force-pushed
the
permissions_check_user_8941
branch
from
09d49f1
to
c047124
Compare
| } | ||
|
|
||
| $return = false; | ||
| if (_elgg_services()->session->isAdminLoggedIn()) { |
There was a problem hiding this comment.
I assume this was a bug (checking if the logged in user was admin, even if you pass in a different GUID).
mrclay
force-pushed
the
permissions_check_user_8941
branch
from
c047124
to
6fca089
Compare
| public function getUserForPermissionsCheck($guid = 0) { | ||
| if (!$guid) { | ||
| return _elgg_services()->session->getLoggedInUser(); |
There was a problem hiding this comment.
I've been wondering whether we should start discouraging the use of permission checks without passing in the user. It would make checks safer (forces devs to think what exactly they're doing instead of relying on magic) and also would reduce the complexity of core (less stuff to inject).
This is just a hunch however. I'm not sure if it would also introduce problems.
There was a problem hiding this comment.
I've been doing this with my own code for some time now for these exact reasons. I'm starting to dislike the magic default of logged in user more and more as I work around edge cases.
There was a problem hiding this comment.
Anyway, thanks to this function we now have a good place to trigger the deprecation message in case we later decide to deprecate the magic.
|
The |
mrclay
force-pushed
the
permissions_check_user_8941
branch
from
6fca089
to
9b4ae87
Compare
| * will not be returned unless its type matches. | ||
| * @param string $subtype Entity subtype (use "" for default subtype). If given, even an existing | ||
| * entity with the given GUID will not be returned unless this matches. | ||
| * @param bool $ignore_condition If true, hidden/disabled entities may be found |
There was a problem hiding this comment.
I think $ignore_access would be better name because devs are already familiar with it.
There was a problem hiding this comment.
True, but it does more, digging up disabled entities. Maybe two separate options?
There was a problem hiding this comment.
Ah, true. I suppose the current name is fine.
|
LGTM |
…stently All permissions functions now find hidden/disabled users and, when a given GUID isn't found, they return false and issue a warning. Previously, only some functions issued warnings, and some functions would silently substitute the logged in user if a given GUID couldn't load. Fixes Elgg#8941 Fixes Elgg#8038 Fixes Elgg#8945
mrclay
force-pushed
the
permissions_check_user_8941
branch
from
9b4ae87
to
b875fd3
Compare
|
I've limited this to what's necessary to implement the fix. |
|
I believe this is safe to merge? |
fix(permissions): All permissions functions handle user fetches consistently
All permissions functions now find hidden/disabled users and, when a given GUID isn't found, they return false and issue a warning. Previously, only some functions issued warnings, and some functions would silently substitute the logged in user if a given GUID couldn't load.
Fixes #8941
Fixes #8038
Fixes #8945