WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#38521 closed defect (bug) (fixed)

REST API: Get rid of the `/users/me` redirect

Reported by: jnylen0 Owned by: rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

From @youknowriad in Slack:

Hi! I noticed something in the API, not sure if it's a bug or not. When performing a request to /users/me I'm redirected to /users/1 (seems logic because the user with id 1 is me).
But I'm using CORS to tackle the API via OAuth1 and CORS disallow redirects if the request require preflight.
So the /me is not working unless I call directly /users/1
Maybe the /me should not redirect but gives the user object directly ?

There is even a special section in the docs about this: http://v2.wp-api.org/guide/problems/#authentication-errors-with-usersme

I understand that we don't want to duplicate resources across API URLs. However, this redirect is breaking things for a pretty common use-case of this endpoint (any application that runs in a browser on a different domain than the WP site) and we should remove it.

We already have a mechanism to indicate the canonical location of a resource: response._links.self[0].href. Let's use that instead.

Attachments (3)

38521.diff (1.6 KB) - added by jnylen0 4 years ago.
38521.2.diff (9.8 KB) - added by pento 4 years ago.
38521.3.diff (10.0 KB) - added by pento 4 years ago.

Download all attachments as: .zip

Change History (29)

#1 @joehoyle
4 years ago

Fair bit of discussion in Slack over this - I think really the redirect doesn't give us anything and causes more harm that good, so could be removed. Technically, it's odd to have a resource available at two urls, but pragmatically, it's better to just deal with it.

Also, this is a more general issue with the Location header, we may want to make sure we are not redirecting anywhere else, as from what I understand, it's going to break with CORS 100% of the time.

#2 @youknowriad
4 years ago

I totally agree @joehoyle We should avoid redirections to avoid breaking CORS.

I'm not familiar with the arguments behind the choice of avoiding resource duplication on different URLs (Maybe related to caching), naively, I would have just done it. But extensive usage of _links instead of redirections could be a good compromise.

#3 @jnylen0
4 years ago

Caching should be disabled for authenticated users - the same endpoints will already return different results for different users due to permissions etc.

@jnylen0
4 years ago

#4 @jnylen0
4 years ago

  • Keywords has-patch has-unit-tests added

#5 @rachelbaker
4 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Owner set to rachelbaker
  • Status changed from new to accepted

#6 @rachelbaker
4 years ago

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

In 38980:

REST API: Remove the Location header redirect for the /users/me endpoint.

Props youknowriad jnylen0.
Fixes #38521.

#7 @jnylen0
4 years ago

We can remove this section from the docs now (or when 4.7 ships I guess): http://v2.wp-api.org/guide/problems/#authentication-errors-with-usersme

PR with that change: https://github.com/WP-API/docs-v2/pull/188

The only other instances of Location headers in the API endpoints are when creating a post/user/etc, accompanied by a 201 Created status code. This will not cause problems with CORS: http://stackoverflow.com/a/39728229

Related, we should look at #37994 more closely. I'll post repro steps and a patch if needed later today.

Last edited 4 years ago by jnylen0 (previous) (diff)

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


4 years ago

#9 @jnylen0
4 years ago

Here is a comparison of what several different types of client will experience under each option being discussed: https://docs.google.com/spreadsheets/d/1fSP8lNrq4DEx1LtlHOFh5Qr0SFlo7Q-smtPm6SmDbdk/edit

Client types:

  • external client written in JS
  • external client not written in JS
  • internal client written in JS

Options:

  • revert to redirect (/users/me/users/:id)
  • use _links as pointer to correct resource
  • add POST/DELETE /users/me

The key reason I'm pushing for this change is that under the redirect behavior, existing clients will "experience unexpected breakage in [up to] 2 ways (redirects not allowed with CORS+preflight; oauth1 requires re-signing)".

While it is true that _envelope is a workaround for this behavior, in order to find that out, first you have to dig through Google/specs to figure out what is going on. Nobody wants to do that: APIs should make life easy for their consumers and the rest of the code that needs to interoperate with them. Therefore, in my opinion this decision should be made based on minimizing the expected number of future frustrated developer-hours.

The argument for leaving the redirect in place is for resource purity: if the redirect is removed, this REST resource will now be available at 2 different URLs. I'm not aware of any long-term practical consequences due to introducing this impurity.

Last edited 4 years ago by jnylen0 (previous) (diff)

#10 @pento
4 years ago

In 38990:

REST API: Revert [38980].

/users/me still needs attention, but this change wasn't quite ready.

See #38521.

#11 @pento
4 years ago

  • Keywords has-patch has-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for more discussion.

#12 @rmccue
4 years ago

  • Keywords close added

So, I think that we should look at this issue, but I don't believe that removing the redirect gains us enough to warrant breaking the resource paradigm. Additionally, this issue does not apply currently, as external authentication is not included in core yet.

As @jnylen0 noted, there are three types of client:

  • Internal
  • External, JS-based
  • External, non-JS

There are two separate issues caused by redirects:

  • CORS issue: Browsers do not appear to be allowing access to the resource due to the CORS preflight OPTIONS request. This applies only to External, JS-based clients.
  • OAuth issue: If the request is not re-signed on redirect, the signature will be invalid. This can apply to any External client.

The CORS issue is a bug, and we should look at how we can fix this. CORS should work fine, as it's designed into the spec to follow redirects; I think the issue may be that we need to issue a redirect on the OPTIONS request too.

The OAuth issue is really about the usage, and I think this is a documentation issue only. Clients must be able to follow redirects correctly, otherwise we leave ourselves in a state where clients may be broken by changing server URLs, potential backwards-compatibility redirects, and a whole host of other issues. We should document that OAuth must be reapplied on redirects.

