Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SearchKit - Add "administer search_kit" permission #23670

Merged
merged 3 commits into from
Jun 4, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jun 2, 2022

Overview

Adds a permission which allows non-admins to use Search Kit.

See https://lab.civicrm.org/dev/core/-/issues/3457

Before

Previously the user needed 'administer CiviCRM data' permission to use Search Kit.

After

image

Technical Details

When updating the navigation menu permissions, I switched it over to use a .mgd.php to insert the menu item instead of creating it with the installer. This should also work better for multi-domain sites.

@civibot
Copy link

civibot bot commented Jun 2, 2022

(Standard links)

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Jun 2, 2022

I know there's no standard and it's already mixed just I think it would be neat if all the internal names of permissions were all lower-case, to avoid mismatches that have come up before.

Not sure what the fail is about:

CRM_Member_Form_MembershipTest::testFinancialEntriesOnCancelledContribution
Failed asserting that '-20.00' matches expected -259.0.

/home/jenkins/bknix-dfl/build/core-23670-3djbs/web/sites/all/modules/civicrm/tests/phpunit/CRM/Member/Form/MembershipTest.php:941

jenkins retest this please

@colemanw colemanw force-pushed the searchKitAddPerm branch from 4247757 to 0239941 Compare June 2, 2022 18:34
@colemanw
Copy link
Member Author

colemanw commented Jun 2, 2022

@demeritcowboy that would be nice in retrospect. And we can do it going forward. I've changed this one to "administer search_kit" since that's the proper name of the extension anyway, good for it to match!

@demeritcowboy
Copy link
Contributor

Working: Don't give administer civicrm or administer civicrm data or administer search_kit => can't do admin stuff but can use existing search displays.

But then add the permission to administer search_kit and while I can now see the admin interface, if I try to save a new search I get a spinning save button. It gives error 403 with {"error_code":"1","error_message":"Sorry an error occurred and your request was not completed. (Error ID: UYOC-Qphn-rkNB)"}

@eileenmcnaughton
Copy link
Contributor

sounds like progress :-)

This permission allows non-admins to use search kit.
Previously the user needed 'administer CiviCRM data' permission.

Fixes dev/core#3457
@colemanw colemanw force-pushed the searchKitAddPerm branch from 0239941 to f02286d Compare June 3, 2022 03:30
@colemanw
Copy link
Member Author

colemanw commented Jun 3, 2022

How about that, it turns out there was a fixme in the API code anticipating we'd hit that problem eventually.
I fixed the fixme and now it's fixed :)

@awestuk
Copy link
Contributor

awestuk commented Jun 3, 2022

Tested this on production and it's working great! Non-admins can now create and view searches, which is exactly what we wanted.

Only minor issue is that non-admins with this and 'edit all contacts' don't see the 'Related Contacts' entity. I'm assuming that's because permissions on relationships are more complicated. I can see it devolves down to addSelectWhereClause, but haven't got any further than that yet.

@colemanw colemanw changed the title SearchKit - Add "administer SearchKit" permission SearchKit - Add "administer search_kit" permission Jun 3, 2022
@colemanw
Copy link
Member Author

colemanw commented Jun 3, 2022

@awestuk great. Here's a fix for the other thing you noticed. Can you please review/test #23684.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jun 3, 2022
colemanw added 2 commits June 3, 2022 22:22
This allows extensions to expand permissions via the civi.api.authorize event,
instead of assuming that all permissions are hard-coded in the core entity.
@colemanw colemanw force-pushed the searchKitAddPerm branch from f02286d to 1936f6c Compare June 4, 2022 02:22
@eileenmcnaughton eileenmcnaughton added merge on pass and removed merge ready PR will be merged after a few days if there are no objections labels Jun 4, 2022
@eileenmcnaughton
Copy link
Contributor

Issues have been addressed upping to MOP

@eileenmcnaughton eileenmcnaughton merged commit 99106f7 into civicrm:master Jun 4, 2022
@eileenmcnaughton eileenmcnaughton deleted the searchKitAddPerm branch June 4, 2022 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants