From ff67bc10cbea245e63d78b6a593d3abe28b59fee Mon Sep 17 00:00:00 2001 From: Brady McDonough Date: Mon, 19 Feb 2024 23:44:28 -0700 Subject: [PATCH] Improved otpauth uri generation to elide defaults --- src/URI/Otpauth.php | 51 +++++++++++++++----- src/Workflow/UserManagement.php | 10 ++-- src/WorkflowInterface.php | 2 +- tests/URITest.php | 83 ++++++++++----------------------- 4 files changed, 70 insertions(+), 76 deletions(-) diff --git a/src/URI/Otpauth.php b/src/URI/Otpauth.php index 268b4a2..a9921c3 100644 --- a/src/URI/Otpauth.php +++ b/src/URI/Otpauth.php @@ -6,6 +6,12 @@ use InvalidArgumentException; class Otpauth { + const DEFAULTS = [ + "algorithm" => "SHA1", + "period" => 30, + "digits" => 6, + ]; + private readonly string $secret; public function __construct( @@ -71,12 +77,9 @@ class Otpauth $parsedQuery = []; \parse_str($parsedUri['query'], $parsedQuery); - $queryDefaults = [ - "algorithm" => "SHA1", - "period" => "30", - "digits" => "6", + $queryDefaults = \array_merge (self::DEFAULTS, [ "issuer" => \rawurldecode(\ltrim($label[0], "/")), - ]; + ]); $parsedQuery = \array_merge($queryDefaults, $parsedQuery); $runCheck($queryChecks, $parsedQuery); @@ -115,20 +118,44 @@ class Otpauth ); } - public function emitStr(): string + private function buildUri(array $queryValues): string { $encodeIssuer = \rawurlencode($this->issuer); $label = $encodeIssuer . ":" . \rawurlencode($this->userid); - $issuer = "issuer=" . $encodeIssuer; - $algo = "algorithm=" . $this->algorithm; - $digits = "digits=" . $this->digits; - $period = "period=" . $this->period; - $secret = "secret=" . \rawurlencode($this->secret); - $query = \implode("&", [$secret, $issuer, $algo, $period, $digits]); + $query = \http_build_query($queryValues); return "otpauth://totp/" . $label . "?" . $query; } + //TODO: Add a logger in each of the following + public function uriString(): string + { + $queryValues = [ + 'issuer' => $this->issuer, + 'secret' => $this->secret, + 'algorithm' => ($this->algorithm == self::DEFAULTS['algorithm']) ? null : $this->algorithm, + 'period' => ($this->period == self::DEFAULTS['period']) ? null : $this->period, + 'digits' => ($this->digits == self::DEFAULTS['digits']) ? null : $this->digits, + ]; + return $this->buildUri($queryValues); + } + + public function uriStringExplicit(): string + { + $queryValues = [ + 'issuer' => $this->issuer, + 'secret' => $this->secret, + 'algorithm' => $this->algorithm, + 'period' => $this->period, + 'digits' => $this->digits, + ]; + return $this->buildUri($queryValues); + } + + public function getSecret(): string + { + return $this->secret; + } } ?> diff --git a/src/Workflow/UserManagement.php b/src/Workflow/UserManagement.php index 939178c..8dd45e7 100644 --- a/src/Workflow/UserManagement.php +++ b/src/Workflow/UserManagement.php @@ -44,15 +44,17 @@ class UserManagement implements WorkflowInterface $html .= ""; $html .= ""; - $provisioningUri = (new Otpauth( + $otpauthURI = new Otpauth( $this->db->userString($this->userIndex), "taatp", $this->hash->keygen(), $this->hash->hashType(), 30, 6 - ))->emitStr(); - $this->session->store("secret", $provisioningUri); + ); + $provisioningUri = $otpauthURI->uriString(); + $persistentUri = $otpauthURI->uriStringExplicit(); + $this->session->store("secret", $persistentUri); $values = [ "%frm" => $this->request->formProps("enroll"), @@ -98,7 +100,7 @@ class UserManagement implements WorkflowInterface $enrollFlag && $enrollFlag = $this->session->get('secret'); $totp = _6238( - $pUri.secret, + $pUri->getSecret(), $pUri.period, $enrollFlag? 0:$this->db->getLastTime($this->userIndex), 2, diff --git a/src/WorkflowInterface.php b/src/WorkflowInterface.php index 82b8114..aec98be 100644 --- a/src/WorkflowInterface.php +++ b/src/WorkflowInterface.php @@ -13,7 +13,7 @@ interface WorkflowInterface * returns the workflow's relevant form. * @return string */ - public function emitStr():string; + public function emitStr(): string; /** * Handles any response given to the workflow's form. * @return bool diff --git a/tests/URITest.php b/tests/URITest.php index 7e4a87d..ac26125 100644 --- a/tests/URITest.php +++ b/tests/URITest.php @@ -9,77 +9,42 @@ use BradyMcD\TAATP\URI\Otpauth; /** @SuppressWarnings(PHPMD.StaticAccess)*/ final class URITest extends TestCase { - public function testRejectsInvalidUri(): void - { - $string = 'totp/ACME%20Co:john.doe@email.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=ACME%20Co&algorithm=MD5&digits=8&period=60'; - - $this->expectException(\InvalidArgumentException::class); - - Otpauth::fromString($string); - } + private static $defaults; + private static $exampleUri; + private static $exampleComponents; - - public function testRejectsMissingSecretInformation(): void + public static function setUpBeforeClass(): void { - $string = 'otpauth://totp/ACME%20Co:john.doe@email.com?issuer=ACME%20Co&algorithm=MD5&digits=8&period=60'; - - $this->expectException(\InvalidArgumentException::class); + self::$defaults = Otpauth::DEFAULTS; + self::$exampleUri = 'otpauth://totp/ACME%20Co:john.doe@email.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=ACME%20Co&algorithm=MD5&digits=8&period=60'; + \parse_str(\parse_url(self::$exampleUri)['query'], self::$exampleComponents); - Otpauth::fromString($string); } - public function testPreservesAllUriFields(): void + /** @SuppressWarnings(PHPMD.EmptyCatchBlock)*/ + public function testRejectsInvalidUris(): void { - $string = 'otpauth://totp/ACME%20Co:john.doe@email.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=ACME%20Co&algorithm=MD5&digits=8&period=60'; - - $queryComponents = [ "secret" => "HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ", - "issuer" => "ACME Co", - "algorithm" => "MD5", - "digits" => 8, - "period" => 60,]; - - $provisioningUri = Otpauth::fromString($string); + $invalids = [ + "https://www.example.org", + "Just a normal string", + "otpauth://totp/NOSECRETS:john.doe@email.com?issuer=ACME%20Co&algorithm=MD5&digits=8&period=60", + "otpauth://totp/MISS:john.doe@email.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=MATCH&algorithm=MD5&digits=8&period=60", + "otpauth://totp/vendor:john.doe@email.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=vendohr&algorithm=NotAnAlgo&digits=8&period=60", + "", - $this->assertSame($provisioningUri->issuer, $queryComponents['issuer']); - $this->assertSame($provisioningUri->digits, $queryComponents['digits']); - $this->assertSame($provisioningUri->period, $queryComponents['period']); - $this->assertSame($provisioningUri->algorithm, $queryComponents['algorithm']); + ]; + $pUri; - $calculatedUrl = $provisioningUri->emitStr(); + foreach ($invalids as $invalid) + { + $this->expectException(\InvalidArgumentException::class); - $parsedOtp = \parse_url($calculatedUrl); - $parsedTest = \parse_url($string); + $pUri = Otpauth::fromString($invalid); - $parsedOtpQuery = []; - $parsedTestQuery = []; - - \parse_str($parsedOtp['query'], $parsedOtpQuery); - unset($parsedOtp['query']); - \parse_str($parsedTest['query'], $parsedTestQuery); - unset($parsedTest['query']); - - $this->assertEqualsCanonicalizing($parsedOtp, $parsedTest); - $this->assertEqualsCanonicalizing($parsedOtpQuery, $parsedTestQuery); + $this->assertNull($pUri); + } } - public function testCanFallbackToDefaults(): void - { - $string = 'otpauth://totp/Example:john.doe@email.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ'; - $queryComponents = [ "secret" => "HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ", - "issuer" => "Example", - "algorithm" => "SHA1", - "digits" => 6, - "period" => 30,]; - - - - $provisioningUri = Otpauth::fromString($string); - - $this->assertSame($provisioningUri->algorithm, $queryComponents['algorithm']); - $this->assertSame($provisioningUri->digits, $queryComponents['digits']); - $this->assertSame($provisioningUri->period, $queryComponents['period']); - $this->assertSame($provisioningUri->issuer, $queryComponents['issuer']); - } } ?>