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

send notification when passwords are about to expire #27

Merged
merged 3 commits into from
Jul 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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');
}
5 changes: 4 additions & 1 deletion appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Administrators find the configuration options in the 'Security' section of the o
<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
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
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
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
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)

}
}
34 changes: 33 additions & 1 deletion lib/Db/OldPasswordMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,36 @@ 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 Generator to traverse the selected passwords
*/
public function getPasswordsAboutToExpire($maxTimestamp) {
$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;
}

while ($row = $stmt->fetch()) {
yield OldPassword::fromRow($row);
}
$stmt->closeCursor();
}
}
42 changes: 40 additions & 2 deletions lib/HooksHandler.php
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