diff --git a/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php b/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php index ac840ea7d..797133c15 100644 --- a/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php +++ b/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php @@ -405,7 +405,7 @@ class AliasController extends ApiMutableModelControllerBase foreach ($data['aliases']['alias'] as $uuid => $content) { $type = !empty($content['type']) ? $content['type'] : ""; if (is_array($content) && !empty($content['name']) && $type != 'internal') { - $node = $this->getModel()->getByName($content['name']); + $node = $this->getModel()->getByName($content['name'], true); if ($node == null) { $node = $this->getModel()->aliases->alias->Add(); $result['new'] += 1; diff --git a/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php b/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php index 2908c1d6a..26daf64d2 100644 --- a/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php +++ b/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php @@ -91,6 +91,11 @@ abstract class BaseModel */ private $internalMissingUuids = false; + /** + * @var int internal validation sequence (number of times validation has run) + */ + private int $internalValidationSequence = 0; + /** * If the model needs a custom initializer, override this init() method * Default behaviour is to do nothing in this init. @@ -110,7 +115,7 @@ abstract class BaseModel if ($xmlNode->count() == 0) { $result = (string)$xmlNode; } else { - $result = array(); + $result = []; foreach ($xmlNode->children() as $childNode) { // item keys can be overwritten using value attributes if (!isset($childNode->attributes()['value'])) { @@ -467,6 +472,16 @@ abstract class BaseModel return str_ends_with($this->internal_mountpoint, '+') && strpos($this->internal_mountpoint, "//") !== 0; } + /** + * Return the number of times performValidation() has been called. + * This can be practical if validations need to cache outcomes which are consistent for the full validation + * sequence. + * @return int + */ + public function getValidationSequence() + { + return $this->internalValidationSequence; + } /** * validate full model using all fields and data in a single (1 deep) array @@ -480,6 +495,8 @@ abstract class BaseModel $validation_data = array(); $all_nodes = $this->internalData->getFlatNodes(); + $this->internalValidationSequence++; + foreach ($all_nodes as $key => $node) { if ($validateFullModel || $node->isFieldChanged()) { $node_validators = $node->getValidators(); diff --git a/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php b/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php index 7047d09a4..7da128561 100644 --- a/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php +++ b/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php @@ -36,6 +36,9 @@ namespace OPNsense\Base\Constraints; */ class UniqueConstraint extends BaseConstraint { + private static $itemmap = []; + private static $validationSequence = 0; + /** * Executes validation * @@ -51,38 +54,53 @@ class UniqueConstraint extends BaseConstraint if (!$node->isRequired() && empty((string)$node)) { return true; } - $containerNode = $node; - $nodeName = $node->getInternalXMLTagName(); - $parentNode = $node->getParentNode(); - $level = 0; - // dive into parent - while ($containerNode != null && !$containerNode->isArrayType()) { - $containerNode = $containerNode->getParentNode(); - $level++; + $mdl = $node->getParentModel(); + if ($mdl === null || $mdl->getValidationSequence() != static::$validationSequence) { + // reset cache, new validation round. + static::$validationSequence = $mdl !== null ? $mdl->getValidationSequence() : 0; + static::$itemmap = []; } - if ($containerNode != null && $level == 2) { - // collect (additional) key fields - $keyFields = array($nodeName); - $keyFields = array_unique(array_merge($keyFields, $this->getOptionValueList('addFields'))); - // calculate the key for this node - $nodeKey = ''; - foreach ($keyFields as $field) { - $nodeKey .= $fieldSeparator . $parentNode->$field; + $keyFields = array_unique( + array_merge( + [$node->getInternalXMLTagName()], + $this->getOptionValueList('addFields') + ) + ); + asort($keyFields); + $nodeKey = implode('|', $keyFields); + $parentNode = $node->getParentNode(); + // calculate the key for this node + if (!isset(static::$itemmap[$nodeKey])) { + static::$itemmap[$nodeKey] = []; + $level = 0; + // dive into parent + $containerNode = $node; + while ($containerNode != null && !$containerNode->isArrayType()) { + $containerNode = $containerNode->getParentNode(); + $level++; } - // when an ArrayField is found in range, traverse nodes and compare keys - foreach ($containerNode->iterateItems() as $item) { - if ($item !== $parentNode) { - $itemKey = ''; + if ($containerNode != null && $level == 2) { + // when an ArrayField is found in range, traverse nodes and compare keys + foreach ($containerNode->iterateItems() as $item) { + $itemValue = ''; foreach ($keyFields as $field) { - $itemKey .= $fieldSeparator . $item->$field; + $itemValue .= $fieldSeparator . $item->$field; } - if ($itemKey == $nodeKey) { - $this->appendMessage($validator, $attribute); - return false; + if (empty(static::$itemmap[$nodeKey][$itemValue])) { + static::$itemmap[$nodeKey][$itemValue] = 0; } + static::$itemmap[$nodeKey][$itemValue]++ ; } } } + $nodeValue = ''; + foreach ($keyFields as $field) { + $nodeValue .= $fieldSeparator . $parentNode->$field; + } + if ((static::$itemmap[$nodeKey][$nodeValue] ?? 0) > 1) { + $this->appendMessage($validator, $attribute); + return false; + } } return true; } diff --git a/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php b/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php index f2e78cae3..bc164d3b9 100644 --- a/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php +++ b/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php @@ -217,28 +217,21 @@ class Alias extends BaseModel public function getByName($name, $flush = false) { if ($flush) { - // Flush false negative results (searched earlier, didn't exist at the time) as positive matches will - // always be resolved. + // Flush cache in case model data has been changed. $this->aliasReferenceCache = []; } - // cache alias uuid references, but always validate existence before return. + if (empty($this->aliasReferenceCache)) { + // cache alias uuid references, but always validate existence before return. + foreach ($this->aliases->alias->iterateItems() as $uuid => $alias) { + if ((string)$alias->name == $name) { + $this->aliasReferenceCache[(string)$alias->name] = $alias; + } + } + } if (isset($this->aliasReferenceCache[$name])) { - $uuid = $this->aliasReferenceCache[$name]; - if ($uuid === false) { - return null; - } - $alias = $this->getNodeByReference('aliases.alias.' . $uuid); - if ($alias != null && (string)$alias->name == $name) { - return $alias; - } + return $this->aliasReferenceCache[$name]; + } else { + return null; } - foreach ($this->aliases->alias->iterateItems() as $uuid => $alias) { - if ((string)$alias->name == $name) { - $this->aliasReferenceCache[$name] = $uuid; - return $alias; - } - } - $this->aliasReferenceCache[$name] = false; - return null; } } diff --git a/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php b/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php index 9a55a7ffa..daac90ba9 100644 --- a/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php +++ b/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php @@ -34,9 +34,8 @@ use OPNsense\Base\FieldTypes\TextField; class UniqueTestContainer extends ArrayField { - private $uniqueConstraints = []; - private $valuesRequired = false; - private $internalNodes = []; + private bool $valuesRequired = false; + private array $internalNodes = []; /** * @param $nodes a single node or an array of nodes @@ -48,47 +47,48 @@ class UniqueTestContainer extends ArrayField { // UniqueConstraint requires a depth of 2, so add a container node $container = new ContainerField(); - $constraint = new UniqueConstraint(); - $this->addChildNode('UniqueTest', $container); - $addFields = []; - $fields_added = false; + $idx = count($this->internalNodes); + $this->addChildNode('item'. $idx, $container); foreach ($nodes as $name => $value) { $node = new TextField(null, $name); $node->setRequired($this->valuesRequired ? "Y" : "N"); - if ($name === array_key_first($nodes)) { - $constraint->setOption('node', $node); - $constraint->setOption('name', $name); - $constraint->setOption('ValidationMessage', 'Validation Failed'); - } else { - $addFields[] = $name; - $fields_added = true; - } - $node->setValue($value); + $node->setValue((string)$value); $container->addChildNode($name, $node); - $this->internalNodes[] = $node; } - - if ($fields_added) { - $constraint->setOption('addFields', $addFields); - } - - $this->uniqueConstraints[] = $constraint; + $this->internalNodes[] = $nodes; } public function setRequired($required) { $this->valuesRequired = $required; - foreach ($this->internalNodes as $node) { - /* cover earlier set nodes */ - $node->setRequired($this->valuesRequired ? "Y" : "N"); + foreach ($this->iterateItems() as $records) { + foreach ($records->iterateItems() as $node) { + /* cover earlier set nodes */ + $node->setRequired($this->valuesRequired ? "Y" : "N"); + } } } public function validate() { + $uniqueConstraints = []; $validator = new \OPNsense\Base\Validation(); - foreach ($this->uniqueConstraints as $idx => $constraint) { + foreach ($this->internalNodes as $idx => $nodes) { + $addFields = []; + $constraint = new UniqueConstraint(); + foreach ($nodes as $name => $value) { + if ($name === array_key_first($nodes)) { + $constraint->setOption('node', $this->{'item'. $idx}->$name); + $constraint->setOption('name', $name); + $constraint->setOption('ValidationMessage', 'Validation Failed'); + } else { + $addFields[] = $name; + } + } + if ($addFields) { + $constraint->setOption('addFields', $addFields); + } $validator->add($idx, $constraint); } $msgs = $validator->validate([]); @@ -111,7 +111,8 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $container->addNode(['unique_test' => 'value1']); $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + + $this->assertEquals(2, count($msgs)); } public function testMultipleNonEqualAndEqualValues() @@ -126,7 +127,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $container->addNode(['unique_test' => 'value1', 'unique_test2' => 'value2']); $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } public function testEmptyValuesNotRequiredAndRequired() @@ -143,7 +144,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } public function testMultipleEmptyValuesNotRequiredAndRequired() @@ -151,7 +152,6 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $container = new UniqueTestContainer(); $container->addNode(['unique_test' => '', 'unique_test2' => '']); $container->addNode(['unique_test' => '', 'unique_test2' => '']); - $msgs = $container->validate(); $this->assertEquals(0, count($msgs)); @@ -160,7 +160,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } public function testFirstValueEmptyPassAll() @@ -177,6 +177,6 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } }