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

Uniformize spam-checker API, part 3: Expand check_event_for_spam with the ability to return additional fields #12846

Merged
merged 4 commits into from
May 30, 2022
Merged

Uniformize spam-checker API, part 3: Expand check_event_for_spam with the ability to return additional fields #12846

merged 4 commits into from
May 30, 2022

Conversation

Yoric
Copy link
Contributor

@Yoric Yoric commented May 23, 2022 •
edited
Loading

Extends the check_event_for_spam module callback to support returning a tuple of (http_error_error, response_body_dict). This is something of an intermediate solution to unblock prototyping account suspension.

It turns out that returning an instance of Codes is not good enough to support that usecase, which requires the ability to lead the user to, for example, a webpage with the ToS they have broken. Originally we hoped that the client could be smart enough to handle this, but it seems that is not correct.

See also:

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.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@Yoric Yoric requested a review from a team as a code owner May 23, 2022 16:16
@Yoric Yoric requested a review from babolivier May 23, 2022 16:16
@Yoric Yoric changed the title Yoric/spam errors union Uniformize spam-checker API, part 3: Expand check_event_for_spam with the ability to return additional fields May 23, 2022
@Yoric Yoric force-pushed the yoric/spam-errors-union branch 2 times, most recently from 9a16801 to 092cb94 Compare May 23, 2022 16:19
Base automatically changed from yoric/spam-errors-2 to develop May 23, 2022 17:27
@Yoric Yoric force-pushed the yoric/spam-errors-union branch 2 times, most recently from cf78077 to 4abb05b Compare May 25, 2022 16:00
babolivier
babolivier suggested changes May 27, 2022
View reviewed changes
synapse/handlers/message.py Outdated Show resolved Hide resolved
changelog.d/12846.feature Outdated Show resolved Hide resolved
@Yoric Yoric requested review from babolivier and MadLittleMods May 30, 2022 08:46
@Yoric Yoric force-pushed the yoric/spam-errors-union branch 2 times, most recently from e2d4a1e to 4079736 Compare May 30, 2022 08:54
Yoric added 3 commits May 30, 2022 11:15
@Yoric
Uniformize spam-checker API, part 2: check_event_for_spam
16945be 
Signed-off-by: David Teller <[email protected]>
@Yoric
Uniformize spam-checker API, part 3: Expand check_event_for_spam with…
83b4130 
… the ability to return additional fields.
@Yoric
WIP: Applied feedback
c5e36ca 
See also:
- Part 1: #12703
- Part 2: #12808
- Adding an error code for suspended accounts #12845
@Yoric Yoric force-pushed the yoric/spam-errors-union branch from 4079736 to c5e36ca Compare May 30, 2022 09:15
babolivier
babolivier suggested changes May 30, 2022
View reviewed changes
Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

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

lgtm apart from this

synapse/handlers/message.py Outdated Show resolved Hide resolved
@Yoric Yoric requested a review from babolivier May 30, 2022 16:18
@babolivier babolivier merged commit af7db19 into develop May 30, 2022
@babolivier babolivier deleted the yoric/spam-errors-union branch May 30, 2022 16:24
richvdh
richvdh reviewed May 30, 2022
View reviewed changes
synapse/events/spamcheck.py
@@ -270,7 +275,7 @@ def register_callbacks(

async def check_event_for_spam(
self, event: "synapse.events.EventBase"
) -> Union[Decision, str]:
) -> Union[Decision, Tuple[Codes, Dict], str]:
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a super confusing api. why are there four different potential return types? (Decision is itself a Union of two types)

It also doesn't seem to correspond to the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

(it's this function's job to flatten the myriad different potential return types from the module callback into a sane api for the rest of synapse. It seems to be spectacularly failing at that job currently)

MadLittleMods added a commit to matrix-org/matrix-js-sdk that referenced this pull request Jun 2, 2022
@MadLittleMods
More detail on the context/rationale that should be included when con…
792321b 
…tributing

Follow-up to #1933

Spawning from various recent documents and comments:

 - https://github.com/vector-im/element-meta/wiki/Review-process
 - matrix-org/synapse#12846 (comment)
 - https://gitlab.matrix.org/new-vector/internal/-/wikis/Backend/Reviews
