Sharing a file with just create and/or delete permission should fail

[phil-davis]

Steps to reproduce

Use the sharing API. Attempt to share a single file with another user, but request just create (4) and/or delete (8) permissions.

Expected behaviour

create and delete permissions are not valid/relevant for single file shares, so they get masked out. The resulting effective requested share permission is 0 (no permissions).

The share should not be created. A "40*" response should be returned.

Actual behaviour

When create is requested, the share is created with 0 permissions. It is a useless share because the receiver cannot see the file in any way.

When delete is requested, the share is created with read 1 permissions. The receiver can read the file - that is weird, I wonder what code causes that to happen.

See PR #35921 for the example test scenarios that demonstrate this.

Server configuration

Current core on GitHub.

[karakayasemi]

When delete is requested, the share is created with read 1 permissions. The receiver can read the file - that is weird, I wonder what code causes that to happen.

This condition causes described behaviour: https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/Controller/Share20OcsController.php#L369
There is an inconsistency between behaviors. We need to decide what should we return when a request has only create or delete permission.

We added acceptance tests for returning 40x response in these cases: https://github.com/owncloud/core/pull/35921/files#diff-129a7916711570e3ddb2eb9f655ce577R633. However, as far as I understand, current implementation's intention is adding read permission as the default unless it is an anonymous upload only link share. But, there is a bug in this aimed behavior also. When a user or group share has only create permission ShareController can not add default read permission because of this line: https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/Controller/Share20OcsController.php#L369.

Should I correct if statement to return 20x response in these described scenarios? Or should I remove it completely to return 40x response?
@jvillafanez, @micbar , @phil-davis , @individual-it any opinion?

[phil-davis]

So we need to know if "adding read permission as the default" is a "proper/intended" requirement.

If so, then a share request that asks for permissions 0 should succeed and return with permissions 1. Behaviour like that will mean that the other "odd" things work - when sharing a single file and asking for just create and/or delete permissions, those will get masked off, resulting in effective permissions of 0, and then the "default read" will be applied, and the share is created with permissions 1.

But, IMO, why not just implement what the API caller asks for, masking off create+delete bits for single file shares. And then if the permissions end up being 0, return a 4xx error response and do not create the share. (all the permission bit combinations seem to have "reasonable" behaviour that maybe someone might want/use - e.g. setting create+delete on a shared folder, the user can create new files in the folder, the user cannot read any files, but if they remember the name of the file that they created then they can delete it - setting delete only, if they somehow know by other means the name of a file in the folder then they can delete it)

[jvillafanez]

My personal opinion is, at least for a first iteration, "keep things as simple as possible".
For a first iteration, if you're asked to create a share with just create permission, you just create it. It's the client's responsability to provide the permissions it wants.
Then, for a second iteration, you can include permission validation on top. You can include different validators depending on the share to be a file or folder.

Personally, I'm against to automatically change the permissions. If the client requests wrong permissions, fail the request. I find terribly wrong to ask for one thing and get another.

[phil-davis]

At the moment, when a client asks for "all" (31) permissions on a folder share, they get 31. When they ask for all (31) permissions on a single file share, they get 19 (31 - 8 (delete) - 4 (create = 19)

If we start refusing a permissions 31 request for a single file share, then I expect that existing clients will/might break. I do not expect that existing clients will have the knowledge built into them that "all permissions" for a single file share is only 19.

So I think we have to keep the server behaviour of masking out create/delete bits on share create/update requests for single file shares.

Apart from that, I think we can:

• set whatever permissions the client asks for. (e.g. if they do not ask for read, then do not give it)
• if the requested permissions end up being 0 then reply with a 4xx error
• if the requested permissions end up being greater than 31 then reply with a 4xx error

[jvillafanez]

I know we can't change things so easily, I just wanted to point out how things should have been done in the first place, and how a wrong architecture and an overly complex logic has led to the current mess.

Apart from that, I think we can:
set whatever permissions the client asks for. (e.g. if they do not ask for read, then do not give it)
if the requested permissions end up being 0 then reply with a 4xx error
if the requested permissions end up being greater than 31 then reply with a 4xx error

As long as there are no big changes in the behaviour, I think this is the way to go. We'll have to test with all the clients to ensure there are no crashes.

If we start refusing a permissions 31 request for a single file share, then I expect that existing clients will/might break. I do not expect that existing clients will have the knowledge built into them that "all permissions" for a single file share is only 19.

I'd set and return 31, and check the behaviour of all clients. It's the client's responsability to know what to do with those permissions in that file. I mean, the client can ignore whatever permission doesn't make sense. As long as the clients don't break, I'd move this way.

[phil-davis]

I'd set and return 31, and check the behaviour of all clients. It's the client's responsability to know what to do with those permissions in that file. I mean, the client can ignore whatever permission doesn't make sense. As long as the clients don't break, I'd move this way.

To me, that is the better thing to do. And then later in the backend, if we see create or delete permission on a single file share, then ignore it.

For create there should be nothing special to do - a create file request for a single file share should already be failing with "file already exists" anyway.

For delete we have the special case that (for whatever reason in the past) somebody has decided that you cannot share a single file and give delete access to the share/file. I guess that is the same principle that happens when you share a folder and give delete access - the receiver of the share can delete stuff in the folder, but they cannot delete the folder itself. So we just have to be careful to make sure that a single file share cannot be "accidentally" deleted by the receiver.

[karakayasemi]

Modifying the request doesn't make any sense to me either. However, recently a small similar change (do not set expiration date, if the request didn't send expiration date and if it is not enforced) in sharing API has introduced a regression in clients. The same thing can happen again, clients may think that if reading permission is already granted as default, there is no need to send it. :)

So if nobody objects , I am planning to keep changes as small as possible and implement exact same behavior with @phil-davis's first suggestion:

• keep the server behaviour of masking out create/delete bits on share create/update requests for single file shares.
• set whatever permissions the client asks for. (e.g. if they do not ask for read, then do not give it)
• if the requested permissions end up being 0 then reply with a 4xx error
• if the requested permissions end up being greater than 31 then reply with a 4xx error