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

joshcanhelp · 2018-05-22T21:26:54Z

The SLO functionality (loads a checkSession call on front-end pages if the user is logged in) was using an incorrect redirectUri value and also redirected to a malformed logout link (prompted the user to logout instead of doing it automatically). Also:

Hide and turn off SLO if SSO is not on (fixes SSO disabled, Single Logout enabled causes users to get logged out automatically a few seconds after logging in #336)
Removed unnecessary code check in SSO checkSession call
More clear responseType handling
Use client credential grant to update Application instead of likely-expired app token

lbalmaceda

🦅

lbalmaceda · 2018-05-23T20:50:05Z

lib/admin/WP_Auth0_Admin_Features.php

@@ -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' );


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

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?

lbalmaceda · 2018-05-24T17:27:59Z

templates/auth0-singlelogout-handler.php

-    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() ?>' };


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

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.

lbalmaceda · 2018-05-24T17:29:42Z

templates/auth0-singlelogout-handler.php

+    var sloOptions = { 'responseType' : 'token id_token', 'redirectUri' : '<?php echo home_url() ?>' };
+    webAuth.checkSession( sloOptions, function ( err ) {
+            if ( err && err.error && 'login_required' === err.error ) {
+                window.location = '<?php echo $logout_url ?>';


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?

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.

lbalmaceda · 2018-05-24T17:47:37Z

lib/admin/WP_Auth0_Admin_Features.php

-				$error .= '<a href="https://auth0.com/docs/sso/single-sign-on#1">HERE</a>.';
-				$this->add_validation_error( $error );
-
+		$is_sso = ! empty( $input['sso'] );


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.

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.

lbalmaceda · 2018-05-24T17:48:45Z

lib/admin/WP_Auth0_Admin_Features.php

+	 * @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 );


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']

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)

lbalmaceda · 2018-05-24T17:55:14Z

lib/admin/WP_Auth0_Admin_Features.php

+			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;


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

$app_update_failed = ! $update_result

lbalmaceda · 2018-05-24T17:59:57Z

lib/admin/WP_Auth0_Admin_Features.php

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


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?

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)

Fixing SLO redirect and SSO setting push to dashboard

3bc2f09

joshcanhelp added CH: Fixed labels May 22, 2018

joshcanhelp added this to the 3.6.0 milestone May 22, 2018

lbalmaceda reviewed May 24, 2018

View reviewed changes

lbalmaceda approved these changes May 24, 2018

View reviewed changes

Changes based on PR review feedback

9aa938b

joshcanhelp force-pushed the fix-slo-sso-templates branch from 0906727 to 9aa938b Compare May 24, 2018 19:44

joshcanhelp removed the Ready for Review label May 24, 2018

joshcanhelp merged commit 21e6092 into dev May 24, 2018

joshcanhelp deleted the fix-slo-sso-templates branch May 24, 2018 19:45

joshcanhelp mentioned this pull request May 24, 2018

SSO disabled, Single Logout enabled causes users to get logged out automatically a few seconds after logging in #336

Closed

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

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

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

joshcanhelp commented May 22, 2018

lbalmaceda left a comment

lbalmaceda May 23, 2018

joshcanhelp May 24, 2018

lbalmaceda May 24, 2018

joshcanhelp May 24, 2018

lbalmaceda May 24, 2018

joshcanhelp May 24, 2018

lbalmaceda May 24, 2018

joshcanhelp May 24, 2018

lbalmaceda May 24, 2018

joshcanhelp May 24, 2018

lbalmaceda May 24, 2018

lbalmaceda May 24, 2018

joshcanhelp May 24, 2018

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

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

Conversation

joshcanhelp commented May 22, 2018

lbalmaceda left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment