Issue #75: WOPI proof #79

donquixote · 2024-12-19T02:22:40Z

Fixes #75

This also contains some refactoring of WopiController, moving access checks to the routing layer.
I could split this out but I would prefer not to..

…overy.

This will allow to use the regular route access layer for proofing and authorization.

…f the custom response.

…st().

… a website.

brummbar · 2024-12-20T15:26:15Z

src/Access/WopiTimeoutAccessCheck.php

+    // Unfortunately, is_numeric() confuses the IDE's static analysis, so use
+    // regular expression instead.
+    if (!preg_match('#^[1-9]\d+$#', $wopi_ticks_str)) {


The IDE should never be the driver for how we write the code. Let's put the is_numeric() call.

brummbar · 2024-12-20T16:17:06Z

src/Access/WopiTimeoutAccessCheck.php

+ *
+ * Note that the X-WOPI-Timestamp is in DotNet ticks.
+ */
+class WopiTimeoutAccessCheck implements AccessInterface {


In the end, the timestamp is used also in the other class, and we basically need one single method from here, while we need to duplicate quite a bit of code. Let's merge all into a single class.

One motivation for the split was the distinct service dependencies.
We can fully keep out the crypto stuff from the timeout checker, and we can fully keep out the dotnet time conversion from the proof checker. We get two independent buckets of control flow.
The shared code is the config check and the timestamp header value.

brummbar · 2024-12-20T16:23:13Z

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


We should provide a post_update hook to set this value.

I was not sure if we have an upgrade path.
But I also don't mind to add it.

brummbar · 2024-12-20T16:27:42Z

src/Access/WopiProofAccessCheck.php

+  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.


Is this actually a possible scenario? Like, is it written in some documentation?
Because this piece of code makes me think that it's part of the WOPI protocol.
What would be the implications if we left not-unique headers?

If the keys are not changed regularly, then we will have identical old and new key, and identical old and new signature. You will see that in the existing discovery.xml.
The only implication of keeping duplicates is that we will do redundant checks. So not a big deal, but also easy enough to avoid.

The redundant checks would only happen in the non-happy path, so it is not really the main scenario to optimize for.
The wopi-lib has explicit vars for old and new key and proof and does the 3 checks explicitly with &&.
We could do that.

Currently with the array_filter() and then foreach() we silently ignore if an old key is missing in discovery.xml, or the old proof is missing in the request.
Instead, we could simply fail if the old key or proof is not present.
This would reveal if e.g. we are using the wrong xpath or wrong header name, or if the WOPI client behaves differently due to an update.

brummbar · 2024-12-20T16:31:33Z

tests/src/Functional/ConfigFormTest.php

    $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');


Let's convert this to checkboxChecked/checkboxNotChecked.

brummbar · 2024-12-20T16:44:12Z

tests/src/Unit/CollaboraDiscoveryTest.php

+  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()],
+    ];
+  }


We can simplify this:

return [ 'getWopiClientURL' => ['getWopiClientURL'], 'getProofKey' => ['getProofKey'], 'getProofKeyOld' => ['getProofKeyOld'], ];

And then just call_user_func([$discovery, $callback]);

It works as long as there are no arguments, which is ok for now.
It also means that static analysis would find usages of the methods.
But I think we can change it, overall it will become simpler, so ok.

brummbar · 2024-12-20T17:01:17Z

src/Access/WopiProofAccessCheck.php

+      'current' => $this->discovery->getProofKey(),
+      'old' => $this->discovery->getProofKeyOld(),


These two methods make each an HTTP request to retrieve the XML. This happens for each request to the three routes.
I found some information about the fact that the proof key is rotated every certain amount of time (https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/online/discovery).
We can check for Collabora, but I wold say that implementing a caching system that keeps the discovery XML cached for one hour will already greatly reduce the amount of requests we make.

Absolutely. I think somewhere it was documented that this should be 20 minutes.
But I wanted to do a dedicated PR to add caching to the discovery.
If we implement caching, we need to determine when this should be reset. We can do the < 20 min TTL, and ofc the dependency on the config record. But perhaps we also need a different mechanism to force this reset.

brummbar · 2024-12-20T17:31:52Z

src/Cool/CollaboraDiscovery.php


 /**
- * Service to get a WOPI client url for a given MIME type.
+ * Service to get a WOPI client URL for a given MIME type.


This is not true now. This service is now a representation of the Collabora discovery file.

brummbar · 2024-12-20T17:34:25Z

README.md

-### Installation steps
+### Collabora Online server preparation
+
+It is recommended to generate a proof key as documented here:\


I think there might be a link missing.

brummbar · 2024-12-20T17:42:11Z

src/Cool/CollaboraDiscovery.php


 /**
- * 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 {


We need to rethink a bit how this service works. Probably we should gather once per initialization the file from the fetcher?
I would like a class that completely wraps the XML, and the single methods return the parsed information from it, like a sort of value object.

I was thinking that too. But, once we have a value object, we can't have it respond to config changes etc.
Perhaps this is all ok, but I would like a separate issue to think about this.

One idea I had was a proxy object that deals with the config etc and which then creates the value object to delegate calls to.
When the config changes, we could drop the value object and create a new one.
Or, do we actually need that? Do we care if config changes at runtime, or only between requests?

As a compromise we can have a property that has the cached parsed xml, and then a persistent cache for the xml string. TBD.

…gFormTest.

donquixote added 6 commits December 19, 2024 03:19

Issue 75: Fix an assertion message.

f6dcaf4

Issue 75: Let WopiController check if token is NULL.

e5c9092

Issue 75: Spell URL uppercase in docs in CollaboraDiscovery*.

6697965

Issue 75: Extract getParsedXml() from getWopiClientUrl().

dcf431a

Issue 75: Extract parseXml() from getWopiClientURL() in CollaboraDisc…

8265c3a

…overy.

Issue 75: Intercept errors from XML parsing.

9125d29

donquixote force-pushed the issue-75-wopi-proof branch from d3465fa to c5b5dea Compare December 19, 2024 02:33

donquixote added 4 commits December 19, 2024 03:46

Issue 75: Add CollaboraDiscoveryTest.

00911b5

Issue 75: Add proof key methods in CollaboraDiscovery.

5dd05c9

Issue 75: Set custom response format for WOPI routes.

fca29b8

This will allow to use the regular route access layer for proofing and authorization.

Issue 75: Throw AccessDeniedHttpException in WopiController instead o…

845564b

…f the custom response.

donquixote force-pushed the issue-75-wopi-proof branch 6 times, most recently from f37d4ff to 9789290 Compare December 19, 2024 14:59

donquixote added 4 commits December 19, 2024 16:06

Issue 75: Extract WopiControllerTestBase from WopiControllerTest.

068299f

Issue 75: Fix query parameters in WopiControllerTestBase::createReque…

d1155ee

…st().

Issue 75: Require 'phpseclib/phpseclib' to handle RSA keys.

3ef1c4a

Issue 75: Add setting for wopi_proof.

7400188

donquixote force-pushed the issue-75-wopi-proof branch 3 times, most recently from a58c0f4 to 6fa5037 Compare December 19, 2024 15:10

donquixote added 3 commits December 19, 2024 18:21

Issue 75: Add proofing to WOPI routes.

a08be93

Issue 75: Generate proof key for demo and ExistingSite tests.

9f017f0

Issue 75: Recommend to generate a proof key when this is installed in…

7ead437

… a website.

donquixote force-pushed the issue-75-wopi-proof branch from 6fa5037 to 7ead437 Compare December 19, 2024 17:21

brummbar requested changes Dec 20, 2024

View reviewed changes

Issue 75: Add a post-update hook to set 'wopi_proof' setting.

8703772

donquixote added 4 commits December 21, 2024 00:32

Issue 75: Combine WopiTimeoutAccessCheck back into WopiProofAccessCheck.

aa43d11

Issue 75: Use ->checkboxChecked() and ->checkboxNotChecked() in Confi…

6424f84

…gFormTest.

Issue 75: Simplify the data provider in CollaboraDiscoveryTest.

2812bb4

Issue 75: Fix class doc on CollaboraDiscovery.

46608d6

donquixote force-pushed the issue-75-wopi-proof branch from b81f05a to 46608d6 Compare December 20, 2024 23:33

Issue 75: Add missing link in README.md.

3c10a1c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #75: WOPI proof #79

Issue #75: WOPI proof #79

donquixote commented Dec 19, 2024

brummbar Dec 20, 2024

brummbar Dec 20, 2024

donquixote Dec 20, 2024

brummbar Dec 20, 2024

donquixote Dec 20, 2024

brummbar Dec 20, 2024

donquixote Dec 20, 2024

donquixote Dec 20, 2024

brummbar Dec 20, 2024

brummbar Dec 20, 2024

donquixote Dec 20, 2024

brummbar Dec 20, 2024

donquixote Dec 20, 2024

brummbar Dec 20, 2024

brummbar Dec 20, 2024

brummbar Dec 20, 2024

donquixote Dec 20, 2024

donquixote Dec 20, 2024

		'current' => $this->discovery->getProofKey(),
		'old' => $this->discovery->getProofKeyOld(),

Issue #75: WOPI proof #79

Are you sure you want to change the base?

Issue #75: WOPI proof #79

Conversation

donquixote commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment