diff --git a/.gitignore b/.gitignore index 14ff796..5ce93d4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ vendor output*.txt +*-review-*.md diff --git a/config/services.yml b/config/services.yml index de8e65b..3545d57 100644 --- a/config/services.yml +++ b/config/services.yml @@ -14,6 +14,7 @@ services: - '%core.php_ext%' - '@service_container' - '%core.table_prefix%banhammer_restrict' + - '%core.table_prefix%banhammer_ban_group' tags: - { name: event.listener } phpbbmodders.banhammer.admin.controller: diff --git a/controller/admin_controller.php b/controller/admin_controller.php index 7c7e4a0..955659e 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -109,7 +109,7 @@ public function display_options() 'DEL_PROFILE' => (!empty($this->config['bh_del_profile'])) ? true : false, 'DEL_SIGNATURE' => (!empty($this->config['bh_del_signature'])) ? true : false, 'MOVE_GROUP' => $this->get_groups($this->request->variable('move_group', $this->config['bh_group_id'])), - 'RESTRICT_GROUP' => $this->get_groups($this->request->variable('restrict_group', $this->config['bh_restrict_group_id'])), + 'RESTRICT_GROUP' => $this->get_groups($this->request->variable('restrict_group', $this->config['bh_restrict_group_id']), true), 'SFS_ALLOW_HTTP' => (!empty($this->config['bh_sfs_allow_http'])) ? true : false, 'SFS_API_KEY' => (!empty($this->config['bh_sfs_api_key'])) ? $this->config['bh_sfs_api_key'] : '', 'SFS_CURL' => (function_exists('curl_init')) ? true : false, @@ -130,7 +130,11 @@ protected function set_options() $restrict_group = $this->request->variable('restrict_group', 0); $this->validate_group($move_group); - $this->validate_group($restrict_group); + // Open/Free groups let a member resign from them unilaterally via + // UCP (see includes/ucp/ucp_groups.php), which would let a + // restricted user simply leave the restriction. A banned user + // can't log in to do the same, so this only applies here. + $this->validate_group($restrict_group, true); $this->config->set('bh_ban_email', $this->request->variable('ban_email', 0)); $this->config->set('bh_ban_ip', $this->request->variable('ban_ip', 0)); @@ -141,37 +145,51 @@ protected function set_options() $this->config->set('bh_del_signature', $this->request->variable('del_signature', 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)); + + if (function_exists('curl_init')) + { + // Both fields are hidden from the form when cURL isn't + // available (see the template), so they're absent from the + // submitted data - writing them unconditionally would silently + // blank the stored SFS key on any unrelated settings save. + $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 + * them client-side, a crafted submission can still send their id), a * founder-managed group the current admin isn't allowed to assign users - * into (phpBB's own acp_users.php enforces the same rule). + * into (phpBB's own acp_users.php enforces the same rule), or - when + * $reject_self_service is set - an Open/Free group. * * @param int $group_id + * @param bool $reject_self_service * @return void * @access private */ - private function validate_group($group_id) + private function validate_group($group_id, $reject_self_service = false) { if (!$group_id) { return; } - $sql = 'SELECT group_name, group_founder_manage + $sql = 'SELECT group_name, group_type, 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'])) + if (!$row + || in_array($row['group_name'], $this->ignored_groups(), true) + || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage']) + || ($reject_self_service && ($row['group_type'] == GROUP_OPEN || $row['group_type'] == GROUP_FREE))) { trigger_error($this->user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); } @@ -190,8 +208,14 @@ private function ignored_groups() /** * function to return groups that are allowed + * + * @param int $group_selected + * @param bool $reject_self_service Hide Open/Free groups too (see + * validate_group()); used for the + * restrict-group dropdown, not the + * ban move-to-group one. */ - private function get_groups($group_selected) + private function get_groups($group_selected, $reject_self_service = false) { $this->user->add_lang('acp/groups'); @@ -216,6 +240,13 @@ private function get_groups($group_selected) continue; } + // Open/Free groups let a member resign unilaterally via UCP, + // which would let a restricted user just leave the restriction. + if ($reject_self_service && ($row['group_type'] == GROUP_OPEN || $row['group_type'] == GROUP_FREE)) + { + 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/cron/task/restriction_expiry.php b/cron/task/restriction_expiry.php index 06a7733..bc0124d 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, restrict_group_id + $sql = 'SELECT restrict_id, user_id, original_group_id, restrict_group_id, restrict_new_membership FROM ' . $this->restrict_table . ' WHERE restrict_until > 0 AND restrict_until <= ' . time(); @@ -101,7 +101,13 @@ public function run() // user in whatever group they were actually restricted into. $restrict_group_id = (int) $row['restrict_group_id']; - if ($restrict_group_id) + // Only remove group membership the restriction itself created. + // If the user already belonged to the restrict group for an + // unrelated reason (do_restrict_stuff() found GROUP_USERS_EXIST + // and recorded restrict_new_membership = 0), that membership + // predates this restriction and isn't the restriction's to take + // away - only the default-group change and the tracking row are. + if ($restrict_group_id && $row['restrict_new_membership']) { group_user_del($restrict_group_id, array($user_id)); } diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index d02d93b..547194c 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -67,6 +67,9 @@ class banhammer_listener implements EventSubscriberInterface /** @var string */ protected $restrict_table; + /** @var string */ + protected $ban_group_table; + public function __construct( \phpbb\auth\auth $auth, \phpbb\cache\driver\driver_interface $cache, @@ -79,7 +82,8 @@ public function __construct( $root_path, $phpExt, ContainerInterface $container, - $restrict_table + $restrict_table, + $ban_group_table ) { $this->auth = $auth; @@ -94,6 +98,7 @@ public function __construct( $this->php_ext = $phpExt; $this->container = $container; $this->restrict_table = $restrict_table; + $this->ban_group_table = $ban_group_table; } static public function getSubscribedEvents() @@ -105,9 +110,28 @@ static public function getSubscribedEvents() ), 'core.session_set_custom_ban' => 'undo_bh_group', 'core.mcp_queue_approve_details_template' => 'add_mcp_queue_banhammer_link', + 'core.permissions' => 'add_permission', )); } + /** + * Register m_banhammer_del_posts_all with phpBB's permission system. + * Without this, \phpbb\permissions::permission_defined() never + * recognises it, and the ACP permission editor filters it out of + * every screen - the migration-granted permission would work but be + * invisible and unrevokable through the normal UI. + * + * @param \phpbb\event\data $event The event object + * @return void + * @access public + */ + public function add_permission($event) + { + $permissions = $event['permissions']; + $permissions['m_banhammer_del_posts_all'] = array('lang' => 'ACL_M_BANHAMMER_DEL_POSTS_ALL', 'cat' => 'misc'); + $event['permissions'] = $permissions; + } + /** * Add a Ban Hammer link to the MCP post-approval detail page (mcp_post.html), * so a moderator can ban the poster without leaving the approval workflow to @@ -257,19 +281,14 @@ public function do_ban_hammer_stuff($event) return; } - if ($this->config['bh_group_id']) - { - // Get group name for banned users, if any. - $sql = 'SELECT group_id, group_name FROM ' . GROUPS_TABLE . ' - WHERE group_id = ' . (int) $this->config['bh_group_id']; - $result = $this->db->sql_query($sql); - $group_name = $this->db->sql_fetchfield('group_name'); - $this->db->sql_freeresult($result); + // Re-validated here, not just trusted from when it was saved in the + // ACP: the group may have been deleted, or made founder-managed, + // since then. + $group_name = $this->safe_group_name($this->config['bh_group_id']); - if (empty($group_name)) - { - $this->config['bh_group_id'] = 0; - } + if ($group_name === '') + { + $this->config['bh_group_id'] = 0; } if (!$this->request->is_set('bh') || ($this->request->is_set('bh') && $this->request->is_set('confirm_key') && !confirm_box(true))) @@ -429,12 +448,47 @@ public function do_ban_hammer_stuff($event) if ($this->request->variable('move_group', 0) && !empty($group_name)) { - $return = group_user_add($this->config['bh_group_id'], array($this->user_id), array($this->data['username']), $group_name, true); + $move_group_id = (int) $this->config['bh_group_id']; + $return = group_user_add($move_group_id, array($this->user_id), array($this->data['username']), $group_name, true); + $move_new_membership = 1; + + if ($return === 'GROUP_USERS_EXIST') + { + // Already a member for some unrelated reason: group_user_add() + // returns before setting the default group in that case, so + // set it directly instead (same fix as do_restrict_stuff()'s + // equivalent case). + $return = group_user_attributes('default', $move_group_id, array($this->user_id)); + $move_new_membership = 0; + } - if ($return != false) + if ($return !== false) { $error[] = 'ERROR_MOVE_GROUP'; } + else + { + // Record which group this ban actually moved them into (and + // whether that membership was newly created or pre-existing), + // and their default group beforehand, so undo_bh_group() can + // clean up precisely this action later instead of comparing + // against the *current* ACP setting. + $sql_ary = array( + 'user_id' => $this->user_id, + 'original_group_id' => (int) $this->data['group_id'], + 'move_group_id' => $move_group_id, + 'move_new_membership' => $move_new_membership, + ); + $sql = 'INSERT INTO ' . $this->ban_group_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); + + // A second ban+move on the same already-banned user is + // blocked well before this point (see the banned-user check + // above), but guard the unique index anyway rather than let + // a genuine race surface as an uncaught SQL error. + $this->db->sql_return_on_error(true); + $this->db->sql_query($sql); + $this->db->sql_return_on_error(false); + } } if ($this->request->variable('sfs_report', 0) && !empty($this->config['bh_sfs_api_key']) && $curl_exists) @@ -492,6 +546,15 @@ public function do_restrict_stuff($event) $user_id = (int) $data['user_id']; $restrict_group_id = (int) $this->config['bh_restrict_group_id']; + // Re-validated here, not just trusted from ACP-save time: the group + // may have been deleted, made founder-managed, or turned into an + // Open/Free group (which would let the restricted user just resign + // via UCP - see includes/ucp/ucp_groups.php) since then. + if ($restrict_group_id && $this->safe_group_name($restrict_group_id, true) === '') + { + $restrict_group_id = 0; + } + if (!$this->auth->acl_get('m_ban') || $data['user_type'] == USER_FOUNDER || $user_id == $this->user->data['user_id'] || !$restrict_group_id) { // Nothing to see here, move on. No group configured in the ACP @@ -560,7 +623,7 @@ public function do_restrict_stuff($event) return; } - if (!function_exists('group_user_add')) + if (!function_exists('group_user_add') || !function_exists('group_user_attributes')) { include($this->root_path . 'includes/functions_user.' . $this->php_ext); } @@ -576,11 +639,67 @@ public function do_restrict_stuff($event) 'original_group_id' => $original_group_id, 'restrict_group_id' => $restrict_group_id, 'restrict_until' => $restrict_until, + // Corrected below to 0 if group_user_add() finds the user + // already a member; default of 1 matches the common case. + 'restrict_new_membership' => 1, ); $sql = 'INSERT INTO ' . $this->restrict_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); + + // Two moderators confirming a restriction on the same user at + // nearly the same moment could both pass the active_restriction() + // check above before either commits. The unique index on user_id + // (see the restrict_unique_user migration) turns the loser's + // insert into a caught error here instead of a second, silently + // conflicting tracking row. + $this->db->sql_return_on_error(true); $this->db->sql_query($sql); + $insert_failed = (bool) $this->db->get_sql_error_triggered(); + $this->db->sql_return_on_error(false); + + if ($insert_failed) + { + $this->template->assign_var('RESTRICT_MESSAGE', $this->user->lang['BH_ALREADY_RESTRICTED']); + + return; + } + + $result = group_user_add($restrict_group_id, array($user_id), false, false, true); + $group_action_failed = ($result !== false); + + if ($result === 'GROUP_USERS_EXIST') + { + // Already a member for some unrelated reason: group_user_add() + // returns before setting the default group in that case (see + // the same fix in restriction_expiry's restore path), so set it + // directly instead - but only approved members can be made a + // default group (see group_user_attributes()'s 'default' case); + // a pending join request returns NO_USERS and changes nothing, + // which is still a failure to actually apply the restriction. + $group_action_failed = (group_user_attributes('default', $restrict_group_id, array($user_id)) !== false); - group_user_add($restrict_group_id, array($user_id), false, false, true); + if (!$group_action_failed) + { + // The membership predates this restriction; don't let + // expiry/purge remove it later on this restriction's + // account - only the default-group change and the + // tracking row itself belong to it. + $this->db->sql_query('UPDATE ' . $this->restrict_table . ' + SET restrict_new_membership = 0 + WHERE user_id = ' . $user_id); + } + } + + if ($group_action_failed) + { + // The group action never took effect - pending membership, + // or NO_USER/GROUP_USERS_INVALID (shouldn't happen; $user_id + // comes from the profile this event fired for). Don't leave a + // tracking row behind for a restriction that isn't actually in + // place. + $this->db->sql_query('DELETE FROM ' . $this->restrict_table . ' WHERE user_id = ' . $user_id); + + return; + } $args = array( 'mode' => 'viewprofile', @@ -601,7 +720,7 @@ public function do_restrict_stuff($event) */ protected function active_restriction($user_id) { - $sql = 'SELECT restrict_id + $sql = 'SELECT restrict_id, restrict_group_id FROM ' . $this->restrict_table . ' WHERE user_id = ' . (int) $user_id; $result = $this->db->sql_query_limit($sql, 1); @@ -611,38 +730,115 @@ protected function active_restriction($user_id) return ($row) ?: null; } - // Once a ban is cleared try and remove the user from the banned group set in the ACP of the extension + /** + * The group-move tracking row for a user's currently active ban, if any. + * + * @param int $user_id + * @return array|null The tracking row, or null when there is none. + * @access protected + */ + protected function active_ban_group($user_id) + { + $sql = 'SELECT ban_id, original_group_id, move_group_id, move_new_membership + FROM ' . $this->ban_group_table . ' + WHERE user_id = ' . (int) $user_id; + $result = $this->db->sql_query_limit($sql, 1); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + return ($row) ?: null; + } + + /** + * A configured move/restrict group's name, re-validated at the point + * it's about to be used rather than trusted from ACP-save time: the + * group may have been deleted, made founder-managed, or (when + * $reject_self_service is set) turned into an Open/Free group, since + * then. + * + * @param int $group_id + * @param bool $reject_self_service + * @return string Group name, or '' if unset, gone, founder-managed and + * the acting moderator isn't the founder, or (when + * $reject_self_service is set) Open/Free. + * @access protected + */ + protected function safe_group_name($group_id, $reject_self_service = false) + { + if (!$group_id) + { + return ''; + } + + $sql = 'SELECT group_name, group_type, 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 + || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage']) + || ($reject_self_service && ($row['group_type'] == GROUP_OPEN || $row['group_type'] == GROUP_FREE))) + { + return ''; + } + + return $row['group_name']; + } + + /** + * Once a ban is cleared, undo whatever group move that specific ban + * actually made - not "the group currently configured in the ACP", + * which may have changed since, or matter to some other, unrelated + * group membership entirely. + * + * @param \phpbb\event\data $event The event object + * @return void + * @access public + */ public function undo_bh_group($event) { - if (!empty($this->config['bh_group_id']) && !$event['banned'] && $this->user->data['user_type'] != USER_IGNORE) + if ($event['banned'] || $this->user->data['user_type'] == USER_IGNORE) { - // determine if the user is in the ban hammer group set in the ACP - $sql = 'SELECT group_id FROM ' . USER_GROUP_TABLE . ' - WHERE group_id = ' . (int) $this->config['bh_group_id'] . ' - AND user_id = ' . (int) $this->user->data['user_id']; - $result = $this->db->sql_query($sql); - $group_id = $this->db->sql_fetchfield('group_id'); - $this->db->sql_freeresult($result); + return; + } - 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; - } + $ban_group = $this->active_ban_group($this->user->data['user_id']); - // Remove the user from the banned group set in the ACP - if (!function_exists('group_user_del')) - { - include($this->root_path . 'includes/functions_user.' . $this->php_ext); - } - group_user_del($this->config['bh_group_id'], array($this->user->data['user_id'])); + if ($ban_group === null) + { + return; + } + + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) + { + include($this->root_path . 'includes/functions_user.' . $this->php_ext); + } + + $move_group_id = (int) $ban_group['move_group_id']; + + if ($move_group_id && $ban_group['move_new_membership']) + { + // A restriction can be configured to use the same group as a + // ban's move-to group. If this user also has an active + // restriction recorded against this exact group, leave their + // membership alone - it's still needed for the restriction, + // checked against its own recorded group, not current config. + $restriction = $this->active_restriction($this->user->data['user_id']); + + if ($restriction === null || (int) $restriction['restrict_group_id'] !== $move_group_id) + { + group_user_del($move_group_id, array($this->user->data['user_id'])); } } + + if ($ban_group['original_group_id']) + { + group_user_attributes('default', (int) $ban_group['original_group_id'], array($this->user->data['user_id'])); + } + + $this->db->sql_query('DELETE FROM ' . $this->ban_group_table . ' WHERE ban_id = ' . (int) $ban_group['ban_id']); } private function bh_del_privmsgs() @@ -711,6 +907,19 @@ private function bh_del_posts() } } + if (empty($posts)) + { + // No posts to delete - either there were none to begin with, or + // permission filtered out all of them. Either way, stop here + // rather than still wiping unrelated account data (bookmarks, + // drafts, notifications, ...) below for a del_posts request + // that has nothing to actually act on, regardless of which + // permission granted access. A moderator whose authority + // covers at least one of the target's posts still gets the + // full account cleanup, same as always. + return; + } + // And now handle the reports. $sql = 'SELECT report_id, post_id, report_closed FROM ' . REPORTS_TABLE . ' @@ -772,7 +981,12 @@ private function bh_del_posts() // 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"); + // TOPICS_POSTED_TABLE is also deliberately left alone: delete_posts() + // above already calls update_posted_info() to rebuild it correctly + // for the topics it actually touched. A blanket delete here would + // also wipe the "posted in this topic" marker for topics where the + // user's post survived (not deletable in this pass), which + // delete_posts() never touched and has no reason to be wrong. $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"); $this->db->sql_query('DELETE FROM ' . USER_NOTIFICATIONS_TABLE . " WHERE user_id = $user_id"); diff --git a/migrations/ban_group.php b/migrations/ban_group.php new file mode 100644 index 0000000..cb03918 --- /dev/null +++ b/migrations/ban_group.php @@ -0,0 +1,173 @@ +db_tools->sql_table_exists($this->table_prefix . 'banhammer_ban_group'); + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_membership_column'); + } + + public function update_schema() + { + return array( + 'add_tables' => array( + $this->table_prefix . 'banhammer_ban_group' => array( + 'COLUMNS' => array( + 'ban_id' => array('UINT', null, 'auto_increment'), + 'user_id' => array('UINT', 0), + 'original_group_id' => array('UINT', 0), + 'move_group_id' => array('UINT', 0), + 'move_new_membership' => array('BOOL', 0), + ), + 'PRIMARY_KEY' => 'ban_id', + 'KEYS' => array( + 'user_id' => array('UNIQUE', 'user_id'), + ), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_tables' => array( + $this->table_prefix . 'banhammer_ban_group', + ), + ); + } + + public function update_data() + { + return array( + // A user already banned-and-moved under the old code has no + // tracking row (the table didn't exist yet), so undo_bh_group() + // would never clean them up once unbanned. Back-fill one for + // anyone currently banned and a member of the currently + // configured move-to group. + array('custom', array(array($this, 'backfill_active_bans'))), + ); + } + + /** + * @return void + * @access public + */ + public function backfill_active_bans() + { + $move_group_id = (int) $this->config['bh_group_id']; + + if (!$move_group_id) + { + return; + } + + // There's no record of what these users' default group was before + // being banned (original_group_id = 0, so undo_bh_group() won't + // attempt to restore one), and no way to tell whether the ban + // itself put them in this group or they already belonged for an + // unrelated reason - conservatively assume the latter + // (move_new_membership = 0), same reasoning as + // restrict_membership_column's own legacy default: leaving a + // genuinely ban-created membership in place is a much smaller + // problem than stripping one the ban never granted. + $sql = 'SELECT DISTINCT ug.user_id + FROM ' . USER_GROUP_TABLE . ' ug, ' . BANLIST_TABLE . ' b + WHERE ug.group_id = ' . $move_group_id . ' + AND b.ban_userid = ug.user_id + AND b.ban_exclude = 0 + AND (b.ban_end = 0 OR b.ban_end > ' . time() . ')'; + $result = $this->db->sql_query($sql); + + $sql_ary = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $sql_ary[] = array( + 'user_id' => (int) $row['user_id'], + 'original_group_id' => 0, + 'move_group_id' => $move_group_id, + 'move_new_membership' => 0, + ); + } + $this->db->sql_freeresult($result); + + if (!empty($sql_ary)) + { + $this->db->sql_multi_insert($this->table_prefix . 'banhammer_ban_group', $sql_ary); + } + } + + public function revert_data() + { + return array( + // A purge is about to drop this tracking. Restore anyone still + // actively tracked now, rather than stranding them in whatever + // group a ban moved them into with no way back. + array('custom', array(array($this, 'restore_active_ban_groups'))), + ); + } + + /** + * @return void + * @access public + */ + public function restore_active_ban_groups() + { + 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, move_group_id, move_new_membership + FROM ' . $this->table_prefix . 'banhammer_ban_group'; + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + $user_id = (int) $row['user_id']; + $move_group_id = (int) $row['move_group_id']; + $original_group_id = (int) $row['original_group_id']; + + if ($move_group_id && $row['move_new_membership']) + { + group_user_del($move_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_ban_group'); + } +} diff --git a/migrations/restrict_dedupe.php b/migrations/restrict_dedupe.php new file mode 100644 index 0000000..b0b9876 --- /dev/null +++ b/migrations/restrict_dedupe.php @@ -0,0 +1,64 @@ +table_prefix . 'banhammer_restrict + GROUP BY user_id + HAVING COUNT(*) > 1'; + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + $sql = 'DELETE FROM ' . $this->table_prefix . 'banhammer_restrict + WHERE user_id = ' . (int) $row['user_id'] . ' + AND restrict_id <> ' . (int) $row['keep_id']; + $this->sql_query($sql); + } + $this->db->sql_freeresult($result); + } +} diff --git a/migrations/restrict_group_id_column.php b/migrations/restrict_group_id_column.php index f1d3c2e..843da2d 100644 --- a/migrations/restrict_group_id_column.php +++ b/migrations/restrict_group_id_column.php @@ -64,12 +64,18 @@ public function update_data() 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'))), + // Normally a no-op: restrict_membership_column's own + // revert_data() already restores every active restriction and + // empties this table before this migration's own revert runs + // (migrations revert newest-first). This is only a safety net + // for a purge that happens before restrict_membership_column + // was ever installed - e.g. the extension's files were + // upgraded to include it, then the extension was disabled and + // purged without being re-enabled first to actually run it. + // phpBB's migrator only reverts migrations recorded as + // installed, so a never-installed migration's revert_data() + // simply never runs. + array('custom', array(array($this, 'restore_active_restrictions_fallback'))), ); } @@ -96,14 +102,19 @@ public function backfill_restrict_group_id() * @return void * @access public */ - public function restore_active_restrictions() + public function restore_active_restrictions_fallback() { 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 + // restrict_new_membership may or may not exist depending on + // whether restrict_membership_column ran - see revert_data() above. + $has_membership_column = $this->db_tools->sql_column_exists($this->table_prefix . 'banhammer_restrict', 'restrict_new_membership'); + + $sql = 'SELECT user_id, original_group_id, restrict_group_id' + . ($has_membership_column ? ', restrict_new_membership' : '') . ' FROM ' . $this->table_prefix . 'banhammer_restrict'; $result = $this->db->sql_query($sql); @@ -113,7 +124,12 @@ public function restore_active_restrictions() $restrict_group_id = (int) $row['restrict_group_id']; $original_group_id = (int) $row['original_group_id']; - if ($restrict_group_id) + // Without restrict_new_membership there's no way to tell + // whether this restriction created the membership or found it + // pre-existing; conservatively leave it alone rather than risk + // stripping a membership this restriction never granted (same + // reasoning as restrict_membership_column's own default). + if ($restrict_group_id && $has_membership_column && $row['restrict_new_membership']) { group_user_del($restrict_group_id, array($user_id)); } diff --git a/migrations/restrict_membership_column.php b/migrations/restrict_membership_column.php new file mode 100644 index 0000000..1f9e17f --- /dev/null +++ b/migrations/restrict_membership_column.php @@ -0,0 +1,119 @@ +db_tools->sql_column_exists($this->table_prefix . 'banhammer_restrict', 'restrict_new_membership'); + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_unique_user'); + } + + public function update_schema() + { + return array( + 'add_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + // Existing rows predate this column. Before this fix's + // own group_user_add() result check existed, the + // tracking row was inserted unconditionally regardless + // of whether the user was already a member - so a + // legacy row could represent either case, and there's + // no way to tell which after the fact. Default to 0 + // (don't remove membership): leaving a genuinely + // ban-hammer-created membership in place a little too + // long is a much smaller problem than stripping a + // membership this restriction never granted. + 'restrict_new_membership' => array('BOOL', 0), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'restrict_new_membership', + ), + ), + ); + } + + public function revert_data() + { + return array( + // A purge is about to drop this tracking (this column, then the + // whole table once earlier migrations revert next - migrations + // revert newest-first, so this is the last point the full, + // per-row data is still available). Restore anyone still + // actively restricted now, rather than stranding them + // mid-restriction with no way back. This lives here rather than + // in the older restrict_group_id_column migration because that + // one reverts after this one, by which point + // restrict_new_membership would already be gone. + array('custom', array(array($this, 'restore_active_restrictions'))), + ); + } + + /** + * @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, restrict_new_membership + 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']; + + // Only remove membership this restriction actually created - + // see restrict_new_membership's own docs above. + if ($restrict_group_id && $row['restrict_new_membership']) + { + 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'); + } +} diff --git a/migrations/restrict_unique_user.php b/migrations/restrict_unique_user.php new file mode 100644 index 0000000..da6d79b --- /dev/null +++ b/migrations/restrict_unique_user.php @@ -0,0 +1,63 @@ +db_tools->sql_unique_index_exists($this->table_prefix . 'banhammer_restrict', 'user_id'); + } + + static public function depends_on() + { + // restrict_dedupe removes any leftover duplicate user_id rows from + // the concurrent-restriction race this index closes off; without + // running first, creating a unique index over pre-existing + // duplicates would fail outright. + return array('\phpbbmodders\banhammer\migrations\restrict_dedupe'); + } + + public function update_schema() + { + return array( + 'drop_keys' => array( + $this->table_prefix . 'banhammer_restrict' => array('user_id'), + ), + 'add_unique_index' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'user_id' => array('user_id'), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_keys' => array( + $this->table_prefix . 'banhammer_restrict' => array('user_id'), + ), + 'add_index' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'user_id' => array('user_id'), + ), + ), + ); + } +} diff --git a/tests/cron/fixtures/restriction_expiry.xml b/tests/cron/fixtures/restriction_expiry.xml index 217d8b2..de28762 100644 --- a/tests/cron/fixtures/restriction_expiry.xml +++ b/tests/cron/fixtures/restriction_expiry.xml @@ -10,6 +10,14 @@ original group, 7. - user 3: restriction NOT expired (restrict_until far in the future) - must be left completely alone. + - user 4: restriction expired, but user 4 was already a member of + the restrict group (8) before being restricted + (restrict_new_membership = 0) - that membership must be kept; + only the default group is restored (to 7) and the tracking row + removed. + + Users 2 and 3 were added to their restrict group by the + restriction itself, so restrict_new_membership = 1. --> group_id @@ -95,6 +103,16 @@ + + 4 + 8 + + 0 + existing member user + existingmemberuser + + +
user_id @@ -125,6 +143,18 @@ 00 + + 4 + 7 + 0 + 0 + + + 4 + 8 + 0 + 0 +
restrict_id @@ -132,12 +162,14 @@ original_group_idrestrict_group_idrestrict_until + restrict_new_membership 1 2 7 8 1 + 1 2 @@ -145,6 +177,15 @@ 10 9 2000000000 + 1 + + + 3 + 4 + 7 + 8 + 1 + 0
diff --git a/tests/cron/restriction_expiry_test.php b/tests/cron/restriction_expiry_test.php index 520e7b5..f4125a6 100644 --- a/tests/cron/restriction_expiry_test.php +++ b/tests/cron/restriction_expiry_test.php @@ -107,6 +107,14 @@ public function test_expiry_restores_expired_restriction_only() $this->assertTrue($this->is_group_member($db, 3, 9), 'User 3 should still be in their restrict group'); $this->assertEquals(9, $this->get_default_group($db, 3), "User 3's default group should be unchanged"); $this->assertEquals(1, $this->count_restrict_rows($db, 3), "User 3's tracking row should remain"); + + // User 4's restriction expired, but they were already in the + // restrict group beforehand (restrict_new_membership = 0): keep that + // membership, restore only the default group and drop the row. + $this->assertTrue($this->is_group_member($db, 4, 8), 'User 4 should keep their pre-existing restrict group membership'); + $this->assertTrue($this->is_group_member($db, 4, 7), 'User 4 should still be in their original group'); + $this->assertEquals(7, $this->get_default_group($db, 4), "User 4's default group should be restored to their original group"); + $this->assertEquals(0, $this->count_restrict_rows($db, 4), "User 4's tracking row should be gone"); } protected function is_group_member($db, $user_id, $group_id)