-
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
Replace Rules-based settings with prompt to the dashboard [SDK-474] #624
Conversation
); | ||
|
||
// TODO: Remove this once feature has been removed | ||
if ( $this->options->get( 'link_auth0_users' ) ) { |
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.
Do not show the setting if it's not currently turned on.
} | ||
|
||
$options = $options + array( | ||
( count( $options ) ) => array( |
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.
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' ) . |
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.
"may currently be active" because this setting does not (and did not before) change if the rule was changed in the dashboard .
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.
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' ) . |
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.
We'll keep the MFA prompt here, regardless of whether the feature is on or not.
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.
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!
|
||
// TODO: Remove this once feature has been removed | ||
if ( $this->options->get( 'link_auth0_users' ) ) { | ||
$options[] = array( |
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.
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?
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.
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' ) . |
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.
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' ) . |
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.
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!
Changes
There are currently several settings that attempt to create/activate/deactivate Rules in the Auth0 dashboard:
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.