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

Implement support for deny ACLs #1798

Closed
wants to merge 56 commits into from
Closed

Conversation

glpatcern
Copy link
Member

@glpatcern glpatcern commented Jun 16, 2021

This is still work in progress, but we thought to expose it earlier to gather feedback. Fixes #1792.

The important aspect especially for ownCloud is that now an empty permission set (equivalent to '' in WebDAV terms, and 0 for the integer representation) is considered valid. So the introduced change does not break the existing APIs but does represent a breaking change from a semantic point of view.

EDIT: to avoid breaking any existing assumption, the denial permission gets for now a different bit, PermissionNone. PermissionInvalid is still defined as 0.

@glpatcern glpatcern added the feature New feature label Jun 16, 2021
@glpatcern glpatcern requested a review from labkode as a code owner June 16, 2021 12:45
@update-docs
Copy link

update-docs bot commented Jun 16, 2021

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.

@glpatcern glpatcern marked this pull request as draft June 16, 2021 12:45
pkg/eosclient/eosclient.go Outdated Show resolved Hide resolved
@glpatcern glpatcern force-pushed the denyacl branch 2 times, most recently from cd8764b to 7842ce6 Compare June 23, 2021 08:04
@lgtm-com
Copy link

lgtm-com bot commented Jun 23, 2021

This pull request fixes 20 alerts when merging 532ae59 into b54b159 - view on LGTM.com

fixed alerts:

  • 20 for Uncontrolled data used in path expression

cmd/reva/common.go Outdated Show resolved Hide resolved
@labkode
Copy link
Member

labkode commented Jul 1, 2021

@glpatcern @gmgigi96 I'm looking for a better alternative for GetOwners.
This new method sets a new dependency with the gateway service that was not there before.
With this change the storage providers needs to know where the gateway service is to query group information.
This means to also add this configuration change to our deployment in every storage provider.
We can do better.

While I'm looking how to add this logic into gateway layer reusing the permission set from the FileInfo and pass it down from the gateway to the storage provider you can replicate this PR against https://github.com/cernbox/reva@qa so we can merge it there and deploy it in our QA infra.

EDIT: just did cernbox#3
I'll merge it, but please do a quick review (nothing changes compared to this one)

@@ -234,6 +236,10 @@ func (fs *Decomposedfs) GetPathByID(ctx context.Context, id *provider.ResourceId
return fs.lu.Path(ctx, node)
}

func (fs *Decomposedfs) GetOwners(ctx context.Context, ref *provider.Reference) (*grouppb.Group, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported method Decomposedfs.GetOwners should have comment or be unexported

@@ -234,6 +236,10 @@ func (fs *Decomposedfs) GetPathByID(ctx context.Context, id *provider.ResourceId
return fs.lu.Path(ctx, node)
}

func (fs *Decomposedfs) GetOwners(ctx context.Context, ref *provider.Reference) (*grouppb.Group, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported method Decomposedfs.GetOwners should have comment or be unexported

@labkode
Copy link
Member

labkode commented Jul 19, 2021

Superseded by #1856

@labkode labkode closed this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support full denial as permission
10 participants