Enhancement: calculate and expose actual file permission set #1368

butonic · 2020-12-10T13:50:18Z

Instead of hardcoding the permissions set for every file and folder to ListContainer:true, CreateContainer:true and always reporting the hardcoded string WCKDNVR for the WebDAV permissions we now aggregate the actual cs3 resource permissions in the storage drivers and correctly map them to ocs permissions and webdav permissions using a common role struct that encapsulates the mapping logic.

refactor the create*Share methods to use the same stat
use a common permissions check to prevent users from sharing with permissions they don't have
map between share role, cs3 resource permissionset, ocs permissions
- the mapping needs to be simplified by mapping everything to roles
- we can bring back legacy ocs permissions by mapping them to all legacy role combinations and finding corresponding mappings in cs3 permission sets.
refactor error handling
- ~~use ocdav.HandleErrorStatus in more places~~ left for another PR I already touched too many things.
fix storages and calculate permissions by reading the grants in the path instead of hardcoding ListContainer:true, CreateContainer:true
- ocis
- ~~eos~~ I hardcoded returning all permissions, similar to what the ocs api was doing prior to this PR.
- ~~local~~ I hardcoded returning all permissions, similar to what the ocs api was doing prior to this PR.
- s3 I hardcoded returning all permissions, similar to what the ocs api was doing prior to this PR.
- ~~owncloud~~ I hardcoded returning all permissions, similar to what the ocs api was doing prior to this PR.

should fix

[QA] Resharing is always allowed even when we do not add that permission [QA] Resharing is always allowed even when we do not add that permission owncloud/ocis#552
[QA] Re-Sharer can increase permissions [QA] Re-Sharer can increase permissions owncloud/ocis#893
cannot get ocs:share-permissions via WebDAV https://github.com/owncloud/ocis-reva/issues/47
[EOS] Cannot access public links https://github.com/owncloud/ocis-reva/issues/315
[EOS] share not created with default permissions https://github.com/owncloud/ocis-reva/issues/316

upload-only public link does not refer to files-drop page, nor are the permissions enforced Upload-only public link does not refer to files-drop page, nor are the permissions enforced owncloud/ocis#723
path and other information are not shown if a share does not have "read" permission path and other information are not shown if a share does not have "read" permission owncloud/ocis#762
various sharing settings cannot be set https://github.com/owncloud/ocis-reva/issues/41
Some failing tests with Webdav custom properties https://github.com/owncloud/ocis-reva/issues/217
Different propfind response compared with core https://github.com/owncloud/ocis-reva/issues/236
Sharing seems to work but does not work https://github.com/owncloud/ocis-reva/issues/243
Accessing non-existing public link should return 404, not 500 https://github.com/owncloud/ocis-reva/issues/290
Public link enforce permissions https://github.com/owncloud/ocis-reva/issues/292
[OCIS-Storage] etags don't change for a share receiver [OCIS-Storage] etags don't change for a share receiver owncloud/product#243
[OCIS-storage] resharing doesn't works [OCIS] [OCIS-storage] resharing doesn't work owncloud/product#265
[OCIS] share permissions not enforced [OCIS] share permissions not enforced owncloud/product#270
[OC Storage] etags are not updated on file operations [OCIS][OC Storage] etags are not updated on file operations owncloud/product#279

@labkode eos, localfs, and the other storages need to return a proper PermissionSet for the logged in user so the ocs api can actually return the right ocs permissions. For the ocis I implemented a ReadUserPermissions() that aggregates the permissions on all segments of the path. ReadUserPermissions() is called in GetMD() and ListContainer(). It should be possible to aggregate the permission set for the other storages in a similar manner. Can eos return the acl or permissions for the current user in the fileinfo or is that another call. ... well it might not be necessary if eos really cannot store acls on files directly (only via fuse richacls).

Another option might be to push setting the permission set to the gateway. it could use the storage registry to fill in the permissions without having to stat every path segment, because it can look at the mount points of shares.

Anyway, this PR currently tries to check if the current user is the owner and returns all permissions if that is the case. This works on eos, ocis, local and owncloud AFAICT. not on s3, yet. For shared folders the ocis driver aggregates the permissions. I did not do thet for the other storages yet, but would start implementing it in the same way, unless you have a better idea how to do it more efficiently. For now I want to make it work. Profiling to make it fast comes later.

update-docs · 2020-12-10T13:50:21Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

phil-davis · 2020-12-10T13:56:49Z

Note: drone CI is not running for some reason today. So we are not getting results to know if the behaviour is improved or not.

labkode · 2020-12-10T14:06:17Z

@phil-davis https://discourse.drone.io/t/new-builds-not-appearing-on-drone/8479

lgtm-com · 2020-12-10T14:07:14Z

This pull request introduces 1 alert when merging 761ca60 into e791b55 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

labkode · 2020-12-16T12:37:37Z

@butonic looks good to me. We'll take care of the EOS implementation. There is a conflict to be resolved, maybe just rebase on top of master.

butonic · 2020-12-16T15:50:45Z

@labkode looking at public links I see that we need to add permission checks and permission set filters to the publicstorageprovider.go from @refs because in the case of public shares ... the storage providers currently cannot determine the permissions ... because only the publicshare manager persists them.

We could implement a publicshare manager that persists shares in the storage ... which would be trivial for ocis. I asume you could persist additional metadata like expiry etc in eos as well ... the question is where should the code live that limits the cs3 permissions set to what the currently logged in user is allowed to do?

We should let the underlying storage system handle as many permission checks as possible, which is would allow a tight eos integration. On the other hand permissions for public links need to be handled in the cs3 publicstorageprovider, based on the share permissions... For the ocis driver we handle permission checks in the driver because we don't integrate with the underlying posix filesystem on purpose.

@labkode do you see my problem with the sharing responsibilities?

butonic · 2020-12-16T16:05:50Z

anyway ... this PR can go in as is

C0rby

Awesome! 👍

refs · 2021-01-07T14:46:01Z

bug while testing:

log in as admin:admin
create a folder and share it with Marie
as marie: upload a file to the folder
as admin: try to delete the file that Marie uploaded to the shared folder

expected: admin should be able to delete the file as she is the owner of the shared folder
got: admin cannot delete file uploaded to shared folder

butonic · 2021-01-07T14:58:32Z

moving a file into a shared folder fails with 400

2021-01-07T14:56:58Z DBG gateway: split: parts[1]:Invoice for MBA39A4.pdf != shareFolder:Shares pkg=rgrpc service=storage traceid=5108148c26c8de8e51abc9ab1f42f74c
2021-01-07T14:56:58Z DBG gateway: split: parts[1]:Invoice for MBA39A4.pdf != shareFolder:Shares pkg=rgrpc service=storage traceid=5108148c26c8de8e51abc9ab1f42f74c
2021-01-07T14:56:58Z DBG unary code=OK end="07/Jan/2021:14:56:58 +0000" from=tcp://127.0.0.1:56288 pkg=rgrpc service=storage start="07/Jan/2021:14:56:58 +0000" time_ns=268600 traceid=5108148c26c8de8e51abc9ab1f42f74c uri=/cs3.gateway.v1beta1.GatewayAPI/Move user-agent=grpc-go/1.26.0
2021-01-07T14:56:58Z DBG bad request dst="/home/Shares/Neuer Ordner/Invoice for MBA39A4.pdf" pkg=rhttp service=storage src="/home/Invoice for MBA39A4.pdf" status={"code":4,"message":"gateway: move:error: bad request: gateway: move called on unknown path: /home/Invoice for MBA39A4.pdf","trace":"5108148c26c8de8e51abc9ab1f42f74c"} traceid=5108148c26c8de8e51abc9ab1f42f74c
2021-01-07T14:56:58Z WRN http end="07/Jan/2021:14:56:58 +0000" host=127.0.0.1 method=MOVE pkg=rhttp proto=HTTP/1.1 service=storage size=0 start="07/Jan/2021:14:56:58 +0000" status=400 time_ns=39223900 traceid=5108148c26c8de8e51abc9ab1f42f74c uri=/remote.php/webdav/Invoice%20for%20MBA39A4.pdf url=/remote.php/webdav/Invoice%20for%20MBA39A4.pdf

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

lgtm-com · 2021-01-07T15:56:10Z

This pull request introduces 1 alert when merging 074d616 into 23e52dc - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic · 2021-01-07T16:04:27Z

now at 200+ fixed features:

61 | .../expected-failures-on-EOS-storage.txt           \|   8 ---
62 | .../expected-failures-on-OCIS-storage.txt          \| 176 ---------
63 | .../expected-failures-on-OWNCLOUD-storage.txt      \|  47 +--

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

labkode · 2021-01-08T08:24:42Z

@butonic https://lgtm.com/projects/g/cs3org/reva/rev/pr-0625c1d67fb1de4e2463c285552d4ead77d61d9e

phil-davis

LGTM, the permissions are much better than before. IMO after merging this, people can work on more of the "ToDo"

phil-davis · 2021-01-08T09:11:27Z

@butonic https://lgtm.com/projects/g/cs3org/reva/rev/pr-0625c1d67fb1de4e2463c285552d4ead77d61d9e

A later analysis at https://lgtm.com/projects/g/cs3org/reva/rev/pr-6affa6881c70272cacb9a1b528350a524a4c180d no longer complains about that.

butonic · 2021-01-08T09:35:12Z

@butonic https://lgtm.com/projects/g/cs3org/reva/rev/pr-0625c1d67fb1de4e2463c285552d4ead77d61d9e

fixed in c69f470

ishank011

I think we can merge this. I'll add the ACL traversal for eos. Since we don't really have permissions support for localfs, we can just let it be.

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

ishank011 · 2021-01-08T10:42:00Z

internal/http/services/owncloud/ocdav/propfind.go

+	// files never have the create container permission
+	if md.Type == provider.ResourceType_RESOURCE_TYPE_FILE && md.PermissionSet != nil {
+		// we need to copy the permission set in case it was reused when passing it in while listing a collection
+		md.PermissionSet = copyPermissionSet(md.PermissionSet)


Since we're assigning the permissions to the same variable, we do lose the original value. Why are we making a separate copy then?

PermissionSet is a struct which is passed by reference. There are code paths that will change the PermissionSet for files, because they never have delete or create permission IIRC. When listing a folder this would change the default PermissionSet and folders that are rendered after a file would retain the changed default. 💥

Yes but this particular function is called only from one place and we're forgetting the original reference (since we're storing the returned struct there) so essentially there's no way to get that back.

ok, you are right ... I tried cleaning the permissions and roles stuff up. I think I was confusing this with the storage drivers ...

all tests green!

labkode · 2021-01-08T12:15:21Z

About the EOS ACLs, EOS is different to local/ocis filesystems, EOS does not do ACL traversal, ACLs are examined on the node (not from parents). It is possible in EOS to not have any permissions on /a/b/ but have on /a/b/c.

This change in respect to UNIX is one of the design choices to reach high scalability.

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic requested a review from labkode as a code owner December 10, 2020 13:50

exalate-issue-sync bot mentioned this pull request Dec 10, 2020

[QA] Re-Sharer can increase permissions owncloud/ocis#893

Closed

butonic force-pushed the share-permissions-and-resource-permissions branch from 87713dd to 9637661 Compare December 10, 2020 20:50

butonic changed the title ~~[WIP] prevent sharing with increased permissions~~ [WIP] Enhancement: calculate and expose actual file permission set Dec 11, 2020

butonic changed the base branch from master to unsafe December 14, 2020 15:15

butonic force-pushed the share-permissions-and-resource-permissions branch from 15f25f8 to 8238918 Compare December 14, 2020 15:22

butonic changed the title ~~[WIP] Enhancement: calculate and expose actual file permission set~~ Enhancement: calculate and expose actual file permission set Dec 14, 2020

butonic force-pushed the share-permissions-and-resource-permissions branch from 8238918 to b976591 Compare December 14, 2020 15:37

butonic mentioned this pull request Dec 15, 2020

[tests-only|do not merge] reva PR 1368 owncloud/ocis#1097

Closed

butonic changed the title ~~Enhancement: calculate and expose actual file permission set~~ [WIP] Enhancement: calculate and expose actual file permission set Dec 15, 2020

butonic marked this pull request as draft December 15, 2020 12:53

butonic changed the base branch from unsafe to master December 16, 2020 09:08

butonic force-pushed the share-permissions-and-resource-permissions branch from 79a0ae9 to 3855e0e Compare December 16, 2020 11:09

butonic marked this pull request as ready for review December 16, 2020 11:32

butonic changed the title ~~[WIP] Enhancement: calculate and expose actual file permission set~~ Enhancement: calculate and expose actual file permission set Dec 16, 2020

labkode previously approved these changes Dec 16, 2020

View reviewed changes

butonic dismissed labkode’s stale review via bbceb1c December 16, 2020 14:11

butonic force-pushed the share-permissions-and-resource-permissions branch from 3855e0e to bbceb1c Compare December 16, 2020 14:11

C0rby previously approved these changes Dec 16, 2020

View reviewed changes

butonic mentioned this pull request Dec 17, 2020

Clarify responsibilities of share providers and storage providers #1377

Open

butonic dismissed C0rby’s stale review via fb3c5b3 December 17, 2020 11:08

