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 - Fix viewing search display for anonymous user #24160

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Aug 5, 2022

Overview

Fixes anonymous access to SearchKit displays.

Before

Anonymous user cannot view SearchKit display, even if it is set to bypass permission checks.

After

It works again.

Technical Details

This regressed in aa5d068 when the single SearchDisplay::run api call was replaced with an array of calls. The gatekeeper CRM_Api4_Permission was only able to handle single calls.
This updates it so that it can handle an array of calls - it will call hook_civicrm_alterApiRoutePermissions for each one and reject the request if any of them lack permission.

This regressed in aa5d068 when the single SearchDisplay::run api call
was replacd with an array of calls. The hack in alterApiRoutePermissions can't handle multiple calls.
This updates it so that it can handle an array of calls - it will call the hook for each one and
reject the request if any of them lack permission.
@civibot
Copy link

civibot bot commented Aug 5, 2022

(Standard links)

@totten
Copy link
Member

totten commented Aug 8, 2022

I did some r-run on on 5.52+master and confirmed it fixed the problem.

To my reading, this improves support for alterApiRoutePermissions without altering the general security posture.

The conditional was a little mysterious at first - but after comparing the related code in CRM_Api4_Page_AJAX::run(), it seems to match-up. (Added notes to that effect.)

Merge on pass.

@eileenmcnaughton
Copy link
Contributor

We haven't branched yet so I'm just wondering whether this is already merged to master (which is kinda the rc) - since we won't up-merge this branch

@colemanw
Copy link
Member Author

colemanw commented Aug 9, 2022

@eileenmcnaughton the master branch version of this PR is at #24180

@seamuslee001 seamuslee001 merged commit 368acb0 into civicrm:5.52 Aug 9, 2022
@seamuslee001 seamuslee001 deleted the fixSearchKitEntry branch August 9, 2022 00:03
@totten
Copy link
Member

totten commented Aug 9, 2022

Yeah, to minimize shuffling, we'll branch for 5.53-rc after the current batch of regression-fixes (circa 5.52.1) are merged into master+5.52.

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