-
Notifications
You must be signed in to change notification settings - Fork 377
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
MSC2695: Get event by ID over federation #2695
base: old_master
Are you sure you want to change the base?
Conversation
Co-authored-by: Aaron Raimist <aaron@raim.ist>
``` | ||
{ | ||
"event_id": "$Woq2vwLy8mNukf7e8oz61GxT5gpMmr_asNojhb56-wU", | ||
"room_id": "!jEsUZKDJdhlrceRyVU:example.org" | ||
} | ||
``` |
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 bit of an awkward API shape to represent, but can probably be done.
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.
Open to suggestions on this; the main goal was to at least return the room ID. I structured the result by just removing the other fields from the "full" response above, but I am sure other patterns would work equally well.
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.
an idea would be to let the deprecated endpoint die and introduce a whole new endpoint which does something like:
{
"event": {...},
"event_id": "$whatever",
"room_id": "!whatever"
}
where event
is the full thing and optional under the conditions outlined already.
``` | ||
|
||
If the event is found but is not allowed to be retrieved, then a short form with | ||
only `event_id` and `room_id` is returned with status code 200: |
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 sounds like it could leak sensitive data. Why is this needed, rather than just 404-ing?
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.
One main use case for this part is to allow the new version of matrix.to
URIs which might specify only an event ID to still offer a description of the room that event is in via the matrix.to
interstitial site. matrix.to
would use this API to at least find out room ID, and it can then use the room directory API to learn more about the room if it's not world-readable.
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.
Maybe we should only return the room ID if the room "public", i.e. that the server is happy to leak metadata about it? That way we only return the room ID if fetching metadata would return something useful? (Or perhaps merge the two APIs so that you can attempt to fetch room metadata with just the event ID?)
|
||
### API details | ||
|
||
`GET /_matrix/client/r0/events/{eventId}` |
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.
Small quibble but this feels inconsistent with the other APIs, as a) it has nothing to do with /events
and b) the rooms API uses .../event/...
singular. I'd vote we use /event/{event_id}
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.
Sure, I'd be happy with that as well. Only went for the current path since it had already existed before, but your suggestion seems fine to me.
given that the primary stated usecase for this MSC was shorter permalinks, but MSC2312 does not allow for the room id to be omitted in (If it is a useful MSC, AFAICT the next step would be a demonstration implemention?) |
MSC2312 speculates about omission of room ids as a possible future MSC. It used to be more lenient on that question even in the normative part but specifically that leniency has been met with vehement opposition from @Sorunome who apparently was very concerned that it may be used to advance the change proposed by MSC2695, which they also oppose, along with other community members. If this MSC2695 is unlocked, I can help with adding a section to it that changes MSC2312 provisions accordingly. I'd prefer to abstain from the answer about its merit as the key underlying issue (the scope of event ids uniquiness) is on the server-side. |
This MSC is not actively being worked on... Perhaps it should be closed? Feel free to do so if that's reasonable. |
Rendered