christianwach / civicrm-admin-utilities

CiviCRM Admin Utilities is a WordPress plugin that modifies and enhances CiviCRM's appearance and behaviour in single site and multisite WordPress installs.
https://wordpress.org/plugins/civicrm-admin-utilities/
GNU General Public License v2.0
20 stars 10 forks source link

show CiviCRM shortcut link on users.php on multisite #15

Closed andyburnsco closed 4 years ago

andyburnsco commented 5 years ago

On multisite a site admin (not administrator) may have edit_users capability but not view all contacts. It should check for 'view all contacts in domain' for multisites. Still doesn't show on user-edit.php if not administrator though...

christianwach commented 5 years ago

@andyburnsco Thanks for the PR Andy. Couple of questions:

Also, if it's correct, the new logic would need duplicating in profile_extras() as well.

BTW, what do you mean by:

Still doesn't show on user-edit.php if not administrator though...

Cheers, Christian

andyburnsco commented 5 years ago

When you say "On multisite a site admin (not administrator)" do you mean "On multisite a site admin (not network administrator)"?

No, not administrator either...a newly created role that can create/manage users. It's true if someone is 'Administrator' they see the CiviCRM contact link. But that role has not been appropriate for manage A SITE on multisite per https://docs.civicrm.org/sysadmin/en/latest/install/multi-site/#wordpress-multisites where it says:

Recommendation: Create a Site Admin role for any administrator of sub-sites that are sharing CiviCRM, meaning the default Administrator role is only used on the main site. This is recommended since by default all Adminstrators automatically get access to Administer CiviCRM and will be able to create things like custom data fields. By creating a Site Admin role the CiviCRM permissions can be managed from Administer > Users and Permissions > Permissions (Access Control) and will allow only main site administrator access to Administer CiviCRM.'

My understanding is this recently changed to make administrator role work for on child sites but simply it is too restrictive for just needing to show civi contact link. View all contacts or view all contacts in domain is more appropriate.

What does view all contacts in domain return on non-multisite WordPress? Depending on what it returns, there might need to be a is_multisite() check in there too.

I tested and the behavior is unchanged. Civi link shows for view all contacts perm or administrator role.

Are you sure both permission checks should fail before testing view my contact? Might it be OR logic instead?

I tried

if ( ! $this->check_permission( 'view all contacts' ) || if ! $this->check_permission( 'view all contacts in domain' ) ) { but that did not work.

This request will help because in the situation where I need to add WP user and then navigate to their civi contact to finish their permissions (say a related permissions setup); it will make it more cohesive.

Also, if it's correct, the new logic would need duplicating in profile_extras() as well.

BTW, what do you mean by:

Still doesn't show on user-edit.php if not administrator though...

It doesn't show when on the profile edit screen if you are not network admin or administrator. It does show on hover on user.php. It should show on both and that is what you are saying with

new logic would need duplicating in profile_extras() as well.

christianwach commented 5 years ago

@andyburnsco I'm struggling a bit with this -- mainly because view all contacts in domain is a permission provided by the org.civicrm.multisite extension. It won't be present in "normal" CiviCRM installs.

Furthermore, if someone does have view all contacts in domain and not view all contacts, wouldn't there have to be a check that the WordPress site on which they are viewing the User list or User Profile maps to a Domain for which they have that permission?

FWIW, you could use the civicrm_admin_utilities_permitted filter to roll your own custom check.

andyburnsco commented 4 years ago

We might be making this more complicated that it needs to be? There are 2 places I would see view civicrm contact. 1) if I am viewing my own profile and 2) if I can manage WP users.

Seems to me that view my contact permissions should be all that is needed. Users should be able to easily go to their own Civi contact if they have view my contact permission.

christianwach commented 4 years ago

Users should be able to easily go to their own Civi contact if they have view my contact permission.

@andyburnsco Yes, I fully agree with you. However, I see nothing wrong with the current logic which always shows all links if the user has view all contacts. If they don't have that permission, then it always shows the link to their own Contact if they have view my contact.

There are 2 places I would see view civicrm contact. 1) if I am viewing my own profile

Yes, agreed. If it's my own profile, then:

In what scenario are you saying this logic breaks down?

and 2) if I can manage WP users.

Partly agree - even if a user passes checks for current_user_can( 'edit_user', $user->ID ) then surely view all contacts still needs to be checked? To paraphrase your original scenario:

On multisite a role (not administrator) may have edit_users capability but not view all contacts permission

In this case, they will see their own Contact link on their own Profile and in the User List if they have view my contact permission. You understandably want them to see all Contact links (where that Contact is in the Domain) if they have view all contacts in domain.

As I said previously, view all contacts in domain is a permission provided by the org.civicrm.multisite extension. By analogy therefore, modifying checks for view all contacts should be done by using the civicrm_admin_utilities_permitted filter. I can see persuasive arguments for that filtering to happen in one or more of the following:

What I can't see is an argument for your proposed change in CiviCRM_Admin_Utilities_Single which is described at the top of the file as "a class that encapsulates Single Site admin functionality."

christianwach commented 4 years ago

@andyburnsco So I've been thinking this through a bit more and I've gone for a different approach entirely. Instead of trying to second-guess what combination of permissions will give the right result, I have delegated the decision on whether a Contact is viewable to CiviCRM. You can try out the new logic in this branch. I think it's a better approach but (obviously) let me know if it still doesn't work for you.

christianwach commented 4 years ago

Resolved in f7bc58a506fb0310f82039292f2eabf0ab6490d9

andyburnsco commented 4 years ago

I've tested and I see the Civi contact link in the users.php and users-edit.php. Thanks for changing the approach!