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

Fix search display access for non-admin users #21082

Merged
merged 1 commit into from
Aug 17, 2021

Conversation

colemanw
Copy link
Member

Overview

Non-admin users should be allowed to view any search display that doesn't have permission checks disabled;
for those displays that disable permission checks, non-admins will only be able to view it if embedded in an afform.

Fixes dev/core#2737

Before

Permission checks too strict

After

Permission checks just right

@civibot
Copy link

civibot bot commented Aug 10, 2021

(Standard links)

@civibot civibot bot added the master label Aug 10, 2021
@eileenmcnaughton
Copy link
Contributor

@colemanw fail relates

@colemanw colemanw force-pushed the fixSearchKitPermissions branch from f4a10fa to 6aad1fc Compare August 11, 2021 01:08
@colemanw
Copy link
Member Author

Fixed it @eileenmcnaughton

@eileenmcnaughton
Copy link
Contributor

@colemanw this looks right now - so my take on this is that this is what you thought you had done in the first place? (hence it was hard to convince you it needed doing)

@colemanw
Copy link
Member Author

Yea, the missing 'get' permission was an oversight.

@eileenmcnaughton
Copy link
Contributor

@colemanw so to get it to render I also needed

+++ b/Civi/Api4/SavedSearch.php
@@ -22,4 +22,10 @@ namespace Civi\Api4;
  */
 class SavedSearch extends Generic\DAOEntity {
 
+
+  public static function permissions() {
+    $permissions = parent::permissions();
+    $permissions['get'] = ['access CiviCRM'];
+    return $permissions;
+  }

In addition I hit problems that were entity related - ie my contact had permission to view contributions but not the ability to view financial records related to them - I can put that in gitlab

@eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton
Copy link
Contributor

I would also note I think we could have cleaner error handling - ie the difference between 'there are no results' and 'you don't have permission' would ideally be clearer - in this case there are no results because the permissions on the payments are 'too' tight - per the above gitlab

image

Non-admin users should be allowed to view any search display that doesn't have permission checks disabled;
for those displays that disable permission checks, non-admins will only be able to view it if embedded in an afform.

Fixes dev/core#2737
@colemanw colemanw force-pushed the fixSearchKitPermissions branch from 6aad1fc to 144025a Compare August 11, 2021 04:43
@colemanw
Copy link
Member Author

@eileenmcnaughton I've made that addition to the SavedSearch permissions.

@eileenmcnaughton
Copy link
Contributor

@colemanw so - is there any case where we would not want people with access civicrm to find saved searches? I feel like it could expose some data about what you do business process wise - but I'm not sure.

OTOH it does seem like that would need to be restricted in some way that is less than blanket

@seamuslee001 any thoughts?

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Aug 13, 2021
@eileenmcnaughton
Copy link
Contributor

I'm OK with this but going with merge-ready due to the comments about about extending saved search get permissions

@colemanw
Copy link
Member Author

colemanw commented Aug 17, 2021

I don't think it's a problem @eileenmcnaughton. The SavedSearch entity contains metadata and search criteria but no contact data.

@eileenmcnaughton
Copy link
Contributor

Ok - well 5 days merge ready is enough

@eileenmcnaughton eileenmcnaughton merged commit 88056dc into civicrm:master Aug 17, 2021
@eileenmcnaughton eileenmcnaughton deleted the fixSearchKitPermissions branch August 17, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants