Permalink
Browse files

fix(users): mitigate race conditions when deleting/disabling users

Deleting or disabling a user can be a long process, during which the affected
user may try to continue creating content. We now temporarily ban the user
during the process to prevent further actions.

Refs #8099
  • Loading branch information...
mrclay committed Apr 6, 2016
1 parent fc898ac commit da45affef3fa16106305ab424a51fc69518ca66b
Showing with 71 additions and 48 deletions.
  1. +26 −18 engine/classes/Elgg/Database/UsersTable.php
  2. +45 −30 engine/classes/ElggEntity.php
@@ -43,8 +43,6 @@ public function __construct() {
* @access private
*/
function getRow($guid) {
-
-
$guid = (int)$guid;
return _elgg_services()->db->getDataRow("SELECT * from {$this->CONFIG->dbprefix}users_entity where guid=$guid");
}
@@ -57,7 +55,6 @@ function getRow($guid) {
* @return bool Depending on success
*/
function disableEntities($owner_guid) {
-
$owner_guid = (int) $owner_guid;
if ($entity = get_entity($owner_guid)) {
if (_elgg_services()->events->trigger('disable', $entity->type, $entity)) {
@@ -76,21 +73,19 @@ function disableEntities($owner_guid) {
}
/**
- * Ban a user
+ * Ban a user (calls events, stores the reason)
*
* @param int $user_guid The user guid
* @param string $reason A reason
*
* @return bool
*/
function ban($user_guid, $reason = "") {
-
-
$user_guid = (int)$user_guid;
$user = get_entity($user_guid);
- if (($user) && ($user->canEdit()) && ($user instanceof \ElggUser)) {
+ if (($user instanceof \ElggUser) && $user->canEdit()) {
if (_elgg_services()->events->trigger('ban', 'user', $user)) {
// Add reason
if ($reason) {
@@ -107,27 +102,44 @@ function ban($user_guid, $reason = "") {
$newentity_cache->delete($user_guid);
}
- // Set ban flag
- $query = "UPDATE {$this->CONFIG->dbprefix}users_entity set banned='yes' where guid=$user_guid";
- return _elgg_services()->db->updateData($query);
+ return $this->markBanned($user_guid, true);
}
return false;
}
return false;
}
+
+ /**
+ * Mark a user entity banned or unbanned.
+ *
+ * @note Use ban() or unban()
+ *
+ * @param int $guid User GUID
+ * @param bool $banned Mark the user banned?
+ *
+ * @return int Num rows affected
+ */
+ public function markBanned($guid, $banned) {
+ $banned = $banned ? 'yes' : 'no';
+ $query = "
+ UPDATE {$this->CONFIG->dbprefix}users_entity
+ SET banned = '$banned'
+ WHERE guid = $guid
+ ";
+
+ return _elgg_services()->db->updateData($query);
+ }
/**
- * Unban a user.
+ * Unban a user (calls events, removes the reason)
*
* @param int $user_guid Unban a user.
*
* @return bool
*/
function unban($user_guid) {
-
-
$user_guid = (int)$user_guid;
$user = get_entity($user_guid);
@@ -146,9 +158,7 @@ function unban($user_guid) {
$newentity_cache->delete($user_guid);
}
-
- $query = "UPDATE {$this->CONFIG->dbprefix}users_entity set banned='no' where guid=$user_guid";
- return _elgg_services()->db->updateData($query);
+ return $this->markBanned($user_guid, false);
}
return false;
@@ -165,8 +175,6 @@ function unban($user_guid) {
* @return bool
*/
function makeAdmin($user_guid) {
-
-
$user = get_entity((int)$user_guid);
if (($user) && ($user instanceof \ElggUser) && ($user->canEdit())) {
@@ -1903,6 +1903,14 @@ public function disable($reason = "", $recursive = true) {
return false;
}
+ if ($this instanceof ElggUser && $this->banned === 'no') {
+ // temporarily ban to prevent using the site during disable
+ _elgg_services()->usersTable->markBanned($this->guid, true);
+ $unban_after = true;
+ } else {
+ $unban_after = false;
+ }
+
_elgg_invalidate_cache_for_entity($this->guid);
if ($reason) {
@@ -1916,15 +1924,21 @@ public function disable($reason = "", $recursive = true) {
$hidden = access_get_show_hidden_status();
access_show_hidden_entities(true);
$ia = elgg_set_ignore_access(true);
-
- $sub_entities = $this->getDatabase()->getData("SELECT * FROM {$CONFIG->dbprefix}entities
+
+ $query = "
+ SELECT *
+ FROM {$CONFIG->dbprefix}entities
WHERE (
- container_guid = $guid
- OR owner_guid = $guid
- OR site_guid = $guid
- ) AND enabled='yes'", 'entity_row_to_elggstar');
+ container_guid = $guid
+ OR owner_guid = $guid
+ OR site_guid = $guid
+ )
+ AND enabled = 'yes'
+ ";
+ $sub_entities = $this->getDatabase()->getData($query, 'entity_row_to_elggstar');
if ($sub_entities) {
+ /* @var ElggEntity[] $sub_entities */
foreach ($sub_entities as $e) {
add_entity_relationship($e->guid, 'disabled_with', $this->guid);
$e->disable($reason);
@@ -1938,9 +1952,15 @@ public function disable($reason = "", $recursive = true) {
$this->disableMetadata();
$this->disableAnnotations();
- $res = $this->getDatabase()->updateData("UPDATE {$CONFIG->dbprefix}entities
+ $res = $this->getDatabase()->updateData("
+ UPDATE {$CONFIG->dbprefix}entities
SET enabled = 'no'
- WHERE guid = $guid");
+ WHERE guid = $guid
+ ");
+
+ if ($unban_after) {
+ _elgg_services()->usersTable->markBanned($this->guid, false);
+ }
if ($res) {
$this->attributes['enabled'] = 'no';
@@ -2058,7 +2078,12 @@ public function delete($recursive = true) {
if (!_elgg_services()->events->trigger('delete', $this->type, $this)) {
return false;
}
-
+
+ if ($this instanceof ElggUser) {
+ // ban to prevent using the site during delete
+ _elgg_services()->usersTable->markBanned($this->guid, true);
+ }
+
_elgg_invalidate_cache_for_entity($guid);
// If memcache is available then delete this entry from the cache
@@ -2120,29 +2145,19 @@ public function delete($recursive = true) {
elgg_delete_river(array('target_guid' => $guid));
remove_all_private_settings($guid);
- $res = $this->getDatabase()->deleteData("DELETE FROM {$CONFIG->dbprefix}entities WHERE guid = $guid");
- if ($res) {
- $sub_table = "";
+ $res = $this->getDatabase()->deleteData("
+ DELETE FROM {$CONFIG->dbprefix}entities
+ WHERE guid = $guid
+ ");
- // Where appropriate delete the sub table
- switch ($this->type) {
- case 'object' :
- $sub_table = $CONFIG->dbprefix . 'objects_entity';
- break;
- case 'user' :
- $sub_table = $CONFIG->dbprefix . 'users_entity';
- break;
- case 'group' :
- $sub_table = $CONFIG->dbprefix . 'groups_entity';
- break;
- case 'site' :
- $sub_table = $CONFIG->dbprefix . 'sites_entity';
- break;
- }
+ if ($res && in_array($this->type, ['object', 'user', 'group', 'site'])) {
+ // delete from secondary table
+ $sub_table = "{$CONFIG->dbprefix}{$this->type}s_entity";
- if ($sub_table) {
- $this->getDatabase()->deleteData("DELETE FROM $sub_table WHERE guid = $guid");
- }
+ $this->getDatabase()->deleteData("
+ DELETE FROM $sub_table
+ WHERE guid = $guid
+ ");
}
_elgg_clear_entity_files($this);

0 comments on commit da45aff

Please sign in to comment.