CRM-19494 Refactoring of permission code #9229

bjendres · 2016-10-12T21:02:35Z

Contains:

refactoring of CRM_Contact_BAO_Contact_Permission
tests
bugfixes for existing code
fix for CRM-12645

With the fix for CRM-12645 this should replace #9193.

seamuslee001 · 2016-10-12T21:06:27Z

@systopia can you please fix the style errors found here https://test.civicrm.org/job/CiviCRM-Core-PR/12167/checkstyleResult/new/

bjendres · 2016-10-12T21:56:44Z

@systopia can you please fix the style errors found here https://test.civicrm.org/job/CiviCRM-Core-PR/12167/checkstyleResult/new/

@seamuslee001 done. thanks.

eileenmcnaughton

I've looked at this code as far as Relationship - am going to try to mainline coffee & face that function

eileenmcnaughton · 2016-10-13T09:02:57Z

CRM/Contact/BAO/Contact/Permission.php

+      // empty contact lists would cause trouble in the SQL. And be pointless.
+      return $result_set;
+    }
+    $contact_id_list = implode(',', $contact_ids);


trivial point - but this might be better after the 'early return' for 'edit all contacts' etc

eileenmcnaughton · 2016-10-13T09:04:06Z

CRM/Contact/BAO/Contact/Permission.php

+
+    // get logged in user
+    $session   = CRM_Core_Session::singleton();
+    $contactID = (int) $session->get('userID');


Preferred function here is $contactID = CRM_Core_Session::getLoggedInUser()

it saves a whole line!

will do (found out it's ::getLoggedInContactID(), just in case anybody wonders)

eileenmcnaughton · 2016-10-13T09:07:22Z

CRM/Contact/BAO/Contact/Permission.php

-    $tables = array();
-    $whereTables = array();
+    // get logged in user
+    $session   = CRM_Core_Session::singleton();


ditto about the preferred function - saves 3 lines here as the function name should be clear enough to justify dropping the comment line :-)

eileenmcnaughton · 2016-10-13T09:08:28Z

CRM/Contact/BAO/Contact/Permission.php

+    $contactID = (int) $session->get('userID');
+
+    // first: check if contact is trying to view own contact
+    if ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact')


DIdn't we decide this should be

$type == CRM_Core_Permission::VIEW && (CRM_Core_Permission::check('view my contact') || CRM_Core_Permission::check('edit my contact'))

I decided to not do that for 'view/edit my contact', there might be scenarios where people should be able to e.g. set a flag to their contact without seeing all data....

eileenmcnaughton · 2016-10-13T09:09:02Z

CRM/Contact/BAO/Contact/Permission.php

@@ -60,13 +158,16 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) {
      return TRUE;
    }

-    //check permission based on relationship, CRM-2963
+    // check permission based on relationship, CRM-2963


While we are being fussy it's supposed to be a capital letter & a full stop at the end....

Oops, I think I don't comply with that in any comment. Ignore it?

well I can't standover you & deny you coffee now so we will have to :-)

eileenmcnaughton · 2016-10-13T09:12:24Z

CRM/Contact/BAO/Contact/Permission.php

