-
Notifications
You must be signed in to change notification settings - Fork 380
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?
Conversation
Note: I also want to modify publicRooms to return information in these state events, since it's useful information to anyone wanting to join a room. |
This is probably ready for review now. |
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.
Largely looks good, but have added some bikeshedding and nit picking.
`external_url` works for bridged messages in the AS spec. | ||
|
||
In terms of hierachy, the protocol can contain many networks, which can contain many channels. | ||
|
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 be XMPP/[email protected]
or XMPP//[email protected]
(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-coding protocol
, network
, and channel
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.
And then instead of hard-coding protocol, network, and channel as keys in the content, make it an array where the first element is the protocol and the last element is the channel.
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.
If you're running multiple bridges off the same bridge bot, don't.
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.
Co-Authored-By: Hubert Chathi <[email protected]>
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.
two small points
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.
Some more nit picking
Co-Authored-By: Hubert Chathi <[email protected]>
Added a Slack bridge POC. This means that we now have full implementation for both sides :) |
Co-authored-by: Tulir Asokan <[email protected]>
|
||
There exists a way to do this in a local setting, by using the | ||
[/thirdparty/location](https://matrix.org/docs/spec/application_service/r0.1.2#get-matrix-app-v1-thirdparty-protocol-protocol) | ||
API but this creates a splitbrain view across the federation and is an unnacceptable situation. |
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.
overall this is feeling fine, but the further exposure of the awful third party network API is a bit worrying.
"avatar_url": "mxc://foo/bar", // Optional | ||
"external_url": "irc://chat.freenode.net" // Optional | ||
}, | ||
"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.
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
?
``` | ||
|
||
The `state_key` must be comprised of the bridge's prefix, followed by the `protocol.id`, followed by the `network.id`, | ||
followed by the `channel.id`. Any `/`s must be escaped into `%2F`. The bridge prefix can be anything, but should uniquely |
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
?
"state_key": "org.matrix.appservice-irc://{protocol.id}/{network.id}/{channel.id}", | ||
"type": "m.bridge", | ||
"content": { | ||
"bridgebot": "@appservice-irc:matrix.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.
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".
`external_url` works for bridged messages in the AS spec. | ||
|
||
In terms of hierachy, the protocol can contain many networks, which can contain many channels. | ||
|
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.
This looks to be back with @Half-Shot. |
|
||
```js | ||
{ | ||
"state_key": "org.matrix.appservice-irc://{protocol.id}/{network.id}/{channel.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.
What should be the format of the state key if network ID/channel ID are omitted? e.g. foo.bar.appservice-skype://skype//someid
or foo.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 for org.matrix.appservice-irc://
; but for foo.bar.appservice-skype://
it would always be {protocol.id}/{chat.id}
Rendered
Riot POC
Slack Bridge POC
IRC Bridge POC