Skip to content
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

MSC4194: Batch redaction of events by sender within a room (including soft failed events) #4194

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Gnuxie
Copy link
Contributor

@Gnuxie Gnuxie commented Sep 15, 2024

Rendered

special thanks to @tulir for discussing this with me and providing inspiration with meowlnir.

special thanks to @tulir for prior discussion and inspiration
from maunium/meowlnir@2d5eb93...de2aeda.
@Gnuxie Gnuxie changed the title MSC0000: User Redaction draft MSC4194: User Redaction Sep 15, 2024
@tulir tulir added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Sep 15, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Implementation requirements:

  • Server implementing the endpoint
  • Client or bot using the endpoint

proposals/4194-user-redaction.md Outdated Show resolved Hide resolved
proposals/4194-user-redaction.md Outdated Show resolved Hide resolved
@Gnuxie Gnuxie marked this pull request as ready for review September 15, 2024 15:19
proposals/4194-user-redaction.md Show resolved Hide resolved

`limit`: `integer` - The maximum number of events to redact. Default: 25.

#### Response
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a communication of which Events failed to redact? For example due to a db failure. Similar to how s-s api handles failed events where you get a list of Event ids back. Or should the server retry forever to redact the event?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it should ever fail to redact individual events 🤔

If the database explodes halfway through, that's the server's problem, it can throw an error on the entire request (but really the server should tell the database to not explode)


### Use case for self redaction

Implementers should be cautious over the use of this API for self
Copy link
Contributor

Choose a reason for hiding this comment

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

How about limiting self redaction to non dag critical Events? So messages and custom state events but not for example powerlevel Events or similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

DAG critical events are supposed to be redaction exempt already for critical fields i thought.

Comment on lines 117 to 121
{
"is_more_events": false,
"redacted_events": 5,
"soft_failed_events": 1
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like some thoughts on the names of these fields. I'd also like to know if people agree that it makes less sense to return say an array of event_ids

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like some thoughts on the names of these fields.

This is always going to be subjective but here goes:

soft_failed_eventsmight be slightly ambiguous in that it doesn't explicitly express that these events have also been redacted. Maybe the counters could be nested under a redactedkey?

"redacted": {
  "total": 5,
  "soft_failed": 1
}

I'd also like to know if people agree that it makes less sense to return say an array of event_ids

IIUC the main use case for this API is to redact all of a user's events. The counters in the response could nicely be used to display progress. It might be nice to also return the total number of events to let clients display a completion percentage. Not sure if that's easy to get though?

I can't really think of a reason why the caller would need event IDs for the redacted events.

Copy link
Contributor Author

@Gnuxie Gnuxie Sep 17, 2024

Choose a reason for hiding this comment

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

Maybe the counters could be nested under a redactedkey?

Yeah that sounds like a good idea, 8f1900a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't really think of a reason why the caller would need event IDs for the redacted events.

Yeah, i'll leave this open for now just in case someone else has thoughts

Comment on lines +154 to +170
### Redacting "future" soft-failed events

Given that a request to the `/rooms/redact/user` endpoint is very
likely to occur after a room ban, then it makes sense that there could
still be soft failed events outside the scope of the request. The
moderator's homeserver is likely to discover soft-failed events from
the target user after the moderator's request has completed.

It could make sense to add a flag to the endpoint to tell the
moderator's homeserver to issue redactions on their behalf for the
newly discovered events. However, this could be complicated for
servers to implement. The flag would have to reset if the target user
is unbanned, and all incoming soft failed events will have to be
checked against a list of flagged servers.

At the moment we will remain forward compatible with a future
proposal to add a flag.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Server implementers: Am I right in making a judgement call that this would be too complicated for the proposal?

Copy link
Member

Choose a reason for hiding this comment

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

Meowlnir just has a hacky feature where if the last redacted event is <5 minutes old, it'll sleep for a bit and refetch events to redact. The assumption is that the most common soft fail issue is where events race with the ban, which should be covered by waiting a while for the ban to propagate.

A slightly less hacky method might be some way to opt into receiving soft-failed events, but that's probably a different MSC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Draupnir could in theory just call the endpoint again after a few minutes. I'd still like to just be able to give a flag to this endpoint, even if servers will only honor it for the next 10minutes or something.

proposals/4194-user-redaction.md Outdated Show resolved Hide resolved
proposals/4194-user-redaction.md Outdated Show resolved Hide resolved
@Gnuxie Gnuxie changed the title MSC4194: User Redaction MSC4194: Batch redaction of events by sender within a room (including soft failed events) Sep 18, 2024
@tulir
Copy link
Member

tulir commented Sep 19, 2024

@anoadragon453 informed me that Synapse coincidentally just merged an admin API with basically the exact same goal as this MSC: element-hq/synapse#17506

@Gnuxie
Copy link
Contributor Author

Gnuxie commented Sep 19, 2024

@tulir it's not the exact same goal as this MSC, this MSC is explicitly supposed to be used by room moderators who may not be synapse or server admins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants