From 101829b4dce2874bbe53264c1769bf9699527c2f Mon Sep 17 00:00:00 2001 From: battye Date: Mon, 26 Nov 2018 13:08:57 +0000 Subject: [PATCH 1/4] [ticket/15883] Add error for invalid usernames being added to a group Update the ACP and the UCP so that when bulk adding users to a group, if invalid usernames are submitted alongside valid usernames then a message will be displayed to inform the user what the invalid usernames are. PHPBB3-15883 --- phpBB/includes/acp/acp_groups.php | 14 +++++++++++++- phpBB/includes/functions_user.php | 18 ++++++++++++++++-- phpBB/includes/ucp/ucp_groups.php | 14 +++++++++++++- phpBB/language/en/acp/groups.php | 1 + 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/phpBB/includes/acp/acp_groups.php b/phpBB/includes/acp/acp_groups.php index 0e058213e0..20f913ff29 100644 --- a/phpBB/includes/acp/acp_groups.php +++ b/phpBB/includes/acp/acp_groups.php @@ -293,7 +293,19 @@ class acp_groups // Add user/s to group if ($error = group_user_add($group_id, false, $name_ary, $group_name, $default, $leader, 0, $group_row)) { - trigger_error($user->lang[$error] . adm_back_link($this->u_action . '&action=list&g=' . $group_id), E_USER_WARNING); + $display_message = $user->lang[$error]; + + if ($error == 'GROUP_USERS_INVALID') + { + // Find which users don't exist + $actual_name_ary = $name_ary; + $actual_user_id_ary = false; + user_get_id_name($actual_user_id_ary, $actual_name_ary, false, true); + + $display_message = sprintf($user->lang['GROUP_USERS_INVALID'], implode($user->lang['COMMA_SEPARATOR'], array_diff($name_ary, $actual_name_ary))); + } + + trigger_error($display_message . adm_back_link($this->u_action . '&action=list&g=' . $group_id), E_USER_WARNING); } $message = ($leader) ? 'GROUP_MODS_ADDED' : 'GROUP_USERS_ADDED'; diff --git a/phpBB/includes/functions_user.php b/phpBB/includes/functions_user.php index d019b867fa..e998ffdab9 100644 --- a/phpBB/includes/functions_user.php +++ b/phpBB/includes/functions_user.php @@ -26,8 +26,9 @@ if (!defined('IN_PHPBB')) * @param array &$user_id_ary The user ids to check or empty if usernames used * @param array &$username_ary The usernames to check or empty if user ids used * @param mixed $user_type Array of user types to check, false if not restricting by user type +* @param bool $update_references If false, the supplied array is unset and appears unchanged from where it was called */ -function user_get_id_name(&$user_id_ary, &$username_ary, $user_type = false) +function user_get_id_name(&$user_id_ary, &$username_ary, $user_type = false, $update_references = false) { global $db; @@ -50,7 +51,13 @@ function user_get_id_name(&$user_id_ary, &$username_ary, $user_type = false) } $sql_in = ($which_ary == 'user_id_ary') ? array_map('intval', ${$which_ary}) : array_map('utf8_clean_string', ${$which_ary}); - unset(${$which_ary}); + + // By unsetting the array here, the values passed in at the point user_get_id_name() was called will be retained. + // Otherwise, if we don't unset (as the array was passed by reference) the original array will be updated below. + if ($update_references === false) + { + unset(${$which_ary}); + } $user_id_ary = $username_ary = array(); @@ -2676,6 +2683,13 @@ function group_user_add($group_id, $user_id_ary = false, $username_ary = false, return 'NO_USER'; } + // Because the item that gets passed into the previous function is unset, the reference is lost and our original + // array is retained - so we know there's a problem if there's a different number of ids to usernames now. + if (count($user_id_ary) != count($username_ary)) + { + return 'GROUP_USERS_INVALID'; + } + // Remove users who are already members of this group $sql = 'SELECT user_id, group_leader FROM ' . USER_GROUP_TABLE . ' diff --git a/phpBB/includes/ucp/ucp_groups.php b/phpBB/includes/ucp/ucp_groups.php index 1fb026167a..e32c855179 100644 --- a/phpBB/includes/ucp/ucp_groups.php +++ b/phpBB/includes/ucp/ucp_groups.php @@ -1057,7 +1057,19 @@ class ucp_groups // Add user/s to group if ($error = group_user_add($group_id, false, $name_ary, $group_name, $default, 0, 0, $group_row)) { - trigger_error($user->lang[$error] . $return_page); + $display_message = $user->lang[$error]; + + if ($error == 'GROUP_USERS_INVALID') + { + // Find which users don't exist + $actual_name_ary = $name_ary; + $actual_user_id_ary = false; + user_get_id_name($actual_user_id_ary, $actual_name_ary, false, true); + + $display_message = sprintf($user->lang['GROUP_USERS_INVALID'], implode($user->lang['COMMA_SEPARATOR'], array_diff($name_ary, $actual_name_ary))); + } + + trigger_error($display_message . $return_page); } trigger_error($user->lang['GROUP_USERS_ADDED'] . '

