Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

No .m.rule.contains_user_name rule sent to clients #14348

Closed
deepbluev7 opened this issue Nov 1, 2022 · 6 comments
Closed

No .m.rule.contains_user_name rule sent to clients #14348

deepbluev7 opened this issue Nov 1, 2022 · 6 comments
Assignees
Labels
A-Push Issues related to push/notifications O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Regression Something broke which worked on a previous release

Comments

@deepbluev7
Copy link
Contributor

Description

When I curl /_matrix/client/v3/pushrules/ it doesn't return the .m.rule.contains_user_name rule. This means clients don't properly highlight those messages. It also isn't present in the pseudo account data event

Steps to reproduce

  • curl /_matrix/client/v3/pushrules/ and grep for user_name.

Homeserver

nheko.im, neko.dev, ocean.joedonofry.com

Synapse Version

1.71rc1, 1.69.0

Installation Method

Other (please mention below)

Platform

Docker, custom ebuild, etc

Relevant log output

None

Anything else that would be useful to know?

The rule is present in https://github.com/matrix-org/synapse/blob/2d0ba3f89aaf9545d81c4027500e543ec70b68a6/rust/src/push/base_rules.rs, which would suggest it is formatted. The formatting should happen here: https://github.com/matrix-org/synapse/blob/99a7e7e0230cba5d00ec204926edae89d4b6b8c3/synapse/push/clientformat.py

I have no idea yet, where it goes poof.

@DMRobertson DMRobertson added A-Push Issues related to push/notifications S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Release-Blocker Must be resolved before making a release X-Regression Something broke which worked on a previous release O-Occasional Affects or can be seen by some users regularly or most users rarely labels Nov 2, 2022
@DMRobertson
Copy link
Contributor

AFAICT from the spec, this rule is supposed to show up under the global/content.

This doesn't look promising:

$ curl -s -H "Authorization: Bearer $TOKEN" -X GET https://matrix-client.matrix.org/_matrix/client/v3/pushrules/ | jq '.global.content'
[]

@DMRobertson DMRobertson self-assigned this Nov 2, 2022
@DMRobertson
Copy link
Contributor

On 1.68.0, this snippet

template_rule = _rule_to_template(r)
if not template_rule:

sets template_rule to None where r is

PushRule(rule_id='global/content/.m.rule.contains_user_name', priority_class=4, conditions=[{'kind': 'event_match', 'key': 'content.body', 'pattern_type': 'user_localpart'}], actions=['notify', {'set_tweak': 'sound', 'value': 'default'}, {'set_tweak': 'highlight'}], default=True, default_enabled=True)

Not a regression then?

@DMRobertson
Copy link
Contributor

We return None here

if "pattern" not in thecond:
return None
because rule.conditions[0] is {'kind': 'event_match', 'key': 'content.body', 'pattern_type': 'user_localpart'}.

@DMRobertson
Copy link
Contributor

DMRobertson commented Nov 2, 2022

There is code here to account for this, but it happens after we've already decided the template_rule is None.

# Remove internal stuff.
template_rule["conditions"] = copy.deepcopy(template_rule["conditions"])
for c in template_rule["conditions"]:
c.pop("_cache_key", None)
pattern_type = c.pop("pattern_type", None)
if pattern_type == "user_id":
c["pattern"] = user.to_string()
elif pattern_type == "user_localpart":
c["pattern"] = user.localpart
sender_type = c.pop("sender_type", None)
if sender_type == "user_id":
c["sender"] = user.to_string()

Would guess this was broken in #13522, so a regression in 1.66.

@DMRobertson DMRobertson removed the X-Release-Blocker Must be resolved before making a release label Nov 2, 2022
@DMRobertson
Copy link
Contributor

@erikjohnston I think you're best-placed to look at this when you're back. I don't fully grok the templaterule machinery here so wouldn't want to attempt a fix without your insight, at the very least.

@DMRobertson DMRobertson added this to the Revisit: Next Week milestone Nov 2, 2022
deepbluev7 added a commit to deepbluev7/synapse that referenced this issue Nov 2, 2022
Dirty fix for matrix-org#14348

A proper fix would have proper tests and factor out the pattern_type
cases as well as clean up the logic.

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@deepbluev7
Copy link
Contributor Author

I made a temporary fix in #14356. It is more of a bodge though, so you might want to fix it properly instead.

@erikjohnston erikjohnston removed this from the Revisit: Next Week milestone Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Push Issues related to push/notifications O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Regression Something broke which worked on a previous release
Projects
None yet
Development

No branches or pull requests

3 participants