diff --git a/adm/style/banhammer_body.html b/adm/style/banhammer_body.html index d41c5a8..ef0687d 100644 --- a/adm/style/banhammer_body.html +++ b/adm/style/banhammer_body.html @@ -2,10 +2,6 @@

{L_ACP_BH_SETTINGS}

- -
{L_SETTINGS_SAVED}
- -
diff --git a/config/services.yml b/config/services.yml index 1163fef..de8e65b 100644 --- a/config/services.yml +++ b/config/services.yml @@ -37,6 +37,8 @@ services: - '@language' - '@request' - '@user' + - '%core.root_path%' + - '%core.php_ext%' phpbbmodders.banhammer.cron.restriction_expiry: class: phpbbmodders\banhammer\cron\task\restriction_expiry arguments: diff --git a/controller/admin_controller.php b/controller/admin_controller.php index abcc457..7c7e4a0 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -126,6 +126,12 @@ public function display_options() */ protected function set_options() { + $move_group = $this->request->variable('move_group', 0); + $restrict_group = $this->request->variable('restrict_group', 0); + + $this->validate_group($move_group); + $this->validate_group($restrict_group); + $this->config->set('bh_ban_email', $this->request->variable('ban_email', 0)); $this->config->set('bh_ban_ip', $this->request->variable('ban_ip', 0)); $this->config->set('bh_del_avatar', $this->request->variable('del_avatar', 0)); @@ -133,13 +139,55 @@ protected function set_options() $this->config->set('bh_del_posts', $this->request->variable('del_posts', 0)); $this->config->set('bh_del_profile', $this->request->variable('del_profile', 0)); $this->config->set('bh_del_signature', $this->request->variable('del_signature', 0)); - $this->config->set('bh_group_id', $this->request->variable('move_group', 0)); - $this->config->set('bh_restrict_group_id', $this->request->variable('restrict_group', 0)); + $this->config->set('bh_group_id', $move_group); + $this->config->set('bh_restrict_group_id', $restrict_group); $this->config->set('bh_sfs_api_key', $this->request->variable('sfs_api_key', '', true)); $this->config->set('bh_sfs_allow_http', $this->request->variable('sfs_allow_http', 0)); $this->config->set('bh_ban_time', $this->request->variable('ban_time', 0)); } + /** + * Reject a submitted group id that isn't a valid choice: one of the + * special groups excluded from the dropdown (get_groups() only hides + * them client-side, a crafted submission can still send their id), or a + * founder-managed group the current admin isn't allowed to assign users + * into (phpBB's own acp_users.php enforces the same rule). + * + * @param int $group_id + * @return void + * @access private + */ + private function validate_group($group_id) + { + if (!$group_id) + { + return; + } + + $sql = 'SELECT group_name, group_founder_manage + FROM ' . GROUPS_TABLE . ' + WHERE group_id = ' . (int) $group_id; + $result = $this->db->sql_query($sql); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + if (!$row || in_array($row['group_name'], $this->ignored_groups(), true) || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage'])) + { + trigger_error($this->user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + } + + /** + * Special groups no admin should be moving/restricting users into. + * + * @return array + * @access private + */ + private function ignored_groups() + { + return array('BOTS', 'GUESTS', 'REGISTERED', 'REGISTERED_COPPA', 'NEWLY_REGISTERED', 'ADMINISTRATORS', 'GLOBAL_MODERATORS'); + } + /** * function to return groups that are allowed */ @@ -149,9 +197,9 @@ private function get_groups($group_selected) // Don't display any of the default groups // highly doubt an admin would want to ban someone into a default group - $ignore_groups = array('BOTS', 'GUESTS', 'REGISTERED', 'REGISTERED_COPPA', 'NEWLY_REGISTERED', 'ADMINISTRATORS', 'GLOBAL_MODERATORS'); + $ignore_groups = $this->ignored_groups(); - $sql = 'SELECT group_name, group_id, group_type + $sql = 'SELECT group_name, group_id, group_type, group_founder_manage FROM ' . GROUPS_TABLE . ' WHERE ' . $this->db->sql_in_set('group_name', $ignore_groups, true) . ' ORDER BY group_name ASC'; @@ -161,6 +209,13 @@ private function get_groups($group_selected) $s_group_options = ""; while ($row = $this->db->sql_fetchrow($result)) { + // Same rule as phpBB's own acp_users.php: don't offer a group a + // non-founder isn't allowed to manage. + if ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage']) + { + continue; + } + $selected = ($row['group_id'] == $group_selected) ? ' selected="selected"' : ''; $group_name = ($row['group_type'] == GROUP_SPECIAL) ? $this->user->lang['G_' . $row['group_name']] : $row['group_name']; $s_group_options .= ""; diff --git a/controller/ban_domain_controller.php b/controller/ban_domain_controller.php index 446f788..5315deb 100644 --- a/controller/ban_domain_controller.php +++ b/controller/ban_domain_controller.php @@ -35,6 +35,12 @@ class ban_domain_controller /** @var \phpbb\user */ protected $user; + /** @var string phpBB root path */ + protected $root_path; + + /** @var string phpEx */ + protected $php_ext; + /** * Constructor * @@ -42,19 +48,25 @@ class ban_domain_controller * @param \phpbb\language\language $language Language object * @param \phpbb\request\request $request Request object * @param \phpbb\user $user User object + * @param string $root_path phpBB root path + * @param string $php_ext PHP file extension * @access public */ public function __construct( \phpbb\auth\auth $auth, \phpbb\language\language $language, \phpbb\request\request $request, - \phpbb\user $user + \phpbb\user $user, + $root_path, + $php_ext ) { $this->auth = $auth; $this->language = $language; $this->request = $request; $this->user = $user; + $this->root_path = $root_path; + $this->php_ext = $php_ext; } /** @@ -93,6 +105,11 @@ public function handle() { $reason = $this->request->variable('bh_reason', '', true); + if (!function_exists('user_ban')) + { + include($this->root_path . 'includes/functions_user.' . $this->php_ext); + } + $success = user_ban('email', $ban_pattern, 0, '', false, $reason); if (!$success) diff --git a/cron/task/restriction_expiry.php b/cron/task/restriction_expiry.php index 3ed28e8..06a7733 100644 --- a/cron/task/restriction_expiry.php +++ b/cron/task/restriction_expiry.php @@ -67,7 +67,7 @@ public function run() { $this->config->set('bh_restrict_last_run', time(), false); - $sql = 'SELECT restrict_id, user_id, original_group_id + $sql = 'SELECT restrict_id, user_id, original_group_id, restrict_group_id FROM ' . $this->restrict_table . ' WHERE restrict_until > 0 AND restrict_until <= ' . time(); @@ -86,17 +86,20 @@ public function run() return; } - if (!function_exists('group_user_add') || !function_exists('group_user_del')) + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) { include($this->root_path . 'includes/functions_user.' . $this->php_ext); } - $restrict_group_id = (int) $this->config['bh_restrict_group_id']; - foreach ($expired as $row) { $user_id = (int) $row['user_id']; $original_group_id = (int) $row['original_group_id']; + // The group actually applied when this restriction was created, + // not the current ACP setting: an admin may have changed it + // since, and removing the wrong (or no) group would strand the + // user in whatever group they were actually restricted into. + $restrict_group_id = (int) $row['restrict_group_id']; if ($restrict_group_id) { @@ -105,7 +108,11 @@ public function run() if ($original_group_id) { - group_user_add($original_group_id, array($user_id), false, false, true); + // Not group_user_add(..., true): the user was never removed + // from their original group while restricted, so it would + // see them as already a member and return GROUP_USERS_EXIST + // before reaching the code that sets the default group. + group_user_attributes('default', $original_group_id, array($user_id)); } $this->db->sql_query('DELETE FROM ' . $this->restrict_table . ' WHERE restrict_id = ' . (int) $row['restrict_id']); diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index ae8a4c8..66f4533 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -133,10 +133,14 @@ public function add_mcp_queue_banhammer_link($event) if ($post_info['user_type'] != USER_FOUNDER && $target_user_id != $this->user->data['user_id'] && $target_user_id > 0) { + // Deliberately not 'bh' => 1: that shortcut jumps straight to the + // confirmation step with none of the ban options set (permanent, + // no email/IP ban, no deletions, no group move, no SFS report), + // silently ignoring the ACP-configured defaults. Link to the + // profile page instead, which shows the real options form. $params = array( 'mode' => 'viewprofile', 'u' => $target_user_id, - 'bh' => 1, ); $template_vars['S_MCP_SHOW_BANHAMMER'] = true; @@ -234,7 +238,7 @@ public function do_ban_hammer_stuff($event) } $this->template->assign_vars(array( - 'BH_STYLE' => (($bh_result == 'success') ? 'green' : '#a92c2c') . '; color: white;"', + 'BH_STYLE' => (($bh_result == 'success') ? 'green' : '#a92c2c') . '; color: white;', 'BH_MESSAGE' => $bh_message, )); } @@ -341,6 +345,11 @@ public function do_ban_hammer_stuff($event) $message .= ($hidden_fields['sfs_report'] && $curl_exists) ? $this->user->lang['BH_SUBMIT_SFS'] . '
' : ''; confirm_box(false, $message, build_hidden_fields($hidden_fields)); + + // confirm_box(false, ...) above only returns instead of exiting + // when the request was actually a cancellation (POST 'cancel'), + // in which case we must not fall through to the ban below. + return; } // We have a user to ban. @@ -538,6 +547,11 @@ public function do_restrict_stuff($event) $message .= ($length) ? '

' . $this->user->lang('BH_RESTRICT_FOR', $length) : '

' . $this->user->lang['BH_RESTRICT_PERM']; confirm_box(false, $message, $hidden_fields); + + // confirm_box(false, ...) above only returns instead of exiting + // when the request was actually a cancellation (POST 'cancel'), + // in which case we must not fall through to the restriction below. + return; } if (!function_exists('group_user_add')) @@ -554,6 +568,7 @@ public function do_restrict_stuff($event) $sql_ary = array( 'user_id' => $user_id, 'original_group_id' => $original_group_id, + 'restrict_group_id' => $restrict_group_id, 'restrict_until' => $restrict_until, ); $sql = 'INSERT INTO ' . $this->restrict_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); @@ -605,6 +620,15 @@ public function undo_bh_group($event) if ($group_id) { + // The ban and restrict groups can be configured to be the + // same group. A restricted (not banned) user deliberately + // sits in it, so leave their membership alone while the + // restriction is still active instead of undoing it here. + if ((int) $this->config['bh_restrict_group_id'] === (int) $this->config['bh_group_id'] && $this->active_restriction($this->user->data['user_id']) !== null) + { + return; + } + // Remove the user from the banned group set in the ACP if (!function_exists('group_user_del')) { @@ -619,31 +643,22 @@ private function bh_del_privmsgs() { $user_id = $this->user_id; - // Get private messages - $sql = 'SELECT msg_id, author_id FROM ' . PRIVMSGS_TABLE . " - WHERE author_id = $user_id"; - $result = $this->db->sql_query($sql); - - $privmsgs_ary = array(); - while ($row = $this->db->sql_fetchrow($result)) + // phpBB's own bulk PM cleanup (used when deleting a user account + // entirely): correctly adjusts recipients' unread/new counts, + // removes attachments and notifications, and anonymizes already- + // delivered sent messages instead of deleting them out from under + // their recipients. A hand-rolled DELETE here previously left all + // of that bookkeeping inconsistent. + if (!function_exists('phpbb_delete_users_pms')) { - $privmsgs_ary[] = $row['msg_id']; + include($this->root_path . 'includes/functions_privmsgs.' . $this->php_ext); } - $this->db->sql_freeresult($result); + phpbb_delete_users_pms(array($user_id)); - if (!empty($privmsgs_ary)) - { - // And now close eventual reports. - $sql = 'UPDATE ' . REPORTS_TABLE . ' - SET report_closed = 1 - WHERE ' . $this->db->sql_in_set('pm_id', $privmsgs_ary); - $this->db->sql_query($sql); - } - - $this->db->sql_query('DELETE FROM ' . PRIVMSGS_TABLE . " WHERE author_id = $user_id"); + // The account itself isn't deleted, only its own folder structure + // and rules, which don't affect any other user. $this->db->sql_query('DELETE FROM ' . PRIVMSGS_FOLDER_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . PRIVMSGS_RULES_TABLE . " WHERE user_id = $user_id"); - $this->db->sql_query('DELETE FROM ' . PRIVMSGS_TO_TABLE . " WHERE user_id = $user_id OR author_id = $user_id"); } private function bh_del_posts() @@ -672,6 +687,24 @@ private function bh_del_posts() } $this->db->sql_freeresult($result); + // m_ban alone doesn't grant delete rights in every forum; only the + // extension's own explicit permission does. Without it, only touch + // posts in forums the acting moderator could delete in anyway. + if (!$this->auth->acl_get('m_banhammer_del_posts_all')) + { + foreach ($posts as $post_id => $post_row) + { + if (!$this->auth->acl_get('m_delete', (int) $post_row['forum_id'])) + { + // Only gates $posts: the report-closing loop below only + // ever reads $topics through a $posts[$post_id] lookup, + // so leaving a stale count here for a topic we're no + // longer touching has no effect. + unset($posts[$post_id]); + } + } + } + // And now handle the reports. $sql = 'SELECT report_id, post_id, report_closed FROM ' . REPORTS_TABLE . ' @@ -729,7 +762,10 @@ private function bh_del_posts() $this->db->sql_query('DELETE FROM ' . FORUMS_WATCH_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . MODERATOR_CACHE_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . NOTIFICATIONS_TABLE . " WHERE user_id = $user_id"); - $this->db->sql_query('DELETE FROM ' . POLL_VOTES_TABLE . " WHERE vote_user_id = $user_id"); + // Poll votes are deliberately left alone, same as phpBB's own + // user_delete() (which doesn't touch POLL_VOTES_TABLE either): + // removing them here without decrementing poll_option_total would + // corrupt the poll's totals and let the user vote again later. $this->db->sql_query('DELETE FROM ' . TOPICS_POSTED_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . TOPICS_TRACK_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . TOPICS_WATCH_TABLE . " WHERE user_id = $user_id"); @@ -744,12 +780,14 @@ private function get_file($url) curl_setopt($ch, CURLOPT_URL, $url); curl_setopt($ch, CURLOPT_TIMEOUT, 5); curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 5); - curl_exec($ch); + $response = curl_exec($ch); $httpcode = curl_getinfo($ch, CURLINFO_HTTP_CODE); curl_close($ch); - // if nothing is returned (SFS is down) - if ($httpcode != 200) + // curl_exec() returns false on a transport failure (e.g. the + // connection dropped after headers were already sent), which the + // HTTP code alone would not catch. + if ($response === false || $httpcode != 200) { return false; } diff --git a/language/en/permissions_banhammer.php b/language/en/permissions_banhammer.php new file mode 100644 index 0000000..9ecfebc --- /dev/null +++ b/language/en/permissions_banhammer.php @@ -0,0 +1,29 @@ + 'Can delete a banned user\'s posts in every forum via Ban Hammer, not just forums they can otherwise delete posts in', +)); diff --git a/migrations/permission_del_posts_all.php b/migrations/permission_del_posts_all.php new file mode 100644 index 0000000..7961276 --- /dev/null +++ b/migrations/permission_del_posts_all.php @@ -0,0 +1,53 @@ +db->sql_query($sql); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + return (bool) $row; + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_group_id_column'); + } + + public function update_data() + { + return array( + array('permission.add', array('m_banhammer_del_posts_all', true, 'm_ban')), + ); + } + + public function revert_data() + { + return array( + array('permission.remove', array('m_banhammer_del_posts_all', true)), + ); + } +} diff --git a/migrations/restrict_group_id_column.php b/migrations/restrict_group_id_column.php new file mode 100644 index 0000000..f1d3c2e --- /dev/null +++ b/migrations/restrict_group_id_column.php @@ -0,0 +1,130 @@ +db_tools->sql_column_exists($this->table_prefix . 'banhammer_restrict', 'restrict_group_id'); + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_group'); + } + + public function update_schema() + { + return array( + 'add_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'restrict_group_id' => array('UINT', 0), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'restrict_group_id', + ), + ), + ); + } + + public function update_data() + { + return array( + // Rows from before this column existed predate per-row + // tracking; the current ACP setting is the best available + // guess at which group they actually used. + array('custom', array(array($this, 'backfill_restrict_group_id'))), + ); + } + + public function revert_data() + { + return array( + // A purge is about to drop this tracking (this column, then the + // whole table once restrict_group.php itself reverts next). + // Restore anyone still actively restricted now, while the data + // needed to do it correctly is still here, rather than + // stranding them mid-restriction with no way back. + array('custom', array(array($this, 'restore_active_restrictions'))), + ); + } + + /** + * @return void + * @access public + */ + public function backfill_restrict_group_id() + { + $restrict_group_id = (int) $this->config['bh_restrict_group_id']; + + if (!$restrict_group_id) + { + return; + } + + $sql = 'UPDATE ' . $this->table_prefix . 'banhammer_restrict + SET restrict_group_id = ' . $restrict_group_id . ' + WHERE restrict_group_id = 0'; + $this->sql_query($sql); + } + + /** + * @return void + * @access public + */ + public function restore_active_restrictions() + { + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) + { + include($this->phpbb_root_path . 'includes/functions_user.' . $this->php_ext); + } + + $sql = 'SELECT user_id, original_group_id, restrict_group_id + FROM ' . $this->table_prefix . 'banhammer_restrict'; + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + $user_id = (int) $row['user_id']; + $restrict_group_id = (int) $row['restrict_group_id']; + $original_group_id = (int) $row['original_group_id']; + + if ($restrict_group_id) + { + group_user_del($restrict_group_id, array($user_id)); + } + + if ($original_group_id) + { + group_user_attributes('default', $original_group_id, array($user_id)); + } + } + $this->db->sql_freeresult($result); + + $this->sql_query('DELETE FROM ' . $this->table_prefix . 'banhammer_restrict'); + } +}