@MadLittleMods MadLittleMods mentioned this pull request Jun 2, 2022
Merged
Yoric added a commit that referenced this pull request Jun 6, 2022
@Yoric
Port spam-checker API callbacks user_may_join_room, `user_may_invit…
8166bd5 
…e`, `user_may_send_3pid_invite`, `user_may_create_room`, `user_may_create_room_alias`, `user_may_publish_room`, `check_media_file_for_spam` to more powerful and less ambiguous `Union[Literal["NOT_SPAM"], Codes]`. This is a followup to #12703, #12808, #12846 and builds towards giving the spam-checker API the ability to inform users of *why* their event or operation has been rejected.

WIP: Applying feedback.
MadLittleMods added a commit to matrix-org/matrix-js-sdk that referenced this pull request Jun 7, 2022
@MadLittleMods
Add more detail on the context/rationale that should be included when…
96c35e2 
… contributing (#2432)

Follow-up to #1933

Spawning from various recent documents and comments:

 - https://github.com/vector-im/element-meta/wiki/Review-process
 - matrix-org/synapse#12846 (comment)
 - https://gitlab.matrix.org/new-vector/internal/-/wikis/Backend/Reviews
@MadLittleMods MadLittleMods mentioned this pull request Jun 8, 2022
Merged
4 tasks
This was referenced Jun 8, 2022
Merged
Merged
@Yoric Yoric mentioned this pull request Jun 14, 2022
Merged
4 tasks
Fizzadar added a commit to Fizzadar/synapse that referenced this pull request Jun 15, 2022
@Fizzadar
Merge tag 'v1.61.0' into beeper-master
1f647d7 
Synapse 1.61.0 (2022-06-14)
===========================

This release removes support for the non-standard feature known both as 'groups' and as 'communities', which have been superseded by *Spaces*.

See [the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/docs/upgrade.md#upgrading-to-v1610)
for more details.

Improved Documentation
----------------------

- Mention removed community/group worker endpoints in [upgrade.md](https://github.com/matrix-org/synapse/blob/develop/docs/upgrade.md#upgrading-to-v1610s). Contributed by @olmari. ([\matrix-org#13023](matrix-org#13023))

Synapse 1.61.0rc1 (2022-06-07)
==============================

Features
--------

- Add new `media_retention` options to the homeserver config for routinely cleaning up non-recently accessed media. ([\matrix-org#12732](matrix-org#12732), [\matrix-org#12972](matrix-org#12972), [\matrix-org#12977](matrix-org#12977))
- Experimental support for [MSC3772](matrix-org/matrix-spec-proposals#3772): Push rule for mutually related events. ([\matrix-org#12740](matrix-org#12740), [\matrix-org#12859](matrix-org#12859))
- Update to the `check_event_for_spam` module callback: Deprecate the current callback signature, replace it with a new signature that is both less ambiguous (replacing booleans with explicit allow/block) and more powerful (ability to return explicit error codes). ([\matrix-org#12808](matrix-org#12808))
- Add storage and module API methods to get monthly active users (and their corresponding appservices) within an optionally specified time range. ([\matrix-org#12838](matrix-org#12838), [\matrix-org#12917](matrix-org#12917))
- Support the new error code `ORG.MATRIX.MSC3823.USER_ACCOUNT_SUSPENDED` from [MSC3823](matrix-org/matrix-spec-proposals#3823). ([\matrix-org#12845](matrix-org#12845), [\matrix-org#12923](matrix-org#12923))
- Add a configurable background job to delete stale devices. ([\matrix-org#12855](matrix-org#12855))
- Improve URL previews for pages with empty elements. ([\matrix-org#12951](matrix-org#12951))
- Allow updating a user's password using the admin API without logging out their devices. Contributed by @jcgruenhage. ([\matrix-org#12952](matrix-org#12952))

Bugfixes
--------

- Always send an `access_token` in `/thirdparty/` requests to appservices, as required by the [Application Service API specification](https://spec.matrix.org/v1.1/application-service-api/#third-party-networks). ([\matrix-org#12746](matrix-org#12746))
- Implement [MSC3816](matrix-org/matrix-spec-proposals#3816): sending the root event in a thread should count as having 'participated' in it. ([\matrix-org#12766](matrix-org#12766))
- Delete events from the `federation_inbound_events_staging` table when a room is purged through the admin API. ([\matrix-org#12784](matrix-org#12784))
- Fix a bug where we did not correctly handle invalid device list updates over federation. Contributed by Carl Bordum Hansen. ([\matrix-org#12829](matrix-org#12829))
- Fix a bug which allowed multiple async operations to access database locks concurrently. Contributed by @sumnerevans @ Beeper. ([\matrix-org#12832](matrix-org#12832))
- Fix an issue introduced in Synapse 0.34 where the `/notifications` endpoint would only return notifications if a user registered at least one pusher. Contributed by Famedly. ([\matrix-org#12840](matrix-org#12840))
- Fix a bug where servers using a Postgres database would fail to backfill from an insertion event when MSC2716 is enabled (`experimental_features.msc2716_enabled`). ([\matrix-org#12843](matrix-org#12843))
- Fix [MSC3787](matrix-org/matrix-spec-proposals#3787) rooms being omitted from room directory, room summary and space hierarchy responses. ([\matrix-org#12858](matrix-org#12858))
- Fix a bug introduced in Synapse 1.54.0 which could sometimes cause exceptions when handling federated traffic. ([\matrix-org#12877](matrix-org#12877))
- Fix a bug introduced in Synapse 1.59.0 which caused room deletion to fail with a foreign key violation error. ([\matrix-org#12889](matrix-org#12889))
- Fix a long-standing bug which caused the `/messages` endpoint to return an incorrect `end` attribute when there were no more events. Contributed by @Vetchu. ([\matrix-org#12903](matrix-org#12903))
- Fix a bug introduced in Synapse 1.58.0 where `/sync` would fail if the most recent event in a room was a redaction of an event that has since been purged. ([\matrix-org#12905](matrix-org#12905))
- Fix a potential memory leak when generating thumbnails. ([\matrix-org#12932](matrix-org#12932))
- Fix a long-standing bug where a URL preview would break if the image failed to download. ([\matrix-org#12950](matrix-org#12950))

Improved Documentation
----------------------

- Fix typographical errors in documentation. ([\matrix-org#12863](matrix-org#12863))
- Fix documentation incorrectly stating the `sendToDevice` endpoint can be directed at generic workers. Contributed by Nick @ Beeper. ([\matrix-org#12867](matrix-org#12867))

Deprecations and Removals
-------------------------

- Remove support for the non-standard groups/communities feature from Synapse. ([\matrix-org#12553](matrix-org#12553), [\matrix-org#12558](matrix-org#12558), [\matrix-org#12563](matrix-org#12563), [\matrix-org#12895](matrix-org#12895), [\matrix-org#12897](matrix-org#12897), [\matrix-org#12899](matrix-org#12899), [\matrix-org#12900](matrix-org#12900), [\matrix-org#12936](matrix-org#12936), [\matrix-org#12966](matrix-org#12966))
- Remove contributed `kick_users.py` script. This is broken under Python 3, and is not added to the environment when `pip install`ing Synapse. ([\matrix-org#12908](matrix-org#12908))
- Remove `contrib/jitsimeetbridge`. This was an unused experiment that hasn't been meaningfully changed since 2014. ([\matrix-org#12909](matrix-org#12909))
- Remove unused `contrib/experiements/cursesio.py` script, which fails to run under Python 3. ([\matrix-org#12910](matrix-org#12910))
- Remove unused `contrib/experiements/test_messaging.py` script. This fails to run on Python 3. ([\matrix-org#12911](matrix-org#12911))

Internal Changes
----------------

- Test Synapse against Complement with workers. ([\matrix-org#12810](matrix-org#12810), [\matrix-org#12933](matrix-org#12933))
- Reduce the amount of state we pull from the DB. ([\matrix-org#12811](matrix-org#12811), [\matrix-org#12964](matrix-org#12964))
- Try other homeservers when re-syncing state for rooms with partial state. ([\matrix-org#12812](matrix-org#12812))
- Resume state re-syncing for rooms with partial state after a Synapse restart. ([\matrix-org#12813](matrix-org#12813))
- Remove Mutual Rooms' ([MSC2666](matrix-org/matrix-spec-proposals#2666)) endpoint dependency on the User Directory. ([\matrix-org#12836](matrix-org#12836))
- Experimental: expand `check_event_for_spam` with ability to return additional fields. This enables spam-checker implementations to experiment with mechanisms to give users more information about why they are blocked and whether any action is needed from them to be unblocked. ([\matrix-org#12846](matrix-org#12846))
- Remove `dont_notify` from the `.m.rule.room.server_acl` rule. ([\matrix-org#12849](matrix-org#12849))
- Remove the unstable `/hierarchy` endpoint from [MSC2946](matrix-org/matrix-spec-proposals#2946). ([\matrix-org#12851](matrix-org#12851))
- Pull out less state when handling gaps in room DAG. ([\matrix-org#12852](matrix-org#12852), [\matrix-org#12904](matrix-org#12904))
- Clean-up the push rules datastore. ([\matrix-org#12856](matrix-org#12856))
- Correct a type annotation in the URL preview source code. ([\matrix-org#12860](matrix-org#12860))
- Update `pyjwt` dependency to [2.4.0](https://github.com/jpadilla/pyjwt/releases/tag/2.4.0). ([\matrix-org#12865](matrix-org#12865))
- Enable the `/account/whoami` endpoint on synapse worker processes. Contributed by Nick @ Beeper. ([\matrix-org#12866](matrix-org#12866))
- Enable the `batch_send` endpoint on synapse worker processes. Contributed by Nick @ Beeper. ([\matrix-org#12868](matrix-org#12868))
- Don't generate empty AS transactions when the AS is flagged as down. Contributed by Nick @ Beeper. ([\matrix-org#12869](matrix-org#12869))
- Fix up the variable `state_store` naming. ([\matrix-org#12871](matrix-org#12871))
- Faster room joins: when querying the current state of the room, wait for state to be populated. ([\matrix-org#12872](matrix-org#12872))
- Avoid running queries which will never result in deletions. ([\matrix-org#12879](matrix-org#12879))
- Use constants for EDU types. ([\matrix-org#12884](matrix-org#12884))
- Reduce database load of `/sync` when presence is enabled. ([\matrix-org#12885](matrix-org#12885))
- Refactor `have_seen_events` to reduce memory consumed when processing federation traffic. ([\matrix-org#12886](matrix-org#12886))
- Refactor receipt linearization code. ([\matrix-org#12888](matrix-org#12888))
- Add type annotations to `synapse.logging.opentracing`. ([\matrix-org#12894](matrix-org#12894))
- Remove PyNaCl occurrences directly used in Synapse code. ([\matrix-org#12902](matrix-org#12902))
- Bump types-jsonschema from 4.4.1 to 4.4.6. ([\matrix-org#12912](matrix-org#12912))
- Rename storage classes. ([\matrix-org#12913](matrix-org#12913))
- Preparation for database schema simplifications: stop reading from `event_edges.room_id`. ([\matrix-org#12914](matrix-org#12914))
- Check if we are in a virtual environment before overriding the `PYTHONPATH` environment variable in the demo script. ([\matrix-org#12916](matrix-org#12916))
- Improve the logging when signature checks on events fail. ([\matrix-org#12925](matrix-org#12925))

# -----BEGIN PGP SIGNATURE-----
#
# iQFEBAABCgAuFiEEBTGR3/RnAzBGUif3pULk7RsPrAkFAmKoaa0QHGVyaWtAbWF0
# cml4Lm9yZwAKCRClQuTtGw+sCVn3B/sF8hdhrZ7hWW40ST3eG9cEKNFrj/xZXiaI
# zho3ryrxaQF68BSKot15AvZSprEdwBXWrb8WeTjyw+QH7vTKrCQDZ0p7qubn10Z7
# BuKq9hyYjyCLjBZrgy8d4U3Y8gsSByuO59YKHNLn+UTJLOs5GTH8Wprwh4mpU3Jl
# +o+cC+lMSVcyZij2hihFymcSxWq/I9WL0dsjRif8x0BUQwRXwmXc6+mhlgLBe2Zs
# 2dUouzJ8NVZcjfWvsg4noXPrNQ/IiyCVZlSIgaDftDIxVSPk5/rXiUUNex8Tn1+I
# TnOgnXhpOQD1vwVGYS8LrcfA0ubSili7xUJ8k2e5TkCjpkaVnYNu
# =q+7N
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue Jun 14 11:57:49 2022 BST
# gpg:                using RSA key 053191DFF4670330465227F7A542E4ED1B0FAC09
# gpg:                issuer "[email protected]"
# gpg: WARNING: server 'dirmngr' is older than us (2.2.34 < 2.3.6)
# gpg: Note: Outdated servers may lack important security fixes.
# gpg: Note: Use the command "gpgconf --kill all" to restart them.
# gpg: Can't check signature: No public key

# Conflicts:
#	docs/workers.md
#	synapse/handlers/message.py
#	synapse/handlers/pagination.py
#	synapse/push/bulk_push_rule_evaluator.py
#	synapse/push/push_rule_evaluator.py
#	synapse/rest/client/receipts.py
#	synapse/storage/databases/main/appservice.py
#	tests/push/test_push_rule_evaluator.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh left review comments

@clokep clokep clokep left review comments

@babolivier babolivier babolivier approved these changes

@MadLittleMods MadLittleMods Awaiting requested review from MadLittleMods

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Yoric @clokep @MadLittleMods @richvdh @babolivier