Return 404 when fetching an unset displayname or avatar_url - #20174
Draft
barodeur wants to merge 3 commits into
Draft
Return 404 when fetching an unset displayname or avatar_url#20174barodeur wants to merge 3 commits into
displayname or avatar_url#20174barodeur wants to merge 3 commits into
Conversation
Per the spec, GET /_matrix/client/v3/profile/{userId}/displayname (and
avatar_url) should return 404 when the field is unset, but Synapse
returns 200 with a null value. Custom fields on the same endpoint
already 404 correctly.
These tests currently fail; the fix lands in the next commit.
Per the spec, GET /_matrix/client/v3/profile/{userId}/{keyName} should
return 404 when the requested field is unset. Custom fields already
behaved this way, but displayname and avatar_url returned 200 with a
null value.
Raise the 404 in the servlet rather than in the profile handler's
get_displayname/get_avatar_url, since the SSO handler relies on those
returning None for unset fields, and the federation profile query
handler has its own semantics for missing fields.
Custom fields are unaffected: unlike displayname/avatar_url they can
legitimately hold a JSON null, which still returns 200.
Fixes element-hq/backend-internal#277.
See also element-hq#13137.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GET /_matrix/client/v3/profile/{userId}/displayname(likewiseavatar_url) returned200 {"displayname": null}when the field is unset, where the spec documents a 404 for a profile key that does not exist. Custom fields on the same endpoint already 404 correctly, so built-in and custom keys behaved differently on the same route:Before:
After:
This is not hypothetical: the 200-with-
nullshape already breaks spec-compliant clients — ruma'sget_profile_fieldfails to deserialize{"avatar_url": null}(ruma/ruma#2360), affecting Element X, and matrix-rust-sdk carries workarounds for it (matrix-org/matrix-rust-sdk#6148, matrix-org/matrix-rust-sdk#6883).Backward compatibility
mainagainst this change:Account::get_display_nameimproves: today an unset displayname fails deserialization (get_profile_fielddoesn't handlenullin the response. ruma/ruma#2360); with this change it receives the 404 that the SDK already documents as the expected server response.Account::get_avatar_urlregresses: the workaround from matrix-org/matrix-rust-sdk#6148 uses the legacy endpoint and relies on200 {"avatar_url": null}→Ok(None); it does not handleM_NOT_FOUND, so an unset avatar would start returning an error until the SDK maps the 404 toNone(as suggested in theget_profile_fielddoesn't handlenullin the response. ruma/ruma#2360 thread).GET /profile/{userId}) path is unaffected.Fixes #13137. Related: #19466 (this addresses the
GETinconsistency reported there; thePUT-with-empty-string behaviour is out of scope).Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.