ruma / homeserver

A Matrix homeserver written in Rust.
https://www.ruma.io/
1.08k stars 41 forks source link

Add basic implementation for the /account/deactivate endpoint. #89

Closed mujx closed 8 years ago

mujx commented 8 years ago

I wrote a basic implementation for the deactivate endpoint, but I am not sure if deleting the user from the database is the best approach.

Maybe we can use a flag?

jimmycuadra commented 8 years ago

Thanks for the PR! The spec doesn't say anything about what deactivating means other than "removing all ability for the user to login again." If the user record were to be deleted from the database, that might be a problem since there will still be events in the graph for any rooms they joined that reference them. Given the limited scope of being able to log in that the spec describes, I think you're right that we should just have an "active" field for users that is true initially and stays true until the deactivate. Then we'd need to check the value of this field before issuing any new access tokens for the user.

erikjohnston commented 8 years ago

(The spec may also want to mention that the API should probably also delete any personal information associated with the account, e.g. email addresses etc, fwiw)

On Sat, 17 Sep 2016 19:43 Jimmy Cuadra, notifications@github.com wrote:

Thanks for the PR! The spec doesn't say anything about what deactivating means other than "removing all ability for the user to login again." If the user record were to be deleted from the database, that might be a problem since there will still be events in the graph for any rooms they joined that reference them. Given the limited scope of being able to log in that the spec describes, I think you're right that we should just have an "active" field for users that is true initially and stays true until the deactivate. Then we'd need to check the value of this field before issuing any new access tokens for the user.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ruma/ruma/pull/89#issuecomment-247796759, or mute the thread https://github.com/notifications/unsubscribe-auth/AICaWMbBznQFL8quqffDjOWV1EbCg_RKks5qrDTJgaJpZM4J_eL0 .

jplatte commented 8 years ago

Shouldn't the endpoint be called AccountDeactivate though, to stay consistent with the existing naming scheme?

mujx commented 8 years ago

I updated the PR using the active flag. What would be the process of reactivating the account? I couldn't find anything relevant in the spec.

Maybe the POST /_matrix/client/r0/account/password/email/requestToken endpoint?

@jplatte I think it would be a little awkward to read. Also the other endpoints have the verb first, which is more natural. The AccountPassword could be renamed to ChangeAccountPassword.

jplatte commented 8 years ago

The other endpoints don't have a descriptive verb first, they have the HTTP method first (in case the URL supports multiple HTTP methods). I agree that if the endpoints should be reflective of what they do instead of their http method / url, changing AccountPassword to ChangeAccountPassword or similar would make sense. But I guess the naming scheme is up to @jimmycuadra to decide.

jimmycuadra commented 8 years ago

My guess is that once the account is deactivated it is gone for good. This seems supported by Erik's comment that personal information should be removed from the user when deactivated.

Everyone is right that the struct names for the API endpoints are a bit awkward right now. You can leave them as is here and I'll change them all at once afterwards.

mujx commented 8 years ago

I think that was the info @erikjohnston was referring to. The account data per room has similar implementation, and it will be added soon. #62

Let me know if you have any improvements/changes in mind.

jimmycuadra commented 8 years ago

Looks great! Thanks so much for your work on this.

mujx commented 8 years ago

Thanks for merging!