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
MSC2346: Bridge information state event #2346
base: old_master
Are you sure you want to change the base?
MSC2346: Bridge information state event #2346
Changes from all commits
fa30188
e5daeb9
e13cbec
e887a08
dfba42d
38e635a
0103c30
ffc98ce
ef21d6b
542dfa7
4f05bb3
8deecda
899f4b1
d21d25b
e133838
6e72d10
9104740
a0b98d7
d5dd039
5251039
74bc2d9
380b54d
62afef8
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 MSC doesn't solve what the
/thirdparty
APIs are trying to do though, as they're trying to answer the question of "what third party networks are bridged on the server and what are their rooms", whereas this MSC is trying to answer the question of "which room is this bridged to?".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.
What should be the format of the state key if network ID/channel ID are omitted? e.g.
foo.bar.appservice-skype://skype//someid
orfoo.bar.appservice-skype://skype/someid
? (do we just drop the component and keep all the slashes, or do we collapse slashes?)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 probably makes sense for the format of what comes after
://
to be dependent on the value before://
. Here{protocol.id}/{network.id}/{channel.id}
would be the format fororg.matrix.appservice-irc://
; but forfoo.bar.appservice-skype://
it would always be{protocol.id}/{chat.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.
Should this also be optional? You can imagine that some bridges could just use the bridge users as bridge bots. For example if you bridge a 1:1 chat from Telegram you can just use the remote user as the "bridge bot".
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.
why do we need this? Widgets already suffer from this being unreliable and unhelpful, to the point of us ignoring it.
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.
How's that going for you?
This is actually the same use case that widgets are using it for right now, it's just sugar to point at whoever added the bridge, if it was added by a user.
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.
Oh, you may have been referring to it duplicating the
sender
field. This is intentional, for the plumbing use case. I don't see any reason why this would be unreliable 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.
It's not really trusted information, though I guess if somehow the bridge is limited to being able to send it then it can be trusted. The reason we don't use
creatorUserId
for widgets, even if someone else edits the widget, is because it is displayed so prominently and can cause lies to be shown to the user.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 major concern here is essentially a room admin/mod hopping into the room and changing the state event to say that
@bob:example.org
is responsible for the bridge, who may or may not even be a real user ID. One of the downsides for Bob in that case would be spam about bridging to somewhere controversial. In a general scenario though, as an informational thing it's probably okay, but it would be nice to acknowledge the privacy/social risks in the security section.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.
Nit: Why are we calling this a channel? In Matrix we call these rooms so doesn't it make sense to use the local terminology. Basically "the thing in the source protocol which maps to a room in Matrix". For example if the remote network has groups and Matrix has rooms why are we calling the field
channel
?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 need to escape
%
to%25
.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 guess we should say that the IDs should be URL-encoded, including encoding
/
to%2F
?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.
Can we have examples of the
protocol.id/network.id/channel.id
part of the state key? e.g. for XMPP, would it beXMPP/xsf@muc.xmpp.org
orXMPP//xsf@muc.xmpp.org
(i.e. do we drop a/
if there's no network field? What if there are more levels (e.g. protocol, network, community, room)? Is the network supposed to be human-readable like "Freenode" or something more like "freenode.org"? Maybe it would be better to just say that it's a path representing the hierarchy starting from the protocol and ending in the room? And then instead of hard-codingprotocol
,network
, andchannel
as keys in the content, make it an array where the first element is the protocol and the last element is the channel?Also, if a protocol/network name has a
/
in it, does it get escaped?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 have added several examples of the event body to the description that might help make this more understandable.
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.
Potentially, though I am worried this might make it harder for clients to show a sensible UI. How would riot render:
["Github", "matrix-org", "matrix-doc", "2346"]
vs
{"protocol": "GitHub", "network": "matrix-org/matrix-doc", "channel": "2346"}
(Simplified for readability, and using a deliberately complex example).
In the first example, there are 4 keys and it's hard for a client to decide how to format this in a settings page. Joining them with a delimiter is too ugly (to me). There are probably examples which are restricted by the 3 component limit, but I am struggling to come up with any?
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.
Does the client actually need to care about the state key? The information embedded into the state key is already available in the body, and clients would want to use a more friendly name anyways. I think we can just use an empty state key and let clients figure it out.
If you're running multiple bridges off the same bridge bot, don't.
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.
Or if you are running several bridges off different bridge bots, you will still need different state events and therefore different keys.
Why can't we use user_ids? We could, but that does forever tie your bridge to using one user_id for life when the actual thing the bridge is "keyed" off is the protocol,network and channel.
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 client doesn't need to give a heck about the state key, other than it being more readable for those who want to work on it. Given there isn't really a downside to having a schema for the state key, and it gives more readability, I don't see why not.
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 would much rather that we don't prescribe the state key. Just say that it needs to start with the bridge ID (unique prefix) and the rest is implementation defined. Any meaningful values should be delegated to the context of the state object.
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.
Missing: what if the bridge doesn't have permission to send state events? (a completely valid thing to do)
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.
Such is life. I don't think we can engineer around users not giving their bridges permission to add state. It's not great, but it's better than adding lots of sketchy outside-of-the-room data(though matrix.org bridges in both portal and plumbed rooms have PL50 by default, so this is a relatively unlikely).
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 a lot more concerned with the bridges that aren't on matrix.org, given the popularity of bridges in the last year alone (and the year prior to this thread). Most bridges don't ask for permissions, and a quick poll shows that most non-matrix.org bridges appear to be running without appropriate permissions in those rooms.
This can be considered a room configuration error, but it's still a valid issue that this MSC needs to acknowledge. There's a point where we can't just write off issues as "users should be better".
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.
Missing: what if the bridge doesn't have a bridge bot? (puppet bridges, transparent bridges, etc)
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.
Again, this is more of a "well, meh". There may be a way to solve this in a future MSC, but let's leave it for then.
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.
not sure that's the best option: can we figure out a way to publish this info?
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. Either a user the bridge has ownership over publishes the information, or a user publishes the information on behalf of the bridge. I don't think it's worth speccing this though, as it's down to implementation how they want to insert the event.
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.
Doesn't have to be a state event. Could start publishing this over EDUs or some other DAG
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 having it in the room dag and a state event allows us to reuse pretty much all logic across the homeservers/clients/bridges. This is important, because we can not only reuse the same API routes when synchronizing the information across clients and bridges, but also re-use the same access control semantics as with other information in the room like names and topics (using PL events).
I would like to hear from others if they also think supporting the use case of a bridgebot-less bridge is important and requires us to invent our own non-room dag or EDU structure. Personally, it feels like a lot of faff for little gain.
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.
A note in the MSC about how the non-bridge-bot user can publish this would be great.