Update MSC4028 implementation: Enable the unstable push rule by default

[giomfo]

Since MSC4028 has been implemented, I updated the spec about the unstable push rule with this commit.

Indeed the unstable push rule org.matrix.msc4028.encrypted_event used during development should be enabled by default like the stable push rule.

This change has been triggered by a comment from the mobile dev team here. I realized that it was not risky to enabled this unstable push rule by default because we are using the experimental mode. Apply the same enabling value on the unstable and the stable push rules would make more relevant the current tests of this feature. See here the current test plan.

I would add here a question for the Backend team: How the new push rule is handled when the MSC has successfully passed a merge FCP? I mean when the feature becomes stable.
Is the unstable push rule (stored in the existing account data) renamed/replaced automatically with the stable push rule?

[Velin92]

Hello!
Some users are actually starting to experiment on their homeserver to these settings with Element X, and most of them by simply reading the MSC spec, think that when enabling the MSC in the server configuration, will automatically have the push rule also enabled for all their users, but as of right now that's not the case.

Recently discussed here:
element-hq/element-x-ios#2411

[anoadragon453]

For context, the unstable org.matrix.msc4028.encrypted_event is included in every user's default base append override rules. However, the push rule will only be evaluated if the experimental_features.msc4028_push_encrypted_events config option is set to true:

synapse/rust/src/push/mod.rs

Lines 593 to 597 in d4e3ad0

	if !self.msc4028_push_encrypted_events
	&& rule.rule_id == "global/override/.org.matrix.msc4028.encrypted_event"
	{
	return false;
	}

How the new push rule is handled when the MSC has successfully passed a merge FCP? I mean when the feature becomes stable.

Ideally we would just rename it in the codebase and provide a database migration to do the same for users automatically. This would result in push notifications being sent to mobile clients with the new rules immediately.

As for clients that compute push rules locally... do clients regularly pull down their push rules? If so, then we don't need to worry about telling clients proactively and can wait until they update themselves.

---

I apologise for such a slow response on this ticket. What would you currently like to see done about this issue? I see that the MSC hasn't moved forward much in the meantime, and is still not accepted.

[giomfo]

Thanks @anoadragon453

What would you currently like to see done about this issue?

I'm requesting here to update the synapse implementation of MSC4028 in order to enable by default the unstable push rule org.matrix.msc4028.encrypted_event. By this way the synapse implementation will be aligned to my last MSC update

I see that the MSC hasn't moved forward much in the meantime, and is still not accepted.

Yes, a plan was defined to move progressively on this MSC by enabling it progressively: https://github.com/matrix-org/internal-config/issues/1469. Unfortunately our efforts went on other topic.
This is important to get this update at synapse level, to be aligned with the MSC.
This MSC is the solution I promoted/promote internally to support mention_only notifications on mobile app. I think that the opportunity to work on/test it on EX will be back soon (when the EX team will have bandwidth)

[anoadragon453]

@giomfo generally we don't enable experimental features by default until they've been accepted into the Matrix spec. Doing so would enable it on matrix.org by default as well. I'm afraid still I don't understand why enabling the feature by default on all Synapse instances would make testing easier.

If you need the feature enabled by default on beta.matrix.org, we can do that. In addition, we're able to enable specific experimental features for specific users on matrix.org these days.

[giomfo]

@anoadragon453 I think you misunderstood my request. I'm asking here to enable the new push rule by default. I'm not asking to enable the feature itself. I agree that about the feature we should consider beta.matrix.org (this is the plan described here)

The mistake that I did when I wrote the MSC is to disable the unstable push rule by default (in its definition), which is not relevant and useless, because the unstable push rule is added as an experimental feature (when it is enabled).

Hope this is clearer, ping me for a quick sync if not

[anoadragon453]

I see. So you would like to have the unstable push rule in client's push rules by default, even if the feature is disabled on the homeserver.

The homeserver would ignore the push rule and not process it if the feature is disabled, and clients that don't know what the unstable push rule means won't process it either.

I suppose that's fine, and I can whip up a PR with the appropriate logic.

[anoadragon453]

@giomfo Ah, I've just realised that by "enabling" the push rule, you may have been referring to setting the enabled flag on the push rule. So flipping this line from false to true.

However only doing that, while leaving the experimental feature disabled, will still result in the push rule being filtered out before being sent to clients, due to this logic. Would you like to not filter the push rule even when the experimental feature is disabled?

I don't think there's really an issue with doing so, as only clients that understand the unstable push rule should (in theory) act on it.

[giomfo]

I've just realised that by "enabling" the push rule, you may have been referring to setting the enabled flag on the push rule. So flipping this line from false to true.

Yes, exactly 😄

Would you like to not filter the push rule even when the experimental feature is disabled?

I would prefer to follow the same behavior as the other experimental feature. My request is mainly to have the right enabled flag in the unstable push rule when we run some experimentations

[anoadragon453]

I've created a PR for this at #17826

Please let me know if this does not satisfy the requirements.

[giomfo]

PR #17143 fixed my request 👍