Implementation of MSC2938 #9207

Yoric · 2021-01-22T11:41:27Z

Context

This patch implements (a subset of) MSC 2938, i.e. the ability for users to report content to room moderators instead of system administrators.

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

clokep · 2021-01-22T12:15:07Z

@Yoric asked for some early feedback on this.

clokep · 2021-01-26T17:42:32Z

This relates to matrix-org/matrix-spec-proposals#2938.

clokep

I put some thoughts down on the code here. I think my main concern is how different reporting to a room moderator vs. a server admin is (one stores in the db, the other sends a notices message). I'm also a bit concerned that this might not make sense on other homeserver implementations or for all Synapse configs.

synapse/rest/client/v2_alpha/report_event.py

clokep · 2021-01-26T17:40:29Z

synapse/rest/client/v2_alpha/report_event.py

Please add docstrings and type hints.

This seems very different than how we handle the ones for homeserver admins. Does this behavior make sense?

In particular note that the server notices system is disabled by default in Synapse.

In particular is there need for any more state to be kept of these reports (e.g. the admin one gets stored into a table).

Please add docstrings and type hints.

This seems very different than how we handle the ones for homeserver admins. Does this behavior make sense?

Well, this is being summarized/discussed here. It seems to be the best way for moderators to actually receive the info.

In particular note that the server notices system is disabled by default in Synapse.

It is enabled on matrix.org and EMS, though, right?

In particular is there need for any more state to be kept of these reports (e.g. the admin one gets stored into a table).

Not as far as I can tell. It's a simple message. We can't do that for the homeserver admin because the homeserver admin doesn't have an account, but in the future, we could imagine changing how it works for admins.

synapse/rest/client/v2_alpha/report_event.py

ShadowJonathan · 2021-02-01T15:42:51Z

Continuing thoughts from matrix-org/matrix-spec-proposals#2938, maybe the best course of actions would be to send EDUs to target servers (which're more likely to be dropped safely if the remote server does not support this), and add a hook into the federation receiving mechanism to check for these EDUs, and then add them to an internal database?

Afterwards, when a potential MSC for homeserver-to-homeserver rooms is made ready, the mechanism can switch to that approach, while keeping the database and federation scoop.

This makes the federation-injection approach a glass-cannon hotfix for now (since it's fairly urgent), with the downside of it not being resilient to server downtime of the other side, while in the future a better approach can be steamrolled.

Yoric · 2021-03-19T11:18:41Z

Ok, ready for a new round of review.
We're still missing federation tests.

Yoric · 2021-03-29T10:10:30Z

@clokep Review ping?

richvdh

@Yoric what are you looking for here? Feedback on the MSC? Feedback on whether the impl matches the MSC? Are there particular parts of the impl you have questions on?

The impl looks generally plausible, though as I point out I'd like to see part of it split out to a Handler class.

I'm a little worried about DoS vectors: it seems like it would be easy for a malicious client or federated server to spew hundreds of abuse reports which stack up into server notices. (Of course, that vector exists today, but it's hard to turn into a proper DoS.) Maybe we should apply some pre-emptive rate-limiting?

synapse/rest/client/v2_alpha/report_event.py

synapse/event_auth.py

synapse/rest/client/v2_alpha/report_event.py

Yoric · 2021-03-30T11:22:42Z

@Yoric what are you looking for here? Feedback on the MSC? Feedback on whether the impl matches the MSC? Are there particular parts of the impl you have questions on?

Essentially, I'd like to land this at some point during the next few weeks and then start experimenting with using this in the front-end. So, I'm looking for anything that can bring me closer to that end!

(I will apply your feedback asap)

Yoric · 2021-04-08T13:27:18Z

Maybe we should apply some pre-emptive rate-limiting?

Done.

Yoric · 2021-04-08T15:22:49Z

Note: I still need to write sytests.

richvdh · 2021-04-08T21:22:25Z

[looks like your tests are failing]

synapse/handlers/abuse_reports.py

richvdh

