-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change and improve user profile #532
Conversation
Will review once other merged and rebased. |
f964e10
to
a746b2d
Compare
WP_Auth0.php
Outdated
@@ -123,7 +123,15 @@ public function init() { | |||
$this->social_amplificator = new WP_Auth0_Amplificator( $this->db_manager, $this->a0_options ); | |||
$this->social_amplificator->init(); | |||
|
|||
$edit_profile = new WP_Auth0_EditProfile( $this->db_manager, $users_repo, $this->a0_options ); | |||
$api_change_password = new WP_Auth0_Api_Change_Password( $this->a0_options ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding dependency injection to assist with testing.
@@ -420,7 +428,7 @@ public function render_form( $html ) { | |||
// Do not show Auth0 form when ... | |||
if ( | |||
// .. processing lost password | |||
( isset( $_GET['action'] ) && $_GET['action'] == 'lostpassword' ) | |||
( isset( $_GET['action'] ) && in_array( $_GET['action'], array( 'lostpassword', 'rp' ) ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure we can still reset the password if stuck.
@@ -0,0 +1,81 @@ | |||
/* global jQuery, wpa0UserProfile, alert */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving in all inline JS from user profile.
lib/WP_Auth0_EditProfile.php
Outdated
|
||
if ( $pagenow == 'profile.php' || $pagenow == 'user-edit.php' ) { | ||
add_action( 'admin_footer', array( $this, 'disable_email_field' ) ); | ||
add_action( 'admin_enqueue_scripts', array( $this, 'admin_enqueue_scripts' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all JS to an external file, see above.
75e4066
to
3dd43d0
Compare
$edit_profile = new WP_Auth0_EditProfile( $this->db_manager, $users_repo, $this->a0_options ); | ||
$edit_profile->init(); | ||
|
||
$api_change_password = new WP_Auth0_Api_Change_Password( $this->a0_options, $api_client_creds ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency injection for classes using the new API framework.
|
||
add_action( 'edit_user_profile', array( $this, 'show_delete_identity' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving to individual classes
Codecov Report
@@ Coverage Diff @@
## dev #532 +/- ##
============================================
+ Coverage 12.25% 14.33% +2.08%
- Complexity 1666 1712 +46
============================================
Files 66 71 +5
Lines 5762 5865 +103
============================================
+ Hits 706 841 +135
+ Misses 5056 5024 -32
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a long read, in general LGTM. As always the progression of tests within the SDK is great 👌
One small comment re docheader, but will approve.
/** | ||
* Set the user_id and provider, set the authorization header, call the API, and handle the response. | ||
* | ||
* @param string|null $user_id - Auth0 user ID to delete the MFA provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention in docheader that the default MFA provider is google-authenticator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cocojoe - Function params default there and it's mostly an internally-used functionality.
Improve the user profile to send password changes to Auth0 in all cases, stop password changes if they do not meet the security policy, move all in-line profile JS to an external file, TESTS. Uses the Management API framework merged in #537.
Specific Changes:
WP_Auth0_EditProfile
and into an external JS fileassets/js/edit-user-profile.js
usingwp_localize_script
for PHP-generated data; addWP_Auth0_EditProfile::admin_enqueue_scripts()
to load that JS on the user profile and profile edit pages.WP_Auth0_EditProfile::validate_new_password()
to a new methodWP_Auth0_Profile_Change_Password::validate_new_password
and improve to check for a failed password update and stop WordPress from updating if so; tie this tovalidate_password_reset
so password reset attempts are sent and checked as well.WP_Auth0_EditProfile::delete_user_data()
andWP_Auth0_EditProfile::delete_mfa()
toWP_Auth0_Profile_Delete_Data::delete_user_data
andWP_Auth0_Profile_Delete_Mfa::delete_mfa()
respectively; add nonce and capability checks to both methods for more robust error handling.WP_Auth0_Api_Change_Password
to send password changes to Auth0.WP_Auth0_Api_Delete_User_Mfa
to delete an MFA provider for a user on Auth0.Note to reviewers:
This is a large change but almost 60% is new tests to reach diff coverage of ~100%.
Closes #300
Closes #310
Closes #375