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

[All] Share-jail folder to contain all the accepted shares #7

Closed
labkode opened this issue Feb 11, 2020 · 23 comments
Closed

[All] Share-jail folder to contain all the accepted shares #7

labkode opened this issue Feb 11, 2020 · 23 comments

Comments

@labkode
Copy link

labkode commented Feb 11, 2020

New special folder “MYSHARES”

  • Lives in the home directory of the user: “/home/Shared with me”
  • Gives access to all the shares accepted by the user

CERNBox Share folder semantics

What needs to happen

MYSHARES folder should be marked as read/only, perhaps with some additional visual clue to indicate its special status.

  • Change content of the MYSHARES (ro) folder

    PUTing files and directories directly at the top level of this folder should be rejected.
    MOVE into the MYSHARES folder should be rejected.

    Q: what should happen with these local entries in MYSHARES?

    DELETE a share should be fine and should be interpreted by the server as "unmounting" a share. This should be notified to the user with a Sync Client notification. Nothing inside the share should be removed.

    Q: what is the meaning of “unmounting share”? will it continue to be visible on the web UI and synchronized with other clients?

  • Rename a SHARE folder

    It should be allowed to MOVE a share folder within MYSHARES. In practice, doing this should mean that the share , for that user, changed name.

    Q: should the rename affect all the clients rather than just one? The rename should only affect the user mounting the share.

  • Move share to some other place outside the ro folder

    MOVEing a share outside of MYSHARES should create a copy. It shouldn't prevent the sync client from re-downloading the same SHARE back. This would make it clear that the original share is still there, even though it created a copy in some other place.

    Q: should this create a re-download of the share or unshare?

  • Rename My Shares folder

    MOVEing the MYSHARES folder should make a copy of all the files. But the original MYSHARES folder, and its SHAREs inside, should be re-downloaded again, signaling to the user that a copy was created but nothing changed in the original. Maybe a notification could be given, explaining that a copy was created but nothing else changed. At any point the MYSHARES folder should stay so the user can continue working with the shares.

Race conditions, heads up

  • If there are files created inside this MYSHARES folder, we can't prevent that from happening? What should the system do in case a user created by hand some entries that will clash with the shares? Simple test, remove a SHARE folder and replace it with another one completely different.

  • Renaming/moving MYSHARES around: another behaviour to consider, sync client keeps track of it and simply uses this. The rename/move is only client side, operation is not propagated to server side.

@michaelstingl
Copy link

@labkode Do you already have a server setup with those permissions?

@labkode
Copy link
Author

labkode commented Mar 19, 2020

@michaelstingl no, we need your input from the sync client team to understand what PROPFIND permissions are expected from the client to mimic such behaviour. Maybe @TheOneRing and @ogoffart can help here.

@hodyroff
Copy link

https://stackoverflow.com/questions/31708957/how-to-make-file-read-only-when-exposed-through-webdav
Evert has answered this at some point for "normal" WebDAV. So without implementing WebDAV ACLs - which have IMHO other shortcomings again there is only the possibility for WebDAV locks. ownCloud Server has those implemented (but only activated for files, not folders) and client deals with them in some way which is likely not yet perfect.

In regards of "needs" to happen. We disagree with quite some of the needs. Please also check how ownCloud Server works today when a /Shared folder is enforced. Which was the default till 9.0 or so and sharing 2.0 implementation. For migrations the oCIS storage shouldn't behave too different and different design choices need great reasons.

What does the CS3 API deliver us in regards of read-only markers which we could then at least set client-side?

@butonic
Copy link
Member

butonic commented Nov 6, 2020

The desktop client tries to detect a MOVE on the local machine. When we detect a deleted file an a created file we even try to correlate them by checksum inode and magic to reconstruct the MOVE. Yet, in corner cases the client might send a PUT / upload and DELETE as a fallback.

How the server treats the DELETE depends on the folder semantics. If it it is the MYSHARES folder I think we are unsharing the share. Which would prevent redownloading of shared files ...

@labkode we could prevent unsharing by deleting the entry in the MYSHARES folder. In posix if a folder is readonly, you cannot add, rename or delete children in it. Which translates to not being able to send a DELETE to any MYSHARES/child. might be an option ... then unsharing would need a dedicated endpoint ... og in the sharing api ... like rejecting the share again ...

@butonic
Copy link
Member

butonic commented Nov 6, 2020

Furthermore, looking at the above list it reads as if you are triggering the actions in the web ui ... with the question how the clients will then behave. If so we should seperate the three scenarios, because mobile might be different again ... well come to think of it they should all be the same, however treating a DELETE as an unshare is a special case, because the desktop client might just detect that a file was locally deleted. should it then redownload or unshare?

in theory it should be redownloaded.

also renaming shares in expected, but the shared folder is actually read only so in theory shares cannot be renamed inside the MYSHARES folder.

we need to clarify the semantics. because I think we cannot say the MYSHARES folder is readonly. I think we can only say the MYSHARES folder cannot be deleted.

@labkode can we agree to start with that proposition: 'The MYSHARES folder cannot be deleted.'?

@butonic
Copy link
Member

butonic commented Nov 6, 2020

second restriction: you cannot PUT files or folders there

@butonic
Copy link
Member

butonic commented Nov 6, 2020

in theory we could use webdav references to create shares, see https://tools.ietf.org/html/rfc4437 which nobody implments anyway ... so we should just stick to the ocs sharing api

@michaelstingl
Copy link

Clients can deal with read-only folders.
(imagine shared folder received without the other permissions)

  • client does not change permissions in local filesystem to prevent forbidden operations
  • deleted local files or folders immediately get re-downloaded
  • local added files or folders won’t get uploaded, it will get red overlay icon and listed in not-synced tab

@labkode
Copy link
Author

labkode commented Nov 11, 2020

Hi @butonic, calling read-only the "MyShares" folder is not what we expect from POSIX read-only, I don't have a better adjective to call it, maybe "special" shared folder.

The restrictions are the ones explained in the issue description.
Deleting the MyShares folder can only be prevented while the sync client is running, but users can stop it and delete the local folder. We can expect two things: 1) a deletion of this folder triggers and unshare of all the mounted shares and recreates an empty MyShares folder or 2) the folder gets recreated with the original shares.

@butonic
Copy link
Member

butonic commented Nov 12, 2020

@labkode Clear communication is important and I think we should find a good name for this kind of folder.

  • undeletable: but then people will just call it readonly because they think that is the same thing
  • frozen: you cant change the name
  • zombie: It will come back when deleted
  • static: cannot be moved
  • jail root: hmm
  • share jail: I like this one. The /MyShares folder is a share jail. Which should clarify that it has the properties you mentioned: cannot be deleted via the api. New files and folders cannot be created there (but single file shares can be written to).

I would however not unshare all shares when the shared folder is deleted locally. That seems to be too drastic and it is hard to recreate all shares. So option 2 is what we should aim for.

@michaelstingl the /MyShares share jail is interesting because it's children might be writeable. @TheOneRing The client should be able to deal with that, right?

@TheOneRing
Copy link

Yes I have to validate the behavior but I would expect that the propagation of changes should end after the MyShares delete failed, and not continue for files below that folder.

@michaelstingl
Copy link

@michaelstingl the /MyShares share jail is interesting because it's children might be writeable. @TheOneRing The client should be able to deal with that, right?

I asked for test setup already in March (#7 (comment)) to validate such assumptions. @butonic invite for meeting/workshop for the next steps?

@labkode
Copy link
Author

labkode commented Nov 18, 2020

@butonic I like the share jail, @TheOneRing @michaelstingl is important the desktop client handles in a smooth way these actions, for example deleting the share folder and re-creating it should trigger any red banner on the client, perhaps a notification to the user of what could happen (like when they deleted the entire folder).

@labkode
Copy link
Author

labkode commented Nov 18, 2020

@michaelstingl this is part of OCIS with local filesystem and also with EOS.

@labkode
Copy link
Author

labkode commented Nov 20, 2020

The testing of the Shares jail can be tested against:
https://ocis.owncloud.works/

From the web UI you can use different users to share among them and test the behaviour.

@TheOneRing @michaelstingl

@labkode labkode changed the title [All] Read-only folder to contain all the accepted shares [All] Share-jail folder to contain all the accepted shares Nov 20, 2020
@butonic butonic self-assigned this Nov 20, 2020
@TheOneRing
Copy link

@jnweiger could you test those scenarios 😃

@dragotin
Copy link

Even if you could enforce the MYSHARES folder on the server, you can not on the client. On the client it will aways be possible to remove it, move stuff in and out and change it's permisison because it is local filesystem.

Putting the discussed restrictions and rules has big potential to confuse users IMHO.

Details:

Putting files into the MYSHARES folder

Could be ignored and end up in the "not synchronised" tab. Possible... But as a user I would wonder why that happens, and will be potential data loss. As @moscicki always says, all data that is not on the server, is potentially lost...

Deleting and Renaming a shared folder in MYSHARES

Would work as described and expected.

Rename or delete MYSHARES

The proposed solution would appear to me as a kind of "zombi behaviour", as it shows up again and again after renaming.

I think the local filesystem is owned by the user, and we should not try to restrict the users possibilities with it, as that will decrease the acceptance of the tool.

I think the best solution would be to have the MYSHARES folder on the server, and if it is renamed locally, it is renamed on the server as well. But that would be very much the default behaviour as of today, and I am sure was already discussed.

Adding this kind of behaviour to the sync client would make the sync code much more complex and add kind of arbitrary exceptions for the MYSHARES.

We could find ways to present shares differently in the sync client GUI, but we can not get around the local filesystem. Also setting the MYSHARES folder to read only does not really solve that, as users will find ways...

@labkode
Copy link
Author

labkode commented Nov 24, 2020

Answers inline:

Even if you could enforce the MYSHARES folder on the server, you can not on the client. On the client it will aways be possible to remove it, move stuff in and out and change it's permisison because it is local filesystem.

Putting the discussed restrictions and rules has big potential to confuse users IMHO.

Details:

Putting files into the MYSHARES folder

Could be ignored and end up in the "not synchronised" tab. Possible... But as a user I would wonder why that happens, and will be potential data loss. As @moscicki always says, all data that is not on the server, is potentially lost...

This behaviour is already present in the client when accessing read-only shares. Users can do whatever they please on the local filesystem while the client is or is not running and the client guarantees a correct state. This behaviour has been in the client for long time and end-users are used to it. This is not a change of behaviour, only reducing it to happen on a particular folder rather to in an unbound number of them.

Screenshot 2020-11-24 at 15 11 40

Deleting and Renaming a shared folder in MYSHARES

Would work as described and expected.

Rename or delete MYSHARES

The proposed solution would appear to me as a kind of "zombi behaviour", as it shows up again and again after renaming.

I think the local filesystem is owned by the user, and we should not try to restrict the users possibilities with it, as that will decrease the acceptance of the tool.

I think the best solution would be to have the MYSHARES folder on the server, and if it is renamed locally, it is renamed on the server as well. But that would be very much the default behaviour as of today, and I am sure was already discussed.

This could be doable and could match the user expectations, however, does the user want this local folder rename to be propagated also to other clients? Enforcing the MYSHARES folder naming ensures the folder is the same in any device.

Allowing arbitrary share folders on the tree poses a significant performance penalty when propagating changes and reason why we had to not mount them in the user home. Having the folder living on depth-1, we limit this performance issue while allowing the users to access all the shares.

Adding this kind of behaviour to the sync client would make the sync code much more complex and add kind of arbitrary exceptions for the MYSHARES.

We could find ways to present shares differently in the sync client GUI, but we can not get around the local filesystem. Also setting the MYSHARES folder to read only does not really solve that, as users will find ways...

In the long run, having native support on the OS like the latest Dropbox that creates a virtual drive only when the application runs is much better strategy to control user behaviour, as the user only have access to the files when the app is active.
This mechanism can expose multiple virtual drives containing all the user shares, while not messing up in the home namespace.

Happy to discuss further and find other alternatives.

@butonic
Copy link
Member

butonic commented Nov 30, 2020

Let's not get sidetracked. I'd love to show that jailing shares into a single folder still requires multiple writes and can be further optimized by caching the etag for all mount points - but we agreed to start with a /Shared folder. Btw, the concept of a shared folder was removed because end users want that folder to be named differently, depending on their language ... but back to the actual topic: what happens when end users touch anything that is tied to the share jail?

What needs to be checked

  • share jail folder should be marked as read/only, perhaps with some additional visual clue to indicate its special status.

    • would prevent the user from some unsupported actuons, eg. moving a share outside the shares folder
    • would prevent the user from legal actions, eg renaming the share name from /Shares/Foo to /Shares/Project Foo
    • user can always chmod the file to make the folder writeable, aka snake oil
    • As a result the client is not changing file permissions by design
    • adding an overlay icon would indicate the special semantics
  • Change content of the share jail folder

    PUTing files and directories directly at the top level of this folder should be rejected.
    MOVE into the MYSHARES folder should be rejected.
    The client cannot prevent MKDIR or CREATE operations in the file system (unless the VFS is used - out of scope for now). They will look like new entries and it will try to send an MKCOL / PUT to the server. OCIS denies these requests with a ... oh 500 error

    • for MKCOL
    • as well as POST (TUS upload)
    • I think @labkode @dragotin and @moscicki already pointed out this is a problem of all read-only shares. But we need to differentiate two cases here:
      • Read-Only shares: even if we were setting ro permissions on ro shares the user can still chmod the folder and add files
      • The share jail: the end user expects his files to be synced, but since the server denies writes and the client will not try uploading them indefinritely the file will be added to the ignored files. But this may cause data loss. Another reason why we moved away from the /Shares folder and allow users to mount them in arbitrary locations.
      • This deserves mentioning an additional concept: let me present you with share collections, eg. a subset of 5 images from a large gallery with 10000 pictures. Using RESOURCE_TYPE_REFERENCE we can create arbitrary collections. These collections may be read only or rw ...
      • As a result we can only handle this properly with a virtual filesystem / a fuse mount.
        • properly in so far as we can only implement what the windows / macos / android / fuse / platform cloud provider apis allow.
      • Currently, the server denies adding files and the client will add them to the ignored files after repeated tries.
      • @labkode @dragotin @pmaier1 We could move unsyncable files to a lost+found folder or the users home? Might be better than adding them to the ignored list.
  • DELETE a share should be fine and should be interpreted by the server as "unmounting" a share. This should be notified to the user with a Sync Client notification. Nothing inside the share should be removed.

    Q: what is the meaning of “unmounting share”? will it continue to be visible on the web UI and synchronized with other clients?

    • depends. In oc 10 the share was completely removed. In ocis we could change from accepted to pending.
    • Currently, in ocis the share reference is deleted, but the share manager still lists the share when checking "shared with me".
      • ocdav should set the status to pending when receiving a DELETE
      • deleting a share from the share manager (in the shared with me list) fails, at least on ocis.owncloud.works.
  • Rename a SHARE folder

    It should be allowed to MOVE a share folder within MYSHARES. In practice, doing this should mean that the share , for that user, changed name.
    Q: should the rename affect all the clients rather than just one? The rename should only affect the user mounting the share.

    • This is the case: When einstein shares /Folder with marie, when marie accepts the share it will be mounted as /Shares/Folder. When she renames /Shares/Folder to /Shares/Project Foo einstein will still see it as /Folder because marie has her own reference thet she can name at will. This will cause renames on all the devices that sync her files.
  • Move share to some other place outside the ro folder

MOVEing a share outside of MYSHARES should create a copy. It shouldn't prevent the sync client from re-downloading the same SHARE back. This would make it clear that the original share is still there, even though it created a copy in some other place.

  • this example is flawed. There are two cases:

    1. moving the folder out of the share folder, but within the synced tree
    2. moving the folder out of the synced tree.
  • in the first scenario there are three more cases:
    1.a. The client detects the move via inotify (or the corresponding mechanism)
    1.b. The client heuristics can reassemble a DELETE and CREATE in the sync tree to a MOVE during a single sync run.
    1.c. The client sends a DELETE (and optionally uploads files at the new location)

    • for 1.a and 1.b a MOVE operation can be denied by the server. The client currently reports an error and copies the file back to the source folder. It will also leave a copy in the destination which will be ignored after too many failed sync attempts. If the origin folder has been deleted, the file is not copied back. The user constantly sees a red icon.
    • for 1.c a DELETE will unshare the folder.

    Q: should this create a re-download of the share or unshare?

    • as mentioned above this will unshare the folder.
  • Rename My Shares folder

    MOVEing the MYSHARES folder should make a copy of all the files. But the original MYSHARES folder, and its SHAREs inside, should be re-downloaded again, signaling to the user that a copy was created but nothing changed in the original. Maybe a notification could be given, explaining that a copy was created but nothing else changed. At any point the MYSHARES folder should stay so the user can continue working with the shares.

    • moving files ... does not copy them locally. again we can only prevent this with a VFS, see above.
    • @dragotin will report on what happens when deleting the /Shares folder and the belew race condition items

Race conditions, heads up

  • If there are files created inside this MYSHARES folder, we can't prevent that from happening? What should the system do in case a user created by hand some entries that will clash with the shares? Simple test, remove a SHARE folder and replace it with another one completely different.

  • Renaming/moving MYSHARES around: another behaviour to consider, sync client keeps track of it and simply uses this. The rename/move is only client side, operation is not propagated to server side.

@dragotin
Copy link

dragotin commented Dec 1, 2020

I think the short conclusion is: Currently, by adding the read only /Shares systematic to the client, we will get some very hard to handle extra situations described above. It will add extra conditions to the client which make it break the local filesystem consistency. Users will have ways to get confused.

With the new architecture in oCIS the limitations of the old share system being slow is going away, so some of the reason for having the shares in the jail is going away.

IMHO we only can implement the jail idea properly if we go for a FUSE layer above the file system completely. That is currently not on the roadmap.

@butonic
Copy link
Member

butonic commented Feb 3, 2021

@dragotin @micbar @labkode so how do we continue? We saw the sharing use case, but we still need to write down what to do. AFAICT the /remote.php/webdav endpoint could be configured to list three types of folders:

  • /home the users private files
  • /shares the incoming shares for the current user
  • /projects or /spaces: any other storage spaces the user has access to.

That way, clients at cern, which already use the deskrop client branding option to jail the initial sync into /home should work, right?

@labkode Any objections? Do we need to migrate anything? IIRC you said the current plan is to go into production with the shares mounted at /home/MyShares.

For the web ui we have owncloud/web#4248 @labkode does that work?

What about android and iOS?

Also ... this approach wont work for any other oc10 deployment unless a branded client with the initial /home sync connection preset. We can certainly update all the clients to handle this properly. IMO we should introduce a new endpoint for listing all storages a user has access to and the clients should try that instead of eg the capabilities, because the capabilities vary between storages.

@dragotin
Copy link

FYI, please see this PR describing an API to query the available spaces for clients: owncloud/ocis#1827

@michaelstingl
Copy link

michaelstingl commented Mar 22, 2023

DONE:

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

No branches or pull requests

6 participants