SharingCollection rules API is too restrictive

y-lohse commented 4 years ago

The current sharing API is quite restrictive, it only accepts a sharingType parameter and infers some stuff from it. It will most notably generate the sharing rules automatically which prevents apps from having fine grained control over them. This is an even stronger problem because the rules used for anything that isn't a io.cozy.files only really make sense for photo albums. A smaller but similar problem is that the sharing type also controls whether the sharing should be open or not.

IMO, SharingCollection.share should accept a open_sharing and a rules parameter instead. Since rules have a very specific format, we could expose a helper somewhere (maybe in a sharing model?) to help users generate them. We could make these changes non-breaking.

What do you think?

ptbrowne commented 4 years ago

IMO, SharingCollection.share should accept a open_sharing and a rules parameter instead. Since rules have a very specific format, we could expose a helper somewhere (maybe in a sharing model?) to help users generate them.

Yes I agree. openSharing though for coherence.

I think the helper should be directly on the SharingCollection, as a static method (this way, the user does not have to import two files).

Crash-- commented 3 years ago

I saw this issue a bit too late.

I added the open_sharing param here (https://github.com/cozy/cozy-client/pull/816)

But instead of adding a rules parameter, I added a new boolean to force a rule if really needed. In cozy's world, today, we don't have anything one-way.

I don't want to have to deal with this complexity (rules description) in an App, even by importing / using helpers. I don't want to know that we can have two-way with or without read_only.

I understand the need of having a full control of what we're doing, but since our libs are used by customers, I also want something easy to use at first.

Maybe we can :

create a new create method (instead of share, ShareCollection.share() is not coherent with our codebase)

create({document, description, previewPath, openSharing, recipients, readOnlyRecipients, rules})

Like that we :

remove the "sharingType" notion (aka one-way / two-way)
we can set recipients and readOnlyRecipients at the same time
if the rules object is provided then we use it (and check if the rules give the permission to add recipients for instance)
if not, them we generate the rules with our default configuration

Like that, we can have full control of what we do, and we provide clear API to our customers

After reading again my proposal, it seems close to what you have discussed since I just understand that you wanted to remove the sharingType concept too 🤔

ptbrowne commented 3 years ago

+1 for the options object.

It seems to me that recipients/readOnlyRecipients and rules control the same thing here. This makes me wonder what are the differences between the arguments. I do not like that we can express forbidden things with this API. I think we should make it so that we cannot express invalid things.

I understand the need of having a full control of what we're doing, but since our libs are used by customers, I also want something easy to use at first.

We must be careful not to make future things hard. I think having an orthogonal API (with every options not affecting the others), is a good property to have, both for the user (that does not have to wonder "Am I allowed to use rules here since I pass recipients ?") and for the future of the API.

readOnlyRecipients / recipients

I think that it is cumbersome to require the user of the function to group the users read-onlyness beforehand, to be able to use the function.

Maybe we could have:

create({
  document,
  description,
  previewPath,
  openSharing,
  recipients: [{ _id, _type }, { _id, _type, mode: "read-only" }]
})

?

I see in the rules doc this

          "add": "sync",
          "update": "sync",
          "remove": "sync"

in the rules, it could be good if the API could express this also ?

Crash-- commented 3 years ago

It seems to me that recipients/readOnlyRecipients and rules control the same thing here

Not exactly.

Rules are defined at the root level of an io.cozy.sharings. By default those rules are enabled for each recipients. But we can override those rules for one recipient by adding a read_only flag. But, and the nuance is there, if we have a rule saying :

add : sync,
update: sync,
remove: sync

then we can add a read_only flag to one recipient because we're shrinking the rules to something smaller, and stay with full access by default to others.

But if we create a sharing with the following rule :

add: push,
update: push,
remove: push

We can't add a read_write flag to the recipient.

And because we can't update the rule once created (to pass from an add:push to an add:sync for instance) we create by default the sharing with the most open rules.

I just checked, and we can add recipients to a sharing with either push or sync. So we can't have inconsistency

recipients: [{ _id, _type }, { _id, _type, mode: "read-only" }]

I'm not sure about that. By using recipients and read_only_recipients we use the same terminology as the stack. Since we're remove the sharingType to be as close as possible as the stack api, we should stay the same for that?

To resume:

create({
document, 
description, 
previewPath = '/preview', 
openSharing = 'false', 
rules = [{
...
"add": "sync",
"update": "sync",
"remove": "sync"
}],
recipients,
readOnlyRecipients
})

Crash-- commented 3 years ago

updated https://github.com/cozy/cozy-client/pull/816/files

rules are more complicated that only "add" / "update", but it's the same thing

cozy / cozy-client

SharingCollection rules API is too restrictive #774

readOnlyRecipients / recipients