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.
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
MSC2432: Updated semantics for publishing room aliases #2432
MSC2432: Updated semantics for publishing room aliases #2432
Changes from 3 commits
6bd4b3c
95ff266
786772f
645dbcc
30d762c
98a6cd0
8b9ea10
4e123ca
7917d08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a problem with all state events ftr. Realistically we should just drop the
Required
on all the content fields for state events, unless it's needed for auth rules (like membership).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably. I don't think that matters here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have this in the spec, but for some reason have declared the field as required:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: spec an error code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the synapse implementation (matrix-org/synapse#6971) I currently chose
M_INVALID_PARAM
if the form of the data is incorrect, andM_NOT_FOUND
if the alias does not point to a room. UnfortunatelyM_NOT_FOUND
is also used for if the alias itself does not exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M_NOT_FOUND
doesn't feel like a good match to me. We don't actually know if the underlying resource exists or not (it probably does).I'd go with something like
M_BAD_ALIAS
orM_ALIAS_IN_USE
or maybe even abuseM_ROOM_IN_USE
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saying the
M_ROOM_IN_USE
doesn't quite seem to mean the same thing (it isn't a matter of it being used already, but by the data not matching). Adding anM_BAD_ALIAS
likely makes more sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is
M_INVALID_PARAM
wrong for that purpose? If it's anm.room.canonical_alias
event, there's not much room for ambiguity, while an additional error code means additional treatment (i.e. turning to a human-friendly message) in all the clients.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there was some confusion here over what the behaviour should be if the alias doesn't exist at all. I was envisaging it would be treated the same way as an alias which points at the wrong room (400 with
M_BAD_ALIAS
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to copy what I had put somewhere else, my understanding from the above was we would do the following:
M_INVALID_PARAM
if the form of the alias is wrong.M_NOT_FOUND
if the alias is completely unknown.M_BAD_ALIAS
if the alias is found, but does not point to this room.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think of
M_NOT_FOUND
as a special case. I wouldn't tremendously care if the alias is misplaced or just doesn't exist; I'm happy to either have or not haveM_NOT_FOUND
for that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly conscious that clients are going to forget to deal with 404s as well as 400s (as shown in riot at matrix-org/synapse#7105)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I have clarified this in 7917d08. If people have strong feelings that we should do something different, please say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this enters the spec as "Do not send
m.room.aliases
events" for abundance of clarity. Likewise for endpoints below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, possibly, but if we've got this right,
m.room.aliases
should disappear from the spec altogether. It would make odd reading (to a reader who didn't know the history) if the only place it was mentioned was to say that you shouldn't send one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is largely an implementation detail anyways:
I'd count "can you send the event" as additional access control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeahhh, though I think it's worth emphasising that being able to delete an alias, but not update the event, is a case that server implementers should consider.
I've clarified this to leave the behaviour implementation-defined, but to make a recommendation. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future MSCs: we also need to define authorization and rate limiting requirements. Presumably this endpoint can be rate limited and requires auth.