Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#56971 closed enhancement (fixed)

Add context to send_auth_cookies

Reported by: dd32's profile dd32 Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch commit
Focuses: Cc:

Description

The send_auth_cookies filter was introduced in #39367 for unit testing, but it has other benefits in the authentication flow.

However, this filter doesn't pass any context, making it a lot less useful for conditional usage.

The attached PR will add $user_id as context to the filter, which at least allows for the filter to skip sending auth cookies.

Change History (24)

This ticket was mentioned in PR #3554 on WordPress/wordpress-develop by @dd32.


2 years ago
#1

  • Keywords has-patch added

#2 @mukesh27
2 years ago

  • Keywords changes-requested added
  • Milestone changed from Awaiting Review to 6.2

Thanks @dd32 for ticket and PR. Left one feedback.

Added milestone 6.2 for visibility.

@peterwilsoncc commented on PR #3554:


2 years ago
#3

In instances were these functions are replaced, it's possible the filter will exist with only a single parameter.

Is it worth documenting that developers should account for that somehow when using the second parameter? In all honesty, you probably have a better idea of the history with such changes than I do!

(Please read this as a question rather than a passive aggressive change request. I think you know by now I'd just ask if I thought it needed changes.)

@dd32 commented on PR #3554:


2 years ago
#4

Is it worth documenting that developers should account for that somehow when using the second parameter?

Given it's part of the authentication flow, I would anticipate that anyone who is trying to use it should be aware of validation of parameters. I added the @since for that purpose.

The same goes for any filter that ever that has had additional parameters be set. So I don't see it as being something that needs to be called out any more than the @since.

#5 @costdev
22 months ago

  • Keywords changes-requested removed

Looks like the changes requested by @mukesh27 have been made. Removing the changes-requested keyword.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


22 months ago

@dd32 commented on PR #3554:


22 months ago
#7

Resolved the changes mentioned above in 55989da. If a currently active committer would like to run with this, go for it!

#8 @mukesh27
22 months ago

  • Keywords commit added

Thanks @dd32 for ticket and PR.

This ticket was discussed in the recent bug scrub.

The PR 3554 got enough approval to commit. Adding commit for consideration

Props to @costdev

#9 @audrasjb
22 months ago

  • Owner set to audrasjb
  • Status changed from new to accepted

Thanks everyone, self assigning for commit.

#10 @audrasjb
22 months ago

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

In 55164:

Users: Add context to the send_auth_cookies filter.

This changeset adds $user_id, $expire, $expiration and $token parameters to provide context to send_auth_cookies hook, which allows the filter to skip sending auth cookies.

Props dd32, mukesh27, costdev, peterwilsoncc, audrasjb.
Fixes #56971.
See #39367.

#12 @SergeyBiryukov
22 months ago

In 55165:

Docs: Remove a duplicate line in the send_auth_cookies filter DocBlock.

Describe the default values for the $send and $expire parameters.

Follow-up to [55164].

See #56971, #39367.

#13 @SergeyBiryukov
22 months ago

In 55166:

Docs: Fix typo in the send_auth_cookies filter DocBlock.

Follow-up to [55164], [55165].

See #56971, #39367.

#14 @SergeyBiryukov
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the commit!

Just thinking, should the order of these new parameters be consistent with that of the set_auth_cookie and set_logged_in_cookie actions above? Like this:

if ( ! apply_filters( 'send_auth_cookies', true, $expire, $expiration, $user_id, $scheme, $token ) ) {

instead of this:

if ( ! apply_filters( 'send_auth_cookies', true, $user_id, $expire, $expiration, $token ) ) {

Or is the current order more preferable?

#15 @audrasjb
22 months ago

  • Keywords has-patch commit removed

Thanks for the follow-up commits!

I think the order of the parameters doesn't really matter, so let's make them consistent with other functions

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


22 months ago

This ticket was mentioned in PR #4017 on WordPress/wordpress-develop by @mukesh27.


22 months ago
#17

  • Keywords has-patch added

#18 @costdev
22 months ago

  • Keywords commit added

@mukesh27 submitted PR 4017 to address @audrasjb's feedback. This looks ready for commit consideration.

@audrasjb commented on PR #4017:


22 months ago
#19

I added another commit to this PR to also change the order in the since mention.

@mukesh27 commented on PR #4017:


22 months ago
#20

Thanks @audrasjb for update.

#21 @audrasjb
22 months ago

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

In 55253:

Users: Change parameters order in send_auth_cookies filter.

This changeset makes this filter more consistent with set_auth_cookie and set_logged_in_cookie hooks.

Follow-up to [55164].

Props SergeyBiryukov, audrasjb, mukesh27 , costdev.
Fixes #56971.

#23 @SergeyBiryukov
22 months ago

In 55259:

Users: Pass the authentication scheme to the send_auth_cookies filter.

This brings more consistency with the set_auth_cookie and set_logged_in_cookie hooks.

Follow-up to [55164], [55253].

See #56971.

#24 @SergeyBiryukov
22 months ago

In 55260:

Users: Pass correct number of arguments to send_auth_cookies filter in wp_clear_auth_cookie().

Follow-up to [55164], [55253], [55259].

Props mukesh27.
See #56971.

Note: See TracTickets for help on using tickets.