Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 2 years ago

#46907 closed enhancement (wontfix)

Pass current request object to rest_authentication_errors filter

Reported by: ocean90's profile ocean90 Owned by: ocean90's profile ocean90
Milestone: 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 years ago.

Download all attachments as: .zip

Change History (18)

@ocean90
5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

#4 @ocean90
5 years 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
5 years 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
5 years ago

#47792 was marked as a duplicate.

#7 @ocean90
5 years ago

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

#8 @kadamwhite
5 years 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 years ago

#11 @kadamwhite
5 years 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
5 years ago

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

Version 0, edited 5 years ago by itowhid06 (next)

#13 @kadamwhite
5 years 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
5 years 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
5 years ago

  • Milestone changed from 5.3 to Future Release

#16 @ocean90
4 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

#17 @kakomap
2 years ago

Hi here,
I've followed this and I'd like to add a use-case where the $request might be useful. A plugin might, like in my case, want to have the REST API restricted for all API requests except some particular endpoints.

The plugin needs to hook into rest_authentication_errors but needs to return WP_Error|null|true based on the request being processed.

Note: See TracTickets for help on using tickets.