Skip to content

Commit 1688ce8

Browse files
marvimarvmarvimarv
committed
[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.
1 parent 71da519 commit 1688ce8

5 files changed

Lines changed: 303 additions & 6 deletions

File tree

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

Lines changed: 133 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
<?php
22

3-
declare(strict_types=1);
4-
53
/**
64
* This file is part of ILIAS, a powerful learning management system
75
* published by ILIAS open source e-Learning e.V.
@@ -18,6 +16,8 @@
1816
*
1917
*********************************************************************/
2018

19+
declare(strict_types=1);
20+
2121
namespace XapiProxy;
2222

2323
use GuzzleHttp\Client;
@@ -38,6 +38,20 @@ class XapiProxyRequest
3838
private string $cmdPart2plus = "";
3939
private bool $checkGetStatements = true;
4040

41+
/**
42+
* The xAPI document resources are mutable, so the specification guards them with optimistic
43+
* concurrency (xAPI 1.0.3, "Concurrency"): a content player announces the document revision
44+
* it expects, and an LRS must reject a PUT on an already existing document that carries
45+
* neither of these headers with 409 Conflict.
46+
* @var list<string>
47+
*/
48+
private const CONDITIONAL_REQUEST_HEADERS = ['If-Match', 'If-None-Match'];
49+
50+
/**
51+
* @var list<string>
52+
*/
53+
private const DOCUMENT_RESOURCES = ['activities/state', 'activities/profile', 'agents/profile'];
54+
4155
public function __construct(XapiProxy $xapiproxy)
4256
{
4357
$this->dic = $GLOBALS['DIC'];
@@ -279,6 +293,25 @@ private function handleProxy(\Psr\Http\Message\RequestInterface $request, $fakeP
279293
$this->xapiproxy->log()->error($this->msg($e->getMessage()));
280294
}
281295

296+
$responses['default'] = $this->repeatDocumentWriteWithPrecondition(
297+
$httpclient,
298+
$request,
299+
$responses['default'],
300+
$uriDefault,
301+
$authDefault,
302+
$body,
303+
$req_opts
304+
);
305+
$responses['fallback'] = $this->repeatDocumentWriteWithPrecondition(
306+
$httpclient,
307+
$request,
308+
$responses['fallback'],
309+
$uriFallback,
310+
$authFallback,
311+
$body,
312+
$req_opts
313+
);
314+
282315
$defaultOk = $this->xapiProxyResponse->checkResponse($responses['default'], $endpointDefault);
283316
$fallbackOk = $this->xapiProxyResponse->checkResponse($responses['fallback'], $endpointFallback);
284317

@@ -318,6 +351,16 @@ private function handleProxy(\Psr\Http\Message\RequestInterface $request, $fakeP
318351
} catch (\Exception $e) {
319352
$this->xapiproxy->log()->error($this->msg($e->getMessage()));
320353
}
354+
$responses['default'] = $this->repeatDocumentWriteWithPrecondition(
355+
$httpclient,
356+
$request,
357+
$responses['default'],
358+
$uriDefault,
359+
$authDefault,
360+
$body,
361+
$req_opts
362+
);
363+
321364
if ($this->xapiProxyResponse->checkResponse($responses['default'], $endpointDefault)) {
322365
try {
323366
$this->xapiProxyResponse->handleResponse(
@@ -372,10 +415,98 @@ private function createProxyRequest(\Psr\Http\Message\RequestInterface $request,
372415
$headers['Connection'] = $request->getHeader('Connection');
373416
}
374417

418+
foreach (self::CONDITIONAL_REQUEST_HEADERS as $conditionalHeader) {
419+
if ($request->hasHeader($conditionalHeader)) {
420+
$headers[$conditionalHeader] = $request->getHeader($conditionalHeader);
421+
}
422+
}
423+
375424
//$this->xapiproxy->log()->debug($this->msg($body));
376425

377426
$req = new Request(strtoupper($request->getMethod()), $uri, $headers, $body);
378427

379428
return $req;
380429
}
430+
431+
/**
432+
* TinCanJS, which is bundled with common content players, only sends If-Match on a state
433+
* write when the caller supplied the SHA1 of the document it read before, and no precondition
434+
* at all otherwise. A specification compliant LRS answers such a write with 409 Conflict. In
435+
* that case the current ETag is looked up and the write is repeated once with an If-Match
436+
* built from it. An LRS that does not demand a precondition never answers 409 and therefore
437+
* never causes the additional roundtrip.
438+
* @param array{state: string, value?: \GuzzleHttp\Psr7\Response, reason?: mixed} $response
439+
* @param array<string, mixed> $req_opts
440+
* @return array{state: string, value?: \GuzzleHttp\Psr7\Response, reason?: mixed}
441+
*/
442+
private function repeatDocumentWriteWithPrecondition(
443+
Client $httpclient,
444+
\Psr\Http\Message\RequestInterface $request,
445+
array $response,
446+
Uri $uri,
447+
string $auth,
448+
string $body,
449+
array $req_opts
450+
): array {
451+
if ($response['state'] !== 'fulfilled' || $response['value']->getStatusCode() !== 409) {
452+
return $response;
453+
}
454+
if (!$this->requiresSynthesizedPrecondition($request)) {
455+
return $response;
456+
}
457+
$etag = $this->fetchDocumentEtag($httpclient, $request, $uri, $auth, $req_opts);
458+
if ($etag === '') {
459+
return $response;
460+
}
461+
462+
$this->xapiproxy->log()->debug($this->msg('lrs requires a precondition for ' . $uri . ', repeating request with If-Match: ' . $etag));
463+
464+
try {
465+
/** @var \GuzzleHttp\Psr7\Response $repeated */
466+
$repeated = $httpclient->send(
467+
$this->createProxyRequest($request, $uri, $auth, $body)->withHeader('If-Match', $etag),
468+
$req_opts
469+
);
470+
} catch (\Exception $e) {
471+
$this->xapiproxy->log()->error($this->msg($e->getMessage()));
472+
return $response;
473+
}
474+
475+
return ['state' => 'fulfilled', 'value' => $repeated];
476+
}
477+
478+
private function requiresSynthesizedPrecondition(\Psr\Http\Message\RequestInterface $request): bool
479+
{
480+
return strtoupper($request->getMethod()) === 'PUT'
481+
&& in_array($this->xapiproxy->cmdParts()[3] ?? '', self::DOCUMENT_RESOURCES, true)
482+
&& !$request->hasHeader('If-Match')
483+
&& !$request->hasHeader('If-None-Match');
484+
}
485+
486+
/**
487+
* Returns the current ETag of an xAPI document, or an empty string if it does not exist.
488+
* Some LRS send an ETag along with the 404 of a missing document, so only a 200 is trusted.
489+
* @param array<string, mixed> $req_opts
490+
*/
491+
private function fetchDocumentEtag(
492+
Client $httpclient,
493+
\Psr\Http\Message\RequestInterface $request,
494+
Uri $uri,
495+
string $auth,
496+
array $req_opts
497+
): string {
498+
$headers = ['Authorization' => $auth];
499+
if ($request->hasHeader('X-Experience-API-Version')) {
500+
$headers['X-Experience-API-Version'] = $request->getHeader('X-Experience-API-Version');
501+
}
502+
503+
try {
504+
$probe = $httpclient->send(new Request('GET', $uri, $headers), $req_opts);
505+
} catch (\Exception $e) {
506+
$this->xapiproxy->log()->error($this->msg($e->getMessage()));
507+
return '';
508+
}
509+
510+
return $probe->getStatusCode() === 200 ? $probe->getHeaderLine('ETag') : '';
511+
}
381512
}

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
<?php
22

3-
declare(strict_types=1);
43
/**
54
* This file is part of ILIAS, a powerful learning management system
65
* published by ILIAS open source e-Learning e.V.
@@ -17,6 +16,8 @@
1716
*
1817
*********************************************************************/
1918

19+
declare(strict_types=1);
20+
2021
namespace XapiProxy;
2122

2223
use Psr\Http\Message\ServerRequestInterface;
@@ -26,6 +27,14 @@
2627

2728
class XapiProxyResponse
2829
{
30+
/**
31+
* 304, 409 and 412 are the conditional answers of the xAPI document resources. They are
32+
* regular protocol answers which the content has to evaluate on its own, so they are
33+
* relayed unchanged instead of being replaced by a generic proxy error.
34+
* @var list<int>
35+
*/
36+
private const RELAYED_STATUS_CODES = [200, 204, 304, 404, 409, 412];
37+
2938
// private $dic;
3039
private XapiProxy $xapiproxy;
3140
//private $xapiProxyRequest;
@@ -40,7 +49,7 @@ public function checkResponse(array $response, string $endpoint): bool
4049
{
4150
if ($response['state'] == 'fulfilled') {
4251
$status = $response['value']->getStatusCode();
43-
if ($status === 200 || $status === 204 || $status === 404) {
52+
if (in_array($status, self::RELAYED_STATUS_CODES, true)) {
4453
return true;
4554
} else {
4655
$this->xapiproxy->log()->error("LRS error {$endpoint}: " . $response['value']->getBody());
@@ -241,6 +250,9 @@ public function emit(\GuzzleHttp\Psr7\Response $response): void
241250
}
242251
}
243252

253+
// the content may only read the relayed ETag of a document if it is exposed
254+
header('Access-Control-Expose-Headers: ETag, Last-Modified, X-Experience-API-Version', true, $statusCode);
255+
244256
// statusline
245257
header(sprintf(
246258
'HTTP/%s %d%s',

‎components/ILIAS/CmiXapi/resources/xapiproxy.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
header('Access-Control-Allow-Origin: ' . $_SERVER["HTTP_ORIGIN"]);
3636
header('Access-Control-Allow-Credentials: true');
3737
header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS');
38-
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');
38+
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');
3939
exit;
4040
}
4141

@@ -54,7 +54,7 @@
5454
header('Access-Control-Allow-Origin: ' . $_SERVER["HTTP_ORIGIN"]);
5555
header('Access-Control-Allow-Credentials: true');
5656
header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS');
57-
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');
57+
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');
5858
exit;
5959
}
6060

‎components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
<?php
2+
3+
/**
4+
* This file is part of ILIAS, a powerful learning management system
5+
* published by ILIAS open source e-Learning e.V.
6+
*
7+
* ILIAS is licensed with the GPL-3.0,
8+
* see https://www.gnu.org/licenses/gpl-3.0.en.html
9+
* You should have received a copy of said license along with the
10+
* source code, too.
11+
*
12+
* If this is not the case or you just want to try ILIAS, you'll find
13+
* us at:
14+
* https://www.ilias.de
15+
* https://github.com/ILIAS-eLearning
16+
*
17+
*********************************************************************/
18+
19+
declare(strict_types=1);
20+
21+
use GuzzleHttp\Psr7\Request;
22+
use ILIAS\DI\Container;
23+
use PHPUnit\Framework\TestCase;
24+
use XapiProxy\XapiProxy;
25+
use XapiProxy\XapiProxyRequest;
26+
27+
class XapiProxyRequestTest extends TestCase
28+
{
29+
protected function setUp(): void
30+
{
31+
$GLOBALS['DIC'] = new Container();
32+
}
33+
34+
/**
35+
* @param array<string, string> $headers
36+
* @dataProvider preconditionCases
37+
*/
38+
public function testPreconditionIsOnlySynthesizedForUnconditionalDocumentWrites(
39+
string $method,
40+
string $resource,
41+
array $headers,
42+
bool $expected
43+
): void {
44+
// XapiProxy declares its own method(), so the stub is configured through expects()
45+
$proxy = $this->createMock(XapiProxy::class);
46+
$proxy->expects($this->any())->method('cmdParts')->willReturn(['', '', '', $resource, '']);
47+
48+
$method_under_test = new ReflectionMethod(XapiProxyRequest::class, 'requiresSynthesizedPrecondition');
49+
$method_under_test->setAccessible(true);
50+
51+
$this->assertSame(
52+
$expected,
53+
$method_under_test->invoke(
54+
new XapiProxyRequest($proxy),
55+
new Request($method, 'https://ilias.example.org/xapiproxy.php/' . $resource, $headers)
56+
)
57+
);
58+
}
59+
60+
/**
61+
* @return array<string, array{0: string, 1: string, 2: array<string, string>, 3: bool}>
62+
*/
63+
public static function preconditionCases(): array
64+
{
65+
return [
66+
'unconditional state write' => ['PUT', 'activities/state', [], true],
67+
'unconditional activity profile write' => ['PUT', 'activities/profile', [], true],
68+
'unconditional agent profile write' => ['PUT', 'agents/profile', [], true],
69+
'client sends If-Match' => ['PUT', 'activities/state', ['If-Match' => '"abc"'], false],
70+
'client sends If-None-Match' => ['PUT', 'activities/state', ['If-None-Match' => '*'], false],
71+
'merging POST is not guarded' => ['POST', 'activities/state', [], false],
72+
'document read is not guarded' => ['GET', 'activities/state', [], false],
73+
'document delete is not guarded' => ['DELETE', 'activities/state', [], false],
74+
'statements are immutable' => ['PUT', 'statements', [], false],
75+
];
76+
}
77+
}

0 commit comments

Comments
 (0)