fix credential notification handling

js/app/controllers/edit_credential.js

@@ -104,17 +104,23 @@
 				var storedCredential = SettingsService.getSetting('edit_credential');
 
 				if (!storedCredential) {
+					$scope.storedCredential = {}; // this line is required for reactive model binding to update the value from the async callback
 					CredentialService.getCredential($routeParams.credential_id).then(function (result) {
 						$scope.storedCredential = CredentialService.decryptCredential(angular.copy(result));
+						$scope.storedCredential.password_repeat = angular.copy($scope.storedCredential.password);
+						$scope.storedCredential.expire_time = $scope.storedCredential.expire_time * 1000;
+
+						//store password to check if it was changed if this credential has been compromised
+						$scope.oldPassword = $scope.storedCredential.password;
 					});
 				} else {
 					$scope.storedCredential = CredentialService.decryptCredential(angular.copy(storedCredential));
 					$scope.storedCredential.password_repeat = angular.copy($scope.storedCredential.password);
 					$scope.storedCredential.expire_time = $scope.storedCredential.expire_time * 1000;
-				}
 
-				//store password to check if it was changed if this credential has been compromised
-				$scope.oldPassword=$scope.storedCredential.password;
+					//store password to check if it was changed if this credential has been compromised
+					$scope.oldPassword = $scope.storedCredential.password;
+				}
 
 				$scope.getTags = function ($query) {
 					return TagService.searchTag($query);

lib/Controller/InternalController.php

@@ -12,12 +12,13 @@
 namespace OCA\Passman\Controller;
 
 use OCA\Passman\Service\CredentialService;
+use OCA\Passman\Service\NotificationService;
 use OCP\App\IAppManager;
 use OCP\AppFramework\ApiController;
+use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\IConfig;
 use OCP\IRequest;
-use OCP\Notification\IManager;
 
 class InternalController extends ApiController {
 	private $userId;
@@ -27,8 +28,8 @@ public function __construct(
 		IRequest $request,
 		$UserId,
 		private CredentialService $credentialService,
+		private NotificationService $notificationService,
 		private IConfig $config,
-		private IManager $manager,
 		private IAppManager $appManager,
 	) {
 		parent::__construct(
@@ -49,28 +50,31 @@ public function remind($credential_id) {
 			$credential->setExpireTime(time() + (24 * 60 * 60));
 			$this->credentialService->upd($credential);
 
-			$notification = $this->manager->createNotification();
-			$notification->setApp('passman')
-				->setObject('credential', $credential_id)
-				->setUser($this->userId);
-			$this->manager->markProcessed($notification);
+			$this->notificationService->markNotificationOfCredentialAsProcessed($credential_id, $this->userId);
 		}
 	}
 
 	/**
 	 * @NoAdminRequired
 	 */
 	public function read($credential_id) {
+		try {
+			// need to check overall credential existence before, since getCredentialById() method call below throws a
+			// DoesNotExistException in two different cases, that we cannot differentiate in retrospect
+			$this->credentialService->credentialExistsById($credential_id);
+		} catch (DoesNotExistException) {
+			// got DoesNotExistException from CredentialMapper, means the credential does not even exist for any user,
+			// so we can also delete or mark the corresponding notification message as processed
+			$this->notificationService->markNotificationOfCredentialAsProcessed($credential_id, $this->userId);
+			return;
+		}
+
 		$credential = $this->credentialService->getCredentialById($credential_id, $this->userId);
 		if ($credential) {
 			$credential->setExpireTime(0);
 			$this->credentialService->upd($credential);
 
-			$notification = $this->manager->createNotification();
-			$notification->setApp('passman')
-				->setObject('credential', $credential_id)
-				->setUser($this->userId);
-			$this->manager->markProcessed($notification);
+			$this->notificationService->markNotificationOfCredentialAsProcessed($credential_id, $this->userId);
 		}
 	}
 

lib/Service/CredentialService.php

@@ -32,6 +32,7 @@
 use OCP\AppFramework\Db\Entity;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\IConfig;
+use OCP\IURLGenerator;
 
 
 class CredentialService {
@@ -45,6 +46,9 @@ public function __construct(
 		private ShareService              $shareService,
 		private EncryptService            $encryptService,
 		private CredentialRevisionService $credentialRevisionService,
+		private IURLGenerator             $urlGenerator,
+		private VaultService              $vaultService,
+		private NotificationService       $notificationService,
 		IConfig                           $config,
 	) {
 		$this->server_key = $config->getSystemValue('passwordsalt', '');
@@ -117,6 +121,7 @@ public function deleteCredentialParts(Credential $credential, $userId) {
 				$this->credentialRevisionService->deleteRevision($id, $userId);
 			}
 		}
+		$this->notificationService->deleteNotificationsOfCredential($credential);
 	}
 
 	/**
@@ -188,6 +193,18 @@ public function getCredentialById(int $credential_id, ?string $user_id) {
 		}
 	}
 
+	/**
+	 * Check if a credential exists by id.
+	 *
+	 * @param int $credential_id
+	 * @return bool
+	 * @throws DoesNotExistException
+	 * @throws MultipleObjectsReturnedException
+	 */
+	public function credentialExistsById(int $credential_id): bool {
+		return $this->credentialMapper->getCredentialById($credential_id) !== null;
+	}
+
 	/**
 	 * Get credential label by credential id.
 	 *
@@ -214,4 +231,14 @@ public function getCredentialByGUID(string $credential_guid, string $user_id = n
 		$credential = $this->credentialMapper->getCredentialByGUID($credential_guid, $user_id);
 		return $this->encryptService->decryptCredential($credential);
 	}
+
+	public function getDirectEditLink(Credential $credential): string {
+		$vaults = $this->vaultService->getById($credential->getVaultId(), $credential->getUserId());
+		return $this->urlGenerator->getAbsoluteURL(
+			$this->urlGenerator->linkTo(
+				'',
+				'index.php/apps/passman/#/vault/' . $vaults[0]->getGuid() . '/edit/' . $credential->getGuid()
+			)
+		);
+	}
 }

lib/Service/CronService.php

@@ -24,10 +24,9 @@
 namespace OCA\Passman\Service;
 
 use OCA\Passman\Activity;
+use OCA\Passman\Db\Credential;
 use OCA\Passman\Utility\Utils;
 use OCP\DB\Exception;
-use OCP\DB\QueryBuilder\IQueryBuilder;
-use OCP\IDBConnection;
 use Psr\Log\LoggerInterface;
 
 class CronService {
@@ -38,30 +37,23 @@ public function __construct(
 		private Utils               $utils,
 		private NotificationService $notificationService,
 		private ActivityService     $activityService,
-		private IDBConnection       $db,
 	) {
 	}
 
 	public function expireCredentials() {
+		/** @var Credential[] $expired_credentials */
 		$expired_credentials = $this->credentialService->getExpiredCredentials($this->utils->getTime());
 		foreach ($expired_credentials as $credential) {
-			$link = ''; // @TODO create direct link to credential
-			$qb = $this->db->getQueryBuilder();
-			$qb->select('*')
-				->from('notifications')
-				->where($qb->expr()->eq('object_id', $qb->createNamedParameter($credential->getId(), IQueryBuilder::PARAM_INT)))
-				->andWhere($qb->expr()->eq('subject', $qb->createNamedParameter('credential_expired', IQueryBuilder::PARAM_STR)));
-
 			try {
 				$this->logger->debug($credential->getLabel() . ' is expired, checking notifications!', ['app' => 'passman']);
-				$notificationCount = $qb->execute()->rowCount();
-				if ($notificationCount === 0) {
+				if (!$this->notificationService->hasCredentialExpirationNotification($credential)) {
+				$link = $this->credentialService->getDirectEditLink($credential);
 					$this->logger->debug($credential->getLabel() . ' is expired, adding notification!', ['app' => 'passman']);
 					$this->activityService->add(
 						Activity::SUBJECT_ITEM_EXPIRED, [$credential->getLabel(), $credential->getUserId()],
 						'', [],
 						$link, $credential->getUserId(), Activity::TYPE_ITEM_EXPIRED);
-					$this->notificationService->credentialExpiredNotification($credential);
+					$this->notificationService->credentialExpiredNotification($credential, $link);
 				}
 			} catch (Exception $exception) {
 				$this->logger->error('Error while creating a notification: ' . $exception->getMessage(), ['app' => 'passman']);

lib/Service/NotificationService.php

@@ -24,18 +24,20 @@
 namespace OCA\Passman\Service;
 
 
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IDBConnection;
 use OCP\IURLGenerator;
 use OCP\Notification\IManager;
 
 class NotificationService {
 	public function __construct(
 		private IManager $manager,
 		private IURLGenerator $urlGenerator,
+		private IDBConnection $db,
 	) {
 	}
 
-	function credentialExpiredNotification($credential) {
-		$link = $this->urlGenerator->getAbsoluteURL($this->urlGenerator->linkTo('', 'index.php/apps/passman/#/vault/' . $credential->getVaultId() . '/edit/' . $credential->getId()));
+	function credentialExpiredNotification($credential, $link) {
 		$api = $this->urlGenerator->getAbsoluteURL($this->urlGenerator->linkTo('', 'index.php/apps/passman'));
 		$notification = $this->manager->createNotification();
 		$remindAction = $notification->createAction();
@@ -101,4 +103,29 @@ function credentialAcceptedSharedNotification($data) {
 		$this->manager->notify($notification);
 	}
 
+	function hasCredentialExpirationNotification($credential): bool {
+		$qb = $this->db->getQueryBuilder();
+		$qb->select('*')
+			->from('notifications')
+			->where($qb->expr()->eq('object_id', $qb->createNamedParameter($credential->getId(), IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->eq('subject', $qb->createNamedParameter('credential_expired', IQueryBuilder::PARAM_STR)));
+			return $qb->execute()->rowCount() !== 0;
+	}
+
+	function deleteNotificationsOfCredential($credential) {
+		$qb = $this->db->getQueryBuilder();
+		$qb->delete()
+			->from('notifications')
+			->where($qb->expr()->eq('object_id', $qb->createNamedParameter($credential->getId(), IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->eq('object_type', 'credential'));
+		return $qb->execute();
+	}
+
+	function markNotificationOfCredentialAsProcessed(int $credential_id, string $user_id): void {
+		$notification = $this->manager->createNotification();
+		$notification->setApp('passman')
+			->setObject('credential', $credential_id)
+			->setUser($user_id);
+		$this->manager->markProcessed($notification);
+	}
 }