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.

Sign up for GitHub

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

send notification when passwords are about to expire #27

Merged
merged 3 commits into from
Jul 12, 2018
Merged

send notification when passwords are about to expire #27

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35ccd8b
send notification when passwords are about to expire
jvillafanez Jul 6, 2018
fe7cb37
Hardening and test adjustments
jvillafanez Jul 12, 2018
5157dde
Don't return value in generator
Jul 12, 2018
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
13 changes: 13 additions & 0 deletions appinfo/app.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,16 @@
\OCP\IUser::class . '::firstLogin',
[$handler, 'checkForcePasswordChangeOnFirstLogin']
);

$app = new \OCA\PasswordPolicy\AppInfo\Application();
$app->registerNotifier();

// only load notification JS code in the logged in layout page (not public links not login page)
$request = \OC::$server->getRequest();
if (\OC::$server->getUserSession() !== null && \OC::$server->getUserSession()->getUser() !== null
&& substr($request->getScriptName(), 0 - strlen('/index.php')) === '/index.php'
&& substr($request->getPathInfo(), 0, strlen('/s/')) !== '/s/'
&& substr($request->getPathInfo(), 0, strlen('/login')) !== '/login') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the reason for this if condition obvious? I could use a comment, or some way to understand what is the script+path pattern aimed for and why.

Copy link
Contributor

Choose a reason for hiding this comment

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

copy-pasted from notifications app: https://github.com/owncloud/notifications/blob/master/appinfo/app.php#L29

I'll add a comment


