From f6dcaf4a82245971a4a6e16c81ec53186708f974 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 19 Dec 2024 03:19:51 +0100 Subject: [PATCH 01/26] Issue 75: Fix an assertion message. --- tests/src/Kernel/Controller/WopiControllerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Kernel/Controller/WopiControllerTest.php b/tests/src/Kernel/Controller/WopiControllerTest.php index 5af45422..57cc563a 100644 --- a/tests/src/Kernel/Controller/WopiControllerTest.php +++ b/tests/src/Kernel/Controller/WopiControllerTest.php @@ -361,7 +361,7 @@ protected function createAccessToken(?int $fid = NULL, ?int $uid = NULL, bool $w } /** - * Asserts status code and content in a response given a request. + * Asserts a successful json response given a request. * * @param array $expected_data * The expected response JSON data. From e5c909282e63f89296260be2e598dbcc8553237c Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 12 Dec 2024 11:19:17 +0100 Subject: [PATCH 02/26] Issue 75: Let WopiController check if token is NULL. --- src/Controller/WopiController.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Controller/WopiController.php b/src/Controller/WopiController.php index d41351a1..e700ec79 100644 --- a/src/Controller/WopiController.php +++ b/src/Controller/WopiController.php @@ -81,6 +81,9 @@ public static function permissionDenied(): Response { */ public function wopiCheckFileInfo(string $id, Request $request): Response { $token = $request->query->get('access_token'); + if ($token === NULL) { + return static::permissionDenied(); + } $jwt_payload = $this->verifyTokenForMediaId($token, $id); if ($jwt_payload === NULL) { From 66979658a8a4e488f37d2dccff536bcaa64664b9 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 3 Dec 2024 15:08:12 +0100 Subject: [PATCH 03/26] Issue 75: Spell URL uppercase in docs in CollaboraDiscovery*. --- src/Cool/CollaboraDiscovery.php | 2 +- src/Cool/CollaboraDiscoveryInterface.php | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index e11adb2e..fcb4e0b5 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -17,7 +17,7 @@ use Drupal\collabora_online\Exception\CollaboraNotAvailableException; /** - * Service to get a WOPI client url for a given MIME type. + * Service to get a WOPI client URL for a given MIME type. */ class CollaboraDiscovery implements CollaboraDiscoveryInterface { diff --git a/src/Cool/CollaboraDiscoveryInterface.php b/src/Cool/CollaboraDiscoveryInterface.php index b8aae932..4661cda1 100644 --- a/src/Cool/CollaboraDiscoveryInterface.php +++ b/src/Cool/CollaboraDiscoveryInterface.php @@ -15,7 +15,7 @@ namespace Drupal\collabora_online\Cool; /** - * Service to get a WOPI client url for a given MIME type. + * Service to get a WOPI client URL for a given MIME type. */ interface CollaboraDiscoveryInterface { @@ -23,14 +23,14 @@ interface CollaboraDiscoveryInterface { * Gets the URL for the WOPI client. * * @param string $mimetype - * Mime type for which to get the WOPI client url. + * Mime type for which to get the WOPI client URL. * This refers to config entries in the discovery.xml file. * * @return string - * The WOPI client url. + * The WOPI client URL. * * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException - * The client url cannot be retrieved. + * The client URL cannot be retrieved. */ public function getWopiClientURL(string $mimetype = 'text/plain'): string; From dcf431ab9cbd34ef34f89130a69d56c278de181c Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 21 Nov 2024 17:36:10 +0100 Subject: [PATCH 04/26] Issue 75: Extract getParsedXml() from getWopiClientUrl(). --- src/Cool/CollaboraDiscovery.php | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index fcb4e0b5..bb04b487 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -35,12 +35,7 @@ public function __construct( * {@inheritdoc} */ public function getWopiClientURL(string $mimetype = 'text/plain'): string { - $xml = $this->discoveryFetcher->getDiscoveryXml(); - - $discovery_parsed = simplexml_load_string($xml); - if (!$discovery_parsed) { - throw new CollaboraNotAvailableException('The retrieved discovery.xml file is not a valid XML file.'); - } + $discovery_parsed = $this->getParsedXml(); $result = $discovery_parsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', $mimetype)); if (empty($result[0]['urlsrc'][0])) { @@ -50,4 +45,24 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { return (string) $result[0]['urlsrc'][0]; } + /** + * Fetches the discovery.xml, and gets the parsed contents. + * + * @return \SimpleXMLElement + * Parsed xml from the discovery.xml. + * + * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException + * Fetching the discovery.xml failed, or the result is not valid xml. + */ + protected function getParsedXml(): \SimpleXMLElement { + $xml = $this->discoveryFetcher->getDiscoveryXml(); + + $discovery_parsed = simplexml_load_string($xml); + if (!$discovery_parsed) { + throw new CollaboraNotAvailableException('The retrieved discovery.xml file is not a valid XML file.'); + } + + return $discovery_parsed; + } + } From 8265c3ad8c881c08e7d5f8cfbabd568ec06039a5 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 17 Dec 2024 18:44:47 +0100 Subject: [PATCH 05/26] Issue 75: Extract parseXml() from getWopiClientURL() in CollaboraDiscovery. --- src/Cool/CollaboraDiscovery.php | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index bb04b487..6a1dfe85 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -56,13 +56,27 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { */ protected function getParsedXml(): \SimpleXMLElement { $xml = $this->discoveryFetcher->getDiscoveryXml(); + return $this->parseXml($xml); + } - $discovery_parsed = simplexml_load_string($xml); - if (!$discovery_parsed) { + /** + * Parses an XML string. + * + * @param string $xml + * XML string. + * + * @return \SimpleXMLElement + * Parsed XML. + * + * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException + * The XML is invalid or empty. + */ + protected function parseXml(string $xml): \SimpleXMLElement { + $parsed_xml = simplexml_load_string($xml); + if (!$parsed_xml) { throw new CollaboraNotAvailableException('The retrieved discovery.xml file is not a valid XML file.'); } - - return $discovery_parsed; + return $parsed_xml; } } From 9125d29c33f057b41b47bb53390088b03b22466f Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 13 Dec 2024 00:04:42 +0100 Subject: [PATCH 06/26] Issue 75: Intercept errors from XML parsing. --- composer.json | 3 ++- src/Cool/CollaboraDiscovery.php | 21 ++++++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/composer.json b/composer.json index 80947b68..a99c5409 100644 --- a/composer.json +++ b/composer.json @@ -12,7 +12,8 @@ "require": { "php": ">=8.1", "drupal/key": "^1.19", - "firebase/php-jwt": "^6.10" + "firebase/php-jwt": "^6.10", + "symfony/error-handler": "^6.4|^7.1" }, "require-dev": { "composer/installers": "^2", diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 6a1dfe85..5bb670cc 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -15,6 +15,7 @@ namespace Drupal\collabora_online\Cool; use Drupal\collabora_online\Exception\CollaboraNotAvailableException; +use Symfony\Component\ErrorHandler\ErrorHandler; /** * Service to get a WOPI client URL for a given MIME type. @@ -72,9 +73,23 @@ protected function getParsedXml(): \SimpleXMLElement { * The XML is invalid or empty. */ protected function parseXml(string $xml): \SimpleXMLElement { - $parsed_xml = simplexml_load_string($xml); - if (!$parsed_xml) { - throw new CollaboraNotAvailableException('The retrieved discovery.xml file is not a valid XML file.'); + try { + // Avoid errors from XML parsing hitting the regular error handler. + // An alternative would be libxml_use_internal_errors(), but then we would + // have to deal with the results from libxml_get_errors(). + $parsed_xml = ErrorHandler::call( + fn () => simplexml_load_string($xml), + ); + } + catch (\ErrorException $e) { + throw new CollaboraNotAvailableException('Error in the retrieved discovery.xml file: ' . $e->getMessage(), previous: $e); + } + if ($parsed_xml === FALSE) { + // The parser returned FALSE, but no error was raised. + // This is known to happen when $xml is an empty string. + // Instead we could check for $xml === '' earlier, but we don't know for + // sure if this is, and always will be, the only such case. + throw new CollaboraNotAvailableException('The discovery.xml file is empty.'); } return $parsed_xml; } From 00911b5c2fe66b6f2361eb01e9fcd92c82779c9b Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 12 Dec 2024 23:55:18 +0100 Subject: [PATCH 07/26] Issue 75: Add CollaboraDiscoveryTest. --- tests/fixtures/discovery.mimetypes.xml | 14 ++++ tests/src/Unit/CollaboraDiscoveryTest.php | 96 +++++++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 tests/fixtures/discovery.mimetypes.xml create mode 100644 tests/src/Unit/CollaboraDiscoveryTest.php diff --git a/tests/fixtures/discovery.mimetypes.xml b/tests/fixtures/discovery.mimetypes.xml new file mode 100644 index 00000000..e8458fa6 --- /dev/null +++ b/tests/fixtures/discovery.mimetypes.xml @@ -0,0 +1,14 @@ + + + + + + + + + + + + + + diff --git a/tests/src/Unit/CollaboraDiscoveryTest.php b/tests/src/Unit/CollaboraDiscoveryTest.php new file mode 100644 index 00000000..b10bb5fc --- /dev/null +++ b/tests/src/Unit/CollaboraDiscoveryTest.php @@ -0,0 +1,96 @@ +getDiscoveryFromFile($file); + $this->assertSame( + 'http://collabora.test:9980/browser/61cf2b4/cool.html?', + $discovery->getWopiClientURL(), + ); + $this->assertSame( + 'http://spreadsheet.collabora.test:9980/browser/61cf2b4/cool.html?', + $discovery->getWopiClientURL('text/spreadsheet'), + ); + // Test unknown mime type. + $this->expectException(CollaboraNotAvailableException::class); + $this->expectExceptionMessage('The requested mime type is not handled.'); + $discovery->getWopiClientURL('text/unknown'); + } + + /** + * Tests error behavior for blank xml content. + * + * @covers ::getWopiClientURL + */ + public function testBlankXml(): void { + $discovery = $this->getDiscoveryFromXml(''); + + $this->expectException(CollaboraNotAvailableException::class); + $this->expectExceptionMessage('The discovery.xml file is empty.'); + $discovery->getWopiClientURL(); + } + + /** + * Tests error behavior for malformed xml content. + * + * @covers ::getWopiClientURL + */ + public function testBrokenXml(): void { + $xml = 'This file does not contain valid xml.'; + $discovery = $this->getDiscoveryFromXml($xml); + + $this->expectException(CollaboraNotAvailableException::class); + $this->expectExceptionMessageMatches('#^Error in the retrieved discovery.xml file: #'); + $discovery->getWopiClientURL(); + } + + /** + * Gets a discovery instance based on a test xml file. + * + * @param string $file + * A test xml file. + * + * @return \Drupal\collabora_online\Cool\CollaboraDiscoveryInterface + * Discovery instance. + */ + protected function getDiscoveryFromFile(string $file): CollaboraDiscoveryInterface { + $xml = file_get_contents($file); + return $this->getDiscoveryFromXml($xml); + } + + /** + * Gets a discovery instance based on test xml. + * + * @param string $xml + * Explicit XML content. + * + * @return \Drupal\collabora_online\Cool\CollaboraDiscoveryInterface + * Discovery instance. + */ + protected function getDiscoveryFromXml(string $xml): CollaboraDiscoveryInterface { + $fetcher = $this->createMock(CollaboraDiscoveryFetcherInterface::class); + $fetcher->method('getDiscoveryXml')->willReturn($xml); + return new CollaboraDiscovery($fetcher); + } + +} From 5dd05c98adeb031e8e5b7e66a9e5e5cd22d9fd7c Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 13 Dec 2024 00:10:17 +0100 Subject: [PATCH 08/26] Issue 75: Add proof key methods in CollaboraDiscovery. --- src/Cool/CollaboraDiscovery.php | 18 +++++++ src/Cool/CollaboraDiscoveryInterface.php | 19 +++++++ tests/fixtures/discovery.proof-key.xml | 11 ++++ tests/src/Unit/CollaboraDiscoveryTest.php | 65 +++++++++++++++++++++-- 4 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 tests/fixtures/discovery.proof-key.xml diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 5bb670cc..372183e4 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -46,6 +46,24 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { return (string) $result[0]['urlsrc'][0]; } + /** + * {@inheritdoc} + */ + public function getProofKey(): ?string { + $discovery_parsed = $this->getParsedXml(); + $attribute = $discovery_parsed->xpath('/wopi-discovery/proof-key/@value')[0] ?? NULL; + return $attribute?->__toString(); + } + + /** + * {@inheritdoc} + */ + public function getProofKeyOld(): ?string { + $discovery_parsed = $this->getParsedXml(); + $attribute = $discovery_parsed->xpath('/wopi-discovery/proof-key/@oldvalue')[0] ?? NULL; + return $attribute?->__toString(); + } + /** * Fetches the discovery.xml, and gets the parsed contents. * diff --git a/src/Cool/CollaboraDiscoveryInterface.php b/src/Cool/CollaboraDiscoveryInterface.php index 4661cda1..e9472c8d 100644 --- a/src/Cool/CollaboraDiscoveryInterface.php +++ b/src/Cool/CollaboraDiscoveryInterface.php @@ -34,4 +34,23 @@ interface CollaboraDiscoveryInterface { */ public function getWopiClientURL(string $mimetype = 'text/plain'): string; + /** + * Gets the public key used for proofing. + * + * @return string|null + * The recent key, or NULL if none found. + */ + public function getProofKey(): ?string; + + /** + * Gets the old public key for proofing. + * + * This covers the case when the public key was already updated, but an + * incoming request has a proof that was generated with the previous key. + * + * @return string|null + * The old key, or NULL if none found. + */ + public function getProofKeyOld(): ?string; + } diff --git a/tests/fixtures/discovery.proof-key.xml b/tests/fixtures/discovery.proof-key.xml new file mode 100644 index 00000000..0dd9f537 --- /dev/null +++ b/tests/fixtures/discovery.proof-key.xml @@ -0,0 +1,11 @@ + + + + diff --git a/tests/src/Unit/CollaboraDiscoveryTest.php b/tests/src/Unit/CollaboraDiscoveryTest.php index b10bb5fc..74bc43af 100644 --- a/tests/src/Unit/CollaboraDiscoveryTest.php +++ b/tests/src/Unit/CollaboraDiscoveryTest.php @@ -37,31 +37,88 @@ public function testWopiClientUrl(): void { $discovery->getWopiClientURL('text/unknown'); } + /** + * Tests reading proof keys from the discovery.xml. + * + * @covers ::getProofKey + * @covers ::getProofKeyOld + */ + public function testProofKey(): void { + $file = dirname(__DIR__, 2) . '/fixtures/discovery.proof-key.xml'; + $discovery = $this->getDiscoveryFromFile($file); + $this->assertSame( + 'BgIAAACkAABSU0ExAAgAAAEAAQCxJYuefceQ4XI3/iUQvL9+ebLFZSRdM1n6fkB+OtILJexHUsD/aItTWgzB/G6brdxLlyHXoPjbJl4QoWZVrr1XY+ZHQ/a9Yzf/VN2mPLKFB9hmnUPI580VpFfkC3gCgpqwFwMpAkQSzYSDFQ/7W4ryPP6irvVzhg16IqQ9oEhZWmwy6caKcqh4BK31oI8SrI6bsZLBMTli70197UWHmgIGk4JJbeC8cBFb6uZDaidAcRn1HSAF2JnaEscUNMIsiNMM/71BT6U6hVSv5Qk0oISMLfVOeCPQZ6OmYo4M42wDKBpaJGMOpgoeQX6Feq+agf7uBvd8S/ITGZ8WinQfHZaQ', + $discovery->getProofKey(), + ); + $this->assertSame( + 'BgIAAACkAABSU0ExAAgAAAEAAQDj9QjZQ9bOOw5LfAMxMLMDTLgHsNvBcdRpYQ8S9qK9ylJevgp+j66k9/uyKXSwI9WTVHW+XLTCPq6aId+XqB5e8+H5rov7e4Itkpnr6eXZ1jAu9TW2jEnqCYdGqG6Pv0kbRv1gUFEsjciy8i9UAQ12Ons7J58nQLd3tJ4WATANoCyVJLfA7BQ6IRSq8/K3jqmSE8xu3HDLX+lnMrsK2KL4lYcjerGZpmOKI5tPZbC5xSMkB9alE5NhTYeYw25CyG4FHoss2AwNgvSQDaf6d/icNg5ZoGQwtISGKL6IFc4oogFHFdvR4FQCQ61wdz7RmHjJUpsPFio8htuSeMjbC7fS', + $discovery->getProofKeyOld(), + ); + } + + /** + * Tests behavior if discovery.xml does not have proof keys. + * + * @covers ::getProofKey + * @covers ::getProofKeyOld + */ + public function testNoProofKey(): void { + $xml = ''; + $discovery = $this->getDiscoveryFromXml($xml); + $this->assertNull($discovery->getProofKey()); + $this->assertNull($discovery->getProofKeyOld()); + } + /** * Tests error behavior for blank xml content. * * @covers ::getWopiClientURL + * @covers ::getProofKey + * @covers ::getProofKeyOld + * + * @dataProvider provideAllMethods */ - public function testBlankXml(): void { + public function testBlankXml(callable $callback): void { $discovery = $this->getDiscoveryFromXml(''); $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessage('The discovery.xml file is empty.'); - $discovery->getWopiClientURL(); + $callback($discovery); } /** * Tests error behavior for malformed xml content. * * @covers ::getWopiClientURL + * @covers ::getProofKey + * @covers ::getProofKeyOld + * + * @dataProvider provideAllMethods */ - public function testBrokenXml(): void { + public function testBrokenXml(callable $callback): void { $xml = 'This file does not contain valid xml.'; $discovery = $this->getDiscoveryFromXml($xml); $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessageMatches('#^Error in the retrieved discovery.xml file: #'); - $discovery->getWopiClientURL(); + $callback($discovery); + } + + /** + * Provides all methods as callbacks. + * + * This is used for tests where each method will throw the same exception. + * + * @return list + * Argument tuples. + */ + public static function provideAllMethods(): array { + return [ + // Closures in data providers are ok in unit tests. + 'getWopiClientURL' => [fn (CollaboraDiscoveryInterface $discovery) => $discovery->getWopiClientURL()], + 'getProofKey' => [fn (CollaboraDiscoveryInterface $discovery) => $discovery->getProofKey()], + 'getProofKeyOld' => [fn (CollaboraDiscoveryInterface $discovery) => $discovery->getProofKeyOld()], + ]; } /** From fca29b81c534d5751de866c78760d5d7a3baa34e Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 17 Dec 2024 19:35:13 +0100 Subject: [PATCH 09/26] Issue 75: Set custom response format for WOPI routes. This will allow to use the regular route access layer for proofing and authorization. --- collabora_online.routing.yml | 3 ++ collabora_online.services.yml | 1 + .../ExceptionWopiSubscriber.php | 54 +++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 src/EventSubscriber/ExceptionWopiSubscriber.php diff --git a/collabora_online.routing.yml b/collabora_online.routing.yml index d0619ee0..ea60d3ed 100644 --- a/collabora_online.routing.yml +++ b/collabora_online.routing.yml @@ -48,6 +48,7 @@ collabora-online.wopi.info: methods: [ GET ] requirements: _permission: 'access content' + _format: 'collabora_online_wopi' options: parameters: action: @@ -63,6 +64,7 @@ collabora-online.wopi.contents: methods: [ GET ] requirements: _permission: 'access content' + _format: 'collabora_online_wopi' options: parameters: action: @@ -78,6 +80,7 @@ collabora-online.wopi.save: methods: [ POST ] requirements: _permission: 'access content' + _format: 'collabora_online_wopi' options: parameters: action: diff --git a/collabora_online.services.yml b/collabora_online.services.yml index 98e6eaf1..946e2be1 100644 --- a/collabora_online.services.yml +++ b/collabora_online.services.yml @@ -12,3 +12,4 @@ services: class: Drupal\collabora_online\Jwt\JwtTranscoder Drupal\collabora_online\MediaHelperInterface: class: Drupal\collabora_online\MediaHelper + Drupal\collabora_online\EventSubscriber\ExceptionWopiSubscriber: { } diff --git a/src/EventSubscriber/ExceptionWopiSubscriber.php b/src/EventSubscriber/ExceptionWopiSubscriber.php new file mode 100644 index 00000000..70a1f045 --- /dev/null +++ b/src/EventSubscriber/ExceptionWopiSubscriber.php @@ -0,0 +1,54 @@ +getThrowable(); + + // If the exception is cacheable, generate a cacheable response. + if ($exception instanceof CacheableDependencyInterface) { + $response = new CacheableResponse($exception->getMessage(), $exception->getStatusCode(), $exception->getHeaders()); + $response->addCacheableDependency($exception); + } + else { + $response = new Response($exception->getMessage(), $exception->getStatusCode(), $exception->getHeaders()); + } + + $event->setResponse($response); + } + +} From 845564bee81112814b593465e3c26861fe2d4e4b Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 17 Dec 2024 11:29:01 +0100 Subject: [PATCH 10/26] Issue 75: Throw AccessDeniedHttpException in WopiController instead of the custom response. --- src/Controller/WopiController.php | 71 +++++++++---------- .../Kernel/Controller/WopiControllerTest.php | 34 ++++++--- 2 files changed, 58 insertions(+), 47 deletions(-) diff --git a/src/Controller/WopiController.php b/src/Controller/WopiController.php index e700ec79..47cb92b2 100644 --- a/src/Controller/WopiController.php +++ b/src/Controller/WopiController.php @@ -30,6 +30,7 @@ use Symfony\Component\HttpFoundation\BinaryFileResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; /** * Provides WOPI route responses for the Collabora module. @@ -54,20 +55,6 @@ public function __construct( $this->setStringTranslation($string_translation); } - /** - * Creates a failure response that is understood by Collabora. - * - * @return \Symfony\Component\HttpFoundation\Response - * Response object. - */ - public static function permissionDenied(): Response { - return new Response( - 'Authentication failed.', - Response::HTTP_FORBIDDEN, - ['content-type' => 'text/plain'], - ); - } - /** * Handles the WOPI 'info' request for a media entity. * @@ -82,23 +69,20 @@ public static function permissionDenied(): Response { public function wopiCheckFileInfo(string $id, Request $request): Response { $token = $request->query->get('access_token'); if ($token === NULL) { - return static::permissionDenied(); + throw new AccessDeniedHttpException('Missing access token.'); } $jwt_payload = $this->verifyTokenForMediaId($token, $id); - if ($jwt_payload === NULL) { - return static::permissionDenied(); - } /** @var \Drupal\media\MediaInterface|null $media */ $media = $this->entityTypeManager->getStorage('media')->load($id); if ($media === NULL) { - return static::permissionDenied(); + throw new AccessDeniedHttpException('Media not found.'); } $file = $this->mediaHelper->getFileForMedia($media); if ($file === NULL) { - return static::permissionDenied(); + throw new AccessDeniedHttpException('No file attached to media.'); } $mtime = date_create_immutable_from_format('U', $file->getChangedTime()); @@ -106,13 +90,13 @@ public function wopiCheckFileInfo(string $id, Request $request): Response { /** @var \Drupal\user\UserInterface|null $user */ $user = $this->entityTypeManager->getStorage('user')->load($jwt_payload['uid']); if ($user === NULL) { - return static::permissionDenied(); + throw new AccessDeniedHttpException('User not found.'); } $can_write = $jwt_payload['wri']; if ($can_write && !$media->access('edit in collabora', $user)) { $this->logger->error('Token and user permissions do not match.'); - return static::permissionDenied(); + throw new AccessDeniedHttpException('The user does not have collabora edit access for this media.'); } $payload = [ @@ -159,9 +143,6 @@ public function wopiGetFile(string $id, Request $request): Response { $token = $request->query->get('access_token'); $jwt_payload = $this->verifyTokenForMediaId($token, $id); - if ($jwt_payload === NULL) { - return static::permissionDenied(); - } /** @var \Drupal\user\UserInterface|null $user */ $user = $this->entityTypeManager->getStorage('user')->load($jwt_payload['uid']); @@ -170,12 +151,12 @@ public function wopiGetFile(string $id, Request $request): Response { /** @var \Drupal\media\MediaInterface|null $media */ $media = $this->entityTypeManager->getStorage('media')->load($id); if ($media === NULL) { - return static::permissionDenied(); + throw new AccessDeniedHttpException('Media not found.'); } $file = $this->mediaHelper->getFileForMedia($media); if ($file === NULL) { - return static::permissionDenied(); + throw new AccessDeniedHttpException('No file attached to media.'); } $mimetype = $file->getMimeType(); @@ -207,16 +188,20 @@ public function wopiPutFile(string $id, Request $request): Response { $exitsave = $request->headers->get('x-cool-wopi-isexitsave') == 'true'; $jwt_payload = $this->verifyTokenForMediaId($token, $id); - if ($jwt_payload == NULL || empty($jwt_payload['wri'])) { - return static::permissionDenied(); + if (empty($jwt_payload['wri'])) { + throw new AccessDeniedHttpException('The token only grants read access.'); } /** @var \Drupal\media\MediaInterface|null $media */ $media = $this->entityTypeManager->getStorage('media')->load($id); + if ($media === NULL) { + throw new AccessDeniedHttpException('Media not found.'); + } + /** @var \Drupal\user\UserInterface|null $user */ $user = $this->entityTypeManager->getStorage('user')->load($jwt_payload['uid']); - if ($media === NULL || $user === NULL) { - return static::permissionDenied(); + if ($user === NULL) { + throw new AccessDeniedHttpException('User not found.'); } $this->accountSwitcher->switchTo($user); @@ -339,27 +324,35 @@ public function wopi(string $action, string $id, Request $request): Response { * Media id expected to be in the token payload. * This could be a stringified integer like '123'. * - * @return array|null - * Data decoded from the token, or NULL on failure or if the token has - * expired. + * @return array + * Data decoded from the token. + * + * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException + * The token is malformed, invalid or has expired. */ protected function verifyTokenForMediaId( #[\SensitiveParameter] string $token, int|string $expected_media_id, - ): array|null { + ): array { try { $values = $this->jwtTranscoder->decode($token); } catch (CollaboraNotAvailableException $e) { $this->logger->warning('A token cannot be decoded: @message', ['@mesage' => $e->getMessage()]); - return NULL; + throw new AccessDeniedHttpException('Malformed token'); } if ($values === NULL) { - return NULL; + throw new AccessDeniedHttpException('Empty token values'); } - if ($values['fid'] !== $expected_media_id) { - return NULL; + if ((string) $values['fid'] !== (string) $expected_media_id) { + throw new AccessDeniedHttpException(sprintf( + // The token payload is not encrypted, just encoded. + // It is ok to reveal its values in the response for logging. + 'Found fid %s in request path, but fid %s in token payload', + $expected_media_id, + $values['fid'], + )); } return $values; } diff --git a/tests/src/Kernel/Controller/WopiControllerTest.php b/tests/src/Kernel/Controller/WopiControllerTest.php index 57cc563a..a8c221cd 100644 --- a/tests/src/Kernel/Controller/WopiControllerTest.php +++ b/tests/src/Kernel/Controller/WopiControllerTest.php @@ -211,7 +211,11 @@ public function testBadToken(): void { foreach ($requests as $name => $request) { // Replace the token with a value that is not in the JWT format. $request->query->set('access_token', 'bad_token'); - $this->assertAccessDeniedResponse($request, $name); + $this->assertAccessDeniedResponse( + 'Empty token values', + $request, + $name, + ); } } @@ -226,7 +230,11 @@ public function testWrongTokenPayload(): void { // Inject a bad value into the token payload. $requests = $this->createRequests(token_payload: ['fid' => 4321]); foreach ($requests as $name => $request) { - $this->assertAccessDeniedResponse($request, $name); + $this->assertAccessDeniedResponse( + sprintf('Found fid %s in request path, but fid 4321 in token payload', $this->media->id()), + $request, + $name, + ); } } @@ -240,7 +248,11 @@ public function testWrongTokenPayload(): void { public function testMediaNotFound(): void { $requests = $this->createRequests(media_id: 555); foreach ($requests as $name => $request) { - $this->assertAccessDeniedResponse($request, $name); + $this->assertAccessDeniedResponse( + 'Media not found.', + $request, + $name, + ); } } @@ -255,7 +267,11 @@ public function testUserNotFound(): void { $requests = $this->createRequests(user_id: 555); unset($requests['file']); foreach ($requests as $name => $request) { - $this->assertAccessDeniedResponse($request, $name); + $this->assertAccessDeniedResponse( + 'User not found.', + $request, + $name, + ); } } @@ -398,18 +414,20 @@ protected function assertJsonResponse(int $expected_code, array $expected_data, /** * Asserts an access denied response given a request. * + * @param string $expected_response_message + * Message expected to be in the response. * @param \Symfony\Component\HttpFoundation\Request $request * The request to perform. - * @param string $message + * @param string $assertion_message * Message to distinguish this from other assertions. */ - protected function assertAccessDeniedResponse(Request $request, string $message = ''): void { + protected function assertAccessDeniedResponse(string $expected_response_message, Request $request, string $assertion_message = ''): void { $this->assertResponse( Response::HTTP_FORBIDDEN, - 'Authentication failed.', + $expected_response_message, 'text/plain', $request, - $message, + $assertion_message, ); } From 068299f421a560380e59baf9e6cb9a0bf473db58 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 18 Dec 2024 17:22:34 +0100 Subject: [PATCH 11/26] Issue 75: Extract WopiControllerTestBase from WopiControllerTest. --- .../Kernel/Controller/WopiControllerTest.php | 267 +---------------- .../Controller/WopiControllerTestBase.php | 280 ++++++++++++++++++ 2 files changed, 281 insertions(+), 266 deletions(-) create mode 100644 tests/src/Kernel/Controller/WopiControllerTestBase.php diff --git a/tests/src/Kernel/Controller/WopiControllerTest.php b/tests/src/Kernel/Controller/WopiControllerTest.php index a8c221cd..a29dbc8d 100644 --- a/tests/src/Kernel/Controller/WopiControllerTest.php +++ b/tests/src/Kernel/Controller/WopiControllerTest.php @@ -14,85 +14,13 @@ namespace Drupal\Tests\collabora_online\Kernel\Controller; -use ColinODell\PsrTestLogger\TestLogger; -use Drupal\collabora_online\Jwt\JwtTranscoderInterface; -use Drupal\Component\Serialization\Json; -use Drupal\file\Entity\File; -use Drupal\file\FileInterface; -use Drupal\media\MediaInterface; -use Drupal\Tests\collabora_online\Kernel\CollaboraKernelTestBase; -use Drupal\user\UserInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; /** * @coversDefaultClass \Drupal\collabora_online\Controller\WopiController */ -class WopiControllerTest extends CollaboraKernelTestBase { - - /** - * {@inheritdoc} - */ - protected static $modules = [ - 'collabora_online_test', - ]; - - /** - * The user with access to perform operations. - * - * @var \Drupal\user\UserInterface - */ - protected UserInterface $user; - - /** - * The media where to perform operations. - * - * @var \Drupal\media\MediaInterface - */ - protected MediaInterface $media; - - /** - * The source file. - * - * @var \Drupal\file\FileInterface - */ - protected FileInterface $file; - - /** - * The test logger channel. - * - * @var \ColinODell\PsrTestLogger\TestLogger - */ - protected TestLogger $logger; - - /** - * {@inheritdoc} - */ - protected function setUp(): void { - parent::setUp(); - $this->logger = new TestLogger(); - \Drupal::service('logger.factory')->addLogger($this->logger); - - $collabora_settings = \Drupal::configFactory()->getEditable('collabora_online.settings'); - $cool = $collabora_settings->get('cool'); - $cool['key_id'] = 'collabora'; - $collabora_settings->set('cool', $cool)->save(); - - // Make sure that ids for different entity types are distinguishable. - // This will reveal bugs where one id gets mixed up for another. - \Drupal::database()->query("ALTER TABLE {media} AUTO_INCREMENT = 1000"); - \Drupal::database()->query("ALTER TABLE {file_managed} AUTO_INCREMENT = 2000"); - - $this->media = $this->createMediaEntity('document'); - $this->user = $this->createUser([ - 'access content', - 'edit any document in collabora', - ]); - $fid = $this->media->getSource()->getSourceFieldValue($this->media); - $this->file = File::load($fid); - - $this->setCurrentUser($this->user); - } +class WopiControllerTest extends WopiControllerTestBase { /** * Tests successful requests to the 'collabora-online.wopi.info' route. @@ -275,197 +203,4 @@ public function testUserNotFound(): void { } } - /** - * Creates WOPI requests for different routes, with some shared parameters. - * - * This can be used for tests where each route is expected to have the same - * response. - * - * @param int|null $media_id - * Media entity id, if different from the default. - * @param int|null $user_id - * User id, if different from the default. - * @param array $token_payload - * Explicit token payload values. - * This can be used to cause a bad token. - * - * @return array - * Requests keyed by a distinguishable name. - */ - protected function createRequests(?int $media_id = NULL, ?int $user_id = NULL, array $token_payload = []): array { - $create_request = fn (string $uri_suffix, string $method = 'GET', bool $write = FALSE) => $this->createRequest( - $uri_suffix, - $method, - $media_id, - $user_id, - $write, - $token_payload, - ); - return [ - 'info' => $create_request(''), - 'file' => $create_request('/contents'), - 'save' => $create_request('/contents', 'POST', TRUE), - ]; - } - - /** - * Creates a WOPI request. - * - * @param string $uri_suffix - * Suffix to append to the WOPI media url. - * @param string $method - * E.g. 'GET' or 'POST'. - * @param int|null $media_id - * Media entity id, if different from the default. - * @param int|null $user_id - * User id, if different from the default. - * @param bool $write - * TRUE if write access is requested. - * @param array $token_payload - * Explicit token payload values. - * This can be used to cause a bad token. - * - * @return \Symfony\Component\HttpFoundation\Request - * The request. - */ - protected function createRequest( - string $uri_suffix = '', - string $method = 'GET', - ?int $media_id = NULL, - ?int $user_id = NULL, - bool $write = FALSE, - array $token_payload = [], - ): Request { - $media_id ??= (int) $this->media->id(); - $user_id ??= (int) $this->user->id(); - $uri = '/cool/wopi/files/' . $media_id . $uri_suffix; - $token = $this->createAccessToken($media_id, $user_id, $write, $token_payload); - $parameters = [ - 'id' => $media_id, - 'access_token' => $token, - ]; - return Request::create($uri, $method, $parameters); - } - - /** - * Retrieves an encoded access token. - * - * @param int|null $fid - * The file id. - * @param int|null $uid - * The user id. - * @param bool $write - * The write permission. - * @param array $payload - * Explicit payload values. - * This can be used to cause a bad token. - * - * @return string - * The enconded token. - */ - protected function createAccessToken(?int $fid = NULL, ?int $uid = NULL, bool $write = FALSE, array $payload = []): string { - /** @var \Drupal\collabora_online\Jwt\JwtTranscoderInterface $transcoder */ - $transcoder = \Drupal::service(JwtTranscoderInterface::class); - $expire_timestamp = gettimeofday(TRUE) + 1000; - $payload += [ - 'fid' => (string) ($fid ?? $this->media->id()), - 'uid' => (string) ($uid ?? $this->user->id()), - 'wri' => $write, - 'exp' => $expire_timestamp, - ]; - return $transcoder->encode($payload, $expire_timestamp); - } - - /** - * Asserts a successful json response given a request. - * - * @param array $expected_data - * The expected response JSON data. - * @param \Symfony\Component\HttpFoundation\Request $request - * The request to perform. - * @param string $message - * Message to distinguish this from other assertions. - */ - protected function assertJsonResponseOk(array $expected_data, Request $request, string $message = ''): void { - $this->assertJsonResponse(Response::HTTP_OK, $expected_data, $request, $message); - } - - /** - * Asserts a json response given a request. - * - * @param int $expected_code - * The expected response status code. - * @param array $expected_data - * The expected response JSON data. - * @param \Symfony\Component\HttpFoundation\Request $request - * The request to perform. - * @param string $message - * Message to distinguish this from other assertions. - */ - protected function assertJsonResponse(int $expected_code, array $expected_data, Request $request, string $message = ''): void { - $response = $this->handleRequest($request); - $this->assertEquals($expected_code, $response->getStatusCode(), $message); - $this->assertEquals('application/json', $response->headers->get('Content-Type'), $message); - $content = $response->getContent(); - $data = Json::decode($content); - $this->assertSame($expected_data, $data, $message); - } - - /** - * Asserts an access denied response given a request. - * - * @param string $expected_response_message - * Message expected to be in the response. - * @param \Symfony\Component\HttpFoundation\Request $request - * The request to perform. - * @param string $assertion_message - * Message to distinguish this from other assertions. - */ - protected function assertAccessDeniedResponse(string $expected_response_message, Request $request, string $assertion_message = ''): void { - $this->assertResponse( - Response::HTTP_FORBIDDEN, - $expected_response_message, - 'text/plain', - $request, - $assertion_message, - ); - } - - /** - * Asserts status code and content in a response given a request. - * - * @param int $expected_code - * The expected response status code. - * @param string $expected_content - * The expected response content. - * @param string $expected_content_type - * The type of content of the response. - * @param \Symfony\Component\HttpFoundation\Request $request - * The request to perform. - * @param string $message - * Message to distinguish this from other assertions. - */ - protected function assertResponse(int $expected_code, string $expected_content, string $expected_content_type, Request $request, string $message = ''): void { - $response = $this->handleRequest($request); - - $this->assertEquals($expected_code, $response->getStatusCode(), $message); - $this->assertEquals($expected_content, $response->getContent(), $message); - $this->assertEquals($expected_content_type, $response->headers->get('Content-Type'), $message); - } - - /** - * Handles a request and gets the response. - * - * @param \Symfony\Component\HttpFoundation\Request $request - * Incoming request. - * - * @return \Symfony\Component\HttpFoundation\Response - * The response. - */ - protected function handleRequest(Request $request): Response { - /** @var \Symfony\Component\HttpKernel\HttpKernelInterface $kernel */ - $kernel = \Drupal::service('http_kernel'); - return $kernel->handle($request); - } - } diff --git a/tests/src/Kernel/Controller/WopiControllerTestBase.php b/tests/src/Kernel/Controller/WopiControllerTestBase.php new file mode 100644 index 00000000..c65b603c --- /dev/null +++ b/tests/src/Kernel/Controller/WopiControllerTestBase.php @@ -0,0 +1,280 @@ +logger = new TestLogger(); + \Drupal::service('logger.factory')->addLogger($this->logger); + + $collabora_settings = \Drupal::configFactory()->getEditable('collabora_online.settings'); + $cool = $collabora_settings->get('cool'); + $cool['key_id'] = 'collabora'; + $collabora_settings->set('cool', $cool)->save(); + + // Make sure that ids for different entity types are distinguishable. + // This will reveal bugs where one id gets mixed up for another. + \Drupal::database()->query("ALTER TABLE {media} AUTO_INCREMENT = 1000"); + \Drupal::database()->query("ALTER TABLE {file_managed} AUTO_INCREMENT = 2000"); + + $this->media = $this->createMediaEntity('document'); + $this->user = $this->createUser([ + 'access content', + 'edit any document in collabora', + ]); + $fid = $this->media->getSource()->getSourceFieldValue($this->media); + $this->file = File::load($fid); + + $this->setCurrentUser($this->user); + } + + /** + * Creates WOPI requests for different routes, with some shared parameters. + * + * This can be used for tests where each route is expected to have the same + * response. + * + * @param int|null $media_id + * Media entity id, if different from the default. + * @param int|null $user_id + * User id, if different from the default. + * @param array $token_payload + * Explicit token payload values. + * This can be used to cause a bad token. + * + * @return array + * Requests keyed by a distinguishable name. + */ + protected function createRequests(?int $media_id = NULL, ?int $user_id = NULL, array $token_payload = []): array { + $create_request = fn (string $uri_suffix, string $method = 'GET', bool $write = FALSE) => $this->createRequest( + $uri_suffix, + $method, + $media_id, + $user_id, + $write, + $token_payload, + ); + return [ + 'info' => $create_request(''), + 'file' => $create_request('/contents'), + 'save' => $create_request('/contents', 'POST', TRUE), + ]; + } + + /** + * Creates a WOPI request. + * + * @param string $uri_suffix + * Suffix to append to the WOPI media url. + * @param string $method + * E.g. 'GET' or 'POST'. + * @param int|null $media_id + * Media entity id, if different from the default. + * @param int|null $user_id + * User id, if different from the default. + * @param bool $write + * TRUE if write access is requested. + * @param array $token_payload + * Explicit token payload values. + * This can be used to cause a bad token. + * + * @return \Symfony\Component\HttpFoundation\Request + * The request. + */ + protected function createRequest( + string $uri_suffix = '', + string $method = 'GET', + ?int $media_id = NULL, + ?int $user_id = NULL, + bool $write = FALSE, + array $token_payload = [], + ): Request { + $media_id ??= (int) $this->media->id(); + $user_id ??= (int) $this->user->id(); + $uri = '/cool/wopi/files/' . $media_id . $uri_suffix; + $token = $this->createAccessToken($media_id, $user_id, $write, $token_payload); + $parameters = [ + 'id' => $media_id, + 'access_token' => $token, + ]; + return Request::create($uri, $method, $parameters); + } + + /** + * Retrieves an encoded access token. + * + * @param int|null $fid + * The file id. + * @param int|null $uid + * The user id. + * @param bool $write + * The write permission. + * @param array $payload + * Explicit payload values. + * This can be used to cause a bad token. + * + * @return string + * The enconded token. + */ + protected function createAccessToken(?int $fid = NULL, ?int $uid = NULL, bool $write = FALSE, array $payload = []): string { + /** @var \Drupal\collabora_online\Jwt\JwtTranscoderInterface $transcoder */ + $transcoder = \Drupal::service(JwtTranscoderInterface::class); + $expire_timestamp = gettimeofday(TRUE) + 1000; + $payload += [ + 'fid' => (string) ($fid ?? $this->media->id()), + 'uid' => (string) ($uid ?? $this->user->id()), + 'wri' => $write, + 'exp' => $expire_timestamp, + ]; + return $transcoder->encode($payload, $expire_timestamp); + } + + /** + * Asserts a successful json response given a request. + * + * @param array $expected_data + * The expected response JSON data. + * @param \Symfony\Component\HttpFoundation\Request $request + * The request to perform. + * @param string $message + * Message to distinguish this from other assertions. + */ + protected function assertJsonResponseOk(array $expected_data, Request $request, string $message = ''): void { + $this->assertJsonResponse(Response::HTTP_OK, $expected_data, $request, $message); + } + + /** + * Asserts a json response given a request. + * + * @param int $expected_code + * The expected response status code. + * @param array $expected_data + * The expected response JSON data. + * @param \Symfony\Component\HttpFoundation\Request $request + * The request to perform. + * @param string $message + * Message to distinguish this from other assertions. + */ + protected function assertJsonResponse(int $expected_code, array $expected_data, Request $request, string $message = ''): void { + $response = $this->handleRequest($request); + $this->assertEquals($expected_code, $response->getStatusCode(), $message); + $this->assertEquals('application/json', $response->headers->get('Content-Type'), $message); + $content = $response->getContent(); + $data = Json::decode($content); + $this->assertSame($expected_data, $data, $message); + } + + /** + * Asserts an access denied response given a request. + * + * @param string $expected_response_message + * Message expected to be in the response. + * @param \Symfony\Component\HttpFoundation\Request $request + * The request to perform. + * @param string $assertion_message + * Message to distinguish this from other assertions. + */ + protected function assertAccessDeniedResponse(string $expected_response_message, Request $request, string $assertion_message = ''): void { + $this->assertResponse( + Response::HTTP_FORBIDDEN, + $expected_response_message, + 'text/plain', + $request, + $assertion_message, + ); + } + + /** + * Asserts status code and content in a response given a request. + * + * @param int $expected_code + * The expected response status code. + * @param string $expected_content + * The expected response content. + * @param string $expected_content_type + * The type of content of the response. + * @param \Symfony\Component\HttpFoundation\Request $request + * The request to perform. + * @param string $message + * Message to distinguish this from other assertions. + */ + protected function assertResponse(int $expected_code, string $expected_content, string $expected_content_type, Request $request, string $message = ''): void { + $response = $this->handleRequest($request); + + $this->assertEquals($expected_code, $response->getStatusCode(), $message); + $this->assertEquals($expected_content, $response->getContent(), $message); + $this->assertEquals($expected_content_type, $response->headers->get('Content-Type'), $message); + } + + /** + * Handles a request and gets the response. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * Incoming request. + * + * @return \Symfony\Component\HttpFoundation\Response + * The response. + */ + protected function handleRequest(Request $request): Response { + /** @var \Symfony\Component\HttpKernel\HttpKernelInterface $kernel */ + $kernel = \Drupal::service('http_kernel'); + return $kernel->handle($request); + } + +} From d1155eed992e79536dfefca5b22d86a6a2efe3be Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 19 Dec 2024 02:22:38 +0100 Subject: [PATCH 12/26] Issue 75: Fix query parameters in WopiControllerTestBase::createRequest(). --- tests/src/Kernel/Controller/WopiControllerTestBase.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Kernel/Controller/WopiControllerTestBase.php b/tests/src/Kernel/Controller/WopiControllerTestBase.php index c65b603c..ba5c2e59 100644 --- a/tests/src/Kernel/Controller/WopiControllerTestBase.php +++ b/tests/src/Kernel/Controller/WopiControllerTestBase.php @@ -150,8 +150,8 @@ protected function createRequest( $uri = '/cool/wopi/files/' . $media_id . $uri_suffix; $token = $this->createAccessToken($media_id, $user_id, $write, $token_payload); $parameters = [ - 'id' => $media_id, 'access_token' => $token, + 'access_token_ttl' => '0', ]; return Request::create($uri, $method, $parameters); } From 3ef1c4a855ed46f0ffa1c5cd95120817d0d29608 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 13 Dec 2024 01:08:12 +0100 Subject: [PATCH 13/26] Issue 75: Require 'phpseclib/phpseclib' to handle RSA keys. --- composer.json | 1 + 1 file changed, 1 insertion(+) diff --git a/composer.json b/composer.json index a99c5409..dc32a09e 100644 --- a/composer.json +++ b/composer.json @@ -13,6 +13,7 @@ "php": ">=8.1", "drupal/key": "^1.19", "firebase/php-jwt": "^6.10", + "phpseclib/phpseclib": "^3.0", "symfony/error-handler": "^6.4|^7.1" }, "require-dev": { From 74001887a6ae73e684bc87c8e3404fb198beda28 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 17 Dec 2024 16:07:41 +0100 Subject: [PATCH 14/26] Issue 75: Add setting for wopi_proof. --- config/install/collabora_online.settings.yml | 1 + config/schema/collabora_online.schema.yml | 3 +++ src/Form/ConfigForm.php | 7 +++++++ tests/src/Functional/ConfigFormTest.php | 5 +++++ 4 files changed, 16 insertions(+) diff --git a/config/install/collabora_online.settings.yml b/config/install/collabora_online.settings.yml index 60e09ee9..17f2890f 100644 --- a/config/install/collabora_online.settings.yml +++ b/config/install/collabora_online.settings.yml @@ -10,5 +10,6 @@ cool: access_token_ttl: 86400 # Disable cert checks. disable_cert_check: false + wopi_proof: true # Allow fullscreen - Enabled by default for functionality. allowfullscreen: true diff --git a/config/schema/collabora_online.schema.yml b/config/schema/collabora_online.schema.yml index d187e2f5..2704cb1e 100644 --- a/config/schema/collabora_online.schema.yml +++ b/config/schema/collabora_online.schema.yml @@ -21,6 +21,9 @@ collabora_online.settings: disable_cert_check: type: boolean label: 'Disable cert checks.' + wopi_proof: + type: boolean + label: 'Verify WOPI proof header and timestamp.' allowfullscreen: type: boolean label: 'Allow full-screen.' diff --git a/src/Form/ConfigForm.php b/src/Form/ConfigForm.php index 05a20e84..f5a0c3a1 100644 --- a/src/Form/ConfigForm.php +++ b/src/Form/ConfigForm.php @@ -86,6 +86,12 @@ public function buildForm(array $form, FormStateInterface $form_state): array { '#default_value' => $cool_settings['disable_cert_check'] ?? FALSE, ]; + $form['wopi_proof'] = [ + '#type' => 'checkbox', + '#title' => $this->t('Verify proof header and timestamp in incoming WOPI requests.'), + '#default_value' => $cool_settings['wopi_proof'] ?? TRUE, + ]; + $form['allowfullscreen'] = [ '#type' => 'checkbox', '#title' => $this->t('Allow COOL to use fullscreen mode.'), @@ -116,6 +122,7 @@ public function submitForm(array &$form, FormStateInterface $form_state): void { ->set('cool.key_id', $form_state->getValue('key_id')) ->set('cool.access_token_ttl', $form_state->getValue('access_token_ttl')) ->set('cool.disable_cert_check', $form_state->getValue('disable_cert_check')) + ->set('cool.wopi_proof', $form_state->getValue('wopi_proof')) ->set('cool.allowfullscreen', $form_state->getValue('allowfullscreen')) ->save(); diff --git a/tests/src/Functional/ConfigFormTest.php b/tests/src/Functional/ConfigFormTest.php index 58a03a62..ca9830a0 100644 --- a/tests/src/Functional/ConfigFormTest.php +++ b/tests/src/Functional/ConfigFormTest.php @@ -61,6 +61,7 @@ public function testConfigForm(): void { $assert_session->fieldValueEquals('JWT private key', ''); $assert_session->fieldValueEquals('Access Token Expiration (in seconds)', '86400'); $assert_session->fieldValueEquals('Disable TLS certificate check for COOL.', ''); + $assert_session->fieldValueEquals('Verify proof header and timestamp in incoming WOPI requests.', '1'); $assert_session->fieldValueEquals('Allow COOL to use fullscreen mode.', '1'); // The key select element has no options, because no compatible key exists. @@ -94,6 +95,8 @@ public function testConfigForm(): void { ->setValue('3600'); $assert_session->fieldExists('Disable TLS certificate check for COOL.') ->check(); + $assert_session->fieldExists('Verify proof header and timestamp in incoming WOPI requests.') + ->uncheck(); // Since default is checked we disable the full screen option. $assert_session->fieldExists('Allow COOL to use fullscreen mode.') ->uncheck(); @@ -108,6 +111,7 @@ public function testConfigForm(): void { $assert_session->fieldValueEquals('JWT private key', 'collabora_test'); $assert_session->fieldValueEquals('Access Token Expiration (in seconds)', '3600'); $assert_session->fieldValueEquals('Disable TLS certificate check for COOL.', '1'); + $assert_session->fieldValueEquals('Verify proof header and timestamp in incoming WOPI requests.', ''); $assert_session->fieldValueEquals('Allow COOL to use fullscreen mode.', ''); // Test validation of required fields. @@ -144,6 +148,7 @@ public function testConfigForm(): void { $assert_session->fieldValueEquals('JWT private key', ''); $assert_session->fieldValueEquals('Access Token Expiration (in seconds)', '0'); $assert_session->fieldValueEquals('Disable TLS certificate check for COOL.', ''); + $assert_session->fieldValueEquals('Verify proof header and timestamp in incoming WOPI requests.', '1'); $assert_session->fieldValueEquals('Allow COOL to use fullscreen mode.', ''); $assert_session->buttonExists('Save configuration'); } From a08be93a0de1c51e34d642356e7e14d8af4e0967 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 13 Dec 2024 00:11:02 +0100 Subject: [PATCH 15/26] Issue 75: Add proofing to WOPI routes. --- collabora_online.routing.yml | 6 +- collabora_online.services.yml | 6 + src/Access/WopiProofAccessCheck.php | 229 ++++++++++++++++++ src/Access/WopiTimeoutAccessCheck.php | 89 +++++++ src/Util/DotNetTime.php | 54 +++++ .../Controller/WopiControllerProofTest.php | 186 ++++++++++++++ .../Kernel/Controller/WopiControllerTest.php | 12 + 7 files changed, 579 insertions(+), 3 deletions(-) create mode 100644 src/Access/WopiProofAccessCheck.php create mode 100644 src/Access/WopiTimeoutAccessCheck.php create mode 100644 src/Util/DotNetTime.php create mode 100644 tests/src/Kernel/Controller/WopiControllerProofTest.php diff --git a/collabora_online.routing.yml b/collabora_online.routing.yml index ea60d3ed..c1fb763e 100644 --- a/collabora_online.routing.yml +++ b/collabora_online.routing.yml @@ -47,7 +47,7 @@ collabora-online.wopi.info: action: 'info' methods: [ GET ] requirements: - _permission: 'access content' + _collabora_online_wopi_access: 'TRUE' _format: 'collabora_online_wopi' options: parameters: @@ -63,7 +63,7 @@ collabora-online.wopi.contents: action: 'content' methods: [ GET ] requirements: - _permission: 'access content' + _collabora_online_wopi_access: 'TRUE' _format: 'collabora_online_wopi' options: parameters: @@ -79,7 +79,7 @@ collabora-online.wopi.save: action: 'save' methods: [ POST ] requirements: - _permission: 'access content' + _collabora_online_wopi_access: 'TRUE' _format: 'collabora_online_wopi' options: parameters: diff --git a/collabora_online.services.yml b/collabora_online.services.yml index 946e2be1..e4487891 100644 --- a/collabora_online.services.yml +++ b/collabora_online.services.yml @@ -13,3 +13,9 @@ services: Drupal\collabora_online\MediaHelperInterface: class: Drupal\collabora_online\MediaHelper Drupal\collabora_online\EventSubscriber\ExceptionWopiSubscriber: { } + Drupal\collabora_online\Access\WopiProofAccessCheck: + tags: + - { name: access_check, applies_to: _collabora_online_wopi_access } + Drupal\collabora_online\Access\WopiTimeoutAccessCheck: + tags: + - { name: access_check, applies_to: _collabora_online_wopi_access } diff --git a/src/Access/WopiProofAccessCheck.php b/src/Access/WopiProofAccessCheck.php new file mode 100644 index 00000000..1dbfc05c --- /dev/null +++ b/src/Access/WopiProofAccessCheck.php @@ -0,0 +1,229 @@ +configFactory->get('collabora_online.settings'); + if (!($config->get('cool')['wopi_proof'] ?? TRUE)) { + return AccessResult::allowed() + ->addCacheableDependency($config); + } + // Each incoming request will have a different proof and timestamp, so there + // is no point in caching. + return $this->doCheckAccess($request) + ->setCacheMaxAge(0); + } + + /** + * Checks if the request has a WOPI proof. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * + * @return \Drupal\Core\Access\AccessResult + * The access result. + */ + protected function doCheckAccess(Request $request): AccessResult { + $keys = $this->getKeys(); + if (!isset($keys['current'])) { + return AccessResult::forbidden('Missing or incomplete WOPI proof keys.'); + } + $signatures = $this->getSignatures($request); + if (!isset($signatures['current'])) { + return AccessResult::forbidden('Missing or incomplete WOPI proof headers.'); + } + $subject = $this->getSubject($request); + + // Try different key and signature combinations. + foreach ($keys as $key_name => $key) { + foreach ($signatures as $signature_name => $signature) { + if ($key_name === 'old' && $signature_name === 'old') { + // Don't verify an old signature with an old key. + continue; + } + $success = $key->verify($subject, $signature); + if ($success) { + return AccessResult::allowed(); + } + } + } + return AccessResult::forbidden('WOPI proof mismatch.'); + } + + /** + * Gets the message to be signed. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * + * @return string + * The message to be signed. + */ + protected function getSubject(Request $request): string { + // This class is not responsible for checking the expiration, but it still + // needs the WOPI timestamp to build the message for the signature. + $timestamp_ticks = $request->headers->get('X-WOPI-Timestamp'); + $token = $request->query->get('access_token', ''); + $url = $request->getUri(); + return sprintf( + '%s%s%s%s%s%s', + pack('N', strlen($token)), + $token, + pack('N', strlen($url)), + strtoupper($url), + pack('N', 8), + pack('J', $timestamp_ticks) + ); + } + + /** + * Gets RSA public keys from the discovery.xml. + * + * The discovery.xml has a current and an old key. + * This is to support situations when the key has been recently changed, but + * the incoming request was signed with the older key. + * + * @return array<'current'|'old', \phpseclib3\Crypt\RSA\PublicKey> + * Current and old public key, or just the current if they are the same, or + * empty array if none found. + */ + protected function getKeys(): array { + // Get current and old key. + // Remove empty values. + // If both are the same, keep only the current one. + $public_keys = array_unique(array_filter([ + 'current' => $this->discovery->getProofKey(), + 'old' => $this->discovery->getProofKeyOld(), + ])); + $key_objects = []; + foreach ($public_keys as $key_name => $key_str) { + $key_obj = $this->prepareKey($key_str, $key_name); + if ($key_obj === NULL) { + continue; + } + $key_objects[$key_name] = $key_obj; + } + return $key_objects; + } + + /** + * Gets an RSA key object based on a string value. + * + * @param string $key_str + * Key string value from discovery.xml. + * @param 'current'|'old' $key_name + * Key name, only used for logging. + * + * @return \phpseclib3\Crypt\RSA\PublicKey|null + * An RSA public key object, or NULL on failure. + */ + protected function prepareKey(string $key_str, string $key_name): ?PublicKey { + try { + $key_object = PublicKeyLoader::loadPublicKey($key_str); + } + catch (\Throwable $e) { + $log_message = "Problem with the @name key from discovery.yml:
\n" + . Error::DEFAULT_ERROR_MESSAGE; + $log_args = Error::decodeException($e); + $log_args['@name'] = $key_name; + $this->logger->error($log_message, $log_args); + return NULL; + } + if (!$key_object instanceof PublicKey) { + $log_message = "Problem with the @name key from discovery.yml:
\n" + . "Expected RSA public key, found @type."; + $log_args = [ + '@name' => $key_name, + '@type' => get_debug_type($key_object), + ]; + $this->logger->error($log_message, $log_args); + return NULL; + } + return $key_object + ->withHash('sha256') + ->withPadding(RSA::SIGNATURE_RELAXED_PKCS1); + } + + /** + * Gets the current and old signature from the request. + * + * The request will have a current and an old signature. + * This is to support situations when the key has been recently changed, but + * the cached discovery.xml still has the old key. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * Incoming request that may have a signature to be verified. + * + * @return array{current?: string, old?: string} + * Current and old signature from the request, decoded and ready for use. + * If they are the same, only one of them is returned. + * If no signatures are found, an empty array is returned. + */ + protected function getSignatures(Request $request): array { + // Get the current and old proof header. + // Remove empty values. + // If both are the same, keep only the current one. + $proof_headers = array_unique(array_filter([ + 'current' => $request->headers->get('X-WOPI-Proof'), + 'old' => $request->headers->get('X-WOPI-ProofOld'), + ])); + $decoded_proof_headers = array_map( + fn (string $header_value) => base64_decode($header_value, TRUE), + $proof_headers, + ); + // Remove false values where decoding failed. + return array_filter($decoded_proof_headers); + } + +} diff --git a/src/Access/WopiTimeoutAccessCheck.php b/src/Access/WopiTimeoutAccessCheck.php new file mode 100644 index 00000000..970c4311 --- /dev/null +++ b/src/Access/WopiTimeoutAccessCheck.php @@ -0,0 +1,89 @@ +configFactory->get('collabora_online.settings'); + if (!($config->get('cool')['wopi_proof'] ?? TRUE)) { + return AccessResult::allowed() + ->addCacheableDependency($config); + } + // Each incoming request will have a different timestamp, so there is no + // point in caching. + return $this->doCheckAccess($request) + ->setCacheMaxAge(0); + } + + /** + * Checks if the X-WOPI-Timestamp is expired, without cache metadata. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * + * @return \Drupal\Core\Access\AccessResult + * The access result. + */ + protected function doCheckAccess(Request $request): AccessResult { + $wopi_ticks_str = $request->headers->get('X-WOPI-Timestamp', ''); + // Unfortunately, is_numeric() confuses the IDE's static analysis, so use + // regular expression instead. + if (!preg_match('#^[1-9]\d+$#', $wopi_ticks_str)) { + return AccessResult::forbidden('The X-WOPI-Timestamp header is missing, empty or invalid.'); + } + $wopi_timestamp = DotNetTime::ticksToTimestamp((float) $wopi_ticks_str); + $now_timestamp = $this->time->getRequestTime(); + $wopi_age_seconds = $now_timestamp - $wopi_timestamp; + if ($wopi_age_seconds > $this->ttlSeconds) { + return AccessResult::forbidden(sprintf( + 'The X-WOPI-Timestamp header is %s seconds old, which is more than the %s seconds TTL.', + $wopi_age_seconds, + $this->ttlSeconds, + )); + } + return AccessResult::allowed(); + } + +} diff --git a/src/Util/DotNetTime.php b/src/Util/DotNetTime.php new file mode 100644 index 00000000..163c4e05 --- /dev/null +++ b/src/Util/DotNetTime.php @@ -0,0 +1,54 @@ +getProofKey(). + * + * @var string + */ + protected string $wopiProofKey = self::PROOF_KEY; + + /** + * Mock return value for $discovery->getProofKeyOld(). + * + * @var string + */ + protected string $wopiProofKeyOld = self::PROOF_KEY; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + + // Set a mock discovery with custom proof keys. + $mock_discovery = $this->createMock(CollaboraDiscoveryInterface::class); + $this->container->set(CollaboraDiscoveryInterface::class, $mock_discovery); + $mock_discovery->method('getProofKey')->willReturnReference($this->wopiProofKey); + $mock_discovery->method('getProofKeyOld')->willReturnReference($this->wopiProofKeyOld); + } + + /** + * Tests access check based on WOPI proof and timestamp. + */ + public function testWopiProofAccess(): void { + $requests = $this->createRequests(); + // The values below have been recorded from real WOPI requests coming from + // a Collabora client. + $wopi_tick_times = [ + 'info' => '638701624451393010', + 'file' => '638701624452984064', + 'save' => '638701624725606559', + ]; + $wopi_proofs = [ + 'info' => 'MQSnbW+hu4z18psc+EfHkNBP1oTMTkcTYSOy5bAouB24XKaiaPb/BRF05ds1oq8GIvSjxL0nczIYMOxZyiNzGg3BvkKOm2eGPIMNCJvk7QZyIF/FI6E7uTg4zp7K/x6S+Dph/nVrNT37xjvHsf0MuKjfGdJKDMz8WzhXsKs/yAJVxErrFcabJRuva48fLNmMYkO/c9lLqxpdNvlASBOajeoECuKYwMJitBgMMDwrFuPA7a+RjWIjtYkkHu4oyZXTdWskU6P6LFSE4jWe9rPAxYJsOAXDZpefDYDXUSryazSeK+AgaA3p69ZrFTD5M/FH1hEDKhLqWOll49n7oTnL8w==', + 'file' => 'Ka9G+FAyjzf04Sk2Z1DSX0f0Xk4a9qtUGhKIfF/DPTuJOHykw38XGZy+v045EIsOpVZ6CFnlToI/h6hbstYhBG78O0xMV0L/o65HO8jCkuFPvU4yTXSAfgnKSVuQB3bbti2KnzG57dp6UwwnIUb2kNnCV8W4LTGuCaCBR2Z9Ydwk09UHHaIBo57g6oHGKpLoiMLhOMq4PDW3RBis98Vmc+p6qDipj4f1c7HarBlxew4BLMC0ubRFwXsBxMWMn9E3xJdMdUZOIAyidtghoOSuLbSKyCixsxQ0ONMHRFKq4K//ozoWac+8V8h7IzTbD6LIQwNITJqUW5PwPuzt/dNdTg==', + 'save' => 'Pj5Ak7sBsI4Pa0RVkn+0jJNLhIuGJRKjRi//cv1lHE4O1d3VlgT1WsFc5tRr1IW/OfiLVg6yJielgzSpjNRhushUATq/YdRx4lA61Cx9KQ6Y9hr2SZdg4sNgFjOZSFAfIBBx0P1Hfqgl1olv3EjIO/Fb+7/YNSsSG+rtpjPt8fGdaRxVUa4vCUjVCoJl+uaTY8CohGE4Aj5llXUmL2ZuctA8M/Ts+yOWEPfJ/nTwI0o6oG/2BtrQMQxChM7Lk59W+iGHh/AbxwRU+K0t7bdktzwtYbRmWarJwSIE/7pZ0zVbVj92hFNqtqKzR52+ACTqLB/qQnpSMl3Yu3Z5FkcS9g==', + ]; + foreach ($requests as $name => $request) { + $wopi_tick_time = $wopi_tick_times[$name]; + $wopi_proof = $wopi_proofs[$name]; + $this->doTestWopiProofAccess($request, $wopi_tick_time, $wopi_proof, $name); + } + } + + /** + * Tests a single WOPI route with different proof combinations. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * A basic request for the route. + * @param string $wopi_tick_time + * Proof time in DotNet ticks, as for the X-WOPI-Timestamp header. + * @param string $wopi_proof + * WOPI proof value, as for the X-WOPI-Proof header. + * @param string $message + * Message to use in assertions. + */ + protected function doTestWopiProofAccess(Request $request, string $wopi_tick_time, string $wopi_proof, string $message): void { + $token = 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJmaWQiOiIxMDAwIiwidWlkIjoiMSIsIndyaSI6dHJ1ZSwiZXhwIjoxNzM0NjUyMDQ1LjA0NjY3NX0.tM4aHamo7sQA3pkqiNbABlMM5mi-Z9vODaSm847hxIA'; + $request->query->set('access_token', $token); + $request->query->set('access_token_ttl', '0'); + $request->server->set('QUERY_STRING', http_build_query($request->query->all())); + // The test proof values were generated in an environment with + // 'web.test:8080' as the host name. + $request->headers->set('HOST', 'web.test:8080'); + $request->headers->set('X-WOPI-Timestamp', $wopi_tick_time); + $set_state = function ( + ?int $offset_seconds = NULL, + ?string $proof_recent = NULL, + ?string $proof_old = NULL, + ?string $key_recent = NULL, + ?string $key_old = NULL, + ) use ($request, $wopi_tick_time, $wopi_proof) { + // By default, set a fake request time 18 minutes after the WOPI time. + $_SERVER['REQUEST_TIME'] = DotNetTime::ticksToTimestamp((float) $wopi_tick_time) + + ($offset_seconds ?? 18 * 20); + $request->headers->set('X-WOPI-Proof', $proof_recent ?? $wopi_proof); + $request->headers->set('X-WOPI-ProofOld', $proof_old ?? $wopi_proof); + $this->wopiProofKey = $key_recent ?? self::PROOF_KEY; + $this->wopiProofKeyOld = $key_old ?? self::PROOF_KEY; + }; + + // With all values at their default, access is granted. + $set_state(); + // Clone the request to avoid side effects. + $this->assertRequestSuccess(clone $request, $message); + + // A single bad proof is ok, if the old proof is valid. + $set_state(proof_recent: self::BAD_PROOF); + $this->assertRequestSuccess(clone $request, $message); + + // A single bad old proof is ok, if the recent proof is valid. + $set_state(proof_old: self::BAD_PROOF); + $this->assertRequestSuccess(clone $request, $message); + + // Two bad proofs lead to failure. + $set_state(proof_recent: self::BAD_PROOF, proof_old: self::BAD_PROOF); + $this->assertAccessDeniedResponse('WOPI proof mismatch.', clone $request, $message); + + // A non-matching recent proof key is ok, if the old key still matches the + // recent proof from the request. + $set_state(key_recent: self::BAD_PROOF_KEY); + $this->assertRequestSuccess(clone $request, $message); + + // Two non-matching proof keys lead to failure. + $set_state(key_recent: self::BAD_PROOF_KEY, key_old: self::BAD_PROOF_KEY); + $this->assertAccessDeniedResponse('WOPI proof mismatch.', clone $request, $message); + + // The old proof matching the old key does not work. + $set_state(proof_recent: self::BAD_PROOF, key_recent: self::BAD_PROOF_KEY); + $this->assertAccessDeniedResponse('WOPI proof mismatch.', clone $request, $message); + + // Set a fake request time 22 minutes after the WOPI time. + $set_state(offset_seconds: 22 * 60); + $this->assertAccessDeniedResponse('The X-WOPI-Timestamp header is 1320 seconds old, which is more than the 1200 seconds TTL.', clone $request, $message); + + // Reset everything to clean up. + $set_state(); + } + + /** + * Asserts that a request results in a successful response. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * @param string $message + * A message to distinguish from other assertions. + */ + protected function assertRequestSuccess(Request $request, string $message): void { + $response = $this->handleRequest($request); + $this->assertEquals( + Response::HTTP_OK, + $response->getStatusCode(), + // Print the failure message if this is not 200. + $message . "\n" . substr((string) $response->getContent(), 0, 3000), + ); + } + +} diff --git a/tests/src/Kernel/Controller/WopiControllerTest.php b/tests/src/Kernel/Controller/WopiControllerTest.php index a29dbc8d..33e49988 100644 --- a/tests/src/Kernel/Controller/WopiControllerTest.php +++ b/tests/src/Kernel/Controller/WopiControllerTest.php @@ -22,6 +22,18 @@ */ class WopiControllerTest extends WopiControllerTestBase { + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + + // Disable the WOPI proof for this test. + $this->config('collabora_online.settings') + ->set('cool.wopi_proof', FALSE) + ->save(); + } + /** * Tests successful requests to the 'collabora-online.wopi.info' route. * From 9f017f01393375dceb1228ee81d42d8a93a90679 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 19 Dec 2024 03:55:23 +0100 Subject: [PATCH 16/26] Issue 75: Generate proof key for demo and ExistingSite tests. --- .github/workflows/ci.yaml | 7 +++++++ README.md | 3 +++ 2 files changed, 10 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 289a6827..caaa0fb0 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -61,6 +61,13 @@ jobs: run: | docker-compose exec -T web ./vendor/bin/phpcs -s + - name: Generate proof key + run: | + set -x + docker-compose exec -T collabora coolconfig generate-proof-key + # Collabora becomes unavailable after the above command. + docker-compose restart collabora + - name: Drupal site install run: | docker-compose exec -T web ./vendor/bin/run drupal:site-install diff --git a/README.md b/README.md index 602f3eff..c44ae6f8 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,9 @@ docker-compose up -d docker-compose exec web composer install docker-compose exec web ./vendor/bin/run drupal:site-install + +docker-compose exec collabora coolconfig generate-proof-key +docker-compose restart collabora ``` Optionally, generate an admin login link. From 7ead4374538d35ecfb53852446a2e5e0ac2913a0 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 19 Dec 2024 04:01:01 +0100 Subject: [PATCH 17/26] Issue 75: Recommend to generate a proof key when this is installed in a website. --- README.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c44ae6f8..cd8f5720 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,14 @@ For a local demo or test installation, [see below](#development--demo-installati - (optional) For the 'Collabora Online Group' sub-module, you also need the [Group](https://drupa.org/project/group) and [Group Media](https://drupa.org/project/groupmedia) modules. -### Installation steps +### Collabora Online server preparation + +It is recommended to generate a proof key as documented here:\ +docker-compose exec collabora coolconfig generate-proof-key + +If not, the proof mechanism needs to be disabled in the module settings. + +### Module installation steps See the [Drupal guide to install modules](https://www.drupal.org/docs/extending-drupal/installing-modules). @@ -39,7 +46,7 @@ field in the `composer.json` of your project. Then you can go into Drupal logged as an admin and go to _Extend_. In the list you should be able to find _Collabora Online_ and enable it. -From there you can access the module specific configuration. +From there you can access the module-specific configuration. Please check the "Configuration" section below! From 8703772fa5f9b27f3fa200eae131c6143ffdaff2 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 20 Dec 2024 22:52:57 +0100 Subject: [PATCH 18/26] Issue 75: Add a post-update hook to set 'wopi_proof' setting. --- collabora_online.post_update.php | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 collabora_online.post_update.php diff --git a/collabora_online.post_update.php b/collabora_online.post_update.php new file mode 100644 index 00000000..02de2850 --- /dev/null +++ b/collabora_online.post_update.php @@ -0,0 +1,24 @@ +getEditable('collabora_online.settings'); + $cool_settings = $config->get('cool') ?? []; + $cool_settings['wopi_proof'] ??= TRUE; + $config->set('cool', $cool_settings); + $config->save(); +} From aa43d11980e5098a0b02db36f04c92de2cc0695e Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 20 Dec 2024 23:11:35 +0100 Subject: [PATCH 19/26] Issue 75: Combine WopiTimeoutAccessCheck back into WopiProofAccessCheck. --- collabora_online.services.yml | 3 - src/Access/WopiProofAccessCheck.php | 58 ++++++++++++++++- src/Access/WopiTimeoutAccessCheck.php | 89 --------------------------- 3 files changed, 56 insertions(+), 94 deletions(-) delete mode 100644 src/Access/WopiTimeoutAccessCheck.php diff --git a/collabora_online.services.yml b/collabora_online.services.yml index e4487891..c9c7d982 100644 --- a/collabora_online.services.yml +++ b/collabora_online.services.yml @@ -16,6 +16,3 @@ services: Drupal\collabora_online\Access\WopiProofAccessCheck: tags: - { name: access_check, applies_to: _collabora_online_wopi_access } - Drupal\collabora_online\Access\WopiTimeoutAccessCheck: - tags: - - { name: access_check, applies_to: _collabora_online_wopi_access } diff --git a/src/Access/WopiProofAccessCheck.php b/src/Access/WopiProofAccessCheck.php index 1dbfc05c..12ff4709 100644 --- a/src/Access/WopiProofAccessCheck.php +++ b/src/Access/WopiProofAccessCheck.php @@ -15,6 +15,8 @@ namespace Drupal\collabora_online\Access; use Drupal\collabora_online\Cool\CollaboraDiscoveryInterface; +use Drupal\collabora_online\Util\DotNetTime; +use Drupal\Component\Datetime\TimeInterface; use Drupal\Core\Access\AccessResult; use Drupal\Core\Access\AccessResultInterface; use Drupal\Core\Config\ConfigFactoryInterface; @@ -42,6 +44,9 @@ public function __construct( #[Autowire(service: 'logger.channel.collabora_online')] protected readonly LoggerInterface $logger, protected readonly ConfigFactoryInterface $configFactory, + protected readonly TimeInterface $time, + // The recommended TTL is 20 minutes. + protected readonly int $ttlSeconds = 20 * 60, ) {} /** @@ -66,15 +71,64 @@ public function access(Request $request): AccessResultInterface { } /** - * Checks if the request has a WOPI proof. + * Checks the WOPI proof and the timeout, without adding cache metadata. * * @param \Symfony\Component\HttpFoundation\Request $request * The request. * * @return \Drupal\Core\Access\AccessResult - * The access result. + * An access result without cache metadata. + * Instead, calling code should set cache max age 0. */ protected function doCheckAccess(Request $request): AccessResult { + $timeout_access = $this->checkTimeout($request); + if (!$timeout_access->isAllowed()) { + return $timeout_access; + } + // There is no need for ->andIf(), because there is no cache metadata to + // merge. + return $this->checkProof($request); + } + + /** + * Checks if the X-WOPI-Timestamp is expired, without cache metadata. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * + * @return \Drupal\Core\Access\AccessResult + * An access result without cache metadata. + */ + protected function checkTimeout(Request $request): AccessResult { + $wopi_ticks_str = $request->headers->get('X-WOPI-Timestamp', ''); + // Unfortunately, is_numeric() confuses the IDE's static analysis, so use + // regular expression instead. + if (!preg_match('#^[1-9]\d+$#', $wopi_ticks_str)) { + return AccessResult::forbidden('The X-WOPI-Timestamp header is missing, empty or invalid.'); + } + $wopi_timestamp = DotNetTime::ticksToTimestamp((float) $wopi_ticks_str); + $now_timestamp = $this->time->getRequestTime(); + $wopi_age_seconds = $now_timestamp - $wopi_timestamp; + if ($wopi_age_seconds > $this->ttlSeconds) { + return AccessResult::forbidden(sprintf( + 'The X-WOPI-Timestamp header is %s seconds old, which is more than the %s seconds TTL.', + $wopi_age_seconds, + $this->ttlSeconds, + )); + } + return AccessResult::allowed(); + } + + /** + * Checks the WOPI proof, without adding cache metadata. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * + * @return \Drupal\Core\Access\AccessResult + * An access result without cache metadata. + */ + protected function checkProof(Request $request): AccessResult { $keys = $this->getKeys(); if (!isset($keys['current'])) { return AccessResult::forbidden('Missing or incomplete WOPI proof keys.'); diff --git a/src/Access/WopiTimeoutAccessCheck.php b/src/Access/WopiTimeoutAccessCheck.php deleted file mode 100644 index 970c4311..00000000 --- a/src/Access/WopiTimeoutAccessCheck.php +++ /dev/null @@ -1,89 +0,0 @@ -configFactory->get('collabora_online.settings'); - if (!($config->get('cool')['wopi_proof'] ?? TRUE)) { - return AccessResult::allowed() - ->addCacheableDependency($config); - } - // Each incoming request will have a different timestamp, so there is no - // point in caching. - return $this->doCheckAccess($request) - ->setCacheMaxAge(0); - } - - /** - * Checks if the X-WOPI-Timestamp is expired, without cache metadata. - * - * @param \Symfony\Component\HttpFoundation\Request $request - * The request. - * - * @return \Drupal\Core\Access\AccessResult - * The access result. - */ - protected function doCheckAccess(Request $request): AccessResult { - $wopi_ticks_str = $request->headers->get('X-WOPI-Timestamp', ''); - // Unfortunately, is_numeric() confuses the IDE's static analysis, so use - // regular expression instead. - if (!preg_match('#^[1-9]\d+$#', $wopi_ticks_str)) { - return AccessResult::forbidden('The X-WOPI-Timestamp header is missing, empty or invalid.'); - } - $wopi_timestamp = DotNetTime::ticksToTimestamp((float) $wopi_ticks_str); - $now_timestamp = $this->time->getRequestTime(); - $wopi_age_seconds = $now_timestamp - $wopi_timestamp; - if ($wopi_age_seconds > $this->ttlSeconds) { - return AccessResult::forbidden(sprintf( - 'The X-WOPI-Timestamp header is %s seconds old, which is more than the %s seconds TTL.', - $wopi_age_seconds, - $this->ttlSeconds, - )); - } - return AccessResult::allowed(); - } - -} From 6424f84d5ba76fb1d91060a6231f741e4e6069c3 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 20 Dec 2024 23:20:12 +0100 Subject: [PATCH 20/26] Issue 75: Use ->checkboxChecked() and ->checkboxNotChecked() in ConfigFormTest. --- tests/src/Functional/ConfigFormTest.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/src/Functional/ConfigFormTest.php b/tests/src/Functional/ConfigFormTest.php index ca9830a0..23542ebe 100644 --- a/tests/src/Functional/ConfigFormTest.php +++ b/tests/src/Functional/ConfigFormTest.php @@ -60,9 +60,9 @@ public function testConfigForm(): void { $assert_session->fieldValueEquals('WOPI host URL', 'https://localhost/'); $assert_session->fieldValueEquals('JWT private key', ''); $assert_session->fieldValueEquals('Access Token Expiration (in seconds)', '86400'); - $assert_session->fieldValueEquals('Disable TLS certificate check for COOL.', ''); - $assert_session->fieldValueEquals('Verify proof header and timestamp in incoming WOPI requests.', '1'); - $assert_session->fieldValueEquals('Allow COOL to use fullscreen mode.', '1'); + $assert_session->checkboxNotChecked('Disable TLS certificate check for COOL.'); + $assert_session->checkboxChecked('Verify proof header and timestamp in incoming WOPI requests.'); + $assert_session->checkboxChecked('Allow COOL to use fullscreen mode.'); // The key select element has no options, because no compatible key exists. $this->assertSame( @@ -110,9 +110,9 @@ public function testConfigForm(): void { $assert_session->fieldValueEquals('WOPI host URL', 'http://wopihost.com'); $assert_session->fieldValueEquals('JWT private key', 'collabora_test'); $assert_session->fieldValueEquals('Access Token Expiration (in seconds)', '3600'); - $assert_session->fieldValueEquals('Disable TLS certificate check for COOL.', '1'); - $assert_session->fieldValueEquals('Verify proof header and timestamp in incoming WOPI requests.', ''); - $assert_session->fieldValueEquals('Allow COOL to use fullscreen mode.', ''); + $assert_session->checkboxChecked('Disable TLS certificate check for COOL.'); + $assert_session->checkboxNotChecked('Verify proof header and timestamp in incoming WOPI requests.'); + $assert_session->checkboxNotChecked('Allow COOL to use fullscreen mode.'); // Test validation of required fields. $this->drupalGet(Url::fromRoute('collabora-online.settings')); @@ -147,9 +147,9 @@ public function testConfigForm(): void { $assert_session->fieldValueEquals('WOPI host URL', ''); $assert_session->fieldValueEquals('JWT private key', ''); $assert_session->fieldValueEquals('Access Token Expiration (in seconds)', '0'); - $assert_session->fieldValueEquals('Disable TLS certificate check for COOL.', ''); - $assert_session->fieldValueEquals('Verify proof header and timestamp in incoming WOPI requests.', '1'); - $assert_session->fieldValueEquals('Allow COOL to use fullscreen mode.', ''); + $assert_session->checkboxNotChecked('Disable TLS certificate check for COOL.'); + $assert_session->checkboxChecked('Verify proof header and timestamp in incoming WOPI requests.'); + $assert_session->checkboxNotChecked('Allow COOL to use fullscreen mode.'); $assert_session->buttonExists('Save configuration'); } From 2812bb480dee81567d66b30ee29f4dd20322db00 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 20 Dec 2024 23:25:18 +0100 Subject: [PATCH 21/26] Issue 75: Simplify the data provider in CollaboraDiscoveryTest. --- tests/src/Unit/CollaboraDiscoveryTest.php | 25 +++++++++++------------ 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/tests/src/Unit/CollaboraDiscoveryTest.php b/tests/src/Unit/CollaboraDiscoveryTest.php index 74bc43af..b6959953 100644 --- a/tests/src/Unit/CollaboraDiscoveryTest.php +++ b/tests/src/Unit/CollaboraDiscoveryTest.php @@ -76,14 +76,14 @@ public function testNoProofKey(): void { * @covers ::getProofKey * @covers ::getProofKeyOld * - * @dataProvider provideAllMethods + * @dataProvider provideAllMethodNames */ - public function testBlankXml(callable $callback): void { + public function testBlankXml(string $method): void { $discovery = $this->getDiscoveryFromXml(''); $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessage('The discovery.xml file is empty.'); - $callback($discovery); + $discovery->$method(); } /** @@ -93,15 +93,15 @@ public function testBlankXml(callable $callback): void { * @covers ::getProofKey * @covers ::getProofKeyOld * - * @dataProvider provideAllMethods + * @dataProvider provideAllMethodNames */ - public function testBrokenXml(callable $callback): void { + public function testBrokenXml(string $method): void { $xml = 'This file does not contain valid xml.'; $discovery = $this->getDiscoveryFromXml($xml); $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessageMatches('#^Error in the retrieved discovery.xml file: #'); - $callback($discovery); + $discovery->$method(); } /** @@ -109,15 +109,14 @@ public function testBrokenXml(callable $callback): void { * * This is used for tests where each method will throw the same exception. * - * @return list - * Argument tuples. + * @return list + * Argument tuples with method names. */ - public static function provideAllMethods(): array { + public static function provideAllMethodNames(): array { return [ - // Closures in data providers are ok in unit tests. - 'getWopiClientURL' => [fn (CollaboraDiscoveryInterface $discovery) => $discovery->getWopiClientURL()], - 'getProofKey' => [fn (CollaboraDiscoveryInterface $discovery) => $discovery->getProofKey()], - 'getProofKeyOld' => [fn (CollaboraDiscoveryInterface $discovery) => $discovery->getProofKeyOld()], + 'getWopiClientURL' => ['getWopiClientURL'], + 'getProofKey' => ['getProofKey'], + 'getProofKeyOld' => ['getProofKeyOld'], ]; } From 46608d64b8f140e78461099758404ee2fd32f02a Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 20 Dec 2024 23:26:40 +0100 Subject: [PATCH 22/26] Issue 75: Fix class doc on CollaboraDiscovery. --- src/Cool/CollaboraDiscovery.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 372183e4..5b828b84 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -18,7 +18,7 @@ use Symfony\Component\ErrorHandler\ErrorHandler; /** - * Service to get a WOPI client URL for a given MIME type. + * Service to get values from the discovery.xml. */ class CollaboraDiscovery implements CollaboraDiscoveryInterface { From 3c10a1c5958b94ac7b6481bd7ed82cc2a256b339 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Sat, 21 Dec 2024 00:36:19 +0100 Subject: [PATCH 23/26] Issue 75: Add missing link in README.md. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index cd8f5720..fc321040 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ For a local demo or test installation, [see below](#development--demo-installati ### Collabora Online server preparation It is recommended to generate a proof key as documented here:\ -docker-compose exec collabora coolconfig generate-proof-key +https://sdk.collaboraonline.com/docs/advanced_integration.html#wopi-proof If not, the proof mechanism needs to be disabled in the module settings. From 2fc47cba48d99b680c916d7eabb0484d9457f84f Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 7 Jan 2025 16:23:47 +0100 Subject: [PATCH 24/26] Issue 75: Don't check timeout in JwtTranscoderBase, rely on JWT class to do it. --- src/Jwt/JwtTranscoderBase.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Jwt/JwtTranscoderBase.php b/src/Jwt/JwtTranscoderBase.php index de906e93..0474a025 100644 --- a/src/Jwt/JwtTranscoderBase.php +++ b/src/Jwt/JwtTranscoderBase.php @@ -45,8 +45,8 @@ public function decode( $this->logger->error($e->getMessage()); return NULL; } - if (!isset($payload['exp']) || $payload['exp'] < gettimeofday(TRUE)) { - // The token is expired, or no timeout was set. + if (!isset($payload['exp'])) { + // The token does not have an expiration timestamp. return NULL; } return $payload; From 15dad306e46a19619f0299a2ac6afe3684edd640 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 7 Jan 2025 16:24:06 +0100 Subject: [PATCH 25/26] Issue 75: Set a fake timestamp in JWT class. --- tests/src/Kernel/Controller/WopiControllerProofTest.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/src/Kernel/Controller/WopiControllerProofTest.php b/tests/src/Kernel/Controller/WopiControllerProofTest.php index c5015e2f..3eaf375b 100644 --- a/tests/src/Kernel/Controller/WopiControllerProofTest.php +++ b/tests/src/Kernel/Controller/WopiControllerProofTest.php @@ -16,6 +16,7 @@ use Drupal\collabora_online\Cool\CollaboraDiscoveryInterface; use Drupal\collabora_online\Util\DotNetTime; +use Firebase\JWT\JWT; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -125,6 +126,10 @@ protected function doTestWopiProofAccess(Request $request, string $wopi_tick_tim $request->headers->set('X-WOPI-ProofOld', $proof_old ?? $wopi_proof); $this->wopiProofKey = $key_recent ?? self::PROOF_KEY; $this->wopiProofKeyOld = $key_old ?? self::PROOF_KEY; + // Set a timestamp that is earlier than the timeout. + // For now there is no parameter to manipulate this, but it is still part + // of the "state", so it makes sense to setup in this function. + JWT::$timestamp = 1734605245; }; // With all values at their default, access is granted. From 2d0896c82c9e8a507216c88bb3223569d667ad90 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 8 Jan 2025 15:18:51 +0100 Subject: [PATCH 26/26] Issue 75: Use is_numeric instead of preg_match(). --- src/Access/WopiProofAccessCheck.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Access/WopiProofAccessCheck.php b/src/Access/WopiProofAccessCheck.php index 12ff4709..a58257d6 100644 --- a/src/Access/WopiProofAccessCheck.php +++ b/src/Access/WopiProofAccessCheck.php @@ -101,9 +101,7 @@ protected function doCheckAccess(Request $request): AccessResult { */ protected function checkTimeout(Request $request): AccessResult { $wopi_ticks_str = $request->headers->get('X-WOPI-Timestamp', ''); - // Unfortunately, is_numeric() confuses the IDE's static analysis, so use - // regular expression instead. - if (!preg_match('#^[1-9]\d+$#', $wopi_ticks_str)) { + if (!is_numeric($wopi_ticks_str)) { return AccessResult::forbidden('The X-WOPI-Timestamp header is missing, empty or invalid.'); } $wopi_timestamp = DotNetTime::ticksToTimestamp((float) $wopi_ticks_str);