-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Delete existing user_keys, if password is changed #17827
Changes from 1 commit
9c09cc1
4acd6f3
f895ceb
eb2037d
77742f8
9d53a10
e28a43c
0ef52d9
7b030da
1b6d692
ead6a5b
7d47def
5f6e00b
6ec3c84
4d32707
9d37fd3
ef4d4bb
0d8e407
3215578
10c9fab
33478e1
b1d9d16
4b56855
b9d9b3e
a694d49
b430083
7b0ce39
9804de4
8acd325
f226416
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,4 +94,61 @@ public function onUserLogout($user, $options) | |
|
||
return true; | ||
} | ||
|
||
/** | ||
* Method is called before user data is stored in the database | ||
* If activated in the configuration of the rememeber-me plugin, this method resets all #__user_keys for the current user when the user changes his/her password | ||
* leaving any existing remember-me cookies on any devices useless! | ||
* This functionality was sadly inspired by the horrific events around Alice Ruggles death (see https://www.alicerugglestrust.org ) | ||
* | ||
* @param array $user Holds the old user data. | ||
* @param boolean $isnew True if a new user is stored. | ||
* @param array $data Holds the new user data. | ||
* | ||
* @return boolean | ||
* | ||
* @since 3.1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3.1 has been released few years ago :) |
||
* @throws InvalidArgumentException on invalid date. | ||
*/ | ||
public function onUserBeforeSave($user, $isnew, $data) | ||
{ | ||
// irelevant on new users | ||
if ($isnew) {return true;} | ||
// irelevant, because password was not changed by user | ||
if ($data['password_clear'] == '') {return true;} | ||
// irelevant, because "resetting on pw change" is not activated | ||
if (!$this->params->get('resetRememberMe')) {return true;} | ||
|
||
// Get the application if not done by JPlugin. This may happen during upgrades from Joomla 2.5. | ||
if (!$this->app) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't needed in that event to my knowledge as that event will not be fired during an upgrade. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
{ | ||
$this->app = JFactory::getApplication(); | ||
} | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new line after |
||
* But now, we need to do something | ||
* Delete all tokens for this user! | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Align |
||
$db = JFactory::getDbo(); | ||
$query = $db->getQuery(true) | ||
->delete('#__user_keys') | ||
->where($db->quoteName('user_id') . ' = ' . $db->quote($user['username'])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm how can this work? Isn't the user_id just the ID and not the username? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reference, I logged in as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah got confused by that naming. |
||
try | ||
{ | ||
$db->setQuery($query)->execute(); | ||
} | ||
catch (RuntimeException $e) | ||
{ | ||
// Log an alert for the site admin | ||
JLog::add( | ||
sprintf('Failed to delete cookie token for user %s with the following error: %s', $results[0]->user_id, $e->getMessage()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no |
||
JLog::WARNING, | ||
'security' | ||
); | ||
} | ||
// Destroy the cookie in the browser. | ||
//$this->app->input->cookie->set($cookieName, '', 1, $this->app->get('cookie_path', '/'), $this->app->get('cookie_domain', '')); | ||
$this->app->enqueueMessage(JText::_('COM_USERS_PROFILE_SAVE_REMEMBERME_USERINFO'), 'notice'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new line after |
||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a blank line before line 163. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove tab. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this reference to Alice Ruggles and especially also the URL as it serves no purpose within code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the URL, but I would rather like to keep the reference, as this is the underlying reason why this functionality is developed in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I never heard of her and even after reading that she got killed after being stalked I don't get the reference. So it seems quite useless to me.
Also, on the other hand it's quite obvious why we want to invalidate existing remember-me credentials. Personally I would just add a one-line comment like
Invalidate all existing remember-me cookies after a password change
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nobody expects that any given person "has heard of" each and every stalking victim, this is not the point. However, one major element in this case was that the murderer actually hacked into her online accounts to follow her movements, using exactly this "remember-me" functionality (not on Joomla though).