Merge pull request #483 from nextcloud/fix/registrations-by-entity-id

fix: handle devices by their db entity id

lib/Controller/SettingsController.php

@@ -75,18 +75,18 @@ public function finishRegister(string $name, string $data): JSONResponse {
 	/**
 	 * @NoAdminRequired
 	 * @PasswordConfirmationRequired
-	 *
-	 * @param string $id
 	 */
-	public function remove(string $id): JSONResponse {
-		return new JSONResponse($this->manager->removeDevice($this->userSession->getUser(), $id));
+	public function remove(int $id): JSONResponse {
+		$this->manager->removeDevice($this->userSession->getUser(), $id);
+		return new JSONResponse([]);
 	}
 
 	/**
 	 * @NoAdminRequired
 	 * @PasswordConfirmationRequired
 	 */
-	public function changeActivationState(string $id, bool $active): JSONResponse {
-		return new JSONResponse($this->manager->changeActivationState($this->userSession->getUser(), $id, $active));
+	public function changeActivationState(int $id, bool $active): JSONResponse {
+		$this->manager->changeActivationState($this->userSession->getUser(), $id, $active);
+		return new JSONResponse([]);
 	}
 }

lib/Db/PublicKeyCredentialEntityMapper.php

@@ -26,6 +26,7 @@
 
 namespace OCA\TwoFactorWebauthn\Db;
 
+use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\Entity;
 use OCP\AppFramework\Db\QBMapper;
 use OCP\DB\Exception;
@@ -93,6 +94,26 @@ public function __construct(IDBConnection $db) {
 	 */
 	protected $active;
 
+	/**
+	 * @throws Exception
+	 */
+	public function findById(int $id): ?PublicKeyCredentialEntity {
+		$qb = $this->db->getQueryBuilder();
+
+		$qb->select('*')
+			->from($this->getTableName())
+			->where($qb->expr()->eq(
+				'id',
+				$qb->createNamedParameter($id, IQueryBuilder::PARAM_INT),
+				IQueryBuilder::PARAM_INT
+			));
+		try {
+			return $this->findEntity($qb);
+		} catch (DoesNotExistException $e) {
+			return null;
+		}
+	}
+
 	/**
 	 * @param IUser $user
 	 * @param int $id

lib/Service/WebAuthnManager.php

@@ -246,6 +246,7 @@ 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(),
@@ -360,8 +361,8 @@ public function finishAuthenticate(IUser $user, string $data) {
 		}
 	}
 
-	public function removeDevice(IUser $user, string $id) {
-		$credential = $this->mapper->findPublicKeyCredential($id);
+	public function removeDevice(IUser $user, int $id) {
+		$credential = $this->mapper->findById($id);
 		Assertion::eq($credential->getUserHandle(), $user->getUID());
 
 		$this->mapper->delete($credential);
@@ -378,8 +379,8 @@ public function deactivateAllDevices(IUser $user) {
 		$this->eventDispatcher->dispatch(StateChanged::class, new DisabledByAdmin($user));
 	}
 
-	public function changeActivationState(IUser $user, string $id, bool $active) {
-		$credential = $this->mapper->findPublicKeyCredential($id);
+	public function changeActivationState(IUser $user, int $id, bool $active) {
+		$credential = $this->mapper->findById($id);
 		Assertion::eq($credential->getUserHandle(), $user->getUID());
 
 		$credential->setActive($active);

src/components/Device.vue

@@ -63,6 +63,7 @@ export default {
 		InformationOutline,
 	},
 	props: {
+		entityId: Number,
 		id: String,
 		name: String,
 		active: Boolean,
@@ -84,15 +85,18 @@ export default {
 		async onDelete() {
 			try {
 				await confirmPassword()
-				await this.$store.dispatch('removeDevice', this.id)
+				await this.$store.dispatch('removeDevice', this.entityId)
 			} catch (e) {
 				console.error('could not delete device', e)
 			}
 		},
 		async changeActivation(active) {
 			try {
 				await confirmPassword()
-				await this.$store.dispatch('changeActivationState', { id: this.id, active })
+				await this.$store.dispatch('changeActivationState', {
+					entityId: this.entityId,
+					active,
+				})
 			} catch (e) {
 				console.error('could not change device state', e)
 			}

src/components/PersonalSettings.vue

@@ -30,7 +30,8 @@
 		</p>
 		<Device v-for="device in devices"
 			:id="device.id"
-			:key="device.id"
+			:key="device.entityId"
+			:entity-id="device.entityId"
 			:name="device.name"
 			:active="device.active"
 			:created-at="device.createdAt" />

src/store.js

@@ -38,22 +38,22 @@ export const mutations = {
 		state.devices.sort((d1, d2) => d1.name.localeCompare(d2.name))
 	},
 
-	removeDevice(state, id) {
-		state.devices = state.devices.filter(device => device.id !== id)
+	removeDevice(state, entityId) {
+		state.devices = state.devices.filter(device => device.entityId !== entityId)
 	},
 
-	changeActivationState(state, { id, active }) {
-		state.devices.find(device => device.id === id).active = active
+	changeActivationState(state, { entityId, active }) {
+		state.devices.find(device => device.entityId === entityId).active = active
 	},
 }
 
 export const actions = {
-	removeDevice({ state, commit }, id) {
-		const device = state.devices.find(device => device.id === id)
+	removeDevice({ state, commit }, entityId) {
+		const device = state.devices.find(device => device.entityId === entityId)
 
-		commit('removeDevice', id)
+		commit('removeDevice', entityId)
 
-		removeRegistration(id)
+		removeRegistration(entityId)
 			.catch(err => {
 				// Rollback
 				commit('addDevice', device)
@@ -62,11 +62,11 @@ export const actions = {
 			})
 	},
 
-	changeActivationState({ state, commit }, { id, active }) {
-		commit('changeActivationState', { id, active })
+	changeActivationState({ state, commit }, { entityId, active }) {
+		commit('changeActivationState', { entityId, active })
 
-		changeActivationState(id, active).catch(err => {
-			commit('changeActivationState', { id, active: !active })
+		changeActivationState(entityId, active).catch(err => {
+			commit('changeActivationState', { entityId, active: !active })
 			throw err
 		})
 	},

tests/Unit/Service/WebAuthnManagerTest.php

@@ -110,10 +110,11 @@ public function testGetNoDevices(): void {
 
 	public function testDisableWebAuthn(): void {
 		$user = $this->createMock(IUser::class);
-		$reg = $this->createMock(PublicKeyCredentialEntity::class);
+		$reg = new PublicKeyCredentialEntity();
+		$reg->setId(420);
 		$this->mapper->expects(self::once())
-			->method('findPublicKeyCredential')
-			->with('k1')
+			->method('findById')
+			->with(420)
 			->willReturn($reg);
 		$this->mapper->expects(self::once())
 			->method('delete')
@@ -125,7 +126,7 @@ public function testDisableWebAuthn(): void {
 				self::equalTo(new StateChanged($user, false))
 			);
 
-		$this->manager->removeDevice($user, 'k1');
+		$this->manager->removeDevice($user, 420);
 	}
 
 	public function testStartRegistrationFirstDevice(): void {