Change logged-in user redirect to login_init hook #584

joshcanhelp · 2018-11-09T18:17:40Z

Changes

PR #583 did not address all warnings that came from this change. Moving this to an existing method hooked to login_init and adding tests for this and all other functionality in the login_auto method.

References

#583

Testing

This change adds unit test coverage
This change has been tested on the latest version of the platform/language or why not

joshcanhelp · 2018-11-09T18:18:01Z

tests/testRenderForm.php

- /**
- * Run after each test.
- */
- public function tearDown() {


Moved up ☝️

joshcanhelp · 2018-11-09T18:18:13Z

tests/testRenderForm.php

- *
- * @param boolean $on - True to turn Auth0 on, false to turn off.
- */
- public static function auth0Ready( $on = true ) {


Moved to tests/traits/optionsHelpers.php 👇

joshcanhelp · 2018-11-09T18:18:29Z

tests/testRenderForm.php

- /**
- * Test that a specific region and domain return the correct number of IP addresses.
- */
- public function testThatLoggedInUserIsRedirected() {


Moved to proper test suite, tests/testLoginManager.php ☝️

joshcanhelp · 2018-11-09T18:19:25Z

tests/testLoginManager.php

+ /**
+ * Test that logged-in users are redirected from the wp-login.php page.
+ */
+ public function testThatLoggedInUserIsRedirectedFromWpLogin() {


Moved from tests/testRenderForm.php 👇

joshcanhelp · 2018-11-09T18:19:41Z

tests/testLoginManager.php

+ /**
+ * Test that the ULP redirect happens.
+ */
+ public function testUlpRedirect() {


Set everything up to make sure the ULP redirect happens under normal conditions.

joshcanhelp · 2018-11-09T18:20:27Z

lib/WP_Auth0_LoginManager.php

 || null !== $this->query_vars( 'auth0' )
- // Do not redirect if already authenticated.


Dealing with this condition in the block added above in this method ☝️

joshcanhelp · 2018-11-09T18:20:56Z

WP_Auth0.php

@@ -372,18 +372,6 @@ public function render_auth0_login_css() {
 return;
 }

- // If the user has a WP session, determine where they should end up and redirect.


Meat of the PR here. This still was not early enough to precede any HTML output, leading to a PHP warning.

joshcanhelp · 2018-11-09T18:21:50Z

lib/WP_Auth0_LoginManager.php

 ) {
- return;
+ return false;


This is used in a hook call so return value is never used ... except in tests now!

lbalmaceda · 2018-11-09T18:43:48Z

tests/testLoginManager.php

+ */
+ public static function setUpBeforeClass() {
+ parent::setUpBeforeClass();
+ self::$opts = WP_Auth0_Options::Instance();


this is done once, but its contents are changed multiple times in different tests. I think it should be moved into a method that runs "before each test" so you can ensure the tests are fully independent of each other.

e.g. some tests set the client_id property, some others don't. Independently of if those tests are running methods that use or not that client_id value from the props, if not part of the context then make sure that the props object is always with the values you need for test and nothing else. By doing what I mentioned in the paragraph above you can ensure that those props get reseted on each test run. And each test should prepare their own test context/environment prior to call the "method in test".

Totally agree but keep in mind that we always have a "state," which is the database and that's where these are stored. A new options object pulls from the test DB so it needs to be explicitly reset each time.

Also ... WP_Auth0_Options::Instance() returns an existing object with options stored in memory.

lbalmaceda · 2018-11-09T18:50:29Z

tests/testLoginManager.php

+ self::auth0Ready( false );
+ $this->stopRedirectHalting();
+ self::$error_log->clear();
+ self::$opts->set( 'custom_domain', '' );


Seems easier to throw this instance and create a new one than be changing manually each prop, as you could be forgetting some of them when adding new tests. Note this are tests and performance hit is not relevant here. Instantiate whatever you need the times you need 👍

I agree. There are a number of setUp/tearDown things that can/should happen always. Plan is to have a super class that extends the test suite to handle everything that should be done in between. I'm logging your suggestion here and the others I have so it gets handled.

lbalmaceda · 2018-11-09T18:52:11Z

tests/testLoginManager.php

+ * Test that logged-in users are redirected from the wp-login.php page.
+ */
+ public function testThatLoggedInUserIsRedirectedFromWpLogin() {
+ $this->startRedirectHalting();


This is what I didn't fully understand from yesterday's response. Couldn't you call this line at the very end of the "run before each test" method? It's cleaner than having to copy-paste it on every test.

Cleaner but not needed for every test. Some of these cannot be combined because they throw exceptions at different places.

lbalmaceda · 2018-11-09T18:55:13Z

tests/testLoginManager.php

+ $this->assertFalse( $login_manager->login_auto() );
+ $_SERVER['REQUEST_METHOD'] = $req_method_backup;
+
+ // Test that WP login override will skip the redirect.


All these 3 checks below should be moved into 3 different tests as the condition for the test changes.

lbalmaceda · 2018-11-09T18:57:02Z

tests/traits/optionsHelpers.php

+ */
+ public static function auth0Ready( $on = true ) {
+ $value = $on ? uniqid() : null;
+ self::$opts->set( 'domain', $value );


sounds odd, even for a test, that a domain's test value is not a url shaped string but a random string.

The value is not used or checked anywhere so it's irrelevant. In this way, I can set this to something to make the plugin "active" and if I ever try to check for a specific value, it will fail (correctly).

codecov-io · 2018-11-09T20:11:57Z

Codecov Report

Merging #584 into master will increase coverage by 0.74%.
The diff coverage is 94.11%.

@@             Coverage Diff              @@
##             master     #584      +/-   ##
============================================
+ Coverage     27.76%   28.51%   +0.74%     
+ Complexity     1307     1306       -1     
============================================
  Files            51       51              
  Lines          4200     4202       +2     
============================================
+ Hits           1166     1198      +32     
+ Misses         3034     3004      -30

Impacted Files	Coverage Δ	Complexity Δ
WP_Auth0.php	`22.36% <ø> (-2.64%)`	`62 <0> (-2)`
lib/WP_Auth0_LoginManager.php	`20% <94.11%> (+9.88%)`	`121 <0> (+1)`	⬆️
lib/WP_Auth0_Lock10_Options.php	`69.64% <0%> (-4.47%)`	`61% <0%> (ø)`
lib/WP_Auth0_Nonce_Handler.php	`71.05% <0%> (+23.68%)`	`18% <0%> (ø)`	⬇️

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 7324a03...d534ae8. Read the comment docs.

joshcanhelp commented Nov 9, 2018

View reviewed changes

tests/testRenderForm.php

/**

* Run after each test.

*/

public function tearDown() {

Copy link

Contributor Author

joshcanhelp Nov 9, 2018

Choose a reason for hiding this comment

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

Moved up ☝️

joshcanhelp commented Nov 9, 2018

View reviewed changes

joshcanhelp added this to the 3.8.1 milestone Nov 9, 2018

joshcanhelp added the CH: Changed label Nov 9, 2018

Change login redirect to proper action; tests

896c03b

joshcanhelp force-pushed the change-logged-in-redirect branch from 255403e to 896c03b Compare November 9, 2018 18:25

lbalmaceda reviewed Nov 9, 2018

View reviewed changes

joshcanhelp added 2 commits November 9, 2018 11:50

PR feedback

75d4ed1

Refactor login_auto for clarity

d534ae8

auth0 deleted a comment from codecov-io Nov 9, 2018

lbalmaceda approved these changes Nov 12, 2018

View reviewed changes

joshcanhelp merged commit 6126a34 into master Nov 13, 2018

joshcanhelp deleted the change-logged-in-redirect branch November 13, 2018 14:24

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

Change logged-in user redirect to login_init hook #584

Change logged-in user redirect to login_init hook #584

joshcanhelp commented Nov 9, 2018 •

edited

Loading

joshcanhelp Nov 9, 2018

joshcanhelp Nov 9, 2018

joshcanhelp Nov 9, 2018 •

edited

Loading

joshcanhelp Nov 9, 2018

joshcanhelp Nov 9, 2018

joshcanhelp Nov 9, 2018

joshcanhelp Nov 9, 2018

joshcanhelp Nov 9, 2018 •

edited

Loading

lbalmaceda Nov 9, 2018

joshcanhelp Nov 9, 2018 •

edited

Loading

joshcanhelp Nov 9, 2018

lbalmaceda Nov 9, 2018

joshcanhelp Nov 9, 2018

lbalmaceda Nov 9, 2018

joshcanhelp Nov 9, 2018

lbalmaceda Nov 9, 2018

lbalmaceda Nov 9, 2018

joshcanhelp Nov 9, 2018

codecov-io commented Nov 9, 2018

		\|\| null !== $this->query_vars( 'auth0' )
		// Do not redirect if already authenticated.

Change logged-in user redirect to login_init hook #584

Change logged-in user redirect to login_init hook #584

Conversation

joshcanhelp commented Nov 9, 2018 • edited Loading

Changes

References

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp Nov 9, 2018 • edited Loading

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

joshcanhelp Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp Nov 9, 2018 • edited Loading

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

codecov-io commented Nov 9, 2018

Codecov Report

joshcanhelp commented Nov 9, 2018 •

edited

Loading

joshcanhelp Nov 9, 2018 •

edited

Loading

joshcanhelp Nov 9, 2018 •

edited

Loading

joshcanhelp Nov 9, 2018 •

edited

Loading