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

Support for additional arbitrary advanced share permissions #33458

Closed
PVince81 opened this issue Nov 8, 2018 · 11 comments
Closed

Support for additional arbitrary advanced share permissions #33458

PVince81 opened this issue Nov 8, 2018 · 11 comments

Comments

@PVince81
Copy link
Contributor

PVince81 commented Nov 8, 2018

Add the ability to extend the current share permissions model and add more permissions as defined by apps.

Backend approaches

1) Reusing and extending the existing permissions field

The oc_filecache table has a field "permissions" and also oc_share.permissions for sharing permissions.
The value is a bitmask.
We could reuse additional bits.

Pros

  • No changes in data model, the permissions are already exposed everywhere with no extra efforts.

Cons

  • Would require apps to "reserve" certain bits for their own additional permissions.
  • Or needs core to predefine some bits for certain permissions and let apps implement the actual logic.

1b) Reusing and extending the existing permissions field

Like 1) but store the permissions in a new field "oc_share.extraPermissions".

Pros

  • Minor addition to current share model
  • No storage implications

Cons

  • More wiring up required in various layers.

2) Use extra Webdav properties

The table "oc_properties" contains a list of arbitrary Webdav properties that can be set with PROPPATCH and retrieved with PROPFIND. They are attached to files by file id.

An app could define its own properties to represent additional permissions there.

When the time comes to enforce permissions, the app would need to read the permission from there manually.

Could also provide a permissions manager in core that hides this and exposes it via a public PHP API.

Pros

  • Ability to reuse an existing table.

Cons

3) New table for additional permissions

Create a new table "oc_extra_permissions" with fields "fileid", "app_id", "permission_id", "active".
Need to keep this table in sync with "oc_filecache.fileid" for example with foreign keys or a background job to clear orphaned entries.
Provide a permissions manager in core that manages the permissions table and exposes a public PHP API for managing the permissions in there.

Pros

  • cleaner approach, proper data structures

Cons

  • extra table (the other approaches didn't have one)

4) Use collaborative tags

An app could define a set of collaboratve tags which would define the permissions for everyone.

Pros

No core changes needed. Apps read the tags and decide how to apply restrictions.

Cons

Restriction is for all users, not per user.

Frontend approaches

Need a way for apps to be able to inject UI components.
This would not work on mobile so might need a way in backend to set translatable permission names.
An HTTP API needs to be provided for clients to be able to retrieve all possible translated permission names for a given file or folder, to be displayed as a list of checkboxes.

@PVince81 PVince81 self-assigned this Nov 8, 2018
@PVince81 PVince81 added this to the development milestone Nov 8, 2018
@ownclouders
Copy link
Contributor

GitMate.io thinks possibly related issues are #4057 (Permissions need additional refinement), #22739 (Sharing permission issue), #22735 (CalDAV: support public sharing), #4120 (Permission issues in shared folder), and #10306 (Setting Permissions on the share).

@PVince81
Copy link
Contributor Author

I'm researching a bit how to extend the existing oc_share.permissions with additional "reserved" bits.

When trying to set one of the higher bits, we will bump across multiple blocks, like this one: https://github.com/owncloud/core/blob/stable10/lib/private/Share20/Manager.php#L282

It reads the perms from the storage, which is max 31 and doesn't allow setting bits higher than that. This would imply either adding the extra bits also to the storage permissions, including storing them in oc_filecache, or finding all those code locations and manually adding the extra bits to the allowed part.

In general it still feels a bit weird to reserve additional bits this way.

@PVince81
Copy link
Contributor Author

having the additional bits on the storage layer means we need to support this on storage layer somehow, or always fill them with preset values.

maybe storages would then need a new interface/capability for new permissions. If the capability is set, we let the storage have extra methods to retrieve said permissions from the underlying API.
If the capability is not set, then we apply a storage wrapper that automatically sets these bits to true.

In general this approach is still limited by the number of reserved bits we want to have...

@PVince81
Copy link
Contributor Author

With the extra tables approach, it would not affect the current code paths nor affect the storages.
We would need to make sure the sharing code also reads the extra perms from there and applies the same kind of logic, like for example preventing resharers to give more permissions than the original share.

@PVince81
Copy link
Contributor Author

Whatever app is requiring the extra permissions might be working with the Node API and might expect the extra permissions to be part of said API. So either with approach 1) we would have them exposed as part of Node::getPermissions(), or there would be a new call Node::getExtraPermissions() which must somehow know how to access the extra perms, regardless where they are stored.

It sounds like even in this case we'd need to extend the storage API, or at least the SharedStorage to have a getExtraPermissions() field. If we don't want that we'd then need to be aware that we're talking to a shared storage inside the Node API (which we're not supposed to do) and then get the share model to read the extra permissions. That would be some kind of ugly shortcut...

@mrow4a
Copy link
Contributor

mrow4a commented Dec 18, 2018

I made a quick prototype for view-only permission, but it was quite invasive - stable10...view-only-shares. I decided to go into direction of implementing DAV Plugin similar to Firewall for Richdocuments.

I also looked into how to add new permissions easiest from the app level. I think the only approach which is actually possible without creating a code spaghetti is 3). This should define:

  • permission type e.g. link, user/group
  • permission bitmask
  • app_id, file_id
  • core way of registering new link/user/group permission (description, label etc).

@DeepDiver1975 @PVince81 @pmaier1

@PVince81 PVince81 modified the milestones: development, backlog Dec 18, 2018
@mrow4a
Copy link
Contributor

mrow4a commented Dec 21, 2018

I made POC for Approach 3 in the working branch https://github.com/owncloud/core/compare/secure_view_plugin by adding new field in share table with extra share permissions. The first app to use it will be dav app, with SecureView Plugin

Now, each app has to register with such thing during its startup:

class Application extends App {

	public function __construct(array $urlParams= []) {
                ....
		$this->shareManager->registerExtraPermission('dav', 'secure-view', 'enable secure-view');
                .....
	}

This will result in such thing:
screenshot at dec 21 12-10-53

So far it all looks good, will continue work in this direction and PR it for reviews @pmaier1 @PVince81 @DeepDiver1975

@mrow4a
Copy link
Contributor

mrow4a commented Jan 7, 2019

I started implementation on Approach 3 here - #33994 (comment). It looks good, but I need to decide on tech implementation details.

@mrow4a
Copy link
Contributor

mrow4a commented Jan 8, 2019

I see also quite interesting difficulty. What if we do share permissions for apps specifically. But e.g. sharer has no access to the app e.g. richdocuments because of group membership, but needs to set permissions for other user which will use richdocuments? @PVince81

@mrow4a
Copy link
Contributor

mrow4a commented Jan 8, 2019

Setting such permissions which are universal will require another component in CORE which is universal across editors.

@PVince81 PVince81 removed their assignment Sep 5, 2019
@stale
Copy link

stale bot commented Sep 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

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

4 participants