From 1678a63b1b854729d440de47b7305b5f50c66ce8 Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 26 Jan 2018 11:59:23 +1300 Subject: [PATCH] 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. --- CRM/Contact/DAO/Group.php | 5 ++++- CRM/Core/PseudoConstant.php | 5 +++++ api/v3/utils.php | 7 ------- tests/phpunit/api/v3/GroupTest.php | 3 +-- xml/schema/Contact/Group.xml | 3 +++ 5 files changed, 13 insertions(+), 10 deletions(-) diff --git a/CRM/Contact/DAO/Group.php b/CRM/Contact/DAO/Group.php index e7625923b6..c80191e1ad 100644 --- 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 15227879bd..f67c23a2b0 100644 --- 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 9dadc310df..959c17e0c0 100644 --- 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 2ea630bbcb..8db1ebc33c 100644 --- 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 28c7d96e76..6831add332 100644 --- a/xml/schema/Contact/Group.xml +++ b/xml/schema/Contact/Group.xml @@ -150,6 +150,9 @@ IDs of the parent(s) 2.1 COMMA + + CRM_Core_PseudoConstant::group + children -- 2.25.1