Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unfreeze notification_levels for PushRuleEvaluator #18103

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
3 changes: 3 additions & 0 deletions changelog.d/18103.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fixed a bug that disturbed room creation when a check_event_allowed callback was registered by a module and power_level Notifications mapping exists.

Contributed by @c-cal
3 changes: 2 additions & 1 deletion synapse/push/bulk_push_rule_evaluator.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
from synapse.util import unwrapFirstError
from synapse.util.async_helpers import gather_results
from synapse.util.caches import register_cache
from synapse.util.frozenutils import unfreeze
from synapse.util.metrics import measure_func
from synapse.visibility import filter_event_for_clients_with_state

Expand Down Expand Up @@ -412,7 +413,7 @@ async def _action_for_event_by_user(
# Note that this is done automatically for the sender's power level by
# _get_power_levels_and_sender_level in its call to get_user_power_level
# (even for room V10.)
notification_levels = power_levels.get("notifications", {})
notification_levels = unfreeze(power_levels.get("notifications", {}))
Copy link
Contributor

@MadLittleMods MadLittleMods Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

# 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"
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@c-cal c-cal Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@MadLittleMods MadLittleMods Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if not event.room_version.enforce_int_power_levels:
keys = list(notification_levels.keys())
for key in keys:
Expand Down