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.

Sign up for GitHub

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

Jump to bottom

Fix SLO redirect, SLO on when SSO off, SSO setting not pushed to dashboard #466

Merged
merged 2 commits into from
May 24, 2018
Merged

Fix SLO redirect, SLO on when SSO off, SSO setting not pushed to dashboard #466

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bc2f09
Fixing SLO redirect and SSO setting push to dashboard
joshcanhelp May 22, 2018
9aa938b
Changes based on PR review feedback
joshcanhelp May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions lib/WP_Auth0_Lock10_Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,10 @@ protected function build_settings( $settings ) {
public function get_sso_options() {
$options["scope"] = WP_Auth0_LoginManager::get_userinfo_scope( 'sso' );

$options["responseType"] = 'token id_token';
if ( $this->get_auth0_implicit_workflow() ) {
$options["responseType"] = 'id_token';
$options["redirectUri"] = $this->get_implicit_callback_url();
} else {
$options["responseType"] = 'code';
$options["redirectUri"] = $this->get_code_callback_url();
}

Expand Down
43 changes: 34 additions & 9 deletions lib/admin/WP_Auth0_Admin_Features.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ public function render_password_policy( $args = array() ) {

/**
* Render form field and description for the `sso` option.
* If SSO is off, the SLO setting will be hidden and turned off as well.
* IMPORTANT: Internal callback use only, do not call this function directly!
*
* @param array $args - callback args passed in from add_settings_field().
Expand All @@ -140,7 +141,7 @@ public function render_password_policy( $args = array() ) {
* @see add_settings_field()
*/
public function render_sso( $args = array() ) {
$this->render_switch( $args['label_for'], $args['opt_name'] );
$this->render_switch( $args['label_for'], $args['opt_name'], 'wpa0_singlelogout' );
Copy link
Contributor

Choose a reason for hiding this comment

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

since this method is named after sso. What is this slo line doing? I understand that is displaying a "toggle button" for enabling slo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is this slo line doing? I understand that is displaying a "toggle button" for enabling slo

Seems like you're answering your own question here ... can you rephrase?

$this->render_field_description(
__( 'SSO allows users to sign in once to multiple Applications in the same tenant. ', 'wp-auth0' ) .
__( 'Turning this on will attempt to automatically log a user in when they visit wp-login.php. ', 'wp-auth0' ) .
Expand Down Expand Up @@ -291,24 +292,48 @@ public function render_override_wp_avatars( $args = array() ) {
}

public function basic_validation( $old_options, $input ) {
// SLO will be turned off in WP_Auth0_Admin_Features::sso_validation() if SSO is not on.
$input['singlelogout'] = ( isset( $input['singlelogout'] ) ? $input['singlelogout'] : 0 );
$input['override_wp_avatars'] = ( isset( $input['override_wp_avatars'] ) ? $input['override_wp_avatars'] : 0 );

return $input;
}

/**
* Update the Auth0 Application if SSO is turned on and disable SLO if it is turned off.
*
* @param array $old_options - option values before saving.
* @param array $input - new option values being saved.
*
* @return array
*/
public function sso_validation( $old_options, $input ) {
$input['sso'] = ( isset( $input['sso'] ) ? $input['sso'] : 0 );
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it would be nice if this method doesn't modify the "original values" passed as parameters and instead, creates a copy of them or just new vars for the values it's going to use. e.g. don't overwrite $input['sso']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an option, this is how forms are validated in WP. Overwriting the $input array is necessary to validate what's being saved (security with sanitization and reliability with making sure we expect the correct values later)


if ( $old_options['sso'] != $input['sso'] && 1 == $input['sso'] ) {
if ( false === WP_Auth0_Api_Client::update_client( $input['domain'], $input['auth0_app_token'], $input['client_id'], $input['sso'] == 1 ) ) {

$error = __( 'There was an error updating your Auth0 App to enable SSO. To do it manually, turn it ', 'wp-auth0' );
$error .= '<a href="https://auth0.com/docs/sso/single-sign-on#1">HERE</a>.';
$this->add_validation_error( $error );

$is_sso = ! empty( $input['sso'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

the line above sets a 0 if the value is not available. Is that 0 being treated as "empty" on this line? or would it make sense to just define this line above the one below and use $is_sso as conditional for the ? if clause above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to change how the validation actually works (see the original) since it does work. I'm just forcing a boolean to check more clearly in this method.


if ( $old_options['sso'] !== $input['sso'] && $is_sso ) {
$app_update_failed = true;
$app_token = WP_Auth0_Api_Client::get_client_token();
if ( $app_token ) {
$update_result = WP_Auth0_Api_Client::update_client( $input['domain'], $app_token, $input['client_id'], true );
if ( $update_result ) {
$app_update_failed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

supposing update_client returns a boolean value, there's no need to have this if

$app_update_failed = ! $update_result

}
}
if ( $app_update_failed ) {
$this->add_validation_error(
__( 'The SSO setting for your Application could not be updated automatically. ', 'wp-auth0' ) .
__( 'Check that "Use Auth0 instead of the IdP to do Single Sign On" is turned on in the ', 'wp-auth0' ) .
$this->get_dashboard_link( 'applications/' . $input['client_id'] . '/settings' )
);
}
}

if ( ! $is_sso ) {
// SLO does not function without SSO.
unset( $input['singlelogout'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd flip this 2 if clauses since that would simplify the now second condition

if ( ! $is_sso ) {
  // 
}
else if ( $old_options['sso'] !== $input['sso'] ) {
  //
}

Also. Wouldn't this "unset" already "be set" if the "old config sso" is equal to the "new config sso" ? If that's the case, you can return way earlier in the method. Like:

if ($old_options['sso'] === $input['sso']){
   return $input;
}
if (! $is_sso) {
  // unset..
} else {
  //all that other stuff
}
return $input;

Anyway, a few notes:

  • I don't like multiple exit paths. The way you have this code today is mostly how I'd do it. But if that is adding a lot of indentation, conditions, etc; I'd rather exit early.
  • I don't like modifying the original parameters. However, if you're doing so (e.g. the unset call) there's no need to "return the modded version" as the changes are already contained on the variable passed as parameter in the method call. (Correct me if I'm wrong).
  • Following on the above, the method only modifies the $input variable on the ! $is_sso clause. The rest of the logic doesn't touch that var. So what's the point on returning the $input object anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the exit process changing and I like the early return, that's a standard I follow elsewhere.

I don't like modifying the original parameters.

Mentioned above but that's how this middleware works. It's a "functional" model, like a filter in other places.

So what's the point on returning the $input object anyway?

If I return nothing then the input is blank and all the values are deleted. Has to return the values that should be saved (or handed to the next validation middleware)

}

return $input;
}

Expand Down
12 changes: 9 additions & 3 deletions templates/auth0-singlelogout-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
if ( empty( $current_user->auth0_obj ) ) {
return;
}

$logout_url = wp_logout_url();
$logout_url = html_entity_decode( $logout_url );
$logout_url = add_query_arg( 'redirect_to', get_permalink(), $logout_url );
$logout_url = add_query_arg( 'SLO', 1, $logout_url );
?>
<script id="auth0" src="<?php echo esc_url( $this->a0_options->get('auth0js-cdn') ) ?>"></script>
<script type="text/javascript">
Expand All @@ -18,9 +23,10 @@
domain:'<?php echo sanitize_text_field( $this->a0_options->get( 'domain' ) ); ?>'
});

webAuth.checkSession( { 'responseType' : 'token', 'redirectUri' : window.location.href }, function ( err ) {
if ( err && err.error ==='login_required' ) {
window.location = '<?php echo esc_url( wp_logout_url( get_permalink() ) . '&SLO=1' ); ?>';
var sloOptions = { 'responseType' : 'token id_token', 'redirectUri' : '<?php echo home_url() ?>' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better if this options are obtained through a method, like the one you already have ...Lock10_Options#get_sso_options()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That method exists to check settings and decide what is passed to checkSession. In this case, we're just checking whether someone is logged in or not, which does not depend on site options. Using that method would be overly complicated, IMHO.

webAuth.checkSession( sloOptions, function ( err ) {
if ( err && err.error && 'login_required' === err.error ) {
window.location = '<?php echo $logout_url ?>';
Copy link
Contributor

Choose a reason for hiding this comment

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

if an error happens, you show the log out page. But if the session is still valid? Should you do anything with the new received tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if the session is still valid? Should you do anything with the new received tokens?

No. We're checking for an Auth0 session and, if one is not found, they get logged out. This only runs if someone is logged in so if this SLO check finds a session, they already have a WP session and nothing should change.

}
}
);
Expand Down
9 changes: 2 additions & 7 deletions templates/auth0-sso-handler-lock10.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,12 @@
});

var options = <?php echo json_encode( $lock_options->get_sso_options() ); ?>;
options.responseType = 'token id_token';
webAuth.checkSession(options
, function (err, authResult) {
webAuth.checkSession(options, function (err, authResult) {
if (typeof(authResult) === 'undefined') {
return;
}

if (typeof(authResult.code) !== 'undefined') {
window.location = '<?php echo WP_Auth0_Options::Instance()->get_wp_auth0_url(); ?>&code='
+ authResult.code + '&state=' + authResult.state;
} else if (typeof(authResult.idToken) !== 'undefined') {
if (authResult.idToken) {
jQuery(document).ready(function($){
var $form=$(document.createElement('form')).css({display:'none'}).attr("method","POST").attr("action","<?php
echo add_query_arg( 'auth0', 'implicit', site_url( 'index.php' ) ); ?>");
Expand Down