diff --git a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php index cbb42a9ac46e..d84519cfc60a 100755 --- a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php +++ b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php @@ -1,7 +1,5 @@ + */ + private const CONDITIONAL_REQUEST_HEADERS = ['If-Match', 'If-None-Match']; + + /** + * @var list + */ + 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 $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 $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') : ''; + } } diff --git a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php index 843038b59d67..571ef353ff21 100755 --- a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php +++ b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php @@ -1,6 +1,5 @@ + */ + private const RELAYED_STATUS_CODES = [200, 204, 304, 404, 409, 412]; + // private $dic; private XapiProxy $xapiproxy; //private $xapiProxyRequest; @@ -40,7 +49,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 +250,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', diff --git a/components/ILIAS/CmiXapi/resources/xapiproxy.php b/components/ILIAS/CmiXapi/resources/xapiproxy.php index d86acf0a20c1..34d019a3ca06 100755 --- a/components/ILIAS/CmiXapi/resources/xapiproxy.php +++ b/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; } diff --git a/components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php b/components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php new file mode 100644 index 000000000000..89b0bf3dec3b --- /dev/null +++ b/components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php @@ -0,0 +1,77 @@ + $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, 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], + ]; + } +} diff --git a/components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php b/components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php new file mode 100644 index 000000000000..b9c85f965d5f --- /dev/null +++ b/components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php @@ -0,0 +1,77 @@ +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 + */ + 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' + ) + ); + } +}