Save permission needed on parents for resource tree drag operations

[juro]

juro created Redmine issue ID 4918

"save" permission is needed on parent, grandparent etc. resources to re-arrange child resources using drag-n-drop in resource tree. Even for changing resource order within the same parent. Otherwise user gets Access denied error. Changing menu index or parent using fields in resource editing page works fine.
While drag-n-drop is the fastest way for moving resources and obvious to users, this is quite important. To maintain site structure and consistency ordinary content editors do not have "save" permission on root resources.
Not sure when this issue started, works fine on 2.0.4, doesn't work on 2.1.0rc1 and later.

[juro]

juro submitted:

After investigating more on this, we have found that Save permission is needed on any resource visible in the resource tree at the moment of drag operation, even whet that resource is in completely different branch from dragged resource.
That means there must not be any read-only resource (load, list and view policy) anywhere on the site to be sure that drag operations work.

[opengeek]

opengeek submitted:

Fixed for 2.1.2 — see commit details at 2a8f673

[juro]

juro submitted:

Don't know if this bug was reintroduced in 2.1.3 or was never really resolved, but it doesn't work. Array modifiedNodes in the commit above contains all visible nodes, not only modified ones. So the final behaviour is the same as was originally reported. Please help, we have just upgraded large web to 2.1.3 and this is really annoying.

[juro]

juro submitted:

After more investigation on this we have found that #2737 is involved in this as well. Unfortunately this commit still doesn't fix problems dragging resources in the tree.
/core/model/modx/processors/resource/sort.php performs access rights check on every visible node and also saves every visible node (except children of dragged node), regardles whether it has changed or not. This leads to incorrect access denied errors and unnecessary database updates (calling save() on every node).
Node should be added to modifiedNodes array only if either parent, context or order has changed.

Please reopen and correct this issue in 2.1.4.

[juro]

juro submitted:

As this bug was ignored and doesn't seem to get corrected any more, this is our own fix to file core/model/modx/processors/resource/sort.php solving the issue. Help yourself. Changed lines are marked as /* mxedit */

hasPermission('save_document')) return $modx->error->failure($modx->lexicon('access_denied'));
$modx->lexicon->load('resource', 'context');
$data = urldecode($scriptProperties['data']);
$data = $modx->fromJSON($data);
$nodes = array();
getNodesFormatted($nodes,$data);
$modx->invokeEvent('OnResourceBeforeSort',array(
    'nodes' => &$nodes,
));
/* readjust cache */
$nodeErrors = array();
$modifiedNodes = array();
$contextsAffected = array();
$dontChangeParents = array();
foreach ($nodes as $ar_node) {
      $hasChanged=false; /* mxedit */
    if (!is_array($ar_node) || empty($ar_node['id'])) continue;
    $node = $modx->getObject('modResource',$ar_node['id']);
    if (empty($node)) continue;
    if (empty($ar_node['context'])) continue;
    if (in_array($ar_node['parent'],$dontChangeParents)) continue;
    $old_parent_id = $node->get('parent');
    if ($old_parent_id != $ar_node['parent']) {
        /* get new parent, if invalid, skip, unless is root */
        if ($ar_node['parent'] != 0) {
            $parent = $modx->getObject('modResource',$ar_node['parent']);
            if ($parent == null) {
                $nodeErrors[] = $modx->lexicon('resource_err_new_parent_nf', array('id' => $ar_node['parent']));
                continue;
            }
            if (!$parent->checkPolicy('add_children')) {
                $nodeErrors[] = $modx->lexicon('resource_add_children_access_denied');
                continue;
            }
        } else {
            $context = $modx->getObject('modContext',$ar_node['context']);
            if (empty($context)) {
                $nodeErrors[] = $modx->lexicon('context_err_nfs', array('key' => $ar_node['context']));
                continue;
            }
            if (!$modx->hasPermission('new_document_in_root')) {
                $nodeErrors[] = $modx->lexicon('resource_add_children_access_denied');
                continue;
            }
        }
        /* save new parent */
        $hasChanged=true; /* mxedit */
        $node->set('parent',$ar_node['parent']);
    }
    $old_context_key = $node->get('context_key');
    $contextsAffected[$old_context_key] = true;
    if ($old_context_key != $ar_node['context'] && !empty($ar_node['context'])) {
        $hasChanged=true; /* mxedit */
        $node->set('context_key',$ar_node['context']);
        $contextsAffected[$ar_node['context']] = true;
        $dontChangeParents[] = $node->get('id'); /* prevent children from reverting back */
    }
    if ($node->get('menuindex')!=$ar_node['order']) { /* mxedit */
      $node->set('menuindex',$ar_node['order']);
      $hasChanged=true; /* mxedit */
    } /* mxedit */
    if ($hasChanged) { /* mxedit */
      $modifiedNodes[] = $node;
    } /* mxedit */
}
if (!empty($modifiedNodes)) {
    foreach ($modifiedNodes as $modifiedNode) {
        if (!$modifiedNode->checkPolicy('save')) {
            $nodeErrors[] = $modx->lexicon('resource_err_save');
        }
    }
}
if (!empty($nodeErrors)) {
    return $modx->error->failure(implode("\n", array_unique($nodeErrors)));
}
if (!empty($modifiedNodes)) {
    foreach ($modifiedNodes as $modifiedNode) {
        $modifiedNode->save();
    }
}
$modx->invokeEvent('OnResourceSort', array(
    'nodes' => &$nodes,
    'modifiedNodes' => &$modifiedNodes
));
/* empty cache */
$modx->cacheManager->refresh(array(
    'db' => array(),
    'auto_publish' => array('contexts' => $contextsAffected),
    'context_settings' => array('contexts' => $contextsAffected),
    'resource' => array('contexts' => $contextsAffected),
));
return $modx->error->success();
function getNodesFormatted(&$ar_nodes,$cur_level,$parent = 0) {
    $order = 0;
    foreach ($cur_level as $id => $children) {
        $ar = explode('_',$id);
        if ($ar[1] != '0') {
            $par = explode('_',$parent);
            $ar_nodes[] = array(
                'id' => $ar[1],
                'context' => $par[0],
                'parent' => $par[1],
                'order' => $order,
            );
            $order++;
        }
        getNodesFormatted($ar_nodes,$children,$id);
    }
}

