send notification when passwords are about to expire #27

jvillafanez · 2018-07-09T13:13:42Z

Fixes #18

Open tasks

better SQL query
agree whether the JS hack is ok and/or whether we want to hide the "Change Password" button and exploit hack, see send notification when passwords are about to expire #27 (comment)
delete related notifications after password was changed
solve confusing settings in page (PVince81 was confused)
adjust wording based on @pmaier1 feedback Notify when user password is about to expire or expired #18 (comment)
unit tests
~~JS tests~~

CLAassistant · 2018-07-09T13:13:50Z

All committers have signed the CLA.

PVince81 · 2018-07-09T14:19:51Z

lib/Db/OldPasswordMapper.php

+	public function getPasswordsAboutToExpire($maxTimestamp) {
+		$oldPasswords = [];
+
+		$query = "select `f`.`*` from (";


I'd prefer using a simpler query like

SELECT uid, MAX(change_time) as `maxtime` FROM`*PREFIX*user_password_history` GROUP BY uid HAVING MAX(change_time) < ?

It was pointed to me that the reason for this is because the OldPassword object requires the original fields to be present in the select, so this JOIN is here just to achieve this.

I suggested skipping the OldPassword object here and just returning a list of users. This wouldn't match well with the "ObjectMapper" concept. So maybe we should move this method elsewhere ?

Our problem currently is to extend this query to also cover cases where no change_time is set for #24

@jvillafanez how about adding a new method on OldPassword that lets you create an instance with a given id ? Then here you could simplify the query as discussed instead of having to contraint it to OldPassword::fromRow()

we'll still need to get the information from the rows, an that will be one hit to the DB per object.

it should be possible to include all information in the original query somehow

let's continue for now and if needed we can ask @butonic to help us optimize this

unit tests will prove that it still works regardless

PVince81 · 2018-07-10T18:58:31Z

Query doesn't work on MariaDB:

MariaDB [owncloud]> select `f`.`*` 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` < 90;
ERROR 1054 (42S22): Unknown column 'f.*' in 'field list'

PVince81 · 2018-07-10T19:21:59Z

discard notifications after password change, if possible. maybe need to store notification id in the oc_preferences ?

PVince81 · 2018-07-10T19:29:59Z

It seems the notifications app has a bug: when only a single action is specified, it doesn't display any action button... doesn't even seem to return the actions structure through the OCS call for some reason.
The good news is that the email is sent anyway.

So while we can rely on this bug for now, we do need to think about what happens if the button ever becomes visible. I'll still investigate the JS approach.

codecov-io · 2018-07-10T19:34:54Z

Codecov Report

Merging #27 into master will increase coverage by 5.96%.
The diff coverage is 85.89%.

@@             Coverage Diff              @@
##             master      #27      +/-   ##
============================================
+ Coverage     65.11%   71.07%   +5.96%     
- Complexity      131      190      +59     
============================================
  Files            22       25       +3     
  Lines           602      854     +252     
============================================
+ Hits            392      607     +215     
- Misses          210      247      +37

Impacted Files	Coverage Δ	Complexity Δ
templates/password.php	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
lib/Db/OldPasswordMapper.php	`88.88% <ø> (-11.12%)`	`8 <0> (+3)`
lib/Controller/SettingsController.php	`0% <0%> (ø)`	`14 <2> (+4)`	⬆️
appinfo/app.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
templates/admin.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/AppInfo/Application.php	`0% <0%> (ø)`	`3 <2> (+2)`	⬆️
lib/Notifier.php	`100% <100%> (ø)`	`12 <12> (?)`
lib/UserNotificationConfigHandler.php	`100% <100%> (ø)`	`23 <23> (?)`
lib/Rules/PasswordHistory.php	`92.85% <100%> (ø)`	`5 <0> (ø)`	⬇️
lib/Controller/PasswordController.php	`97.77% <100%> (ø)`	`11 <0> (ø)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35c3f7a...5157dde. Read the comment docs.

PVince81 · 2018-07-10T19:39:45Z

Ok, I debugged a bit and found out why: we need to implement a Notifier to be able to translate the texts.

Currently the action button does not appear because we don't have "parsed actions". This also means that we could deliberately not return a parsed action so no button will be displayed.

The email sending code from the notifications app relies on regular action, not parsed action.
Raised here: owncloud/notifications#213

PVince81 · 2018-07-10T20:13:53Z

@jvillafanez look at this beautiful hack: be90884

I've enabled the parsed actions so the button becomes visible. If we want to, we could decide to still disable it so the button doesn't appear and exploit bug owncloud/notifications#213

Now with the button visible, the notifications app only sends out the "OCA.Notifications.Action" jquery event if the ajax call was successful. So here I've set the link action to points at the settings page with GET, which will succeed anyway with 200 OK. After success, we catch the event in the password policy's "js/notification.js" and actually redirect to the link.

Nice trick but I hope we can make redirects work officially with owncloud/notifications#212

One missing part is that clicking on "Change Password" doesn't automatically delete the notification. This could be acceptable as we should delete the notifications after the password was changed.

phil-davis

I realize this is WIP. Just making a few comments so you can consider them as you work on this.
The main thing I see is that it would be very nice for the days<->seconds conversions to all be encapsulated in a common function, and thus have only one instance of the 24 * 60 * 60 conversion calculation.

phil-davis · 2018-07-11T00:18:11Z

lib/Db/OldPasswordMapper.php

+
+	/**
+	 * Get the passwords that are about to expired or already expired.


"about to expired" -> "about to expire"

phil-davis · 2018-07-11T00:18:37Z

lib/Db/OldPasswordMapper.php

+	/**
+	 * Get the passwords that are about to expired or already expired.
+	 * Last passwords which has been changed before the timestamp are the ones


"which has been" -> "which have been"

phil-davis · 2018-07-11T00:21:02Z

lib/Db/OldPasswordMapper.php

+		if ($result === false) {
+			$info = \json_encode($stmt->erroInfo());
+			$message = "Cannot get the password that are going to be expired: error: {$info}";


"Cannot get the passwords that are about to expire. Error:"

phil-davis · 2018-07-11T01:10:43Z

lib/Jobs/PasswordExpirationNotifierJob.php

+			return;  // expiration not configured
+		}
+
+		$expirationTime = $expirationTime * 24 * 60 * 60;  // convert days to seconds


Personally I do not like seeing units of measure changed in the same variable name. It too easily results in future confusion about the value at different points in the code.
Call the first variable $expirationDays or?

There is a daysToSeconds function in another file above. I wonder how we can contain these kind of unit-of-measure conversions to some always-accessible functions, rather than having mentions of 24 and 60 around the code?

I expect a follow up PR to store the expirationTime in seconds soon. Once it's done we can remove this line and move on. I'd prefer to keep this for now.
It this were a long time solution, I'd definitely agree with you.

In fact, it makes more sense to move this conversion to the getExpirationTime function

phil-davis · 2018-07-11T01:13:09Z

lib/Jobs/PasswordExpirationNotifierJob.php

+		}
+
+		// ensure ranges are correct
+		if ($expirationTime <= $expirationTimeNotification) {


This surprises me. $expirationTime is in seconds. But $expirationTimeNotification has not been converted to seconds.
Maybe getExpirationTime() returns days, and getExpirationTimeForNormalNotification() returns seconds - but if that is true then it also a recipe for bugs.

I'll include a PHPDoc in the functions to clarify

assuming this scenario will never happen in real life and is just hardening ?

there's that occ command to set an expire date, I wonder if an admin could use it the wrong way and set an expire date in the future/past (whichever would cause this condition). Maybe need to check that the occ command properly validates to enforce this ?

occ command allows past and future:
occ user:expire-password user1 "5 days"
occ user:expire-password user1 -- '-5 days'

@jvillafanez would this break here ?

phil-davis · 2018-07-11T02:34:30Z

lib/UserNotificationConfigHandler.php

+		if ($storedId === null) {
+			return false;  // notification not sent
+		} elseif (\intval($storedId) !== $passInfo->getId()) {
+			// if the pasword id doesn't match the one stored, the notification hasn't been sent


s/pasword/password

phil-davis · 2018-07-11T02:34:50Z

lib/UserNotificationConfigHandler.php

+		if ($storedId === null) {
+			return false;  // notification not sent
+		} elseif (\intval($storedId) !== $passInfo->getId()) {
+			// if the pasword id doesn't match the one stored, the notification hasn't been sent


s/pasword/password

phil-davis · 2018-07-11T02:36:39Z

tests/NotifierTest.php

+		// expiration time starting from the password change time
+		$notification->method('getMessageParameters')->willReturn([$initialTime, $expireIn]);
+
+		$this->timeFactory->method('getTime')->willReturn($initialTime + (7 * 24 * 60 * 60));


Another days to seconds conversion.

phil-davis · 2018-07-11T02:37:02Z

tests/NotifierTest.php

+		// expiration time starting from the password change time
+		$notification->method('getMessageParameters')->willReturn([$initialTime, $expireIn]);
+
+		$this->timeFactory->method('getTime')->willReturn($initialTime + (12 * 24 * 60 * 60));


Another days to seconds conversion.

phil-davis · 2018-07-11T02:38:32Z

tests/NotifierTest.php

+
+		$notification->expects($this->once())
+			->method('setParsedMessage')
+			->with('Your password has already expired 2 days ago');


Hmmm - wherever in the real code this message is generated, the text could be more simple and straightforward:
Your password expired 2 days ago

PVince81 · 2018-07-11T04:52:08Z

Typos and texts adjusted

@jvillafanez please look into:

mix up of "days to seconds" in various places, see comments

jvillafanez · 2018-07-11T08:13:22Z

I think the "daysToSeconds" problem should be fine now.

DEFAULT_EXPIRATION_FOR_NORMAL_NOTIFICATION constant is in seconds, but shown like that to make it easy to know that those are 30 days
The conversion that was in the cron job has been moved to the UserNotificationConfigHandler to return seconds instead of days, so it's more consistent with the rest of the functions. Note that the conversion is currently required as the value is stored in days, but it's expected to store in seconds at some point.
The ones for the test should be fine. The tested functions work with seconds, but messages refer to days.

jvillafanez · 2018-07-11T09:22:15Z

discard notifications after password change, if possible. maybe need to store notification id in the oc_preferences ?

The notification is discarded after clicking the action. The problem probably is that the redirection prevent further code execution.
Code flow is:

~~Trigger event~~
~~Respond to event (by all the event listeners)~~
~~Delete notification~~

~~If a redirection happens in 2, the deletion won't happen (or so it seems). Delaying the redirection should do the trick.~~

Got confused with the dismiss.

jvillafanez · 2018-07-11T09:50:07Z

lib/Jobs/PasswordExpirationNotifierJob.php

+	}
+
+	private function getNotificationLink() {
+		return $this->urlGenerator->linkToRouteAbsolute(


I guess this is fine for an "about to expire" notification, but if the password is expired maybe we should send a link to the password policy app instead, as I expect this link won't be accesible for the user.

if the password is expired the session might still be there, so the user will get to the page

now if the user got kicked out, opening the URL will display the login page + "change your password" full screen page. And then the user lands on the settings page. This is ok for now as discussed with @pmaier1 before.

jvillafanez · 2018-07-11T12:05:12Z

delete related notifications after password was changed

This should be fixed with b8c1a00 although the solution is quite dirty. We should schedule some time to find a proper fix.

jvillafanez · 2018-07-11T16:59:45Z

This should be ready unless someone says otherwise.

We'll need to schedule some time to refactor the part of the notifications because of the unittests, but I'll need a bit of time to plan how to do it properly. In any case, the code should be working, so we can do it after merging.

phil-davis · 2018-07-11T17:01:18Z

I will give it a manual test in the morning and see what I can break.

PVince81

Looks good to me overall.

Unless there are objections, let's check the comments then merge.

Texts and query optimization can be done in a separate PR.

PVince81 · 2018-07-12T07:50:02Z

lib/Controller/SettingsController.php

+	 */
+	private function secondsToDays($seconds) {
+		$floatDays = $seconds / (24 * 60 * 60);
+		return \intval(\ceil($floatDays));


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.

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.

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)

PVince81 · 2018-07-12T07:54:58Z

lib/Db/OldPasswordMapper.php

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


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

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.

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

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.

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

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.

PVince81 · 2018-07-12T07:56:28Z

lib/Jobs/PasswordExpirationNotifierJob.php

+		}
+
+		// ensure ranges are correct
+		if ($expirationTime <= $expirationTimeNotification) {


assuming this scenario will never happen in real life and is just hardening ?

there's that occ command to set an expire date, I wonder if an admin could use it the wrong way and set an expire date in the future/past (whichever would cause this condition). Maybe need to check that the occ command properly validates to enforce this ?

PVince81 · 2018-07-12T07:59:33Z

lib/Jobs/PasswordExpirationNotifierJob.php

+		}
+	}
+
+	private function sendAboutToExpireNotification(OldPassword $passInfo, $expirationTime) {


quick PHPdoc ?

PVince81 · 2018-07-12T08:00:00Z

lib/Jobs/PasswordExpirationNotifierJob.php

+		$this->unConfigHandler->markAboutToExpireNotificationSentFor($passInfo);
+	}
+
+	private function sendPassExpiredNotification(OldPassword $passInfo, $expirationTime) {


PVince81 · 2018-07-12T08:03:28Z

lib/UserNotificationConfigHandler.php

+
+	/**
+	 * Return the number of seconds until a user should receive a notification
+	 * that his password is about to expire. This _should_ be less than the value


s/his/their/

PVince81 · 2018-07-12T08:07:15Z

templates/admin.php

+				<label>
+					<input type="checkbox" name="spv_user_password_expiration_notification_checked" <?php if ($_['spv_user_password_expiration_notification_checked']): ?> checked="checked"<?php endif; ?>/>
+					<input type="number" name="spv_user_password_expiration_notification_value"  min="0" value="<?php p($_['spv_user_password_expiration_notification_value']) ?>" placeholder="30"/>
+					<span><?php p($l->t('days to send a notification before the password expires'));?></span>


this message sounds odd, maybe "number of days until expiration after which to send a notification"

@phil-davis any better ideas ?

"send a notification this many days before the password expires"
If it is OK to use this style of grammar.
Forcing the sentence so that it begins with the number "30" makes the English difficult, whatever you do.

PVince81 · 2018-07-12T08:09:36Z

tests/Db/OldPasswordMapperTest.php

+	}
+
+	public function testGetPasswordsAboutToExpireSomePassMarked() {
+		$baseTime = \time();


maybe use a hard-coded value for deterministic tests ?

I don't think it's needed. The rest of the timestamps are based on whatever the $baseTime is, and we have control over the current time via the timeFactory (also based on the $baseTime). There won't be problems about this test failing randomly

PVince81 · 2018-07-12T08:09:54Z

tests/Db/OldPasswordMapperTest.php

+	}
+
+	public function testGetPasswordsAboutToExpireAllMarked() {
+		$baseTime = \time();


PVince81 · 2018-07-12T08:12:40Z

tests/HooksHandlerTest.php

+				$holder = [];
+				$mock = $this->createMock(INotification::class);
+				$mock->method('setApp')->will($this->returnCallback(function($app) use (&$holder, $mock) {
+					$holder['app'] = $app;


there might be a better way, see https://github.com/owncloud/core/blob/master/apps/files_sharing/tests/NotifierTest.php#L145 and https://github.com/owncloud/core/blob/master/apps/files_sharing/tests/Service/NotificationPublisherTest.php#L101

I'm ok if you leave the tests as is

phil-davis · 2018-07-12T08:48:52Z

@jvillafanez @PVince81 can someone rebase this please?
At the moment, if I checkout this branch then I lose other recent fixes that are in master. That makes it difficult when testing.

phil-davis · 2018-07-12T09:07:09Z

Settings UI issue:

Set days until user password expires to 14 and check the box.
Set days to send a notification before the password expires to 30 and check the box.
Press Save

Expected behavior:
You cannot set (2) greater than or equal to (1).
Something should ensure that (2) < (1)

Actual behavior::
It saves OK - something bad is going to happen later! I don't even want to bother finding out what that is.

jvillafanez · 2018-07-12T09:07:13Z

rebased

jvillafanez · 2018-07-12T09:15:46Z

It saves OK - something bad is going to happen later! I don't even want to bother finding out what that is.

it aborts the execution with a kind of cryptic message: https://github.com/owncloud/password_policy/pull/27/files#diff-779257764a9f14bbfce5338eb4e88ff0R83

jvillafanez · 2018-07-12T10:06:00Z

squashed the last 2 commits and included a fix for the failing test.

PVince81 · 2018-07-12T10:39:23Z

damn, created a conflict with parallel merge

PVince81 · 2018-07-12T11:44:03Z

settings UI to be done later: #41

PVince81

👍

PVince81 · 2018-07-12T11:46:20Z

damn, I broke the tests by changing the texts...

PVince81 · 2018-07-12T11:49:43Z

SQL to be addressed separately: #42

PVince81 · 2018-07-12T12:26:46Z

lib/Db/OldPasswordMapper.php

+		}
+		$stmt->closeCursor();
+		return $oldPasswords;


this array is empty ?

PVince81

👍

jvillafanez added this to the development milestone Jul 9, 2018

jvillafanez self-assigned this Jul 9, 2018

PVince81 reviewed Jul 9, 2018

View reviewed changes

jvillafanez force-pushed the notification_for_expiration branch from d7885c8 to 6e90309 Compare July 10, 2018 16:06

PVince81 mentioned this pull request Jul 10, 2018

Notify when user password is about to expire or expired #18

Closed

12 tasks

PVince81 self-assigned this Jul 10, 2018

phil-davis reviewed Jul 11, 2018

View reviewed changes

jvillafanez commented Jul 11, 2018

View reviewed changes

jvillafanez force-pushed the notification_for_expiration branch from b8c1a00 to 82ff3cc Compare July 11, 2018 12:23

jvillafanez changed the title ~~[WIP] send notification when passwords are about to expire~~ send notification when passwords are about to expire Jul 12, 2018

jvillafanez added the 3 - To Review label Jul 12, 2018

PVince81 reviewed Jul 12, 2018

View reviewed changes

jvillafanez force-pushed the notification_for_expiration branch from 61b75a5 to dfa1c7b Compare July 12, 2018 09:06

jvillafanez force-pushed the notification_for_expiration branch from dfa1c7b to 2031e80 Compare July 12, 2018 10:04

send notification when passwords are about to expire

35ccd8b

PVince81 force-pushed the notification_for_expiration branch from 998284a to 35ccd8b Compare July 12, 2018 11:40

PVince81 mentioned this pull request Jul 12, 2018

Validation of policy settings #41

Open

3 tasks

PVince81 approved these changes Jul 12, 2018

View reviewed changes

PVince81 reviewed Jul 12, 2018

View reviewed changes

lib/Db/OldPasswordMapper.php Outdated

}

$stmt->closeCursor();

return $oldPasswords;

Copy link

Contributor

PVince81 Jul 12, 2018

Choose a reason for hiding this comment

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

this array is empty ?

jvillafanez force-pushed the notification_for_expiration branch from b639572 to 58c252c Compare July 12, 2018 12:35

PVince81 approved these changes Jul 12, 2018

View reviewed changes

Hardening and test adjustments

fe7cb37

jvillafanez force-pushed the notification_for_expiration branch from 58c252c to fe7cb37 Compare July 12, 2018 12:46

Don't return value in generator

5157dde

PVince81 merged commit 1125f1d into master Jul 12, 2018

PVince81 deleted the notification_for_expiration branch July 12, 2018 14:18

PVince81 modified the milestones: development, qa Jul 16, 2018


		/**
		* Get the passwords that are about to expired or already expired.


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

send notification when passwords are about to expire #27

send notification when passwords are about to expire #27

Conversation

jvillafanez commented Jul 9, 2018 • edited by PVince81 Loading

Open tasks

CLAassistant commented Jul 9, 2018 • edited Loading

PVince81 Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 10, 2018

PVince81 commented Jul 10, 2018 • edited Loading

PVince81 commented Jul 10, 2018

codecov-io commented Jul 10, 2018 • edited Loading

Codecov Report

PVince81 commented Jul 10, 2018

PVince81 commented Jul 10, 2018

phil-davis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 11, 2018

jvillafanez commented Jul 11, 2018

jvillafanez commented Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Jul 11, 2018

jvillafanez commented Jul 11, 2018

phil-davis commented Jul 11, 2018

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

butonic Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-davis commented Jul 12, 2018

phil-davis commented Jul 12, 2018

jvillafanez commented Jul 12, 2018

jvillafanez commented Jul 12, 2018 • edited Loading

jvillafanez commented Jul 12, 2018

PVince81 commented Jul 12, 2018

PVince81 commented Jul 12, 2018

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Jul 12, 2018

PVince81 commented Jul 12, 2018

Choose a reason for hiding this comment

PVince81 left a comment

jvillafanez commented Jul 9, 2018 •

edited by PVince81

Loading

CLAassistant commented Jul 9, 2018 •

edited

Loading

PVince81 Jul 9, 2018 •

edited

Loading

PVince81 commented Jul 10, 2018 •

edited

Loading

codecov-io commented Jul 10, 2018 •

edited

Loading

jvillafanez commented Jul 11, 2018 •

edited

Loading

jvillafanez Jul 12, 2018 •

edited

Loading

butonic Jul 12, 2018 •

edited

Loading

jvillafanez commented Jul 12, 2018 •

edited

Loading