Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move SSO checking into Lock init #570

Merged
merged 3 commits into from
Oct 24, 2018

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Oct 19, 2018

Changes

Moving the SSO processing from a separately-included HTML template into the Lock init JS file.

  • Remove templates/auth0-sso-handler-lock10.php file
  • Move inline JS from the above to assets/js/lock-init.js to hide the Lock form until the SSO check is complete
  • Redirect users from the wp-login.php page if they are already logged in

References

Closes #508
Closes #414

Testing

Testing steps:

  1. Turn on SSO in the Auth0 settings page
  2. Clear out all WordPress auth cookies
  3. Visit the wp-login.php page

Expected

No Lock form shows and you're redirected to the default login link with a WP session and no JS console errors.

  1. Visit the wp-login.php page again

You're redirected to the default login link with the WP session intact.

  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform

Checklist

  • All existing and new tests complete without errors
  • All code quality tools/guidelines in the Contribution guide have been run/followed
  • All relevant assets have been compiled as directed in the Contribution guide, if applicable
  • All active GitHub CI checks have passed

@joshcanhelp joshcanhelp force-pushed the combine-sso-check-with-lock-init branch 2 times, most recently from 39ba35c to cb232fb Compare October 19, 2018 22:26
@@ -372,6 +372,19 @@ public function render_form( $html ) {
return $html;
}

// If the user has a WP session, determine where they should end up and redirect.
Copy link
Contributor Author

@joshcanhelp joshcanhelp Oct 19, 2018

Choose a reason for hiding this comment

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

Don't show a blank, unhelpful login page if the user is already logged in (#414).

@@ -134,13 +134,8 @@ protected function build_settings( $settings ) {

public function get_sso_options() {
$options['scope'] = WP_Auth0_LoginManager::get_userinfo_scope( '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.

This is only ever used for SSO, which uses the hybrid auth (modified implicit) so there's never a need for a code callback processing.

add_action( 'wp_footer', array( $this, 'auth0_singlelogout_footer' ) );
add_action( 'admin_footer', array( $this, 'auth0_singlelogout_footer' ) );
add_action( 'login_footer', array( $this, 'auth0_singlelogout_footer' ) );
add_action( 'wp_login', array( $this, 'end_session' ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecated in #557 but not removed from the login page.

@@ -90,11 +90,9 @@ public function init() {
add_action( 'login_init', array( $this, 'login_auto' ) );
add_action( 'template_redirect', array( $this, 'init_auth0' ), 1 );
add_action( 'wp_logout', array( $this, 'logout' ) );
add_filter( 'login_message', array( $this, 'auth0_sso_footer' ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removes the existing SSO template templates/auth0-sso-handler-lock10.php

@joshcanhelp joshcanhelp force-pushed the combine-sso-check-with-lock-init branch from cb232fb to 0288c66 Compare October 19, 2018 23:09
@joshcanhelp joshcanhelp added this to the 3.8.0 milestone Oct 19, 2018
@joshcanhelp joshcanhelp changed the title [WIP] Move SSO checking into Lock init Move SSO checking into Lock init Oct 19, 2018
WP_Auth0.php Outdated
@@ -372,6 +372,18 @@ public function render_form( $html ) {
return $html;
}

// If the user has a WP session, determine where they should end up and redirect.
if ( is_user_logged_in() ) {
$login_redirect = ! empty( $_REQUEST['redirect_to'] ) ?
Copy link
Member

Choose a reason for hiding this comment

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

I think in logic usually best to avoid extra operands, you don't need the ! if you switch the Ternary values.

@auth0 auth0 deleted a comment from codecov-io Oct 23, 2018
@joshcanhelp
Copy link
Contributor Author

@cocojoe - Fixed up, tinkered with WP_Auth0 a bit to assist with testing, and added tests 👍

@auth0 auth0 deleted a comment from codecov-io Oct 23, 2018
@codecov-io
Copy link

Codecov Report

Merging #570 into master will increase coverage by 4.19%.
The diff coverage is 73.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #570      +/-   ##
============================================
+ Coverage     15.75%   19.95%   +4.19%     
+ Complexity     1541     1416     -125     
============================================
  Files            66       53      -13     
  Lines          5320     4541     -779     
============================================
+ Hits            838      906      +68     
+ Misses         4482     3635     -847
Impacted Files Coverage Δ Complexity Δ
lib/WP_Auth0_LoginManager.php 10.02% <ø> (+0.05%) 124 <0> (ø) ⬇️
lib/WP_Auth0_Lock10_Options.php 53.2% <100%> (+11.59%) 77 <0> (-1) ⬇️
WP_Auth0.php 20.55% <60%> (ø) 64 <4> (?)
...rd-widgets/WP_Auth0_Dashboard_Plugins_Location.php
lib/php-jwt/tests/JWTTest.php
...oard-widgets/WP_Auth0_Dashboard_Plugins_Gender.php
...ard-widgets/WP_Auth0_Dashboard_Plugins_#s.php
...b/dashboard-widgets/WP_Auth0_Dashboard_Widgets.php
lib/php-jwt/Authentication/JWT.php
lib/php-jwt/tests/bootstrap.php
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaf84c8...abd0099. Read the comment docs.

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

LGTM Also good bump in test coverage.

@joshcanhelp
Copy link
Contributor Author

@cocojoe - Bump is kind of fake as I removed a few external libraries from the test calculation ... don't tell anyone!

@joshcanhelp joshcanhelp merged commit 12de60c into master Oct 24, 2018
@joshcanhelp joshcanhelp deleted the combine-sso-check-with-lock-init branch October 24, 2018 14:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants