implement persistent locking

[mrow4a]

fixes: https://github.com/owncloud/enterprise/issues/5515

This PR makes sure the below scenario is working

- Create a text file with content
- Share the file with full permission to Bob
- Lock the file using Manual File Locking
- Bob sees that the file is locked
- Bob cannot make changes with Texteditor, the file is not possible to be saved

Additionally, it implements locking mechanism for text editor.

• add tests

DEMO

• alice shares with:

  • brian with read-only permission
  • carol with edit permission

• alice starts editing the file
  

• carol can see that alice locked a file
  
  

• when alice finishes the lock is gone (editor is closed)
  

• brian has only read-only permissions, so can only check contents and message is shown to him
  

[CLAassistant]

All committers have signed the CLA.

[jnweiger]

To be included in the 10.12.0 server release. https://github.com/owncloud/enterprise/issues/5545

[phil-davis]

I only found one scenario that does not work as I would expect:

1. Alice creates a text file t.txt
2. Alice shares t.txt read-write with Brian
3. Alice locks t.txt
4. Brian opens t.txt, he gets the editor in read-only mode, it says that the file is locked by Alice - good
5. Alice opens t.txt
   The editor opens in read-only mode.
   Alice cannot edit t.txt - bad

The checks for a locked file need to understand if the lock owner is the current user.

[mrow4a]

@phil-davis I do not agree. Text Editor aApp really works like you would upload a file. Can you try uploading a file via UI and see the outcome in the above scenario?

If this would be collaborative it would be different.

[phil-davis]

@phil-davis I do not agree. Text Editor aApp really works like you would upload a file. Can you try uploading a file via UI and see the outcome in the above scenario?

If this would be collaborative it would be different.

OK, well I don't understand the use-case for Alice to lock a file. But as you say, if Alice locks the file and then Alice tries to upload-overwrite the file, then the upload is blocked. So this is an existing behavior.

[mrow4a]

OK, well I don't understand the use-case for Alice to lock a file. But as you say, if Alice locks the file and then Alice tries to upload-overwrite the file, then the upload is blocked. So this is an existing behavior.

Long story short:

• file can be locked because other app locked it e.g. wopi as part of its collaborative session, only people collaborating using that editors lock can edit the file
• file can be manually locked, this use-case is when user forces a lock on a file. Currently it is weird behaviour, because in fact the user locks also himself until lock is released. This is behaviour using WebUI, OnlyOffice and now TextEditor. I am not sure other clients like Sync Client.

[ownclouders]

💥 Acceptance tests pipeline webUITextEditor-master-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/files_texteditor/1945/12

[phil-davis]

@DeepDiver1975 please review again.

[jnweiger]

I tend to agree with Phil: A manual lock would be most useful, if the user who locked the file, can still upload the file.

https://doc.owncloud.com/server/next/admin_manual/configuration/files/manual_file_locking.html#introduction says:
"Manual file locking allows users, if enabled, to lock files in shared areas while working on them in order to prevent concurrent changes from other users (check-in/check-out)."

@hodyroff But this is a broader topic. Currently (10.11) the owner cannot overwrite a file by uploading, while locked.
I am absolutely unsure, if this works as designed or not. -> owncloud/core#40637

[mrow4a]

@jnweiger it is more complex then this. Apps can set their own locks, like Wopi/MSOFFICE. Even if you own the file, you SHOULD NOT be able to break that lock by just overwriting a file that is in collaborative session. You would need to click on "release lock button" first to be able do that. However if file is in collaborative session, and you break the lock.. ye it asks for problems as work in progress in MSOFFICE app could be lost..

[hodyroff]

The ability to break a lock comes with responsibilities. Yes, works as designed. If locked, no upload shall be possible. Unlock and then you can overwrite ... which also means you can then wait to see if any desktop clients will do their scheduled work after unlock ...

[jnweiger]

Argh, I did not want to sidetrack here. Made a separate issue meanwhile.