@@ -99,7 +202,8 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force
    }

    if (!$force) {
-      if (!empty($_processed[$userID])) {
+      // skip if already calculated
+      if (!empty($_processed[$type][$userID])) {


It might be better to do a select from the table to check there are entries for the user id - I'm worried the cache could be truncated from out of under someone

Good point, but that might have a big impact on performance. Maybe we should investigate how often it's called...

won't change now, since this is also the existing approach. But I added a FIXME comment

eileenmcnaughton · 2016-10-13T09:13:56Z

CRM/Contact/BAO/Contact/Permission.php

@@ -125,6 +229,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force

    $from = CRM_Contact_BAO_Query::fromClause($whereTables);

+    // FIXME: don't use 'ON DUPLICATE KEY UPDATE'


I think I should push a fix for this up & get it in the same release

eileenmcnaughton · 2016-10-13T09:14:36Z

CRM/Contact/BAO/Contact/Permission.php

+    // get the currently logged in user
+    $config    = CRM_Core_Config::singleton();
+    $session   = CRM_Core_Session::singleton();
+    $contactID = (int) $session->get('userID');


ditto about using getLoggedInUser

eileenmcnaughton · 2016-10-13T09:16:07Z

CRM/Contact/BAO/Contact/Permission.php

+    }
+
+    // get the currently logged in user
+    $config    = CRM_Core_Config::singleton();


If you are only using this for one purpose you can also do
$useSecondDegreeUserPermissions = CRM_Core_Config::singleton()->secondDegRelPermissions

eileenmcnaughton

OK - I just have the test class still to read

eileenmcnaughton · 2016-10-13T09:24:08Z

CRM/Contact/BAO/Contact/Permission.php

+            }
+
+            $queries[] = "
+  SELECT DISTINCT(second_degree_relationship.contact_id_{$second_direction['to']}) AS contact_id


I might be wrong but I don't think you need distinct here because the UNION will distinct it all

removed SELECT DISTINCT and used UNION DISTINCT, although that was the default for UNION anyway

eileenmcnaughton · 2016-10-13T09:27:22Z

CRM/Contact/BAO/Contact/Permission.php

+    while ($result->fetch()) {
+      $result_set[(int) $result->contact_id] = TRUE;
+    }
+    $keys = array_keys($result_set);


This line seems redundant

eileenmcnaughton · 2016-10-13T09:31:17Z

CRM/Contact/BAO/Contact/Permission.php

+    $query = "(" . implode(")\nUNION (", $queries) . ")";
+    $result = CRM_Core_DAO::executeQuery($query);
+    while ($result->fetch()) {
+      $result_set[(int) $result->contact_id] = TRUE;


I'd be tempted to skip this loop & do

$query = "SELECT GROUP_CONCAT contact_id FROM ( $query ) as q";
return explode(',', CRM_Core_DAO::singleValueQuery($query));

My guess is more work on mysql & less on php is better

might be dangerous for large sets of contacts...

eileenmcnaughton · 2016-10-13T09:31:35Z

CRM/Contact/BAO/Contact/Permission.php

+    $keys = array_keys($result_set);
+    return array_keys($result_set);
+  }
+


Why didn't jenkins bite you on the double line

eileenmcnaughton · 2016-10-13T09:32:11Z

CRM/Contact/BAO/Contact/Permission.php

@@ -33,6 +33,96 @@
 class CRM_Contact_BAO_Contact_Permission {

  /**
+   * Check which of the given contact IDs the logged in user
+   *   has permissions for the operation type according


According to what ... the suspense

eileenmcnaughton · 2016-10-13T09:52:58Z

tests/phpunit/CRM/ACL/ListTest.php

+    // create test contacts
+    $contacts = $this->createScenarioPlain();
+
+    // test WITH all permissions


Might need to state that NULL has that effect

eileenmcnaughton · 2016-10-13T09:54:42Z

tests/phpunit/CRM/ACL/ListTest.php

+    $this->assertEquals($result, $contacts, "Contacts should be viewable when 'view all contacts'");
+
+    // test WITH explicit permission
+    CRM_Core_Config::singleton()->userPermissionClass->permissions = array('view all contacts');


Add a check for edit all contacts can view all contacts

- style - extra test - documentation - FIXME remarks

eileenmcnaughton · 2016-10-14T09:23:08Z

@systopia We should close this or #9246 -I've given you permission to push to my github repo if you want to use #9246

I believe that one is working now - but it would be good if you would recheck. I think I'd like to merge it shortly AFTER 4.7.13 is forked

monishdeb · 2016-10-14T13:03:28Z

Closing this in favor of #9246

bjendres added 12 commits October 12, 2016 00:51

added new list permission functions

bc8a354

added check for 'view/edit my contact'

65540c4

cleanup and documentation

f468c0d

started unit tests for new list permission functions

8b37b5a

adding new list permission functions (wip)

33d78a9

unit tests for new list permission functions (wip)

fff98d6

unit tests for new list permission functions (wip)

5e93c9a

fixed: EDIT implies VIEW

8c20cee

fixed bug in the original function

5455514

fixed bug in the original function

ddeebf3

finished unit tests

5eb4fc6

using new Permission::allowList to fix CRM-12645

02cd0c2

civicrm-builder added the master label Oct 12, 2016

eileenmcnaughton mentioned this pull request Oct 12, 2016

CRM-12645 Suppress edit link if user does not have edit permissions. #9193

Closed

bjendres added 2 commits October 12, 2016 22:51

obeying master jenkins

02eb154

obeying master jenkins

21fd51a

eileenmcnaughton reviewed Oct 13, 2016

View reviewed changes

implementing @Eileen's suggestions:

714bed1

- style - extra test - documentation - FIXME remarks

bjendres mentioned this pull request Oct 13, 2016

CRM-19494 Refactoring of permission code #9246

Merged

monishdeb closed this Oct 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-19494 Refactoring of permission code #9229

CRM-19494 Refactoring of permission code #9229

bjendres commented Oct 12, 2016 •

edited by civicrm-builder

Loading

seamuslee001 commented Oct 12, 2016

bjendres commented Oct 12, 2016

eileenmcnaughton left a comment •

edited

Loading

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

bjendres Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

bjendres Oct 13, 2016

eileenmcnaughton Oct 13, 2016

bjendres Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

bjendres Oct 13, 2016

bjendres Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton left a comment

eileenmcnaughton Oct 13, 2016

bjendres Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

bjendres Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

eileenmcnaughton Oct 13, 2016

bjendres Oct 13, 2016

eileenmcnaughton commented Oct 14, 2016

monishdeb commented Oct 14, 2016

		@@ -125,6 +229,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force

		$from = CRM_Contact_BAO_Query::fromClause($whereTables);

		// FIXME: don't use 'ON DUPLICATE KEY UPDATE'

CRM-19494 Refactoring of permission code #9229

CRM-19494 Refactoring of permission code #9229

Conversation

bjendres commented Oct 12, 2016 • edited by civicrm-builder Loading

seamuslee001 commented Oct 12, 2016

bjendres commented Oct 12, 2016

eileenmcnaughton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Oct 14, 2016

monishdeb commented Oct 14, 2016

bjendres commented Oct 12, 2016 •

edited by civicrm-builder

Loading

eileenmcnaughton left a comment •

edited

Loading