Improve support for more complex challenge management (#75)

Splits the retrieval and management of challenges. 

From a good vibes perspective, this a) aligns the main interface name
with the functionality it's intended to provide, and b) improves the
interface segregation (hi, SOLID).

In terms of practical needs, this helps solve the (fairly rare) need of
challenges that are managed as part of a broader external system. This
could be a data model, microservice, or whatever. By splitting the
interfaces and only requiring that the newer `ChallengeLoaderInterface`
make it to the verification processes, this becomes both easier and more
testable.

README.md

@@ -69,8 +69,9 @@ Send it to the user as base64.
 ```php
 <?php
 
-// Generate challenge
-$challenge = $challengeManager->createChallenge();
+// Generate and manage challenge
+$challenge = \Firehed\WebAuthn\ExpiringChallenge::withLifetime(300);
+$challengeManager->manageChallenge($challenge);
 
 // Send to user
 header('Content-type: application/json');
@@ -223,7 +224,9 @@ $_SESSION['authenticating_user_id'] = $user['id'];
 // See examples/functions.php for how this works
 $credentialContainer = getCredentialsForUserId($pdo, $user['id']);
 
-$challenge = $challengeManager->createChallenge();
+// Generate and manage challenge
+$challenge = \Firehed\WebAuthn\ExpiringChallenge::withLifetime(300);
+$challengeManager->manageChallenge($challenge);
 
 // Send to user
 header('Content-type: application/json');
@@ -595,6 +598,7 @@ In the event you find this necessary, you SHOULD open an Issue and/or Pull Reque
 Challenges generated by your server SHOULD expire after a short amount of time.
 You MAY use the `ExpiringChallenge` class for convenience (e.g. `$challenge = ExpiringChallenge::withLifetime(60);`), which will throw an exception if the specified expiration window has been exceeded.
 It is RECOMMENDED that your javascript code uses the `timeout` setting (denoted in milliseconds) and matches the server-side challenge expiration, give or take a few seconds.
+W3C recommends timeouts between 5 and 10 minutes.
 
 > [!NOTE]
 > The W3C specification recommends a timeout in the range of 15-120 seconds.

examples/getChallenge.php

@@ -2,11 +2,14 @@
 
 require __DIR__ . '/vendor/autoload.php';
 
+use Firehed\WebAuthn\ExpiringChallenge;
+
 session_start();
 
 // Generate challenge
 $challengeManager = getChallengeManager();
-$challenge = $challengeManager->createChallenge();
+$challenge = ExpiringChallenge::withLifetime(300);
+$challengeManager->manageChallenge($challenge);
 
 // Send to user
 header('Content-type: application/json');

src/ChallengeLoaderInterface.php

@@ -0,0 +1,37 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Firehed\WebAuthn;
+
+/**
+ * In most cases, you will want to interact with ChallengeManagerInterface,
+ * which extends this. That will let you generate challenges, manage (store)
+ * them, and subsequently verify them if they're found in storage. This is
+ * intended as the primary data flow, and is the recommended path.
+ *
+ * In rare circumstances, you may need to verify externally-managed challenges.
+ * If so, the loading component may opt to only implement this interface. Doing
+ * so is NOT RECOMMENDED at this time.
+ *
+ * @api (with the above caveats)
+ */
+interface ChallengeLoaderInterface
+{
+    /**
+     * Consumes the challenge associated with the ClientDataJSON value from the
+     * underlying storage mechanism, and returns that challenge if found.
+     *
+     * Implementations MUST ensure that subsequent calls to this method with
+     * the same value return `null`, regardless of whether the initial call
+     * returned a value or null. Failure to do so will compromise the security
+     * of the webauthn protocol.
+     *
+     * Implementations MUST NOT use the ClientDataJSON value to construct
+     * a challenge. They MUST return a previously-stored value if one is found,
+     * and MAY use $base64Url to search the storage mechanism.
+     *
+     * @internal
+     */
+    public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface;
+}

src/ChallengeLoaderTestTrait.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Firehed\WebAuthn;
+
+/**
+ * Trait for adding PHPUnit test cases to both packaged and custom
+ * ChallengeLoaderInterface implementations.
+ *
+ * @api
+ */
+trait ChallengeLoaderTestTrait
+{
+    public function testChallengeCannotBeRetrievedTwice(): void
+    {
+        $c = Challenge::random();
+        $cl = $this->getChallengeLoaderManagingChallenge($c);
+
+        $result = $cl->useFromClientDataJSON($c->getBase64Url());
+        $result2 = $cl->useFromClientDataJSON($c->getBase64Url());
+
+        self::assertNotNull($result);
+        self::assertSame($c->getBase64Url(), $result->getBase64Url());
+        self::assertNull($result2);
+    }
+
+    public function testManagerDoesNotReturnUnmanagedChallenge(): void
+    {
+        $c = Challenge::random();
+        $cl = $this->getChallengeLoaderManagingChallenge($c);
+
+        $c2 = Challenge::random();
+        assert($c->getBase64Url() !== $c2->getBase64Url());
+        self::assertNull($cl->useFromClientDataJSON($c2->getBase64Url()));
+    }
+
+    abstract protected function getChallengeLoaderManagingChallenge(
+        ChallengeInterface $challenge,
+    ): ChallengeLoaderInterface;
+}

src/ChallengeManagerInterface.php

@@ -4,30 +4,15 @@
 
 namespace Firehed\WebAuthn;
 
-interface ChallengeManagerInterface
+/**
+ * @api
+ */
+interface ChallengeManagerInterface extends ChallengeLoaderInterface
 {
     /**
-     * Generates a new Challenge, stores it in the backing mechanism, and
-     * returns it.
+     * Takes the provided challenge and stores it in the backing mechanism.
      *
      * @api
      */
-    public function createChallenge(): ChallengeInterface;
-
-    /**
-     * Consumes the challenge associated with the ClientDataJSON value from the
-     * underlying storage mechanism, and returns that challenge if found.
-     *
-     * Implementations MUST ensure that subsequent calls to this method with
-     * the same value return `null`, regardless of whether the initial call
-     * returned a value or null. Failure to do so will compromise the security
-     * of the webauthn protocol.
-     *
-     * Implementations MUST NOT use the ClientDataJSON value to construct
-     * a challenge. They MUST return a previously-stored value if one is found,
-     * and MAY use $base64Url to search the storage mechanism.
-     *
-     * @internal
-     */
-    public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface;
+    public function manageChallenge(ChallengeInterface $challenge): void;
 }

tests/ChallengeManagerTestTrait.php → src/ChallengeManagerTestTrait.php

@@ -4,23 +4,30 @@
 
 namespace Firehed\WebAuthn;
 
+/**
+ * Trait for adding PHPUnit test cases to both packaged and custom
+ * ChallengeManagerInterface implementations.
+ *
+ * @api
+ */
 trait ChallengeManagerTestTrait
 {
-    abstract protected function getChallengeManager(): ChallengeManagerInterface;
+    use ChallengeLoaderTestTrait;
 
-    public function testCreateChallengeIsUnique(): void
+    protected function getChallengeLoaderManagingChallenge(ChallengeInterface $challenge): ChallengeLoaderInterface
     {
         $cm = $this->getChallengeManager();
-        $c1 = $cm->createChallenge();
-        $c2 = $cm->createChallenge();
-        self::assertNotSame($c1, $c2);
-        self::assertNotSame($c1->getBase64(), $c2->getBase64());
+        $cm->manageChallenge($challenge);
+        return $cm;
     }
 
+    abstract protected function getChallengeManager(): ChallengeManagerInterface;
+
     public function testMostRecentChallengeCanBeRetrieved(): void
     {
         $cm = $this->getChallengeManager();
-        $c = $cm->createChallenge();
+        $c = $this->createChallenge();
+        $cm->manageChallenge($c);
         $cdjValue = $c->getBinary()->toBase64Url();
 
         $found = $cm->useFromClientDataJSON($cdjValue);
@@ -31,7 +38,8 @@ public function testMostRecentChallengeCanBeRetrieved(): void
     public function testMostRecentChallengeCanBeRetrievedOnlyOnce(): void
     {
         $cm = $this->getChallengeManager();
-        $c = $cm->createChallenge();
+        $c = $this->createChallenge();
+        $cm->manageChallenge($c);
         $cdjValue = $c->getBinary()->toBase64Url();
 
         $found = $cm->useFromClientDataJSON($cdjValue);
@@ -45,7 +53,8 @@ public function testNoChallengeIsReturnedIfManagerIsEmpty(): void
     {
         $cm = $this->getChallengeManager();
 
-        $c = Challenge::random();
+        $c = $this->createChallenge();
+        // Do NOT manage it
         $cdjValue = $c->getBinary()->toBase64Url();
 
         $found = $cm->useFromClientDataJSON($cdjValue);
@@ -56,9 +65,10 @@ public function testNoChallengeIsReturnedIfManagerIsEmpty(): void
     public function testRetrievalDoesNotCreateChallengeFromUserData(): void
     {
         $cm = $this->getChallengeManager();
-        $c = $cm->createChallenge();
+        $c = $this->createChallenge();
+        $cm->manageChallenge($c);
 
-        $userChallenge = Challenge::random();
+        $userChallenge = $this->createChallenge();
         $cdjValue = $userChallenge->getBinary()->toBase64Url();
 
         $retrieved = $cm->useFromClientDataJSON($cdjValue);
@@ -67,4 +77,9 @@ public function testRetrievalDoesNotCreateChallengeFromUserData(): void
         // provided value.
         self::assertNotSame($userChallenge->getBase64(), $retrieved?->getBase64());
     }
+
+    private function createChallenge(): ChallengeInterface
+    {
+        return Challenge::random();
+    }
 }

src/CreateResponse.php

@@ -40,7 +40,7 @@ public function isUserVerified(): bool
      * @link https://www.w3.org/TR/webauthn-3/#sctn-registering-a-new-credential
      */
     public function verify(
-        ChallengeManagerInterface $challenge,
+        ChallengeLoaderInterface $challengeLoader,
         RelyingPartyInterface $rp,
         Enums\UserVerificationRequirement $uv = Enums\UserVerificationRequirement::Preferred,
         bool $rejectUncertainTrustPaths = true,
@@ -62,7 +62,7 @@ public function verify(
 
         // 7.1.8
         $cdjChallenge = $C['challenge'];
-        $challenge = $challenge->useFromClientDataJSON($cdjChallenge);
+        $challenge = $challengeLoader->useFromClientDataJSON($cdjChallenge);
         if ($challenge === null) {
             $this->fail('7.1.8', 'C.challenge');
         }

src/GetResponse.php

@@ -50,7 +50,7 @@ public function isUserVerified(): bool
      * @link https://www.w3.org/TR/webauthn-3/#sctn-verifying-assertion
      */
     public function verify(
-        ChallengeManagerInterface $challenge,
+        ChallengeLoaderInterface $challengeLoader,
         RelyingPartyInterface $rp,
         CredentialContainer | CredentialInterface $credential,
         Enums\UserVerificationRequirement $uv = Enums\UserVerificationRequirement::Preferred,
@@ -108,7 +108,7 @@ public function verify(
 
         // 7.2.13
         $cdjChallenge = $C['challenge'];
-        $challenge = $challenge->useFromClientDataJSON($cdjChallenge);
+        $challenge = $challengeLoader->useFromClientDataJSON($cdjChallenge);
         if ($challenge === null) {
             $this->fail('7.2.12', 'C.challenge');
         }

src/Responses/AssertionInterface.php

@@ -7,7 +7,7 @@
 use Firehed\WebAuthn\{
     BinaryString,
     ChallengeInterface,
-    ChallengeManagerInterface,
+    ChallengeLoaderInterface,
     CredentialContainer,
     CredentialInterface,
     RelyingPartyInterface,
@@ -57,7 +57,7 @@ public function isUserVerified(): bool;
      * @api
      */
     public function verify(
-        ChallengeManagerInterface $challenge,
+        ChallengeLoaderInterface $challengeLoader,
         RelyingPartyInterface $rp,
         CredentialContainer | CredentialInterface $credential,
         UserVerificationRequirement $uv = UserVerificationRequirement::Preferred,

src/Responses/AttestationInterface.php

@@ -6,7 +6,7 @@
 
 use Firehed\WebAuthn\{
     ChallengeInterface,
-    ChallengeManagerInterface,
+    ChallengeLoaderInterface,
     CredentialInterface,
     RelyingPartyInterface,
     Enums\UserVerificationRequirement,
@@ -26,7 +26,7 @@ interface AttestationInterface
     public function isUserVerified(): bool;
 
     public function verify(
-        ChallengeManagerInterface $challenge,
+        ChallengeLoaderInterface $challengeLoader,
         RelyingPartyInterface $rp,
         UserVerificationRequirement $uv = UserVerificationRequirement::Preferred,
         bool $rejectUncertainTrustPaths = true,

src/SessionChallengeManager.php

@@ -23,11 +23,9 @@ public function __construct()
         }
     }
 
-    public function createChallenge(): ChallengeInterface
+    public function manageChallenge(ChallengeInterface $c): void
     {
-        $c = ExpiringChallenge::withLifetime(120);
         $_SESSION[self::SESSION_KEY] = $c;
-        return $c;
     }
 
     public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface
@@ -37,7 +35,10 @@ public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface
         }
         $challenge = $_SESSION[self::SESSION_KEY];
         unset($_SESSION[self::SESSION_KEY]);
-        // Validate that the stored challenge matches the CDJ value?
-        return $challenge;
+        // Validate that the stored challenge matches the CDJ value
+        if ($challenge->getBase64Url() === $base64Url) {
+            return $challenge;
+        }
+        return null;
     }
 }

tests/FixedChallengeManager.php

@@ -15,7 +15,7 @@ public function __construct(private ChallengeInterface $challenge)
     {
     }
 
-    public function createChallenge(): ChallengeInterface
+    public function manageChallenge(ChallengeInterface $c): void
     {
         throw new BadMethodCallException('Should not be used during testing');
     }

tests/IntegrationTest.php

@@ -32,7 +32,7 @@ public function testReg(string $dir): void
         $rp = new SingleOriginRelyingParty($metadata['origin']);
 
         $createReq = self::read($dir, 'reg-req');
-        $challengeManager = new TestUtilities\TestVectorChallengeManager(
+        $challengeManager = new TestUtilities\TestVectorChallengeLoader(
             $createReq['publicKey']['challenge'], // @phpstan-ignore-line
         );
 

tests/TestUtilities/TestVectorChallengeManager.php → tests/TestUtilities/TestVectorChallengeLoader.php

@@ -8,20 +8,15 @@
 use Firehed\WebAuthn\{
     BinaryString,
     ChallengeInterface,
-    ChallengeManagerInterface,
+    ChallengeLoaderInterface,
 };
 
-class TestVectorChallengeManager implements ChallengeManagerInterface
+class TestVectorChallengeLoader implements ChallengeLoaderInterface
 {
     public function __construct(private string $b64u)
     {
     }
 
-    public function createChallenge(): ChallengeInterface
-    {
-        throw new Exception('Not for use during testing');
-    }
-
     public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface
     {
         if ($this->b64u === $base64Url) {

tests/TestUtilities/TestVectorFixedChallenge.php

@@ -12,7 +12,7 @@
 };
 
 /**
- * Like TestVectorChallengeManager, this would be quite dangerous to use unless
+ * Like TestVectorChallengeLoader, this would be quite dangerous to use unless
  * challenges are coming from a known source. DO NOT USE THIS as an example
  * implementation.
  */