Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HTTP 403 to possible profile responses #3530

Merged
merged 3 commits into from Dec 29, 2021

Conversation

callahad
Copy link
Contributor

@callahad callahad commented Nov 25, 2021

Some servers may not allow profile lookup over federation, and thus
respond to GET /_matrix/client/v3/profile/{userId} with an HTTP 403.

For example, Synapse can be configured to behave in this way by setting:

allow_profile_lookup_over_federation=false

Thus, this behavior already exists in the wild, and may cause issues for
clients such as element-hq/element-web#17269.

Synapse could alter its behavior and return an HTTP 404 in these cases,
but amending the Spec seems preferable to align with extant behavior.
Further, allowing HTTP 403 gives clients more specific information as to
why a request has failed, enabling more precise error handling.

Signed-off-by: Dan Callahan danc@element.io

Preview: https://pr3530--matrix-org-previews.netlify.app

Some servers may not allow profile lookup over federation, and thus
respond to GET /_matrix/client/v3/profile/{userId} with an HTTP 403.

For example, Synapse can be configured to behave in this way by setting:

    allow_profile_lookup_over_federation=false

Thus, this behavior already exists in the wild, and may cause issues for
clients such as element-hq/element-web#17269.

Synapse could alter its behavior and return an HTTP 404 in these cases,
but amending the Spec seems preferable to align with extant behavior.
Further, allowing HTTP 403 gives clients more specific information as to
why a request has failed, enabling more precise error handling.

Signed-off-by: Dan Callahan <danc@element.io>
@turt2live
Copy link
Member

I'm torn between asking for this to be an MSC and just merging it. Open to convincing on either direction.

@uhoreg
Copy link
Member

uhoreg commented Nov 25, 2021

Given that:

  • this is in response to disagreement between two existing implementations
  • synapse added this behaviour about 9 months ago (so it isn't legacy behaviour that never got specced, but is new behaviour)
  • the initial comment is 75% of an MSC already

my opinion is that this should be an MSC first. I'd expect that it should be a fairly quick and easy MSC, though.

@callahad
Copy link
Contributor Author

My argument in favor of "just merge it" would be along the lines of: All this change does is highlight a likely response code, not change any semantics, nor imply anything semantically beyond what is already the definition of 403 Forbidden in the HTTP specification.

The list of response codes in the Matrix spec is not comprehensive, nor should we expect it to be. Matrix is an application atop HTTP, software integrating it can be reasonably expected to gracefully handle a wide gamut of response codes. For example, I can imagine that same endpoint legitimately returning 414 URI Too Long, 429 Too Many Requests, 431 Request Header Fields Too Large, or 451 Unavailable For Legal Reasons to clients. Not to mention 500 Internal Server Error, 502 Bad Gateway, 503 Service Unavailable, or 504 Gateway Timeout.

Listing each of those for each endpoint would be madness (they're already part of the HTTP spec!), so the only rational approach to that section of the Matrix spec is to use it as an additional signpost for developers, not as an exhaustive enumeration.

@callahad
Copy link
Contributor Author

...but happy to MSC if y'all think it needs an MSC.

@turt2live
Copy link
Member

turt2live commented Nov 25, 2021

While the error codes themselves are part of the HTTP spec, we do apply meaning to many of them (403, 429, 401, and 404 included).

This should probably be an MSC to document the motivation for the error response, as that doesn't feel captured in the history very well.

fwiw, Element Web was/is aware of that error code, but there's no other reliable way to determine if a user exists. In fact, the dialog/existence of a profile is reliant on Synapse behaviour as Synapse (as of a few years ago) sets a default profile for all new users - there's no requirement that a server do this, though. This is all why the dialog says "may not exist" and not "does not exist".

@H-Shay H-Shay self-assigned this Dec 1, 2021
@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Dec 14, 2021
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise lgtm for when the MSC passes FCP - will re-review then.

changelogs/client_server/newsfragments/3530.clarification Outdated Show resolved Hide resolved
@turt2live turt2live self-requested a review December 21, 2021 16:19
Co-authored-by: Travis Ralston <travpc@gmail.com>
@callahad
Copy link
Contributor Author

Need to move the changelog stub from .clarification to... something different.

@turt2live turt2live merged commit 48d8f72 into matrix-org:main Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something needs to be done before action can be taken on this PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants