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.

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

Fix SLO callback URL #479

Merged
merged 3 commits into from
Jun 6, 2018
Merged

Fix SLO callback URL #479

merged 3 commits into from
Jun 6, 2018

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Jun 6, 2018

Reported here - https://wordpress.org/support/topic/sso-logout-issues/

SLO is using home_url(), which some sites don't have in the "Allowed Callback URLs" field for the Application. Changed to the main Auth0 callback URL which is always there. Callback is not used as a redirect so does not need to change for Implicit, just needs to be valid.

Also adds SLO check to the admin + login pages and changes the redirect URL used after logout to point to an external page if there is one or the wp-login.php page.

@joshcanhelp joshcanhelp added this to the v3-next milestone Jun 6, 2018
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

After reading the PR description, I'm convinced this is a user configuration issue. If they want SLO, they must set the home_url parameter. This is not broken IMO.

@@ -6,7 +6,7 @@

$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( 'redirect_to', ( get_the_ID() ? get_permalink() : wp_login_url() ), $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.

please explain the logic behind this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_the_ID() checks if we're on a page/post, which means get_permalink() will return something useful. If not, we send them to the login page where Lock will appear.

@@ -23,7 +23,10 @@
domain:'<?php echo sanitize_text_field( $this->a0_options->get( 'domain' ) ); ?>'
});

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.

can this be considered a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. checkSession just needs a valid callback, it doesn't actually send the browser there.

@joshcanhelp
Copy link
Contributor Author

@lbalmaceda - home_url() always returns something but what it returns is not always something that /authorize will accept (i.e. it's not always set in the "Allowed Callback URLs"). We have a URL we can use that's always there (saved when the App is created) so it's better to use that.

@lbalmaceda
Copy link
Contributor

Alright. I'm not so convinced though. Like I said, this looks like something we should "help fix" from the documentation side. A good doc on "how to achieve SLO" would mention this url needs to be set as a requirement, here and whitelisted in the tenant. That's what most of the SDKs do AFAIK. But again, I'm not going to block you for this. Your call :)

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Left 1 comment ☝️ but LGTM

@joshcanhelp joshcanhelp merged commit dceca55 into dev Jun 6, 2018
@joshcanhelp joshcanhelp deleted the fix-slo-callback branch June 6, 2018 18:49
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants