search

cs3org / OCM-API

OpenCloudMesh API
38 stars 11 forks source link

Improved properties of the NewShare endpoint + some more cleanup #57

Closed gmgigi96 closed 1 year ago

gmgigi96 commented 1 year ago

Closes https://github.com/cs3org/OCM-API/issues/56

Co-authored with @glpatcern

Includes:

The corresponding changes in the CS3 APIs are in https://github.com/cs3org/cs3apis/pull/199

glpatcern commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

This also closes https://github.com/cs3org/OCM-API/issues/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 commented 1 year ago

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 https://github.com/cs3org/reva/issues/2104). Can we make this a property of the datatx type protocol?

glpatcern commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

@gmgigi96 @glpatcern : OK

michielbdejong commented 1 year ago

redundant now that the invitation workflow already includes this information

That doesn't hold for the share-with and public-link ("save to your cloud") flows.

glpatcern commented 1 year ago

That doesn't hold for the share-with and public-link ("save to your cloud") flows.

Ah, interesting to know, so those flows don't actually require a prior invitation. Let's discuss the details Edit: just discussed with Hugo, we clearly have to restore those properties indeed.

gmgigi96 commented 1 year ago

I reverted the recipient display name and as we discussed, I have marked the providerId as deprecated for backward compatibility.

gmgigi96 commented 1 year ago

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

It's in the description of providerId

gmgigi96 commented 1 year ago

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:

What do you think? @labkode @glpatcern @michielbdejong

glpatcern commented 1 year ago

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 commented 1 year ago

@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:

glpatcern commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

OK, following further discussions we have settled in postponing all privacy-related and security-related issues (including e.g. https://github.com/cs3org/OCM-API/issues/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?

smesterheide commented 1 year ago

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 commented 1 year ago

@michielbdejong following the direct discussion, https://github.com/cs3org/OCM-API/pull/57/commits/a59b273e0c694e17877057027fe4fbe7105a9c45 should do. Note that the notification is consistently renamed as well.

glpatcern commented 1 year ago

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.