Add the ability to flag a user as deleted in GH #1586
Conversation
When a user deletes their account in GitHub we don't receive any notification that this has happened. If another user creates an account with the same name, we will link to this new user on any crates owned by the old user, which can make it appear that the new user is an owner and lead to social engineering situations we don't want to permit. The front-end is pretty coupled to the (false) concept that a user always has a GH login, and that we can display it. Ideally I'd like to move what we currently call "login" to the "id" field, but we have at least 2 places today where we use the actual user id in the URL, so we'll have to change those before we can make that happen. For now I've just introduced a `display_name` field (which is confusingly named since it's usually the same as `login`, not `name`, but I couldn't come up with a better name). When a user deletes their account (or renames it, we have no way of knowing which one happened), we will no longer report their old github name, and will instead report `deleted_123` where 123 is *our* user ID for them. This is then used in the users route to look them up, instead of their old GH login, which may be reused by another user. Since we don't necessarily want to allow crawling of user profiles, this can only be used to access deleted accounts. Attempting to visit /users/deleted_123 for a user ID that isn't deleted will 404. Additionally, since we can't generally tell if a user deleted their account or renamed it, logging back into crates.io (which will update gh_login with the new value) automatically marks them as undeleted. This isn't something we can reasonably automate detection of, but when we do find out that it's happened, we can now manually flag a user. We *can* detect this when a new user is created with the same gh_login as an existing user, which will be handled in a followup PR. Fixes rust-lang#1585
|
Say we find out that carols10cents has deleted her github account, and another user warols10cents has registered the carols10cents github username and logged in to crates.io. How does marking the original crates.io carols10cents' account as deleted change what someone browsing crates.io sees, exactly? How does marking the original carols10cents' account as deleted prevent warols10cents from performing social engineering attacks? |
|
Your crates.io account is still associated with crates. What changes is
that when someone clicks on your avatar on one of those crates pages, we
still link to your crates.io account instead of warols10cents. On that page
we make it clear that this account used to have a GitHub account with that
name, but it's been deleted. We do not link to warols10cents' GitHub
profile.
Currently when someone clicks on the deleted carols10cents, we take them to
warols10cents, even though that account has no affiliation with the crate
in question. We also link to that GitHub profile, making it seem like they
are the same user.
…On Thu, Dec 27, 2018, 5:59 PM Carol (Nichols || Goulding) < ***@***.***> wrote:
Say we find out that carols10cents has deleted her github account, and
another user warols10cents has registered the carols10cents github username
and logged in to crates.io. How does marking the original crates.io
carols10cents' account as deleted change what someone browsing crates.io
sees, exactly? How does marking the original carols10cents' account as
deleted prevent warols10cents from performing social engineering attacks?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1586 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdWK1nSaEc7Iha2Lyb5J-Rlp-x9lJVCks5u9W0BgaJpZM4Zi2Ve>
.
|
|
@carols10cents Did you have any other concerns about this PR? |
|
☔ The latest upstream changes (presumably #1699) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
I tested this manually using this branch. It works mostly as advertised, except for a small issue I commented on below.
Another nit is that the display name should also be used in the message that is shown when deleting an owner.
Apart from that, the code looks good to me.
| @@ -21,7 +21,7 @@ export default Component.extend({ | |||
| height: readOnly('width'), | |||
|
|
|||
| alt: computed('user', function() { | |||
| return `${this.get('user.name')} (${this.get('user.login')})`; | |||
| return `${this.get('user.name')} (${this.get('user.display_name')})`; | |||
There was a problem hiding this comment.
The /crates/:crate_name/owner_user does not return the display name for owners, so this will display "undefied" for all owners (except for the currently logged in one, since the /me endpoint does return the display name).
We will probably need to update the owner_user endpoint as well (which uses the EncodableOwner struct for serialization).
|
@sgrif In case it helps, I would be happy to implement the changes I proposed myself, provided that you think the proposal is reasonable – just let me know. |
|
@smarnach Please do. If you'd like to rebase this as well that'd be great. |
|
The last time I had a chance to look into this some, I believe I was able to convince myself that including the The only suggestion I have, is that maybe we should use |
|
since there has't been any movement on this PR for a while and it has plenty of conflicts by now I'll go ahead and close this. if we want to revive it we can always reopen it. |
When a user deletes their account in GitHub we don't receive any
notification that this has happened. If another user creates an account
with the same name, we will link to this new user on any crates owned
by the old user, which can make it appear that the new user is an owner
and lead to social engineering situations we don't want to permit.
The front-end is pretty coupled to the (false) concept that a user
always has a GH login, and that we can display it. Ideally I'd like to
move what we currently call "login" to the "id" field, but we have
at least 2 places today where we use the actual user id in the URL, so
we'll have to change those before we can make that happen. For now I've
just introduced a
display_namefield (which is confusingly named sinceit's usually the same as
login, notname, but I couldn't come upwith a better name).
When a user deletes their account (or renames it, we have no way of
knowing which one happened), we will no longer report their old github
name, and will instead report
deleted_123where 123 is our user IDfor them. This is then used in the users route to look them up, instead
of their old GH login, which may be reused by another user. Since we
don't necessarily want to allow crawling of user profiles, this can only
be used to access deleted accounts. Attempting to visit
/users/deleted_123 for a user ID that isn't deleted will 404.
Additionally, since we can't generally tell if a user deleted their
account or renamed it, logging back into crates.io (which will update
gh_login with the new value) automatically marks them as undeleted.
This isn't something we can reasonably automate detection of, but when
we do find out that it's happened, we can now manually flag a user. We
can detect this when a new user is created with the same gh_login as
an existing user, which will be handled in a followup PR.
Fixes #1585