[juro]

juro submitted:

We have found several other bugs in this processor:

1. any context that is open in the resource tree will get into $contextsAffected even if not involved in drag operation, thus entire context cache is unnecessarily refreshed
2. context_settings cache is not refreshed for any context (thought partial resource cache is still refreshed), resulting in mismatched information in cached context structure and database records

The latter bug is caused by calling $modx->cacheManager->refresh having $contextsAffected as associative array (context names as array keys), while other processors use simple array (context names as array values).

Seems that all bugs mentioned in this issue are same in version 2.2.

Because this issue was never reopened and corrected as we have asked for, help yourself:

hasPermission('save_document')) return $modx->error->failure($modx->lexicon('access_denied'));
$modx->lexicon->load('resource', 'context');
$data = urldecode($scriptProperties['data']);
$data = $modx->fromJSON($data);
$nodes = array();
getNodesFormatted($nodes,$data);
$modx->invokeEvent('OnResourceBeforeSort',array(
    'nodes' => &$nodes,
));
/* readjust cache */
$nodeErrors = array();
$modifiedNodes = array();
$contextsAffected = array();
$dontChangeParents = array();
foreach ($nodes as $ar_node) {
      $hasChanged=false; /* mxedit */
    if (!is_array($ar_node) || empty($ar_node['id'])) continue;
    $node = $modx->getObject('modResource',$ar_node['id']);
    if (empty($node)) continue;
    if (empty($ar_node['context'])) continue;
    if (in_array($ar_node['parent'],$dontChangeParents)) continue;
    $old_parent_id = $node->get('parent');
    if ($old_parent_id != $ar_node['parent']) {
        /* get new parent, if invalid, skip, unless is root */
        if ($ar_node['parent'] != 0) {
            $parent = $modx->getObject('modResource',$ar_node['parent']);
            if ($parent == null) {
                $nodeErrors[] = $modx->lexicon('resource_err_new_parent_nf', array('id' => $ar_node['parent']));
                continue;
            }
            if (!$parent->checkPolicy('add_children')) {
                $nodeErrors[] = $modx->lexicon('resource_add_children_access_denied');
                continue;
            }
        } else {
            $context = $modx->getObject('modContext',$ar_node['context']);
            if (empty($context)) {
                $nodeErrors[] = $modx->lexicon('context_err_nfs', array('key' => $ar_node['context']));
                continue;
            }
            if (!$modx->hasPermission('new_document_in_root')) {
                $nodeErrors[] = $modx->lexicon('resource_add_children_access_denied');
                continue;
            }
        }
        /* save new parent */
        $hasChanged=true; /* mxedit */
        $node->set('parent',$ar_node['parent']);
    }
    $old_context_key = $node->get('context_key');
    /* $contextsAffected[$old_context_key] = true; */ /* mxedit */
    if ($old_context_key != $ar_node['context'] && !empty($ar_node['context'])) {
        $hasChanged=true; /* mxedit */
        $node->set('context_key',$ar_node['context']);
        $contextsAffected[$ar_node['context']] = true;
        $dontChangeParents[] = $node->get('id'); /* prevent children from reverting back */
    }
    if ($node->get('menuindex')!=$ar_node['order']) { /* mxedit */
      $node->set('menuindex',$ar_node['order']);
      $hasChanged=true; /* mxedit */
    } /* mxedit */
    if ($hasChanged) { /* mxedit */
      $modifiedNodes[] = $node;
      $contextsAffected[$old_context_key] = true; /* mxedit */
    } /* mxedit */
}
if (!empty($modifiedNodes)) {
    foreach ($modifiedNodes as $modifiedNode) {
        if (!$modifiedNode->checkPolicy('save')) {
            $nodeErrors[] = $modx->lexicon('resource_err_save');
        }
    }
}
if (!empty($nodeErrors)) {
    return $modx->error->failure(implode("\n", array_unique($nodeErrors)));
}
if (!empty($modifiedNodes)) {
    foreach ($modifiedNodes as $modifiedNode) {
        $modifiedNode->save();
    }
}
$modx->invokeEvent('OnResourceSort', array(
    'nodes' => &$nodes,
    'modifiedNodes' => &$modifiedNodes
));
/* empty cache */
$modx->cacheManager->refresh(array(
    'db' => array(),
    'auto_publish' => array('contexts' => $contextsAffected),
    'context_settings' => array('contexts' => array_keys($contextsAffected)), /* mxedit */
    'resource' => array('contexts' => $contextsAffected),
));
return $modx->error->success();
function getNodesFormatted(&$ar_nodes,$cur_level,$parent = 0) {
    $order = 0;
    foreach ($cur_level as $id => $children) {
        $ar = explode('_',$id);
        if ($ar[1] != '0') {
            $par = explode('_',$parent);
            $ar_nodes[] = array(
                'id' => $ar[1],
                'context' => $par[0],
                'parent' => $par[1],
                'order' => $order,
            );
            $order++;
        }
        getNodesFormatted($ar_nodes,$children,$id);
    }
}