' . sprintf($user->lang['RETURN_PAGE'], '', '')); diff --git a/phpBB/language/en/acp/groups.php b/phpBB/language/en/acp/groups.php index c3a5ae9e44..5d68132a34 100644 --- a/phpBB/language/en/acp/groups.php +++ b/phpBB/language/en/acp/groups.php @@ -111,6 +111,7 @@ $lang = array_merge($lang, array( 'GROUP_USERS_ADDED' => 'New users added to group successfully.', 'GROUP_USERS_EXIST' => 'The selected users are already members.', 'GROUP_USERS_REMOVE' => 'Users removed from group and new defaults set successfully.', + 'GROUP_USERS_INVALID' => 'No users were added to the group as the following usernames do not exist: %s', 'LEGEND_EXPLAIN' => 'These are the groups which are displayed in the group legend:', 'LEGEND_SETTINGS' => 'Legend settings', From 565f6925410b1022529670d47f3b00c244ccaed7 Mon Sep 17 00:00:00 2001 From: battye Date: Thu, 29 Nov 2018 14:37:14 +0000 Subject: [PATCH 2/4] [ticket/15883] Use the new language object Using the new ->lang() format. Made the username comparison case insensitive. Also fixed a bug where the return to previous page link in the UCP was going back to the list of groups rather than the specific manage group page when an error occurred. PHPBB3-15883 --- phpBB/includes/acp/acp_groups.php | 7 +++++-- phpBB/includes/ucp/ucp_groups.php | 13 +++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/phpBB/includes/acp/acp_groups.php b/phpBB/includes/acp/acp_groups.php index 20f913ff29..f541025bf2 100644 --- a/phpBB/includes/acp/acp_groups.php +++ b/phpBB/includes/acp/acp_groups.php @@ -29,6 +29,9 @@ class acp_groups global $phpbb_root_path, $phpbb_admin_path, $phpEx; global $request, $phpbb_container, $phpbb_dispatcher; + /** @var \phpbb\language\language $language Language object */ + $language = $phpbb_container->get('language'); + $user->add_lang('acp/groups'); $this->tpl_name = 'acp_groups'; $this->page_title = 'ACP_GROUPS_MANAGE'; @@ -293,7 +296,7 @@ class acp_groups // Add user/s to group if ($error = group_user_add($group_id, false, $name_ary, $group_name, $default, $leader, 0, $group_row)) { - $display_message = $user->lang[$error]; + $display_message = $language->lang($error); if ($error == 'GROUP_USERS_INVALID') { @@ -302,7 +305,7 @@ class acp_groups $actual_user_id_ary = false; user_get_id_name($actual_user_id_ary, $actual_name_ary, false, true); - $display_message = sprintf($user->lang['GROUP_USERS_INVALID'], implode($user->lang['COMMA_SEPARATOR'], array_diff($name_ary, $actual_name_ary))); + $display_message = $language->lang('GROUP_USERS_INVALID', implode($language->lang('COMMA_SEPARATOR'), array_udiff($name_ary, $actual_name_ary, 'strcasecmp'))); } trigger_error($display_message . adm_back_link($this->u_action . '&action=list&g=' . $group_id), E_USER_WARNING); diff --git a/phpBB/includes/ucp/ucp_groups.php b/phpBB/includes/ucp/ucp_groups.php index e32c855179..d0ac598920 100644 --- a/phpBB/includes/ucp/ucp_groups.php +++ b/phpBB/includes/ucp/ucp_groups.php @@ -32,6 +32,9 @@ class ucp_groups global $db, $user, $auth, $cache, $template; global $request, $phpbb_container, $phpbb_log; + /** @var \phpbb\language\language $language Language object */ + $language = $phpbb_container->get('language'); + $user->add_lang('groups'); $return_page = '

' . sprintf($user->lang['RETURN_PAGE'], '', ''); @@ -1054,10 +1057,12 @@ class ucp_groups if (confirm_box(true)) { + $return_manage_page = '

