Merge pull request #56 from owncloud/verify_user_exists

Check the user exists before sending the notification
owncloud · Jul 16, 2018 · 4e2e765 · 4e2e765
2 parents 49d7cbe + e8f14bc
commit 4e2e765
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 2 deletions.
diff --git a/lib/Jobs/PasswordExpirationNotifierJob.php b/lib/Jobs/PasswordExpirationNotifierJob.php
@@ -26,6 +26,7 @@
 use OCP\Notification\IManager;
 use OCP\IURLGenerator;
 use OCP\ILogger;
+use OCP\IUserManager;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCA\PasswordPolicy\Db\OldPasswordMapper;
 use OCA\PasswordPolicy\Db\OldPassword;
@@ -41,6 +42,9 @@ class PasswordExpirationNotifierJob extends TimedJob {
 	/** @var UserNotificationConfigHandler */
 	private $unConfigHandler;
 
+	/** @var IUserManager */
+	private $userManager;
+
 	/** @var ITimeFactory */
 	private $timeFactory;
 
@@ -54,13 +58,15 @@ public function __construct(
 		OldPasswordMapper $mapper,
 		IManager $manager,
 		UserNotificationConfigHandler $unConfigHandler,
+		IUserManager $userManager,
 		ITimeFactory $timeFactory,
 		IURLGenerator $urlGenerator,
 		ILogger $logger
 	) {
 		$this->mapper = $mapper;
 		$this->manager = $manager;
 		$this->unConfigHandler = $unConfigHandler;
+		$this->userManager = $userManager;
 		$this->timeFactory = $timeFactory;
 		$this->urlGenerator = $urlGenerator;
 		$this->logger = $logger;
@@ -122,7 +128,8 @@ protected function run($arguments) {
 	 * (for example, 90 days - in seconds)
 	 */
 	private function sendAboutToExpireNotification(OldPassword $passInfo, $expirationTime) {
-		if ($this->unConfigHandler->isSentAboutToExpireNotification($passInfo)) {
+		if (!$this->userManager->userExists($passInfo->getUid()) ||
+				$this->unConfigHandler->isSentAboutToExpireNotification($passInfo)) {
 			return;  // notification already sent
 		}
 
@@ -158,7 +165,8 @@ private function sendAboutToExpireNotification(OldPassword $passInfo, $expiratio
 	 * (for example, 90 days - in seconds)
 	 */
 	private function sendPassExpiredNotification(OldPassword $passInfo, $expirationTime) {
-		if ($this->unConfigHandler->isSentExpiredNotification($passInfo)) {
+		if (!$this->userManager->userExists($passInfo->getUid()) ||
+				$this->unConfigHandler->isSentExpiredNotification($passInfo)) {
 			return;  // notification already sent
 		}
 

diff --git a/tests/Jobs/PasswordExpirationNotifierJobTest.php b/tests/Jobs/PasswordExpirationNotifierJobTest.php
@@ -27,6 +27,7 @@
 use OCP\Notification\IAction;
 use OCP\IURLGenerator;
 use OCP\ILogger;
+use OCP\IUserManager;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCA\PasswordPolicy\Db\OldPasswordMapper;
 use OCA\PasswordPolicy\Db\OldPassword;
@@ -44,6 +45,9 @@ class PasswordExpirationNotifierJobTest extends TestCase {
 	/** @var UserNotificationConfigHandler */
 	private $unConfigHandler;
 
+	/** @var IUserManager */
+	private $userManager;
+
 	/** @var ITimeFactory */
 	private $timeFactory;
 
@@ -67,6 +71,9 @@ protected function setUp() {
 		$this->unConfigHandler = $this->getMockBuilder(UserNotificationConfigHandler::class)
 			->disableOriginalConstructor()
 			->getMock();
+		$this->userManager = $this->getMockBuilder(IUserManager::class)
+			->disableOriginalConstructor()
+			->getMock();
 		$this->timeFactory = $this->getMockBuilder(ITimeFactory::class)
 			->disableOriginalConstructor()
 			->getMock();
@@ -81,6 +88,7 @@ protected function setUp() {
 			$this->mapper,
 			$this->manager,
 			$this->unConfigHandler,
+			$this->userManager,
 			$this->timeFactory,
 			$this->urlGenerator,
 			$this->logger
@@ -209,6 +217,10 @@ public function testRunAboutToExpireAlreadySent() {
 		$this->mapper->method('getPasswordsAboutToExpire')
 			->willReturn([$returnedOldPassword]);
 
+		$this->userManager->method('userExists')
+			->with('usertest')
+			->willReturn(true);
+
 		$this->unConfigHandler->method('isSentAboutToExpireNotification')
 			->willReturn(true);
 
@@ -235,6 +247,64 @@ public function testRunExpiredAlreadySent() {
 		$this->unConfigHandler->method('isSentExpiredNotification')
 			->willReturn(true);
 
+		$this->userManager->method('userExists')
+			->with('usertest')
+			->willReturn(true);
+
+		$this->manager->expects($this->never())
+			->method('notify');
+
+		$this->invokePrivate($this->job, 'run', [[]]);
+	}
+
+	public function testRunAboutToExpireMissingUser() {
+		$this->unConfigHandler->method('getExpirationTime')
+			->willReturn(180);
+		$this->unConfigHandler->method('getExpirationTimeForNormalNotification')
+			->willReturn(120);
+
+		$baseTime = 1531232050;
+		$this->timeFactory->method('getTime')
+			->willReturn($baseTime + 150);
+
+		$returnedOldPassword = $this->getOldPassword('22', 'usertest', $baseTime);
+		$this->mapper->method('getPasswordsAboutToExpire')
+			->willReturn([$returnedOldPassword]);
+
+		$this->userManager->method('userExists')
+			->with('usertest')
+			->willReturn(false);
+
+		$this->unConfigHandler->method('isSentAboutToExpireNotification')
+			->willReturn(false);
+
+		$this->manager->expects($this->never())
+			->method('notify');
+
+		$this->invokePrivate($this->job, 'run', [[]]);
+	}
+
+	public function testRunExpiredMissingUser() {
+		$this->unConfigHandler->method('getExpirationTime')
+			->willReturn(180);
+		$this->unConfigHandler->method('getExpirationTimeForNormalNotification')
+			->willReturn(120);
+
+		$baseTime = 1531232050;
+		$this->timeFactory->method('getTime')
+			->willReturn($baseTime + 250);
+
+		$returnedOldPassword = $this->getOldPassword('22', 'usertest', $baseTime);
+		$this->mapper->method('getPasswordsAboutToExpire')
+			->willReturn([$returnedOldPassword]);
+
+		$this->unConfigHandler->method('isSentExpiredNotification')
+			->willReturn(false);
+
+		$this->userManager->method('userExists')
+			->with('usertest')
+			->willReturn(false);
+
 		$this->manager->expects($this->never())
 			->method('notify');
 
@@ -269,6 +339,10 @@ public function testRunAboutToExpire($password) {
 		$this->unConfigHandler->method('isSentAboutToExpireNotification')
 			->willReturn(false);
 
+		$this->userManager->method('userExists')
+			->with('usertest')
+			->willReturn(true);
+
 		$this->manager->expects($this->once())
 			->method('notify');
 		$this->unConfigHandler->expects($this->once())
@@ -298,6 +372,10 @@ public function testRunExpired($password) {
 		$this->unConfigHandler->method('isSentExpiredNotification')
 			->willReturn(false);
 
+		$this->userManager->method('userExists')
+			->with('usertest')
+			->willReturn(true);
+
 		$this->manager->expects($this->once())
 			->method('notify');
 		$this->unConfigHandler->expects($this->once())
@@ -324,6 +402,10 @@ public function testRunAboutToExpireNotConfigured() {
 		$this->unConfigHandler->method('isSentAboutToExpireNotification')
 			->willReturn(false);
 
+		$this->userManager->method('userExists')
+			->with('usertest')
+			->willReturn(true);
+
 		$this->manager->expects($this->never())
 			->method('notify');
 		$this->unConfigHandler->expects($this->never())