Make the RetryJob work on the userId only

To ensure there is only 1 background job per user

Signed-off-by: Joas Schilling <coding@schilljs.com>

apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php

@@ -1,6 +1,8 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016 Bjoern Schiessle <bjoern@schiessle.org>
+ * @copyright Copyright (c) 2019 Joas Schilling <coding@schilljs.com>
  *
  * @license GNU AGPL version 3 or any later version
  *
@@ -22,43 +24,55 @@
 namespace OCA\LookupServerConnector\BackgroundJobs;
 
 
+use OC\Security\IdentityProof\Signer;
+use OCP\Accounts\IAccountManager;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\Job;
 use OCP\BackgroundJob\IJobList;
 use OCP\Http\Client\IClientService;
 use OCP\IConfig;
 use OCP\ILogger;
+use OCP\IUser;
+use OCP\IUserManager;
 
 class RetryJob extends Job {
 	/** @var IClientService */
 	private $clientService;
-	/** @var IJobList */
-	private $jobList;
 	/** @var string */
 	private $lookupServer;
-	/** @var int how much time should be between two, will be increased for each retry */
-	private $interval = 100;
 	/** @var IConfig */
 	private $config;
+	/** @var IUserManager */
+	private $userManager;
+	/** @var IAccountManager */
+	private $accountManager;
+	/** @var Signer */
+	private $signer;
+	/** @var int */
+	protected $retries = 0;
+	/** @var bool */
+	protected $retainJob = false;
 
 	/**
 	 * @param ITimeFactory $time
 	 * @param IClientService $clientService
-	 * @param IJobList $jobList
 	 * @param IConfig $config
+	 * @param IUserManager $userManager
+	 * @param IAccountManager $accountManager
+	 * @param Signer $signer
 	 */
 	public function __construct(ITimeFactory $time,
 								IClientService $clientService,
-								IJobList $jobList,
-								IConfig $config) {
+								IConfig $config,
+								IUserManager $userManager,
+								IAccountManager $accountManager,
+								Signer $signer) {
 		parent::__construct($time);
 		$this->clientService = $clientService;
-		$this->jobList = $jobList;
 		$this->config = $config;
-
-		if ($config->getSystemValue('has_internet_connection', true) === false) {
-			return;
-		}
+		$this->userManager = $userManager;
+		$this->accountManager = $accountManager;
+		$this->signer = $signer;
 
 		$this->lookupServer = $config->getSystemValue('lookup_server', 'https://lookup.nextcloud.com');
 		if (!empty($this->lookupServer)) {
@@ -74,24 +88,66 @@ class RetryJob extends Job {
 	 * @param ILogger|null $logger
 	 */
 	public function execute($jobList, ILogger $logger = null): void {
-		if ($this->shouldRun($this->argument)) {
-			parent::execute($jobList, $logger);
+		if (!isset($this->argument['userId'])) {
+			// Old background job without user id, just drop it.
 			$jobList->remove($this, $this->argument);
+			return;
 		}
+
+		$this->retries = (int) $this->config->getUserValue($this->argument['userId'], 'lookup_server_connector', 'update_retries', 0);
+
+		if ($this->shouldRemoveBackgroundJob()) {
+			$jobList->remove($this, $this->argument);
+			return;
+		}
+
+		if ($this->shouldRun()) {
+			parent::execute($jobList, $logger);
+			if (!$this->retainJob) {
+				$jobList->remove($this, $this->argument);
+			}
+		}
+	}
+
+	/**
+	 * Check if we should kill the background job:
+	 *
+	 * - internet connection is disabled
+	 * - no valid lookup server URL given
+	 * - lookup server was disabled by the admin
+	 * - max retries are reached (set to 5)
+	 *
+	 * @return bool
+	 */
+	protected function shouldRemoveBackgroundJob(): bool {
+		return $this->config->getSystemValueBool('has_internet_connection', true) === false ||
+			$this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') === '' ||
+			$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'yes') !== 'yes' ||
+			$this->retries >= 5;
+	}
+
+	protected function shouldRun(): bool {
+		$delay = 100 * 6 ** $this->retries;
+		return ($this->time->getTime() - $this->lastRun) > $delay;
 	}
 
 	protected function run($argument): void {
-		if ($this->killBackgroundJob((int)$argument['retryNo'])) {
+		$user = $this->userManager->get($this->argument['userId']);
+		if (!$user instanceof IUser) {
+			// User does not exist anymore
 			return;
 		}
 
+		$data = $this->getUserAccountData($user);
+		$signedData = $this->signer->sign('lookupserver', $data, $user);
 		$client = $this->clientService->newClient();
 
 		try {
-			if (count($argument['dataArray']) === 1) {
+			if (count($data) === 1) {
+				// No public data, just the federation Id
 				$client->delete($this->lookupServer,
 					[
-						'body' => json_encode($argument['dataArray']),
+						'body' => json_encode($signedData),
 						'timeout' => 10,
 						'connect_timeout' => 3,
 					]
@@ -99,52 +155,58 @@ class RetryJob extends Job {
 			} else {
 				$client->post($this->lookupServer,
 					[
-						'body' => json_encode($argument['dataArray']),
+						'body' => json_encode($signedData),
 						'timeout' => 10,
 						'connect_timeout' => 3,
 					]
 				);
 			}
-		} catch (\Exception $e) {
-			$this->jobList->add(self::class,
-				[
-					'dataArray' => $argument['dataArray'],
-					'retryNo' => $argument['retryNo'] + 1,
-					'lastRun' => $this->time->getTime(),
-				]
+
+			// Reset retry counter
+			$this->config->deleteUserValue(
+				$user->getUID(),
+				'lookup_server_connector',
+				'update_retries'
 			);
 
+		} catch (\Exception $e) {
+			// An error occurred, retry later
+			$this->retainJob = true;
+			$this->config->setUserValue(
+				$user->getUID(),
+				'lookup_server_connector',
+				'update_retries',
+				$this->retries + 1
+			);
 		}
 	}
 
-	/**
-	 * test if it is time for the next run
-	 *
-	 * @param array $argument
-	 * @return bool
-	 */
-	protected function shouldRun(array $argument): bool {
-		$retryNo = (int)$argument['retryNo'];
-		$delay = $this->interval * 6 ** $retryNo;
-		return !isset($argument['lastRun']) || (($this->time->getTime() - $argument['lastRun']) > $delay);
-	}
+	protected function getUserAccountData(IUser $user): array {
+		$account = $this->accountManager->getAccount($user);
 
-	/**
-	 * check if we should kill the background job
-	 *
-	 * The lookup server should no longer be contacted if:
-	 *
-	 * - max retries are reached (set to 5)
-	 * - lookup server was disabled by the admin
-	 * - no valid lookup server URL given
-	 *
-	 * @param int $retryCount
-	 * @return bool
-	 */
-	protected function killBackgroundJob(int $retryCount): bool {
-		$maxTriesReached = $retryCount >= 5;
-		$lookupServerDisabled = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'yes') !== 'yes';
+		$publicData = [];
+		foreach ($account->getProperties() as $property) {
+			if ($property->getScope() === IAccountManager::VISIBILITY_PUBLIC) {
+				$publicData[$property->getName()] = $property->getValue();
+			}
+		}
+
+		$data = ['federationId' => $user->getCloudId()];
+		if (!empty($publicData)) {
+			$data['name'] = $publicData[IAccountManager::PROPERTY_DISPLAYNAME]['value'] ?? '';
+			$data['email'] = $publicData[IAccountManager::PROPERTY_EMAIL]['value'] ?? '';
+			$data['address'] = $publicData[IAccountManager::PROPERTY_ADDRESS]['value'] ?? '';
+			$data['website'] = $publicData[IAccountManager::PROPERTY_WEBSITE]['value'] ?? '';
+			$data['twitter'] = $publicData[IAccountManager::PROPERTY_TWITTER]['value'] ?? '';
+			$data['phone'] = $publicData[IAccountManager::PROPERTY_PHONE]['value'] ?? '';
+			$data['twitter_signature'] = $publicData[IAccountManager::PROPERTY_TWITTER]['signature'] ?? '';
+			$data['website_signature'] = $publicData[IAccountManager::PROPERTY_WEBSITE]['signature'] ?? '';
+			$data['verificationStatus'] = [
+				IAccountManager::PROPERTY_WEBSITE => $publicData[IAccountManager::PROPERTY_WEBSITE]['verified'] ?? '',
+				IAccountManager::PROPERTY_TWITTER => $publicData[IAccountManager::PROPERTY_TWITTER]['verified'] ?? '',
+			];
+		}
 
-		return $maxTriesReached || $lookupServerDisabled || empty($this->lookupServer);
+		return $data;
 	}
 }

apps/lookup_server_connector/lib/UpdateLookupServer.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016 Bjoern Schiessle <bjoern@schiessle.org>
  * @copyright Copyright (c) 2016 Lukas Reschke <lukas@statuscode.ch>
@@ -22,11 +23,8 @@
 
 namespace OCA\LookupServerConnector;
 
-use OC\Accounts\AccountManager;
-use OC\Security\IdentityProof\Signer;
 use OCA\LookupServerConnector\BackgroundJobs\RetryJob;
 use OCP\BackgroundJob\IJobList;
-use OCP\Http\Client\IClientService;
 use OCP\IConfig;
 use OCP\IUser;
 
@@ -36,109 +34,51 @@ use OCP\IUser;
  * @package OCA\LookupServerConnector
  */
 class UpdateLookupServer {
-	/** @var AccountManager */
-	private $accountManager;
-	/** @var Signer */
-	private $signer;
+	/** @var IConfig */
+	private $config;
 	/** @var IJobList */
 	private $jobList;
-	/** @var string URL point to lookup server */
-	private $lookupServer;
-	/** @var bool  */
-	private $lookupServerEnabled;
 
 	/**
-	 * @param AccountManager $accountManager
-	 * @param Signer $signer
 	 * @param IJobList $jobList
 	 * @param IConfig $config
 	 */
-	public function __construct(AccountManager $accountManager,
-								Signer $signer,
-								IJobList $jobList,
+	public function __construct(IJobList $jobList,
 								IConfig $config) {
-		$this->accountManager = $accountManager;
-		$this->signer = $signer;
+		$this->config = $config;
 		$this->jobList = $jobList;
-
-		if($config->getSystemValue('has_internet_connection', true) === false) {
-			return;
-		}
-
-		$this->lookupServerEnabled = $config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'yes') === 'yes';
-
-		$this->lookupServer = $config->getSystemValue('lookup_server', 'https://lookup.nextcloud.com');
-		if(!empty($this->lookupServer)) {
-			$this->lookupServer = rtrim($this->lookupServer, '/');
-			$this->lookupServer .= '/users';
-		}
 	}
 
 	/**
 	 * @param IUser $user
 	 */
 	public function userUpdated(IUser $user): void {
-
 		if (!$this->shouldUpdateLookupServer()) {
 			return;
 		}
 
-		$userData = $this->accountManager->getUser($user);
-		$publicData = [];
-
-		foreach ($userData as $key => $data) {
-			if ($data['scope'] === AccountManager::VISIBILITY_PUBLIC) {
-				$publicData[$key] = $data;
-			}
-		}
-
-		$this->sendToLookupServer($user, $publicData);
-	}
-
-	/**
-	 * send public user data to the lookup server
-	 *
-	 * @param IUser $user
-	 * @param array $publicData
-	 */
-	protected function sendToLookupServer(IUser $user, array $publicData): void {
-
-		$dataArray = ['federationId' => $user->getCloudId()];
-
-		if (!empty($publicData)) {
-			$dataArray['name'] = $publicData[AccountManager::PROPERTY_DISPLAYNAME]['value'] ?? '';
-			$dataArray['email'] = $publicData[AccountManager::PROPERTY_EMAIL]['value'] ?? '';
-			$dataArray['address'] = $publicData[AccountManager::PROPERTY_ADDRESS]['value'] ?? '';
-			$dataArray['website'] = $publicData[AccountManager::PROPERTY_WEBSITE]['value'] ?? '';
-			$dataArray['twitter'] = $publicData[AccountManager::PROPERTY_TWITTER]['value'] ?? '';
-			$dataArray['phone'] = $publicData[AccountManager::PROPERTY_PHONE]['value'] ?? '';
-			$dataArray['twitter_signature'] = $publicData[AccountManager::PROPERTY_TWITTER]['signature'] ?? '';
-			$dataArray['website_signature'] = $publicData[AccountManager::PROPERTY_WEBSITE]['signature'] ?? '';
-			$dataArray['verificationStatus'] = [
-				AccountManager::PROPERTY_WEBSITE => $publicData[AccountManager::PROPERTY_WEBSITE]['verified'] ?? '',
-				AccountManager::PROPERTY_TWITTER => $publicData[AccountManager::PROPERTY_TWITTER]['verified'] ?? '',
-			];
-		}
-
-		$dataArray = $this->signer->sign('lookupserver', $dataArray, $user);
-		$this->jobList->add(RetryJob::class,
-			[
-				'dataArray' => $dataArray,
-				'retryNo' => 0,
-			]
+		// Reset retry counter
+		$this->config->deleteUserValue(
+			$user->getUID(),
+			'lookup_server_connector',
+			'update_retries'
 		);
+		$this->jobList->add(RetryJob::class, ['userId' => $user->getUID()]);
 	}
 
 	/**
 	 * check if we should update the lookup server, we only do it if
 	 *
-	 * * we have a valid URL
-	 * * the lookup server update was enabled by the admin
+	 * + we have an internet connection
+	 * + the lookup server update was not disabled by the admin
+	 * + we have a valid lookup server URL
 	 *
 	 * @return bool
 	 */
 	private function shouldUpdateLookupServer(): bool {
-		return $this->lookupServerEnabled && !empty($this->lookupServer);
+		return $this->config->getSystemValueBool('has_internet_connection', true) === true &&
+			$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'yes') === 'yes' &&
+			$this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') !== '';
 	}
 
 }