Improved properties of the NewShare endpoint + some more cleanup

[gmgigi96]

Closes #56

Co-authored with @glpatcern

Includes:

• Properties of the protocols parameter in NewShare fully spelled out [this is a breaking change].
• Renamed providerId as shareId, both in NewShare and in /notifications, as this is how it is meant to be [this is a breaking change, though semantic is unchanged].

The corresponding changes in the CS3 APIs are in cs3org/cs3apis#199

[glpatcern]

@michielbdejong we're now trying to implement this "for real", what do you think about this?

The example can surely be made with no repetitions

[glpatcern]

For reference, OCM permissions are handled as follows in nextcloud:

https://github.com/nextcloud/server/blob/1021c894542a778948bfddb9722e10dddf91f4b8/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php#L694-L709

[glpatcern]

@michielbdejong and @labkode we should be good to go with those changes, could you please review?

This also closes #49, by using the explicit list of strings adopted by Nextcloud (cf. my previous comment).

We can also discuss what was raised by Michiel some weeks ago.

[redblom]

I mentioned my reservations about using webdav as keyword for regular shares to @glpatcern recently at a cs3mesh4eosc wp4.4 call. WebDAV is a protocol that's being used at least in transfers also. How about simply changing that to share.
So: share, webapp, datatx

Also, the receiver of a transfer type share would like to know the transfer size before deciding to accept it (see cs3org/reva#2104). Can we make this a property of the datatx type protocol?

[glpatcern]

I mentioned my reservations about using webdav as keyword for regular shares to @glpatcern recently at a cs3mesh4eosc wp4.4 call. WebDAV is a protocol that's being used at least in transfers also. How about simply changing that to share. So: share, webapp, datatx

Good point, thought to address that on yet another PR but we could do this here indeed. Now I'd try and find another synonym given that the OCM endpoint is already called /shares and its one JSON parameter including all required properties is called share, making it already overused ;-)

Also, the receiver of a transfer type share would like to know the transfer size before deciding to accept it (see cs3org/reva#2104). Can we make this a property of the datatx type protocol?

Sure. I can do that if you want.

[gmgigi96]

I mentioned my reservations about using webdav as keyword for regular shares to @glpatcern recently at a cs3mesh4eosc wp4.4 call. WebDAV is a protocol that's being used at least in transfers also. How about simply changing that to share. So: share, webapp, datatx

I'm not so convinced of renaming the keyword webdav. The idea here is that on a share the user knows how to access (i.e. which protocol to use) the resource. The fact that WebDAV is already used in a transfer also is just a matter of internal implementation of the datatx protocol.

[redblom]

Also, the receiver of a transfer type share would like to know the transfer size before deciding to accept it (see cs3org/reva#2104). Can we make this a property of the datatx type protocol?

Sure. I can do that if you want.

Great !

[redblom]

I mentioned my reservations about using webdav as keyword for regular shares to @glpatcern recently at a cs3mesh4eosc wp4.4 call. WebDAV is a protocol that's being used at least in transfers also. How about simply changing that to share. So: share, webapp, datatx

I'm not so convinced of renaming the keyword webdav. The idea here is that on a share the user knows how to access (i.e. which protocol to use) the resource. The fact that WebDAV is already used in a transfer also is just a matter of internal implementation of the datatx protocol.

My thinking is as follows:
I differentiate between ocm-share type and the protocol to use. Therefor I see the 'properties hierarchy' as follows:

   datatx:                  <-- the type
      protocol: "webdav"    <-- the protocol
      sharedSecret: "hfiuhworzwnur98d3wjiwhr"
      srcUri: "https://open-cloud-mesh.org/remote.php/webdav/path/to/resource"
      size: 1000000000000

Or the protocol could be taken one level up:

   protocol: "webdav"    <-- the protocol
   datatx:               <-- the type
      sharedSecret: "hfiuhworzwnur98d3wjiwhr"
      srcUri: "https://open-cloud-mesh.org/remote.php/webdav/path/to/resource"
      size: 1000000000000

But it's all webDAV anyway (for all 3 types I suppose) so that could even be taken for granted (as default protocol) and possibly be left out. So again I tend to lean towards share, webapp, datatx
What do you think?

[gmgigi96]

My thinking is as follows: I differentiate between ocm-share type and the protocol to use. Therefor I see the 'properties hierarchy' as follows:

   datatx:                  <-- the type
      protocol: "webdav"    <-- the protocol
      sharedSecret: "hfiuhworzwnur98d3wjiwhr"
      srcUri: "https://open-cloud-mesh.org/remote.php/webdav/path/to/resource"
      size: 1000000000000

Or the protocol could be taken one level up:

   protocol: "webdav"    <-- the protocol
   datatx:               <-- the type
      sharedSecret: "hfiuhworzwnur98d3wjiwhr"
      srcUri: "https://open-cloud-mesh.org/remote.php/webdav/path/to/resource"
      size: 1000000000000

Probably it was not clear before, and in this PR with @glpatcern we tried to be clearer in the specs, but the shareType is the recipient's type, so if a recipient is a user or a group. Currently only the user will be actually implemented as the group one was never discussed/formalized properly since the beginning of the project (the invitation workflow takes into consideration only the discovers of users in fact).

We intended the protocol as the way of accessing/use the share. For example:

• if a user receives a share with protocol webdav, the recipient knows that to access the resource he needs to use webdav as protocol, and that he can use those resources for browsing, downloading files, create dirs (according to the permissions), but he cannot open a remote app.
• if a user receives a share with protocol webapp, he will use the link and make a GET request, to open the file in the remote site with the default app of that resource for example (this might need more formalization).
• if a user receives a share with protocol datatx, he knows that he can trigger a data transfer from the remote site to its desired destination and then use the data locally. It's matter of the datatx protocol understand which is the way of accessing the resource (in your case webdav, but in theory can be everything).

Considering the last example, can be useful when creating the transfer know which is the source protocol, so we can even put this info in the source link, as dav+https://open-cloud-mesh.org/remote.php/webdav/path/to/resource, or add an extra option, like sourceType.

But it's all webDAV anyway (for all 3 types I suppose) so that could even be taken for granted (as default protocol) and possibly be left out.

I'm not so sure about this. I would not strict OCM to have webdav as default protocol. For example in the context of web apps this does not make any sense imo.

[glpatcern]

But it's all webDAV anyway (for all 3 types I suppose) so that could even be taken for granted (as default protocol) and possibly be left out. So again I tend to lean towards share, webapp, datatx

Indeed a webapp share is a pure https link that is expected to be browser-friendly, and directly usable by the user without any other intermediate service.

Overall it's a bit of an academic question to say that datatx is not a "protocol" strictly speaking but a "class" or "type" orthogonal to the protocol, but the reality is also that OCM so far did not have such "type" (the shareType refers to users vs groups as mentioned), and IMHO it is not worth to introduce it. Instead, I support the idea to extend the protocol to an array and "flatten" this property as one of those protocols, with the potential added value that the same share may have all of the webdav, webapp, and datatx protocols - meaning that it is available to be transferred as well as remotely accessible.

[redblom]

@gmgigi96 @glpatcern : OK

[labkode]

Add comment regarding migration: now is needed both values, later one will be deprecated

[gmgigi96]

Add comment regarding migration: now is needed both values, later one will be deprecated

It's in the description of providerId

[gmgigi96]

I was just thinking that probably the properties of WebDAV are not fully spelled out. It's not clear what the sharedSecret is, if it's basic/digest/bearer authentication. Should we specify the type of authentication?
And in case, the shared secret should change accordingly:

• basic: username:password
• digest/bearer: an hash

What do you think? @labkode @glpatcern @michielbdejong

[glpatcern]

I was just thinking that probably the properties of WebDAV are not fully spelled out. It's not clear what the sharedSecret is, if it's basic/digest/bearer authentication. Should we specify the type of authentication? And in case, the shared secret should change accordingly:

Good point, but for what I understand the OCM specs are more of a guideline, yet quite detailed, than a full protocol spec like the CS3 APIs (which we use to generate strict bindings). Therefore I don't see this is needed, in the sense that it may remain as it is.

[labkode]

@gmgigi96 ideally the protocol should specify how the other end will make a request and I agree that having just sharedSecret is not enough as you don't know two things:

• a) How to send them (query parameter? header? body?)
• b) How to encode it: basic auth? jwt?
  If we assume every platform implements all possible methods in a) then we need to:
