-
Notifications
You must be signed in to change notification settings - Fork 343
Add report user API from MSC4260 #18120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
test failures appear unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some further notes and ideas below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very non-blocking concerns, happy to merge as-is, just let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we probably should have defined a way to delete these reports during the MSC process... Do we have a plan to clear reports out once they're handled?
Otherwise the below is the only blocking comment.
Like any other report: no. Implementations are left to their own decisions in this regard. We'll add applicable admin APIs later - I'd like to avoid overloading this PR. |
Sorry, I wasn't suggesting adding any more APIs to this PR. I was more wondering if there were plans to create tooling to remove reports. It sounds like there are, but from a different angle that the C-S API. Happy to discuss that more in a separate issue/MSC. |
honestly, the hope is to bypass the Synapse database entirely one day, but that's very long term thoughts. In the meantime, we'll slowly add admin APIs as we (T&S) need them, if others don't contribute them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. Thanks for working on this and other T&S features ❤️
if not self._hs.is_mine_id(target_user_id): | ||
return # hide that they're not ours/that we can't do anything about them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking, but would it not be useful for the reporter to get an error here telling them that the homeserver operator likely don't do much about remote users? And tell them to contact the remote homeserver's operator instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly/probably, though IIRC this is in the MSC.
One day we do also hope to support federated reports.
matrix-org/matrix-spec-proposals#4260
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.(run the linters)