WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 10 hours ago

#46907 reopened enhancement

Pass current request object to rest_authentication_errors filter

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

Description

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 5 months ago.

Download all attachments as: .zip

Change History (16)

@ocean90
5 months ago

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


5 months ago

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


4 months ago

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


8 weeks ago

#4 @ocean90
8 weeks 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
8 weeks 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
8 weeks ago

#47792 was marked as a duplicate.

#7 @ocean90
8 weeks ago

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

#8 @kadamwhite
7 weeks 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.


5 weeks ago

#11 @kadamwhite
5 weeks 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
3 weeks 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 3 weeks ago by itowhid06 (previous) (diff)

#13 @kadamwhite
11 hours 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
10 hours 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
10 hours ago

  • Milestone changed from 5.3 to Future Release
Note: See TracTickets for help on using tickets.