Use extra salt stored in database for userid anonymization #12844
Conversation
| if (is_null($salt)) { | ||
| $salt = Option::get(self::OPTION_USERID_SALT); | ||
| if (empty($salt)) { | ||
| $salt = Common::generateUniqId(); |
There was a problem hiding this comment.
You could generate a stronger salt like Common::getRandomString($len = 40, $alphabet = "abcdefghijklmnoprstuvwxyzABCDEFGHIJKLMNOPRSTUVWXYZ0123456789_-$");
Also you may want to write an update method for this as it be better to avoid DB connections during tracker. You would also need to add a test to see if it actually works during tracker mode... because it might not actually work when operating in tracker mode. I had this a few times as some methods like fetchOne may not be available during tracker mode.
| if (!empty($trackerCache[PrivacyManager::OPTION_USERID_SALT])) { | ||
| $salt = $trackerCache[PrivacyManager::OPTION_USERID_SALT]; | ||
| } else { | ||
| $salt = PrivacyManager::getUserIdSalt(); |
There was a problem hiding this comment.
Ideally the else was avoided and in case it is not present, simply the raw userId removed and we could install maybe the salt with an update, and otherwise it gets installed through the tracker cache creation.
| public static function getUserIdSalt() | ||
| { | ||
| static $salt = null; | ||
| if (is_null($salt)) { |
There was a problem hiding this comment.
Is this cache really needed? The Option class would cache it anyway? Such a static property makes it bit complicated for tests.
There was a problem hiding this comment.
correct. it's kind of useless in this case
| $salt = Option::get(self::OPTION_USERID_SALT); | ||
| if (empty($salt)) { | ||
| $salt = Common::generateUniqId(); | ||
| Option::set(self::OPTION_USERID_SALT, $salt, 1); |
There was a problem hiding this comment.
I presume to make the tests work, you may also need to add a possibility to inject the salt through DI
| if (!empty($trackerCache[PrivacyManager::OPTION_USERID_SALT])) { | ||
| $salt = $trackerCache[PrivacyManager::OPTION_USERID_SALT]; | ||
| } | ||
| return sha1($userId . $salt); |
There was a problem hiding this comment.
If no salt is set, I think it should return the raw $userId. Otherwise you eventually create two different userIds for the same userId. It should be only for a short amount of time though I presume so may be fine to not use return raw $userId. However, it makes it also easier to discover when there is a bug as currently we would not notice if $trackerCache[PrivacyManager::OPTION_USERID_SALT]) is never set.
…g#12844) * Use salt stored in database for userid anonymization * use update script + small review changes * fix tests * don't hash if salt is empty
No description provided.