the diff (https://github.com/matrix-org/synapse/pull/9207/files) seems to contain a bunch of unrelated changes: can you fix that so that this can be reviewed, please?

As a matter of general principle, in future please avoid rebasing PR branches once they have received a review, so that it's possible to see how it has changed since the previous review.

Yoric · 2021-04-09T11:36:05Z

Apologies, looks like a merge error.

As a matter of general principle, in future please avoid rebasing PR branches once they have received a review, so that it's possible to see how it has changed since the previous review.

I'm not sure how I can do that when the review process has started 4 months ago and the work on typing has changed much of the underlying code since then. What's the policy in such cases?

Signed-off-by: David Teller <[email protected]>

richvdh · 2021-04-09T12:27:23Z

As a matter of general principle, in future please avoid rebasing PR branches once they have received a review, so that it's possible to see how it has changed since the previous review.

I'm not sure how I can do that when the review process has started 4 months ago and the work on typing has changed much of the underlying code since then. What's the policy in such cases?

I agree if it's a complete rewrite then there's not much value in a merge rather than a rebase, but I'm assuming it hasn't completely changed since I first reviewed this (11 days ago).

Yoric · 2021-04-12T13:15:06Z

I agree if it's a complete rewrite then there's not much value in a merge rather than a rebase, but I'm assuming it hasn't completely changed since I first reviewed this (11 days ago).

I believe that we're using different meanings of the word "rebase". I'll try and avoid overwriting history in the future!

richvdh · 2021-04-12T15:00:42Z

I'm referring to git rebase ;-p. Thanks!

synapse/event_auth.py

synapse/handlers/abuse_reports.py

synapse/server.py

synapse/handlers/abuse_reports.py

richvdh · 2021-04-13T21:49:51Z

synapse/handlers/abuse_reports.py

+        self.federation_sender = None
+        if hs.should_send_federation():
+            self.federation_sender = hs.get_federation_sender()
+        hs.get_federation_registry().register_edu_handler(
+            "org.matrix.m.content_report", self.on_receive_report_through_federation
+        )


I think this stuff probably isn't going to work right in a multi-worker synapse deployment, though I think fixing that is out of scope for this PR review...

synapse/handlers/abuse_reports.py

richvdh

getting there! could you implement the msc2938_enabled setting properly please?

richvdh · 2021-04-29T17:21:08Z

synapse/handlers/abuse_reports.py

-                retry_after_ms=int(1000 * (time_allowed - time_now_s))
-            )
+        """
+        Report an event as abuse.


Suggested change

Report an event as abuse.

Handle a request from a client to report an event as abuse.

richvdh · 2021-04-29T17:21:37Z

synapse/handlers/abuse_reports.py

+            room_id: The room in which the event took place.
+            reason: The human-readable reason provided by the user.
+            content: A JSON dictionary `{reason: String?, score: Number?}`.
+            score Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".


Suggested change

score Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".

score: Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".

richvdh · 2021-04-29T17:22:21Z

synapse/handlers/abuse_reports.py

+            room_id: The room in which the event took place.
+            reason: The human-readable reason provided by the user.
+            content: A JSON dictionary `{reason: String?, score: Number?}`.
+            score Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".


in what way is it optional? the type declaration is int, not Optional.

richvdh · 2021-04-29T17:22:30Z

synapse/handlers/abuse_reports.py

+            reason: The human-readable reason provided by the user.
+            content: A JSON dictionary `{reason: String?, score: Number?}`.
+            score Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".
+            nature An optional flag to aid classification and prioritization of abuse reports, e.g. "m.abuse.spam".


Suggested change

nature An optional flag to aid classification and prioritization of abuse reports, e.g. "m.abuse.spam".

nature: An optional flag to aid classification and prioritization of abuse reports, e.g. "m.abuse.spam".

richvdh · 2021-04-29T17:23:37Z

synapse/config/experimental.py

        self.msc3026_enabled = experimental.get("msc3026_enabled", False)  # type: bool
+
+        # MSC2938 (report to moderator)
+        self.msc2938_enabled = experimental.get("msc2938_enabled", False)  # type: bool


I don't think this is actually used?

richvdh · 2021-04-29T17:25:03Z

synapse/handlers/abuse_reports.py

+            room_id The room in which the event took place.
+            event_id The event to report. We do not check whether the event took place in that room.
+            user_id The user who reported the event. We do not check whether the user can actually witness the event.
+            reason The human-readable reason provided by the user.
+            nature An optional flag to aid classification and prioritization of abuse reports, e.g. "abuse.spam".
+            score Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".


all need :

(also, could they be in the right order, please?)

richvdh · 2021-04-29T17:26:51Z

synapse/handlers/abuse_reports.py


        # A little sanity check on the event itself.
        event = await self.store.get_event(event_id=event_id, check_room_id=None)
        if event.room_id != room_id.to_string():


again I ask: why not let get_event do this check for you?

Generally it's a pretty poor use of your time and mine for me to make the same review comment multiple times. If something I've written is unclear, just comment on the thread and I'll do my best to clarify.

richvdh · 2021-04-29T17:38:31Z

synapse/rest/client/v2_alpha/report_event.py

-            request, requester.user, body, room_id, event_id
+        await self._rate_limiter.ratelimit(requester=requester)
+
+        if not isinstance(body["reason"], str):


this will 500 if there is no reason property.

Suggested change

if not isinstance(body["reason"], str):

reason = body.get("reason")

if not isinstance(reason, str):

likewise below for score.

richvdh · 2021-06-02T14:16:32Z

@Yoric I believe you're planning to abandon this in favour of MSC3215 - are you happy for us to close this PR?

Yoric · 2021-06-02T14:22:54Z

Yes, let's close it.

Yoric marked this pull request as draft January 22, 2021 11:41

clokep requested a review from a team January 22, 2021 12:14

clokep reviewed Jan 26, 2021

View reviewed changes

Yoric requested a review from clokep March 19, 2021 11:18

Yoric force-pushed the develop-msc2938 branch 2 times, most recently from 06d07f8 to 2a7ac22 Compare March 19, 2021 13:23

richvdh requested a review from a team March 29, 2021 10:56

clokep removed their request for review March 29, 2021 14:15

richvdh reviewed Mar 29, 2021

View reviewed changes

Yoric force-pushed the develop-msc2938 branch 2 times, most recently from 5b1c536 to 7113dc1 Compare April 7, 2021 15:14

Yoric force-pushed the develop-msc2938 branch 3 times, most recently from 2b164b9 to 09ddd87 Compare April 8, 2021 15:14

Yoric changed the title ~~[WIP] Prototyping MSC 2938~~ Prototyping MSC 2938 Apr 8, 2021

Yoric marked this pull request as ready for review April 8, 2021 15:22

Yoric requested a review from richvdh April 8, 2021 15:22

richvdh requested a review from a team April 8, 2021 21:19

richvdh reviewed Apr 9, 2021

View reviewed changes

synapse/handlers/abuse_reports.py Outdated Show resolved Hide resolved

Yoric requested a review from richvdh April 9, 2021 10:44

richvdh changed the title ~~Prototyping MSC 2938~~ Implementation of MSC2938 Apr 9, 2021

richvdh suggested changes Apr 9, 2021

View reviewed changes

Yoric added 12 commits April 9, 2021 13:37

Prototyping MSC 2938

45916fc

Signed-off-by: David Teller <[email protected]>

[WIP] Now with tests and an early prototype for federation

39e1b89

[FIX] Remove runtime circular dependency

5e03df8

Now with a changelog

3f3ebc6

FIXUP: Applying feedback

e990ba0

FIXUP: Fixing tests

9a41d16

FIXUP: Merging

cf65e77

FIXUP: Better if we don't forget files

5e1dafe

FIXUP: Rate-limiting

3348171

FIXUP: Rate limiting

db710ea

FIXUP: EventID is a lie

457ccef

FIXUP: Also, linting

92f3c7f

Yoric force-pushed the develop-msc2938 branch from 549a431 to 92f3c7f Compare April 9, 2021 11:37

richvdh self-requested a review April 9, 2021 12:31

richvdh suggested changes Apr 13, 2021

View reviewed changes

Yoric added 2 commits April 14, 2021 17:42

FIXUP: Applying feedback

254d492

FIXUP

ac0ef8c

Yoric requested a review from richvdh April 20, 2021 14:23

richvdh suggested changes Apr 29, 2021

View reviewed changes

erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label May 20, 2021

Yoric closed this Jun 2, 2021

	Report an event as abuse.
	Handle a request from a client to report an event as abuse.

	score Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".
	score: Optionally, a "badness" score where -100 is "really bad" and 0 is "acceptable".

	nature An optional flag to aid classification and prioritization of abuse reports, e.g. "m.abuse.spam".
	nature: An optional flag to aid classification and prioritization of abuse reports, e.g. "m.abuse.spam".

	if not isinstance(body["reason"], str):
	reason = body.get("reason")
	if not isinstance(reason, str):

Uh oh!

Implementation of MSC2938 #9207

Implementation of MSC2938 #9207

Uh oh!

Conversation

Yoric commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Pull Request Checklist

Uh oh!

clokep commented Jan 22, 2021

Uh oh!

clokep commented Jan 26, 2021

Uh oh!

clokep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ShadowJonathan commented Feb 1, 2021

Uh oh!

Yoric commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yoric commented Mar 29, 2021

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yoric commented Mar 30, 2021

Uh oh!

Yoric commented Apr 8, 2021

Uh oh!

Yoric commented Apr 8, 2021

Uh oh!

richvdh commented Apr 8, 2021

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Yoric commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richvdh commented Apr 9, 2021

Uh oh!

Yoric commented Apr 12, 2021

Uh oh!

richvdh commented Apr 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Yoric commented Jan 22, 2021 •

edited

Loading

Yoric commented Mar 19, 2021 •

edited

Loading

Yoric commented Apr 9, 2021 •

edited

Loading