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

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

@dbkr

dbkr approved these changes Jul 16, 2019

Copy link
Member

left a comment

otherwise this lgtm

@anoadragon453

This comment has been minimized.

Copy link
Member Author

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 :)

@turt2live
Copy link
Member

left a comment

lgtm

@Half-Shot
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Jul 17, 2019

Contributor

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?).

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jul 17, 2019

Author Member

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?

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jul 18, 2019

Author Member

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/

This comment has been minimized.

Copy link
@turt2live

turt2live Jul 18, 2019

Member

... 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

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

I think this is trending towards the proposed solution, so

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

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:

  • Users who log in with 3PIDs won't be told they're deactivated. resolved by #2181 (comment)

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.

Update proposals/2181-user-deactivated-errcode.md
Co-Authored-By: Hubert Chathi <hubert@uhoreg.ca>
@anoadragon453

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

@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 Jul 22, 2019

@turt2live
Copy link
Member

left a comment

largely I'm still okay with this but have enough comments here to warrant "Request Changes" imo.

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

dbkr approved these changes Jul 23, 2019

@KitsuneRal
Copy link
Member

left a comment

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.

anoadragon453 added some commits Jul 26, 2019

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2019

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

@mscbot

This comment has been minimized.

Copy link
Collaborator

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

8 checks passed

buildkite/matrix-doc Build #549 passed (1 minute, 51 seconds)
Details
ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

@turt2live turt2live self-assigned this Jul 31, 2019

@turt2live

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

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

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

Spec PR has been created: #2234

anoadragon453 added a commit that referenced this pull request Aug 15, 2019

Add M_USER_DEACTIVATED to list of error codes (#2234)
Spec PR for [MSC 2181](#2181).

Adds the `M_USER_DEACTIVATED` error code and a short description to the client-server API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.