' . $language->lang('RETURN_PAGE', '', ''); + // Add user/s to group if ($error = group_user_add($group_id, false, $name_ary, $group_name, $default, 0, 0, $group_row)) { - $display_message = $user->lang[$error]; + $display_message = $language->lang($error); if ($error == 'GROUP_USERS_INVALID') { @@ -1066,13 +1071,13 @@ class ucp_groups $actual_user_id_ary = false; user_get_id_name($actual_user_id_ary, $actual_name_ary, false, true); - $display_message = sprintf($user->lang['GROUP_USERS_INVALID'], implode($user->lang['COMMA_SEPARATOR'], array_diff($name_ary, $actual_name_ary))); + $display_message = $language->lang('GROUP_USERS_INVALID', implode($language->lang('COMMA_SEPARATOR'), array_udiff($name_ary, $actual_name_ary, 'strcasecmp'))); } - trigger_error($display_message . $return_page); + trigger_error($display_message . $return_manage_page); } - trigger_error($user->lang['GROUP_USERS_ADDED'] . '

' . sprintf($user->lang['RETURN_PAGE'], '', '')); + trigger_error($language->lang('GROUP_USERS_ADDED') . $return_manage_page); } else { From 3f19d32f768974454046f744f126515fb3747b99 Mon Sep 17 00:00:00 2001 From: battye Date: Fri, 4 Jan 2019 15:49:15 +0000 Subject: [PATCH 3/4] [ticket/15883] Review changes PHPBB3-15883 --- phpBB/includes/acp/acp_groups.php | 2 +- phpBB/includes/functions_user.php | 1 + phpBB/includes/ucp/ucp_groups.php | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/acp/acp_groups.php b/phpBB/includes/acp/acp_groups.php index f541025bf2..7b1dc706db 100644 --- a/phpBB/includes/acp/acp_groups.php +++ b/phpBB/includes/acp/acp_groups.php @@ -302,7 +302,7 @@ class acp_groups { // Find which users don't exist $actual_name_ary = $name_ary; - $actual_user_id_ary = false; + $actual_user_id_ary = []; user_get_id_name($actual_user_id_ary, $actual_name_ary, false, true); $display_message = $language->lang('GROUP_USERS_INVALID', implode($language->lang('COMMA_SEPARATOR'), array_udiff($name_ary, $actual_name_ary, 'strcasecmp'))); diff --git a/phpBB/includes/functions_user.php b/phpBB/includes/functions_user.php index e998ffdab9..2be9d089a5 100644 --- a/phpBB/includes/functions_user.php +++ b/phpBB/includes/functions_user.php @@ -27,6 +27,7 @@ if (!defined('IN_PHPBB')) * @param array &$username_ary The usernames to check or empty if user ids used * @param mixed $user_type Array of user types to check, false if not restricting by user type * @param bool $update_references If false, the supplied array is unset and appears unchanged from where it was called +* @return null */ function user_get_id_name(&$user_id_ary, &$username_ary, $user_type = false, $update_references = false) { diff --git a/phpBB/includes/ucp/ucp_groups.php b/phpBB/includes/ucp/ucp_groups.php index d0ac598920..4673912aed 100644 --- a/phpBB/includes/ucp/ucp_groups.php +++ b/phpBB/includes/ucp/ucp_groups.php @@ -1068,7 +1068,7 @@ class ucp_groups { // Find which users don't exist $actual_name_ary = $name_ary; - $actual_user_id_ary = false; + $actual_user_id_ary = []; user_get_id_name($actual_user_id_ary, $actual_name_ary, false, true); $display_message = $language->lang('GROUP_USERS_INVALID', implode($language->lang('COMMA_SEPARATOR'), array_udiff($name_ary, $actual_name_ary, 'strcasecmp'))); From 08968bdb60c9ff286cb71901718500c7720f1da4 Mon Sep 17 00:00:00 2001 From: battye Date: Sat, 5 Jan 2019 08:19:21 +0000 Subject: [PATCH 4/4] [ticket/15883] Doc block change PHPBB3-15883 --- phpBB/includes/functions_user.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/functions_user.php b/phpBB/includes/functions_user.php index 2be9d089a5..71c4f41817 100644 --- a/phpBB/includes/functions_user.php +++ b/phpBB/includes/functions_user.php @@ -26,8 +26,8 @@ if (!defined('IN_PHPBB')) * @param array &$user_id_ary The user ids to check or empty if usernames used * @param array &$username_ary The usernames to check or empty if user ids used * @param mixed $user_type Array of user types to check, false if not restricting by user type -* @param bool $update_references If false, the supplied array is unset and appears unchanged from where it was called -* @return null +* @param boolean $update_references If false, the supplied array is unset and appears unchanged from where it was called +* @return boolean|string Returns false on success, error string on failure */ function user_get_id_name(&$user_id_ary, &$username_ary, $user_type = false, $update_references = false) {