[cyclissmo]

cyclissmo submitted:

There is still a serious flaw with drag and drop operations in the Resource Tree when using ACL and rights-limited resource groups. The patch posted here does not work for the most recent version of Revo (2.2.7). I have spent all day trying to create an efficient patch in @sort.class.php@ with no success. I will keep working on a fix, but in the meantime, I would recommend that this bug be reopened.

[juro]

juro submitted:

Hi, this is our 2.2.7 version of this processor, seems to work so far.

modx->hasPermission('save_document');
    }
    public function getLanguageTopics() {
        return array('resource','context');
    }
    public function process() {
        $data = urldecode($this->getProperty('data',''));
        if (empty($data)) $this->failure($this->modx->lexicon('invalid_data'));
        $data = $this->modx->fromJSON($data);
        if (empty($data)) $this->failure($this->modx->lexicon('invalid_data'));
        $this->getNodesFormatted($data,0);
        $this->fireBeforeSort();
        /* sort contexts */
        foreach ($this->contexts as $key => $value) {
            $context = $this->modx->getObject('modContext',array(
                'key' => $value
            ));
            if ($context !== null) {
                $context->set('rank', $key);
                $context->save();
            }
        }
        
        /* readjust cache */
        $nodeErrors = array();
        $dontChangeParents = array();
        foreach ($this->nodes as $ar_node) {
            $hasChanged=false; /* mxedit */
            if (!is_array($ar_node) || empty($ar_node['id'])) continue;
            /** @var modResource $node */
            $node = $this->modx->getObject('modResource',$ar_node['id']);
            if (empty($node)) continue;
            if (empty($ar_node['context'])) continue;
            if (in_array($ar_node['parent'],$dontChangeParents)) continue;
            $old_parent_id = $node->get('parent');
            if ($old_parent_id != $ar_node['parent']) {
                /* get new parent, if invalid, skip, unless is root */
                if ($ar_node['parent'] != 0) {
                    /** @var modResource $parent */
                    $parent = $this->modx->getObject('modResource',$ar_node['parent']);
                    if ($parent == null) {
                        $nodeErrors[] = $this->modx->lexicon('resource_err_new_parent_nf', array('id' => $ar_node['parent']));
                        continue;
                    }
                    if (!$parent->checkPolicy('add_children')) {
                        $nodeErrors[] = $this->modx->lexicon('resource_add_children_access_denied');
                        continue;
                    }
                } else {
                    $context = $this->modx->getObject('modContext',$ar_node['context']);
                    if (empty($context)) {
                        $nodeErrors[] = $this->modx->lexicon('context_err_nfs', array('key' => $ar_node['context']));
                        continue;
                    }
                    if (!$this->modx->hasPermission('new_document_in_root')) {
                        $nodeErrors[] = $this->modx->lexicon('resource_add_children_access_denied');
                        continue;
                    }
                }
                /* save new parent */
                $hasChanged=true; /* mxedit */
                $node->set('parent',$ar_node['parent']);
            }
            $old_context_key = $node->get('context_key');
            $this->contextsAffected[$old_context_key] = true;
            if ($old_context_key != $ar_node['context'] && !empty($ar_node['context'])) {
                $hasChanged=true; /* mxedit */
                $node->set('context_key',$ar_node['context']);
                $this->contextsAffected[$ar_node['context']] = true;
                $dontChangeParents[] = $node->get('id'); /* prevent children from reverting back */
            }
            if ($node->get('menuindex')!=$ar_node['order']) { /* mxedit */
                $node->set('menuindex',$ar_node['order']);
                $hasChanged=true; /* mxedit */
            } /* mxedit */
            if ($hasChanged) { /* mxedit */
                $this->nodesAffected[] = $node;
            } /* mxedit */
        }
        if (!empty($this->nodesAffected)) {
            /** @var modResource $modifiedNode */
            foreach ($this->nodesAffected as $modifiedNode) {
                if (!$modifiedNode->checkPolicy('save')) {
                    $nodeErrors[] = $this->modx->lexicon('resource_err_save');
                }
            }
        }
        if (!empty($nodeErrors)) {
            return $this->modx->error->failure(implode("\n", array_unique($nodeErrors)));
        }
        if (!empty($this->nodesAffected)) {
            foreach ($this->nodesAffected as $modifiedNode) {
                $modifiedNode->save();
            }
        }
        $this->fireAfterSort();
        /* empty cache */
        $this->clearCache();
        return $this->success();
    }
    protected function getNodesFormatted($currentLevel,$parent = 0) {
        $order = 0;
        $previousContext = null;
        foreach ($currentLevel as $id => $children) {
            $explodedArray = explode('_',$id);
            if ($explodedArray[1] != '0') {
                $explodedParentArray = explode('_',$parent);
                $this->nodes[] = array(
                    'id' => $explodedArray[1],
                    'context' => $explodedParentArray[0],
                    'parent' => $explodedParentArray[1],
                    'order' => $order,
                );
                $order++;
            } else {
                if ($previousContext !== $explodedArray[0]) {
                    $this->contexts[] = $explodedArray[0];
                }
                $previousContext = $explodedArray[0];
            }
            $this->getNodesFormatted($children,$id);
        }
    }
    public function fireBeforeSort() {
        $this->modx->invokeEvent('OnResourceBeforeSort',array(
            'nodes' => &$this->nodes,
            'contexts' => &$this->contexts,
        ));
    }
    public function fireAfterSort() {
        $this->modx->invokeEvent('OnResourceSort', array(
            'nodes' => &$this->nodes,
            'nodesAffected' => &$this->nodesAffected,
            'contexts' => &$this->contexts,
            'contexts' => &$this->contextsAffected,
            'modifiedNodes' => &$this->nodesAffected, /* backward compat */
        ));
    }
    public function clearCache() {
        $this->modx->cacheManager->refresh(array(
            'db' => array(),
            'auto_publish' => array('contexts' => $this->contextsAffected),
            'context_settings' => array('contexts' => $this->contextsAffected),
            'resource' => array('contexts' => $this->contextsAffected),
        ));
    }
}
return 'modResourceSortProcessor';