• 1. define what b is in the protocol spec
• 2. or specify in the protocol that implementors should try first one method, then other, ...

[glpatcern]

On another side, to complete what we discussed we should rephrase this:

https://github.com/cs3org/OCM-API/pull/57/files#diff-9cfca4a1b73e1e28e30fb9b0b984aad6d4caaf0819c61ed40ad338600531f745R259-R264

And add something like

To protect the identity of the parties, for shares created following an OCM invitation,
the user id SHALL be hashed, and recipients implementing the OCM invitation workflow
MAY refuse to process shares coming from unknown parties.

And in the invitation request and response we should also detail that we hash the ids (with examples).

[gmgigi96]

@gmgigi96 ideally the protocol should specify how the other end will make a request and I agree that having just sharedSecret is not enough as you don't know two things:

* a) How to send them (query parameter? header? body?)

* b) How to encode it: basic auth? jwt?
  If we assume every platform implements all possible methods in a) then we need to:

* 1. define what b is in the protocol spec

* 2. or specify in the protocol that implementors should try first one method, then other, ...

To keep things simple for now we can just specify the authentication type (can be only basic and digest) as mentioned in the WebDAV RFC https://www.rfc-editor.org/rfc/rfc4918#section-20.1, and maybe expand in the future if we need more.

[glpatcern]

