-
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 Feature settings output #436
Conversation
This commit will refactor WP_Auth0_Admin_Appearance to use the new rendering funtions in WP_Auth0_Admin_Generic; change settings descriptions to be more clear; fix broken, incorrect, or redirected doc links; move passwordless switch to Features tab; reformat and test password policy validation.
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.
:)
<?php | ||
public function render_password_policy( $args = array() ) { | ||
$curr_val = $this->options->get( $args['opt_name'] ); | ||
$this->render_radio_button( $args['label_for'] . '_none', $args['opt_name'], '', 'None', empty( $curr_val ) ); |
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'm not sure about this. But check if once the policy is enabled, if turning it OFF and then ON again still removes the policy object from the JSON or if it now does send a "none" value.
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 tested it back and forth between none
and other values and everything works 👍
public function render_sso( $args = array() ) { | ||
$this->render_switch( $args['label_for'], $args['opt_name'] ); | ||
$this->render_field_description( | ||
__( 'SSO allows users to sign in once to multiple Clients in the same tenant. ', '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.
Clients or Applications?
sprintf( | ||
__( 'You can enable other MFA providers in the %s. ', 'wp-auth0' ), | ||
$this->get_dashboard_link( 'multifactor' ) | ||
) . __( 'For more information, see our %s. ', '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.
This %s
is not inside the sprintf
scope so I think there's no substitution happening here, am I right?
public function render_passwordless_enabled( $args = array() ) { | ||
$this->render_switch( $args[ 'label_for' ], $args[ 'opt_name' ] ); | ||
$this->render_field_description( | ||
__( 'Turn on Passwordless login (email or SMS) in the Auth0 form. ', '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.
What about the MFA
grant? that needs to be enabled for the Application too, at least when calling this from an SDK. Test if creating a new Application leaves that grant enabled/disabled and if you can log in successfully using this 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.
No special grant needed for Passwordless, uses the Auth Code grant.
This commit will refactor
WP_Auth0_Admin_Appearance
to use the newrendering funtions in
WP_Auth0_Admin_Generic
; change settingsdescriptions to be more clear; fix broken, incorrect, or redirected
doc links; move passwordless switch to Features tab; reformat and
test password policy validation.