Add an Admin API endpoint to redact all a user's events #17506
Add an Admin API endpoint to redact all a user's events #17506anoadragon453 merged 15 commits intoelement-hq:developfrom
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 :) |
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
This is looking pretty good so far, thanks for continuing to make sysadmin's lives easier!
|
sidenote: MSC4084: Mass redactions would make this a lot more efficient. |
|
complement failures are flakey partial-state join tests - unrelated |
anoadragon453
left a comment
anoadragon453
left a comment
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. |
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Sorry, it looks like I got halfway through a review here and then got distracted :|
This PR is close! Just a few things now.
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Just a couple things now, but overall this is looking nearly ready to go!
| content = event.content | ||
| 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.
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Small things now, lgtm otherwise!
| content = event.content | ||
| 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.
| 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. |
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Right, this LGTM! Thanks for your patience, this is a really welcome change :)
4 participants
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.