Merge pull request #53760 from nextcloud/bug/noid/federation-background-job-same-url-different-token

fix(federation): remove background jobs when removing trusted servers

apps/federation/composer/composer/autoload_classmap.php

@@ -16,6 +16,7 @@ return array(
     'OCA\\Federation\\DAV\\FedAuth' => $baseDir . '/../lib/DAV/FedAuth.php',
     'OCA\\Federation\\DbHandler' => $baseDir . '/../lib/DbHandler.php',
     'OCA\\Federation\\Listener\\SabrePluginAuthInitListener' => $baseDir . '/../lib/Listener/SabrePluginAuthInitListener.php',
+    'OCA\\Federation\\Listener\\TrustedServerRemovedListener' => $baseDir . '/../lib/Listener/TrustedServerRemovedListener.php',
     'OCA\\Federation\\Migration\\Version1010Date20200630191302' => $baseDir . '/../lib/Migration/Version1010Date20200630191302.php',
     'OCA\\Federation\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php',
     'OCA\\Federation\\SyncFederationAddressBooks' => $baseDir . '/../lib/SyncFederationAddressBooks.php',

apps/federation/composer/composer/autoload_static.php

@@ -31,6 +31,7 @@ class ComposerStaticInitFederation
         'OCA\\Federation\\DAV\\FedAuth' => __DIR__ . '/..' . '/../lib/DAV/FedAuth.php',
         'OCA\\Federation\\DbHandler' => __DIR__ . '/..' . '/../lib/DbHandler.php',
         'OCA\\Federation\\Listener\\SabrePluginAuthInitListener' => __DIR__ . '/..' . '/../lib/Listener/SabrePluginAuthInitListener.php',
+        'OCA\\Federation\\Listener\\TrustedServerRemovedListener' => __DIR__ . '/..' . '/../lib/Listener/TrustedServerRemovedListener.php',
         'OCA\\Federation\\Migration\\Version1010Date20200630191302' => __DIR__ . '/..' . '/../lib/Migration/Version1010Date20200630191302.php',
         'OCA\\Federation\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php',
         'OCA\\Federation\\SyncFederationAddressBooks' => __DIR__ . '/..' . '/../lib/SyncFederationAddressBooks.php',

apps/federation/lib/AppInfo/Application.php

@@ -9,10 +9,12 @@ namespace OCA\Federation\AppInfo;
 
 use OCA\DAV\Events\SabrePluginAuthInitEvent;
 use OCA\Federation\Listener\SabrePluginAuthInitListener;
+use OCA\Federation\Listener\TrustedServerRemovedListener;
 use OCP\AppFramework\App;
 use OCP\AppFramework\Bootstrap\IBootContext;
 use OCP\AppFramework\Bootstrap\IBootstrap;
 use OCP\AppFramework\Bootstrap\IRegistrationContext;
+use OCP\Federation\Events\TrustedServerRemovedEvent;
 
 class Application extends App implements IBootstrap {
 
@@ -25,6 +27,7 @@ class Application extends App implements IBootstrap {
 
 	public function register(IRegistrationContext $context): void {
 		$context->registerEventListener(SabrePluginAuthInitEvent::class, SabrePluginAuthInitListener::class);
+		$context->registerEventListener(TrustedServerRemovedEvent::class, TrustedServerRemovedListener::class);
 	}
 
 	public function boot(IBootContext $context): void {

apps/federation/lib/Listener/TrustedServerRemovedListener.php

@@ -0,0 +1,57 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Federation\Listener;
+
+use OCA\Federation\BackgroundJob\GetSharedSecret;
+use OCA\Federation\BackgroundJob\RequestSharedSecret;
+use OCP\BackgroundJob\IJobList;
+use OCP\EventDispatcher\Event;
+use OCP\EventDispatcher\IEventListener;
+use OCP\Federation\Events\TrustedServerRemovedEvent;
+
+/** @template-implements IEventListener<TrustedServerRemovedEvent> */
+class TrustedServerRemovedListener implements IEventListener {
+	public function __construct(
+		private readonly IJobList $jobList,
+	) {
+	}
+
+	public function handle(Event $event): void {
+		if (!$event instanceof TrustedServerRemovedEvent) {
+			return;
+		}
+
+		if ($event->getUrl() === null) {
+			return; // safe guard
+		}
+
+		$this->removeJobsByUrl(RequestSharedSecret::class, $event->getUrl());
+		$this->removeJobsByUrl(GetSharedSecret::class, $event->getUrl());
+	}
+
+	/**
+	 * Remove RequestSharedSecret or GetSharedSecret jobs from the job list by their URL.
+	 * The jobs are scheduled with url, token, and created as arguments.
+	 * Thus, we have to loop over the jobs here and cannot use IJobList.remove.
+	 */
+	private function removeJobsByUrl(string $class, string $url): void {
+		foreach ($this->jobList->getJobsIterator($class, null, 0) as $job) {
+			$arguments = $job->getArgument();
+			if (isset($arguments['url']) && $arguments['url'] === $url) {
+				try {
+					$this->jobList->removeById($job->getId());
+				} catch (\Exception) {
+					// Removing the background jobs is optional because they will expire sometime.
+					// Therefore, we are using catch and ignore.
+				}
+			}
+		}
+	}
+}

apps/federation/lib/TrustedServers.php

@@ -88,7 +88,7 @@ class TrustedServers {
 	public function removeServer(int $id): void {
 		$server = $this->dbHandler->getServerById($id);
 		$this->dbHandler->removeServer($id);
-		$this->dispatcher->dispatchTyped(new TrustedServerRemovedEvent($server['url_hash']));
+		$this->dispatcher->dispatchTyped(new TrustedServerRemovedEvent($server['url_hash'], $server['url']));
 
 	}
 

apps/federation/tests/Listener/TrustedServerRemovedListenerTest.php

@@ -0,0 +1,71 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Federation\Tests\Listener;
+
+use OC\AppFramework\Utility\TimeFactory;
+use OCA\Federation\BackgroundJob\GetSharedSecret;
+use OCA\Federation\Listener\TrustedServerRemovedListener;
+use OCA\Federation\TrustedServers;
+use OCP\BackgroundJob\IJobList;
+use OCP\Federation\Events\TrustedServerRemovedEvent;
+use OCP\Http\Client\IClientService;
+use OCP\IConfig;
+use OCP\IURLGenerator;
+use OCP\OCS\IDiscoveryService;
+use Psr\Log\NullLogger;
+use Test\BackgroundJob\DummyJobList;
+use Test\TestCase;
+
+class TrustedServerRemovedListenerTest extends TestCase {
+
+	private IJobList $jobList;
+	private TrustedServerRemovedListener $listener;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->jobList = new DummyJobList();
+		$this->listener = new TrustedServerRemovedListener($this->jobList);
+	}
+
+	public function testHandle(): void {
+		// Arrange
+		$url = 'https://example.com';
+		$event = new TrustedServerRemovedEvent(md5($url), $url); // we are using a different hashing in the tests.
+		$job1 = $this->createGetSharedSecretMock();
+		$job2 = $this->createGetSharedSecretMock();
+		$job3 = $this->createGetSharedSecretMock();
+		$job4 = $this->createGetSharedSecretMock();
+		$this->jobList->add($job1, ['url' => 'https://example.org', 'token' => 'nei0dooX', 'created' => 0]);
+		$this->jobList->add($job2, ['url' => 'https://example.net', 'token' => 'ci6Shah7', 'created' => 0]);
+		$this->jobList->add($job3, ['url' => $url, 'token' => 'ieXie6Me', 'created' => 0]);
+		$this->jobList->add($job4, ['url' => $url, 'token' => 'thoQu8th', 'created' => 0]);
+
+		// Act
+		$this->listener->handle($event);
+		$jobs = iterator_to_array($this->jobList->getJobsIterator(GetSharedSecret::class, null, 0), false);
+
+		// Assert
+		$this->assertCount(2, $jobs);
+	}
+
+	private function createGetSharedSecretMock(): GetSharedSecret {
+		return new GetSharedSecret(
+			$this->createMock(IClientService::class),
+			$this->createMock(IURLGenerator::class),
+			$this->jobList,
+			$this->createMock(TrustedServers::class),
+			new NullLogger(),
+			$this->createMock(IDiscoveryService::class),
+			new TimeFactory(),
+			$this->createMock(IConfig::class),
+		);
+	}
+}

apps/federation/tests/TrustedServersTest.php

@@ -120,15 +120,15 @@ class TrustedServersTest extends TestCase {
 
 	public function testRemoveServer(): void {
 		$id = 42;
-		$server = ['url_hash' => 'url_hash'];
+		$server = ['url' => 'url', 'url_hash' => 'url_hash'];
 		$this->dbHandler->expects($this->once())->method('removeServer')->with($id);
 		$this->dbHandler->expects($this->once())->method('getServerById')->with($id)
 			->willReturn($server);
 		$this->dispatcher->expects($this->once())->method('dispatchTyped')
 			->willReturnCallback(
 				function ($event): void {
-					$this->assertSame(get_class($event), TrustedServerRemovedEvent::class);
-					/** @var \OCP\Federated\Events\TrustedServerRemovedEvent $event */
+					$this->assertInstanceOf(TrustedServerRemovedEvent::class, $event);
+					$this->assertSame('url', $event->getUrl());
 					$this->assertSame('url_hash', $event->getUrlHash());
 				}
 			);

lib/public/Federation/Events/TrustedServerRemovedEvent.php

@@ -14,14 +14,16 @@ use OCP\EventDispatcher\Event;
  * @since 25.0.0
  */
 class TrustedServerRemovedEvent extends Event {
-	private string $urlHash;
 
 	/**
 	 * @since 25.0.0
+	 * @since 32.0.0 Added $url argument
 	 */
-	public function __construct(string $urlHash) {
+	public function __construct(
+		private readonly string $urlHash,
+		private readonly ?string $url = null,
+	) {
 		parent::__construct();
-		$this->urlHash = $urlHash;
 	}
 
 	/**
@@ -30,4 +32,11 @@ class TrustedServerRemovedEvent extends Event {
 	public function getUrlHash(): string {
 		return $this->urlHash;
 	}
+
+	/**
+	 * @since 32.0.0
+	 */
+	public function getUrl(): ?string {
+		return $this->url;
+	}
 }

tests/lib/BackgroundJob/DummyJobList.php

@@ -8,6 +8,7 @@
 
 namespace Test\BackgroundJob;
 
+use ArrayIterator;
 use OC\BackgroundJob\JobList;
 use OCP\BackgroundJob\IJob;
 use OCP\BackgroundJob\Job;
@@ -98,20 +99,23 @@ class DummyJobList extends JobList {
 		return $this->jobs;
 	}
 
-	public function getJobsIterator($job, ?int $limit, int $offset): array {
+	public function getJobsIterator($job, ?int $limit, int $offset): iterable {
 		if ($job instanceof IJob) {
 			$jobClass = get_class($job);
 		} else {
 			$jobClass = $job;
 		}
-		return array_slice(
+
+		$jobs = array_slice(
 			array_filter(
 				$this->jobs,
-				fn ($job) => ($jobClass === null) || (get_class($job) == $jobClass)
+				fn ($job) => ($jobClass === null) || (get_class($job) === $jobClass)
 			),
 			$offset,
 			$limit
 		);
+
+		return new ArrayIterator($jobs);
 	}
 
 	/**