Fix wrong provider registry updates.

This is a multi-device provider, so adding a device does
not automatically mean that the provider gets enabled (there
might be an existing device), nor that removing a device disables
the provider (there might be remaining devices).

This adds an additional check to count the number of devices
*after* the state change to only react when the first device
has been added and when the last one is removed.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

lib/Listener/StateChangeRegistryUpdater.php

@@ -16,6 +16,7 @@ namespace OCA\TwoFactorU2F\Listener;
 
 use OCA\TwoFactorU2F\Event\StateChanged;
 use OCA\TwoFactorU2F\Provider\U2FProvider;
+use OCA\TwoFactorU2F\Service\U2FManager;
 use OCP\Authentication\TwoFactorAuth\IRegistry;
 use Symfony\Component\EventDispatcher\Event;
 
@@ -24,19 +25,26 @@ class StateChangeRegistryUpdater implements IListener {
 	/** @var IRegistry */
 	private $providerRegistry;
 
+	/** @var U2FManager */
+	private $manager;
+
 	/** @var U2FProvider */
 	private $provider;
 
-	public function __construct(IRegistry $providerRegistry, U2FProvider $provider) {
+	public function __construct(IRegistry $providerRegistry, U2FManager $manager, U2FProvider $provider) {
 		$this->providerRegistry = $providerRegistry;
 		$this->provider = $provider;
+		$this->manager = $manager;
 	}
 
 	public function handle(Event $event) {
 		if ($event instanceof StateChanged) {
-			if ($event->isEnabled()) {
+			$devices = $this->manager->getDevices($event->getUser());
+			if ($event->isEnabled() && count($devices) === 1) {
+				// The first device was enabled -> enable provider for this user
 				$this->providerRegistry->enableProviderFor($this->provider, $event->getUser());
-			} else {
+			} else if (!$event->isEnabled() && empty($devices)) {
+				// The last device was removed -> disable provider for this user
 				$this->providerRegistry->disableProviderFor($this->provider, $event->getUser());
 			}
 		}

tests/Unit/Listener/StateChangeRegistryUpdaterTest.php

@@ -11,6 +11,7 @@ namespace OCA\TwoFactorU2F\Tests\Unit\Listener;
 use OCA\TwoFactorU2F\Event\StateChanged;
 use OCA\TwoFactorU2F\Listener\StateChangeRegistryUpdater;
 use OCA\TwoFactorU2F\Provider\U2FProvider;
+use OCA\TwoFactorU2F\Service\U2FManager;
 use OCP\Authentication\TwoFactorAuth\IRegistry;
 use OCP\IUser;
 use PHPUnit\Framework\TestCase;
@@ -22,6 +23,9 @@ class StateChangeRegistryUpdaterTest extends TestCase {
 	/** @var IRegistry|PHPUnit_Framework_MockObject_MockObject */
 	private $providerRegistry;
 
+	/** @var U2FManager|PHPUnit_Framework_MockObject_MockObjec */
+	private $manager;
+
 	/** @var U2FProvider|PHPUnit_Framework_MockObject_MockObjec */
 	private $provider;
 
@@ -32,9 +36,10 @@ class StateChangeRegistryUpdaterTest extends TestCase {
 		parent::setUp();
 
 		$this->providerRegistry = $this->createMock(IRegistry::class);
+		$this->manager = $this->createMock(U2FManager::class);
 		$this->provider = $this->createMock(U2FProvider::class);
 
-		$this->listener = new StateChangeRegistryUpdater($this->providerRegistry, $this->provider);
+		$this->listener = new StateChangeRegistryUpdater($this->providerRegistry, $this->manager, $this->provider);
 	}
 
 	public function testHandleGenericEvent() {
@@ -47,9 +52,17 @@ class StateChangeRegistryUpdaterTest extends TestCase {
 		$this->listener->handle($event);
 	}
 
-	public function testHandleEnableEvent() {
+	public function testHandleEnableFirstDevice() {
 		$user = $this->createMock(IUser::class);
 		$event = new StateChanged($user, true);
+		$this->manager->expects($this->once())
+			->method('getDevices')
+			->willReturn([
+				[
+					'id' => 1,
+					'name' => 'utf1',
+				],
+			]);
 		$this->providerRegistry->expects($this->once())
 			->method('enableProviderFor')
 			->with(
@@ -60,9 +73,33 @@ class StateChangeRegistryUpdaterTest extends TestCase {
 		$this->listener->handle($event);
 	}
 
-	public function testHandleDisableEvent() {
+	public function testHandleEnableSecondDevice() {
+		$user = $this->createMock(IUser::class);
+		$event = new StateChanged($user, true);
+		$this->manager->expects($this->once())
+			->method('getDevices')
+			->willReturn([
+				[
+					'id' => 1,
+					'name' => 'utf1',
+				],
+				[
+					'id' => 2,
+					'name' => 'utf2',
+				],
+			]);
+		$this->providerRegistry->expects($this->never())
+			->method('enableProviderFor');
+
+		$this->listener->handle($event);
+	}
+
+	public function testHandleDisableLastDevice() {
 		$user = $this->createMock(IUser::class);
 		$event = new StateChanged($user, false);
+		$this->manager->expects($this->once())
+			->method('getDevices')
+			->willReturn([]);
 		$this->providerRegistry->expects($this->once())
 			->method('disableProviderFor')
 			->with(
@@ -72,4 +109,25 @@ class StateChangeRegistryUpdaterTest extends TestCase {
 
 		$this->listener->handle($event);
 	}
+
+	public function testHandleDisableWithRemainingDevices() {
+		$user = $this->createMock(IUser::class);
+		$event = new StateChanged($user, false);
+		$this->manager->expects($this->once())
+			->method('getDevices')
+			->willReturn([
+				[
+					'id' => 2,
+					'name' => 'utf2',
+				],
+			]);
+		$this->providerRegistry->expects($this->never())
+			->method('disableProviderFor')
+			->with(
+				$this->provider,
+				$user
+			);
+
+		$this->listener->handle($event);
+	}
 }