From 40b1d1e6fff1fedc960f8ad160d9928c9ccf26d8 Mon Sep 17 00:00:00 2001 From: Oliver Schramm Date: Fri, 5 Oct 2018 21:32:44 +0200 Subject: [PATCH] [ticket/9687] Remove dependency on log and user services PHPBB3-9687 --- .../config/default/container/services_ban.yml | 11 +- phpBB/includes/functions_user.php | 2 + phpBB/phpbb/ban/manager.php | 119 +++-------------- phpBB/phpbb/ban/type/base.php | 120 ++++++++++++++++-- phpBB/phpbb/ban/type/email.php | 28 ++-- phpBB/phpbb/ban/type/type_interface.php | 38 +++--- phpBB/phpbb/ban/type/user.php | 96 +------------- 7 files changed, 169 insertions(+), 245 deletions(-) diff --git a/phpBB/config/default/container/services_ban.yml b/phpBB/config/default/container/services_ban.yml index 86991b93b6..141374d890 100644 --- a/phpBB/config/default/container/services_ban.yml +++ b/phpBB/config/default/container/services_ban.yml @@ -6,12 +6,8 @@ services: - '@ban.type_collection' - '@cache' - '@dbal.conn' - - '@log' - - '@user' - '%tables.bans%' - '%tables.users%' - - '%tables.sessions%' - - '%tables.sessions_keys%' # ----- Ban types ----- ban.type_collection: @@ -25,8 +21,9 @@ services: class: \phpbb\ban\type\email arguments: - '@dbal.conn' - - '@user' - '%tables.users%' + - '%tables.sessions%' + - '%tables.sessions_keys%' tags: - { name: ban.type } @@ -34,8 +31,8 @@ services: class: \phpbb\ban\type\user arguments: - '@dbal.conn' - - '@log' - - '@user' - '%tables.users%' + - '%tables.sessions%' + - '%tables.sessions_keys%' tags: - { name: ban.type } diff --git a/phpBB/includes/functions_user.php b/phpBB/includes/functions_user.php index 84b3b30e1f..b3bdcf1480 100644 --- a/phpBB/includes/functions_user.php +++ b/phpBB/includes/functions_user.php @@ -971,6 +971,8 @@ function user_ban($mode, $ban, $ban_len, $ban_len_other, $ban_exclude, $ban_reas $end->setTimestamp($ban_end); return $ban_manager->ban($mode, $items, $start, $end, $ban_reason, $ban_give_reason); + + // TODO: logging } /** diff --git a/phpBB/phpbb/ban/manager.php b/phpBB/phpbb/ban/manager.php index 346387349e..8c77c67eab 100644 --- a/phpBB/phpbb/ban/manager.php +++ b/phpBB/phpbb/ban/manager.php @@ -32,15 +32,6 @@ class manager /** @var \phpbb\db\driver\driver_interface */ protected $db; - /** @var \phpbb\log\log_interface */ - protected $log; - - /** @var string */ - protected $sessions_keys_table; - - /** @var string */ - protected $sessions_table; - /** @var \phpbb\di\service_collection */ protected $types; @@ -57,23 +48,15 @@ class manager * @param \phpbb\di\service_collection $types A service collection containing all ban types * @param \phpbb\cache\service $cache A cache object * @param \phpbb\db\driver\driver_interface $db A phpBB DBAL object - * @param \phpbb\log\log_interface $log A log object - * @param \phpbb\user $user An user object * @param string $bans_table The bans table * @param string $users_table The users table - * @param string $sessions_table The sessions table - * @param string $sessions_keys_table The sessions key table */ - public function __construct($types, \phpbb\cache\service $cache, \phpbb\db\driver\driver_interface $db, \phpbb\log\log_interface $log, \phpbb\user $user, $bans_table, $users_table = '', $sessions_table = '', $sessions_keys_table = '') + public function __construct($types, \phpbb\cache\service $cache, \phpbb\db\driver\driver_interface $db, $bans_table, $users_table = '') { $this->bans_table = $bans_table; $this->cache = $cache; $this->db = $db; - $this->log = $log; - $this->sessions_keys_table = $sessions_keys_table; - $this->sessions_table = $sessions_table; $this->types = $types; - $this->user = $user; $this->users_table = $users_table; } @@ -103,6 +86,10 @@ class manager { throw new type_not_found_exception(); // TODO } + if (!empty($this->user)) + { + $ban_mode->set_user($this->user); + } $this->tidy(); $ban_items = $ban_mode->prepare_for_storage($items); @@ -137,19 +124,6 @@ class manager throw new ban_insert_failed_exception(); // TODO } - if ($ban_mode->get_ban_log_string() !== false) - { - $ban_items_log = implode(', ', $ban_items); - - $this->log->add('admin', $this->user->data['user_id'], $this->user->ip, $ban_mode->get_ban_log_string(), false, [$reason, $ban_items_log]); - $this->log->add('mod', $this->user->data['user_id'], $this->user->ip, $ban_mode->get_ban_log_string(), false, [ - 'forum_id' => 0, - 'topic_id' => 0, - $reason, - $ban_items_log, - ]); - } - $ban_data = [ 'items' => $ban_items, 'start' => $start, @@ -160,56 +134,7 @@ class manager if ($ban_mode->after_ban($ban_data)) { - $user_column = $ban_mode->get_user_column(); - if (!empty($user_column) && !empty($this->users_table)) - { - if ($user_column !== 'user_id') - { - $ban_items_sql = []; - $ban_or_like = ''; - foreach ($ban_items as $ban_item) - { - if (stripos($ban_item, '*') === false) - { - $ban_items_sql[] = $ban_item; - } - else - { - $ban_or_like .= ' OR ' . $user_column . ' ' . $this->db->sql_like_expression(str_replace('*', $this->db->get_any_char(), $ban_item)); - } - } - - // TODO: Prevent logging out founders - $sql = 'SELECT user_id - FROM ' . $this->users_table . ' - WHERE ' . $this->db->sql_in_set('u.' . $user_column, $ban_items_sql, false, true) . $ban_or_like; - $result = $this->db->sql_query($sql); - - $user_ids = []; - while ($row = $this->db->sql_fetchrow($result)) - { - $user_ids[] = (int) $row['user_id']; - } - $this->db->sql_freeresult($result); - } - else - { - $user_ids = $ban_items; - } - - if (!empty($user_ids) && !empty($this->sessions_table)) - { - $sql = 'DELETE FROM ' . $this->sessions_table . ' - WHERE ' . $this->db->sql_in_set('session_user_id', $user_ids); - $this->db->sql_query($sql); - } - if (!empty($user_ids) && !empty($this->sessions_keys_table)) - { - $sql = 'DELETE FROM ' . $this->sessions_keys_table . ' - WHERE ' . $this->db->sql_in_set('user_id', $user_ids); - $this->db->sql_query($sql); - } - } + // TODO } $this->cache->destroy(self::CACHE_KEY_INFO); @@ -223,9 +148,8 @@ class manager * * @param string $mode The ban type in which the ban IDs were created * @param array $items An array of ban IDs which should be removed - * @param bool $logging True, if log entries should be created, false otherwise. */ - public function unban($mode, array $items, $logging = true) + public function unban($mode, array $items) { /** @var \phpbb\ban\type\type_interface $ban_mode */ $ban_mode = $this->find_type($mode); @@ -252,23 +176,10 @@ class manager WHERE ' . $this->db->sql_in_set('ban_id', $sql_ids); $this->db->sql_query($sql); - if ($logging && $ban_mode->get_unban_log_string() !== false) - { - $unban_items_log = implode(', ', $unbanned_items); - - $this->log->add('admin', $this->user->data['user_id'], $this->user->ip, $ban_mode->get_unban_log_string(), false, [$unban_items_log]); - $this->log->add('mod', $this->user->data['user_id'], $this->user->ip, $ban_mode->get_unban_log_string(), false, [ - 'forum_id' => 0, - 'topic_id' => 0, - $unban_items_log, - ]); - } - $unban_data = [ 'items' => $unbanned_items, - 'logging' => $logging, ]; - $ban_mode->after_unban($unban_data); + $unbanned_users = $ban_mode->after_unban($unban_data); $this->cache->destroy(self::CACHE_KEY_INFO); $this->cache->destroy(self::CACHE_KEY_USERS); @@ -347,7 +258,7 @@ class manager /** * Returns all bans for a given ban type. False, if none were found * - * @param strng $mode The ban type for which the entries should be retrieved + * @param string $mode The ban type for which the entries should be retrieved * * @return array|bool */ @@ -416,7 +327,7 @@ class manager $where_array, ], ['u.user_type', '<>', USER_FOUNDER], - ] + ], ], ]; $sql = $this->db->sql_build_query('SELECT', $sql_array); @@ -457,6 +368,16 @@ class manager }); } + /** + * Sets the current user to exclude from banning + * + * @param \phpbb\user $user An user object + */ + public function set_user(\phpbb\user $user) + { + $this->user = $user; + } + /** * Cleans up the database of e.g. stale bans */ diff --git a/phpBB/phpbb/ban/type/base.php b/phpBB/phpbb/ban/type/base.php index 9a1e70ab77..c269bb640e 100644 --- a/phpBB/phpbb/ban/type/base.php +++ b/phpBB/phpbb/ban/type/base.php @@ -21,6 +21,12 @@ abstract class base implements type_interface /** @var array */ protected $excluded; + /** @var string */ + protected $sessions_keys_table; + + /** @var string */ + protected $sessions_table; + /** @var \phpbb\user */ protected $user; @@ -30,15 +36,26 @@ abstract class base implements type_interface /** * Creates a ban type. * - * @param \phpbb\db\driver\driver_interface $db A phpBB DBAL object - * @param \phpbb\user $user An user object - * @param string $users_table The users table + * @param \phpbb\db\driver\driver_interface $db A phpBB DBAL object + * @param string $users_table The users table + * @param string $sessions_table The sessions table + * @param string $sessions_keys_table The sessions keys table */ - public function __construct(\phpbb\db\driver\driver_interface $db, \phpbb\user $user, $users_table) + public function __construct(\phpbb\db\driver\driver_interface $db, $users_table, $sessions_table, $sessions_keys_table) { $this->db = $db; - $this->user = $user; $this->users_table = $users_table; + $this->sessions_table = $sessions_table; + $this->sessions_keys_table = $sessions_keys_table; + } + + /** + * {@inheritDoc} + */ + public function set_user(\phpbb\user $user) + { + // TODO: Implement new logging + $this->user = $user; } /** @@ -46,7 +63,7 @@ abstract class base implements type_interface */ public function after_ban(array $data) { - return true; + return $data['items']; } /** @@ -54,6 +71,7 @@ abstract class base implements type_interface */ public function after_unban(array $data) { + return $data['items']; } /** @@ -94,9 +112,12 @@ abstract class base implements type_interface return false; } - $this->excluded = [ - (int) $this->user->data['user_id'] => $this->user->data[$user_column], - ]; + $this->excluded = []; + + if (!empty($this->user)) + { + $this->excluded[(int) $this->user->data['user_id']] = $this->user->data[$user_column]; + } $sql = "SELECT user_id, {$user_column} FROM {$this->users_table} @@ -111,4 +132,85 @@ abstract class base implements type_interface return true; } + + /** + * Logs out all affected users in the given array. The values + * have to match the values of the column returned by get_user_column(). + * Returns all banned users. + * + * @param array $ban_items + * + * @return array + */ + protected function logout_affected_users(array $ban_items) + { + $user_column = $this->get_user_column(); + + if (empty($user_column)) + { + // TODO throw ex (it's a developer exception) + } + + if ($user_column !== 'user_id') + { + $ban_items_sql = []; + $ban_like_items = []; + foreach ($ban_items as $ban_item) + { + if (stripos($ban_item, '*') === false) + { + $ban_items_sql[] = $ban_item; + } + else + { + $ban_like_items[] = [$user_column, 'LIKE', str_replace('*', $this->db->get_any_char(), $ban_item)]; + } + } + + $sql_array = [ + 'SELECT' => 'user_id', + 'FROM' => [ + $this->users_table => '', + ], + 'WHERE' => ['AND', + [ + ['OR', + array_merge([ + [$user_column, 'IN', $ban_items_sql] + ], $ban_like_items), + ], + ['user_id', 'NOT_IN', array_map('intval', array_keys($this->excluded))], + ], + ], + ]; + $sql = $this->db->sql_build_query('SELECT', $sql_array); + $result = $this->db->sql_query($sql); + + $user_ids = []; + while ($row = $this->db->sql_fetchrow($result)) + { + $user_ids[] = (int) $row['user_id']; + } + $this->db->sql_freeresult($result); + } + else + { + $user_ids = array_map('intval', $ban_items); + } + + if (!empty($user_ids) && !empty($this->sessions_table)) + { + $sql = 'DELETE FROM ' . $this->sessions_table . ' + WHERE ' . $this->db->sql_in_set('session_user_id', $user_ids); + $this->db->sql_query($sql); + } + if (!empty($user_ids) && !empty($this->sessions_keys_table)) + { + $sql = 'DELETE FROM ' . $this->sessions_keys_table . ' + WHERE ' . $this->db->sql_in_set('user_id', $user_ids); + $this->db->sql_query($sql); + } + + return $user_ids; + } } diff --git a/phpBB/phpbb/ban/type/email.php b/phpBB/phpbb/ban/type/email.php index 1a0e515aaa..ec3e56315f 100644 --- a/phpBB/phpbb/ban/type/email.php +++ b/phpBB/phpbb/ban/type/email.php @@ -18,22 +18,6 @@ use phpbb\exception\runtime_exception; class email extends base { - /** - * {@inheritDoc} - */ - public function get_ban_log_string() - { - return 'LOG_BAN_EMAIL'; - } - - /** - * {@inheritDoc} - */ - public function get_unban_log_string() - { - return 'LOG_UNBAN_EMAIL'; - } - /** * {@inheritDoc} */ @@ -50,6 +34,16 @@ class email extends base return 'user_email'; } + /** + * {@inheritDoc} + */ + public function after_ban(array $data) + { + $this->logout_affected_users($data['items']); + + return parent::after_ban($data); + } + /** * {@inheritDoc} */ @@ -59,7 +53,7 @@ class email extends base { throw new runtime_exception(); // TODO } - $regex = '#^.*?@.*|(([a-z0-9\-]+\.)+([a-z]{2,3}))$#i'; + $regex = '#^.*?@.*|(([a-z0-9\-]+\.)+([a-z]{2,3}))$#i'; // TODO $ban_items = []; foreach ($items as $item) diff --git a/phpBB/phpbb/ban/type/type_interface.php b/phpBB/phpbb/ban/type/type_interface.php index cc4e0d0790..d0678bc897 100644 --- a/phpBB/phpbb/ban/type/type_interface.php +++ b/phpBB/phpbb/ban/type/type_interface.php @@ -18,22 +18,6 @@ namespace phpbb\ban\type; */ interface type_interface { - /** - * Returns the language key that's used for the ban log entry. - * False, if there is none (and thus no logs are created) - * - * @return string|bool - */ - public function get_ban_log_string(); - - /** - * Returns the language key that's used for the unban log entry. - * False, if thee is none (and thus no logs are created) - * - * @return string|bool - */ - public function get_unban_log_string(); - /** * Returns the type identifier for this ban type * @@ -51,28 +35,38 @@ interface type_interface */ public function get_user_column(); + /** + * Sets a user object to the ban type to have it excluded + * from banning. + * + * @param \phpbb\user $user An user object + * + * @return null + */ + public function set_user(\phpbb\user $user); + /** * Gives the possibility to do some clean up after banning. - * Returns true if affected users should be logged out and - * false otherwise + * The return value of this method will be passed through + * to the caller. * * @param array $data An array containing information about * the bans, like the reason or the start * and end of the ban * - * @return bool + * @return mixed */ public function after_ban(array $data); /** * Gives the possiblity to do some clean up after unbanning. - * The return value of this method will be ignored and thus - * should be null + * The return value of this method will be passed through + * to the caller. * * @param array $data An array containing information about * the unbans, e.g. the unbanned items. * - * @return null + * @return mixed */ public function after_unban(array $data); diff --git a/phpBB/phpbb/ban/type/user.php b/phpBB/phpbb/ban/type/user.php index 299418df17..7bc2bfefaa 100644 --- a/phpBB/phpbb/ban/type/user.php +++ b/phpBB/phpbb/ban/type/user.php @@ -21,47 +21,6 @@ class user extends base /** @var array */ private $banned_users; - /** @var \phpbb\log\log_interface */ - protected $log; - - /** @var string */ - private $ban_log_string = 'LOG_BAN_USER'; - - /** @var string */ - private $unban_log_string = 'LOG_UNBAN_USER'; - - /** - * Creates the user ban type - * - * @param \phpbb\db\driver\driver_interface $db A phpBB DBAL object - * @param \phpbb\log\log_interface $log A log object - * @param \phpbb\user $user An user object - * @param string $users_table The users table - */ - public function __construct(\phpbb\db\driver\driver_interface $db, \phpbb\log\log_interface $log, \phpbb\user $user, $users_table) - { - $this->log = $log; - - parent::__construct($db, $user, $users_table); - } - - /** - * {@inheritDoc} - */ - public function get_ban_log_string() - { - // Have to handle logging differently here - return false; - } - - /** - * {@inheritDoc} - */ - public function get_unban_log_string() - { - return false; - } - /** * {@inheritDoc} */ @@ -83,26 +42,8 @@ class user extends base */ public function after_ban(array $data) { - $usernames_log = implode(', ', $this->banned_users); - - $this->log->add('admin', $this->user->data['user_id'], $this->user->ip, $this->ban_log_string, false, [$data['reason'], $usernames_log]); - $this->log->add('mod', $this->user->data['user_id'], $this->user->ip, $this->ban_log_string, false, [ - 'forum_id' => 0, - 'topic_id' => 0, - $data['reason'], - $usernames_log, - ]); - - foreach ($this->banned_users as $user_id => $username) - { - $this->log->add('user', $this->user->data['user_id'], $this->user->ip, $this->ban_log_string, false, [ - 'reportee_id' => $user_id, - $data['reason'], - $usernames_log, - ]); - } - - return true; + $this->logout_affected_users($data['items']); + return $this->banned_users; } /** @@ -110,11 +51,6 @@ class user extends base */ public function after_unban(array $data) { - if (empty($data['logging'])) - { - return; - } - $user_ids = array_map('intval', $data['items']); $sql = 'SELECT user_id, username @@ -122,36 +58,14 @@ class user extends base WHERE ' . $this->db->sql_in_set('user_id', $user_ids); $result = $this->db->sql_query($sql); - $real_user_ids = []; - $usernames = []; + $unbanned_users = []; while ($row = $this->db->sql_fetchrow($result)) { - $real_user_ids[] = $row['user_id']; - $usernames[] = $row['username']; + $unbanned_users[(int) $row['user_id']] = $row['username']; } $this->db->sql_freeresult($result); - if (empty($usernames)) - { - return; - } - - $usernames_log = implode(', ', $usernames); - - $this->log->add('admin', $this->user->data['user_id'], $this->user->ip, $this->unban_log_string, false, [$usernames_log]); - $this->log->add('mod', $this->user->data['user_id'], $this->user->ip, $this->unban_log_string, false, [ - 'forum_id' => 0, - 'topic_id' => 0, - $usernames_log, - ]); - - foreach ($real_user_ids as $user_id) - { - $this->log->add('user', $this->user->data['user_id'], $this->user->ip, $this->unban_log_string, false, [ - 'reportee_id' => $user_id, - $usernames_log, - ]); - } + return $unbanned_users; } /**