MSC4155: Invite filtering

[Johennes]

Rendered

---

In line with matrix-org/matrix-spec#1700, the following disclosure applies:

I am a Systems Architect at gematik, Software Engineer at Unomed, Matrix community member and former Element employee. This proposal was written and published with my gematik hat on.

---

SCT stuff:

MSC checklist

FCP tickyboxes

[turt2live]

Implementation requirements:

• Client supporting the feature, and using it

[Johennes]

Since I believe all the gematik implementations will be closed source, I'll reference #3860 (comment) as an example for how cases like this were handled in the past. Thanks @clokep for digging it up.

[Half-Shot]

element-hq/synapse#18288 is a serverside implementation, albeit with #4155 (comment) "corrected"

[Half-Shot]

element-hq/element-web#29603 exposes the serverside settings in the client, but does no filtering of itself. @Johennes does this evoke any worries from you if the invite filtering is done server-side?

[Johennes]

No, I this is fine and consistent with the proposal which allows but doesn't enforce the filtering on either the client or the server. Now that I think of it, we might need a capability so that the client knows when the server does not filter in which case the client needs to filter itself.

[Half-Shot]

element-hq/synapse#18288 supports the updated proposal.

[turt2live]

With my SCT hat:

The following implementations demonstrate the desire for filtering, and experiment with different ordering, but all demonstrate that the code is more than possible:

• Add support for MSC4155 Invite filtering element-hq/synapse#18288
• Nheko-Reborn/mtxclient@d6a0a4e
• Nheko-Reborn/nheko@978174a
• https://github.com/famedly/synapse-invite-checker/blob/5608f40ffeaef1320edef8554cb432fa2c881fdd/synapse_invite_checker/types.py#L71
• https://github.com/matrix-org/msc4155-synapse-invite-filter

The following implementations show that there's desire, but don't do much with the actual config:

• Implement MSC4155: Invite filtering element-hq/element-web#29603

Collectively, I feel this satisfies the implementation component, though we should monitor for user feedback if the rule processing order feels wrong. If so, we can correct it with a future MSC relatively easily (it'll suck for people on implementations which use the 'old' rules, but as those implementations update users will see the changes).

[richvdh]

If so, we can correct it with a future MSC relatively easily (it'll suck for people on implementations which use the 'old' rules, but as those implementations update users will see the changes).

I wish I shared your confidence. Won't it (also) suck for people who have set up rules with the "old" system, which suddenly don't work because the ordering has changed? And then they'll change the rules and get something that works with their client but not their server.

[Johennes]

I cannot think of a good way to prevent that other than blocking this MSC to let implementations try out the currently proposed ordering in practice. That'll involve exactly the same migration pain for users if the proposal changes, just that people will (hopefully consciously) have opted into an explicitly unstable implementation at their own risk.

[richvdh]

Yeah I don't disagree with you there. At some point we're going to have to just decide that we have enough confidence in the ordering that it's worth shipping. I just think it's worth having our eyes open to the fact that changing it later is going to be a bit of a buttpain, rather than "relatively easy" as Travis claimed.

[turt2live]

other than the confusion of block versus ignore, this looks good to me - thank you!

If we do switch to using split out fields, supporting globs would be ideal.

[turt2live]

thanks for updating this! I'll do some implementation review then hopefully send this for FCP (with the assumption that the listed suggestions aren't too controversial 😇 )

[turt2live]

@mscbot resolve Clarify ignore behaviour (preferred: send an event, but don't deliver, like normal user ignores)
@mscbot resolve Checklist incomplete

[Gnuxie]

The allow semantic here is in the reverse order to server ACL, and the implication is that an allow rules overrule ban rules. This will cause problems later if an allow policy rule recommendation is created, because it will have different semantics based on context.

To be explicit, for it to be consistent with server ACL, the allow rule would have to be applied first and processing can only continue if there is a match.

[turt2live]

it feels beneficial to follow the Server ACL order, with a slight change to the very last rule. Instead of "Otherwise, deny", we "Otherwise, allow" for backwards compatibility.

I think this means processing the rules as such:

1. Verify the invite against each *_users setting:
   1. If it matches blocked_users, stop processing and block.
   2. If it matches ignored_users, stop processing and ignore.
   3. If it matches allowed_users, stop processing and allow.
2. Verify the invite against each *_servers setting:
   1. If it matches blocked_servers, stop processing and block.
   2. If it matches ignored_servers, stop processing and ignore.
   3. If it matches allowed_servers, stop processing and allow.
3. Otherwise, allow.

I don't think we want to check each in batches (all blocks, then all ignores, then all allows) specifically to ensure the user is able to allow-list certain senders from otherwise banned servers. For example: blocked_servers: [matrix.org], allowed_users: [@abuse:matrix.org].

Later, when we have more rules, we'd append to just before the "Otherwise, allow" rule.

[Gnuxie]

Consider the following issue¹.

1. @alice:example.com is added to allowed_users.
2. example.com becomes hostile.
3. example.com is added to blocked_servers.
4. @alice:example.com sends malicious invitations.

When example.com is added to blocked_servers, the entry for each user from that server in allowed_users becomes stale. There could probably be other scenarios where entries can become stale too.

A way to fix this could be to instead provide a *_users object for eachserver.

{
  "example.com": { "allowed_users": "@alice:example.com" }
}

You would then also need to be able to state whether all users from the server can send invitations, or specific users. So you would also need to introduce a "match all" rule for *_users.

Footnotes

1. You can't avoid this problem by inverting the behavior described in the comment above (ie requiring that both checks for servers and users pass) because then you would end up in a situation where the allowed_servers entry for the user being added can become stale as the allowed_users entries are changed. To be explicit, adding @alice:example.com to allowed_users would require adding example.com to allowed_servers first. ↩

[dbkr]

If we're explicitly extending to include null we should be complete, ie. undefined or indeed anything that isn't an array should be treated as absent (if we just said 'missing' I'd assume it was implicit, but if you're calling out one I think you need to be complete).

[clokep]

I'd prefer only missing to be considered empty. What's the goal of including null here?

[dbkr]

How would clients/servers treat null differently to missing though?

[clokep]

Why is null valid in the grammar?

[Johennes]

If we're explicitly extending to include null we should be complete, ie. undefined or indeed anything that isn't an array should be treated as absent [...]

I think undefined is just the JS equivalent of "missing" in JSON but good point on other data types.

What's the goal of including null here?

There's occasionally been complaints that the spec doesn't clearly describe the behavior for null, missing or empty values. So I was trying to be specific here with the intention that a null or missing value doesn't have any special meaning.

However, I suppose that's sort of obvious because how else would such a value be interpreted? I have removed the sentence entirely with 5c3cab8.

[clokep]

I think the options are that invalid data should be treated as empty or that it invalidates the whole object. 🤷

[Johennes]

I wonder if this is something that could be defined spec-wide?

FWIW I'm dealing with this when implementing element-hq/synapse#18195 as well.

[turt2live]

It may be a spec-wide problem, but being explicit in the MSC in the meantime helps ensure it's covered and not delayed behind yet more MSCs.

[Johennes]

Ok, then I think anything that doesn't match the schema should lead to the whole object being discarded. This is because the different lists will commonly work together and using only some of them might lead to unexpected results.

All properties in content are optional arrays. A missing property MUST be treated like an empty array. Any value that is not an array of strings MUST lead to the entire event being considered invalid.

How does that sound?

[turt2live]

@mscbot concern Pending review from Foundation T&S team