#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: |
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)
Change History (18)
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
#5
@
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.
#7
@
5 years ago
- Keywords needs-patch added; has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
#8
@
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
@
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
@
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.
#14
@
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.
#16
@
4 years ago
- Milestone Future Release deleted
- Resolution set to wontfix
- Status changed from reopened to closed
#17
@
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.
In 45687: