Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New feature: implement locking via Lock API #51

Merged
merged 29 commits into from
Mar 2, 2022
Merged

New feature: implement locking via Lock API #51

merged 29 commits into from
Mar 2, 2022

Conversation

glpatcern
Copy link
Member

@glpatcern glpatcern commented Nov 26, 2021

This PR introduces a lock API in the storage interfaces and uses it throughout the WOPI logic instead of directly creating lock files.

The lock API is implemented in the xrootd and local interfaces. Once cs3org/cs3apis#160 is merged and implemented in Reva, the cs3iface will also be implemented. Edit: this was tested in CERNBox QA.

Along with this enhancement, all cases of failures or permission issues when saving a file have been identified and an attempt is made to save a copy to a configured local storage in order to ease later recovery by service operations.
The same process is performed by the bridge extension: this fixes #39.

Highlights of the implementation:

  • Lock operations are now clearly separated in the WOPI code, and abstracted by the storage layers.
  • The xrootd and localfs implementations use xattrs. The xattr content is compatible with the Reva/eos implementation, and this is of course deliberate as we're still running the wopiserver in production over xrootd for backwards compatibility reasons.
  • The storage operations that imply modifications (write, set/rm attr, rename) now also have a lockid argument. This is not enforced anywhere but we expect to have Reva enforce it in the future (cf. cs3apis#162).
  • SetLock is protected against race conditions with itself, but the whole wopiserver logic does suffer (as it did - so not a regression) from a race condition in the following case:
    • Precondition: a file is locked but the lock is expired
    • WOPI SetLock logic: 1) read the existing lock, 2) remove it as expired, 3) set a new one
    • Two users doing WOPI SetLock may race between 1 and 3 (usual non-atomic set-after-check).
  • Locking single-shared files is fully supported now, i.e. the wopiserver does not assume write permissions in the containing folder. In the only case where this is required, i.e. when generating webconflict files, a conflictpath can be configured as a target location: typically for CERN we'd use the user's home path. Those webconflict files ought not to be created any longer once the lock is fully enforced by Reva; yet as users may access the storage via direct mount (outside of Reva), we keep the protection and this logic around it.

Concerning the format of the lock-id payload, we made an attempt to ensure compatibility with WebDAV locks as follows:

  • WebDAV specs require to use as lock-id format
    opaquelocktoken: UUID [Extension]
    where Extension = path, and path can actually be anything.
  • The WOPI server uses therefore: opaquelocktoken:hardcoded_uuid base64-encoded_wopi_lock.

This approach allows the WOPI server to uniquely recognize the WOPI locks from anything else as the [hardcoded] UUID would be its signature. Yet, this implementation may change in the future should this format not be suitable for some reasons or should the WebDAV compatibility be ineffective after all.

@glpatcern glpatcern self-assigned this Nov 26, 2021
@glpatcern glpatcern marked this pull request as draft November 26, 2021 14:12
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2021

This pull request introduces 1 alert when merging c31bee4 into 26b6aa5 - view on LGTM.com

new alerts:

  • 1 for Unused import

@glpatcern glpatcern force-pushed the lockapi branch 4 times, most recently from 2efca41 to 114beb9 Compare November 26, 2021 16:38
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2021

This pull request fixes 1 alert when merging 114beb9 into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2021

This pull request fixes 1 alert when merging 97f126f into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2021

This pull request fixes 1 alert when merging a25cba6 into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@glpatcern glpatcern force-pushed the lockapi branch 2 times, most recently from 1710ac5 to 16ff754 Compare November 26, 2021 22:56
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2021

This pull request fixes 1 alert when merging 16ff754 into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@glpatcern glpatcern force-pushed the lockapi branch 2 times, most recently from a661318 to e30440f Compare November 26, 2021 23:21
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2021

This pull request fixes 1 alert when merging e30440f into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 29, 2021

This pull request fixes 1 alert when merging 5452088 into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 29, 2021

This pull request fixes 1 alert when merging 0d4aa28 into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 29, 2021

This pull request fixes 1 alert when merging a12b247 into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 29, 2021

This pull request fixes 1 alert when merging f67481f into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 30, 2021

This pull request fixes 1 alert when merging c074ec1 into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@glpatcern glpatcern force-pushed the lockapi branch 2 times, most recently from 58fb01f to 88d35ab Compare November 30, 2021 16:50
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 30, 2021

This pull request fixes 1 alert when merging 46aec7a into 5e98007 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

glpatcern and others added 26 commits March 2, 2022 14:08
Use a WebDAV-friendly prefix for the lock_id
and use simple b64 encoding/decoding as opposed to a JWT
In case of failures or permission issues saving a file,
an attempt is made to save a copy to a configured local storage
in order to ease later recovery by service operations.
Same process is performed by the bridge extension: fixes #39.
This includes several unrelated fixes on top of the lock-related APIs.
In particular:
- Save As operations (PutRelative) now respect UTF7 encoding,
  and always return a JSON response
- Where required, the X-WOPI-ItemVersion header is present

Remaining known issues:
- The ItemVersion is based on mtime with a precision of 1 second
- PutRelativeFile.RelativeNameConflictOverwriteFalse fails and
  crashes the WOPI validator suite
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 2, 2022

This pull request introduces 1 alert and fixes 3 when merging 5a208f1 into 91dec37 - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in path expression

fixed alerts:

  • 2 for Information exposure through an exception
  • 1 for Reflected server-side cross-site scripting

@glpatcern glpatcern merged commit d541e91 into master Mar 2, 2022
@glpatcern glpatcern deleted the lockapi branch March 2, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bridge: save to local temporary storage in case of failures
3 participants