Add an Admin API endpoint to redact all a user's events #17506
Conversation
H-Shay
changed the title
H-Shay
force-pushed
the
shay/admin_redact
branch
from
c7630ce
to
2eb245b
Compare
|
This looks like a potentially long process, you should probably use the resumable task scheduler so that it survives a restart, and move to an async friendly API, similarly to the delete room v2 endpoint. |
|
Also feel free to ping me in Matrix (@mat:tout.im) if you need some clarity on how the resumable task scheduler work. |
Great point thanks for the tip - I've gone ahead and done so :) |
|
sidenote: MSC4084: Mass redactions would make this a lot more efficient. |
|
complement failures are flakey partial-state join tests - unrelated |
There was a problem hiding this comment.
I'd like to see the impersonation of the event sender explored before merging this PR, as I agree with @MatMaul that you're likely to need that functionality to redact the majority of a user's messages. Perhaps start with a test with a room that contains the user, but not the admin requester, and check whether their messages are redacted.
I'd also like to see an allowlist of event types to redact. I don't think it makes sense to redact state events that a user has sent, for instance (except in the case of m.room.member joins, which could contain offensive display name data).
|
Right so I think this is ready for another look. My only concern is the filter mechanism for event types is a bit noddy, any suggestions on how to make that more graceful (or if it matters) are appreciated. |
|
@anoadragon453 just wondering if the request review notif got lost? This should be ready for another go-around and is hopefully very close to done. |
synapse/handlers/admin.py
Outdated
| if content: | ||
| if content.get("membership") == "join": | ||
| if content.get("membership") == "Membership.JOIN": |
There was a problem hiding this comment.
Oh err, Membership.JOIN is a type. Looks like we may be missing a test for this codepath? 😄
There was a problem hiding this comment.
Right I have switched it back and ensured that the tests are verifying that we are redacting the join event.
There was a problem hiding this comment.
Thanks for updating the tests! Using Membership.JOIN (without quotes) should work though? It's equivalent to "join", but uses a Final instead of a loose string.
synapse/handlers/admin.py
Outdated
| if content: | ||
| if content.get("membership") == "join": | ||
| if content.get("membership") == "Membership.JOIN": |
There was a problem hiding this comment.
Thanks for updating the tests! Using Membership.JOIN (without quotes) should work though? It's equivalent to "join", but uses a Final instead of a loose string.
synapse/rest/admin/users.py
Outdated
| reason = body.get("reason") | ||
| limit = body.get("limit") |
There was a problem hiding this comment.
nit: you can use parse_integer_from_args and parse_string_from_args to ensure the type of these arguments. Otherwise we may start generating events with a non-string reason field.
It's also a good idea to verify that limit is not 0 or negative.
There was a problem hiding this comment.
I'm slightly confused as this function seems to parse an integer from a request string, and I am trying to get the values from the request's json object - am I missing something here?
There was a problem hiding this comment.
Oop, you have every right to be confused. Those functions indeed won't be helpful. Could you manually add the mentioned checks instead?
|
This just needs a couple very minor points addressed, and then it's good to go: #17506 (comment), #17506 (comment) |
|
riiight sorry for the delay - I think this is ready now. |
Adds an Admin API to redact all the events of a given user in the provided rooms. If an empty dict is provided, all the events of user in all the rooms they are a member of will be redacted.