CRM-21707 use metadata to validate 'parents' field on group.parents

There was a test attempting to pass

[1 => 'Test Group'] which was getting through to the BAO but I feel should
have been blocked at the api level as Test Group did not exist. We have existing code to validate against pseudoconstants but this was not kicking in
because
a) there was no pseudoconstant defined for the field
b) we were giving TEXT fields less validation than STRING fields
(the assumption looks like it was that they were html fields but that is
not consistent & IMHO non of the additional checks are harmful.)
c) the getoptions was failing because it was passing 'validate' in
as a param to the pseudoconstant once set up, I handled that in
the pseudoconstant function.

I also altered the test to see [1 => 'non existent group'] as a fail
(rather than converting it to group = 1 & succeed.

There are a couple of follow on thoughts I have
1) I think we should xss check keys as well as values - will add that as a commit
2) there may be a very very hypothetical hole in _civicrm_api3_api_match_pseudoconstant_value - if  is empty it returns early. I think that used
to be valid as some psuedoconstants were not resolveable but now
I think if options is empty it means there are no valid options &
it should be rejected. Note that these strings are still xss checked.

diff --git a/CRM/Contact/DAO/Group.php b/CRM/Contact/DAO/Group.php
index e7625923b660d4a9c70612a2c3db043966c4fc65..c80191e1ad990746de388afe526471eef2a049d2 100644 (file)
--- a/CRM/Contact/DAO/Group.php
+++ b/CRM/Contact/DAO/Group.php
@@ -6,7 +6,7 @@
  *
  * Generated from xml/schema/CRM/Contact/Group.xml
  * DO NOT EDIT.  Generated by CRM_Core_CodeGen
- * (GenCodeChecksum:d0b8ad59d0e80070e103ca1db922cfd5)
+ * (GenCodeChecksum:80e014865346a828f58a093fcfa515dc)
  */
 
 /**
@@ -382,6 +382,9 @@ class CRM_Contact_DAO_Group extends CRM_Core_DAO {
           'bao' => 'CRM_Contact_BAO_Group',
           'localizable' => 0,
           'serialize' => self::SERIALIZE_COMMA,
+          'pseudoconstant' => [
+            'callback' => 'CRM_Core_PseudoConstant::allGroup',
+          ]
         ],
         'children' => [
           'name' => 'children',

diff --git a/CRM/Core/PseudoConstant.php b/CRM/Core/PseudoConstant.php
index 15227879bd2d43fca86c2d1dd5ae88e2c4679a62..f67c23a2b0ba918256d98b1e6e1ae4214bad8b36 100644 (file)
--- a/CRM/Core/PseudoConstant.php
+++ b/CRM/Core/PseudoConstant.php
@@ -931,6 +931,11 @@ WHERE  id = %1";
    *   array reference of all groups.
    */
   public static function allGroup($groupType = NULL, $excludeHidden = TRUE) {
+    if ($groupType === 'validate') {
+      // validate gets passed through from getoptions. Handle in the deprecated
+      // fn rather than change the new pattern.
+      $groupType = NULL;
+    }
     $condition = CRM_Contact_BAO_Group::groupTypeCondition($groupType, $excludeHidden);
     $groupKey = ($groupType ? $groupType : 'null') . !empty($excludeHidden);
 

diff --git a/api/v3/utils.php b/api/v3/utils.php
index 9dadc310df479d0f6c23e51c4b0c6a6f8fda8e3a..959c17e0c000c2caa184119ea5e1ec6342dee7e0 100644 (file)
--- a/api/v3/utils.php
+++ b/api/v3/utils.php
@@ -1530,9 +1530,6 @@ function _civicrm_api3_validate_switch_cases($fieldName, $fieldInfo, $entity, $p
       break;
 
     case CRM_Utils_Type::T_TEXT:
-      _civicrm_api3_validate_html($params, $fieldName, $fieldInfo);
-      break;
-
     case CRM_Utils_Type::T_STRING:
       _civicrm_api3_validate_string($params, $fieldName, $fieldInfo, $entity);
       break;
@@ -1602,10 +1599,6 @@ function _civicrm_api3_validate_fields($entity, $action, &$params, $fields) {
         break;
 
       case CRM_Utils_Type::T_TEXT:
-        //blob
-        _civicrm_api3_validate_html($params, $fieldName, $fieldInfo);
-        break;
-
       case CRM_Utils_Type::T_STRING:
         _civicrm_api3_validate_string($params, $fieldName, $fieldInfo, $entity);
         break;

diff --git a/tests/phpunit/api/v3/GroupTest.php b/tests/phpunit/api/v3/GroupTest.php
index 2ea630bbcbec85a9a8f5dafe3b3dd84446d36364..8db1ebc33c6ea32e6c518dc39baf4486961f7613 100644 (file)
--- a/tests/phpunit/api/v3/GroupTest.php
+++ b/tests/phpunit/api/v3/GroupTest.php
@@ -261,8 +261,7 @@ class api_v3_GroupTest extends CiviUnitTestCase {
     $params['parents']["(SELECT api_key FROM civicrm_contact where id = 1)"] = "Test";
     $this->callAPIFailure('group', 'create', $params);
     unset($params['parents']["(SELECT api_key FROM civicrm_contact where id = 1)"]);
-    $group2 = $this->callAPISuccess('group', 'create', $params);
-    $this->assertEquals(count($group2['values'][$group2['id']]['parents']), 1);
+    $this->callAPIFailure('group', 'create', $params, '\'test Group\' is not a valid option for field parents');
   }
 
   /**

diff --git a/xml/schema/Contact/Group.xml b/xml/schema/Contact/Group.xml
index 28c7d96e769ee3068bfb146288f9317ee347ca22..6831add3329b2528ff887fa3a106b58ca4a08f8c 100644 (file)
--- a/xml/schema/Contact/Group.xml
+++ b/xml/schema/Contact/Group.xml
@@ -150,6 +150,9 @@
     <comment>IDs of the parent(s)</comment>
     <add>2.1</add>
     <serialize>COMMA</serialize>
+    <pseudoconstant>
+      <callback>CRM_Core_PseudoConstant::group</callback>
+    </pseudoconstant>
   </field>
   <field>
     <name>children</name>