search

owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.36k stars 2.06k forks source link

Sharing 2.0 #19331

Closed rullzer closed 8 years ago

rullzer commented 9 years ago

@schiesbn and myself spend some time at the conference coming up with a possible design for sharing 2.0. Here is basically an overview of my notes.

Note

As discussed in Berlin the sharing stuff will for now only be for files. Contact and calendar will use the old sharing code but will be moved to webdav.

Architecture

We want to move all real sharing to separate apps. Right now sharing is mostly in core and this is not really the right place. It makes it hard to have custom sharing implementations.

We propose a single sharemanager in core where apps can register themself. Furthermore in core we will provide a number of interface that apps have to implement in order to become a shareprovider.

Currently we have the following 3 categories of shares:

We will of course keep those 3 categories but each of them will be a separate app. This would mean we would get more core apps. Which will take some time but in the end we should end up with a more modular (and better maintainable) sharing setup.

We will then also need another new app that handles the OCS Sharing API. This clearly separates the API and the sharing implementation

No more sharing hierarchy

We propose to move to what we like to call a flat sharing model. This will prevent cyclic sharing or at least make sure it is no longer an issue. (see also https://github.com/owncloud/core/issues/9058)

This effectively means that shares are a combination of the following items

This means that the share owner always has the complete overview of who has access to a share. And that we can easily find out the max permissions for a sharee.

Interfaces

sharemanager

The share manager is just a place where shareproviders can register and where they can be queried. There will be some functions to get a specific share or to get a traversal of all shares in a path etc.

shareprovider

The share provider just implements sharing for a specific type. You can of course tell the provider to share a file. Or to get a share etc.

Each share provider has a unique id.

We plan to have two share providers:

  1. "internal share provider" which will be provided by the primary storage and handle (user, group and link sharing)
  2. "federated share provider".

In general share provider will register them self at the share manager. This way it is easy to develop and add new share provider. For the internal share provider (which could be seen as the main share provider) the share manager will ask the primary storage directly to provide it.

We will implement two share providers as apps, the "oc internals share provider" which does sharing basically as we are doing it today and the "oc federated share provider". Bot will register them self at the share manager if the apps are activated. Additionally the share manager will ask the primary storage if he implements his own share provider. If he returns a share provider it will be used for internal shares, if it returns false the "oc default internal share provider" will be used. That's also the reason why the "oc default share provider" needs to be implemented as a app and not as part of our default storage, because the share provider needs to be also available for other storage if they don't implement their own share provider.

share

Just an object representation of the share. Where you can get and set the attributes etc.

Each share withing a shareprovider has a unique id.

A system wide unique id could be constructed as "providerID#localShareID" or something similiar.

We decided to only pass around userids/groupids/federatedshare ids etc. This makes sure that only the places that have to care about existing users etc will handle that.

Example Code

I took the liberty of already starting with some interfaces:

ShareManager ShareProvider Share

Limitation

We can only handle 1 share provider per type.

During the conference we had the idea to do things on a per storage level. But this brings in a lot of problems. Assume there are 2 folders dirA and dirB. Now the storage where dirA lives supports link sharing but the storage of dirB does not. Now what happens if a file with link share is moved from dirA to dirB?

In general most people will just use the apps that we provide. And in the case that people want very specific sharing behaviour for certain storages they should write their own sharing apps.

Forcing this limitations keeps things simple and understandable and we do not believe this to be a limitation a lot of people will have problems with.

Migration/deprecation path

For third party apps like calendar and contact we will move the existing sharing code to the apps and will make them work. From this point on the app delevlopers have to maintain or replace the existing code.

For file sharing we will re-use the exiting table, update the values add needed columns and remove old columns we no longer need at the end. The idea is that this process will be triggered with a occ script after the main upgrade to 9.0 while the system is live (still need to evaluate if this is possible).

Open Question:

Since we kind of start over. There are a lot of things where we can improve and unify the current sharing design. Some things we came up with.

Expiration date on everything

Currently it is only possible to have expiration dates on link shares. There is no real technical reason for this and it is perfectly valid to have other shares expire as well.

Multiple link shares

Currently only 1 link share is allowed per file. Having multiple makes sense since I might want to share a file with multiple people but not for the same period of time. This could be combined with easily sharing via email so the individual link is automatically sent to the respective person. Individual links can then also be revoked easily based on email/name.

Open Issues

The idea is to implement it in parallel to the existing sharing with small pull request merging sharing 2.0 one by one. In this transition phase it will be possible to switch between old and new implementation for testing purpose. Only if sharing 2.0 is complete and works reasonable well (including all integration tests for the OCS API) we will move the old sharing to the calendar and contacts app like explained under "Migration" and switch permanently to the new sharing.

DB implentation and considerations

In order to faciliate the new sharing we need to modify the database. We want to do this as little as possible to avoid large and huge migration steps.

Eventually we will remove the columns that are only needed for calendar/contacts sharing since they move to webdav. But this can only be done once we kill off the old code (since it is heavily wired into it).

Currently we store the initiated of the share in uid_owner. So the table uid_owner maps to the sharer (sharedBy). We can get the owner of a file via the fileid in the table. But this does not scale. Thus we plan to introduce a new column that holds the owner of the file (shareOwner). This is then allows for quick lookups of share info for the owner since the owner of a file should always have the full overview.

Related issues:

rullzer commented 9 years ago

FYI: @DeepDiver1975 @karlitschek @cmonteroluque @MTRichards

DeepDiver1975 commented 9 years ago

We propose a single sharemanager in core where apps can register themself. Furthermore in core we will provide a number of interface that apps have to implement in order to become a shareprovider.

I question the use case - what would such a share provider be? If sharing is file only - this is a pure private implementation - no app needs to interact on this level.

schiessle commented 9 years ago

I question the use case - what would such a share provider be?

A share provider could be a sharing implementation from a specific storage for example. We need a manager to track the share provider (we may have more than one in the future) and redirect for example OCS share api calls to the right implementation.

DeepDiver1975 commented 9 years ago

A share provider could be a sharing implementation from a specific storage for example.

Which we need to get from the storage interface from my pov

schiessle commented 9 years ago

Which we need to get from the storage interface from my pov

We will see if we need it or not if we move forward with the implementation. But if we assume that not every storage will have is own implementation of sharing we probably need some kind of manager as entry point. For example you have a OCS call to change permission to read-only for share "42", then we need a central instance (the share manager) who says OK share "42" is managed by share provider X (which might come from storage Y) and forward the request to change the permissions. How should a OCS share request for share "42" find the right storage by itself?

rullzer commented 9 years ago

Which we need to get from the storage interface from my pov

I think we should have a clear separation. If you have multiple storage providers there is no guarantee that they support the same set of sharing functionality.

DeepDiver1975 commented 9 years ago

hmmm .. true ... I guess we need both mechanisms.

On the one hand there is the need to explicitly share a file/folder on storage level

$storage->getSharing()->share()

on the other hand there has to be a mechanism to resolve a share to the real physical resource

$shareManager->resolveShare('42')
DeepDiver1975 commented 9 years ago

I think we should have a clear separation. If you have multiple storage providers there is no guarantee that they support the same set of sharing functionality.

There will be no new kinds of sharing.

And even if this is to be analysed how this might fit into the existing structures

rullzer commented 9 years ago

We propose a single sharemanager in core where apps can register themself. Furthermore in core we will provide a number of interface that apps have to implement in order to become a shareprovider.

I question the use case - what would such a share provider be? If sharing is file only - this is a pure private implementation - no app needs to interact on this level.

A share provider for

And maybe more in the future. This way if for example somebody wants to implement their own user/groups sharing on a file metadata level. But still wants to have link sharing. They only need to write their own user/group sharing app.

schiessle commented 9 years ago

@DeepDiver1975 Yes, we need both. I think this are two different levels of abstraction. The share manager is on a higher level and manage sharing on a system wide level no matter which kind of storage or share implementation is used. It will then resolve the storage/share implementation which has to handle the share (as said by @rullzer depending on the share type this can be even something like the "federated share provider" which will be storage independent) and call the share implementation to perform the action.

DeepDiver1975 commented 9 years ago

@schiesbn sounds reasonable

But never the less I still question the share provider use case

PVince81 commented 9 years ago

What happened to the similarly named ticket #13014 ?

schiessle commented 9 years ago

13014 was a early brain storming about what could/should be improved. This one is far more concrete and will be the issue where we define sharing for 9.0 and track the progress. I will check the other issue and see what we can/should move over.

karlitschek commented 9 years ago

Looks good to me. We have to keep the upgrade time under control as discussed in the zero upgrade discussion. So we either make the new code also work with the old sharing tables and run the migration script in the background? Or we make sure that the migration script runs in minutes even for huge installations.

DeepDiver1975 commented 9 years ago

So we either make the new code also work with the old sharing tables

Are there any reasons (for now) to introduce a new table?

karlitschek commented 9 years ago

@DeepDiver1975 Excellent point. I assumed that there will be changes in the data model. Everything would be a lot easier if this can be avoided of course.

rullzer commented 9 years ago

Are there any reasons (for now) to introduce a new table?

I don't know yet. We have to see when we get a bit more into detail. I think yes. since we want to move from a hierarchical sharing model to a flat sharing model (so the owner of the data always knows who has access etc). Doing this live in the same table as the current one is asking for complicated code.

But as I said I'm simply do not know yet. We probably should have a more in depth meeting at some point and maybe even have some (simple) code ready to look at so the discussion is not purely on a high level.

jancborchardt commented 8 years ago

Also keep in mind sharing via email. Normally this is just sending the share link to a person. You should be able to simply type in a person’s email (autocompletion from Contacts as well) and then ownCloud will generate an individual link for that person and also show them in the share list.

That way you can also remove these people from the share. In the backend it would just invalidate the custom link of that person – for others it would still work.

schiessle commented 8 years ago

cc @cmonteroluque as discussed I updated the issue with the latest state (all important information should be in the initial comment https://github.com/owncloud/core/issues/19331#issue-108085914)

@rullzer @DeepDiver1975 please also review it. I updated the sections "share provider", "migration" and added the section "how we implement it"... all based on our discussion we had last Friday

ghost commented 8 years ago

thanks @schiesbn!

schiessle commented 8 years ago

Updated the "shareprovider" section at the top to discuss the need of a way to register share providers at the manager and the rationals why both the "oc default internal share provider" and the "federated share provider" will be a app outside of the storage.

nickvergessen commented 8 years ago

Just a comment, we need a php internal API for shares. I just spoke to @schiesbn and he said that this was not the plan anymore. Quick example where we need it: Activities

When writing a file, we surely do not want to make a OCS request to the share API to get all users that have this file shared.

DeepDiver1975 commented 8 years ago

When writing a file, we surely do not want to make a OCS request to the share API to get all users that have this file shared.

can you please link the current code which is taking care of this?

nickvergessen commented 8 years ago

Activity app listens to the following hooks:

        Util::connectHook('OC_Filesystem', 'post_create', 'OCA\Activity\FilesHooksStatic', 'fileCreate');
        Util::connectHook('OC_Filesystem', 'post_update', 'OCA\Activity\FilesHooksStatic', 'fileUpdate');
        Util::connectHook('OC_Filesystem', 'delete', 'OCA\Activity\FilesHooksStatic', 'fileDelete');
        Util::connectHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', 'OCA\Activity\FilesHooksStatic', 'fileRestore');
        Util::connectHook('OCP\Share', 'post_shared', 'OCA\Activity\FilesHooksStatic', 'share');

Then I grab the original file owner for the file:

        $uidOwner = Filesystem::getOwner($path);
        $fileId = 0;
        if ($uidOwner !== $this->currentUser) {
            Filesystem::initMountPoints($uidOwner);
        }
        $info = Filesystem::getFileInfo($path);
        if ($info !== false) {
            $ownerView = new View('/' . $uidOwner . '/files');
            $fileId = (int) $info['fileid'];
            $path = $ownerView->getPath($fileId);
        }

And finally I get all users that share the file: https://github.com/owncloud/activity/blob/master/lib/fileshooks.php#L176

DeepDiver1975 commented 8 years ago

okay - this is something we need to consider - THX

@rullzer @schiesbn this still fits into our agreed approach to define the ocp interface in a later stage

rullzer commented 8 years ago

Made a start with a bunch of checklist of stuff that we need to do. This list might expand more but it helps us keep track.

@MTRichards to keep you updated. The plan now is to have small incremental improvements to (as a first step) improve the OCS Share API . Since that would also be what the web UI will start using. During this we work with QA to make sure we don't break API and stuff keeps working. So hopefully we will have a steady stream of small PR's.

MTRichards commented 8 years ago

Excellent. :) Thanks for the list!

MTRichards commented 8 years ago

As a question, does ShareManager allow for reporting, or is that out of scope? We get requests for creating share reports based on a user, folder or file to know who shared what with whom. If we had a core API we could query, we could just use that and build an app. Alternatively, we just query the database, but that is more likely to break and we should abstract that if we can.

Note: This is NOT a requirement, just a question.

rullzer commented 8 years ago

THose kind of queries should be possible. But maybe we should have a chat soonish what you actually want (some use cases) so we can see if we can incorporate that in the future.

DeepDiver1975 commented 8 years ago

As a question, does ShareManager allow for reporting, or is that out of scope? We get requests for creating share reports based on a user, folder or file to know who shared what with whom. If we had a core API we could query, we could just use that and build an app. Alternatively, we just query the database, but that is more likely to break and we should abstract that if we can.

Note: This is NOT a requirement, just a question.

@MTRichards generally speaking: anything is possible - can we postpone this so that we can for now focus on getting 9.0 finished? THX

MTRichards commented 8 years ago

Certainly, not a problem. I just want to plant it in your head - sometimes thinking ahead of time prevents decisions that create problems later.

Like I said:

Note: This is NOT a requirement, just a question.

DeepDiver1975 commented 8 years ago

Im happy to chat about this anytime.

PVince81 commented 8 years ago

@MTRichards performing inception ? :wink:

Ideal would be to raise a separate ticket to start a discussion about this possible enhancement.

MTRichards commented 8 years ago

https://github.com/owncloud/core/issues/20492 Done!

rullzer commented 8 years ago

Intergration tests are tracked in https://github.com/owncloud/core/issues/20553

rullzer commented 8 years ago

@DeepDiver1975 @schiesbn what do we want to do regarding ids? Currently id's are ints. However if we have multiple providers (which we will) then just ints is not enough. Since that can lead to collisions.

Now there are 2 approaches I see:

  1. Do <shareid><sharetype>. So assume I have a user share with id 42. Then this would get id 4200 for example. And a federated id with id 13 would become 1306.

This is very simple and will make sure that the id stays an int. But it will force shareproviders to use ints as id.

Of course we could just use strings. So

  1. <providerid>:<shareid>. Assume a federated share with id 8 would then become e.g. fed:8, a group share with our default provider with id 10 ocdef:10. Or a link share with some custom implentation might be custom:c8fff9a.

The second one is more flexible but might require changes to consumers of the OCS Share API (e.g. mobile/desktop clients).

I think in the long run the second option might be the most flexible. But I'm not sure. Opinions/suggestions?

DeepDiver1975 commented 8 years ago

both approaches above will result in a string datatype because

But most importantly: do we consider the change of an share id as public api breakage?

example: before 9.0 the share id was 12345 after 9.0 the share is will be "default:12345"

summoning @karlitschek for api changes summoning @dragotin @rperezb for the clients

rullzer commented 8 years ago

Well of course the providers don't have to store their own ID in the databae. Than can just be added when the object is created. So we can (and should!) still use ints for our internal numbering. Since generating unique id's is very non trivial.

If it is breakage kind of depends on your definition. We do say on https://doc.owncloud.org/server/8.2/developer_manual/core/ocs-share-api.html that the shareid is an int. However if we tell the clients in advance they can just use a string for that field and this should not break current behaviour since the int can just be stored as a string.

It is even a string when passed to the API. We just convert it back to an int. The id is internal anyway and should never be shown to the user.

rullzer commented 8 years ago

@dragotin FYI, while writing the new sharing code for the client I already made the id a string (https://github.com/owncloud/client/blob/master/src/gui/share.h#L111). So nothing to do for the desktop client.

karlitschek commented 8 years ago

@DeepDiver1975 good question. int vs. string makes definitely a different in client depending on the technology. If this would be a 'write only' variable then it would be ok because it is still backwards compatible. Unfortunately we also return the share id in several API calls. So this is a problem. :-( Let's talk about this in the next call.

rullzer commented 8 years ago

@DeepDiver1975 suggested to use a wrapper in the OCS endpoint. So if you only pass an int it is by default an OC internal share. And gets mapped to default:int. Maybe something to consider.

rullzer commented 8 years ago

@DeepDiver1975 trouble is that fails when fetching shares. because there we'll return the new id. Since we have no way of knowing if the client support the new ids this will sometimes fail.

DeepDiver1975 commented 8 years ago

well - v1 of the sharing api is only for non-federated shares and as a result no provider prefix is required - right?

@schiesbn

rullzer commented 8 years ago

Not entirely true. Currently we do return outgoing federated shares in the oc_share table (and thus return them via the OCS Share API).

Also this would then mean that all share providers must use ints as identifiers.

schiessle commented 8 years ago

Maybe we could keep the ids as ints if we make sure that the request always comes with two information, the id and the share type instead of combining both information in the id. But this would be probably also a API change because not all api calls contain both information separately, id and share type.

Further I'm not sure if it work out well if we limit the id to ints, maybe some storages will generate ids in a different way. With strings we would be more flexible.

DeepDiver1975 commented 8 years ago

Further I'm not sure if it work out well if we limit the id to ints, maybe some storages will generate ids in a different way. With strings we would be more flexible.

from my pov we will need string because of all discussed reasons - but this would require v2 of the sharing api which I'd like to postpone for now (maybe a topic for 9.1).

We should not add too much changes at a time

rullzer commented 8 years ago

New issue that popped up.

  1. user0 shares a folder with user1
  2. user1 reshares a subfolder of that with user2

So far so good. So user0 will see

folder (shared with user1)
|-subfolder (shared with user2 by user1)
  1. user0 removes the share of folder with user1

Now what happens? Is the share of subfolder with user2 automatically removed. The thing here is that it can be very costly to traverse the whole tree below a share to find subshares.

This is somewhat inconsistent with the flat sharing model. Since if user1 would just share folder with user2. It would be flattend out. And removing the share with user1 does not harm.

Some suggestions that came up:

Removing of the reshare functionality of subfolder is by far the simplest solution. Which will remove a lot of complexity. But also limits the user experince. If somebody shares a photo album with me and I want to share one of those pictures with a link. This would then be no longer possible.

Leaving the reshare as is feels weird. Since well user1 no longer has access to the files.

Traversing could be an option but might be potentially costly. ( @icewind1991 can you comment on this?)

Comments? @DeepDiver1975 @schiesbn

icewind1991 commented 8 years ago

I think removing the reshare is correct.

As for performance maybe keep track of the relation between a share and all reshares made from that in the db

MTRichards commented 8 years ago

Yep, remove the reshare because you don't have access to the folder anymore.

schiessle commented 8 years ago

I think removing the re-shares of a sub-file or sub-folder could be OK. But what happens if the top-folder was re-shared?

Following situation:

If we remove the re-shares as well user3 will also disappear from the list of recipients, although user1 unshared the folder only from user2. Wouldn't this be a strange behavior?

With the new way of handling shares, a flat list instead of a tree of re-shares I think it would be fine if we don't try to unshare re-shares. The owner sees all shares, so he is in full control. No need to "magically" remove additional shares just because a parent folder was unshared. Sure this gives us the challenge described by @rullzer, how to filter out shares from the "getAllShares" call which are no longer visible for the user. But I think we can solve it. Maybe instead of un-sharing the re-shares we could just set the "shared-by" value to the owner?