Skip to content
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

Replace Rules-based settings with prompt to the dashboard [SDK-474] #624

Merged
merged 2 commits into from
Feb 12, 2019

Conversation

joshcanhelp
Copy link
Contributor

Changes

There are currently several settings that attempt to create/activate/deactivate Rules in the Auth0 dashboard:

  • Features > Multifactor Authentication Multifactor-Guardian-Do-Not-Rename
  • Features > Fullcontact integration Enrich-profile-with-FullContact-Do-Not-Rename
  • Features > Store Geolocation Store-Geo-Location-Do-Not-Rename
  • Features > Store Zipcode Income Enrich-profile-with-Zipcode-Income-Do-Not-Rename
  • Advanced > Link Users with Same Email Account-Linking-Do-Not-Rename

These require a valid API token or a client credentials grant (not implemented for these settings currently) and often fail because the Auth0 state is different than the WP admin one. It also makes changes for the whole tenant rather than just the WP site, which could cause issues.

This PR removes this settings for sites where the above features is currently turned off. If the feature is marked as on, there is a prompt added to manage it in the dashboard.

@joshcanhelp joshcanhelp added this to the 3.9.0 milestone Feb 4, 2019
);

// TODO: Remove this once feature has been removed
if ( $this->options->get( 'link_auth0_users' ) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not show the setting if it's not currently turned on.

}

$options = $options + array(
( count( $options ) ) => array(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to make sure the setting stays in the same place, if we're showing it.

$this->render_field_description(
__( 'Link accounts with the same verified e-mail address. ', 'wp-auth0' ) .
__( 'See the "Require Verified Email" setting above for more information on email verification', 'wp-auth0' )
__( 'This feature may currently be active. ', 'wp-auth0' ) .
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"may currently be active" because this setting does not (and did not before) change if the rule was changed in the dashboard .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider "enabled" instead of active.

__( 'You can enable other MFA providers in the %s. ', 'wp-auth0' ),
$this->get_dashboard_link( 'multifactor' )
) . __( 'For more information, see our ', 'wp-auth0' ) .
__( 'MFA is a method of verifying identity by requiring more than 1 piece of identifying information. ', 'wp-auth0' ) .
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll keep the MFA prompt here, regardless of whether the feature is on or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this particular line confusing. For me it is clearer to say "MFA is a method to verify a user's identity by checking a second factor in addition to the password". The phrases below are ok!

@cocojoe cocojoe changed the title Replace Rules-based settings with prompt to the dashboard Replace Rules-based settings with prompt to the dashboard [SDK-474] Feb 6, 2019

// TODO: Remove this once feature has been removed
if ( $this->options->get( 'link_auth0_users' ) ) {
$options[] = array(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it OK this change? you are assigning it to $options[]. What does that mean? is it overriding the whole array or just and index, and if so, which one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just pushes a new item to the end of an array. Same effect as array_push() (ref)

$this->render_field_description(
__( 'Link accounts with the same verified e-mail address. ', 'wp-auth0' ) .
__( 'See the "Require Verified Email" setting above for more information on email verification', 'wp-auth0' )
__( 'This feature may currently be active. ', 'wp-auth0' ) .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider "enabled" instead of active.

__( 'You can enable other MFA providers in the %s. ', 'wp-auth0' ),
$this->get_dashboard_link( 'multifactor' )
) . __( 'For more information, see our ', 'wp-auth0' ) .
__( 'MFA is a method of verifying identity by requiring more than 1 piece of identifying information. ', 'wp-auth0' ) .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this particular line confusing. For me it is clearer to say "MFA is a method to verify a user's identity by checking a second factor in addition to the password". The phrases below are ok!

@joshcanhelp joshcanhelp merged commit 190b0ac into master Feb 12, 2019
@joshcanhelp joshcanhelp deleted the remove-profile-rules-admin branch February 12, 2019 00:23
@joshcanhelp joshcanhelp modified the milestones: 3.9.0, 3.10.0 Feb 18, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants