Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TOTP: introduce new interface that prevents code reuse #232

Open
wants to merge 2 commits into
base: 11.4.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Factory.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Throwable;
use function assert;
use function count;
use function sprintf;

/**
* This class is used to load OTP object from a provisioning Uri.
Expand All @@ -21,7 +22,7 @@
{
try {
$parsed_url = Url::fromString($uri);
$parsed_url->getScheme() === 'otpauth' || throw new InvalidArgumentException('Invalid scheme.');

Check warning on line 25 in src/Factory.php

View workflow job for this annotation

GitHub Actions / 5️⃣ Mutation Testing

Escaped Mutant for Mutator "Throw_": @@ @@ { try { $parsed_url = Url::fromString($uri); - $parsed_url->getScheme() === 'otpauth' || throw new InvalidArgumentException('Invalid scheme.'); + $parsed_url->getScheme() === 'otpauth' || new InvalidArgumentException('Invalid scheme.'); } catch (Throwable $throwable) { throw new InvalidArgumentException('Not a valid OTP provisioning URI', $throwable->getCode(), $throwable); }

Check warning on line 25 in src/Factory.php

View workflow job for this annotation

GitHub Actions / 5️⃣ Mutation Testing

Escaped Mutant for Mutator "LogicalOrNegation": @@ @@ { try { $parsed_url = Url::fromString($uri); - $parsed_url->getScheme() === 'otpauth' || throw new InvalidArgumentException('Invalid scheme.'); + !($parsed_url->getScheme() === 'otpauth' || throw new InvalidArgumentException('Invalid scheme.')); } catch (Throwable $throwable) { throw new InvalidArgumentException('Not a valid OTP provisioning URI', $throwable->getCode(), $throwable); }
} catch (Throwable $throwable) {
throw new InvalidArgumentException('Not a valid OTP provisioning URI', $throwable->getCode(), $throwable);
}
Expand Down Expand Up @@ -60,15 +61,15 @@
}

if ($otp->getIssuer() !== null) {
$result[0] === $otp->getIssuer() || throw new InvalidArgumentException(

Check warning on line 64 in src/Factory.php

View workflow job for this annotation

GitHub Actions / 5️⃣ Mutation Testing

Escaped Mutant for Mutator "LogicalOrNegation": @@ @@ return; } if ($otp->getIssuer() !== null) { - $result[0] === $otp->getIssuer() || throw new InvalidArgumentException('Invalid OTP: invalid issuer in parameter'); + !($result[0] === $otp->getIssuer() || throw new InvalidArgumentException('Invalid OTP: invalid issuer in parameter')); $otp->setIssuerIncludedAsParameter(true); } assert($result[0] !== '');
'Invalid OTP: invalid issuer in parameter'
);
$otp->setIssuerIncludedAsParameter(true);

Check warning on line 67 in src/Factory.php

View workflow job for this annotation

GitHub Actions / 5️⃣ Mutation Testing

Escaped Mutant for Mutator "MethodCallRemoval": @@ @@ } if ($otp->getIssuer() !== null) { $result[0] === $otp->getIssuer() || throw new InvalidArgumentException('Invalid OTP: invalid issuer in parameter'); - $otp->setIssuerIncludedAsParameter(true); + } assert($result[0] !== ''); $otp->setIssuer($result[0]);
}

assert($result[0] !== '');

$otp->setIssuer($result[0]);

Check warning on line 72 in src/Factory.php

View workflow job for this annotation

GitHub Actions / 5️⃣ Mutation Testing

Escaped Mutant for Mutator "MethodCallRemoval": @@ @@ $otp->setIssuerIncludedAsParameter(true); } assert($result[0] !== ''); - $otp->setIssuer($result[0]); + } private static function createOTP(Url $parsed_url, ClockInterface $clock): OTPInterface {
}

private static function createOTP(Url $parsed_url, ClockInterface $clock): OTPInterface
Expand Down
1 change: 1 addition & 0 deletions src/OTP.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use function chr;
use function count;
use function is_string;
use function sprintf;
use const STR_PAD_LEFT;

abstract class OTP implements OTPInterface
Expand Down
24 changes: 24 additions & 0 deletions src/OTPWithPreviousTimestampInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

declare(strict_types=1);

namespace OTPHP;

interface OTPWithPreviousTimestampInterface extends OTPInterface
{
/**
* Verify method which prevents previously used codes from being used again. The passed values are in seconds.
*
* @param non-empty-string $otp
* @param null|0|positive-int $timestamp
* @param null|0|positive-int $leeway
* @param null|0|positive-int $previousTimestamp
* @return int|false the timestamp matching the otp on success, and false on error
*/
public function verifyWithPreviousTimestamp(
string $otp,
null|int $timestamp = null,
null|int $leeway = null,
null|int $previousTimestamp = null
): int|false;
}
1 change: 1 addition & 0 deletions src/ParameterTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use function in_array;
use function is_int;
use function is_string;
use function sprintf;

trait ParameterTrait
{
Expand Down
54 changes: 49 additions & 5 deletions src/TOTP.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
/**
* @see \OTPHP\Test\TOTPTest
*/
final class TOTP extends OTP implements TOTPInterface
final class TOTP extends OTP implements TOTPInterface, OTPWithPreviousTimestampInterface
{
private readonly ClockInterface $clock;

Expand Down Expand Up @@ -118,12 +118,30 @@ public function now(): string
*/
public function verify(string $otp, null|int $timestamp = null, null|int $leeway = null): bool
{
return $this->verifyWithPreviousTimestamp($otp, $timestamp, $leeway, null) !== false;
}

/**
* Verify method which prevents previously used codes from being used again. The passed values are in seconds.
*
* @param non-empty-string $otp
* @param 0|positive-int $timestamp
* @param null|0|positive-int $leeway
* @param null|0|positive-int $previousTimestamp
* @return int|false the timestamp matching the otp on success, and false on error
*/
public function verifyWithPreviousTimestamp(
string $otp,
null|int $timestamp = null,
null|int $leeway = null,
null|int $previousTimestamp = null
): int|false {
$timestamp ??= $this->clock->now()
->getTimestamp();
$timestamp >= 0 || throw new InvalidArgumentException('Timestamp must be at least 0.');

if ($leeway === null) {
return $this->compareOTP($this->at($timestamp), $otp);
return $this->verifyOTPAtTimestamps($otp, [$timestamp], $previousTimestamp);
}

$leeway = abs($leeway);
Expand All @@ -135,9 +153,11 @@ public function verify(string $otp, null|int $timestamp = null, null|int $leeway
'The timestamp must be greater than or equal to the leeway.'
);

return $this->compareOTP($this->at($timestampMinusLeeway), $otp)
|| $this->compareOTP($this->at($timestamp), $otp)
|| $this->compareOTP($this->at($timestamp + $leeway), $otp);
return $this->verifyOTPAtTimestamps(
$otp,
[$timestampMinusLeeway, $timestamp, $timestamp + $leeway],
$previousTimestamp
);
}

public function getProvisioningUri(): string
Expand Down Expand Up @@ -200,6 +220,30 @@ protected function filterOptions(array &$options): void
ksort($options);
}

/**
* @param non-empty-string $otp
* @param array<0|positive-int> $timestamps
*/
private function verifyOTPAtTimestamps(string $otp, array $timestamps, null|int $previousTimestamp): int|false
{
$previousTimeCode = null;
if ($previousTimestamp > 0) {
$previousTimeCode = $this->timecode($previousTimestamp);
}

foreach ($timestamps as $timestamp) {
if ($previousTimeCode !== null && $previousTimeCode >= $this->timecode($timestamp)) {
continue;
}

if ($this->compareOTP($this->at($timestamp), $otp)) {
return $timestamp;
}
}

return false;
}

/**
* @param 0|positive-int $timestamp
*
Expand Down
25 changes: 25 additions & 0 deletions tests/TOTPTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,31 @@ public function verifyOtpWithEpoch(): void
static::assertFalse($otp->verify('139664', 1_301_012_297));
}

#[Test]
public function verifyOtpWithPreviousTimestamp(): void
{
$otp = self::createTOTP(6, 'sha1', 30);

static::assertSame(319_690_800, $otp->verifyWithPreviousTimestamp('762124', 319_690_800, null, 0));
static::assertSame(
319_690_800,
$otp->verifyWithPreviousTimestamp('762124', 319_690_800, null, 319_690_770),
'Can use new code'
);
static::assertFalse(
$otp->verifyWithPreviousTimestamp('762124', 319_690_800, null, 319_690_800),
'Cannot use same code again'
);
static::assertFalse(
$otp->verifyWithPreviousTimestamp('762124', 319_690_801, null, 319_690_800),
'Cannot use same code again at different timestamp'
);
static::assertFalse(
$otp->verifyWithPreviousTimestamp('762124', 319_690_800, null, 319_690_830),
'Cannot use previous code'
);
}

#[Test]
public function notCompatibleWithGoogleAuthenticator(): void
{
Expand Down
Loading