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

Bring spamchecker to parity with mainline #108

Merged
merged 11 commits into from
Nov 11, 2021

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Oct 26, 2021

Note to reviewer:

Apologies for the massive PR size, as splitting turned out to be very non trivial (see #108 (comment))

Can be reviewed commit by commit, or through the following diffs (which exclude cherry picks and merge conflict resolutions):

First 3 commits are git cherry-picks of:

The 5th commit is a git cherry-pick of matrix-org/synapse#11204.

Note to ops:

When deploying a Synapse version including this PR, a Synapse config change is required. All that's needed should be to move the config for the DomainRuleChecker from the spam_checker section to the modules one of the configuration file, since I also had to port the module to the new system. So this means this bit of configuration:

spam_checker:
    - module: synapse.rulecheck.DomainRuleChecker
      config: {...}

Should become:

modules:
    - module: synapse.rulecheck.DomainRuleChecker
      config: {...}

Note that this PR also removes support for the now useless can_only_create_one_to_one_rooms config option, but Synapse or the module won't fail if this option is kept around.

babolivier and others added 4 commits October 26, 2021 11:11
…vites (#10898)

This is in the context of creating new module callbacks that modules in https://github.com/matrix-org/synapse-dinsic can use, in an effort to reconcile the spam checker API in synapse-dinsic with the one in mainline.

This adds a callback that's fairly similar to user_may_create_room except it also allows processing based on the invites sent at room creation.
Co-authored-by: Erik Johnston <erik@matrix.org>
This is in the context of creating new module callbacks that modules in https://github.com/matrix-org/synapse-dinsic can use, in an effort to reconcile the spam checker API in synapse-dinsic with the one in mainline.

Note that a module callback already exists for 3pid invites (https://matrix-org.github.io/synapse/develop/modules/third_party_rules_callbacks.html#check_threepid_can_be_invited) but it doesn't check whether the sender of the invite is allowed to send it.
Bring other callbacks to party with mainline, and fixup code calling to
the various callbacks.
@babolivier
Copy link
Contributor Author

Tests are expected to fail currently, since I haven't updated the DomainRuleChecker yet.

@babolivier babolivier force-pushed the babolivier/update_spam_checker branch from c82793a to 23e48a1 Compare November 1, 2021 14:24
@babolivier babolivier marked this pull request as ready for review November 3, 2021 16:38
@babolivier babolivier requested a review from a team November 3, 2021 16:38
@babolivier babolivier force-pushed the babolivier/update_spam_checker branch 3 times, most recently from c7552b2 to 81c2455 Compare November 4, 2021 09:35
@babolivier babolivier force-pushed the babolivier/update_spam_checker branch from 81c2455 to ed31c6b Compare November 4, 2021 09:40
@erikjohnston
Copy link
Member

Would it be easier to have the commits that are pure cherry-picks a separate PR? Or do the other commits unbreak stuff?

@babolivier
Copy link
Contributor Author

Would it be easier to have the commits that are pure cherry-picks a separate PR? Or do the other commits unbreak stuff?

Definitely possible, I was thinking it would be just as simple to ignore them and review the other commits but now I'm not sure that's the correct approach. I'll close this PR and open two new ones.

@babolivier babolivier closed this Nov 8, 2021
@erikjohnston
Copy link
Member

Thanks!

@babolivier
Copy link
Contributor Author

babolivier commented Nov 8, 2021

@erikjohnston Actually, I've tried doing this, and found that things are kind of clashing because of differing signatures for callbacks with the same name between mainline and this repo (e.g. user_may_invite has extra arguments in its synapse-dinsic incarnation).

So opening a PR with just the cherry-picks means the DomainRuleChecker tests fail because the module hasn't been updated to use the new versions of the callbacks yet. And I can't seem to find a way to both make the tests work and have mypy not complain about calls not following type definitions, or mismatch between calls added in mainline and those added in this repo, etc, without making a cherry-pick-only PR result in something too messy for its own good which would be some kind of hacky mix between the spamchecker interface from mainline and the current one.

So I'm afraid I think I'll have to reopen this PR.

To help with review, these are the diffs with actual changes that aren't just cherry-picks and resolved merge conflicts:

c9c1bed

https://github.com/matrix-org/synapse-dinsic/pull/108/files/23e48a1f8ef61de97abd7a5c00b38e601fd6e9a4..ed31c6b1f8e035b520812be9ad101ae53d7525c6

I've updated the PR description to reflect this.

@@ -0,0 +1 @@
Add a `user_may_send_3pid_invite` spam checker callback for modules to allow or deny 3PID invites.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep the changelogs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we care, we don't generate changelogs with towncrier for this repo.

@babolivier babolivier merged commit cef83c7 into dinsic Nov 11, 2021
babolivier added a commit that referenced this pull request Dec 6, 2021
The module has now moved to https://github.com/matrix-org/synapse-domain-rule-checker

__Note to ops__:

To deploy this change, the module needs to be installed in addition to Synapse (from the aforementioned repo, or the PyPI project mentioned its readme). The configuration doesn't change besides a) `default` is renamed into `can_invite_if_not_in_domain_mapping` and b) the module's configuration has moved to the `modules` section of the configuration file (though that was already the case as of #108).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants