From be3bd7326420017f569e9bc70c4d80bc6b6d4352 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 14 Nov 2024 02:18:34 +0100 Subject: [PATCH 01/38] Issue #56: Distinguish readonly vs edit mode in the integration test. --- .../CollaboraIntegrationTest.php | 98 ++++++++++++++++--- 1 file changed, 85 insertions(+), 13 deletions(-) diff --git a/tests/src/ExistingSiteJavascript/CollaboraIntegrationTest.php b/tests/src/ExistingSiteJavascript/CollaboraIntegrationTest.php index 0fdd0bb2..3332d704 100644 --- a/tests/src/ExistingSiteJavascript/CollaboraIntegrationTest.php +++ b/tests/src/ExistingSiteJavascript/CollaboraIntegrationTest.php @@ -14,6 +14,7 @@ namespace Drupal\Tests\collabora_online\ExistingSiteJavascript; +use Behat\Mink\Element\NodeElement; use Drupal\file\Entity\File; use Drupal\media\Entity\Media; use Drupal\media\MediaInterface; @@ -40,26 +41,97 @@ public function testCollaboraPreview(): void { ]); $this->drupalLogin($user); $media = $this->createDocumentMedia('Shopping list', 'shopping-list', 'Chocolate, pickles'); + $this->drupalGet('/cool/view/' . $media->id()); + $this->getSession()->switchToIFrame('collabora-online-viewer'); + $this->assertCollaboraDocumentCanvas(); + $this->assertCollaboraDocumentName('shopping-list.txt'); + $this->assertCollaboraWordCountString('2 words, 18 characters'); - $assert_session = $this->assertSession(); - $canvas = $assert_session->waitForElement('css', 'canvas#document-canvas'); - $this->assertNotNull($canvas, 'The canvas element was not found after 10 seconds.'); + // Verify the read-only mode. + $readonly_indicator = $this->assertWaitForElement('.status-readonly-mode'); + $this->assertSame('Read-only', $readonly_indicator->getText()); + } + + /** + * Tests the Collabora editor in edit mode. + */ + public function testCollaboraEdit(): void { + $user = $this->createUser([ + 'edit any document in collabora', + 'administer media', + ]); + $this->drupalLogin($user); + $media = $this->createDocumentMedia('Shopping list', 'shopping-list', 'Chocolate, pickles'); + + $this->drupalGet('/cool/edit/' . $media->id()); + + $this->getSession()->switchToIFrame('collabora-online-viewer'); + $this->assertCollaboraDocumentCanvas(); + $this->assertCollaboraDocumentName('shopping-list.txt'); + $this->assertCollaboraWordCountString('2 words, 18 characters'); + + // Verify the edit mode. + // The button is always present when in edit mode, but it is only + // visible on a mobile / touch device. + $this->assertWaitForElement('#mobile-edit-button'); + } + + /** + * Asserts that the Collabora canvas is present. + * + * This is a general heuristic indicating that the editor has loaded. + */ + protected function assertCollaboraDocumentCanvas(): void { + $this->assertWaitForElement('canvas#document-canvas'); + } - $document_field = $assert_session->waitForElement('css', 'input#document-name-input'); - $this->assertNotNull($document_field, 'The document name input was not found after 10 seconds.'); - $this->getCurrentPage()->waitFor(10, function () use ($document_field) { - return $document_field->getValue() === 'shopping-list.txt'; + /** + * Asserts the document name displayed at the top of the editor. + * + * @param string $expected_name + * Expected document name. + */ + protected function assertCollaboraDocumentName(string $expected_name): void { + $document_field = $this->assertWaitForElement('input#document-name-input'); + $this->getCurrentPage()->waitFor(10, function () use ($document_field, $expected_name) { + return $document_field->getValue() === $expected_name; }); - $this->assertEquals('shopping-list.txt', $document_field->getValue(), 'The document name input did not contain the correct value after 10 seconds.'); + $this->assertEquals($expected_name, $document_field->getValue(), 'The document name input did not contain the correct value after 10 seconds.'); + } - $word_count_element = $assert_session->waitForElement('css', 'div#StateWordCount'); - $this->assertNotNull($word_count_element, 'The word count element was not found after 10 seconds.'); - $this->getCurrentPage()->waitFor(10, function () use ($word_count_element) { - return $word_count_element->getText() === '2 words, 18 characters'; + /** + * Asserts text in the word count element. + * + * This is a placeholder for testing the actual document text. + * + * @param string $expected_text + * Expected text for the word count element. + */ + protected function assertCollaboraWordCountString(string $expected_text): void { + $word_count_element = $this->assertWaitForElement('div#StateWordCount'); + $this->getCurrentPage()->waitFor(10, function () use ($word_count_element, $expected_text) { + return $word_count_element->getText() === $expected_text; }); - $this->assertEquals('2 words, 18 characters', $word_count_element->getText(), 'The word count element did not contain the correct text after 10 seconds.'); + $this->assertEquals($expected_text, $word_count_element->getText(), 'The word count element did not contain the correct text after 10 seconds.'); + } + + /** + * Waits for an element, and fails if not found. + * + * @param string|array $locator + * The locator / selector string. + * @param string $selector + * The selector type. + * + * @return \Behat\Mink\Element\NodeElement + * Node element that matches the selector. + */ + protected function assertWaitForElement(string|array $locator, string $selector = 'css'): NodeElement { + $element = $this->assertSession()->waitForElement($selector, $locator); + $this->assertNotNull($element, "The '$selector' element was not found after 10 seconds."); + return $element; } /** From 7ada8f4c76e83d169e63c88bb67fdefb073a3857 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 19:50:58 +0100 Subject: [PATCH 02/38] Issue #56: Add FetchClientUrlTest. --- tests/src/ExistingSite/FetchClientUrlTest.php | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 tests/src/ExistingSite/FetchClientUrlTest.php diff --git a/tests/src/ExistingSite/FetchClientUrlTest.php b/tests/src/ExistingSite/FetchClientUrlTest.php new file mode 100644 index 00000000..b39d75e8 --- /dev/null +++ b/tests/src/ExistingSite/FetchClientUrlTest.php @@ -0,0 +1,54 @@ +getWopiClientURL(); + // The protocol, domain and port are known when this test runs in the + // docker-compose setup. + $this->assertMatchesRegularExpression('@^http://collabora\.test:9980/browser/[0-9a-f]+/cool\.html\?$@', $client_url); + $this->assertSame('0: Success', $cool_request->errorString()); + } + + /** + * Tests fetching client url when the connection is misconfigured. + */ + public function testFetchClientUrlWithMisconfiguration(): void { + \Drupal::configFactory() + ->get('collabora_online.settings') + ->setSettingsOverride([ + 'cool' => [ + 'server' => 'httx://example.com', + ], + ]); + $cool_request = new CoolRequest(); + $client_url = $cool_request->getWopiClientURL(); + $this->assertNull($client_url); + $this->assertSame('204: Warning! You have to specify the scheme protocol too (http|https) for the server address.', $cool_request->errorString()); + } + +} From ca7103c02e27e3d869efdc33a78f661b3252ff22 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 22:59:02 +0100 Subject: [PATCH 03/38] Issue #56: Drop strStartsWith(), use str_starts_with() instead. Original steps: - Rename vars in strStartsWith(). - Simplify strStartsWith(). - Fix strStartsWith() by using strpos() instead of strrpos(). - Replace strStartsWith() with str_starts_with(). --- src/Cool/CoolRequest.php | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index c93f906d..869c0b07 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -64,24 +64,6 @@ function getWopiSrcUrl($discovery_parsed, $mimetype) { return NULL; } -/** - * Checks if a string starts with another string. - * - * @param string $s - * Haystack. - * @param string $ss - * Needle. - * - * @return bool - * TRUE if $ss is a prefix of $s. - * - * @see str_starts_with() - */ -function strStartsWith($s, $ss) { - $res = strrpos($s, $ss); - return !is_bool($res) && $res == 0; -} - /** * Helper class to fetch a WOPI client url. */ @@ -146,12 +128,12 @@ public function getWopiClientURL() { } $wopi_client_server = trim($wopi_client_server); - if (!strStartsWith($wopi_client_server, 'http')) { + if (!str_starts_with($wopi_client_server, 'http')) { $this->error_code = 204; return NULL; } - if (!strStartsWith($wopi_client_server, $_HOST_SCHEME . '://')) { + if (!str_starts_with($wopi_client_server, $_HOST_SCHEME . '://')) { $this->error_code = 202; return NULL; } From 50927119069bbcaa16f943e0e43224299e1a01a5 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 19:48:42 +0100 Subject: [PATCH 04/38] Issue #56: Drop $wopi_src object property, use local var instead. --- src/Cool/CoolRequest.php | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 869c0b07..86631100 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -87,19 +87,11 @@ class CoolRequest { 204 => 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', ]; - /** - * The WOPI url that was last fetched, or '' as initial value. - * - * @var int - */ - private $wopi_src; - /** * Constructor. */ public function __construct() { $this->error_code = 0; - $this->wopi_src = ''; } /** @@ -150,13 +142,13 @@ public function getWopiClientURL() { return NULL; } - $this->wopi_src = strval(getWopiSrcUrl($discovery_parsed, 'text/plain')[0]); - if (!$this->wopi_src) { + $wopi_src = strval(getWopiSrcUrl($discovery_parsed, 'text/plain')[0]); + if (!$wopi_src) { $this->error_code = 103; return NULL; } - return $this->wopi_src; + return $wopi_src; } } From c02250d65d731f860f8dec0176b601d5406f56c8 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 19:49:44 +0100 Subject: [PATCH 05/38] Issue #56: Initialize $error_code property in declaration, drop constructor. --- src/Cool/CoolRequest.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 86631100..77805e17 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -74,7 +74,7 @@ class CoolRequest { * * @var int */ - private $error_code; + private $error_code = 0; const ERROR_MSG = [ 0 => 'Success', @@ -87,13 +87,6 @@ class CoolRequest { 204 => 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', ]; - /** - * Constructor. - */ - public function __construct() { - $this->error_code = 0; - } - /** * Gets an error string from the last attempt to fetch the WOPI url. * From 3455e579b78c42b2488c0479168ff9579805d94e Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 20:28:53 +0100 Subject: [PATCH 06/38] Issue #56: Avoid string concat with t() strings. --- phpcs.xml | 3 --- src/Controller/ViewerController.php | 4 +++- src/Cool/CoolUtils.php | 4 +++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/phpcs.xml b/phpcs.xml index a0eca955..92ec131a 100644 --- a/phpcs.xml +++ b/phpcs.xml @@ -18,8 +18,5 @@ - - - diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index 951f0ffd..3bf4545f 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -70,7 +70,9 @@ public function editor(Media $media, $edit = FALSE) { $render_array = CoolUtils::getViewerRender($media, $edit, $options); if (!$render_array || array_key_exists('error', $render_array)) { - $error_msg = 'Viewer error: ' . ($render_array ? $render_array['error'] : 'NULL'); + $error_msg = (string) $this->t('Viewer error: @message', [ + '@message' => $render_array ? $render_array['error'] : 'NULL', + ]); \Drupal::logger('cool')->error($error_msg); return new Response( $error_msg, diff --git a/src/Cool/CoolUtils.php b/src/Cool/CoolUtils.php index f1a548b0..2d4b49c8 100644 --- a/src/Cool/CoolUtils.php +++ b/src/Cool/CoolUtils.php @@ -251,7 +251,9 @@ public static function getViewerRender(Media $media, bool $can_write, $options = $wopi_client = $req->getWopiClientURL(); if ($wopi_client === NULL) { return [ - 'error' => t('The Collabora Online server is not available: ') . $req->errorString(), + 'error' => t('The Collabora Online server is not available: @message', [ + '@message' => $req->errorString(), + ]), ]; } From 847757f249a20d27015094d84443a1819eb6a351 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 22:46:51 +0100 Subject: [PATCH 07/38] Issue #56: Let CoolRequest throw an exception on failure. --- src/Cool/CoolRequest.php | 71 ++++++++----------- src/Cool/CoolUtils.php | 9 ++- .../CollaboraNotAvailableException.php | 24 +++++++ tests/src/ExistingSite/FetchClientUrlTest.php | 11 +-- 4 files changed, 67 insertions(+), 48 deletions(-) create mode 100644 src/Exception/CollaboraNotAvailableException.php diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 77805e17..4158a41c 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -12,6 +12,8 @@ namespace Drupal\collabora_online\Cool; +use Drupal\collabora_online\Exception\CollaboraNotAvailableException; + /** * Gets the contents of discovery.xml from the Collabora server. * @@ -70,75 +72,62 @@ function getWopiSrcUrl($discovery_parsed, $mimetype) { class CoolRequest { /** - * Error code from last attempt to fetch the client WOPI url. - * - * @var int - */ - private $error_code = 0; - - const ERROR_MSG = [ - 0 => 'Success', - 101 => 'GET Request not found.', - 201 => 'Collabora Online server address is not valid.', - 202 => 'Collabora Online server address scheme does not match the current page url scheme.', - 203 => 'Not able to retrieve the discovery.xml file from the Collabora Online server.', - 102 => 'The retrieved discovery.xml file is not a valid XML file.', - 103 => 'The requested mime type is not handled.', - 204 => 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', - ]; - - /** - * Gets an error string from the last attempt to fetch the WOPI url. + * Gets the URL for the WOPI client. * * @return string - * Error string containing int error code and a message. - */ - public function errorString() { - return $this->error_code . ': ' . static::ERROR_MSG[$this->error_code]; - } - - /** - * Gets the URL for the WOPI client. + * The WOPI client url. * - * @return string|null - * The WOPI client url, or NULL on failure. + * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException + * The client url cannot be retrieved. */ public function getWopiClientURL() { $_HOST_SCHEME = isset($_SERVER['HTTPS']) ? 'https' : 'http'; $default_config = \Drupal::config('collabora_online.settings'); $wopi_client_server = $default_config->get('cool')['server']; if (!$wopi_client_server) { - $this->error_code = 201; - return NULL; + throw new CollaboraNotAvailableException( + 'Collabora Online server address is not valid.', + 201, + ); } $wopi_client_server = trim($wopi_client_server); if (!str_starts_with($wopi_client_server, 'http')) { - $this->error_code = 204; - return NULL; + throw new CollaboraNotAvailableException( + 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', + 204, + ); } if (!str_starts_with($wopi_client_server, $_HOST_SCHEME . '://')) { - $this->error_code = 202; - return NULL; + throw new CollaboraNotAvailableException( + 'Collabora Online server address scheme does not match the current page url scheme.', + 202, + ); } $discovery = getDiscovery($wopi_client_server); if ($discovery === FALSE) { - $this->error_code = 203; - return NULL; + throw new CollaboraNotAvailableException( + 'Not able to retrieve the discovery.xml file from the Collabora Online server.', + 203, + ); } $discovery_parsed = simplexml_load_string($discovery); if (!$discovery_parsed) { - $this->error_code = 102; - return NULL; + throw new CollaboraNotAvailableException( + 'The retrieved discovery.xml file is not a valid XML file.', + 102, + ); } $wopi_src = strval(getWopiSrcUrl($discovery_parsed, 'text/plain')[0]); if (!$wopi_src) { - $this->error_code = 103; - return NULL; + throw new CollaboraNotAvailableException( + 'The requested mime type is not handled.', + 103, + ); } return $wopi_src; diff --git a/src/Cool/CoolUtils.php b/src/Cool/CoolUtils.php index 2d4b49c8..5439a543 100644 --- a/src/Cool/CoolUtils.php +++ b/src/Cool/CoolUtils.php @@ -12,6 +12,7 @@ namespace Drupal\collabora_online\Cool; +use Drupal\collabora_online\Exception\CollaboraNotAvailableException; use Drupal\Core\Url; use Drupal\file\Entity\File; use Drupal\media\Entity\Media; @@ -248,11 +249,13 @@ public static function getViewerRender(Media $media, bool $can_write, $options = $allowfullscreen = $default_config->get('cool')['allowfullscreen'] ?? FALSE; $req = new CoolRequest(); - $wopi_client = $req->getWopiClientURL(); - if ($wopi_client === NULL) { + try { + $wopi_client = $req->getWopiClientURL(); + } + catch (CollaboraNotAvailableException $e) { return [ 'error' => t('The Collabora Online server is not available: @message', [ - '@message' => $req->errorString(), + '@message' => $e->getCode() . ': ' . $e->getMessage(), ]), ]; } diff --git a/src/Exception/CollaboraNotAvailableException.php b/src/Exception/CollaboraNotAvailableException.php new file mode 100644 index 00000000..77a18216 --- /dev/null +++ b/src/Exception/CollaboraNotAvailableException.php @@ -0,0 +1,24 @@ +assertMatchesRegularExpression('@^http://collabora\.test:9980/browser/[0-9a-f]+/cool\.html\?$@', $client_url); - $this->assertSame('0: Success', $cool_request->errorString()); } /** @@ -46,9 +46,12 @@ public function testFetchClientUrlWithMisconfiguration(): void { ], ]); $cool_request = new CoolRequest(); - $client_url = $cool_request->getWopiClientURL(); - $this->assertNull($client_url); - $this->assertSame('204: Warning! You have to specify the scheme protocol too (http|https) for the server address.', $cool_request->errorString()); + + $this->expectException(CollaboraNotAvailableException::class); + $this->expectExceptionMessage('Warning! You have to specify the scheme protocol too (http|https) for the server address.'); + $this->expectExceptionCode(204); + + $cool_request->getWopiClientURL(); } } From d5eb1d40a3b746cacab78651c2b39d0eb23f99b2 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 22:42:17 +0100 Subject: [PATCH 08/38] Issue #56: The config object from Drupal::config() cannot be NULL. --- src/Cool/CoolRequest.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 4158a41c..18a2da17 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -27,9 +27,6 @@ function getDiscovery($server) { $discovery_url = $server . '/hosting/discovery'; $default_config = \Drupal::config('collabora_online.settings'); - if ($default_config === NULL) { - return FALSE; - } $disable_checks = (bool) $default_config->get('cool')['disable_cert_check']; $stream_context = stream_context_create([ From 54d5857079cedbba1b024d20394f2be2d64117cf Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 12 Sep 2024 03:15:37 +0200 Subject: [PATCH 09/38] Issue #56: Use curl instead of file_get_contents() to get discovery. It can happen that file_get_contents() hangs at the end of a stream, waiting for more data. With curl it seems this does not happen. Even better would be to use e.g. Guzzle http client, but that's too big a change for now. --- src/Cool/CoolRequest.php | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 18a2da17..5fb0b37f 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -29,13 +29,25 @@ function getDiscovery($server) { $default_config = \Drupal::config('collabora_online.settings'); $disable_checks = (bool) $default_config->get('cool')['disable_cert_check']; - $stream_context = stream_context_create([ - 'ssl' => [ - 'verify_peer' => !$disable_checks, - 'verify_peer_name' => !$disable_checks, - ], + // Previously, file_get_contents() was used to fetch the discovery xml data. + // Depending on the environment, it can happen that file_get_contents() will + // hang at the end of a stream, expecting more data. + // With curl, this does not happen. + // @todo Refactor this and use e.g. Guzzle http client. + $curl = curl_init($discovery_url); + curl_setopt_array($curl, [ + CURLOPT_RETURNTRANSFER => TRUE, + // Previously, when this request was done with file_get_contents() and + // stream_context_create(), the 'verify_peer' and 'verify_peer_name' + // options were set. + // @todo Check if an equivalent to 'verify_peer_name' exists for curl. + CURLOPT_SSL_VERIFYPEER => !$disable_checks, ]); - $res = file_get_contents($discovery_url, FALSE, $stream_context); + $res = curl_exec($curl); + + if ($res === FALSE) { + \Drupal::logger('cool')->error('Cannot fetch from @url.', ['@url' => $discovery_url]); + } return $res; } From e42664ccec40a7e3382de540ac58370637bd4448 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 23:08:56 +0100 Subject: [PATCH 10/38] Issue #56: Refactor, then inline getWopiSrcUrl(). Changes: - Narrow parameter type for $discovery_parsed, don't check for NULL or FALSE anymore. - Use (string) cast instead of strval(). - Harden the condition for return value from xpath. - Let getWopiSrcUrl() always return string or throw exception. - Inline the function. - Invert if statement, to align with the rest. --- src/Cool/CoolRequest.php | 30 +++--------------------------- 1 file changed, 3 insertions(+), 27 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 5fb0b37f..bff494fe 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -51,30 +51,6 @@ function getDiscovery($server) { return $res; } -/** - * Extracts a WOPI url from the parsed discovery.xml. - * - * @param \SimpleXMLElement|null|false $discovery_parsed - * Parsed contents from discovery.xml from the Collabora server. - * Currently, NULL or FALSE are supported too, but lead to NULL return value. - * @param string $mimetype - * MIME type for which to fetch the WOPI url. E.g. 'text/plain'. - * - * @return mixed|null - * WOPI url as configured for this MIME type in discovery.xml, or NULL if none - * was found for the given MIME type. - */ -function getWopiSrcUrl($discovery_parsed, $mimetype) { - if ($discovery_parsed === NULL || $discovery_parsed == FALSE) { - return NULL; - } - $result = $discovery_parsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', $mimetype)); - if ($result && count($result) > 0) { - return $result[0]['urlsrc']; - } - return NULL; -} - /** * Helper class to fetch a WOPI client url. */ @@ -131,15 +107,15 @@ public function getWopiClientURL() { ); } - $wopi_src = strval(getWopiSrcUrl($discovery_parsed, 'text/plain')[0]); - if (!$wopi_src) { + $result = $discovery_parsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', 'text/plain')); + if (empty($result[0]['urlsrc'][0])) { throw new CollaboraNotAvailableException( 'The requested mime type is not handled.', 103, ); } - return $wopi_src; + return (string) $result[0]['urlsrc'][0]; } } From 850a376fb14530b9d208ed25c2d900ceaaed8549 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 00:09:52 +0100 Subject: [PATCH 11/38] Issue #56: Throw exception directly from getDiscovery(). --- src/Cool/CoolRequest.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index bff494fe..9c1ea02d 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -20,8 +20,11 @@ * @param string $server * Url of the Collabora Online server. * - * @return string|false - * The full contents of discovery.xml, or FALSE on failure. + * @return string + * The full contents of discovery.xml. + * + * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException + * The client url cannot be retrieved. */ function getDiscovery($server) { $discovery_url = $server . '/hosting/discovery'; @@ -47,6 +50,10 @@ function getDiscovery($server) { if ($res === FALSE) { \Drupal::logger('cool')->error('Cannot fetch from @url.', ['@url' => $discovery_url]); + throw new CollaboraNotAvailableException( + 'Not able to retrieve the discovery.xml file from the Collabora Online server.', + 203, + ); } return $res; } @@ -92,12 +99,6 @@ public function getWopiClientURL() { } $discovery = getDiscovery($wopi_client_server); - if ($discovery === FALSE) { - throw new CollaboraNotAvailableException( - 'Not able to retrieve the discovery.xml file from the Collabora Online server.', - 203, - ); - } $discovery_parsed = simplexml_load_string($discovery); if (!$discovery_parsed) { From 5da826901cb5ec58d0907a052c28e3e1df1ecf56 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 29 Nov 2024 14:43:45 +0100 Subject: [PATCH 12/38] Issue #56: Add param and return types. --- src/Cool/CoolRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 9c1ea02d..782ec128 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -26,7 +26,7 @@ * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException * The client url cannot be retrieved. */ -function getDiscovery($server) { +function getDiscovery(string $server): string { $discovery_url = $server . '/hosting/discovery'; $default_config = \Drupal::config('collabora_online.settings'); From 84b91a14e7bdb71a69f524f0a4c6dfdb82232383 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 23:21:37 +0100 Subject: [PATCH 13/38] Issue #56: Add php param and return types. --- src/Cool/CoolRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 782ec128..b22d3236 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -72,7 +72,7 @@ class CoolRequest { * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException * The client url cannot be retrieved. */ - public function getWopiClientURL() { + public function getWopiClientURL(): string { $_HOST_SCHEME = isset($_SERVER['HTTPS']) ? 'https' : 'http'; $default_config = \Drupal::config('collabora_online.settings'); $wopi_client_server = $default_config->get('cool')['server']; From 48b2bd3bdc85dfb2bd736e7e1bdb575c01573e70 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 28 Nov 2024 23:16:34 +0100 Subject: [PATCH 14/38] Issue #56: Rename $_HOST_SCHEME -> $host_scheme, and move it to where it is used. --- src/Cool/CoolRequest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index b22d3236..f8dcb6ed 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -73,7 +73,6 @@ class CoolRequest { * The client url cannot be retrieved. */ public function getWopiClientURL(): string { - $_HOST_SCHEME = isset($_SERVER['HTTPS']) ? 'https' : 'http'; $default_config = \Drupal::config('collabora_online.settings'); $wopi_client_server = $default_config->get('cool')['server']; if (!$wopi_client_server) { @@ -91,7 +90,8 @@ public function getWopiClientURL(): string { ); } - if (!str_starts_with($wopi_client_server, $_HOST_SCHEME . '://')) { + $host_scheme = isset($_SERVER['HTTPS']) ? 'https' : 'http'; + if (!str_starts_with($wopi_client_server, $host_scheme . '://')) { throw new CollaboraNotAvailableException( 'Collabora Online server address scheme does not match the current page url scheme.', 202, From adc362f8f4becc00e11066eac6ec1f92357e8569 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 14 Nov 2024 01:17:33 +0100 Subject: [PATCH 15/38] Issue #56: Support different MIME types. --- src/Cool/CoolRequest.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index f8dcb6ed..f05bbfbb 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -66,13 +66,17 @@ class CoolRequest { /** * Gets the URL for the WOPI client. * + * @param string $mimetype + * 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. * * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException * The client url cannot be retrieved. */ - public function getWopiClientURL(): string { + public function getWopiClientURL(string $mimetype = 'text/plain'): string { $default_config = \Drupal::config('collabora_online.settings'); $wopi_client_server = $default_config->get('cool')['server']; if (!$wopi_client_server) { @@ -108,7 +112,7 @@ public function getWopiClientURL(): string { ); } - $result = $discovery_parsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', 'text/plain')); + $result = $discovery_parsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', $mimetype)); if (empty($result[0]['urlsrc'][0])) { throw new CollaboraNotAvailableException( 'The requested mime type is not handled.', From abc2c2399fd32c9450d5bade21df5a65f04d9c56 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 22:24:14 +0100 Subject: [PATCH 16/38] Issue #56: Local var for $cool_settings. --- src/Cool/CoolRequest.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index f05bbfbb..6a2bd1ba 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -29,8 +29,8 @@ function getDiscovery(string $server): string { $discovery_url = $server . '/hosting/discovery'; - $default_config = \Drupal::config('collabora_online.settings'); - $disable_checks = (bool) $default_config->get('cool')['disable_cert_check']; + $cool_settings = \Drupal::config('collabora_online.settings')->get('cool'); + $disable_checks = (bool) $cool_settings['disable_cert_check']; // Previously, file_get_contents() was used to fetch the discovery xml data. // Depending on the environment, it can happen that file_get_contents() will @@ -77,8 +77,8 @@ class CoolRequest { * The client url cannot be retrieved. */ public function getWopiClientURL(string $mimetype = 'text/plain'): string { - $default_config = \Drupal::config('collabora_online.settings'); - $wopi_client_server = $default_config->get('cool')['server']; + $cool_settings = \Drupal::config('collabora_online.settings')->get('cool'); + $wopi_client_server = $cool_settings['server']; if (!$wopi_client_server) { throw new CollaboraNotAvailableException( 'Collabora Online server address is not valid.', From fd918cfe813fb0257039c616663c8e7f5cf75fee Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 23:14:08 +0100 Subject: [PATCH 17/38] Issue #56: Rename method to getDiscoveryXml, and var to $xml. --- src/Cool/CoolRequest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index 6a2bd1ba..b6493b70 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -26,7 +26,7 @@ * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException * The client url cannot be retrieved. */ -function getDiscovery(string $server): string { +function getDiscoveryXml(string $server): string { $discovery_url = $server . '/hosting/discovery'; $cool_settings = \Drupal::config('collabora_online.settings')->get('cool'); @@ -46,16 +46,16 @@ function getDiscovery(string $server): string { // @todo Check if an equivalent to 'verify_peer_name' exists for curl. CURLOPT_SSL_VERIFYPEER => !$disable_checks, ]); - $res = curl_exec($curl); + $xml = curl_exec($curl); - if ($res === FALSE) { + if ($xml === FALSE) { \Drupal::logger('cool')->error('Cannot fetch from @url.', ['@url' => $discovery_url]); throw new CollaboraNotAvailableException( 'Not able to retrieve the discovery.xml file from the Collabora Online server.', 203, ); } - return $res; + return $xml; } /** @@ -102,9 +102,9 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { ); } - $discovery = getDiscovery($wopi_client_server); + $xml = getDiscoveryXml($wopi_client_server); - $discovery_parsed = simplexml_load_string($discovery); + $discovery_parsed = simplexml_load_string($xml); if (!$discovery_parsed) { throw new CollaboraNotAvailableException( 'The retrieved discovery.xml file is not a valid XML file.', From 9547b2c07b6a07f5cfcb3850dc506626ac42bd9a Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 29 Nov 2024 14:52:51 +0100 Subject: [PATCH 18/38] Issue #56: Move xml fetching into new class CollaboraDiscoveryFetcher. --- src/Cool/CollaboraDiscoveryFetcher.php | 68 ++++++++++++++++++++++++++ src/Cool/CoolRequest.php | 47 +----------------- 2 files changed, 70 insertions(+), 45 deletions(-) create mode 100644 src/Cool/CollaboraDiscoveryFetcher.php diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php new file mode 100644 index 00000000..08a0a9b1 --- /dev/null +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -0,0 +1,68 @@ +get('cool'); + $disable_checks = (bool) $cool_settings['disable_cert_check']; + + // Previously, file_get_contents() was used to fetch the discovery xml data. + // Depending on the environment, it can happen that file_get_contents() will + // hang at the end of a stream, expecting more data. + // With curl, this does not happen. + // @todo Refactor this and use e.g. Guzzle http client. + $curl = curl_init($discovery_url); + curl_setopt_array($curl, [ + CURLOPT_RETURNTRANSFER => TRUE, + // Previously, when this request was done with file_get_contents() and + // stream_context_create(), the 'verify_peer' and 'verify_peer_name' + // options were set. + // @todo Check if an equivalent to 'verify_peer_name' exists for curl. + CURLOPT_SSL_VERIFYPEER => !$disable_checks, + ]); + $xml = curl_exec($curl); + + if ($xml === FALSE) { + \Drupal::logger('cool')->error('Cannot fetch from @url.', ['@url' => $discovery_url]); + throw new CollaboraNotAvailableException( + 'Not able to retrieve the discovery.xml file from the Collabora Online server.', + 203, + ); + } + return $xml; + } + +} diff --git a/src/Cool/CoolRequest.php b/src/Cool/CoolRequest.php index b6493b70..0b0a44bb 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CoolRequest.php @@ -14,50 +14,6 @@ use Drupal\collabora_online\Exception\CollaboraNotAvailableException; -/** - * Gets the contents of discovery.xml from the Collabora server. - * - * @param string $server - * Url of the Collabora Online server. - * - * @return string - * The full contents of discovery.xml. - * - * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException - * The client url cannot be retrieved. - */ -function getDiscoveryXml(string $server): string { - $discovery_url = $server . '/hosting/discovery'; - - $cool_settings = \Drupal::config('collabora_online.settings')->get('cool'); - $disable_checks = (bool) $cool_settings['disable_cert_check']; - - // Previously, file_get_contents() was used to fetch the discovery xml data. - // Depending on the environment, it can happen that file_get_contents() will - // hang at the end of a stream, expecting more data. - // With curl, this does not happen. - // @todo Refactor this and use e.g. Guzzle http client. - $curl = curl_init($discovery_url); - curl_setopt_array($curl, [ - CURLOPT_RETURNTRANSFER => TRUE, - // Previously, when this request was done with file_get_contents() and - // stream_context_create(), the 'verify_peer' and 'verify_peer_name' - // options were set. - // @todo Check if an equivalent to 'verify_peer_name' exists for curl. - CURLOPT_SSL_VERIFYPEER => !$disable_checks, - ]); - $xml = curl_exec($curl); - - if ($xml === FALSE) { - \Drupal::logger('cool')->error('Cannot fetch from @url.', ['@url' => $discovery_url]); - throw new CollaboraNotAvailableException( - 'Not able to retrieve the discovery.xml file from the Collabora Online server.', - 203, - ); - } - return $xml; -} - /** * Helper class to fetch a WOPI client url. */ @@ -102,7 +58,8 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { ); } - $xml = getDiscoveryXml($wopi_client_server); + $fetcher = new CollaboraDiscoveryFetcher(); + $xml = $fetcher->getDiscoveryXml($wopi_client_server); $discovery_parsed = simplexml_load_string($xml); if (!$discovery_parsed) { From 3d0afacabbcbf5b1823195683d1ac1afde4f9942 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 28 Nov 2024 21:43:31 +0100 Subject: [PATCH 19/38] Issue #56: Rename CoolRequest -> CollaboraDiscovery. --- src/Cool/{CoolRequest.php => CollaboraDiscovery.php} | 2 +- src/Cool/CoolUtils.php | 4 ++-- tests/src/ExistingSite/FetchClientUrlTest.php | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) rename src/Cool/{CoolRequest.php => CollaboraDiscovery.php} (98%) diff --git a/src/Cool/CoolRequest.php b/src/Cool/CollaboraDiscovery.php similarity index 98% rename from src/Cool/CoolRequest.php rename to src/Cool/CollaboraDiscovery.php index 0b0a44bb..5ef357f1 100644 --- a/src/Cool/CoolRequest.php +++ b/src/Cool/CollaboraDiscovery.php @@ -17,7 +17,7 @@ /** * Helper class to fetch a WOPI client url. */ -class CoolRequest { +class CollaboraDiscovery { /** * Gets the URL for the WOPI client. diff --git a/src/Cool/CoolUtils.php b/src/Cool/CoolUtils.php index 5439a543..3bb087da 100644 --- a/src/Cool/CoolUtils.php +++ b/src/Cool/CoolUtils.php @@ -248,9 +248,9 @@ public static function getViewerRender(Media $media, bool $can_write, $options = $wopi_base = $default_config->get('cool')['wopi_base']; $allowfullscreen = $default_config->get('cool')['allowfullscreen'] ?? FALSE; - $req = new CoolRequest(); + $discovery = new CollaboraDiscovery(); try { - $wopi_client = $req->getWopiClientURL(); + $wopi_client = $discovery->getWopiClientURL(); } catch (CollaboraNotAvailableException $e) { return [ diff --git a/tests/src/ExistingSite/FetchClientUrlTest.php b/tests/src/ExistingSite/FetchClientUrlTest.php index c3673b8f..813b570d 100644 --- a/tests/src/ExistingSite/FetchClientUrlTest.php +++ b/tests/src/ExistingSite/FetchClientUrlTest.php @@ -14,7 +14,7 @@ namespace Drupal\Tests\collabora_online\ExistingSite; -use Drupal\collabora_online\Cool\CoolRequest; +use Drupal\collabora_online\Cool\CollaboraDiscovery; use Drupal\collabora_online\Exception\CollaboraNotAvailableException; use weitzman\DrupalTestTraits\ExistingSiteBase; @@ -27,8 +27,8 @@ class FetchClientUrlTest extends ExistingSiteBase { * Tests fetching the client url. */ public function testFetchClientUrl(): void { - $cool_request = new CoolRequest(); - $client_url = $cool_request->getWopiClientURL(); + $discovery = new CollaboraDiscovery(); + $client_url = $discovery->getWopiClientURL(); // The protocol, domain and port are known when this test runs in the // docker-compose setup. $this->assertMatchesRegularExpression('@^http://collabora\.test:9980/browser/[0-9a-f]+/cool\.html\?$@', $client_url); @@ -45,13 +45,13 @@ public function testFetchClientUrlWithMisconfiguration(): void { 'server' => 'httx://example.com', ], ]); - $cool_request = new CoolRequest(); + $discovery = new CollaboraDiscovery(); $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessage('Warning! You have to specify the scheme protocol too (http|https) for the server address.'); $this->expectExceptionCode(204); - $cool_request->getWopiClientURL(); + $discovery->getWopiClientURL(); } } From 59a44b0db92e388db95e0721c9fe989d7f2f8abe Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 22:53:26 +0100 Subject: [PATCH 20/38] Issue #56: Convert CollaboraDiscovery and *Fetcher into services. --- collabora_online.services.yml | 5 +++++ src/Cool/CollaboraDiscovery.php | 5 +++-- src/Cool/CoolUtils.php | 3 ++- tests/src/ExistingSite/FetchClientUrlTest.php | 6 ++++-- 4 files changed, 14 insertions(+), 5 deletions(-) create mode 100644 collabora_online.services.yml diff --git a/collabora_online.services.yml b/collabora_online.services.yml new file mode 100644 index 00000000..3750ff2f --- /dev/null +++ b/collabora_online.services.yml @@ -0,0 +1,5 @@ +services: + _defaults: + autowire: true + Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher: { } + Drupal\collabora_online\Cool\CollaboraDiscovery: { } diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 5ef357f1..214be8a2 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -15,7 +15,7 @@ use Drupal\collabora_online\Exception\CollaboraNotAvailableException; /** - * Helper class to fetch a WOPI client url. + * Service to get a WOPI client url for a given MIME type. */ class CollaboraDiscovery { @@ -58,7 +58,8 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { ); } - $fetcher = new CollaboraDiscoveryFetcher(); + /** @var \Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher $fetcher */ + $fetcher = \Drupal::service(CollaboraDiscoveryFetcher::class); $xml = $fetcher->getDiscoveryXml($wopi_client_server); $discovery_parsed = simplexml_load_string($xml); diff --git a/src/Cool/CoolUtils.php b/src/Cool/CoolUtils.php index 3bb087da..ad7aa9da 100644 --- a/src/Cool/CoolUtils.php +++ b/src/Cool/CoolUtils.php @@ -248,7 +248,8 @@ public static function getViewerRender(Media $media, bool $can_write, $options = $wopi_base = $default_config->get('cool')['wopi_base']; $allowfullscreen = $default_config->get('cool')['allowfullscreen'] ?? FALSE; - $discovery = new CollaboraDiscovery(); + /** @var \Drupal\collabora_online\Cool\CollaboraDiscovery $discovery */ + $discovery = \Drupal::service(CollaboraDiscovery::class); try { $wopi_client = $discovery->getWopiClientURL(); } diff --git a/tests/src/ExistingSite/FetchClientUrlTest.php b/tests/src/ExistingSite/FetchClientUrlTest.php index 813b570d..0c406400 100644 --- a/tests/src/ExistingSite/FetchClientUrlTest.php +++ b/tests/src/ExistingSite/FetchClientUrlTest.php @@ -27,7 +27,8 @@ class FetchClientUrlTest extends ExistingSiteBase { * Tests fetching the client url. */ public function testFetchClientUrl(): void { - $discovery = new CollaboraDiscovery(); + /** @var \Drupal\collabora_online\Cool\CollaboraDiscovery $discovery */ + $discovery = \Drupal::service(CollaboraDiscovery::class); $client_url = $discovery->getWopiClientURL(); // The protocol, domain and port are known when this test runs in the // docker-compose setup. @@ -45,7 +46,8 @@ public function testFetchClientUrlWithMisconfiguration(): void { 'server' => 'httx://example.com', ], ]); - $discovery = new CollaboraDiscovery(); + /** @var \Drupal\collabora_online\Cool\CollaboraDiscovery $discovery */ + $discovery = \Drupal::service(CollaboraDiscovery::class); $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessage('Warning! You have to specify the scheme protocol too (http|https) for the server address.'); From cf4b603e74951a35a591d7dda937be0c061aea08 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 29 Nov 2024 15:00:13 +0100 Subject: [PATCH 21/38] Issue #56: Replace Drupal::service() calls in new services. --- collabora_online.services.yml | 3 +++ src/Cool/CollaboraDiscovery.php | 20 ++++++++++++++++---- src/Cool/CollaboraDiscoveryFetcher.php | 21 +++++++++++++++++++-- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/collabora_online.services.yml b/collabora_online.services.yml index 3750ff2f..b548f9b3 100644 --- a/collabora_online.services.yml +++ b/collabora_online.services.yml @@ -1,5 +1,8 @@ services: _defaults: autowire: true + logger.channel.collabora_online: + parent: logger.channel_base + arguments: ['cool'] Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher: { } Drupal\collabora_online\Cool\CollaboraDiscovery: { } diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 214be8a2..8ce7161e 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -13,12 +13,26 @@ namespace Drupal\collabora_online\Cool; use Drupal\collabora_online\Exception\CollaboraNotAvailableException; +use Drupal\Core\Config\ConfigFactoryInterface; /** * Service to get a WOPI client url for a given MIME type. */ class CollaboraDiscovery { + /** + * Constructor. + * + * @param \Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher $discoveryFetcher + * Discovery fetcher. + * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory + * Config factory. + */ + public function __construct( + protected readonly CollaboraDiscoveryFetcher $discoveryFetcher, + protected readonly ConfigFactoryInterface $configFactory, + ) {} + /** * Gets the URL for the WOPI client. * @@ -33,7 +47,7 @@ class CollaboraDiscovery { * The client url cannot be retrieved. */ public function getWopiClientURL(string $mimetype = 'text/plain'): string { - $cool_settings = \Drupal::config('collabora_online.settings')->get('cool'); + $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool'); $wopi_client_server = $cool_settings['server']; if (!$wopi_client_server) { throw new CollaboraNotAvailableException( @@ -58,9 +72,7 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { ); } - /** @var \Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher $fetcher */ - $fetcher = \Drupal::service(CollaboraDiscoveryFetcher::class); - $xml = $fetcher->getDiscoveryXml($wopi_client_server); + $xml = $this->discoveryFetcher->getDiscoveryXml($wopi_client_server); $discovery_parsed = simplexml_load_string($xml); if (!$discovery_parsed) { diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 08a0a9b1..0182e4fe 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -15,12 +15,29 @@ namespace Drupal\collabora_online\Cool; use Drupal\collabora_online\Exception\CollaboraNotAvailableException; +use Drupal\Core\Config\ConfigFactoryInterface; +use Drupal\Core\Logger\LoggerChannelInterface; +use Symfony\Component\DependencyInjection\Attribute\Autowire; /** * Service to load the discovery.xml from the Collabora server. */ class CollaboraDiscoveryFetcher { + /** + * Constructor. + * + * @param \Drupal\Core\Logger\LoggerChannelInterface $logger + * Logger channel. + * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory + * Config factory. + */ + public function __construct( + #[Autowire(service: 'logger.channel.collabora_online')] + protected readonly LoggerChannelInterface $logger, + protected readonly ConfigFactoryInterface $configFactory, + ) {} + /** * Gets the contents of discovery.xml from the Collabora server. * @@ -36,7 +53,7 @@ class CollaboraDiscoveryFetcher { public function getDiscoveryXml(string $server): string { $discovery_url = $server . '/hosting/discovery'; - $cool_settings = \Drupal::config('collabora_online.settings')->get('cool'); + $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool'); $disable_checks = (bool) $cool_settings['disable_cert_check']; // Previously, file_get_contents() was used to fetch the discovery xml data. @@ -56,7 +73,7 @@ public function getDiscoveryXml(string $server): string { $xml = curl_exec($curl); if ($xml === FALSE) { - \Drupal::logger('cool')->error('Cannot fetch from @url.', ['@url' => $discovery_url]); + $this->logger->error('Cannot fetch from @url.', ['@url' => $discovery_url]); throw new CollaboraNotAvailableException( 'Not able to retrieve the discovery.xml file from the Collabora Online server.', 203, From 04aac106d0f7190ab0767cb1693e7c6d89a90b2a Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 00:30:33 +0100 Subject: [PATCH 22/38] Issue #56: Use Guzzle to fetch discovery.xml. --- src/Cool/CollaboraDiscoveryFetcher.php | 31 ++++++++++++-------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 0182e4fe..e02a9631 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -17,6 +17,9 @@ use Drupal\collabora_online\Exception\CollaboraNotAvailableException; use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Logger\LoggerChannelInterface; +use GuzzleHttp\ClientInterface; +use GuzzleHttp\RequestOptions; +use Psr\Http\Client\ClientExceptionInterface; use Symfony\Component\DependencyInjection\Attribute\Autowire; /** @@ -31,11 +34,14 @@ class CollaboraDiscoveryFetcher { * Logger channel. * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory * Config factory. + * @param \GuzzleHttp\ClientInterface $httpClient + * Http client. */ public function __construct( #[Autowire(service: 'logger.channel.collabora_online')] protected readonly LoggerChannelInterface $logger, protected readonly ConfigFactoryInterface $configFactory, + protected readonly ClientInterface $httpClient, ) {} /** @@ -56,27 +62,18 @@ public function getDiscoveryXml(string $server): string { $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool'); $disable_checks = (bool) $cool_settings['disable_cert_check']; - // Previously, file_get_contents() was used to fetch the discovery xml data. - // Depending on the environment, it can happen that file_get_contents() will - // hang at the end of a stream, expecting more data. - // With curl, this does not happen. - // @todo Refactor this and use e.g. Guzzle http client. - $curl = curl_init($discovery_url); - curl_setopt_array($curl, [ - CURLOPT_RETURNTRANSFER => TRUE, - // Previously, when this request was done with file_get_contents() and - // stream_context_create(), the 'verify_peer' and 'verify_peer_name' - // options were set. - // @todo Check if an equivalent to 'verify_peer_name' exists for curl. - CURLOPT_SSL_VERIFYPEER => !$disable_checks, - ]); - $xml = curl_exec($curl); - - if ($xml === FALSE) { + try { + $response = $this->httpClient->get($discovery_url, [ + RequestOptions::VERIFY => !$disable_checks, + ]); + $xml = $response->getBody()->getContents(); + } + catch (ClientExceptionInterface $e) { $this->logger->error('Cannot fetch from @url.', ['@url' => $discovery_url]); throw new CollaboraNotAvailableException( 'Not able to retrieve the discovery.xml file from the Collabora Online server.', 203, + $e, ); } return $xml; From ea8a16ef102d529500232e0d89e9dcc30e7dc97f Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 28 Nov 2024 12:43:41 +0100 Subject: [PATCH 23/38] Issue #56: Extract new method loadSettings(), and fail if config is empty. --- src/Cool/CollaboraDiscovery.php | 8 ++------ src/Cool/CollaboraDiscoveryFetcher.php | 21 ++++++++++++++++++++- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 8ce7161e..144e72c3 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -13,7 +13,6 @@ namespace Drupal\collabora_online\Cool; use Drupal\collabora_online\Exception\CollaboraNotAvailableException; -use Drupal\Core\Config\ConfigFactoryInterface; /** * Service to get a WOPI client url for a given MIME type. @@ -24,13 +23,10 @@ class CollaboraDiscovery { * Constructor. * * @param \Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher $discoveryFetcher - * Discovery fetcher. - * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory - * Config factory. + * Service to load the discovery.xml from the Collabora server. */ public function __construct( protected readonly CollaboraDiscoveryFetcher $discoveryFetcher, - protected readonly ConfigFactoryInterface $configFactory, ) {} /** @@ -47,7 +43,7 @@ public function __construct( * The client url cannot be retrieved. */ public function getWopiClientURL(string $mimetype = 'text/plain'): string { - $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool'); + $cool_settings = $this->discoveryFetcher->loadSettings(); $wopi_client_server = $cool_settings['server']; if (!$wopi_client_server) { throw new CollaboraNotAvailableException( diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index e02a9631..6f809f48 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -59,7 +59,7 @@ public function __construct( public function getDiscoveryXml(string $server): string { $discovery_url = $server . '/hosting/discovery'; - $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool'); + $cool_settings = $this->loadSettings(); $disable_checks = (bool) $cool_settings['disable_cert_check']; try { @@ -79,4 +79,23 @@ public function getDiscoveryXml(string $server): string { return $xml; } + /** + * Loads the relevant configuration. + * + * @return array + * Configuration. + * + * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException + * The module is not configured. + * + * @todo Make this protected, only call from within. + */ + public function loadSettings(): array { + $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool'); + if (!$cool_settings) { + throw new CollaboraNotAvailableException('The Collabora Online connection is not configured.'); + } + return $cool_settings; + } + } From f09af443182e0d625bafd2ffb0a6f4d8c5f69d47 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 23:48:56 +0100 Subject: [PATCH 24/38] Issue #56: Extract getWopiClientServerBaseUrl(). --- src/Cool/CollaboraDiscovery.php | 26 +-------------- src/Cool/CollaboraDiscoveryFetcher.php | 45 ++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 28 deletions(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 144e72c3..33d5c097 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -43,31 +43,7 @@ public function __construct( * The client url cannot be retrieved. */ public function getWopiClientURL(string $mimetype = 'text/plain'): string { - $cool_settings = $this->discoveryFetcher->loadSettings(); - $wopi_client_server = $cool_settings['server']; - if (!$wopi_client_server) { - throw new CollaboraNotAvailableException( - 'Collabora Online server address is not valid.', - 201, - ); - } - $wopi_client_server = trim($wopi_client_server); - - if (!str_starts_with($wopi_client_server, 'http')) { - throw new CollaboraNotAvailableException( - 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', - 204, - ); - } - - $host_scheme = isset($_SERVER['HTTPS']) ? 'https' : 'http'; - if (!str_starts_with($wopi_client_server, $host_scheme . '://')) { - throw new CollaboraNotAvailableException( - 'Collabora Online server address scheme does not match the current page url scheme.', - 202, - ); - } - + $wopi_client_server = $this->discoveryFetcher->getWopiClientServerBaseUrl(); $xml = $this->discoveryFetcher->getDiscoveryXml($wopi_client_server); $discovery_parsed = simplexml_load_string($xml); diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 6f809f48..455304b8 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -79,6 +79,47 @@ public function getDiscoveryXml(string $server): string { return $xml; } + /** + * Loads the WOPI server url from configuration. + * + * @return string + * Base URL to access the WOPI server from Drupal. + * + * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException + * The WOPI server url is misconfigured, or the protocol does not match + * that of the current Drupal request. + * + * @todo Make this protected, only call from within. + */ + public function getWopiClientServerBaseUrl(): string { + $cool_settings = $this->loadSettings(); + $wopi_client_server = $cool_settings['server']; + if (!$wopi_client_server) { + throw new CollaboraNotAvailableException( + 'Collabora Online server address is not valid.', + 201, + ); + } + $wopi_client_server = trim($wopi_client_server); + + if (!str_starts_with($wopi_client_server, 'http')) { + throw new CollaboraNotAvailableException( + 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', + 204, + ); + } + + $host_scheme = isset($_SERVER['HTTPS']) ? 'https' : 'http'; + if (!str_starts_with($wopi_client_server, $host_scheme . '://')) { + throw new CollaboraNotAvailableException( + 'Collabora Online server address scheme does not match the current page url scheme.', + 202, + ); + } + + return $wopi_client_server; + } + /** * Loads the relevant configuration. * @@ -87,10 +128,8 @@ public function getDiscoveryXml(string $server): string { * * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException * The module is not configured. - * - * @todo Make this protected, only call from within. */ - public function loadSettings(): array { + protected function loadSettings(): array { $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool'); if (!$cool_settings) { throw new CollaboraNotAvailableException('The Collabora Online connection is not configured.'); From 31919450c6850cb6163659675dda4355dac14aa6 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 12 Nov 2024 23:50:18 +0100 Subject: [PATCH 25/38] Issue #56: Call getWopiClientServerBaseUrl() directly from getDiscovery(). --- src/Cool/CollaboraDiscovery.php | 3 +-- src/Cool/CollaboraDiscoveryFetcher.php | 11 +++-------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 33d5c097..5282acf4 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -43,8 +43,7 @@ public function __construct( * The client url cannot be retrieved. */ public function getWopiClientURL(string $mimetype = 'text/plain'): string { - $wopi_client_server = $this->discoveryFetcher->getWopiClientServerBaseUrl(); - $xml = $this->discoveryFetcher->getDiscoveryXml($wopi_client_server); + $xml = $this->discoveryFetcher->getDiscoveryXml(); $discovery_parsed = simplexml_load_string($xml); if (!$discovery_parsed) { diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 455304b8..6b9ec98e 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -47,17 +47,14 @@ public function __construct( /** * Gets the contents of discovery.xml from the Collabora server. * - * @param string $server - * Url of the Collabora Online server. - * * @return string * The full contents of discovery.xml. * * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException * The client url cannot be retrieved. */ - public function getDiscoveryXml(string $server): string { - $discovery_url = $server . '/hosting/discovery'; + public function getDiscoveryXml(): string { + $discovery_url = $this->getWopiClientServerBaseUrl() . '/hosting/discovery'; $cool_settings = $this->loadSettings(); $disable_checks = (bool) $cool_settings['disable_cert_check']; @@ -88,10 +85,8 @@ public function getDiscoveryXml(string $server): string { * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException * The WOPI server url is misconfigured, or the protocol does not match * that of the current Drupal request. - * - * @todo Make this protected, only call from within. */ - public function getWopiClientServerBaseUrl(): string { + protected function getWopiClientServerBaseUrl(): string { $cool_settings = $this->loadSettings(); $wopi_client_server = $cool_settings['server']; if (!$wopi_client_server) { From 2023e61d10b71dae675b029d9afa4c9d87bb95ba Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 22:48:03 +0100 Subject: [PATCH 26/38] Issue #56: Avoid php notice for missing config keys. --- src/Cool/CollaboraDiscoveryFetcher.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 6b9ec98e..0103e3e8 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -57,7 +57,7 @@ public function getDiscoveryXml(): string { $discovery_url = $this->getWopiClientServerBaseUrl() . '/hosting/discovery'; $cool_settings = $this->loadSettings(); - $disable_checks = (bool) $cool_settings['disable_cert_check']; + $disable_checks = !empty($cool_settings['disable_cert_check']); try { $response = $this->httpClient->get($discovery_url, [ @@ -88,7 +88,7 @@ public function getDiscoveryXml(): string { */ protected function getWopiClientServerBaseUrl(): string { $cool_settings = $this->loadSettings(); - $wopi_client_server = $cool_settings['server']; + $wopi_client_server = $cool_settings['server'] ?? NULL; if (!$wopi_client_server) { throw new CollaboraNotAvailableException( 'Collabora Online server address is not valid.', From 4985c311211c82cc98b5c3268684e2b47c2cffba Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 28 Nov 2024 23:25:25 +0100 Subject: [PATCH 27/38] Issue #56: Harden the protocol prefix condition for $wopi_client_server. --- src/Cool/CollaboraDiscoveryFetcher.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 0103e3e8..0569c15b 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -97,7 +97,7 @@ protected function getWopiClientServerBaseUrl(): string { } $wopi_client_server = trim($wopi_client_server); - if (!str_starts_with($wopi_client_server, 'http')) { + if (!str_starts_with($wopi_client_server, 'http://') && !str_starts_with($wopi_client_server, 'https://')) { throw new CollaboraNotAvailableException( 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', 204, From 88594bccfc95ea636f5287fb46d2f415d9da5f75 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 22:51:34 +0100 Subject: [PATCH 28/38] Issue #56: Change exception messages. --- src/Cool/CollaboraDiscoveryFetcher.php | 13 ++++++++++--- tests/src/ExistingSite/FetchClientUrlTest.php | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 0569c15b..d3ce95ec 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -91,7 +91,7 @@ protected function getWopiClientServerBaseUrl(): string { $wopi_client_server = $cool_settings['server'] ?? NULL; if (!$wopi_client_server) { throw new CollaboraNotAvailableException( - 'Collabora Online server address is not valid.', + 'The configured Collabora Online server address is empty.', 201, ); } @@ -99,7 +99,10 @@ protected function getWopiClientServerBaseUrl(): string { if (!str_starts_with($wopi_client_server, 'http://') && !str_starts_with($wopi_client_server, 'https://')) { throw new CollaboraNotAvailableException( - 'Warning! You have to specify the scheme protocol too (http|https) for the server address.', + sprintf( + "The configured Collabora Online server address must begin with 'http://' or 'https://'. Found '%s'.", + $wopi_client_server, + ), 204, ); } @@ -107,7 +110,11 @@ protected function getWopiClientServerBaseUrl(): string { $host_scheme = isset($_SERVER['HTTPS']) ? 'https' : 'http'; if (!str_starts_with($wopi_client_server, $host_scheme . '://')) { throw new CollaboraNotAvailableException( - 'Collabora Online server address scheme does not match the current page url scheme.', + sprintf( + "The url scheme '%s' of the current request does not match the url scheme of the configured Collabora Online server address '%s'.", + $host_scheme, + $wopi_client_server, + ), 202, ); } diff --git a/tests/src/ExistingSite/FetchClientUrlTest.php b/tests/src/ExistingSite/FetchClientUrlTest.php index 0c406400..c127b05b 100644 --- a/tests/src/ExistingSite/FetchClientUrlTest.php +++ b/tests/src/ExistingSite/FetchClientUrlTest.php @@ -50,7 +50,7 @@ public function testFetchClientUrlWithMisconfiguration(): void { $discovery = \Drupal::service(CollaboraDiscovery::class); $this->expectException(CollaboraNotAvailableException::class); - $this->expectExceptionMessage('Warning! You have to specify the scheme protocol too (http|https) for the server address.'); + $this->expectExceptionMessage("The configured Collabora Online server address must begin with 'http://' or 'https://'. Found 'httx://example.com'."); $this->expectExceptionCode(204); $discovery->getWopiClientURL(); From 1260936868e556b4e466ee3e03f2eaf3ca86ded9 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 23:04:31 +0100 Subject: [PATCH 29/38] Issue #56: Change log message. --- src/Cool/CollaboraDiscoveryFetcher.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index d3ce95ec..a21f8336 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -66,7 +66,12 @@ public function getDiscoveryXml(): string { $xml = $response->getBody()->getContents(); } catch (ClientExceptionInterface $e) { - $this->logger->error('Cannot fetch from @url.', ['@url' => $discovery_url]); + // The backtrace of a client exception is typically not very + // interesting. Just log the message. + $this->logger->error("Failed to fetch from '@url': @message.", [ + '@url' => $discovery_url, + '@message' => $e->getMessage(), + ]); throw new CollaboraNotAvailableException( 'Not able to retrieve the discovery.xml file from the Collabora Online server.', 203, From dee080a0c17d755c0b05423423b47a88eeec6e0b Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Mon, 9 Sep 2024 23:35:20 +0200 Subject: [PATCH 30/38] Issue #56: Use constructor property promotion, readonly, AutowireTrait in ViewerController. Also make the private property protected. --- src/Controller/ViewerController.php | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index 3bf4545f..5ac8a587 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -16,7 +16,6 @@ use Drupal\Core\Controller\ControllerBase; use Drupal\Core\Render\RendererInterface; use Drupal\media\Entity\Media; -use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\HttpFoundation\Response; /** @@ -24,31 +23,15 @@ */ class ViewerController extends ControllerBase { - /** - * The renderer service. - * - * @var \Drupal\Core\Render\RendererInterface - */ - private $renderer; - /** * The controller constructor. * * @param \Drupal\Core\Render\RendererInterface $renderer * The renderer service. */ - public function __construct(RendererInterface $renderer) { - $this->renderer = $renderer; - } - - /** - * {@inheritdoc} - */ - public static function create(ContainerInterface $container): self { - return new self( - $container->get('renderer'), - ); - } + public function __construct( + protected readonly RendererInterface $renderer, + ) {} /** * Returns a raw page for the iframe embed. From 89520218f5e259c627af4eb5d1dddc57be5baef8 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 23:58:59 +0100 Subject: [PATCH 31/38] Issue #56: Avoid Drupal::logger() in the controller. --- src/Controller/ViewerController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index 5ac8a587..5a22fa31 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -56,7 +56,7 @@ public function editor(Media $media, $edit = FALSE) { $error_msg = (string) $this->t('Viewer error: @message', [ '@message' => $render_array ? $render_array['error'] : 'NULL', ]); - \Drupal::logger('cool')->error($error_msg); + $this->getLogger('cool')->error($error_msg); return new Response( $error_msg, Response::HTTP_BAD_REQUEST, From e4d0cb98abcc0b5cdb22f53ecd39b6d089e2c6de Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 14 Nov 2024 00:18:22 +0100 Subject: [PATCH 32/38] Issue #56: Call ->getWopiClientURL() from the controller, and pass the value to getViewerRender(). --- src/Controller/ViewerController.php | 21 ++++++++++++++++----- src/Cool/CoolUtils.php | 18 +++--------------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index 5a22fa31..37c15c99 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -12,7 +12,9 @@ namespace Drupal\collabora_online\Controller; +use Drupal\collabora_online\Cool\CollaboraDiscovery; use Drupal\collabora_online\Cool\CoolUtils; +use Drupal\collabora_online\Exception\CollaboraNotAvailableException; use Drupal\Core\Controller\ControllerBase; use Drupal\Core\Render\RendererInterface; use Drupal\media\Entity\Media; @@ -26,10 +28,13 @@ class ViewerController extends ControllerBase { /** * The controller constructor. * + * @param \Drupal\collabora_online\Cool\CollaboraDiscovery $discovery + * Service to fetch a WOPI client URL. * @param \Drupal\Core\Render\RendererInterface $renderer * The renderer service. */ public function __construct( + protected readonly CollaboraDiscovery $discovery, protected readonly RendererInterface $renderer, ) {} @@ -50,20 +55,26 @@ public function editor(Media $media, $edit = FALSE) { 'closebutton' => 'true', ]; - $render_array = CoolUtils::getViewerRender($media, $edit, $options); - - if (!$render_array || array_key_exists('error', $render_array)) { + try { + $wopi_client_url = $this->discovery->getWopiClientURL(); + } + catch (CollaboraNotAvailableException $e) { + $error_msg = $this->t('The Collabora Online server is not available: @message', [ + '@message' => $e->getCode() . ': ' . $e->getMessage(), + ]); $error_msg = (string) $this->t('Viewer error: @message', [ - '@message' => $render_array ? $render_array['error'] : 'NULL', + '@message' => $error_msg, ]); $this->getLogger('cool')->error($error_msg); return new Response( $error_msg, Response::HTTP_BAD_REQUEST, - ['content-type' => 'text/plain'] + ['content-type' => 'text/plain'], ); } + $render_array = CoolUtils::getViewerRender($media, $wopi_client_url, $edit, $options); + $render_array['#theme'] = 'collabora_online_full'; $render_array['#attached']['library'][] = 'collabora_online/cool.frame'; diff --git a/src/Cool/CoolUtils.php b/src/Cool/CoolUtils.php index ad7aa9da..b6960228 100644 --- a/src/Cool/CoolUtils.php +++ b/src/Cool/CoolUtils.php @@ -12,7 +12,6 @@ namespace Drupal\collabora_online\Cool; -use Drupal\collabora_online\Exception\CollaboraNotAvailableException; use Drupal\Core\Url; use Drupal\file\Entity\File; use Drupal\media\Entity\Media; @@ -233,6 +232,8 @@ public static function getEditorUrl(Media $media, $can_write = FALSE) { * * @param \Drupal\media\Entity\Media $media * The media entity to view / edit. + * @param string $wopi_client + * The WOPI client url. * @param bool $can_write * Whether this is a viewer (false) or an edit (true). Permissions will * also be checked. @@ -243,24 +244,11 @@ public static function getEditorUrl(Media $media, $can_write = FALSE) { * @return array|array{error: string} * A stub render element array, or an array with an error on failure. */ - public static function getViewerRender(Media $media, bool $can_write, $options = NULL) { + public static function getViewerRender(Media $media, string $wopi_client, bool $can_write, $options = NULL) { $default_config = \Drupal::config('collabora_online.settings'); $wopi_base = $default_config->get('cool')['wopi_base']; $allowfullscreen = $default_config->get('cool')['allowfullscreen'] ?? FALSE; - /** @var \Drupal\collabora_online\Cool\CollaboraDiscovery $discovery */ - $discovery = \Drupal::service(CollaboraDiscovery::class); - try { - $wopi_client = $discovery->getWopiClientURL(); - } - catch (CollaboraNotAvailableException $e) { - return [ - 'error' => t('The Collabora Online server is not available: @message', [ - '@message' => $e->getCode() . ': ' . $e->getMessage(), - ]), - ]; - } - $id = $media->id(); $ttl = static::getAccessTokenTtl(); From fd198be656cf593735372e81bcb95488c400698b Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 13 Nov 2024 23:48:32 +0100 Subject: [PATCH 33/38] Issue #56: Validate client url scheme instead of server url scheme. --- src/Controller/ViewerController.php | 21 ++++++++++++++++++++- src/Cool/CollaboraDiscoveryFetcher.php | 12 ------------ 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index 37c15c99..24a18157 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -18,6 +18,7 @@ use Drupal\Core\Controller\ControllerBase; use Drupal\Core\Render\RendererInterface; use Drupal\media\Entity\Media; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; /** @@ -43,6 +44,8 @@ public function __construct( * * @param \Drupal\media\Entity\Media $media * Media entity. + * @param \Symfony\Component\HttpFoundation\Request $request + * The incoming request. * @param bool $edit * TRUE to open Collabora Online in edit mode. * FALSE to open Collabora Online in readonly mode. @@ -50,7 +53,7 @@ public function __construct( * @return \Symfony\Component\HttpFoundation\Response * Response suitable for iframe, without the usual page decorations. */ - public function editor(Media $media, $edit = FALSE) { + public function editor(Media $media, Request $request, $edit = FALSE) { $options = [ 'closebutton' => 'true', ]; @@ -73,6 +76,22 @@ public function editor(Media $media, $edit = FALSE) { ); } + $current_request_scheme = $request->getScheme(); + if (!str_starts_with($wopi_client_url, $current_request_scheme . '://')) { + $this->getLogger('cool')->error($this->t( + "The current request uses '@current_request_scheme' url scheme, but the Collabora client url is '@wopi_client_url'.", + [ + '@current_request_scheme' => $current_request_scheme, + '@wopi_client_url' => $wopi_client_url, + ], + )); + return new Response( + (string) $this->t('Viewer error: Protocol mismatch.'), + Response::HTTP_BAD_REQUEST, + ['content-type' => 'text/plain'], + ); + } + $render_array = CoolUtils::getViewerRender($media, $wopi_client_url, $edit, $options); $render_array['#theme'] = 'collabora_online_full'; diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index a21f8336..82bd5ec4 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -112,18 +112,6 @@ protected function getWopiClientServerBaseUrl(): string { ); } - $host_scheme = isset($_SERVER['HTTPS']) ? 'https' : 'http'; - if (!str_starts_with($wopi_client_server, $host_scheme . '://')) { - throw new CollaboraNotAvailableException( - sprintf( - "The url scheme '%s' of the current request does not match the url scheme of the configured Collabora Online server address '%s'.", - $host_scheme, - $wopi_client_server, - ), - 202, - ); - } - return $wopi_client_server; } From 7a3eaea3791031d0808548a42811658de5784b87 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 21 Nov 2024 12:10:41 +0100 Subject: [PATCH 34/38] Issue #56: Drop the numeric error codes. --- src/Cool/CollaboraDiscovery.php | 10 ++-------- src/Cool/CollaboraDiscoveryFetcher.php | 19 ++++++------------- tests/src/ExistingSite/FetchClientUrlTest.php | 1 - 3 files changed, 8 insertions(+), 22 deletions(-) diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 5282acf4..3c3b285b 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -47,18 +47,12 @@ public function getWopiClientURL(string $mimetype = 'text/plain'): string { $discovery_parsed = simplexml_load_string($xml); if (!$discovery_parsed) { - throw new CollaboraNotAvailableException( - 'The retrieved discovery.xml file is not a valid XML file.', - 102, - ); + throw new CollaboraNotAvailableException('The retrieved discovery.xml file is not a valid XML file.'); } $result = $discovery_parsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', $mimetype)); if (empty($result[0]['urlsrc'][0])) { - throw new CollaboraNotAvailableException( - 'The requested mime type is not handled.', - 103, - ); + throw new CollaboraNotAvailableException('The requested mime type is not handled.'); } return (string) $result[0]['urlsrc'][0]; diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 82bd5ec4..600893ef 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -74,8 +74,7 @@ public function getDiscoveryXml(): string { ]); throw new CollaboraNotAvailableException( 'Not able to retrieve the discovery.xml file from the Collabora Online server.', - 203, - $e, + previous: $e, ); } return $xml; @@ -95,21 +94,15 @@ protected function getWopiClientServerBaseUrl(): string { $cool_settings = $this->loadSettings(); $wopi_client_server = $cool_settings['server'] ?? NULL; if (!$wopi_client_server) { - throw new CollaboraNotAvailableException( - 'The configured Collabora Online server address is empty.', - 201, - ); + throw new CollaboraNotAvailableException('The configured Collabora Online server address is empty.'); } $wopi_client_server = trim($wopi_client_server); if (!str_starts_with($wopi_client_server, 'http://') && !str_starts_with($wopi_client_server, 'https://')) { - throw new CollaboraNotAvailableException( - sprintf( - "The configured Collabora Online server address must begin with 'http://' or 'https://'. Found '%s'.", - $wopi_client_server, - ), - 204, - ); + throw new CollaboraNotAvailableException(sprintf( + "The configured Collabora Online server address must begin with 'http://' or 'https://'. Found '%s'.", + $wopi_client_server, + )); } return $wopi_client_server; diff --git a/tests/src/ExistingSite/FetchClientUrlTest.php b/tests/src/ExistingSite/FetchClientUrlTest.php index c127b05b..9d1f04cb 100644 --- a/tests/src/ExistingSite/FetchClientUrlTest.php +++ b/tests/src/ExistingSite/FetchClientUrlTest.php @@ -51,7 +51,6 @@ public function testFetchClientUrlWithMisconfiguration(): void { $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessage("The configured Collabora Online server address must begin with 'http://' or 'https://'. Found 'httx://example.com'."); - $this->expectExceptionCode(204); $discovery->getWopiClientURL(); } From 705985c734ca005c9bcbbfafcb3864b05dfc2142 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Thu, 21 Nov 2024 22:59:30 +0100 Subject: [PATCH 35/38] Issue #56: Don't disclose exception message to visitor. --- src/Controller/ViewerController.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index 24a18157..55bfc80f 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -17,6 +17,7 @@ use Drupal\collabora_online\Exception\CollaboraNotAvailableException; use Drupal\Core\Controller\ControllerBase; use Drupal\Core\Render\RendererInterface; +use Drupal\Core\Utility\Error; use Drupal\media\Entity\Media; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -62,15 +63,12 @@ public function editor(Media $media, Request $request, $edit = FALSE) { $wopi_client_url = $this->discovery->getWopiClientURL(); } catch (CollaboraNotAvailableException $e) { - $error_msg = $this->t('The Collabora Online server is not available: @message', [ - '@message' => $e->getCode() . ': ' . $e->getMessage(), - ]); - $error_msg = (string) $this->t('Viewer error: @message', [ - '@message' => $error_msg, - ]); - $this->getLogger('cool')->error($error_msg); + $this->getLogger('cool')->warning( + "Collabora Online is not available.
\n" . Error::DEFAULT_ERROR_MESSAGE, + Error::decodeException($e) + [], + ); return new Response( - $error_msg, + (string) $this->t('The Collabora Online editor/viewer is not available.'), Response::HTTP_BAD_REQUEST, ['content-type' => 'text/plain'], ); From adc2b6dd3631f1516935ca7ad89064a61442fe1f Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 27 Nov 2024 11:07:51 +0100 Subject: [PATCH 36/38] Issue #56: Use MediaInterface as parameter type. --- src/Controller/ViewerController.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index 55bfc80f..e1059f83 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -18,7 +18,7 @@ use Drupal\Core\Controller\ControllerBase; use Drupal\Core\Render\RendererInterface; use Drupal\Core\Utility\Error; -use Drupal\media\Entity\Media; +use Drupal\media\MediaInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -43,7 +43,7 @@ public function __construct( /** * Returns a raw page for the iframe embed. * - * @param \Drupal\media\Entity\Media $media + * @param \Drupal\media\MediaInterface $media * Media entity. * @param \Symfony\Component\HttpFoundation\Request $request * The incoming request. @@ -54,7 +54,7 @@ public function __construct( * @return \Symfony\Component\HttpFoundation\Response * Response suitable for iframe, without the usual page decorations. */ - public function editor(Media $media, Request $request, $edit = FALSE) { + public function editor(MediaInterface $media, Request $request, $edit = FALSE) { $options = [ 'closebutton' => 'true', ]; From d9ce6af1cee24527f9cde6448be65951f5692d3a Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Wed, 27 Nov 2024 12:42:35 +0100 Subject: [PATCH 37/38] Issue #56: Add strict_types declaration, don't rely on implicit cast. --- src/Controller/ViewerController.php | 4 +++- src/Cool/CollaboraDiscovery.php | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index e1059f83..bf35410f 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -10,6 +10,8 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +declare(strict_types=1); + namespace Drupal\collabora_online\Controller; use Drupal\collabora_online\Cool\CollaboraDiscovery; @@ -96,7 +98,7 @@ public function editor(MediaInterface $media, Request $request, $edit = FALSE) { $render_array['#attached']['library'][] = 'collabora_online/cool.frame'; $response = new Response(); - $response->setContent($this->renderer->renderRoot($render_array)); + $response->setContent((string) $this->renderer->renderRoot($render_array)); return $response; } diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 3c3b285b..2f761e4d 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -10,6 +10,8 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +declare(strict_types=1); + namespace Drupal\collabora_online\Cool; use Drupal\collabora_online\Exception\CollaboraNotAvailableException; From e663b8d5923283ab971d1ddd4fab822c39e46fca Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 3 Dec 2024 10:00:18 +0100 Subject: [PATCH 38/38] Issue #56: Introduce interfaces. --- collabora_online.services.yml | 6 ++- src/Controller/ViewerController.php | 6 +-- src/Cool/CollaboraDiscovery.php | 18 ++------- src/Cool/CollaboraDiscoveryFetcher.php | 10 +---- .../CollaboraDiscoveryFetcherInterface.php | 33 +++++++++++++++++ src/Cool/CollaboraDiscoveryInterface.php | 37 +++++++++++++++++++ tests/src/ExistingSite/FetchClientUrlTest.php | 10 ++--- 7 files changed, 88 insertions(+), 32 deletions(-) create mode 100644 src/Cool/CollaboraDiscoveryFetcherInterface.php create mode 100644 src/Cool/CollaboraDiscoveryInterface.php diff --git a/collabora_online.services.yml b/collabora_online.services.yml index b548f9b3..a630a6ac 100644 --- a/collabora_online.services.yml +++ b/collabora_online.services.yml @@ -4,5 +4,7 @@ services: logger.channel.collabora_online: parent: logger.channel_base arguments: ['cool'] - Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher: { } - Drupal\collabora_online\Cool\CollaboraDiscovery: { } + Drupal\collabora_online\Cool\CollaboraDiscoveryFetcherInterface: + class: Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher + Drupal\collabora_online\Cool\CollaboraDiscoveryInterface: + class: Drupal\collabora_online\Cool\CollaboraDiscovery diff --git a/src/Controller/ViewerController.php b/src/Controller/ViewerController.php index bf35410f..c83714f1 100644 --- a/src/Controller/ViewerController.php +++ b/src/Controller/ViewerController.php @@ -14,7 +14,7 @@ namespace Drupal\collabora_online\Controller; -use Drupal\collabora_online\Cool\CollaboraDiscovery; +use Drupal\collabora_online\Cool\CollaboraDiscoveryInterface; use Drupal\collabora_online\Cool\CoolUtils; use Drupal\collabora_online\Exception\CollaboraNotAvailableException; use Drupal\Core\Controller\ControllerBase; @@ -32,13 +32,13 @@ class ViewerController extends ControllerBase { /** * The controller constructor. * - * @param \Drupal\collabora_online\Cool\CollaboraDiscovery $discovery + * @param \Drupal\collabora_online\Cool\CollaboraDiscoveryInterface $discovery * Service to fetch a WOPI client URL. * @param \Drupal\Core\Render\RendererInterface $renderer * The renderer service. */ public function __construct( - protected readonly CollaboraDiscovery $discovery, + protected readonly CollaboraDiscoveryInterface $discovery, protected readonly RendererInterface $renderer, ) {} diff --git a/src/Cool/CollaboraDiscovery.php b/src/Cool/CollaboraDiscovery.php index 2f761e4d..e11adb2e 100644 --- a/src/Cool/CollaboraDiscovery.php +++ b/src/Cool/CollaboraDiscovery.php @@ -19,30 +19,20 @@ /** * Service to get a WOPI client url for a given MIME type. */ -class CollaboraDiscovery { +class CollaboraDiscovery implements CollaboraDiscoveryInterface { /** * Constructor. * - * @param \Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher $discoveryFetcher + * @param \Drupal\collabora_online\Cool\CollaboraDiscoveryFetcherInterface $discoveryFetcher * Service to load the discovery.xml from the Collabora server. */ public function __construct( - protected readonly CollaboraDiscoveryFetcher $discoveryFetcher, + protected readonly CollaboraDiscoveryFetcherInterface $discoveryFetcher, ) {} /** - * Gets the URL for the WOPI client. - * - * @param string $mimetype - * 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. - * - * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException - * The client url cannot be retrieved. + * {@inheritdoc} */ public function getWopiClientURL(string $mimetype = 'text/plain'): string { $xml = $this->discoveryFetcher->getDiscoveryXml(); diff --git a/src/Cool/CollaboraDiscoveryFetcher.php b/src/Cool/CollaboraDiscoveryFetcher.php index 600893ef..290647b2 100644 --- a/src/Cool/CollaboraDiscoveryFetcher.php +++ b/src/Cool/CollaboraDiscoveryFetcher.php @@ -25,7 +25,7 @@ /** * Service to load the discovery.xml from the Collabora server. */ -class CollaboraDiscoveryFetcher { +class CollaboraDiscoveryFetcher implements CollaboraDiscoveryFetcherInterface { /** * Constructor. @@ -45,13 +45,7 @@ public function __construct( ) {} /** - * Gets the contents of discovery.xml from the Collabora server. - * - * @return string - * The full contents of discovery.xml. - * - * @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException - * The client url cannot be retrieved. + * {@inheritdoc} */ public function getDiscoveryXml(): string { $discovery_url = $this->getWopiClientServerBaseUrl() . '/hosting/discovery'; diff --git a/src/Cool/CollaboraDiscoveryFetcherInterface.php b/src/Cool/CollaboraDiscoveryFetcherInterface.php new file mode 100644 index 00000000..726663dd --- /dev/null +++ b/src/Cool/CollaboraDiscoveryFetcherInterface.php @@ -0,0 +1,33 @@ +getWopiClientURL(); // The protocol, domain and port are known when this test runs in the // docker-compose setup. @@ -46,8 +46,8 @@ public function testFetchClientUrlWithMisconfiguration(): void { 'server' => 'httx://example.com', ], ]); - /** @var \Drupal\collabora_online\Cool\CollaboraDiscovery $discovery */ - $discovery = \Drupal::service(CollaboraDiscovery::class); + /** @var \Drupal\collabora_online\Cool\CollaboraDiscoveryInterface $discovery */ + $discovery = \Drupal::service(CollaboraDiscoveryInterface::class); $this->expectException(CollaboraNotAvailableException::class); $this->expectExceptionMessage("The configured Collabora Online server address must begin with 'http://' or 'https://'. Found 'httx://example.com'.");