MSC3613: Combinatorial join rules #3613
Conversation
turt2live
added
client-server
MSC: matrix-org/matrix-spec-proposals#3613 This is not a comprehensive implementation and was only built to prove that the idea works in a limited test case (knocking+restricted). The following still need to be considered/completed: * [ ] Make code quality on par with the rest of the project * [ ] Tests * [ ] Use the utility function in all the places * [ ] Update redaction algorithm * [ ] Remove auth rule changes (see MSC) * [ ] Fix handling of `allow` (see TODOs in code)
Co-authored-by: Patrick Cloke <[email protected]>
|
putting this up for SCT review as I think it's generally good to go for FCP, but feels a bit wrong to call FCP myself. |
|
in an effort to accelerate review that has not been received: @mscbot fcp merge |
|
This FCP proposal has been cancelled by #3613 (comment). Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
mscbot
added
disposition-merge
proposed-final-comment-period
|
|
||
| Note how the `join_rule` persists in the protocol, even with this MSC's introduction. This is to | ||
| ensure backwards compatibility with clients and various places in the protocol which aren't aware | ||
| of the new rules. While servers will be required to parse the event as described below, areas such |
There was a problem hiding this comment.
I'm somewhat concerned that by including join_rule, it will confuse older clients into thinking that the join rule is something that it can modify, and so a room admin using an older client will try to change something and unintentionally lose information. Would it work to use a new value for join_rule, say, "join_rule": "combinatorial", so that clients know that it's something new, and they should be careful changing it? (I have no idea how clients deal with join_rules that they don't understand, but they've at least gone through it with restricted.)
There was a problem hiding this comment.
A new join rule ends up breaking support for the room being in the room directory and such (and can have an impact on whether or not the room is visible in a space directory), so would generally try and steer away from that. This wouldn't really be an issue if knocking wasn't on the table, but unfortunately the exact case of knocking+restricted is what's at play.
Clients do generally handle new join rules reasonably well, but only because they tend to be not super smart about it. For setting join rules, clients seem to use a pattern where they set the entire event content each time, but this is hardly defensible as a pattern to rely upon (for the case where an older client tries to change the join rule and either does nothing or breaks the previous conditions).
To try and mitigate both, does string concatenation feel too awful? Specifically so that servers can joinRule.contains("knock") over federation on things like space hierarchies and to make it clear to clients that they need to fix something. Eg: join_rule: "knock+restricted"
| than one rule active at a time, which makes it difficult to allow anyone from room X to join, but also | ||
| join if their knock was accepted. The room would instead have to choose which condition it favoured. | ||
|
|
||
| This proposal aims to introduce the smallest possible baseline for a slightly improved join rules |
There was a problem hiding this comment.
I see where you're trying to go here, but honestly I can't help but feel it's overcomplicating the situation.
Let's consider what combinations actually make sense. Currently we have the following permissible join_rules:
publicknockinviterestrictedprivate
private is undefined, so we can forget about that. There's not much point combining public with anything else. invite doesn't do anything if you combine it with knock or restricted.
So the only combination we actually need to care about is knock and restricted. So why don't we just define a new join rule which does that? Granted it doesn't scale if we add more join rule types in future, but you seem to be phrasing this as a stop-gap while we sort out MSC3386 anyway.
There was a problem hiding this comment.
The hope was that we'd be able to fill the gap+some percentage, but yea: it's feeling more and more appropriate to just define restricted_knock or something.
There was a problem hiding this comment.
An alternative could be to add an allow_knock key to the restricted join rule that restricts who is allowed to knock. Pairing this with a new allow condition type might allow people in rooms X to join automatically and anyone to knock.
There was a problem hiding this comment.
Yeah, I think I agree that introducing combining operations between join rules seems likely to be simple to express but hard to actually define all the edge cases of, and polyfilling the gap we have is probably better for a short-term solution.
|
The feedback on this causes the MSC to diverge quite a bit from its original intent, so going to write up an implementation and open a new MSC to cover the suggestions. The original plan was to re-use this MSC number and move quickly enough to not have to cancel FCP, but empirically time is not something I have the luxury of speeding up ;) In the meantime: |
mscbot
removed
the
proposed-final-comment-period
turt2live
added
the
blocked
|
Has there been development on the new MSC, will it be linked here? |
|
This MSC is rejected in favour of #3787 |
turt2live
added
obsolete
Alternative titles:
Rendered
FCP proposal
This proposal is intended as a shorter term solution while other MSCs, like MSC3386, work out a potentially all-new system.
Proof of concept implementation: matrix-org/synapse#11668
Preview: https://pr3613--matrix-org-previews.netlify.app