Remove and migrate Passwordless setting #425

joshcanhelp · 2018-04-03T17:21:31Z

Removes the passwordless method and CDN URL settings. Method is removed to simplify the wp-admin and move passwordless management to the Dashbaord. Passwordless URL is hard-coded to new combined Lock, removing the settings field to change that as well. Going forward, Lock will use the same library for both login form types. Also added a DB version bump and migration script to modify the connections setting so the login form will work as expected after a plugin update. Added a handful of TODOs that will be better suited for a future branch (same release).

cocojoe · 2018-04-04T10:19:41Z

lib/WP_Auth0_Lock_Options.php

-		} else {
-			return 'show';
-		}
+    return 'show';


cocojoe · 2018-04-04T10:21:13Z

lib/WP_Auth0_Options_Generic.php

-		if ( !isset( $options[$key] ) )
-			return apply_filters( 'wp_auth0_get_option', $default, $key );
-		return apply_filters( 'wp_auth0_get_option', $options[$key], $key );
+		if ( 'passwordless_cdn_url' === $key ) {


So in a generic function for returning settings values, we are hacking in passwordless cdn url?

Hence the TODO on the line below. There is another PR for changing how the scripts are being loaded and if I changed it everywhere, I'd have conflicts. I'll clear all the TODOs before release.

I have the same concern. This key has been removed on this same PR, why are you catching that case here? And how are you tracking the pending TODOs? Would be nice to have a linter we can run locally to verify it before the release.

This is just a stop-gap until #419 is merged. I'll resubmit this once that's been rebased against this one.

TODOs are just tracked in my IDE. I consider TODOs are requiring resolution before the release. If it's going to go past then, I'll create an issue here or Jira.

Fixed this. Instead of deprecating the setting, I'm just replacing it and leaving it in the options array but without a field to update it. I'll merge these two settings once Lock 10 is deprecated and we can assume most folks are on a version that supports passwordless.

cocojoe · 2018-04-04T10:23:01Z

lib/WP_Auth0_Options.php

@@ -218,11 +218,9 @@ protected function defaults() {
 			'remember_users_session' => false,
 			'default_login_redirection' => home_url(),
 			'passwordless_enabled' => false,
-			'passwordless_method' => 'magiclink',
 			'force_https_callback' => false,
 			'cdn_url' => 'https://cdn.auth0.com/js/lock/11.1/lock.min.js',


Why can't you use this URL for passwordless?

Passwordless was added in 11.2

lbalmaceda · 2018-04-04T18:03:05Z

lib/WP_Auth0_DBManager.php

+				}
+
+				if ( in_array( $pwl_method, array( 'emailcode', 'socialOrEmailcode' ) ) ) {
+					$lock_json = trim( $options->get( 'extra_conf' ) );


Is this extra_conf a generic extra configuration field, available to all other connections as well or only for passwordless? If you're only using this for passwordless I'd suggest renaming it to something more descriptive like extra_passwordless_configuration

Generic for Lock

lbalmaceda · 2018-04-04T18:04:30Z

lib/WP_Auth0_DBManager.php

+						$options->set( 'extra_conf', '{"passwordlessMethod": "code"}' );
+					} else {
+						$lock_json_decoded = json_decode( $lock_json, TRUE );
+						$lock_json_decoded[ 'passwordlessMethod' ] = 'code';


You're basically forcing pwdlessmethod=code. So link is not available? What if I have socialOrMagiclink enabled, wouldn't I expect to receive a link??

I might not be understanding what you're asking or you might not be familiar with how passwordless is configured. Link is the default so I'm setting code if that's what it was set as before.

lbalmaceda · 2018-04-04T18:07:53Z

lib/WP_Auth0_DBManager.php

+
+				if ( in_array( $pwl_method, array( 'emailcode', 'socialOrEmailcode' ) ) ) {
+					$lock_json = trim( $options->get( 'extra_conf' ) );
+					if ( empty( $lock_json ) ) {


I think this whole if/else can be simplified to:

$lock_json = trim( $options-> get( 'extra_conf', '{}') ) $lock_json_decoded = json_decode( $lock_json, TRUE ); $lock_json_decoded[ 'passwordlessMethod' ] = 'code'; $options->set( 'extra_conf', json_encode( $lock_json_decoded ) );

The second parameter in that get method isn't used if it's empty, only if it's not set. If we're doing an upgrade then it's set to something so that second parameter will not be used.

Still, could be improved ... fixed!

lbalmaceda · 2018-04-04T18:09:52Z

lib/WP_Auth0_DBManager.php

+
+			$update_options = $options->get_options();
+			unset( $update_options[ 'passwordless_cdn_url' ] );
+			unset( $update_options[ 'passwordless_method' ] );


why? are this deprecated properties?

lbalmaceda · 2018-04-04T18:13:45Z

lib/WP_Auth0_Options_Generic.php

-		if ( !isset( $options[$key] ) )
-			return apply_filters( 'wp_auth0_get_option', $default, $key );
-		return apply_filters( 'wp_auth0_get_option', $options[$key], $key );
+		if ( 'passwordless_cdn_url' === $key ) {


I have the same concern. This key has been removed on this same PR, why are you catching that case here? And how are you tracking the pending TODOs? Would be nice to have a linter we can run locally to verify it before the release.

lbalmaceda · 2018-04-04T18:18:11Z

lib/admin/WP_Auth0_Admin_Advanced.php

@@ -77,39 +75,8 @@ public function init() {
    $this->init_option_section( '', 'advanced', $advancedOptions );
  }

+  // TODO: Deprecate


If this is not going to be deprecated on this PR, don't remove the logic now. That's part of the deprecation :) It's OK to leave the TODO.

@lbalmaceda - I went a little too far in the removal here :)

Should I add it back and do a deprecation PR? Or just deprecate here?

Make a new separate PR for related deprecations.
Note that if the method can "still work well" you probably should just flag it as deprecated (docs, log a warning, etc) and keep the logic in there. Users who were using it should expect the same behavior. That's what deprecation means: We don't encourage the use but we don't break it. If by any reason you need to remove the logic, please describe future users which is the right method to call or make this old method call the new one inside so behavior is slightly maintained.

lbalmaceda · 2018-04-04T18:20:58Z

lib/admin/WP_Auth0_Admin_Advanced.php

@@ -546,51 +505,8 @@ public function link_accounts_validation( $old_options, $input ) {
    return $this->rule_validation($old_options, $input, 'link_auth0_users', WP_Auth0_RulesLib::$link_accounts['name'] . '-' . get_auth0_curatedBlogName(), $link_script);
  }

+  // TODO: Deprecate


Same comment

Removes the passwordless method and CDN URL settings. Method is removed to simplify the wp-admin and move passwordless management to the Dashbaord. Passwordless URL is hard-coded to new combined Lock, removing the settings field to change that as well. Going forward, Lock will use the same library for both login form types. Also added a DB version bump and migration script to modify the connections setting so the login form will work as expected after a plugin update. Added a handful of TODOs that will be better suited for a future branch (same release).

lbalmaceda · 2018-04-06T20:21:00Z

lib/WP_Auth0_DBManager.php

+
+					// Social + SMS means there are existing social connections we want to keep
+					case 'socialOrSms' :
+						$options->add_lock_connection( 'sms' );


I guess this line works both for "sms" and "socialOrSms", so the switch could be simplified to:

case 'sms' case 'socialOrSms' $options->add_lock_connection( 'sms' ); case 'emailcode' case 'magiclink' case 'socialOrMagiclink' case 'socialOrEmailcode' $options->add_lock_connection( 'email' );

$options->set() !== $options->add_lock_connection() ... one replaces all saved connections, the other adds to existing.

lbalmaceda · 2018-04-06T20:27:43Z

lib/WP_Auth0_DBManager.php

+			$options->set( 'passwordless_cdn_url', WPA0_LOCK_CDN_URL );
+
+			// Force passwordless_method to delete
+			$update_options = $options->get_options();


Just a vague comment. If you are doing this 3 liner logic in many places, might be good to consider adding a "remove_from_options (keys)" method

lbalmaceda · 2018-04-06T20:32:53Z

lib/WP_Auth0_DBManager.php

+				if ( in_array( $pwl_method, array( 'emailcode', 'socialOrEmailcode' ) ) ) {
+					$lock_json = trim( $options->get( 'extra_conf' ) );
+					$lock_json_decoded = ! empty( $lock_json ) ? json_decode( $lock_json, TRUE ) : array();
+					$lock_json_decoded[ 'passwordlessMethod' ] = 'code';


above you're switching over $options->get( 'passwordless_method' ). Here you're setting options.extra_conf.passwordlessMethod to code/link. Since they are related but you're talking about actually different configurations of the flow (one is the channel sms/email and the other the type link/code), is there any change you can rename this parameters names, or would that be a breaking change?

options.extra_conf.passwordlessMethod is defined by Lock:

https://github.com/auth0/lock#passwordless-options

... so no changing that. The passwordless_method option name in WP is going away with this PR so no need to worry about that.

lbalmaceda · 2018-04-06T20:35:22Z

lib/WP_Auth0_DBManager.php

+				}
+
+				// Need to set a special passwordlessMethod flag if using email code
+				if ( in_array( $pwl_method, array( 'emailcode', 'socialOrEmailcode' ) ) ) {


If I understand correctly, this if is checking if the selected type wishes a code instead of a link. Because you're relying on the auth0.js default value (currently link as you pointed out) this thing only changes that value if the type is "code". What if tomorrow that library changes the default?? ---> You should add an else to this if that sets the "link" type.

$lock_json = trim( $options->get( 'extra_conf' ) ); $lock_json_decoded = ! empty( $lock_json ) ? json_decode( $lock_json, TRUE ) : array(); $lock_json_decoded[ 'passwordlessMethod' ] = in_array( $pwl_method, array( 'emailcode', 'socialOrEmailcode' ) ) ? 'code' : 'link'; //or however it's called $options->set( 'extra_conf', json_encode( $lock_json_decoded ) );

lbalmaceda

LGTM

joshcanhelp added the CH: Removed label Apr 3, 2018

joshcanhelp added this to the v3-Next milestone Apr 3, 2018

joshcanhelp self-assigned this Apr 3, 2018

joshcanhelp added the Ready for Review label Apr 3, 2018

cocojoe changed the title ~~Remove and migrate PWL setting~~ Remove and migrate Passwordless setting Apr 4, 2018

cocojoe reviewed Apr 4, 2018

View reviewed changes

joshcanhelp force-pushed the remove-pwl-setting branch from 5bcd79f to 9a55b5c Compare April 4, 2018 14:06

lbalmaceda suggested changes Apr 4, 2018

View reviewed changes

joshcanhelp force-pushed the remove-pwl-setting branch 6 times, most recently from f91dffb to 21d44e7 Compare April 5, 2018 19:01

joshcanhelp force-pushed the remove-pwl-setting branch from 21d44e7 to 17e4d99 Compare April 5, 2018 19:08

lbalmaceda reviewed Apr 6, 2018

View reviewed changes

set code or link

94a908c

lbalmaceda approved these changes Apr 10, 2018

View reviewed changes

joshcanhelp merged commit fb23eb3 into dev Apr 10, 2018

joshcanhelp deleted the remove-pwl-setting branch April 10, 2018 20:44

joshcanhelp removed the Ready for Review label Apr 17, 2018

joshcanhelp mentioned this pull request Jun 5, 2018

Release 3.6.0 #475

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove and migrate Passwordless setting #425

Remove and migrate Passwordless setting #425

joshcanhelp commented Apr 3, 2018

cocojoe Apr 4, 2018

cocojoe Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 5, 2018

joshcanhelp Apr 5, 2018

cocojoe Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 5, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 5, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 5, 2018 •

edited

Loading

lbalmaceda Apr 4, 2018

lbalmaceda Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 5, 2018

lbalmaceda Apr 5, 2018

lbalmaceda Apr 4, 2018

lbalmaceda Apr 6, 2018

joshcanhelp Apr 6, 2018

lbalmaceda Apr 6, 2018

lbalmaceda Apr 6, 2018

joshcanhelp Apr 6, 2018 •

edited

Loading

lbalmaceda Apr 6, 2018

lbalmaceda left a comment

Remove and migrate Passwordless setting #425

Remove and migrate Passwordless setting #425

Conversation

joshcanhelp commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

joshcanhelp Apr 5, 2018 •

edited

Loading

joshcanhelp Apr 6, 2018 •

edited

Loading