Only process replacement from check_event_allowed module callbacks after calling every callback

[babolivier]

I haven't updated the documentation on this callback because I believe this would be the most logical way to handle this, but I'm happy to do it if the reviewer disagree.

[clokep]

This still seems to return early in the failure case though? I'm a bit confused at why this change is wanted? Is it in-case one of the callbacks stores something externally or something like that?

We should likely enforce whatever is wanted here via updates to the tests, as this seems likely to be optimized out next time someone looks at the code.

[babolivier]

This still seems to return early in the failure case though?

It only returns early if a module forbids the event from being sent, at which point it's not worth continuing processing it - which is what we also do in other similar callbacks such as check_event_for_spam fwiw.

I'm a bit confused at why this change is wanted?

Because if one module says an event can go through with some changes and another says it should be blocked, the event should be blocked. At least that's how we've done it with other callbacks.

I'm also considering a more concrete use case (related to a customer) with a server uses two modules: one that demotes users with non-default PL when they leave and https://github.com/matrix-org/synapse-freeze-room which might also do stuff when a user with non-default PL leaves the room. Some callbacks may return a dummy replacement dict (i.e. just event.get_dict()) to force the homeserver to recalculate its context after updating the state of the room (this is to work around a bug I need to investigate a bit more where some state changes might get instantly reverted if the context isn't rebuilt, which might have some roots in the state res implem), and this would be the case for this one. We don't want the second module to be bypassed because the first one let the event through but returned a dict, because otherwise the room might not end up being frozen when it should.

It's worth noting there already are a few issues with this callback, namely with the way it replaces events, e.g. #10406 - as the doc says it's an experimental one that module devs shouldn't use until we figure out what to do about them.

We should likely enforce whatever is wanted here via updates to the tests

Gah, indeed, I wanted to write a test for that but forgot 🤦

[clokep]

I'm a bit confused at why this change is wanted?

Because if one module says an event can go through with some changes and another says it should be blocked, the event should be blocked. At least that's how we've done it with other callbacks.

Oh, duh! I didn't quite grok this from the description. This is fixing a bug, not changing behavior. Carry on! 👍

[richvdh]

It kinda feels like later modules in the chain should see the replaced content, rather than the original. If the first module makes a change which the second module would deem unacceptable, shouldn't it be allowed to say so?

I don't know how practical that is to implement though.

[babolivier]

Yeah my reasoning was that we usually consider the highest module in the config to be the source of truth, though I can see how it doesn't apply here. The right solution would probably be to rebuild the event before moving onto the next module if a replacement was provided. My main motivation for getting this over the line was considering #10830 but as it says I'm not seeing it happen anymore, which #10835 seems to confirm, so I'm tempted to park this until we figure out what to do about #10406, which looks like a superset of this issue.