Document fail-open behaviour of check_event_allowed module API callback

[reivilibre]

Signed-off-by: Olivier Wilkinson (reivilibre) oliverw@matrix.org

With my module developer hat on, this behaviour didn't seem expected.

It's probably not my say whether we should change this (I guess it's been thought through already) but it should at least be documented, I think.

[babolivier]

For extra context, this was added in #10386 following this suggestion that came up during review. I'm not saying the module system is behaving the right way here, and I seem to remember @richvdh arguing that if a module raises then we should make it fail the request. So I think the question of whether Synapse is doing the right thing here is still up in the air and we might want to discuss it further before we burn it into our docs.

Also, if we decide in favour of keeping this error handling in, we should also update the docs for the rest of the third-party event rules callbacks, as well as the others that implement similar handling (from memory I think this is only the presence router?)

[reivilibre]

I share the view that failing closed is the right thing to do.

Do we need a proper discussion about this or are we willing to accept that the current behaviour is 'wrong'..?

[babolivier]

I don't think we know yet the current behaviour is wrong. I've asked the question in #synapse-dev, let's take it there.

[reivilibre]

So it seems the current behaviour is wrong — issue to be opened imminently.