search

processwire / processwire-issues

ProcessWire issue reports.
44 stars 2 forks source link

$user->editable returns false even if the current user has the user-admin permission #1322

Open schwarzdesign opened 3 years ago

schwarzdesign commented 3 years ago

Short description of the issue

Using $user->editable is unreliable because it returns different results based on the current Process. This results in incorrect behaviour when used in a custom context or Process module. In particular, unless the current user is a superuser, $user->editable evaluates to false even if the current user has the user-admin permission and can, in fact, edit the user in question using ProcessUser.

Expected behavior

$user->editable should always return the correct result (i.e. if the current user can edit the user in question, based on whether they have the required permission), independent of the current Process or context.

Actual behavior

The method responsible for returning the result for editable checks the current Process module and just returns false if it isn't one of the 'whitelisted' ones. See PagePermissions.module#L276-L280

Screenshots/Links that demonstrate the issue

This came up in the context of the Dashboard module, which allows you to display a collection of pages, including edit / view links. The module uses $page->editable to check if the current user can edit the pages in the collection. When using this to display a list of users, the module incorrectly disables the edit link since $page->editable returns false, even if the current user has the user-admin permission (unless they are also a superuser).

Suggestion for a possible fix

The best solution would be to make $user->editable context-independent. Whether a user account is editable should be based on the permission system, not on context / current process. The current implementation will always yield unexpected results, since it can return different results when used in different places, which violates SOLID principles.

If it is unfeasible / too much work to drop the context-dependence, it would be great if ProcessWire could provide an alternative to $user->editable that returns the correct result independent of context. This would allow third-party modules to check for this special case without replicating core logic.

Steps to reproduce the issue

Setup/Environment

Toutouwai commented 3 years ago

I agree that this is a problem.

We need some core method that answers the question: "Is the current user allowed to edit the given user in ProcessUser?" regardless of whether or not the current process is ProcessUser. I have no problem with restricting the Process modules through which a user may be edited, but there can still be cases where you simply need an answer to the question above in some other context besides ProcessUser.

And perhaps it would be better to limit user editing to Process modules that are instances of ProcessUser rather than restricting by the classname, in case somebody wants to extend ProcessUser.

ryancramerdesign commented 3 years ago

@schwarzdesign In the core, a user is not editable unless through ProcessUser. If User::editable returned true when ProcessUser was not the current process, then it could enable one to edit in any WirePageEditor Instance, whereas only ProcessUser is aware of user-specific security matters such as a non-superuser not being able to assign a superuser role and things of that sort. That is the purview of ProcessUser and nothing else. No user is editable unless you first go through ProcessUser. It would not be safe for User::editable to be returning true in other contexts because the user is never editable in any other contexts. Keep in mind that an editable() call doesn't just indicate whether you can show an edit link, but whether it will actually let them edit the page/user. If a module wants to account independently for those considerations then it can with its own check like below, or whatever conditions are necessary in your installation. However, I think it's best that edit links are not shown for users unless directly from ProcessUser.

$user->hasPermission('user-admin') && !$editUser->hasRole('superuser');
Toutouwai commented 3 years ago

@ryancramerdesign, how about making ProcessUser::getEditableRoles() public so that it's a bit easier to answer the question "Is the current user allowed to edit the given user in ProcessUser?" outside the context of ProcessUser?

teppokoivula commented 3 years ago

Just to provide a bit more context, VersionControl is one case where I need to check if current user has edit access on specific Page, which may or may not be a User Page. There I ended up switching the process in the middle of the request. Obviously not a very clean solution, but I wasn't sure that I could catch all potential factors that might now or in the future core versions affect user's "editability".

Checking for user-admin or user-admin-all (etc.) won't catch the case where the user is editing him/herself via ProcessProfile, though that's admittedly kind of a special case.

schwarzdesign commented 3 years ago

@ryancramerdesign Thanks for the insights and the workaround! But I want to argue why I still think this is an issue that should be addressed in the core.

The root of the problem is that the logic for when/how a user is allowed to edit another user is closely tied to a Process, which in turn is closely tied to a particular page/route in the backend. The assumption here is that users only ever need to edit other users manually through the interface provided by ProcessWire, but that assumption just isn't true for many use cases. Of course, you can say those use-cases aren't valid, but that is a severe limitation for some projects. So if we need those editing capabilities for a project, this would be an argument against using ProcessWire.

If User::editable returned true when ProcessUser was not the current process, then it could enable one to edit in any WirePageEditor Instance, whereas only ProcessUser is aware of user-specific security matters such as a non-superuser not being able to assign a superuser role and things of that sort.

I understand that reasoning from a technical perspective. But from a user-side, the limitation doesn't make sense, it's only the result of technical difficulties with the architecture of ProcessWire. From a user perspective, there's no reason why I shouldn't be able to answer the question "Can the current user edit this user?" from anywhere. Just returning false in this case is a band-aid for the underlying architecture problem, which is that the business logic (i.e. "editability" of a user) is closely tied to a particular interface/route. That tight coupling is a problem because it means developers are out of luck as soon as their use-case doesn't match the assumptions made by the core.

A good solution would be to separate the business logic (all the rules regarding user roles, special case handling for super users, etc) into a separate class from the ProcessUser class and just call that from ProcessUser. That would allow developers to use the business logic independently of a particular interface. A good example for this approach are the services in Craft CMS, which provide helper functions to perform common tasks. Those are completely independent of the Controllers that perform actions for particular routes. Both can be used independently from each other, so I can both call a service directly from my own code or point a form to a controller endpoint, without having to go through a particular route or user interface provided by Craft's core.