invite -> knock membership change is actually illegal

[zecakeh]

Link to problem area: https://spec.matrix.org/v1.3/client-server-api/#mroommember

Issue
There was this spec clarification that says that invite -> knock is allowed according to the auth rules.

Now I look at the auth rules (rule 5.7.3) and I get this reconstituted sentence if I pick the correct parts:

If (the new) membership is knock and if the current membership is not invite, allow.

Which means invite -> knock should be rejected.

Expected behaviour
This part of the clarification should be reverted with this action not allowed, like before.

[richvdh]

I think actually that's an error in the auth rules, which got missed as part of matrix-org/matrix-spec-proposals#3730. It looks like Synapse allows the transition, in any case.

[richvdh]

That said https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/2403-knock.md#auth-rules is fairly explicit about denying the invite->knock transition, so perhaps this should be defined as an implementation bug.

@turt2live: did you happen to consider any of this when looking at matrix-org/matrix-spec-proposals#3730?

[turt2live]

The consideration was loosely around preferring the implementation because it's less annoying to fix the problem that way. I'd be highly inclined to keep going that route and further fix the spec to remove the implied sentence denying invite->knock, simply because it's been so long with Synapse accepting it that it's possible it happened in the wild.

I will note that this isn't my favourite answer in the slightest though. Ideally the spec would be able to tell Synapse to fix its bug and deal with the potential auth conflicts it caused, but we're not quite there yet. We are getting closer, though.

@richvdh are your thoughts roughly aligned in this regard? (specifically the bit about fixing the spec, not necessarily yelling at Synapse for introducing an implied spec change)

[richvdh]

@richvdh are your thoughts roughly aligned in this regard?

At this point I could go either way: I doubt there are enough invite->knock transitions that this is going to make a meaningful difference. But I'm not in a position to fix either the spec or synapse: would be happy to see someone adopt either approach (as long as they are prepared to see it through and drive through the changes in all the necessary places).

cc @Sorunome as the person who wrote MSC2403 and the implementation of it in Synapse (#6739), and @anoadragon453 who was also heavily involved with both.

[anoadragon453]

The history behind the deny rule in the MSC stems from this conversation.

To me this looks like a change that occurred in the doc but was never propagated to the implementation, and then missed during implementation review.

I don't really see an issue either way other than potentially receiving questions in the future about why the spec allows such a transition. A plan to fix both words could potentially be:

1. Update the spec now to match reality.
2. Draft an MSC for a future room version to deny invite->knock in the future.

Notably I'd wait on executing step 2 until usage of knocking picks up around the ecosystem, just in case someone does stumble on to a valuable use case for invite->knock.

[turt2live]

with the comments in mind here, I'm planning to pick it up per @anoadragon453's comment, though with a slight modification:

1. Update the spec (and MSC) to match reality, per process
2. Leave invite->knock alone. If someone else feels strongly enough the ban it, feel free to open an MSC.