The only potentially unsolvable issue is that some clients cannot control redirects, primarily JS-based ones using XMLHttpRequest. This however is solvable by using the newer fetch() API, which allows { redirects: 'manual' } to be set; this is not polyfillable. It is for this client only (external XMLHttpRequest) that _envelope is the preferred solution.

We should open two new issues:

  1. Document redirect behaviour for OAuth.
  2. Fix CORS issue with redirects.

This ticket should then be closed.

#13 @jnylen0
4 years ago

For requests that require preflight (which is the case here because this endpoint only works when authenticated), the CORS spec explicitly prohibits redirects during both the preflight request and the actual request.

https://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0

The following request rules are to be observed while making the preflight request:
[...]
If the response has an HTTP status code that is not in the 2xx range:
Apply the network error steps.

This is the actual request. Apply the make a request steps and observe the request rules below while making the request.
If the response has an HTTP status code of 301, 302, 303, 307, or 308:
Apply the cache and network error steps.

@pento
4 years ago

@pento
4 years ago

#14 follow-up: @pento
4 years ago

  • Keywords close removed

38521.3.diff updates the patch to add edit and delete to /users/me.

I'm not convinced that this is a documentation issue. With the redirect in place, the developer experience will either be "nice, this just works", or "omg, everything is exploding, I don't know why", particularly when it comes to the internals of OAuth libraries, or XMLHttpRequest behaviour.

Making /users/me behave as a proper resource seems like it will fix this.

(PS: I know that delete on /users/me has fairly limited use, but the ability to delete yourself is too good not to add.)

#15 in reply to: ↑ 14 ; follow-up: @rmccue
4 years ago

  • Keywords close added

Replying to jnylen0:

For requests that require preflight (which is the case here because this endpoint only works when authenticated), the CORS spec explicitly prohibits redirects during both the preflight request and the actual request.

You're correct that the preflight checking doesn't follow redirects, my bad. This again still only applies to External JS-based clients.

It's worth noting as well that these clients are somewhat discouraged from using OAuth 1.0a, as they would need to expose their secret publicly. The typical process for creating JS apps that connect with OAuth 1.0a is to have a server proxy the requests in any case.

Replying to pento:

I'm not convinced that this is a documentation issue. With the redirect in place, the developer experience will either be "nice, this just works", or "omg, everything is exploding, I don't know why", particularly when it comes to the internals of OAuth libraries, or XMLHttpRequest behaviour.

Let me be clear: if OAuth libraries don't follow redirects correctly, they are broken. Similar arguments could be made for the various HTTP request methods, headers we send back, etc etc: the client may see "this doesn't work", but it's not always our job to fix this. If the OAuth libraries don't work properly, that is an issue with them.

Making /users/me behave as a proper resource seems like it will fix this.

The thing is that /users/me is not a resource. We cannot just pretend it is. There's no entry in the database with the ID me. It is a pointer to a real resource with a real ID.

Again, I have to strongly recommend closing this in favour of stronger documentation.

#16 @jnylen0
4 years ago

I don't believe that removing the redirect gains us enough to warrant breaking the resource paradigm.

I would like to hear a practical reason why this matters. I respect the intent to build an ideologically pure API, but we're building it on top of HTTP... browsers... and WordPress.

Compromises are inevitable. I still think the "omg, everything is exploding" effect as Gary put it far outweighs the desire to maintain resource purity.

#17 in reply to: ↑ 15 @pento
4 years ago

Replying to rmccue:

Let me be clear: if OAuth libraries don't follow redirects correctly, they are broken. Similar arguments could be made for the various HTTP request methods, headers we send back, etc etc: the client may see "this doesn't work", but it's not always our job to fix this. If the OAuth libraries don't work properly, that is an issue with them.

Agreed, OAuth libraries that don't support redirects are probably broken in other ways.

Recommending fetch() isn't a viable workaround for XMLHttpRequest behaviour, though. Browser support is at 61%, which is way too low for us to recommend.

The thing is that /users/me is not a resource. We cannot just pretend it is. There's no entry in the database with the ID me. It is a pointer to a real resource with a real ID.

The only argument against this is that /users/me isn't a "real" resource? It's pretty common for APIs to provide an API for the current user - Facebook has /me, GitHub has /user, Twitter has /account, Google+ has /people/me, Spotify has /me, Shopify has /users/current.

#18 follow-up: @pento
4 years ago

  • Keywords has-patch has-unit-tests 2nd-opinion added; close removed

@rachelbaker, @rmccue: Do you have any further thoughts on this?

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


4 years ago

#20 in reply to: ↑ 18 @rachelbaker
4 years ago

  • Keywords 2nd-opinion removed

Replying to pento:

@rachelbaker, @rmccue: Do you have any further thoughts on this?

I disagree with @rmccue here. I see this as a harmless change that removes a stumbling block for our users.

#21 @rmccue
4 years ago

  • Keywords commit added

Count me as overruled then. :)

#22 @rmccue
4 years ago

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

In 39085:

REST API: Remove the Location redirect for the /users/me endpoint.

This is a re-commit of [38980], which was reverted in [38990].

Props youknowriad, jnylen0, pento.
Fixes #38521.

#23 @pento
4 years ago

@rmccue: One of the concerns expressed when we previously discussed this is that having /users/me as read only is a confusing experience. Any particular reason you chose to keep it read only, rather than adding the edit/delete capabilities?

#24 @rmccue
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@pento Because I'm an idiot and just did the revert, rather than using the patch here. :)

#25 @pento
4 years ago

:tada:

#26 @rmccue
4 years ago

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

In 39092:

REST API: Add update and delete endpoints to /users/me

Now that /users/me is a standalone resource, it should have all the standard endpoints for a resource.

Props pento.
Fixes #38521 (hopefully).

Note: See TracTickets for help on using tickets.