Skip to content
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

Merged
merged 30 commits into from
Feb 7, 2018
Merged
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9c09cc1
Delete existing user_keys, if password is changed
schultz-it-solutions Sep 1, 2017
4acd6f3
corrected styling issues
schultz-it-solutions Sep 1, 2017
f895ceb
deploy version - as I said, this is my first pr
schultz-it-solutions Sep 1, 2017
eb2037d
pushing to patch-2
schultz-it-solutions Sep 1, 2017
77742f8
newline after }
schultz-it-solutions Sep 1, 2017
9d53a10
push to patch-2
schultz-it-solutions Sep 1, 2017
e28a43c
push to patch-2
schultz-it-solutions Sep 1, 2017
0ef52d9
Update en-GB.com_users.ini
schultz-it-solutions Sep 1, 2017
7b030da
Update remember.php
schultz-it-solutions Sep 1, 2017
1b6d692
Update remember.xml
schultz-it-solutions Sep 1, 2017
ead6a5b
configuration option in XML file
schultz-it-solutions Sep 1, 2017
7d47def
Update en-GB.plg_system_remember.ini
schultz-it-solutions Sep 1, 2017
5f6e00b
hm...
schultz-it-solutions Sep 1, 2017
6ec3c84
Update remember.php
schultz-it-solutions Sep 1, 2017
4d32707
Update remember.php
schultz-it-solutions Sep 1, 2017
9d37fd3
XML styles
schultz-it-solutions Sep 1, 2017
ef4d4bb
commenting out the user message
schultz-it-solutions Sep 1, 2017
0d8e407
Update remember.php
schultz-it-solutions Sep 2, 2017
3215578
Update en-GB.plg_system_remember.ini
schultz-it-solutions Sep 2, 2017
10c9fab
btn-group-yesno
schultz-it-solutions Sep 2, 2017
33478e1
Update remember.php
schultz-it-solutions Sep 3, 2017
b1d9d16
Update remember.php
schultz-it-solutions Sep 3, 2017
4b56855
reference to Alice Ruggles removed!
schultz-it-solutions Sep 5, 2017
b9d9b3e
making it mandatory
schultz-it-solutions Sep 6, 2017
a694d49
Update remember.php
schultz-it-solutions Sep 6, 2017
b430083
making it mandatory
schultz-it-solutions Sep 6, 2017
7b0ce39
making it mandatory
schultz-it-solutions Sep 6, 2017
9804de4
making it mandatory
schultz-it-solutions Sep 6, 2017
8acd325
as per the remarks of Quy
schultz-it-solutions Dec 19, 2017
f226416
changed as per Quy's remarks
schultz-it-solutions Dec 21, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions plugins/system/remember/remember.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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).

*
* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.1 has been released few years ago :)
Please change this to __DEPLOY_VERSION__

* @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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{
$this->app = JFactory::getApplication();
}
/*
Copy link
Contributor

Choose a reason for hiding this comment

The 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!
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align * with the other *.

$db = JFactory::getDbo();
$query = $db->getQuery(true)
->delete('#__user_keys')
->where($db->quoteName('user_id') . ' = ' . $db->quote($user['username']));
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user_id is the username. The column name is misleading.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, I logged in as admin and the user_id is admin in the database.

Copy link
Contributor

Choose a reason for hiding this comment

The 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()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no $results. Use $user['username'].

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line after }

return true;
Copy link
Contributor

@Quy Quy Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a blank line before line 163.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove tab.

}