From 0e530d753a7765360d881d0e366bddf87787e6d4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=B4me=20Chilliet?= <come@opensides.be>
Date: Thu, 21 Dec 2017 11:18:21 +0100
Subject: [PATCH] :ambulance: fix(simple-plugin): Fix tab remove and object
deletion
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Renamed remove_from_parent to remove
Added a boolean parameter for fulldelete indication
Added a PHP error for ACL bypassing
Added an ACLÂ check in tabs at whole object level for deletion
Cancel deletion at first tab deletion error
issue #5747
---
.../password-methods/class_password-methods.inc | 9 ---------
include/simpleplugin/class_multiPlugin.inc | 6 +++---
include/simpleplugin/class_simpleManagement.inc | 2 +-
include/simpleplugin/class_simplePlugin.inc | 10 ++++++++--
include/simpleplugin/class_simpleTabs.inc | 17 ++++++++++++-----
plugins/admin/acl/class_aclAssignment.inc | 2 +-
plugins/admin/groups/tabs_ogroups.inc | 2 +-
plugins/personal/roles/class_userRoles.inc | 5 +++--
8 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/include/password-methods/class_password-methods.inc b/include/password-methods/class_password-methods.inc
index b8d11ebe8..2af280ce4 100644
--- a/include/password-methods/class_password-methods.inc
+++ b/include/password-methods/class_password-methods.inc
@@ -246,15 +246,6 @@ class passwordMethod
return "";
}
-
- /*!
- * \brief Method to let password backends remove additional information besides
- * the userPassword attribute
- */
- function remove_from_parent()
- {
- }
-
/*!
* \brief Method to check if a password matches a hash
*/
diff --git a/include/simpleplugin/class_multiPlugin.inc b/include/simpleplugin/class_multiPlugin.inc
index eda8457c8..23727954c 100644
--- a/include/simpleplugin/class_multiPlugin.inc
+++ b/include/simpleplugin/class_multiPlugin.inc
@@ -163,7 +163,7 @@ class multiPlugin extends simplePlugin
if ($plug->is_account || $plug->ignore_account) {
$result = $plug->save();
} else {
- $result = $plug->remove_from_parent();
+ $result = $plug->remove(FALSE);
}
if (!empty($result)) {
$errors = array_merge($errors, $result);
@@ -173,13 +173,13 @@ class multiPlugin extends simplePlugin
return $errors;
}
- function remove_from_parent()
+ function remove($fulldelete = FALSE)
{
$errors = array();
/* Remove objects */
foreach ($this->plugin as &$plug) {
$plug->dn = $this->dn;
- $result = $plug->remove_from_parent();
+ $result = $plug->remove($fulldelete);
if (!empty($result)) {
$errors = array_merge($errors, $result);
}
diff --git a/include/simpleplugin/class_simpleManagement.inc b/include/simpleplugin/class_simpleManagement.inc
index 9670bf57d..2980c169c 100644
--- a/include/simpleplugin/class_simpleManagement.inc
+++ b/include/simpleplugin/class_simpleManagement.inc
@@ -1095,7 +1095,7 @@ class simpleManagement
// Remove the lock for the current object.
del_lock($this->dn);
} else {
- msg_dialog::display(_('Permission error'), msgPool::permDelete(), ERROR_DIALOG);
+ msg_dialog::display(_('Permission error'), msgPool::permDelete($dn), ERROR_DIALOG);
logging::log('security', 'simpleManagement/'.get_class($this), $dn, array(), 'Tried to trick deletion.');
}
}
diff --git a/include/simpleplugin/class_simplePlugin.inc b/include/simpleplugin/class_simplePlugin.inc
index ca36da71a..c61cb04c9 100644
--- a/include/simpleplugin/class_simplePlugin.inc
+++ b/include/simpleplugin/class_simplePlugin.inc
@@ -962,11 +962,17 @@ class simplePlugin
/*! \brief This function removes the object from LDAP
*/
- function remove_from_parent()
+ function remove($fulldelete = FALSE)
{
- if (!$this->initially_was_account || !$this->acl_is_removeable()) {
+ if (!$this->initially_was_account) {
return array();
}
+
+ if (!$fulldelete && !$this->acl_is_removeable()) {
+ trigger_error('remove was called on a tab without enough ACL rights');
+ return array();
+ }
+
$this->prepare_remove();
if ($this->is_template && (!defined('_OLD_TEMPLATES_') || !_OLD_TEMPLATES_)) {
$this->attrs = $this->templateSaveAttrs();
diff --git a/include/simpleplugin/class_simpleTabs.inc b/include/simpleplugin/class_simpleTabs.inc
index fd42f6838..54f624603 100644
--- a/include/simpleplugin/class_simpleTabs.inc
+++ b/include/simpleplugin/class_simpleTabs.inc
@@ -299,17 +299,24 @@ class simpleTabs
}
/*!
- * \brief Remove object from parent
+ * \brief Remove object from LDAP
*/
function delete()
{
- /* Delete for all plugins */
+ if (!$this->getBaseObject()->acl_is_removeable()) {
+ msg_dialog::display(_('Permission'), msgPool::permDelete($this->getBaseObject()->dn), ERROR_DIALOG);
+ return FALSE;
+ }
+
+ /* Delete all tabs in reverse order */
foreach (array_reverse($this->by_object) as $obj) {
- $errors = $obj->remove_from_parent();
+ $errors = $obj->remove(TRUE);
if (!empty($errors)) {
msg_dialog::displayChecks($errors);
+ return FALSE;
}
}
+ return TRUE;
}
/*!
@@ -408,11 +415,11 @@ class simpleTabs
if ($obj->is_account || $obj->ignore_account) {
$result = $obj->save();
} else {
- $result = $obj->remove_from_parent();
+ $result = $obj->remove(FALSE);
}
if (!empty($result)) {
if ($creation && $first) {
- /* If the fail of main tab fails for a creation, cancel the save of other tabs */
+ /* If the save of main tab fails for a creation, cancel the save of other tabs */
$this->dn = $old_dn;
$obj->dn = $this->dn;
return $result;
diff --git a/plugins/admin/acl/class_aclAssignment.inc b/plugins/admin/acl/class_aclAssignment.inc
index 04e799e2d..19c59043b 100644
--- a/plugins/admin/acl/class_aclAssignment.inc
+++ b/plugins/admin/acl/class_aclAssignment.inc
@@ -320,7 +320,7 @@ class aclAssignment extends simplePlugin
if ($this->is_account) {
return parent::save();
} else {
- return $this->remove_from_parent();
+ return $this->remove(FALSE);
}
}
diff --git a/plugins/admin/groups/tabs_ogroups.inc b/plugins/admin/groups/tabs_ogroups.inc
index 5e941e614..0b1c6e160 100644
--- a/plugins/admin/groups/tabs_ogroups.inc
+++ b/plugins/admin/groups/tabs_ogroups.inc
@@ -150,7 +150,7 @@ class ogrouptabs extends simpleTabs_noSpecial
@DEBUG (DEBUG_TRACE, __LINE__, __FUNCTION__, __FILE__, $key, "Removing");
$obj->dn = $this->dn;
- $errors = $obj->remove_from_parent();
+ $errors = $obj->remove(FALSE);
if (!empty($errors)) {
msg_dialog::displayChecks($errors);
}
diff --git a/plugins/personal/roles/class_userRoles.inc b/plugins/personal/roles/class_userRoles.inc
index 18d90b466..ebf1a0448 100644
--- a/plugins/personal/roles/class_userRoles.inc
+++ b/plugins/personal/roles/class_userRoles.inc
@@ -167,10 +167,10 @@ class userRoles extends simplePlugin
return FALSE;
}
- function remove_from_parent()
+ protected function ldap_remove()
{
if ($this->is_template) {
- parent::remove_from_parent();
+ return parent::ldap_remove();
} elseif (($this->dn != '') && ($this->dn != 'new')) {
/* Remove all groups */
foreach ($this->savedGroupsMembership as $ogroupdn) {
@@ -185,6 +185,7 @@ class userRoles extends simplePlugin
$r->save();
}
}
+ return array();
}
function save_object()
--
GitLab