@gmgigi96 ideally the protocol should specify how the other end will make a request and I agree that having just sharedSecret is not enough as you don't know two things:

This of course makes even more sense, but my point was in the spirit of general backward compatibility: what is the current behavior of e.g. Nextcloud? and ownCloud? and ...?

In a way, going from something totally unspecified (we barely had the notion of a secret, as an example in a generic properties object) to a full spec would be like instantly deprecating implementations that do not follow that spec. What about suggesting a method in the comments?

[glpatcern]

OK, following further discussions we have settled in postponing all privacy-related and security-related issues (including e.g. #23, as well as how WebDAV secrets are expected to be used by implementations) to a topical discussion during the upcoming CS3 conference, when OCM will be one of the prominent topics.

Therefore @gmgigi96 I'd just rephrase the user id SHALL be hashed -> the user id MAY be hashed, and then this is ready for merge and completes the package of changes to fully support the invitation workflow and the sharing within ScienceMesh.

@michielbdejong could you please review?

[michielbdejong]

please clarify whether this PR will break compatibility with existing implementations?

[smesterheide]

Hello all, I have worked with OCM in Nexctloud recently. I think all changes are warranted but we should keep in mind that this PR will break OCM compatibility for existing implementations. NC uses the /ocm-provider endpoint and reports apiVersion as 1.0-proposal1. It would be great to have this discoverability in the spec itself.

[glpatcern]

@michielbdejong following the direct discussion, a59b273 should do. Note that the notification is consistently renamed as well.

[michielbdejong]

LGTM
Implementers can start supporting the new field name shareId alongside its old name providerId: send both, and accept whichever one is present.
Just a minor comment about the description of the uniqueness of shareId.

[glpatcern]

LGTM
Implementers can start supporting the new field name shareId alongside its old name providerId: send both, and accept whichever one is present.

Thanks Michiel, I agree implementers can go ahead using both field names and eventually drop the deprecated one, but it's good to have the specs reflect the final state. I also adapted the phrasing.