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

public.php and public-files endpoints won't report locking support #36402

Merged
merged 6 commits into from Dec 6, 2019

Conversation

jvillafanez
Copy link
Member

Description

Remove support for persistent locking in the public endpoints. Requested in #34394 (comment)
Note that locking wasn't supported in public endpoints as seen in https://github.com/owncloud/core/blob/master/apps/dav/lib/Files/FileLocksBackend.php#L166-L168 . This PR will remove the lockdiscovery from the propfind response

Related Issue

#34394

Motivation and Context

Libreoffice was showing an error trying to lock the file through the public endpoint. Now it won't try to lock the file.

How Has This Been Tested?

Manually tested using the steps defined in #34394

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@jvillafanez
Copy link
Member Author

jvillafanez commented Nov 12, 2019

Need to check if this is the approach we want or not. The PR fixes the issue mentioned, but it break quite a bunch of lock tests, which would need to be removed or adjusted.
There are other tests failing that don't seem to be related with the changes. We need to double-check those.

Note that this could be dangerous if the file is locked and someone edits the file through the public link at the same time.

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #36402 into master will decrease coverage by 0.3%.
The diff coverage is 69.69%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36402      +/-   ##
============================================
- Coverage     65.16%   64.86%   -0.31%     
- Complexity    19802    19815      +13     
============================================
  Files          1271     1272       +1     
  Lines         74760    74790      +30     
  Branches       1309     1309              
============================================
- Hits          48719    48513     -206     
- Misses        25655    25891     +236     
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.05% <69.69%> (-0.34%) 19815 <12> (+13)
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Server.php 51.87% <57.14%> (-0.39%) 31 <2> (+3)
...s/dav/lib/Connector/Sabre/PublicDavLocksPlugin.php 72.72% <72.72%> (ø) 9 <9> (?)
apps/dav/lib/Connector/Sabre/ServerFactory.php 94.8% <75%> (-1.15%) 11 <1> (+1)
apps/files_external/lib/Lib/Storage/SMB.php 0% <0%> (-52.2%) 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f70c0ba...62836e8. Read the comment docs.

@jvillafanez
Copy link
Member Author

Need to check if this is the approach we want or not. The PR fixes the issue mentioned, but it break quite a bunch of lock tests, which would need to be removed or adjusted.
There are other tests failing that don't seem to be related with the changes. We need to double-check those.

This is fixed. Failing test caused by a infrastructure problem.

Note that this could be dangerous if the file is locked and someone edits the file through the public link at the same time.

Still happening. The file is edited through the public link even though the file is locked.

For public links:
* LOCK and UNLOCK methods won't be reported as allowed
* lock information (d:lockdiscovery) and support (d:supportedlock) will
be shown in the propfind if requested (lock support is needed for
libreoffice to try to lock the file)
* trying to lock the file through public link will either throw a Locked
exception if there is a lock in place, or MethodNotAllowed if there are
no locks (the Locked exception is needed for libreoffice to show a popup
warning the user that the file is locked)
@jvillafanez
Copy link
Member Author

Copying from the commit:

For public links:

  • LOCK and UNLOCK methods won't be reported as allowed
  • lock information (d:lockdiscovery) and support (d:supportedlock) will
    be shown in the propfind if requested (lock support is needed for
    libreoffice to try to lock the file)
  • trying to lock the file through public link will either throw a Locked
    exception if there is a lock in place, or MethodNotAllowed if there are
    no locks (the Locked exception is needed for libreoffice to show a popup
    warning the user that the file is locked)

This means that the file can't be locked through the public link, but both the "Locked" exception trying to unlock and also the information in the propfind should be enough information to know if the file is locked or not.
If the file is locked, libreoffice will show a popup warning the user that the file is locked and he can open the file as read-only or a copy (this seems quite nice because the user knows there is someone editing the file - no user info is shown though). If the file isn't locked, the user can edit the file normally, but the file won't be locked.

There is still some polishing to do in the PR, but I think this is the behaviour we want.

@jvillafanez
Copy link
Member Author

coverage upload errors.... the tests are fine

@phil-davis
Copy link
Contributor

@micbar this is in the current server sprint.
Are we aiming to merge it for 10.4?
There are a few small and good changes to acceptance tests here - just needs a quick review to think if there are extra scenarios worth adding.

@phil-davis
Copy link
Contributor

Note that this PR does not fix #36064 - if a folder/file has been locked by the owner, and a public link share of it has been made, then the public can still upload files etc into the locked folder...
See the scenarios in tests/acceptance/features/apiWebdavLocks/publicLink.feature

So this PR is stage1, fixing up a bit of locking support stuff. But actually it will be good to also "some day soon" address #36064 so that, e.g. public uploads into a locked folder will get rejected.

@phil-davis
Copy link
Contributor

There are a few small and good changes to acceptance tests here - just needs a quick review to think if there are extra scenarios worth adding.

This looks OK to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants