WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 9 months ago

#39367 closed defect (bug) (fixed)

Don't no-op $user_id in test suite's wp_set_auth_cookie()

Reported by: danielbachhuber Owned by: johnbillion
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.7
Component: Build/Test Tools Keywords: has-patch fixed-major
Focuses: Cc:

Description

The implementation of wp_set_auth_cookie() in the test suite sets $user_id=null, when it should respect the $user_id passed to the function.

If I pass a user id to wp_set_auth_cookie(), I'd expect the user id to be then passed through to wp_set_auth_cookie()'s actions.

Discovered in https://github.com/pantheon-systems/wp-saml-auth/pull/48

From r39219

Attachments (3)

39367.1.diff (558 bytes) - added by danielbachhuber 11 months ago.
39367.diff (2.7 KB) - added by johnbillion 9 months ago.
39367.2.diff (11.6 KB) - added by johnbillion 9 months ago.

Download all attachments as: .zip

Change History (23)

#1 @danielbachhuber
11 months ago

@rmccue @jnylen0 Do you recall the origin of $user_id=null?

#2 @jnylen0
11 months ago

That was just the bare minimum necessary to get the new tests to pass for changing a user's password. I see no problem with changing it as needed.

#3 @danielbachhuber
11 months ago

  • Keywords has-patch added; dev-feedback removed
  • Milestone changed from Future Release to 4.8

#4 follow-up: @johnbillion
11 months ago

  • Keywords needs-patch added; has-patch removed

Overriding this function in the test suite is asking for trouble. The pluggable function should be refactored to make it testable.

#5 in reply to: ↑ 4 ; follow-up: @jnylen0
11 months ago

Replying to johnbillion:

Overriding this function in the test suite is asking for trouble.

What kind of trouble would you expect to see?

The pluggable function should be refactored to make it testable.

I think this could be done later, and the simple patch here is probably fine for this ticket - the function is already being overridden in trunk and 4.7.

#6 in reply to: ↑ 5 @danielbachhuber
10 months ago

Replying to jnylen0:

Replying to johnbillion:

Overriding this function in the test suite is asking for trouble.

What kind of trouble would you expect to see?

I actually ran into the trouble when I opened the issue originally. WP SAML Auth defines wp_set_auth_cookie() in its bootstrap.php to overload the behavior of wp_set_auth_cookie() in pluggable.php. When core's test suite began to define wp_set_auth_cookie() too, it caused fatal errors in WP SAML Auth's tests.

I agree with @johnbillion that defining pluggable functions in core's test suite is less than ideal.

The pluggable function should be refactored to make it testable.

I think this could be done later, and the simple patch here is probably fine for this ticket - the function is already being overridden in trunk and 4.7.

We should undo the function being overridden sooner rather than later.

@johnbillion
9 months ago

#7 follow-up: @johnbillion
9 months ago

  • Keywords has-patch added; needs-patch removed

39367.diff adds a filter to wp_set_auth_cookie() and wp_clear_auth_cookie() which allows the actual output of the cookies to be prevented, while retaining the various logic included with them to run, and the hooks to fire.

Thoughts?

#8 @johnbillion
9 months ago

Also worth pointing out that the change in [39219] also broke the tests for my User Switching plugin because of the no-oped values in the overridden wp_set_auth_cookie() function.

#9 in reply to: ↑ 7 @jnylen0
9 months ago

Replying to johnbillion:

Thoughts?

39367.diff looks great to me 👍🏻

Do we want to get this change into a point release? Seems like we broke the tests for a few plugins.

#10 @johnbillion
9 months ago

  • Milestone changed from 4.8 to 4.7.4

Just what I was thinking :-)

#11 @johnbillion
9 months ago

Here's my proposal.

For trunk and the 4.7 branch:

  • Remove the overridden wp_set_auth_cookie() and wp_clear_auth_cookie() functions from the test suite.

For the 4.7 branch only:

  • Remove the tests which use WP_Test_REST_Users_Controller::verify_user_roundtrip(), as these are the tests which result in the "headers already sent" error unless the auth cookie functions are overridden.

For trunk:

@johnbillion
9 months ago

#12 @johnbillion
9 months ago

39367.2.diff is a patch for the 4.7 branch.

#13 @johnbillion
9 months ago

  • Milestone changed from 4.7.4 to 4.7.3

#14 @jnylen0
9 months ago

@johnbillion I don't think we should put this in 4.7.3 for several reasons.

  • It's very late in the cycle - the milestone is clear except for this ticket, and we need to do an RC soon.
  • We're making changes to the users endpoint in 4.7.3 (see #39701). While I'm not aware of anything that would break, I'd prefer to leave the related tests in place.
  • I'd rather wait a week or two and treat trunk and 4.7 the same way, avoiding divergence between branches and the resulting extra work.

#15 @johnbillion
9 months ago

  • Milestone changed from 4.7.3 to 4.7.4

Agreed.

#16 @johnbillion
9 months ago

  • Version set to 4.7

#17 @johnbillion
9 months ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 40263:

Build/Test tools: Don't override the wp_set_auth_cookie() and wp_clear_auth_cookie() functions.

Overriding pluggable functions in the test suite is asking for trouble in the future. In addition, it means the test suite can't be guaranteed to behave the same as core.

This instead introduces a send_auth_cookies filter which can be hooked in during the test suite to prevent these functions from attempting to send cookie headers to the client.

Fixes #39367

#18 @johnbillion
9 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#19 @johnbillion
9 months ago

In 40264:

Docs: Remove the duplicate hook documentation for the newly introduced send_auth_cookies filter.

See #39367

#20 @johnbillion
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40265:

Build/Test tools: Don't override the wp_set_auth_cookie() and wp_clear_auth_cookie() functions.

Overriding pluggable functions in the test suite is asking for trouble in the future. In addition, it means the test suite can't be guaranteed to behave the same as core.

This instead introduces a send_auth_cookies filter which can be hooked in during the test suite to prevent these functions from attempting to send cookie headers to the client.

Fixes #39367

Merges [40263] and [40264] to the 4.7 branch.

Note: See TracTickets for help on using tickets.