[FIX] CmiXapi: Handle xAPI document resource preconditions in the proxy

The xAPI proxy did not forward If-Match/If-None-Match to the LRS. Both
headers are missing from the allowlist in createProxyRequest(), so a
content player could not announce the document revision it expects.

For the xAPI document resources the specification requires an LRS to
reject a PUT on an already existing document that carries neither header
with 409 Conflict, which checkResponse() then replaced by a generic
"412 Wrong Response". A specification compliant LRS therefore rejected
every rewrite of a state document and the content never learned why.
Learning Locker ignores both headers, so the defect only surfaced on
strict LRS.

Both headers are now relayed, the ETag of a document is exposed to the
content via CORS, and the conditional status codes reach the content
unchanged instead of being masked. TinCanJS, which is bundled with
common content players, sends no precondition at all on a state write
unless the caller supplied the SHA1 of the document it read before, so
a write rejected with 409 is repeated once with an If-Match built from
the current ETag.

Author: marvimarv

components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php

@@ -38,6 +38,20 @@ class XapiProxyRequest
     private string $cmdPart2plus = "";
     private bool $checkGetStatements = true;
 
+    /**
+     * The xAPI document resources are mutable, so the specification guards them with optimistic
+     * concurrency (xAPI 1.0.3, "Concurrency"): a content player announces the document revision
+     * it expects, and an LRS must reject a PUT on an already existing document that carries
+     * neither of these headers with 409 Conflict.
+     * @var list<string>
+     */
+    private const CONDITIONAL_REQUEST_HEADERS = ['If-Match', 'If-None-Match'];
+
+    /**
+     * @var list<string>
+     */
+    private const DOCUMENT_RESOURCES = ['activities/state', 'activities/profile', 'agents/profile'];
+
     public function __construct(XapiProxy $xapiproxy)
     {
         $this->dic = $GLOBALS['DIC'];
@@ -279,6 +293,25 @@ private function handleProxy(\Psr\Http\Message\RequestInterface $request, $fakeP
                 $this->xapiproxy->log()->error($this->msg($e->getMessage()));
             }
 
+            $responses['default'] = $this->repeatDocumentWriteWithPrecondition(
+                $httpclient,
+                $request,
+                $responses['default'],
+                $uriDefault,
+                $authDefault,
+                $body,
+                $req_opts
+            );
+            $responses['fallback'] = $this->repeatDocumentWriteWithPrecondition(
+                $httpclient,
+                $request,
+                $responses['fallback'],
+                $uriFallback,
+                $authFallback,
+                $body,
+                $req_opts
+            );
+
             $defaultOk = $this->xapiProxyResponse->checkResponse($responses['default'], $endpointDefault);
             $fallbackOk = $this->xapiProxyResponse->checkResponse($responses['fallback'], $endpointFallback);
 
@@ -318,6 +351,16 @@ private function handleProxy(\Psr\Http\Message\RequestInterface $request, $fakeP
             } catch (\Exception $e) {
                 $this->xapiproxy->log()->error($this->msg($e->getMessage()));
             }
+            $responses['default'] = $this->repeatDocumentWriteWithPrecondition(
+                $httpclient,
+                $request,
+                $responses['default'],
+                $uriDefault,
+                $authDefault,
+                $body,
+                $req_opts
+            );
+
             if ($this->xapiProxyResponse->checkResponse($responses['default'], $endpointDefault)) {
                 try {
                     $this->xapiProxyResponse->handleResponse(
@@ -372,10 +415,98 @@ private function createProxyRequest(\Psr\Http\Message\RequestInterface $request,
             $headers['Connection'] = $request->getHeader('Connection');
         }
 
+        foreach (self::CONDITIONAL_REQUEST_HEADERS as $conditionalHeader) {
+            if ($request->hasHeader($conditionalHeader)) {
+                $headers[$conditionalHeader] = $request->getHeader($conditionalHeader);
+            }
+        }
+
         //$this->xapiproxy->log()->debug($this->msg($body));
 
         $req = new Request(strtoupper($request->getMethod()), $uri, $headers, $body);
 
         return $req;
     }
+
+    /**
+     * TinCanJS, which is bundled with common content players, only sends If-Match on a state
+     * write when the caller supplied the SHA1 of the document it read before, and no precondition
+     * at all otherwise. A specification compliant LRS answers such a write with 409 Conflict. In
+     * that case the current ETag is looked up and the write is repeated once with an If-Match
+     * built from it. An LRS that does not demand a precondition never answers 409 and therefore
+     * never causes the additional roundtrip.
+     * @param array{state: string, value?: \GuzzleHttp\Psr7\Response, reason?: mixed} $response
+     * @param array<string, mixed> $req_opts
+     * @return array{state: string, value?: \GuzzleHttp\Psr7\Response, reason?: mixed}
+     */
+    private function repeatDocumentWriteWithPrecondition(
+        Client $httpclient,
+        \Psr\Http\Message\RequestInterface $request,
+        array $response,
+        Uri $uri,
+        string $auth,
+        string $body,
+        array $req_opts
+    ): array {
+        if ($response['state'] !== 'fulfilled' || $response['value']->getStatusCode() !== 409) {
+            return $response;
+        }
+        if (!$this->requiresSynthesizedPrecondition($request)) {
+            return $response;
+        }
+        $etag = $this->fetchDocumentEtag($httpclient, $request, $uri, $auth, $req_opts);
+        if ($etag === '') {
+            return $response;
+        }
+
+        $this->xapiproxy->log()->debug($this->msg('lrs requires a precondition for ' . $uri . ', repeating request with If-Match: ' . $etag));
+
+        try {
+            /** @var \GuzzleHttp\Psr7\Response $repeated */
+            $repeated = $httpclient->send(
+                $this->createProxyRequest($request, $uri, $auth, $body)->withHeader('If-Match', $etag),
+                $req_opts
+            );
+        } catch (\Exception $e) {
+            $this->xapiproxy->log()->error($this->msg($e->getMessage()));
+            return $response;
+        }
+
+        return ['state' => 'fulfilled', 'value' => $repeated];
+    }
+
+    private function requiresSynthesizedPrecondition(\Psr\Http\Message\RequestInterface $request): bool
+    {
+        return strtoupper($request->getMethod()) === 'PUT'
+            && in_array($this->xapiproxy->cmdParts()[3] ?? '', self::DOCUMENT_RESOURCES, true)
+            && !$request->hasHeader('If-Match')
+            && !$request->hasHeader('If-None-Match');
+    }
+
+    /**
+     * Returns the current ETag of an xAPI document, or an empty string if it does not exist.
+     * Some LRS send an ETag along with the 404 of a missing document, so only a 200 is trusted.
+     * @param array<string, mixed> $req_opts
+     */
+    private function fetchDocumentEtag(
+        Client $httpclient,
+        \Psr\Http\Message\RequestInterface $request,
+        Uri $uri,
+        string $auth,
+        array $req_opts
+    ): string {
+        $headers = ['Authorization' => $auth];
+        if ($request->hasHeader('X-Experience-API-Version')) {
+            $headers['X-Experience-API-Version'] = $request->getHeader('X-Experience-API-Version');
+        }
+
+        try {
+            $probe = $httpclient->send(new Request('GET', $uri, $headers), $req_opts);
+        } catch (\Exception $e) {
+            $this->xapiproxy->log()->error($this->msg($e->getMessage()));
+            return '';
+        }
+
+        return $probe->getStatusCode() === 200 ? $probe->getHeaderLine('ETag') : '';
+    }
 }

components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php

@@ -26,6 +26,14 @@
 
 class XapiProxyResponse
 {
+    /**
+     * 304, 409 and 412 are the conditional answers of the xAPI document resources. They are
+     * regular protocol answers which the content has to evaluate on its own, so they are
+     * relayed unchanged instead of being replaced by a generic proxy error.
+     * @var list<int>
+     */
+    private const RELAYED_STATUS_CODES = [200, 204, 304, 404, 409, 412];
+
     //        private $dic;
     private XapiProxy $xapiproxy;
     //private $xapiProxyRequest;
@@ -40,7 +48,7 @@ public function checkResponse(array $response, string $endpoint): bool
     {
         if ($response['state'] == 'fulfilled') {
             $status = $response['value']->getStatusCode();
-            if ($status === 200 || $status === 204 || $status === 404) {
+            if (in_array($status, self::RELAYED_STATUS_CODES, true)) {
                 return true;
             } else {
                 $this->xapiproxy->log()->error("LRS error {$endpoint}: " . $response['value']->getBody());
@@ -241,6 +249,9 @@ public function emit(\GuzzleHttp\Psr7\Response $response): void
             }
         }
 
+        // the content may only read the relayed ETag of a document if it is exposed
+        header('Access-Control-Expose-Headers: ETag, Last-Modified, X-Experience-API-Version', true, $statusCode);
+
         // statusline
         header(sprintf(
             'HTTP/%s %d%s',

components/ILIAS/CmiXapi/resources/xapiproxy.php

@@ -35,7 +35,7 @@
     header('Access-Control-Allow-Origin: ' . $_SERVER["HTTP_ORIGIN"]);
     header('Access-Control-Allow-Credentials: true');
     header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS');
-    header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Modified-Since,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With');
+    header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Match,If-Modified-Since,If-None-Match,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With');
     exit;
 }
 
@@ -54,7 +54,7 @@
     header('Access-Control-Allow-Origin: ' . $_SERVER["HTTP_ORIGIN"]);
     header('Access-Control-Allow-Credentials: true');
     header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS');
-    header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Modified-Since,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With');
+    header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Match,If-Modified-Since,If-None-Match,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With');
     exit;
 }
 

components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php

@@ -0,0 +1,77 @@
+<?php
+
+/**
+ * This file is part of ILIAS, a powerful learning management system
+ * published by ILIAS open source e-Learning e.V.
+ *
+ * ILIAS is licensed with the GPL-3.0,
+ * see https://www.gnu.org/licenses/gpl-3.0.en.html
+ * You should have received a copy of said license along with the
+ * source code, too.
+ *
+ * If this is not the case or you just want to try ILIAS, you'll find
+ * us at:
+ * https://www.ilias.de
+ * https://github.com/ILIAS-eLearning
+ *
+ *********************************************************************/
+
+declare(strict_types=1);
+
+use GuzzleHttp\Psr7\Request;
+use ILIAS\DI\Container;
+use PHPUnit\Framework\TestCase;
+use XapiProxy\XapiProxy;
+use XapiProxy\XapiProxyRequest;
+
+class XapiProxyRequestTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        $GLOBALS['DIC'] = new Container();
+    }
+
+    /**
+     * @param array<string, string> $headers
+     * @dataProvider preconditionCases
+     */
+    public function testPreconditionIsOnlySynthesizedForUnconditionalDocumentWrites(
+        string $method,
+        string $resource,
+        array $headers,
+        bool $expected
+    ): void {
+        // XapiProxy declares its own method(), so the stub is configured through expects()
+        $proxy = $this->createMock(XapiProxy::class);
+        $proxy->expects($this->any())->method('cmdParts')->willReturn(['', '', '', $resource, '']);
+
+        $method_under_test = new ReflectionMethod(XapiProxyRequest::class, 'requiresSynthesizedPrecondition');
+        $method_under_test->setAccessible(true);
+
+        $this->assertSame(
+            $expected,
+            $method_under_test->invoke(
+                new XapiProxyRequest($proxy),
+                new Request($method, 'https://ilias.example.org/xapiproxy.php/' . $resource, $headers)
+            )
+        );
+    }
+
+    /**
+     * @return array<string, array{0: string, 1: string, 2: array<string, string>, 3: bool}>
+     */
+    public static function preconditionCases(): array
+    {
+        return [
+            'unconditional state write' => ['PUT', 'activities/state', [], true],
+            'unconditional activity profile write' => ['PUT', 'activities/profile', [], true],
+            'unconditional agent profile write' => ['PUT', 'agents/profile', [], true],
+            'client sends If-Match' => ['PUT', 'activities/state', ['If-Match' => '"abc"'], false],
+            'client sends If-None-Match' => ['PUT', 'activities/state', ['If-None-Match' => '*'], false],
+            'merging POST is not guarded' => ['POST', 'activities/state', [], false],
+            'document read is not guarded' => ['GET', 'activities/state', [], false],
+            'document delete is not guarded' => ['DELETE', 'activities/state', [], false],
+            'statements are immutable' => ['PUT', 'statements', [], false],
+        ];
+    }
+}

components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php

@@ -0,0 +1,77 @@
+<?php
+
+/**
+ * This file is part of ILIAS, a powerful learning management system
+ * published by ILIAS open source e-Learning e.V.
+ *
+ * ILIAS is licensed with the GPL-3.0,
+ * see https://www.gnu.org/licenses/gpl-3.0.en.html
+ * You should have received a copy of said license along with the
+ * source code, too.
+ *
+ * If this is not the case or you just want to try ILIAS, you'll find
+ * us at:
+ * https://www.ilias.de
+ * https://github.com/ILIAS-eLearning
+ *
+ *********************************************************************/
+
+declare(strict_types=1);
+
+use GuzzleHttp\Psr7\Response;
+use PHPUnit\Framework\TestCase;
+use XapiProxy\XapiProxy;
+use XapiProxy\XapiProxyResponse;
+
+class XapiProxyResponseTest extends TestCase
+{
+    /**
+     * @dataProvider statusCodes
+     */
+    public function testConditionalAnswersAreRelayedInsteadOfBeingReplacedByAProxyError(
+        int $status,
+        bool $expected
+    ): void {
+        // XapiProxy declares its own method(), so the stub is configured through expects()
+        $proxy = $this->createMock(XapiProxy::class);
+        $proxy->expects($this->any())->method('log')->willReturn($this->createMock(ilLogger::class));
+
+        $this->assertSame(
+            $expected,
+            (new XapiProxyResponse($proxy))->checkResponse(
+                ['state' => 'fulfilled', 'value' => new Response($status)],
+                'https://lrs.example.org/xapi'
+            )
+        );
+    }
+
+    /**
+     * @return array<string, array{0: int, 1: bool}>
+     */
+    public static function statusCodes(): array
+    {
+        return [
+            'ok' => [200, true],
+            'no content' => [204, true],
+            'not modified' => [304, true],
+            'document does not exist' => [404, true],
+            'precondition required by the lrs' => [409, true],
+            'precondition failed' => [412, true],
+            'bad request' => [400, false],
+            'server error' => [500, false],
+        ];
+    }
+
+    public function testConnectionErrorsRemainAnError(): void
+    {
+        $proxy = $this->createMock(XapiProxy::class);
+        $proxy->expects($this->any())->method('log')->willReturn($this->createMock(ilLogger::class));
+
+        $this->assertFalse(
+            (new XapiProxyResponse($proxy))->checkResponse(
+                ['state' => 'rejected', 'reason' => new Exception('connection refused')],
+                'https://lrs.example.org/xapi'
+            )
+        );
+    }
+}