fix: unfreeze notification_levels for PushRuleEvaluator

[c-cal]

…which can't handle immutabledict

Pull Request Checklist

• Pull request is based on the develop branch

• Pull request includes a changelog file. The entry should:

  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.

• Code style is correct
  (run the linters)

  fixes: check_event_allowed callback from the module API cause a TypeError #18101

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[MadLittleMods]

I've just had a thought on how we could solve all of the cases in #18117 pretty elegantly without having to muck about in the downstream code,

Instead of calling unfreeze at the spot we're trying to use it here, we could unfreeze after we're done with the frozen event in the third party module callbacks (where we called freeze in the first place).

(unfreeze after we run all of the callbacks)

synapse/synapse/module_api/callbacks/third_party_event_rules_callbacks.py

Lines 291 to 316 in a0b7047

	# Ensure that the event is frozen, to make sure that the module is not tempted
	# to try to modify it. Any attempt to modify it at this point will invalidate
	# the hashes and signatures.
	event.freeze()
	for callback in self._check_event_allowed_callbacks:
	try:
	res, replacement_data = await delay_cancellation(
	callback(event, state_events)
	)
	except CancelledError:
	raise
	except SynapseError as e:
	# FIXME: Being able to throw SynapseErrors is relied upon by
	# some modules. PR https://github.com/matrix-org/synapse/pull/10386
	# accidentally broke this ability.
	# That said, we aren't keen on exposing this implementation detail
	# to modules and we should one day have a proper way to do what
	# is wanted.
	# This module callback needs a rework so that hacks such as
	# this one are not necessary.
	raise e
	except Exception:
	raise ModuleFailedException(
	"Failed to run `check_event_allowed` module API callback"
	)

[MadLittleMods]

@c-cal or @blackmad (author of #18066) Do you have an interest in making that change?

[c-cal]

In this case, unfreeze() must be applied to event._dict, but since it's a “private” attribute, it would probably be cleaner to do it directly from a dedicated method (eg: event.unfreeze). Is it reasonable to make such a method so easily accessible to callbacks?

[MadLittleMods]

Is it reasonable to make such a method so easily accessible to callbacks?

This is a good point to consider. We could update the name to mark the method as internal (event._freeze()) which would discourage people from using it by convention.

Perhaps, we should just make a copy of the event that we freeze. Since we're taking the hit to freeze things anyway, it doesn't seem unreasonable to just make the event copy and freeze. We can use clone_event(...) and this prevents any possible tampering problems. If the clone is deep enough, we could avoid freezing altogether as we aren't really worried if they modify their own copy but I have a feeling we don't clone deep enough for the prev_event_ids and other lists etc.

There is FrozenEvent but it doesn't actually freeze the event all the time because it's "expensive" (see the USE_FROZEN_DICTS logic) so we can't use that.

[MatMaul]

@c-cal do you want to reopen that or should I take it over ?

There are other side effects of having a frozen dict around, like pydantic validation failing if m.mentions is present.

We are hitting that in production and would like to have a fix upstream.

[c-cal]

Github doesn't just allow me to reopen this one. But I can open an equivalent if the solution I proposed is suitable. I don't have time to investigate more about this.

[MatMaul]

The solution is good, but I would like to add a test and a clearer message. If you don't mind I can keep your commit, and iterate on top of it in my own PR, you will then keep the authorship.

[c-cal]

OK, no pb

[MatMaul]

here it is #19547