Scrub personal user info when disabling a user #46
Conversation
| if user.disabled? | ||
| raise ScrubDisabledUserError.new("Can't scrub personal details of a disabled user with id: #{user.id}") | ||
| else | ||
| hashed_login_string = Zlib.crc32(user.login).to_s |
There was a problem hiding this comment.
Any particular reason you choose this? Mostly just curious.
There was a problem hiding this comment.
It was in the standard ruby libs and seemed to be the same across platforms. I may be wrong. Any thoughts to change it?
There was a problem hiding this comment.
I'm not super sure. I was reading its wikipedia page, and it seems like its probably pretty easy to reverse. I was just wondering if it made sense to hash the user login for deleted users with something cryptographically stronger like bcrypt?
There was a problem hiding this comment.
So from the meeting in Oxford and #39 we would reserve the right to reverse it should we desire to, e.g. user says my account was hacked and everything is deleted. Not saying that we'd do this but the idea was to remove the personal information (email), change the display name and then hash the login name so we might be able to re-enable the account.
I thought that if the login is unique then the hashed login would not fail the unique login index criteria. Perhaps I should add a retry block with a suffix appender?
There was a problem hiding this comment.
No I think it's fine if we don't consider the user's login to be personal information. Since crc32 isn't a cryptographic hash it doesn't requite a lot of computational power to go from crc32 hash -> plaintext login (it seems even less than md5 or something like that). If we're fine with people who have access to our database having that ability, I'm fine with that. Although it does raise the question about whether we should even hash deleted logins in the first place?
I found this SO Question about CRC collisions. It looks like CRC32 may not be good enough if we have any number of deleted users.
There was a problem hiding this comment.
Good points.
Re security, I guess it's just a layer of obfuscation, perhaps we don't need it or we add a crypto hash with a secret key and never lose it.
Re collisions, thoughts on replacement hash functions?
There was a problem hiding this comment.
So it looks like unless we have a way to salt the hash there isn't a reasonable way to store it that won't be vulnerable to a pretty trivial rainbow table attack, and if we salt the hash, there isn't a good way for us to look up a user that wants to re-enable their account.
I also think it may be a bad idea to allow a user's account to be deactivated, and then allow someone to signup with their old login, especially if we tie the login to a users's URL name.
To me it makes the most sense to just leave the login in plaintext, and to offer some sort of "Really delete my entire account" option.
Just leave the login as is since it's not personal information thus avoiding probelms with duplicate login names on revert and uniq index clashes when hashing.
|
Merged this but again github didn't close the pull request. I think it might because because I rebased from origin/master after I merged your branch in. |
No description provided.