Make WordPress Core

Opened 20 months ago

Closed 5 months ago

#46907 closed enhancement (wontfix)

Pass current request object to rest_authentication_errors filter

Reported by: ocean90 Owned by: ocean90
Milestone: Priority: normal
Severity: normal Version:
Component: REST API Keywords: needs-patch
Focuses: rest-api Cc:


If you hook into rest_authentication_errors and need to check the used route or method you currently have to do the same handling which WP_REST_Server::serve_request() has already done before WP_REST_Server::check_authentication() is called.
To avoid this I'm proposing to pass $request as a parameter to WP_REST_Server::check_authentication() and the rest_authentication_errors filter.

Attachments (1)

46907.diff (1.8 KB) - added by ocean90 20 months ago.

Download all attachments as: .zip

Change History (17)

20 months ago

This ticket was mentioned in Slack in #core-restapi by ocean90. View the logs.

20 months ago

This ticket was mentioned in Slack in #core-restapi by espellcaste. View the logs.

19 months ago

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.

17 months ago

#4 @ocean90
16 months ago

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

In 45687:

REST API: Pass current request object to rest_authentication_errors filter in WP_REST_Server::check_authentication().

Fixes #46907.

#5 @TimothyBlynJacobs
16 months ago

I added this in the Slack scrub.

This is tricky. I’ve always thought that the request object was omitted intentionally, since the purpose of the authentication hook is to set the current user for the request and let permission checks dictate everything else. However, I think it is at least somewhat common for plugins that make use of the REST API to provide custom authentication schemes that only work for their endpoints.

I think feedback from the brain trust would be quite helpful here @kadamwhite.

I think at one point I tried to find the original discussion on the filter, but IIRC it originated in the GSOC project and I couldn’t really find much more written about the design of it.

Regardless, should the $request parameter be optional here to maintain BC if someone is calling the method directly? It is a public method, and could've been protected or private, so it would appear to developers to be safe to call.

#6 @ocean90
16 months ago

#47792 was marked as a duplicate.

#7 @ocean90
16 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#8 @kadamwhite
16 months ago

@ocean90 Can you provide a concrete example of functionality which is prevented by omitting the request argument? I'm not sure I entirely understand why we need to be able to determine request context here, it feels like a separate concern to authentication.

Additionally, while I know we're slow to respond on these issues sometimes I'm somewhat surprised that you landed this without a +1 from a component maintainer.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.

16 months ago

#11 @kadamwhite
16 months ago

Discussed again in ticket scrub today; I think the issue here is that we're using a strange pattern in the JWT plugin, and not that the logic here is incorrect. The authentication check should just be determining the current user. I'm inclined to revert this.

#12 @itowhid06
15 months ago

What I was about to say has been told. I think public function check_authentication( $request = null ) should mitigate the Backward Compatibility issue.

Last edited 15 months ago by itowhid06 (previous) (diff)

#13 @kadamwhite
15 months ago

In 46191:

REST API: Revert [45687].

This change may not be needed and further investigation is required before we accept it into a release.

See #46907.

#14 @kadamwhite
15 months ago

As discussed above and in slack, I have reverted this for 5.3. This may be an over-conservative abundance of caution but I'd like to talk through the JWT auth flow in greater detail before we proceed.

#15 @kadamwhite
15 months ago

  • Milestone changed from 5.3 to Future Release

#16 @ocean90
5 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.