\OCP\Util::addScript('password_policy', 'notification');
}
7 changes: 5 additions & 2 deletions appinfo/info.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ The definition of certain password rules support administrators in the task of e
Password history and expiration policies are supplements that allow IT to establish a level of password security that can comply with corporate guidelines of all sorts. The provided tools enable administrators to granularly choose their desired security level. At this point it is important to keep in mind that high levels of security might sacrifice usability and come at the expense of user experience. For this reason it is highly recommended to check [best practices](https://pages.nist.gov/800-63-3/sp800-63b.html) and decide carefully on the hurdles that are put upon users in order to maintain and optimize user adoption and satisfaction.

Administrators find the configuration options in the 'Security' section of the ownCloud administration settings panel. The respective policies are designed for local user accounts, not for user accounts imported from LDAP or other user backends as these provide their own mechanisms. For more information and recommendations when deploying policies in an existing ownCloud, please consult the [ownCloud Documentation](https://doc.owncloud.com/server/latest/admin_manual/configuration/server/security/password-policy.html).</description>
<version>2.0.0</version>
<version>2.0.1</version>
<documentation>
<admin>https://doc.owncloud.com/server/10.0/admin_manual/configuration/server/security/password_policy.html</admin>
</documentation>
<dependencies>
<owncloud min-version="10.0.5" max-version="10.1" />
<owncloud min-version="10.0.9" max-version="10.1" />
</dependencies>
<namespace>PasswordPolicy</namespace>
<settings>
Expand All @@ -30,6 +30,9 @@ Administrators find the configuration options in the 'Security' section of the o
<account-modules>
<module>OCA\PasswordPolicy\Authentication\AccountModule</module>
</account-modules>
<background-jobs>
<job>OCA\PasswordPolicy\Jobs\PasswordExpirationNotifierJob</job>
</background-jobs>

<screenshot>https://raw.githubusercontent.com/owncloud/screenshots/master/password_policy/owncloud-app-password_policy.jpg</screenshot>
<screenshot>https://raw.githubusercontent.com/owncloud/screenshots/master/password_policy/owncloud-app-password_policy2.jpg</screenshot>
Expand Down
25 changes: 25 additions & 0 deletions js/notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (c) 2018 Vincent Petry <[email protected]>
*
* This file is licensed under the Affero General Public License version 3
* or later.
*
* See the COPYING-README file.
*
*/
(function () {

$(document).ready(function() {
// convert action URL to redirect
$('body').on('OCA.Notification.Action', function(e) {
if (e.notification.app === 'password_policy'
&& (e.notification.object_type === 'about_to_expire' || e.notification.object_type === 'expired')
&& e.action.type === 'GET'
) {
OC.redirect(e.notification.link);
return false;
}
});
});
})();

13 changes: 13 additions & 0 deletions lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,24 @@
namespace OCA\PasswordPolicy\AppInfo;

use OCP\AppFramework\App;
use OCP\Notification\Events\RegisterNotifierEvent;
use OCA\PasswordPolicy\Notifier;

class Application extends App {

public function __construct (array $urlParams = []) {
parent::__construct('password_policy', $urlParams);
}

/**
* Registers the notifier
*/
public function registerNotifier() {
$container = $this->getContainer();
$dispatcher = $container->getServer()->getEventDispatcher();
$dispatcher->addListener(RegisterNotifierEvent::NAME, function (RegisterNotifierEvent $event) use ($container) {
$l10n = $container->getServer()->getL10N('password_policy');
$event->registerNotifier($container->query(Notifier::class), 'password_policy', $l10n->t('Password Policy'));
});
}
}
4 changes: 2 additions & 2 deletions lib/Controller/PasswordController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public function update($current_password, $new_password, $confirm_password, $red
if ($new_password !== $confirm_password) {
return $this->createPasswordTemplateResponse(
$redirect_url,
$this->l10n->t('New passwords are not the same.')
$this->l10n->t('Password confirmation does not match the password.')
);
}

Expand All @@ -150,7 +150,7 @@ public function update($current_password, $new_password, $confirm_password, $red
if(!$this->userManager->checkPassword($user->getUID(), $current_password)) {
return $this->createPasswordTemplateResponse(
$redirect_url,
$this->l10n->t('Incorrect current password supplied.')
$this->l10n->t('The current password is incorrect.')
);
}

Expand Down
44 changes: 43 additions & 1 deletion lib/Controller/SettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use OCP\IRequest;
use OCP\Settings\ISettings;
use OCP\Template;
use OCA\PasswordPolicy\UserNotificationConfigHandler;

class SettingsController extends Controller implements ISettings {

Expand All @@ -49,13 +50,26 @@ class SettingsController extends Controller implements ISettings {
'spv_password_history_value' => 3,
'spv_user_password_expiration_checked' => false,
'spv_user_password_expiration_value' => 90,
'spv_user_password_expiration_notification_checked' => false,
'spv_user_password_expiration_notification_value' => UserNotificationConfigHandler::DEFAULT_EXPIRATION_FOR_NORMAL_NOTIFICATION,
'spv_user_password_force_change_on_first_login_checked' => false,
'spv_expiration_password_checked' => false,
'spv_expiration_password_value' => 7,
'spv_expiration_nopassword_checked' => false,
'spv_expiration_nopassword_value' => 7,
];

/**
* functions to convert values between what is shown and what is stored
* these functions must be defined in this class, they're per config key
*/
const CONVERSIONS = [
'spv_user_password_expiration_notification_value' => [
'in' => 'daysToSeconds',
'out' => 'secondsToDays',
],
];

public function __construct($appName,
IRequest $request,
IConfig $config) {
Expand All @@ -71,6 +85,10 @@ public function updatePolicy() {
if ($this->request->getParam($key) !== null) {
if ($key !== 'spv_def_special_chars_value' && \substr($key, -6) === '_value') {
$value = \min(\max(0, (int)$this->request->getParam($key)), 255);
if (isset(self::CONVERSIONS[$key]['in'])) {
$convertFuncName = self::CONVERSIONS[$key]['in'];
$value = $this->$convertFuncName($value);
}
$this->config->setAppValue('password_policy', $key, $value);
} else {
$this->config->setAppValue('password_policy', $key, \strip_tags($this->request->getParam($key)));
Expand All @@ -92,9 +110,33 @@ public function getPriority() {
public function getPanel() {
$template = new Template('password_policy', 'admin');
foreach(self::DEFAULTS as $key => $default) {
$template->assign($key, $this->config->getAppValue('password_policy', $key, $default));
$value = $this->config->getAppValue('password_policy', $key, $default);
if (isset(self::CONVERSIONS[$key]['out'])) {
$convertFuncName = self::CONVERSIONS[$key]['out'];
$value = $this->$convertFuncName($value);
}
$template->assign($key, $value);
}
return $template;
}

/**
* Convert the days to seconds
* @param int $days
* @return int the number of seconds
*/
private function daysToSeconds($days) {
return $days * 24 * 60 * 60;
}

/**
* Convert seconds to days. The value will always be rounded up,
* so 1 second will be converted to 1 day
* @param int $seconds the number of seconds to be converted
* @return int the number of days in those seconds, rounded up
*/
private function secondsToDays($seconds) {
$floatDays = $seconds / (24 * 60 * 60);
return \intval(\ceil($floatDays));
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if using ceil is correct here.

so this means if someone uses occ to set a seconds value in the DB, the UI would show it as the higher value for days.
I guess ok for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The UI is only displaying integer number of days. So we have to do something here. When testing, I will understand that it will show 1 day, but underneath I actually set something like 300 seconds only.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure either.
If you set 12 hours instead of 1 day, you'll see 1 day instead of 0 days.

Maybe we should take the restrictive approach and show 0 days. This will send notifications before the expected time.

The good thing is that it's just a visualization problem in the UI if you set the value via occ. The notifications will be sent accordingly to the value stored in the DB (in seconds)

}
}
37 changes: 36 additions & 1 deletion lib/Db/OldPasswordMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,39 @@ public function getLatestPassword($uid) {
}
return $passwords[0];
}
}

/**
* Get the passwords that are about to expire or already expired.
* Last passwords which have been changed before the timestamp are the ones
* selectable. Previous stored passwords won't be included
* In addition, passwords from multiple users are expected
* @param int $maxTimestamp timestamp marker, last passwords changed before
* the timestamp will be selected
* @return OldPassword[] the selected passwords
*/
public function getPasswordsAboutToExpire($maxTimestamp) {
$oldPasswords = [];

$query = "SELECT `f`.`id`, `f`.`uid`, `f`.`password`, `f`.`change_time` FROM (";
$query .= "SELECT `uid`, max(`change_time`) AS `maxtime` FROM `*PREFIX*user_password_history` GROUP BY `uid`";
$query .= ") AS `x` INNER JOIN `*PREFIX*user_password_history` AS `f` ON `f`.`uid` = `x`.`uid` AND `f`.`change_time` = `x`.`maxtime`";
$query .= " WHERE `f`.`change_time` < ?";

$stmt = $this->db->prepare($query);
$stmt->bindValue(1, $maxTimestamp);
$result = $stmt->execute();

if ($result === false) {
$info = \json_encode($stmt->erroInfo());
$message = "Cannot get the passwords that are about to expire. Error: {$info}";
\OCP\Util::writeLog('password_policy', $message, \OCP\Util::ERROR);
return false;
}

while ($row = $stmt->fetch()) {
$oldPasswords[] = OldPassword::fromRow($row);
Copy link
Contributor

Choose a reason for hiding this comment

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

@butonic is it ok scalability-wise ? or should we LIMIT and rely on rerunning the job more often, or use yield ?

I imagine this could happen if an admin runs the occ command and decides that ALL users of the system need to change their password, so this would return all user entries here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yield seems a better idea than the current one.
Running the job more often might not be a good idea: we'll need to rely on offset and limit, and some password might have changed in between messing with the expected order. We might not sent notifications for some users. We'll also need to hit the DB with the same query several times per day, instead of once or twice per day.

Copy link
Member

Choose a reason for hiding this comment

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

@jvillafanez the subselect is what you actually need, right?

In the result set you want the columns, necessary to build the entitiy, which is the outer select. You do a join on uid AND the change time to the subselect. Here you could use the id to match the exact password. However, the subselect already selects all the information you want. No need to join that to the table to get the other columns:

mysql> SELECT * FROM oc_user_password_history;
+-----+-------+----------------------------------------------------------------+-------------+
| id  | uid   | password                                                       | change_time |
+-----+-------+----------------------------------------------------------------+-------------+
|   1 | admin | 1|$2y$10$iJcpqCvfrm7RN/EBd4NPIOQrZvVr76RBrBI4DZm7pHSMVzs51/jXC |  1528802661 |
|   2 | admin | 1|$2y$10$K3v18hQGc2QppEAUwfIlVeT3v5I0WpY.Q5GG2GtiGg20r0pyd0fmS |  1529010840 |
|   3 | admin | 1|$2y$10$G5JjxOGGXT1PdcmFNf.p.evOIu.64dTeDOPEdTdRSTSSeuXI3ntkS |  1529010886 |
|   4 | admin | 1|$2y$10$SuAmaBeyA0LBXji54ekG.eMHywoqP1h573VbsYgwTndCMqN6aTYcS |  1529012884 |
|   5 | admin | 1|$2y$10$gKn4dZ60ALZU8DKFLzxCgeMIop5I.G6/.4PSzYZcY2Pb3vA57y4NS |  1529012941 |
|   6 | admin | 1|$2y$10$.5LFRjes2A0O061MRWxSGuzqbpOfqWS7EnlP45jKC8.LNYg1tymBq |  1529013064 |
|   7 | admin | 1|$2y$10$NVaealeCQTdfv5aU68RQ.OVbEQKUfJwlln0Mfp8Ac03p7iHqQtY06 |  1529013338 |
|   8 | admin | 1|$2y$10$SaFlqZSrolZs4wqGd7j/AOWvIBBVM8SfLfVCzpqg3ZfRdx55/Jqh2 |  1529013408 |
|   9 | admin | 1|$2y$10$cx/UbEsFFj0YQ3ICajrcPeCXum8AGwkiz08pf.ZSB1/wnQSH19WZS |  1529408997 |
|  10 | admin | 1|$2y$10$YdLFzYh36M12OQ6VllkwCeLePRQrXn/PJNl77OTHACz4BAIazXnNO |  1529409291 |
|  11 | admin | 1|$2y$10$n64oJTgbkDDuQtEKVw4/yOREctvjP1qreD5zPwR7bU.M.HV37I0gG |  1529409359 |
|  12 | admin | 1|$2y$10$rXQ5h9nSPUg9UVPhiLmPaOhkYxvX.YEU9/UIw5CDA.vgEcUeAfuYW |  1529409618 |
|  13 | admin | 1|$2y$10$H7q2D2JxgZ8CuuPqyBMqIOCxbwKl.8gSiE2vl.B5lFBDQSJ8Mcbva |  1529409667 |
|  14 | admin | 1|$2y$10$6bQRZnS0ezfJWsLKmbPX2.eyGiL6PM6d147kjMYnhSMvY6FG5oqj6 |  1529409986 |
|  15 | admin | 1|$2y$10$WaIcpl4Fm2BjxzEZx3.nvuG.vIvKkrUg0Vbu4M34ZpVTYc/AKAuNq |  1529410455 |
|  16 | admin | 1|$2y$10$awvxbG5wL54Zg6sYGgFpveskJ/bsjlPJVluKOtMe5r6/IgDHH3P2e |  1529410477 |
|  17 | admin | 1|$2y$10$UBEo7SutLVoQUQGysRk5seYvt8YUcl0M8Z3lP10m1KvuTtTDMT8i2 |  1529437059 |
|  18 | admin | 1|$2y$10$TWYrQpBkWMVJN45MmUi1KOaD.9gsRTUlQoYeaBpXdB/Xkuzb9aEli |  1529672819 |
|  19 | admin | 1|$2y$10$8QkkU1ZDwak0Ih2dIyTXW.1RmgwdYf9Es9bTD2UQuODAUpDc9xUTe |  1529672848 |
|  20 | admin | 1|$2y$10$Co8qKhNqf.ww9.BRzHbX0uh7E4p8XkPrctWZNxdMg3OSX4afImIQm |  1529673004 |
|  21 | admin | 1|$2y$10$l/bvtHoAzfgHX9O7VIsJiu1nUhQAd7y1WhAWZgpUm42fcDzYKtlne |  1529673041 |
|  22 | admin | 1|$2y$10$l1ToXrY7MgD2KkiZVDjYce.Q14m2vkWx7q1KlvloE4W3oZGlGN.t2 |  1529674775 |
|  23 | admin | 1|$2y$10$03WKH1TKscnOiqCoOTJoqOujnfJFlFSeWgObtSQj2nCh9DO99gfKK |  1529677007 |
|  24 | admin | 1|$2y$10$L/MDFrjGMnPm4nOiZ7mvuephhVUL1pvn1KsnMCPo3XwmCukB40R.K |  1529919562 |
|  25 | admin | 1|$2y$10$WRVgklX06FPQ9.gNGUTiLuMyTlinMw73DJZLqx3LJ2LlSBXB4WvvG |  1529919654 |
|  26 | admin | 1|$2y$10$VHt.R.lZX0PdYR86kJre.OgJCyaLXOgNjHAex6xXW/wKkOgRjnffK |  1529922722 |
|  27 | admin | 1|$2y$10$fXYr17rsqyztzu6HokqLpua/nm9fNf5mPZotAYKPLpvB2uU5C8gcq |  1529922783 |
|  28 | admin | 1|$2y$10$qR7w7vzae4XnZmWXOEhvEeGFqr.e7S6pKP0eEH4NCNHJHKiNwal4q |  1529928109 |
|  29 | admin | 1|$2y$10$x/be52NbGUdyqk2RtKihxOwr7QszeWVGHjRHcteiFwtNSRvXLH27K |  1529928317 |
|  30 | admin | 1|$2y$10$0/1NgwaFYkz0byBDP6FRauSesDuaa39KR.QlbbjXmDHX/LTDaHJU2 |  1529929051 |
|  31 | admin | 1|$2y$10$ncy2nAfKqopDD7xfL2xzQutajq1ULz7U7llKoYJA7f5kpJiyfISF6 |  1529929158 |
|  32 | admin | 1|$2y$10$Kojhu9ogf71pDODVoZAcwufBVAYYfZ0rskzTKGt.7u.0TcLKItn1K |  1529929495 |
|  33 | admin | 1|$2y$10$mQGjUuW52Si1SFpj8FaXfO4xFL6LJCLjrQm2WSahG2db0qw7vpjm. |  1529934553 |
|  34 | admin | 1|$2y$10$OFiETck8EABckSgbyKl6r.eOeWdd8WvHRR8OwEyVePN0ww9Gmer2m |  1530000377 |
|  35 | admin | 1|$2y$10$e8wPr0e.5qgv.rQ5BzKhRe8p5ErCe2kCIxWLhjsPcU3CzsBVVqs9i |  1530000452 |
|  36 | admin | 1|$2y$10$83uDOakjvWOhcBtv1QqnZu19AmMVSAd8Z7vHQ8yB326qBGdSS/Kty |  1530016008 |
|  37 | admin | 1|$2y$10$Igv7hYqcnNeBLASfr4UAxeYc1.5N/DJYqkHye/WBqLKVTcBIJhN6m |  1530019248 |
|  38 | admin | 1|$2y$10$ivCQvEIH2NS7KT.3nStpR.maBNcCMdOrPFWHvZqlzzUUr6Dk16of. |  1530087458 |
|  39 | admin | 1|$2y$10$pPNexxPypESD5MUobKKx..q4pbNzkhDSrb04fKvu29pJTb5cYaWtu |  1530087496 |
|  40 | admin | 1|$2y$10$wMAebL07zo3jfhY1g9f5KumVygs5HVIwnnYLoxTOilarc0oAO8GGS |  1530088064 |
| 215 | admin | 1|$2y$10$9DvIrgMhp16vS8PIIAiEL.9FgV03auKPWYzdnkLJ7ti0yyvo22SWG |  1530267611 |
| 216 | test  | 1|$2y$10$zfc50VR1AvRoAagYA/v94egSY9S1tmDzmK.stUWjP86ENem3KtO3G |  1530611134 |
| 217 | admin | 1|$2y$10$lCNQKQGGrfyag2Nc53w92eH/kuQEVxYX66RphyvoByE.nll.B0j6q |  1531385660 |
+-----+-------+----------------------------------------------------------------+-------------+
43 rows in set (0.00 sec)

mysql> SELECT `f`.`id`, `f`.`uid`, `f`.`password`, `f`.`change_time` FROM (   SELECT `uid`, max(`change_time`) AS `maxtime` FROM `oc_user_password_history` GROUP BY `uid`   ) AS `x` INNER JOIN `oc_user_password_history` AS `f` ON `f`.`uid` = `x`.`uid` AND `f`.`change_time` = `x`.`maxtime`   WHERE `f`.`change_time` < 1540019248;
+-----+-------+----------------------------------------------------------------+-------------+
| id  | uid   | password                                                       | change_time |
+-----+-------+----------------------------------------------------------------+-------------+
| 217 | admin | 1|$2y$10$lCNQKQGGrfyag2Nc53w92eH/kuQEVxYX66RphyvoByE.nll.B0j6q |  1531385660 |
| 216 | test  | 1|$2y$10$zfc50VR1AvRoAagYA/v94egSY9S1tmDzmK.stUWjP86ENem3KtO3G |  1530611134 |
+-----+-------+----------------------------------------------------------------+-------------+
2 rows in set (0.00 sec)

mysql> SELECT `id`, `uid`, `password`, max(`change_time`) AS `change_time` FROM `oc_user_password_history` WHERE `change_time` < 1540019248 GROUP BY `uid`;
+-----+-------+----------------------------------------------------------------+-------------+
| id  | uid   | password                                                       | change_time |
+-----+-------+----------------------------------------------------------------+-------------+
|   1 | admin | 1|$2y$10$iJcpqCvfrm7RN/EBd4NPIOQrZvVr76RBrBI4DZm7pHSMVzs51/jXC |  1531385660 |
| 216 | test  | 1|$2y$10$zfc50VR1AvRoAagYA/v94egSY9S1tmDzmK.stUWjP86ENem3KtO3G |  1530611134 |
+-----+-------+----------------------------------------------------------------+-------------+
2 rows in set (0.00 sec)

Can you double check if I misunderstand the query you built? You should be able to get the same results with this.

The indexes that need to be used will vary, depending on the number of passwords per user in the table.

yield would be great, but does not add too much benefit. even passwords 300k users should fit in memory ...

Copy link
Member Author

@jvillafanez jvillafanez Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

mysql> SELECT `id`, `uid`, `password`, max(`change_time`) AS `change_time` FROM `user_password_history` WHERE `change_time` < 1540019248 GROUP BY `uid`;
ERROR 1055 (42000): Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'owncloud.user_password_history.id' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by

I expect something similar with postgresql.

Note that I also expect to get valid data, specially when we are going to get objects from there. The last query doesn't return a valid existing object, although the information we need from it is correct. I'd prefer to be strict with this.

Copy link
Member

@butonic butonic Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Then move the mtime comparison into the subselect. Hm or we do a left join to get rid of the derived table:

mysql> SET SESSION sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION';
Query OK, 0 rows affected (0.00 sec)

mysql> SELECT p.* FROM `oc_user_password_history` p LEFT OUTER JOIN `oc_user_password_history` p2 ON p.uid = p2.uid AND p.change_time < p2.change_time WHERE p2.id IS NULL;
+-----+-------+----------------------------------------------------------------+-------------+
| id  | uid   | password                                                       | change_time |
+-----+-------+----------------------------------------------------------------+-------------+
| 216 | test  | 1|$2y$10$zfc50VR1AvRoAagYA/v94egSY9S1tmDzmK.stUWjP86ENem3KtO3G |  1530611134 |
| 217 | admin | 1|$2y$10$lCNQKQGGrfyag2Nc53w92eH/kuQEVxYX66RphyvoByE.nll.B0j6q |  1531385660 |
+-----+-------+----------------------------------------------------------------+-------------+
2 rows in set (0.00 sec)

mysql> EXPLAIN SELECT p.* FROM `oc_user_password_history` p LEFT JOIN `oc_user_password_history` p2 ON p.uid = p2.uid AND p.change_time < p2.change_time WHERE p2.id IS NULL;
+----+-------------+-------+------+-------------------------------------------------------------------+--------------------+---------+-------------+------+--------------------------------------+
| id | select_type | table | type | possible_keys                                                     | key                | key_len | ref         | rows | Extra                                |
+----+-------------+-------+------+-------------------------------------------------------------------+--------------------+---------+-------------+------+--------------------------------------+
|  1 | SIMPLE      | p     | ALL  | NULL                                                              | NULL               | NULL    | NULL        |   43 |                                      |
|  1 | SIMPLE      | p2    | ref  | pp_uid_index,pp_mtime_index,pp_uid_mtime_index,pp_mtime_uid_index | pp_uid_mtime_index | 258     | core1.p.uid |    1 | Using where; Using index; Not exists |
+----+-------------+-------+------+-------------------------------------------------------------------+--------------------+---------+-------------+------+--------------------------------------+
2 rows in set (0.00 sec)

As you can see I added another index pp_uid_mtime_index with CREATE INDEX pp_uid_mtime_index ON oc_user_password_history (uid, change_time); and the query can use it: Using where; Using index; Not exists

Copy link
Member

Choose a reason for hiding this comment

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

Hm ... it really depends on the dbms which query is more efficient. go with the current code. In any case this wont be a performance issue.

}
$stmt->closeCursor();
return $oldPasswords;
Copy link
Contributor

Choose a reason for hiding this comment

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

this array is empty ?

}
}
42 changes: 40 additions & 2 deletions lib/HooksHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@
use OCA\PasswordPolicy\Db\OldPasswordMapper;
use OCA\PasswordPolicy\Rules\PasswordExpired;
use OCA\PasswordPolicy\Rules\PolicyException;
use OCA\PasswordPolicy\UserNotificationConfigHandler;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\IConfig;
use OCP\IL10N;
use OCP\ISession;
use OCP\IUser;
use OCP\Security\IHasher;
use OCP\Notification\IManager;
use Symfony\Component\EventDispatcher\GenericEvent;

class HooksHandler {
Expand Down Expand Up @@ -60,6 +62,12 @@ class HooksHandler {
/** @var ISession */
private $session;

/** @var IManager */
private $notificationManager;

/** @var UserNotificationConfigHandler */
private $userNotificationConfigHandler;

public function __construct(
IConfig $config = null,
Engine $engine = null,
Expand All @@ -68,7 +76,9 @@ public function __construct(
IL10N $l10n = null,
PasswordExpired $passwordExpiredRule = null,
OldPasswordMapper $oldPasswordMapper = null,
ISession $session = null
ISession $session = null,
IManager $notificationManager = null,
UserNotificationConfigHandler $userNotificationConfigHandler = null
) {
$this->config = $config;
$this->engine = $engine;
Expand All @@ -78,6 +88,8 @@ public function __construct(
$this->passwordExpiredRule = $passwordExpiredRule;
$this->oldPasswordMapper = $oldPasswordMapper;
$this->session = $session;
$this->notificationManager = $notificationManager;
$this->userNotificationConfigHandler = $userNotificationConfigHandler;
}

private function fixDI() {
Expand All @@ -103,6 +115,8 @@ private function fixDI() {
$this->hasher
);
$this->session = \OC::$server->getSession();
$this->notificationManager = \OC::$server->getNotificationManager();
$this->userNotificationConfigHandler = new UserNotificationConfigHandler($this->config);
}
}

Expand Down Expand Up @@ -196,11 +210,34 @@ public function saveOldPassword(GenericEvent $event) {
$user = $this->getUser($event);
$password = $event->getArgument('password');

$userId = $user->getUID();

$oldPassword = new OldPassword();
$oldPassword->setUid($user->getUID());
$oldPassword->setUid($userId);
$oldPassword->setPassword($this->hasher->hash($password));
$oldPassword->setChangeTime($this->timeFactory->getTime());
$this->oldPasswordMapper->insert($oldPassword);

// get previous marks
$aboutToExpireMark = $this->userNotificationConfigHandler->getMarkAboutToExpireNotificationSentFor($userId);
$expiredMark = $this->userNotificationConfigHandler->getMarkExpiredNotificationSentFor($userId);

$this->userNotificationConfigHandler->resetExpirationMarks($userId);

if ($aboutToExpireMark !== null) {
$notification = $this->notificationManager->createNotification();
$notification->setApp('password_policy')
->setUser($userId)
->setObject('about_to_expire', $aboutToExpireMark);
$this->notificationManager->markProcessed($notification);
}
if ($expiredMark !== null) {
$notification = $this->notificationManager->createNotification();
$notification->setApp('password_policy')
->setUser($userId)
->setObject('expired', $expiredMark);
$this->notificationManager->markProcessed($notification);
}
}

public function savePasswordForCreatedUser(GenericEvent $event) {
Expand All @@ -214,6 +251,7 @@ public function savePasswordForCreatedUser(GenericEvent $event) {
$oldPassword->setPassword($this->hasher->hash($password));
$oldPassword->setChangeTime($this->timeFactory->getTime());
$this->oldPasswordMapper->insert($oldPassword);
$this->userNotificationConfigHandler->resetExpirationMarks($userid);
}

/**
Expand Down
Loading