Fix logout process #453

joshcanhelp · 2018-05-01T17:23:09Z

Remove federated logout for SSO (heavy-handed and was causing issues with some users)
Remove end_session method call (we're not setting any sessions so we should not be deleting any either)
Fix telemetry header issues (fixes Encode auth0Client in login redirect URL #448)
Fix SLO link empty issue (get_permalink() is empty on non-post pages)
Fix improper filter handling in auth0_sso_footer (cannot echo in filters, must return modified data)
Add no_sso check for SSO footer on wp-login.php to skip SSO processing if there was a login error (can cause redirect loops)
Minor code quality fixes (calling out internal methods

lbalmaceda · 2018-05-03T15:14:14Z

lib/WP_Auth0_LoginManager.php

+				$this->a0_options->get( 'domain' ),
+				rawurlencode( home_url() ),
+				$this->a0_options->get( 'client_id' ),
+				$telemetry_headers['Auth0-Client']


I can't find any GH issues created for refactoring this usage. Are you tracking this?

Yes, it's in Jira

lbalmaceda · 2018-05-03T15:16:05Z

lib/WP_Auth0_LoginManager.php

+			} else {
+				$redirect_url = home_url();
+			}
+			wp_redirect( $redirect_url );
 			exit;


at this line $is_sso and $is_auto_login were not used and the function is already returning. Would it make sense to define those variables right above the line that uses it? Would that help this code perform better?

I think that's less clear and $this->a0_options->get() is just returning an array value so I don't think optimization there is helpful.

lbalmaceda · 2018-05-03T15:16:51Z

lib/WP_Auth0_LoginManager.php

 			exit;
 		}

-		if ( $auto_login ) {
+		// If auto-login is in use, cannot redirect back to login page.


"login page" would be the "authorize page" / HLP?

No, this is the WP login page. Auto-login forwards to /authorize with a connection param so we need to drop folks logging out somewhere else.

lbalmaceda · 2018-05-03T15:18:37Z

lib/WP_Auth0_LoginManager.php

-			return;
+		// No need to checkSession if already logged in.
+		// URL parameter `no_sso` is set to skip checkSession.
+		if ( is_user_logged_in() || ! empty( $_GET['no_sso'] ) || ! $this->a0_options->get( 'sso' ) ) {


is like the negative (!) of the negative (empty) of the negative (no_). Should this be extracted to a variable for readability ?

lbalmaceda · 2018-05-03T15:21:48Z

lib/WP_Auth0_LoginManager.php

-			include WPA0_PLUGIN_DIR . 'templates/auth0-sso-handler-lock10.php';
-		}
+		wp_enqueue_script( 'wpa0_auth0js', $this->a0_options->get( 'auth0js-cdn' ) );
+		ob_start();


what's this? Can't find it in this diff. Also the one ob_get_clean()

ob_start() is output buffer, ob_get_clean() returns the buffer and clears it out. This is used to "capture" an echo basically

So you're creating a new buffer and putting in the contents of the 'templates/auth0-sso-handler-lock10.php' file, and returning $previous_html plus the buffer contents?

Correct. I would rather this be in an external JS file since it's doing something similar to the implicit flow we have as part of the plugin but this is all being removed on the next minor in favor of UL so I don't want to waste time on it.

lbalmaceda · 2018-05-03T15:22:48Z

lib/WP_Auth0_LoginManager.php

@@ -578,8 +588,6 @@ public function auth0_singlelogout_footer() {

 	/**
 	 * End the PHP session.
-	 *
-	 * TODO: Remove, not necessary!


this method is necessary now or what?

Should be deprecated, added that comment below 👇

lbalmaceda · 2018-05-03T15:24:13Z

lib/WP_Auth0_LoginManager.php

@@ -664,7 +672,7 @@ protected function die_on_login( $msg = '', $code = 0, $login_link = true ) {
 					: __( 'Please see the site administrator', 'wp-auth0' ),
 				__( 'error code', 'wp-auth0' ),
 				$code ? sanitize_text_field( $code ) : __( 'unknown', 'wp-auth0' ),
-				$login_link ? wp_login_url() : wp_logout_url(),
+				$login_link ? add_query_arg( 'no_sso', 1, wp_login_url() ) : wp_logout_url(),


so add_query_arg will return the modified URL?

Remove federated logout for SSO. Fix SLO link empty issue. Minor code quality fixes.

joshcanhelp · 2018-05-03T17:04:31Z

lib/WP_Auth0_LoginManager.php

-			return;
+		// No need to checkSession if already logged in.
+		// URL parameter `skip_sso` is set to skip checkSession.
+		if ( is_user_logged_in() || isset( $_GET['skip_sso'] ) || ! $this->a0_options->get( 'sso' ) ) {


is like the negative (!) of the negative (empty) of the negative (no_). Should this be extracted to a variable for readability ?

I liked your suggestion here so I changed the param name and now just checking if it's there.

lbalmaceda

LGTM

joshcanhelp added CH: Fixed labels May 1, 2018

joshcanhelp added this to the v3-Next milestone May 1, 2018

lbalmaceda reviewed May 3, 2018

View reviewed changes

joshcanhelp added 2 commits May 3, 2018 09:54

Fix logout process

328bf68

Remove federated logout for SSO. Fix SLO link empty issue. Minor code quality fixes.

phpcs/phpcbf pass

a598553

joshcanhelp force-pushed the fix-logout-process branch from 3408d7f to a598553 Compare May 3, 2018 16:55

joshcanhelp added 2 commits May 3, 2018 10:01

More clear SSO skip URL parameter

9901d7c

Deprecate comment

b2a3c59

joshcanhelp commented May 3, 2018

View reviewed changes

lbalmaceda approved these changes May 3, 2018

View reviewed changes

joshcanhelp merged commit 56ff379 into dev May 3, 2018

joshcanhelp deleted the fix-logout-process branch May 3, 2018 17:45

joshcanhelp removed the Ready for Review label May 3, 2018

This was referenced May 3, 2018

Encode auth0Client in login redirect URL #448

Closed

Logout does not work when Wordpress is locked down (private site) #39

Closed

This was referenced Jun 4, 2018

Expose a configurable toggle that allows Users to state if federated logout should be used #471

Closed

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 logout process #453

Fix logout process #453

joshcanhelp commented May 1, 2018

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda May 3, 2018

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda left a comment

Fix logout process #453

Fix logout process #453

Conversation

joshcanhelp commented May 1, 2018

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

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment