Include settings for the persistent locks

[jvillafanez]

Description

Include a configuration page in the "storage" section (to be reviewed) to configure the persistent locks.
Default values remain the same as they were before: 30 minutes by default and a maximum of 1 day

Related Issue

Related to https://github.com/owncloud/enterprise/issues/3131#issuecomment-554302666

Motivation and Context

Adjust lock timeouts in order to avoid the files to be locked forever

How Has This Been Tested?

1. Change configuration values: default timeout 60 secs, maximum timeout 120 secs
2. Check that trying to lock a file with an infinite timeout sets the lock with 120 secs of timeout. Same for any timeout greater than 120 secs

$ curl -v -u admin:Password -X LOCK -H "Timeout: Infinite" 'http://10.0.2.27:6080/remote.php/webdav/welcome.txt' -d "<?xml version='1.0' encoding='UTF-8'?><d:lockinfo xmlns:d='DAV:'><d:lockscope><d:exclusive/></d:lockscope></d:lockinfo>"
<?xml version="1.0"?>
<d:prop xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
 <d:lockdiscovery>
  <d:activelock>
   <d:lockscope>
    <d:exclusive/>
   </d:lockscope>
   <d:locktype>
    <d:write/>
   </d:locktype>
   <d:lockroot>
    <d:href>welcome.txt</d:href>
   </d:lockroot>
   <d:depth>infinity</d:depth>
   <d:timeout>Second-120</d:timeout>
   <d:locktoken>
    <d:href>opaquelocktoken:7df9d844-b368-4705-849c-e0ef1d3a0f40</d:href>
   </d:locktoken>
  </d:activelock>
 </d:lockdiscovery>
</d:prop>

3. Check that setting a lock without a timeout sets the lock with a timeout of 60 secs

$ curl -v -u admin:Password -X LOCK 'http://10.0.2.27:6080/remote.php/webdav/welcome.txt' -d "<?xml version='1.0' encoding='UTF-8'?><d:lockinfo xmlns:d='DAV:'><d:lockscope><d:exclusive/></d:lockscope></d:lockinfo>"
<?xml version="1.0"?>
<d:prop xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
 <d:lockdiscovery>
  <d:activelock>
   <d:lockscope>
    <d:exclusive/>
   </d:lockscope>
   <d:locktype>
    <d:write/>
   </d:locktype>
   <d:lockroot>
    <d:href>welcome.txt</d:href>
   </d:lockroot>
   <d:depth>infinity</d:depth>
   <d:timeout>Second-60</d:timeout>
   <d:locktoken>
    <d:href>opaquelocktoken:ad772384-9e80-4b1d-bccf-d7abc0c0fd89</d:href>
   </d:locktoken>
  </d:activelock>
 </d:lockdiscovery>
</d:prop>

4. Check that setting a lock with a valid timeout within the maximum range set the right value

$ curl -v -u admin:Password -X LOCK -H "Timeout: Second-88" 'http://10.0.2.27:6080/remote.php/webdav/welcome.txt' -d "<?xml version='1.0' encoding='UTF-8'?><d:lockinfo xmlns:d='DAV:'><d:lockscope><d:exclusive/></d:lockscope></d:lockinfo>"
<?xml version="1.0"?>
<d:prop xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
 <d:lockdiscovery>
  <d:activelock>
   <d:lockscope>
    <d:exclusive/>
   </d:lockscope>
   <d:locktype>
    <d:write/>
   </d:locktype>
   <d:lockroot>
    <d:href>welcome.txt</d:href>
   </d:lockroot>
   <d:depth>infinity</d:depth>
   <d:timeout>Second-88</d:timeout>
   <d:locktoken>
    <d:href>opaquelocktoken:c500a662-cbf9-46a7-83a3-60360ee59cef</d:href>
   </d:locktoken>
  </d:activelock>
 </d:lockdiscovery>
</d:prop>

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

As said, default values are 30 minutes for the default timeout and 1 day for the maximum timeout. Note that it isn't possible to reset those values at the moment (the admin must set them again by himself if wanted)

@pmaier1 to check the placement, section, wording, and other things. Note sure if the storage section is the best place taking into account there are problems with the lock and external storages (all the users should use the same account for the locks to work properly, otherwise the locks will only lock the file "seen" by the user, but not the rest - the storages are different). It might be confusing.

[pmaier1]

Thanks!

• I'd go for the headline "Manual File Locking"
• "Storage" section doesn't fit that well for me. I'd put it under "Additional" for now.
• Shouldn't we allow more than 1 day as maximum?
• Shouldn't we set the default a little higher? Say, 6 hours maybe?
• If a user can't deviate from the default (which is not planned right now), does it make sense to have a default and maximum value? Actually I only want to configure how long a lock will stay until it's unlocked automatically.
• Should the config interface be part of the file locking frontend app? (without the frontend the settings do not make that much sense from a user POV, I think)

[codecov]

Codecov Report

Merging #36452 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master   #36452   +/-   ##
=========================================
  Coverage     64.66%   64.67%           
- Complexity    19343    19350    +7     
=========================================
  Files          1279     1281    +2     
  Lines         75600    75623   +23     
  Branches       1333     1333           
=========================================
+ Hits          48885    48907   +22     
- Misses        26323    26324    +1     
  Partials        392      392           

Flag	Coverage Δ	Complexity Δ
#javascript	54.03% <ø> (ø)	0.00 <ø> (ø)
#phpunit	65.84% <100.00%> (+0.01%)	19350.00 <4.00> (+7.00)

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Lock/Persistent/LockManager.php	98.27% <100.00%> (+0.12%)	23.00 <0.00> (+1.00)
lib/private/Settings/SettingsManager.php	72.95% <100.00%> (+0.17%)	44.00 <0.00> (ø)
settings/Panels/Admin/PersistentLocking.php	100.00% <100.00%> (ø)	4.00 <4.00> (?)
...tings/templates/panels/admin/persistentlocking.php	100.00% <100.00%> (ø)	0.00 <0.00> (?)
lib/private/Archive/Archive.php	65.00% <0.00%> (-3.43%)	11.00% <0.00%> (+1.00%)	⬇️
..._sharing/lib/Controller/NotificationController.php	42.10% <0.00%> (ø)	14.00% <0.00%> (ø%)
lib/private/Share/MailNotifications.php	95.74% <0.00%> (+0.09%)	33.00% <0.00%> (ø%)

---

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 ab72fd5...a992f40. Read the comment docs.

[jvillafanez]

Section and header changed.

Shouldn't we allow more than 1 day as maximum?

I don't think someone needs to lock a file for more than 1 day, but in any case the admin can change that value to allow a higher timeout.
In theory, you can refresh the lock to extend the timeout, although I haven't tested yet.

Shouldn't we set the default a little higher? Say, 6 hours maybe

I guess it depends on what is the expected usage and the expected tools:
For remote edition using libreoffice, maybe 30 minutes is enough or maybe we can increase the default to 1 hour. I don't expect users to be editing a remote file for 6 hours.
For other kind of usages, it might make sense to increase the default timeout, although I can't think about a use case which requires such a long timeout.

Should the config interface be part of the file locking frontend app? (without the frontend the settings do not make that much sense from a user POV, I think)

Those values are being used by the LockManager, which is part of core. The API provided through webdav is already using that LockManager.
I've checked that libreoffice already uses locks while opening a remote file through webdav (you can open a remote file and the ownCloud's web UI already shows the lockpad). You can think that the webdav interface already acts as a frontend for these locks.

I'll wait for confirmation on the default values. I'd say we can increase the default timeout to 1 hour, but the maximum timeout looks good to me.

[pmaier1]

I don't think someone needs to lock a file for more than 1 day, but in any case the admin can change that value to allow a higher timeout.

Ok 👍

For remote edition using libreoffice, maybe 30 minutes is enough or maybe we can increase the default to 1 hour. I don't expect users to be editing a remote file for 6 hours.

Well, I expect scenarios where someone locks a file, starts editing, then gets a coffee and talks to colleagues, then continues. All in all it takes 4 hours or so until the file is checked back in. Anyway, as you like, the admin can set the value as desired .

Good to go from my side! THX

[micbar]

@jvillafanez This change should not be merged without acceptance testing.

[phil-davis]

@micbar is this for 10.4 ?
If so, please add to 10.4 project and add QA-team and dev:acceptance-tests labels and we will add tests.

[jvillafanez]

This is ready from dev's point of view. To be decided if we want to merge it now, wait for additional acceptance tests from QA, or wait until we have the frontend for the persistent locks.

[mmattel]

Ping ?

[micbar]

@pmaier1 we need to test that.

@jvillafanez Please rebase

[mmattel]

Doc relevant ! Pls file a doc issue!

[micbar]

@jvillafanez @phil-davis We need to figure out why the acceptance tests are failing.

[phil-davis]

Note: there is no validity checking in the "Manual File Locking" settings UI:

e.g. I can set the default greater than the maximum:

IMO the code will work OK, it will first set the value to the default (7200 in this example) and then limit it to the max (2000 in this example), and effectively the "default" will be 2000.

I can set negative values:

I don't want to even try what happens to locking with those settings!

I can also put values like 12.345E3, which is OK I guess because that is 12345 seconds. 12.3456E3 makes the input box red, but it still saves it. And similarly if I put 1234.56 the input box goes red, but actually the value seems to be saved. These will probably work anyway, the code calculating when the timeout is reached will cope with fractional seconds anyway.

When I enter abc the input box goes red, and when I refresh, then the value has become the empty string. Making the value empty should be OK, because then the defaults will apply anyhow.

How much effort to put into validating this type of setting that is "done once" by an admin?

[jvillafanez]

I can set the default greater than the maximum

It will behave as you've said. I think everyone would expect the same behaviour, so I don't think we need to change anything

I can set negative values

This should have been fixed in the web UI. I've added a minimum value in the HTML so most of the browsers shouldn't accept negative values. "0" is still permitted, but we might need to set "1" as minimum (I'm not sure how locks will behave)
For browser that still accepts negative values (such as firefox), additional validation has been added in js so prevent saving negative values.

In case negative values end up being saved (via occ for example, which is why I didn't want to add to much validation in the web UI), the behaviour is the following:

1. Set the current value to the default value, no matter if it's negative
2. If a value has been sent by the user, overwrite the current value with it, no matter if it's negative
3. Get the max value.
4. If the max value is negative, use the "default" max value (1 day) instead
5. If the current value is negative or over the max value, use the max value (step 4).

This way we ensure we have a reasonable timeout.

[jvillafanez]

I think we might need some kind of visual indicator if the value isn't in range (not positive) to show the value is wrong. Firefox handles this by itself showing a red border around, but not chrome.
For chrome, the user can enter a negative value without receiving any feedback. The value won't be saved, but the admin won't know the value is wrong.

Taking into account the admin shouldn't try to set a negative value there because it doesn't make sense, we could skip this visual indication and blame the admin.