From ac8939dbf89c68b7e1d7b35d15c594737d3ecc51 Mon Sep 17 00:00:00 2001 From: Carlos Proensa Date: Mon, 14 Mar 2016 11:31:31 +0100 Subject: [PATCH 1/8] Create a token type for password activation hash Having the activation hash stored as a token allows for more control over when the token is used and deactivated. Now, this token is created when the activation or password reset is requested, and remains valid until the user actually changes their password. --- core/constant_inc.php | 2 ++ core/user_api.php | 4 ++++ lost_pwd.php | 1 + verify.php | 4 ++-- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/core/constant_inc.php b/core/constant_inc.php index fb216a9533..f212f3ca55 100644 --- a/core/constant_inc.php +++ b/core/constant_inc.php @@ -496,6 +496,7 @@ define( 'TOKEN_AUTHENTICATED', 4 ); define( 'TOKEN_COLLAPSE', 5 ); define( 'TOKEN_ACCOUNT_VERIFY', 6 ); +define( 'TOKEN_ACCOUNT_ACTIVATION', 7 ); define( 'TOKEN_USER', 1000 ); # token expirations @@ -505,6 +506,7 @@ define( 'TOKEN_EXPIRY_LAST_VISITED', 24 * 60 * 60 ); define( 'TOKEN_EXPIRY_AUTHENTICATED', 5 * 60 ); define( 'TOKEN_EXPIRY_COLLAPSE', 365 * 24 * 60 * 60 ); +define( 'TOKEN_EXPIRY_ACCOUNT_ACTIVATION', 24 * 60 * 60 ); # config types define( 'CONFIG_TYPE_DEFAULT', 0 ); diff --git a/core/user_api.php b/core/user_api.php index 0f146dcdf9..7f08c3dc19 100644 --- a/core/user_api.php +++ b/core/user_api.php @@ -602,6 +602,7 @@ function user_create( $p_username, $p_password, $p_email = '', # Send notification email if( !is_blank( $p_email ) ) { $t_confirm_hash = auth_generate_confirm_hash( $t_user_id ); + token_set( TOKEN_ACCOUNT_ACTIVATION, $t_confirm_hash, TOKEN_EXPIRY_ACCOUNT_ACTIVATION, $t_user_id ); email_signup( $t_user_id, $t_confirm_hash, $p_admin_name ); } @@ -1566,6 +1567,8 @@ function user_set_password( $p_user_id, $p_password, $p_allow_protected = false # When the password is changed, invalidate the cookie to expire sessions that # may be active on all browsers. $c_cookie_string = auth_generate_unique_cookie_string(); + # Delete token for password activation if there is any + token_delete( TOKEN_ACCOUNT_ACTIVATION, $p_user_id ); $c_password = auth_process_plain_password( $p_password ); @@ -1661,6 +1664,7 @@ function user_reset_password( $p_user_id, $p_send_email = true ) { # Send notification email if( $p_send_email ) { $t_confirm_hash = auth_generate_confirm_hash( $p_user_id ); + token_set( TOKEN_ACCOUNT_ACTIVATION, $t_confirm_hash, TOKEN_EXPIRY_ACCOUNT_ACTIVATION, $p_user_id ); email_send_confirm_hash_url( $p_user_id, $t_confirm_hash ); } } else { diff --git a/lost_pwd.php b/lost_pwd.php index bd36819710..81ea790746 100644 --- a/lost_pwd.php +++ b/lost_pwd.php @@ -95,6 +95,7 @@ } $t_confirm_hash = auth_generate_confirm_hash( $t_user_id ); +token_set( TOKEN_ACCOUNT_ACTIVATION, $t_confirm_hash, TOKEN_EXPIRY_ACCOUNT_ACTIVATION, $t_user_id ); email_send_confirm_hash_url( $t_user_id, $t_confirm_hash ); user_increment_lost_password_in_progress_count( $t_user_id ); diff --git a/verify.php b/verify.php index 68ee7e7b5e..a76bae2209 100644 --- a/verify.php +++ b/verify.php @@ -61,9 +61,9 @@ print_header_redirect( 'verify.php?id=' . $f_user_id . '&confirm_hash=' . $f_confirm_hash ); } -$t_calculated_confirm_hash = auth_generate_confirm_hash( $f_user_id ); +$t_token_confirm_hash = token_get_value( TOKEN_ACCOUNT_ACTIVATION, $f_user_id ); -if( $f_confirm_hash != $t_calculated_confirm_hash ) { +if( $f_confirm_hash != $t_token_confirm_hash ) { trigger_error( ERROR_LOST_PASSWORD_CONFIRM_HASH_INVALID, ERROR ); } From f782d97993a6ba73da0374e242b64aae458ab669 Mon Sep 17 00:00:00 2001 From: Carlos Proensa Date: Mon, 14 Mar 2016 14:14:11 +0100 Subject: [PATCH 2/8] Implement verify.php as a standalone page Redesign verify.php as a clean page that only prompts for new password, does not log the user in the application, so no navigation is possible until a new password provided. --- account_update.php | 21 +++++++++++----- css/default.css | 1 + verify.php | 63 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 74 insertions(+), 11 deletions(-) diff --git a/account_update.php b/account_update.php index 100473e1d7..ff43a88d0e 100644 --- a/account_update.php +++ b/account_update.php @@ -57,17 +57,22 @@ form_security_validate( 'account_update' ); -$t_user_id = auth_get_current_user_id(); - # If token is set, it's a password reset request from verify.php, and if # not we need to reauthenticate the user -$t_account_verification = token_get_value( TOKEN_ACCOUNT_VERIFY, $t_user_id ); +$t_verify_user_id = gpc_get( 'verify_user_id', false ); +$t_account_verification = $t_verify_user_id ? token_get_value( TOKEN_ACCOUNT_VERIFY, $t_verify_user_id ) : false; if( !$t_account_verification ) { auth_reauthenticate(); + $t_user_id = auth_get_current_user_id(); +} else { + # set a temporary cookie so the login information is passed between pages. + auth_set_cookies( $t_verify_user_id, false ); + # fake login so the user can set their password + auth_attempt_script_login( user_get_field( $t_verify_user_id, 'username' ) ); + $t_user_id = $t_verify_user_id; } auth_ensure_user_authenticated(); - current_user_ensure_unprotected(); $f_email = gpc_get_string( 'email', '' ); @@ -86,7 +91,9 @@ $t_ldap = ( LDAP == config_get( 'login_method' ) ); # Update email (but only if LDAP isn't being used) -if( !( $t_ldap && config_get( 'use_ldap_email' ) ) ) { +# Do not update email for a user verification +if( !( $t_ldap && config_get( 'use_ldap_email' ) ) + && !$t_account_verification ) { if( $f_email != user_get_email( $t_user_id ) ) { user_set_email( $t_user_id, $f_email ); $t_email_updated = true; @@ -94,7 +101,9 @@ } # Update real name (but only if LDAP isn't being used) -if( !( $t_ldap && config_get( 'use_ldap_realname' ) ) ) { +# Do not update real name for a user verification +if( !( $t_ldap && config_get( 'use_ldap_realname' ) ) + && !$t_account_verification ) { # strip extra spaces from real name $t_realname = string_normalize( $f_realname ); if( $t_realname != user_get_field( $t_user_id, 'realname' ) ) { diff --git a/css/default.css b/css/default.css index 8576c18a68..7af5088ca1 100644 --- a/css/default.css +++ b/css/default.css @@ -480,6 +480,7 @@ div.form-container table th, div.table-container table th { text-align: left; } +div#verify-div, div#reauth-div, div#login-div, div#lost-password-div, diff --git a/verify.php b/verify.php index a76bae2209..e1c3c8abae 100644 --- a/verify.php +++ b/verify.php @@ -67,9 +67,6 @@ trigger_error( ERROR_LOST_PASSWORD_CONFIRM_HASH_INVALID, ERROR ); } -# set a temporary cookie so the login information is passed between pages. -auth_set_cookies( $f_user_id, false ); - user_reset_failed_login_count_to_zero( $f_user_id ); user_reset_lost_password_in_progress_count_to_zero( $f_user_id ); @@ -79,5 +76,61 @@ user_increment_login_count( $f_user_id ); -define( 'ACCOUNT_VERIFICATION_INC', true ); -include ( dirname( __FILE__ ) . '/account_page.php' ); +# extracts the user information +# and prefixes it with u_ +$t_row = user_get_row( $f_user_id ); + +extract( $t_row, EXTR_PREFIX_ALL, 'u' ); + +$t_can_change_password = helper_call_custom_function( 'auth_can_change_password', array() ); + +html_page_top1(); +html_page_top2a(); + +?> + +
+
    + ' . lang_get( 'verify_warning' ) . ''; + echo '
  • ' . lang_get( 'verify_change_password' ) . '
    • '; + ?> +
+
+ + +
+
+
+ +
+ + + +
+ + + +
+ + + +
+
+ + + +
+ + +
+
+
+ + Date: Wed, 13 Apr 2016 18:13:42 +0200 Subject: [PATCH 3/8] Verify password, adjustements for LDAP In case LDAP is active, don't show the password change form, and print the warning message explaining that passwords can't be modified. When LDAP login is active, or in a general case, the user cannot change its own password, this page should not be reached in the first place, because the options to reset user password should not be available. --- verify.php | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/verify.php b/verify.php index e1c3c8abae..e7d13b3d1f 100644 --- a/verify.php +++ b/verify.php @@ -92,12 +92,19 @@
    ' . lang_get( 'verify_warning' ) . ''; - echo '
  • ' . lang_get( 'verify_change_password' ) . '
    • '; + if( $t_can_change_password ) { + echo '
  • ' . lang_get( 'verify_warning' ) . '
    • '; + echo '
  • ' . lang_get( 'verify_change_password' ) . '
    • '; + } else { + echo '
  • ' . lang_get( 'no_password_change' ) . '
    • '; + } ?>
+
@@ -109,10 +116,8 @@
- @@ -127,10 +132,11 @@ - Date: Wed, 13 Apr 2016 18:19:18 +0200 Subject: [PATCH 4/8] Verify password, text adjustements Change some text string to match verification behaviour: - Change warning as token is still valid after initial access - Change label for "new password" field. --- lang/strings_english.txt | 3 ++- verify.php | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lang/strings_english.txt b/lang/strings_english.txt index bac055788f..c6b5a3c16d 100644 --- a/lang/strings_english.txt +++ b/lang/strings_english.txt @@ -357,13 +357,14 @@ $s_realname_label = 'Real Name'; $s_email = 'E-mail'; $s_email_label = 'E-mail'; $s_password = 'Password'; +$s_new_password = 'New password'; $s_no_password_change = 'The password is controlled by another system, hence cannot be edited here.'; $s_confirm_password = 'Confirm Password'; $s_current_password = 'Current Password'; $s_access_level = 'Access Level'; $s_access_level_label = 'Access Level'; $s_update_user_button = 'Update User'; -$s_verify_warning = 'Your account information has been verified. The account confirmation message you have received is now invalid.'; +$s_verify_warning = 'Your account information has been verified.'; $s_verify_change_password = 'You must set a password here to allow you to log in again.'; # API tokens page diff --git a/verify.php b/verify.php index e7d13b3d1f..f9b21d8bc6 100644 --- a/verify.php +++ b/verify.php @@ -122,7 +122,7 @@ token_set( TOKEN_ACCOUNT_VERIFY, true, TOKEN_EXPIRY_AUTHENTICATED, $u_id ); ?>
- +
From 8fadfb2c4be01c76f491fae5adf6c2aba1fd3e0a Mon Sep 17 00:00:00 2001 From: Carlos Proensa Date: Sun, 17 Apr 2016 17:36:38 +0200 Subject: [PATCH 5/8] Include realname field in verify.php Show realname field in account verification, and allow the user to modify their real name in the same page. This is useful for self registration, where real name is created blank. So even if real name is not required, there is more chances that the user fills in their name on first activation. --- account_update.php | 4 +--- verify.php | 7 +++++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/account_update.php b/account_update.php index ff43a88d0e..4309c89cd4 100644 --- a/account_update.php +++ b/account_update.php @@ -101,9 +101,7 @@ } # Update real name (but only if LDAP isn't being used) -# Do not update real name for a user verification -if( !( $t_ldap && config_get( 'use_ldap_realname' ) ) - && !$t_account_verification ) { +if( !( $t_ldap && config_get( 'use_ldap_realname' ) ) ) { # strip extra spaces from real name $t_realname = string_normalize( $f_realname ); if( $t_realname != user_get_field( $t_user_id, 'realname' ) ) { diff --git a/verify.php b/verify.php index f9b21d8bc6..573feabc28 100644 --- a/verify.php +++ b/verify.php @@ -121,6 +121,13 @@ # When verifying account, set a token and don't display current password token_set( TOKEN_ACCOUNT_VERIFY, true, TOKEN_EXPIRY_AUTHENTICATED, $u_id ); ?> +
+ + + + + +
From 01affc804ac56b0f603d493e2d59dd5e26749ae4 Mon Sep 17 00:00:00 2001 From: Carlos Proensa Date: Sun, 17 Apr 2016 18:48:51 +0200 Subject: [PATCH 6/8] Do not allow blank password on account verification Fixes #20816 --- account_update.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/account_update.php b/account_update.php index 4309c89cd4..57c2317daa 100644 --- a/account_update.php +++ b/account_update.php @@ -88,6 +88,12 @@ $t_password_updated = false; $t_realname_updated = false; +# Do not allow blank passwords in account verification/reset +if( $t_account_verification && is_blank( $f_password ) ) { + error_parameters( lang_get( 'password' ) ); + trigger_error( ERROR_EMPTY_FIELD, ERROR ); +} + $t_ldap = ( LDAP == config_get( 'login_method' ) ); # Update email (but only if LDAP isn't being used) From ae8871cef46788f044f27bc225dde74e5e50ed67 Mon Sep 17 00:00:00 2001 From: Carlos Proensa Date: Sun, 17 Apr 2016 19:29:48 +0200 Subject: [PATCH 7/8] Fix labels in account update form - Fix "current password" label association in form source. - Change label for "new password", to better explain the function of that field. --- account_page.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/account_page.php b/account_page.php index 47ece9fe19..01163ff474 100644 --- a/account_page.php +++ b/account_page.php @@ -171,13 +171,13 @@ } else { ?>
- +
- +
From 777f5e81ecd6176ec0cffb84c38136e09e45851e Mon Sep 17 00:00:00 2001 From: Carlos Proensa Date: Sun, 17 Apr 2016 21:26:00 +0200 Subject: [PATCH 8/8] Only update user data if all fields are valid On user account update, first verify all submitted fields, and only update the data if they are valid. Fixes #20817 --- account_update.php | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/account_update.php b/account_update.php index 57c2317daa..e1efaa8bf1 100644 --- a/account_update.php +++ b/account_update.php @@ -84,9 +84,9 @@ $t_redirect_url = 'index.php'; # @todo Listing what fields were updated is not standard behaviour of MantisBT - it also complicates the code. -$t_email_updated = false; -$t_password_updated = false; -$t_realname_updated = false; +$t_update_email = null; +$t_update_password = null; +$t_update_realname = null; # Do not allow blank passwords in account verification/reset if( $t_account_verification && is_blank( $f_password ) ) { @@ -101,8 +101,7 @@ if( !( $t_ldap && config_get( 'use_ldap_email' ) ) && !$t_account_verification ) { if( $f_email != user_get_email( $t_user_id ) ) { - user_set_email( $t_user_id, $f_email ); - $t_email_updated = true; + $t_update_email = $f_email; } } @@ -114,8 +113,7 @@ # checks for problems with realnames $t_username = user_get_field( $t_user_id, 'username' ); user_ensure_realname_unique( $t_username, $t_realname ); - user_set_realname( $t_user_id, $t_realname ); - $t_realname_updated = true; + $t_update_realname = $t_realname; } } @@ -129,37 +127,39 @@ } if( !auth_does_password_match( $t_user_id, $f_password ) ) { - user_set_password( $t_user_id, $f_password ); - $t_password_updated = true; + $t_update_password = $f_password; } } } -form_security_purge( 'account_update' ); - -# Clear the verification token -if( $t_account_verification ) { - token_delete( TOKEN_ACCOUNT_VERIFY, $t_user_id ); -} - html_page_top( null, $t_redirect_url ); $t_message = ''; -if( $t_email_updated ) { +if( $t_update_email ) { + user_set_email( $t_user_id, $f_email ); $t_message .= lang_get( 'email_updated' ); } -if( $t_password_updated ) { +if( $t_update_password ) { + user_set_password( $t_user_id, $f_password ); $t_message = is_blank( $t_message ) ? '' : $t_message . '
'; $t_message .= lang_get( 'password_updated' ); + + # Clear the verification token + if( $t_account_verification ) { + token_delete( TOKEN_ACCOUNT_VERIFY, $t_user_id ); + } } -if( $t_realname_updated ) { +if( $t_update_realname ) { + user_set_realname( $t_user_id, $t_realname ); $t_message = is_blank( $t_message ) ? '' : $t_message . '
'; $t_message .= lang_get( 'realname_updated' ); } +form_security_purge( 'account_update' ); + html_operation_successful( $t_redirect_url, $t_message ); html_page_bottom();