Hmm, I did not expect that 'manual locking' is the same as app locking. Sure thing: wopi or text editor should do their own locking to prevent oven the same user to mess with the file, while the editor is open.

[mrow4a]

@jnweiger I agree, manual lock is a disputable matter. But how do I distinguish manual lock from wopi lock? this would require me to embeed logic from other apps into this one 😉

[hodyroff]

It must be the same, there is only one WebDAV lock ... which is also adhered to by all WebDAV Clients, not only the ownCloud once!

[mrow4a]

@DeepDiver1975 @phil-davis locking is now implemented, description adjusted.

[phil-davis]

It works for me.

The only hassle is if a do:

• Alice shares file.txt read-write with Brian
• Brian edits the file with files_texteditor
• Brian closes his browser tab/window without first closing the text-editor

Now the file is left locked by Brian. It will be stuck like that until the lock times out, or a user with the priv to delete locks does delete the lock. Alice cannot edit the file.

And Brain won't realize what he has done, because the files_texteditor app took out the lock on behalf of Brian. Brian did not click anything to lock the file. So Brian is not likely to even be aware that he has a lock on the fie.

But I think that these kind of user sequences are beyond the scope of this PR.

[phil-davis]

LGTM - manually testing with multiple users editing and viewing the file works as advertised.

It won't be so easy to make automate UI tests for those sequences, because it requires having multiple users logged in at the same time.

[hodyroff]

It works for me.

The only hassle is if a do:

• Alice shares file.txt read-write with Brian
• Brian edits the file with files_texteditor
• Brian closes his browser tab/window without first closing the text-editor

Now the file is left locked by Brian. It will be stuck like that until the lock times out, or a user with the priv to delete locks does delete the lock. Alice cannot edit the file.

And Brain won't realize what he has done, because the files_texteditor app took out the lock on behalf of Brian. Brian did not click anything to lock the file. So Brian is not likely to even be aware that he has a lock on the fie.

But I think that these kind of user sequences are beyond the scope of this PR.

Yes, expected. Thats what we have the "lockbreaker group" for and by default a timeout at midnight or so ...

[DeepDiver1975]

It is possible to block closing a tab and notify the user that the text editor needs to be closed/saved.
https://stackoverflow.com/questions/2229942/how-to-block-users-from-closing-a-window-in-javascript

[phil-davis]

It is possible to block closing a tab and notify the user that the text editor needs to be closed/saved.

It looks reasonably possible to implement. @mrow4a are you going to add that?
What is the requirement here - to cover how many workflow scenarios?

[mrow4a]

@DeepDiver1975 added logic to block closing windows (requesting confirmation) when editor is not closed. This does not much prevent the issue, but user maybe needs to think twice, so definitely better than nothing.

@phil-davis so:

• now one acceptance tests fail because there was a case of "not closing the editor". Now this behaviour throws an alert in the browser, and thus fails the test
• as you correctly noticed, checking the locking for this app you would need to have two users logged in and editing at the same time. However, this is sufficiently covered with unit tests in my opinion.
• what can be tested is that there is "manual locking done", and then one tries to edit. This would cover handling existing lock scenario. in my opinion optional too, as it is sufficiently covered with unit tests. Your call.
• what can be tested is that editing works in sharing scenario, thus lock is correctly released. I think this one makes sense to test (if is not yet done).

[phil-davis]

I pushed an adjustment to the failing test so that it waits and checks the editor autosave, rather than doing a reload of the page.
That should pass.

[phil-davis]

LGTM

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
21 Code Smells

76.9% Coverage
0.0% Duplication

[phil-davis]

@DeepDiver1975 please review. IMO this is good to merge.

[pako81]

Can we have this merged if considered to be ok?

[DeepDiver1975]

looks good -> merge

[jnweiger]

Confirmed fixed in 2.5.0-rc.1 -> Details see #384 (comment)

Confirmed fixed with core bundle 10.12.0-rc.3