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

[WIP] MSC2438: Local and Federated User Erasure Requests #2438

Draft
wants to merge 4 commits into
base: old_master
Choose a base branch
from

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Feb 19, 2020

@anoadragon453 anoadragon453 added the proposal A matrix spec change proposal label Feb 19, 2020
@turt2live turt2live self-requested a review February 19, 2020 17:13
@anoadragon453 anoadragon453 changed the title MSCXXXX: Local and Federated User Erasure Requests MSC2438: Local and Federated User Erasure Requests Feb 19, 2020
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Various thoughts and feelings about appservices (as you'd expect 😛)


At this point, the application service SHOULD try to erase as much
identifying information about this user as possible. Upon successfully
acknowledging the request, the application service should return a `200 OK`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the expected response if the application service fails to remove the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently this MSC is designed to just fan out the request, without returning any success/failure back to the user on whether the data was actually deleted. Thus here the 200 OK is just so that the AS acknowledged the request.

At this point, the application service SHOULD try to erase as much
identifying information about this user as possible. Upon successfully
acknowledging the request, the application service should return a `200 OK`
with an empty JSON body.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the expected response if the application service hasn't ever seen the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

A 200 OK, as it'd be the same result as the AS deleting all data of a known user.

proposals/2438-local-and-federated-erasure-requests.md Outdated Show resolved Hide resolved
other homeserver should respond with a `403 M_FORBIDDEN`.

For application services, a new API endpoint will be added on the application
service: `POST /_matrix/app/v1/users/erase`. It contains a single, required
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooi, why a blob of json and not /_matrix/app/v1/users/erase/@someone:example.com? It makes it marginally easier to trace in logging

Copy link
Member

Choose a reason for hiding this comment

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

Because the point is to remove personal data, so forcing more personal data into logs is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm inclined to agree with @turt2live here, but I also agree that it'd be good to have a record of which users were erased... but how you'd do that without keeping their User ID around in a database I'm not sure...

Therefore logs, which can be deleted according to a retention policy, may indeed be the best option?

}
```

At this point, the application service SHOULD try to erase as much
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting that this proposal send a 200 on acknowledging but not if it's successful, not successful or even possible. It would be nice if the appservice could feedback in these different cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment we don't plan to send feedback to the user about which application services out there couldn't process the request.

And I'm not sure what the homeserver would do in this case. It could keep retrying, but if the application service couldn't delete it the first time, why would a second request make any difference?


```
{
"id_server_unbind_result": "success",
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda sucks that we have feedback for identity servers, but not for other services. It would be helpful if the API supported some granularity how successful erase has been.

Copy link
Member

Choose a reason for hiding this comment

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

This field is a hack anyways. Granular responses is probably something for a different MSC, unless this MSC can somehow make it easy and not a nightmare to deal with. There's so much more UX to consider with granularity when the user probably only cares about it working or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, this MSC is only concerned with getting the request out there, not getting feedback to the user. I'm still inclined to remove this field and just have the absence of an HTTP error mean success.

* Other homeservers
* Application services
* Identity Servers
* Integration managers
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting that the data contract for integration managers is between itself and the client, the homeserver isn't aware of the integration manager at all.

Copy link
Member

Choose a reason for hiding this comment

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

False, the homeserver is aware of integration managers. The homeserver is also best positioned to deal with the data removal.

I'd still take integration managers out of scope of this MSC though because they don't exist at the spec level.

@aaronraimist
Copy link
Contributor

So is this erasing everything (to the point of allowing usernames to be reused)?

@anoadragon453
Copy link
Member Author

So is this erasing everything (to the point of allowing usernames to be reused)?

This is simply ferrying erasure requests to different services, the spec deliberately leaves what is actually erased up to the implementation, as we can't actually enforce that at the protocol level.

@anoadragon453
Copy link
Member Author

@Half-Shot What do you think about this proposal?

@Half-Shot
Copy link
Contributor

oooooooh yes

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

looks like this is still in progress, but some clarifying comments.

proposals/2438-local-and-federated-erasure-requests.md Outdated Show resolved Hide resolved
proposals/2438-local-and-federated-erasure-requests.md Outdated Show resolved Hide resolved
Comment on lines +61 to +62
* Whether we should just fail the request entirely if local user erase was
unsuccessful
Copy link
Member

Choose a reason for hiding this comment

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

this

Copy link
Member Author

Choose a reason for hiding this comment

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

We may need to introduce a custom error code for this then?

Copy link
Member

Choose a reason for hiding this comment

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

yup

Upon receiving this request, the homeserver should forward it to every
homeserver it believes could also contain that user's data. How it does so is
left as an implementation detail. Once it's decided, the request will be
communicated over a new Federation API, `/_matrix/federation/v1/user/erase`.
Copy link
Member

Choose a reason for hiding this comment

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

We're on v2 now:

Suggested change
communicated over a new Federation API, `/_matrix/federation/v1/user/erase`.
communicated over a new Federation API, `/_matrix/federation/v2/user/erase`.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was my understanding that we'd start new APIs with v1, and only introduce a v2 for them if they need to be updated with breaking changes. Is this not right?

Copy link
Member

Choose a reason for hiding this comment

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

We don't have any formal standard for this, but using the identity server as an example we added /v2 endpoints without /v1 counterparts, implying that we ratchet the whole version and not on a per-endpoint basis.

Copy link
Contributor

@babolivier babolivier Mar 27, 2020

Choose a reason for hiding this comment

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

There's also a difference here which is that in the IS spec every existing endpoint got a v2 (even those that haven't changed in the transition), which imho means vX is the version of the API, whereas in the S2S spec most existing endpoints are only v1 and we've only been bumping per endpoint when we need to change it, which imho means vX is the version of the endpoint.

Argh all this is confusing.

Copy link
Member

Choose a reason for hiding this comment

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

I've opened #2475 to settle this.

Copy link
Member

Choose a reason for hiding this comment

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

vX is the version of the API,

no. The API doesn't have a single version. It just sounds like we happened to bump all the IS APIs at once.

Example request:

```
POST /_matrix/federation/v1/user/erase
Copy link
Member

Choose a reason for hiding this comment

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

We're on v2 now:

Suggested change
POST /_matrix/federation/v1/user/erase
POST /_matrix/federation/v2/user/erase

@turt2live turt2live changed the title MSC2438: Local and Federated User Erasure Requests [WIP] MSC2438: Local and Federated User Erasure Requests Mar 5, 2020
@richvdh richvdh mentioned this pull request Mar 27, 2020
@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 20, 2020
@turt2live turt2live marked this pull request as draft April 8, 2021 23:36
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@turt2live turt2live self-assigned this Jun 1, 2023
Comment on lines +22 to +27
A new parameter to the
[`/account/deactivate`](https://matrix.org/docs/spec/client_server/r0.6.0#post-matrix-client-r0-account-deactivate)
Client-Server API endpoint will be added, called `erase`, which is a boolean
that specifies whether the homeserver MUST attempt to erase all personal
data pertaining to the user off of the homeserver and as much of the rest of
the federation as it can.
Copy link
Member

Choose a reason for hiding this comment

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

We ended up implementing erase long ago, but without the federation propagation bit. #4025 covers the history here (and may affect how this proposal operates).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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