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

MSC2181: Add an Error Code for Signaling a Deactivated User #2181

Merged
merged 12 commits into from
Jul 31, 2019

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jul 16, 2019

@anoadragon453 anoadragon453 changed the title Add proposal regarding user deactivated err codes MSC2181: Add an Error Code for Signaling a Deactivated User Jul 16, 2019
@anoadragon453 anoadragon453 added proposal A matrix spec change proposal proposal-in-review labels Jul 16, 2019
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

otherwise this lgtm

@anoadragon453
Copy link
Member Author

anoadragon453 commented Jul 16, 2019

otherwise this lgtm

Did you mean to leave a feedback as well?

Edit: Found his comment in my email. It was:

Probably better would be if the server only returned this error if the username and password was correct, which synapse would be able to do if it didn't implement account deactivation by nulling out the password.

Which has already been discussed below :)

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.

lgtm

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.

Looks like a sane thing to have, but there are a few bits that don't sit right with me.

proposals/2181-user-deactivated-errcode.md Outdated Show resolved Hide resolved

Currently, when a user attempts to log in, they will receive an `M_FORBIDDEN`
errcode if their password is incorrect. However, if the user's account is
deactivated, they will also receive an `M_FORBIDDEN`, leaving clients in a
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What do homeservers do if the user is deactivated and somebody tries to log in with an incorrect password? Is the homeserver expected to retain the password forever? If the password is not retained, should all attempts to login as a deactivated user return the deactivated error (which may have some privacy implications?).

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 should still return M_USER_DEACTIVATED. Password hashes are wiped (at least in Synapse) upon user deactivation.

which may have some privacy implications?

Privacy implications are here whether password hashes are retained or not, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with shifting it so that you need to login to see if you're deactivated, is that we already have tons of deactivated users whose password hashes have been cleared.

Also worth noting reddit's APIs allow you to tell if any user has been shadowbanned, something that ideally even the user wouldn't know, and that doesn't seem to have caused their service any harm. https://nullprogram.com/am-i-shadowbanned/

Copy link
Member

Choose a reason for hiding this comment

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

... also by nature of being deactivated you shouldn't be allowed back in. Why would we let people get that far into the process without telling them to go away? I think the proposed approach is fine

proposals/2181-user-deactivated-errcode.md Show resolved Hide resolved
@anoadragon453
Copy link
Member Author

I think this is trending towards the proposed solution, so

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Jul 18, 2019

Team member @anoadragon453 has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jul 18, 2019
proposals/2181-user-deactivated-errcode.md Outdated Show resolved Hide resolved
proposals/2181-user-deactivated-errcode.md Outdated Show resolved Hide resolved
Co-Authored-By: Hubert Chathi <hubert@uhoreg.ca>
@anoadragon453
Copy link
Member Author

anoadragon453 commented Jul 19, 2019

@mscbot concern Users who log in with 3PIDs won't be told they're deactivated.

3PIDs are deleted upon deactivation (https://github.com/matrix-org/synapse/blob/32e7c9e7f20b57dd081023ac42d6931a8da9b3a3/synapse/handlers/deactivate_account.py#L92) so how do we tell those users they were deactivated?

@anoadragon453
Copy link
Member Author

@mscbot resolve Users who log in with 3PIDs won't be told they're deactivated.

Implementations can hash user's 3PIDs instead of removing them, thus allowing them to continue to check if a user is deactivated without keeping personal information about.

proposals/2181-user-deactivated-errcode.md Outdated Show resolved Hide resolved
@richvdh richvdh requested review from richvdh and removed request for richvdh July 22, 2019 11:18
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Assuming that it just defines a new error code, I'm fine with the proposal. I'm not sure why we don't at least recommend servers to use this error code, though.

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Jul 26, 2019
@mscbot
Copy link
Collaborator

mscbot commented Jul 26, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Jul 31, 2019
@mscbot
Copy link
Collaborator

mscbot commented Jul 31, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@anoadragon453 anoadragon453 merged commit f989263 into master Jul 31, 2019
@turt2live turt2live self-assigned this Jul 31, 2019
@turt2live
Copy link
Member

This has an implementation so bumping it to spec-pr-missing: matrix-org/synapse#5686

@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Jul 31, 2019
@anoadragon453 anoadragon453 deleted the anoa/user_deactivated_msc branch July 31, 2019 14:52
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Aug 15, 2019
@anoadragon453
Copy link
Member Author

Spec PR has been created: #2234

anoadragon453 added a commit that referenced this pull request Aug 15, 2019
Spec PR for [MSC 2181](#2181).

Adds the `M_USER_DEACTIVATED` error code and a short description to the client-server API.
@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Aug 26, 2019
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants