refactor: convert device data to a DTO

st3iny · st3iny · commit 8d8c6bbdabcd · 2025-09-04T13:13:55.000+02:00
Signed-off-by: Richard Steinmetz &lt;richard@steinmetz.cloud&gt;
diff --git a/lib/Db/PublicKeyCredentialEntity.php b/lib/Db/PublicKeyCredentialEntity.php
@@ -121,6 +121,7 @@ public static function fromPublicKeyCrendentialSource(string $name,
 		$publicKeyCredentialEntity->setUserHandle($publicKeyCredentialSource->getUserHandle());
 		$publicKeyCredentialEntity->setCounter($publicKeyCredentialSource->getCounter());
 		$publicKeyCredentialEntity->setCreatedAt($ctime);
+		$publicKeyCredentialEntity->setActive(true);
 
 		return $publicKeyCredentialEntity;
 	}
diff --git a/lib/Listener/StateChangeRegistryUpdater.php b/lib/Listener/StateChangeRegistryUpdater.php
@@ -10,6 +10,7 @@
 namespace OCA\TwoFactorWebauthn\Listener;
 
 use OCA\TwoFactorWebauthn\Event\StateChanged;
+use OCA\TwoFactorWebauthn\Model\Device;
 use OCA\TwoFactorWebauthn\Provider\WebAuthnProvider;
 use OCA\TwoFactorWebauthn\Service\WebAuthnManager;
 use OCP\Authentication\TwoFactorAuth\IRegistry;
@@ -38,13 +39,15 @@ public function __construct(IRegistry $providerRegistry, WebAuthnManager $manage
 
 	public function handle(Event $event): void {
 		if ($event instanceof StateChanged) {
-			$devices = array_filter($this->manager->getDevices($event->getUser()), function ($device) {
-				return $device['active'] === true;
-			});
-			if ($event->isEnabled() && count($devices) > 0) {
+			$devices = array_filter(
+				$this->manager->getDevices($event->getUser()),
+				static fn (Device $device) => $device->isActive(),
+			);
+			$hasDevices = !empty($devices);
+			if ($hasDevices && $event->isEnabled()) {
 				// The first device was enabled -> enable provider for this user
 				$this->providerRegistry->enableProviderFor($this->provider, $event->getUser());
-			} elseif (!$event->isEnabled() && empty($devices)) {
+			} elseif (!$hasDevices && !$event->isEnabled()) {
 				// The last device was removed -> disable provider for this user
 				$this->providerRegistry->disableProviderFor($this->provider, $event->getUser());
 			}
diff --git a/lib/Model/Device.php b/lib/Model/Device.php
@@ -0,0 +1,64 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\TwoFactorWebauthn\Model;
+
+use OCA\TwoFactorWebauthn\Db\PublicKeyCredentialEntity;
+
+final class Device implements \JsonSerializable {
+	public function __construct(
+		private readonly int $entityId,
+		private readonly string $id,
+		private readonly string $name,
+		private readonly ?int $createdAt,
+		private readonly bool $active,
+	) {
+	}
+
+	public static function fromPublicKeyCredentialEntity(PublicKeyCredentialEntity $entity): self {
+		return new self(
+			$entity->getId(),
+			$entity->getPublicKeyCredentialId(),
+			$entity->getName(),
+			$entity->getCreatedAt(),
+			$entity->isActive() === true,
+		);
+	}
+
+	public function getEntityId(): int {
+		return $this->entityId;
+	}
+
+	public function getId(): string {
+		return $this->id;
+	}
+
+	public function getName(): string {
+		return $this->name;
+	}
+
+	public function getCreatedAt(): ?int {
+		return $this->createdAt;
+	}
+
+	public function isActive(): bool {
+		return $this->active;
+	}
+
+	#[\ReturnTypeWillChange]
+	public function jsonSerialize() {
+		return [
+			'entityId' => $this->getEntityId(),
+			'id' => $this->getId(),
+			'name' => $this->getName(),
+			'createdAt' => $this->getCreatedAt(),
+			'active' => $this->isActive(),
+		];
+	}
+}
diff --git a/lib/Provider/WebAuthnProvider.php b/lib/Provider/WebAuthnProvider.php
@@ -10,6 +10,7 @@
 namespace OCA\TwoFactorWebauthn\Provider;
 
 use OCA\TwoFactorWebauthn\AppInfo\Application;
+use OCA\TwoFactorWebauthn\Model\Device;
 use OCA\TwoFactorWebauthn\Service\WebAuthnManager;
 use OCA\TwoFactorWebauthn\Settings\Personal;
 use OCP\AppFramework\Services\IInitialState;
@@ -104,9 +105,10 @@ public function verifyChallenge(IUser $user, string $challenge): bool {
 	 * Decides whether 2FA is enabled for the given user
 	 */
 	public function isTwoFactorAuthEnabledForUser(IUser $user): bool {
-		$devices = array_filter($this->manager->getDevices($user), function ($device) {
-			return $device['active'] === true;
-		});
+		$devices = array_filter(
+			$this->manager->getDevices($user),
+			static fn (Device $device) => $device->isActive(),
+		);
 		return count($devices) > 0;
 	}
 
diff --git a/lib/Service/WebAuthnManager.php b/lib/Service/WebAuthnManager.php
@@ -20,7 +20,9 @@
 use OCA\TwoFactorWebauthn\Db\PublicKeyCredentialEntityMapper;
 use OCA\TwoFactorWebauthn\Event\DisabledByAdmin;
 use OCA\TwoFactorWebauthn\Event\StateChanged;
+use OCA\TwoFactorWebauthn\Model\Device;
 use OCA\TwoFactorWebauthn\Repository\WebauthnPublicKeyCredentialSourceRepository;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\ISession;
 use OCP\IUser;
@@ -78,6 +80,7 @@ public function __construct(
 		PublicKeyCredentialEntityMapper $mapper,
 		IEventDispatcher $eventDispatcher,
 		LoggerInterface $logger,
+		private readonly ITimeFactory $time,
 	) {
 		$this->session = $session;
 		$this->repository = $repository;
@@ -169,7 +172,7 @@ public function buildPublicKeyCredentialLoader(AttestationStatementSupportManage
 		return $publicKeyCredentialLoader;
 	}
 
-	public function finishRegister(IUser $user, string $name, $data): array {
+	public function finishRegister(IUser $user, string $name, $data): Device {
 		if (!$this->session->exists(self::TWOFACTORAUTH_WEBAUTHN_REGISTRATION)) {
 			throw new Exception('Twofactor Webauthn registration process was not properly initialized');
 		}
@@ -212,30 +215,24 @@ public function finishRegister(IUser $user, string $name, $data): array {
 			$request
 		);
 
-		$this->repository->saveCredentialSource($publicKeyCredentialSource, $name);
-		$entity = $this->mapper->findPublicKeyCredential(base64_encode($publicKeyCredentialSource->getPublicKeyCredentialId()));
+		$entity = PublicKeyCredentialEntity::fromPublicKeyCrendentialSource(
+			$name,
+			$publicKeyCredentialSource,
+			$this->time->getTime(),
+		);
+		$entity = $this->mapper->insert($entity);
 		$this->eventDispatcher->dispatch(StateChanged::class, new StateChanged($user, true));
 
-		return [
-			'entityId' => $entity->getId(),
-			'id' => base64_encode($publicKeyCredentialSource->getPublicKeyCredentialId()),
-			'name' => $name,
-			'createdAt' => $entity !== null ? $entity->getCreatedAt() : null,
-			'active' => true,
-		];
+		return Device::fromPublicKeyCredentialEntity($entity);
 	}
 
+	/**
+	 * @param IUser $user
+	 * @return Device[]
+	 */
 	public function getDevices(IUser $user): array {
 		$credentials = $this->mapper->findPublicKeyCredentials($user->getUID());
-		return array_map(function (PublicKeyCredentialEntity $credential) {
-			return [
-				'entityId' => $credential->getId(),
-				'id' => $credential->getPublicKeyCredentialId(),
-				'name' => $credential->getName(),
-				'createdAt' => $credential->getCreatedAt(),
-				'active' => ($credential->isActive() === true)
-			];
-		}, $credentials);
+		return array_map(Device::fromPublicKeyCredentialEntity(...), $credentials);
 	}
 
 	private function stripPort(string $serverHost): string {
diff --git a/tests/Unit/Controller/SettingsControllerTest.php b/tests/Unit/Controller/SettingsControllerTest.php
@@ -10,6 +10,7 @@
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
 use OCA\TwoFactorWebauthn\Controller\SettingsController;
+use OCA\TwoFactorWebauthn\Model\Device;
 use OCA\TwoFactorWebauthn\Service\WebAuthnManager;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\IRequest;
@@ -71,16 +72,17 @@ public function testFinishRegister(): void {
 			->willReturn($user);
 		$data = 'some data';
 
+		$device = new Device(1, 'key-1', 'my key', null, true);
 		$this->webauthnManager->expects(self::once())
 			->method('finishRegister')
 			->with(
 				self::equalTo($user),
 				self::equalTo('my key'),
 				self::equalTo($data))
-			->willReturn([]);
+			->willReturn($device);
 
 		$resp = $this->controller->finishRegister('my key', $data);
 
-		self::assertEquals(new JSONResponse([]), $resp);
+		self::assertEquals(new JSONResponse($device), $resp);
 	}
 }
diff --git a/tests/Unit/Listener/StateChangeRegistryUpdaterTest.php b/tests/Unit/Listener/StateChangeRegistryUpdaterTest.php
@@ -11,6 +11,7 @@
 
 use OCA\TwoFactorWebauthn\Event\StateChanged;
 use OCA\TwoFactorWebauthn\Listener\StateChangeRegistryUpdater;
+use OCA\TwoFactorWebauthn\Model\Device;
 use OCA\TwoFactorWebauthn\Provider\WebAuthnProvider;
 use OCA\TwoFactorWebauthn\Service\WebAuthnManager;
 use OCP\Authentication\TwoFactorAuth\IRegistry;
@@ -59,11 +60,7 @@ public function testHandleEnableFirstDevice(): void {
 		$this->manager->expects(self::once())
 			->method('getDevices')
 			->willReturn([
-				[
-					'id' => 1,
-					'name' => 'utf1',
-					'active' => true,
-				],
+				new Device(1, 'credential-id-1', 'utf2', null, true),
 			]);
 		$this->providerRegistry->expects(self::once())
 			->method('enableProviderFor')
@@ -97,11 +94,7 @@ public function testHandleDisableWithRemainingDevices(): void {
 		$this->manager->expects(self::once())
 			->method('getDevices')
 			->willReturn([
-				[
-					'id' => 2,
-					'name' => 'utf2',
-					'active' => true,
-				],
+				new Device(2, 'credential-id-2', 'utf2', null, true),
 			]);
 		$this->providerRegistry->expects(self::never())
 			->method('disableProviderFor')
diff --git a/tests/Unit/Provider/WebAuthnProviderTest.php b/tests/Unit/Provider/WebAuthnProviderTest.php
@@ -9,6 +9,7 @@
 
 namespace OCA\TwoFactorWebauthn\Tests\Unit\Provider;
 
+use OCA\TwoFactorWebauthn\Model\Device;
 use OCA\TwoFactorWebauthn\Provider\WebAuthnLoginProvider;
 use OCA\TwoFactorWebauthn\Provider\WebAuthnProvider;
 use OCA\TwoFactorWebauthn\Service\WebAuthnManager;
@@ -129,11 +130,7 @@ public function testIsTwoFactorAuthEnabledForUser(): void {
 		$this->manager->expects(self::once())
 			->method('getDevices')
 			->willReturn([
-				[
-					'id' => 'k1',
-					'name' => 'n1',
-					'active' => true,
-				],
+				new Device(1, 'k1', 'n1', null, true),
 			]);
 
 		self::assertTrue($this->provider->isTwoFactorAuthEnabledForUser($user));
@@ -144,11 +141,7 @@ public function testIsTwoFactorAuthDisabledForUserBecauseDisabledDevice(): void
 		$this->manager->expects(self::once())
 			->method('getDevices')
 			->willReturn([
-				[
-					'id' => 'k1',
-					'name' => 'n1',
-					'active' => false,
-				],
+				new Device(1, 'k1', 'n1', null, false),
 			]);
 
 		self::assertFalse($this->provider->isTwoFactorAuthEnabledForUser($user));
diff --git a/tests/Unit/Service/WebAuthnManagerTest.php b/tests/Unit/Service/WebAuthnManagerTest.php
@@ -14,6 +14,7 @@
 use OCA\TwoFactorWebauthn\Event\StateChanged;
 use OCA\TwoFactorWebauthn\Repository\WebauthnPublicKeyCredentialSourceRepository;
 use OCA\TwoFactorWebauthn\Service\WebAuthnManager;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\ISession;
 use OCP\IUser;
@@ -41,6 +42,8 @@ class WebAuthnManagerTest extends TestCase {
 	/** @var MockObject|LoggerInterface */
 	private $logger;
 
+	private ITimeFactory&MockObject $time;
+
 	protected function setUp(): void {
 		parent::setUp();
 
@@ -49,24 +52,28 @@ protected function setUp(): void {
 		$this->mapper = $this->createMock(PublicKeyCredentialEntityMapper::class);
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->time = $this->createMock(ITimeFactory::class);
 
 		$this->manager = new WebAuthnManager(
 			$this->session,
 			$this->repository,
 			$this->mapper,
 			$this->eventDispatcher,
 			$this->logger,
+			$this->time,
 		);
 	}
 
-	/**
-	 * @param IUser $user
-	 * @param int $nr
-	 */
-	private function mockRegistrations(IUser $user, $nr): void {
+	private function mockRegistrations(IUser $user, int $nr): void {
 		$regs = [];
 		for ($i = 0; $i < $nr; $i++) {
 			$reg = new PublicKeyCredentialEntity();
+			$reg->setId($i);
+			$reg->setPublicKeyCredentialId("credential-id-$i");
+			$reg->setName("key-$i");
+			$reg->setUserHandle($user->getUID());
+			$reg->setActive(true);
+			$reg->setCreatedAt(null);
 			$regs[] = $reg;
 		}
 		$this->mapper->expects(self::once())

Original file line number	Diff line number	Diff line change
`@@ -121,6 +121,7 @@ public static function fromPublicKeyCrendentialSource(string $name,`
`121`	`121`	`$publicKeyCredentialEntity->setUserHandle($publicKeyCredentialSource->getUserHandle());`
`122`	`122`	`$publicKeyCredentialEntity->setCounter($publicKeyCredentialSource->getCounter());`
`123`	`123`	`$publicKeyCredentialEntity->setCreatedAt($ctime);`
	`124`	`+ $publicKeyCredentialEntity->setActive(true);`
`124`	`125`
`125`	`126`	`return $publicKeyCredentialEntity;`
`126`	`127`	`}`