butonic force-pushed the share-permissions-and-resource-permissions branch from fb3c5b3 to 6063f11 Compare December 17, 2020 11:28

butonic marked this pull request as draft December 17, 2020 15:34

butonic added 3 commits January 7, 2021 15:16

parent owner becomes owner of uploaded files

e36dd46

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

parent owner becomes owner of new folders

0f14362

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

fix home creation

074d616

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic force-pushed the share-permissions-and-resource-permissions branch from 763de06 to 074d616 Compare January 7, 2021 15:42

update expected failures and changelog

79f82c6

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

fix lint

c69f470

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic force-pushed the share-permissions-and-resource-permissions branch from d367114 to c69f470 Compare January 7, 2021 20:10

phil-davis previously approved these changes Jan 8, 2021

View reviewed changes

ishank011 reviewed Jan 8, 2021

View reviewed changes

phil-davis mentioned this pull request Jan 8, 2021

[tests-only] Bump core commit id for API tests #1392

Merged

cleanup permissions rendering for propfind

c3cbdd1

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic dismissed phil-davis’s stale review via c3cbdd1 January 8, 2021 14:50

ishank011 approved these changes Jan 8, 2021

View reviewed changes

ishank011 merged commit 36d7490 into cs3org:master Jan 8, 2021

butonic deleted the share-permissions-and-resource-permissions branch January 8, 2021 15:15

butonic mentioned this pull request Jan 11, 2021

Update reva to v1.4.1-0.20210111080247 owncloud/ocis#1194

Merged

C0rby mentioned this pull request Apr 9, 2021

Permissions in webdav propfinds are hardcoded owncloud/ocis#42

Closed

phil-davis mentioned this pull request Aug 17, 2021

[OCIS] [OCIS-storage] resharing doesn't work owncloud/product#265

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: calculate and expose actual file permission set #1368

Enhancement: calculate and expose actual file permission set #1368

butonic commented Dec 10, 2020 •

edited

Loading

update-docs bot commented Dec 10, 2020

phil-davis commented Dec 10, 2020

labkode commented Dec 10, 2020

lgtm-com bot commented Dec 10, 2020

labkode commented Dec 16, 2020

butonic commented Dec 16, 2020

butonic commented Dec 16, 2020 •

edited

Loading

C0rby left a comment

refs commented Jan 7, 2021

butonic commented Jan 7, 2021

lgtm-com bot commented Jan 7, 2021

butonic commented Jan 7, 2021

labkode commented Jan 8, 2021

phil-davis left a comment

phil-davis commented Jan 8, 2021

butonic commented Jan 8, 2021

ishank011 left a comment

ishank011 Jan 8, 2021

butonic Jan 8, 2021 •

edited

Loading

ishank011 Jan 8, 2021

butonic Jan 8, 2021

butonic Jan 8, 2021

ishank011 Jan 8, 2021

labkode commented Jan 8, 2021 •

edited

Loading

Enhancement: calculate and expose actual file permission set #1368

Enhancement: calculate and expose actual file permission set #1368

Conversation

butonic commented Dec 10, 2020 • edited Loading

update-docs bot commented Dec 10, 2020

phil-davis commented Dec 10, 2020

labkode commented Dec 10, 2020

lgtm-com bot commented Dec 10, 2020

labkode commented Dec 16, 2020

butonic commented Dec 16, 2020

butonic commented Dec 16, 2020 • edited Loading

C0rby left a comment

Choose a reason for hiding this comment

refs commented Jan 7, 2021

butonic commented Jan 7, 2021

lgtm-com bot commented Jan 7, 2021

butonic commented Jan 7, 2021

labkode commented Jan 8, 2021

phil-davis left a comment

Choose a reason for hiding this comment

phil-davis commented Jan 8, 2021

butonic commented Jan 8, 2021

ishank011 left a comment

Choose a reason for hiding this comment

ishank011 Jan 8, 2021

Choose a reason for hiding this comment

butonic Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

ishank011 Jan 8, 2021

Choose a reason for hiding this comment

butonic Jan 8, 2021

Choose a reason for hiding this comment

butonic Jan 8, 2021

Choose a reason for hiding this comment

ishank011 Jan 8, 2021

Choose a reason for hiding this comment

labkode commented Jan 8, 2021 • edited Loading

butonic commented Dec 10, 2020 •

edited

Loading

butonic commented Dec 16, 2020 •

edited

Loading

butonic Jan 8, 2021 •

edited

Loading

labkode commented Jan 8, 2021 •

edited

Loading