-
-
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 10 commits
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 |
---|---|---|
|
@@ -5,3 +5,5 @@ | |
|
||
PLG_REMEMBER_XML_DESCRIPTION="Provides remember me functionality. The cookie authentication plugin must be enabled for this plugin to function." | ||
PLG_SYSTEM_REMEMBER="System - Remember Me" | ||
PLG_REMEMBER_RESET_DESC="Should the 'remember-me' functionality be reset (recommended), when a user changes his password? This will render any existing login cookies on other devices useless." | ||
PLG_REMEMBER_RESET_LABEL="reset on password change" | ||
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. Capitalize each word. 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. not sure about this, I suggest: Reset on password change |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,7 @@ COM_USERS_PROFILE_PASSWORD2_DESC="Confirm your password." | |
COM_USERS_PROFILE_PASSWORD2_LABEL="Confirm Password" | ||
COM_USERS_PROFILE_REGISTERED_DATE_LABEL="Registered Date" | ||
COM_USERS_PROFILE_SAVE_FAILED="Profile could not be saved: %s" | ||
COM_USERS_PROFILE_SAVE_REMEMBERME_USERINFO="Your password change was successful. For your security, we also deactivated any existing 'remember-me' logins on other devices." | ||
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. I don't think we actually need to explicitly say "your remember me cookies are invalidated", I can't say I see this come up on other platforms where I have a remember me login in place, the device just knows the next time I use it to access a service I need to reauthenticate (well, the server tells it when it tries to automatically log in, but that's all transparent to the 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. This might seem 'over-the-top', but it was intended to 'reassure the user, everything is settled'. We could ignore this message OR (which I would prefer) let the website administrator decide whether to show this message or not (another configuration option in the plugin). 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. I'm also in favour of less clutter... 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. I would also not show the message. |
||
COM_USERS_PROFILE_SAVE_SUCCESS="Profile saved." | ||
COM_USERS_PROFILE_TWO_FACTOR_AUTH="Two Factor Authentication" | ||
COM_USERS_PROFILE_TWOFACTOR_LABEL="Authentication Method" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,4 +94,73 @@ 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 ) | ||
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. 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 commentThe 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 commentThe 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. 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. 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). |
||
* | ||
* @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 __DEPLOY_VERSION__ | ||
* @throws InvalidArgumentException on invalid date. | ||
*/ | ||
public function onUserBeforeSave($user, $isnew, $data) | ||
{ | ||
// Irrelevant on new users | ||
if ($isnew) | ||
{ | ||
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. Remove tabs. |
||
// Irrelevant, because password was not changed by 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. new line after |
||
if ($data['password_clear'] == '') | ||
{ | ||
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. Remove tabs. |
||
// Irrelevant, because "resetting on pw change" is not activated | ||
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 |
||
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(); | ||
} | ||
|
||
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 blank line. 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 |
||
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' | ||
); | ||
} | ||
|
||
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 tabs. |
||
$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.
Sort keys in alpha order.
Remove comma before
when
.Remove
his
to be gender neutral.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.
Use this string please:
"When enabled, a user's login cookies will be invalidated when their password is reset."
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.
done, as per mbabker suggestion