[stable32] perf: load totp secret just once for verification

lib/Provider/TotpProvider.php

@@ -10,6 +10,7 @@
 namespace OCA\TwoFactorTOTP\Provider;
 
 use OCA\TwoFactorTOTP\AppInfo\Application;
+use OCA\TwoFactorTOTP\Exception\NoTotpSecretFoundException;
 use OCA\TwoFactorTOTP\Service\ITotp;
 use OCA\TwoFactorTOTP\Settings\Personal;
 use OCP\AppFramework\IAppContainer;
@@ -70,7 +71,12 @@ public function getTemplate(IUser $user): Template {
 	 */
 	public function verifyChallenge(IUser $user, string $challenge): bool {
 		$challenge = preg_replace('/[^0-9]/', '', $challenge);
-		return $this->totp->validateSecret($user, $challenge);
+		try {
+			$secret = $this->totp->getSecret($user);
+		} catch (NoTotpSecretFoundException $e) {
+			return false;
+		}
+		return $this->totp->validateSecret($secret, $challenge);
 	}
 
 	/**

lib/Service/ITotp.php

@@ -9,6 +9,7 @@
 
 namespace OCA\TwoFactorTOTP\Service;
 
+use OCA\TwoFactorTOTP\Db\TotpSecret;
 use OCA\TwoFactorTOTP\Exception\NoTotpSecretFoundException;
 use OCA\TwoFactorTOTP\Exception\TotpSecretAlreadySet;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -33,6 +34,11 @@ public function hasSecret(IUser $user): bool;
 	 */
 	public function createSecret(IUser $user): string;
 
+	/**
+	 * @throws NoTotpSecretFoundException
+	 */
+	public function getSecret(IUser $user): TotpSecret;
+
 	/**
 	 * Enable OTP for the given user. The secret has to be generated
 	 * beforehand, using ITotp::createSecret
@@ -47,5 +53,5 @@ public function enable(IUser $user, $key): bool;
 
 	public function deleteSecret(IUser $user, bool $byAdmin = false): void;
 
-	public function validateSecret(IUser $user, string $key): bool;
+	public function validateSecret(TotpSecret $secret, string $key): bool;
 }

lib/Service/Totp.php

@@ -70,11 +70,23 @@ public function createSecret(IUser $user): string {
 		return $secret;
 	}
 
+	public function getSecret(IUser $user): TotpSecret {
+		try {
+			return $this->secretMapper->getSecret($user);
+		} catch (DoesNotExistException $e) {
+			throw new NoTotpSecretFoundException(
+				$e->getMessage(),
+				$e->getCode(),
+				$e,
+			);
+		}
+	}
+
 	public function enable(IUser $user, $key): bool {
-		if (!$this->validateSecret($user, $key)) {
+		$dbSecret = $this->secretMapper->getSecret($user);
+		if (!$this->validateSecret($dbSecret, $key)) {
 			return false;
 		}
-		$dbSecret = $this->secretMapper->getSecret($user);
 		$dbSecret->setState(ITotp::STATE_ENABLED);
 		$this->secretMapper->update($dbSecret);
 
@@ -99,26 +111,20 @@ public function deleteSecret(IUser $user, bool $byAdmin = false): void {
 		}
 	}
 
-	public function validateSecret(IUser $user, string $key): bool {
-		try {
-			$dbSecret = $this->secretMapper->getSecret($user);
-		} catch (DoesNotExistException $ex) {
-			throw new NoTotpSecretFoundException();
-		}
-
-		$secret = $this->crypto->decrypt($dbSecret->getSecret());
-		$otp = Factory::getTOTP(Base32::decode($secret), 30, 6);
+	public function validateSecret(TotpSecret $secret, string $key): bool {
+		$decryptedSecret = $this->crypto->decrypt($secret->getSecret());
+		$otp = Factory::getTOTP(Base32::decode($decryptedSecret), 30, 6);
 
 		$counter = null;
-		$lastCounter = $dbSecret->getLastCounter();
+		$lastCounter = $secret->getLastCounter();
 		if ($lastCounter !== -1) {
 			$counter = $lastCounter;
 		}
 
 		$result = $otp->verify($key, 3, $counter);
 		if ($result instanceof TOTPValidResultInterface) {
-			$dbSecret->setLastCounter($result->getCounter());
-			$this->secretMapper->update($dbSecret);
+			$secret->setLastCounter($result->getCounter());
+			$this->secretMapper->update($secret);
 
 			return true;
 		}

tests/Unit/Provider/TotpProviderTest.php

@@ -10,6 +10,8 @@
 namespace OCA\TwoFactorTOTP\Test\Unit\Provider;
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
+use OCA\TwoFactorTOTP\Db\TotpSecret;
+use OCA\TwoFactorTOTP\Exception\NoTotpSecretFoundException;
 use OCA\TwoFactorTOTP\Provider\AtLoginProvider;
 use OCA\TwoFactorTOTP\Provider\TotpProvider;
 use OCA\TwoFactorTOTP\Service\ITotp;
@@ -128,6 +130,35 @@ public function testGetPersonalSettings(): void {
 		self::assertEquals($expected, $actual);
 	}
 
+	public function testVerifyChallengeSecretNotFound(): void {
+		$user = $this->createMock(IUser::class);
+		$this->totp->expects($this->once())
+			->method('getSecret')
+			->with($user)
+			->willThrowException(new NoTotpSecretFoundException());
+
+		$result = $this->provider->verifyChallenge($user, '123456');
+
+		$this->assertFalse($result);
+	}
+
+	public function testVerifyChallengeStripNonDigits(): void {
+		$user = $this->createMock(IUser::class);
+		$secret = new TotpSecret();
+		$this->totp->expects(self::once())
+			->method('getSecret')
+			->with($user)
+			->willReturn($secret);
+		$this->totp->expects(self::once())
+			->method('validateSecret')
+			->with($secret, '123456')
+			->willReturn(true);
+
+		$result = $this->provider->verifyChallenge($user, '  123456  a	');
+
+		$this->assertTrue($result);
+	}
+
 	public function testDeactivate(): void {
 		$user = $this->createMock(IUser::class);
 		$this->totp->expects($this->once())