Skip to content

Commit

Permalink
MVC / ApiMutableModelControllerBase - scope xxxBase validations to th…
Browse files Browse the repository at this point in the history 
…e item in question, which should prevent automatically applied defaults triggering validation issues which can't be fixed from the caller in question. To prevent setAction() not triggering on consecutive calls we should validate all items when executed.

Ideally we should only force validation on the requested node and its children, but since we currently don't have a method for this and the performance decrease is likely low, we just request full validation on performValidation().

closes #6978
  • Loading branch information
@AdSchellevis
AdSchellevis committed Nov 20, 2023
1 parent 30146f1 commit e36123c
Showing 1 changed file with 27 additions and 20 deletions.
47 changes: 27 additions & 20 deletions src/opnsense/mvc/app/controllers/OPNsense/Base/ApiMutableModelControllerBase.php
View file
Expand Up @@ -100,7 +100,7 @@ private function checkAndThrowSafeDelete($uuid)
{
if (static::$internalModelUseSafeDelete) {
$configObj = Config::getInstance()->object();
$usages = array();
$usages = [];
// find uuid's in our config.xml
foreach ($configObj->xpath("//text()[contains(.,'{$uuid}')]") as $node) {
$referring_node = $node->xpath("..")[0];
Expand Down Expand Up @@ -158,7 +158,7 @@ private function checkAndThrowSafeDelete($uuid)
public function getAction()
{
// define list of configurable settings
$result = array();
$result = [];
if ($this->request->isGet()) {
$result[static::$internalModelName] = $this->getModelNodes();
}
Expand Down Expand Up @@ -217,28 +217,33 @@ protected function validateAndSave($node = null, $prefix = null)

/**
* Validate this model
* @param $node reference node, to use as relative offset
* @param $node reference node, to use as relative offset, ignore validation issues outside this scope
* @param $prefix prefix to use when $node is provided (defaults to static::$internalModelName)
* @param bool $validateFullModel by default we only validate the fields we have changed
* @return array result / validation output
* @throws \ReflectionException when binding to the model class fails
*/
protected function validate($node = null, $prefix = null)
protected function validate($node = null, $prefix = null, $validateFullModel = false)
{
$result = array("result" => "");
$result = ["result" => ""];
$resultPrefix = empty($prefix) ? static::$internalModelName : $prefix;
// perform validation
$valMsgs = $this->getModel()->performValidation();
$valMsgs = $this->getModel()->performValidation($validateFullModel);
foreach ($valMsgs as $field => $msg) {
if (!array_key_exists("validations", $result)) {
$result["validations"] = array();
$result["result"] = "failed";
}
// replace absolute path to attribute for relative one at uuid.
if ($node != null) {
/* replace absolute path with attribute for relative one at uuid,
ignore validation issues when triggered outside the node scope. */
if (strpos($msg->getField(), $node->__reference) === false) {
continue;
}
$fieldnm = str_replace($node->__reference, $resultPrefix, $msg->getField());
} else {
$fieldnm = $resultPrefix . "." . $msg->getField();
}
if (!array_key_exists("validations", $result)) {
$result["validations"] = [];
$result["result"] = "failed";
}
$msgText = $msg->getMessage();
if (empty($result["validations"][$fieldnm])) {
$result["validations"][$fieldnm] = $msgText;
Expand All @@ -257,15 +262,17 @@ protected function validate($node = null, $prefix = null)

/**
* Save model after update or insertion, validate() first to avoid raising exceptions
* @param bool $validateFullModel by default we only validate the fields we have changed
* @param bool $disable_validation skip validation, be careful to use this!
* @return array result / validation output
* @throws \Phalcon\Filter\Validation\Exception on validation issues
* @throws \ReflectionException when binding to the model class fails
* @throws \OPNsense\Base\UserException when denied write access
*/
protected function save()
protected function save($validateFullModel = false, $disable_validation = false)
{
if (!(new ACL())->hasPrivilege($this->getUserName(), 'user-config-readonly')) {
if ($this->getModel()->serializeToConfig()) {
if ($this->getModel()->serializeToConfig($validateFullModel, $disable_validation)) {
Config::getInstance()->save();
}
return array("result" => "saved");
Expand Down Expand Up @@ -308,7 +315,7 @@ public function setAction()
if (!empty($hookErrorMessage)) {
$result['error'] = $hookErrorMessage;
} else {
return $this->save();
return $this->save(false, true);
}
}
}
Expand Down Expand Up @@ -371,7 +378,7 @@ public function getBase($key_name, $path, $uuid = null)
$node = $mdl->Add();
return array($key_name => $node->getNodes());
}
return array();
return [];
}

/**
Expand Down Expand Up @@ -402,7 +409,7 @@ public function addBase($post_field, $path, $overlay = null)

if (empty($result['validations'])) {
// save config if validated correctly
$this->save();
$this->save(false, true);
$result = array(
"result" => "saved",
"uuid" => $node->getAttribute('uuid')
Expand Down Expand Up @@ -437,7 +444,7 @@ public function delBase($path, $uuid)
$tmp = $tmp->{$step};
}
if ($tmp->del($uuid)) {
$this->save();
$this->save(false, true);
$result['result'] = 'deleted';
} else {
$result['result'] = 'not found';
Expand Down Expand Up @@ -482,10 +489,10 @@ public function setBase($post_field, $path, $uuid, $overlay = null)
if (is_array($overlay)) {
$node->setNodes($overlay);
}
$result = $this->validate($node, $post_field);
$result = $this->validate($node, $post_field, true);
if (empty($result['validations'])) {
// save config if validated correctly
$this->save();
$this->save(false, true);
$result = ["result" => "saved"];
} else {
$result["result"] = "failed";
Expand Down Expand Up @@ -531,7 +538,7 @@ public function toggleBase($path, $uuid, $enabled = null)
}
// if item has toggled, serialize to config and save
if ($result['changed']) {
$this->save();
$this->save(false, true);
}
}
}
Expand Down

0 comments on commit e36123c

Please sign in to comment.