cs3org / wopiserver

A vendor-neutral application gateway compatible with the WOPI specifications.
Apache License 2.0
52 stars 27 forks source link

fix UnLockAndRelock and length of resourceIDs #85

Closed micbar closed 1 year ago

micbar commented 1 year ago

Description

needed to improve #76

WOPI Validator

./Microsoft.Office.WopiValidator -n SuccessfulLockSequence -w https://wopiserver.team.owncloud.works/wopi/files/<fileID> -t <token> -l 1663776371000

Test group: Locks
  Pass: SuccessfulLockSequence
micbar commented 1 year ago

@wkloucek FYI

glpatcern commented 1 year ago

Hello @micbar, thanks for this, happy to merge the CS3API part and the "heuristic", but for the UnlockAndRelock requests a priori the existing code covers them (though I reckon in a convoluted way, I shall add some comments to explain how) - and the test suite does pass as well.

So can we try without that change first? In particular I guess the more permissive heuristic should allow you to run the test suite with your WOPI proxy.

micbar commented 1 year ago

but for the UnlockAndRelock requests a priori the existing code covers them (though I reckon in a convoluted way, I shall add some comments to explain how) - and the test suite does pass as well.

Can you point me to that? In fact the testsuite was failing before my change. I used https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/unlockandrelock as a reference.

So can we try without that change first? In particular I guess the more permissive heuristic should allow you to run the test suite with your WOPI proxy.

Yes. resourceIDs are longer on the edge branch. Other than that we could improve that particular part of the code and do not check the length but the type of the fileID.

glpatcern commented 1 year ago

Yes, that is what I have used as well, but it's covered (and in a more atomic way) by the check at https://github.com/cs3org/wopiserver/blob/master/src/core/wopi.py#L250

And the screenshot in #76 shows that the lock tests are all green

glpatcern commented 1 year ago

Yes. resourceIDs are longer on the edge branch. Other than that we could improve that particular part of the code and do not check the length but the type of the fileID.

And yes I suspected that. Agreed that we could also try to check if the fileID looks like a JWT, but probably the heuristic is good enough and we could raise the threshold even more

micbar commented 1 year ago

Yes, that is what I have used as well, but it's covered (and in a more atomic way) by the check at https://github.com/cs3org/wopiserver/blob/master/src/core/wopi.py#L250

And the screenshot in #76 shows that the lock tests are all green

Let me explain:

The UnlockAndRelock operation:

This operation is basically a LOCK operation which sends a new Lock ID and also the old Lock ID.

UnlockAndRelock is similar semantically to the [Lock](https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/lock) operation. 
The two operations share the same X-WOPI-Override value. 
So, hosts must differentiate the two operations based on the presence, or lack of, the X-WOPI-OldLock request header.

I think that https://github.com/cs3org/wopiserver/blob/master/src/core/wopi.py#L250 doesn't cover it.

We need to check if the Lockop == LOCK and when an X-WOPI-OldLock is set we should do a unlock operation.

glpatcern commented 1 year ago

OK, now I understand why we have different results. The xroot storage interface does cover that case, but not the CS3API storage interface. Yet, the CS3API storage interface is correct according to the CS3API specifications, in the sense that refreshLock fails in that case when oldLock is given and is different from the given lock.

So on one side this PR is OK, and on the other side I'll look if we can have a better approximation of atomicity of the unlock+lock operation (not that obvious of course), plus make the xroot layer behave exactly like the CS3 one.