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

Caps Disco #136

Merged
merged 5 commits into from
Nov 14, 2024
Merged

Caps Disco #136

merged 5 commits into from
Nov 14, 2024

Conversation

michielbdejong
Copy link
Contributor

In my presentation about OCM at CS3 2023 in Barcelona I had this slide about CAPabilitieS DISCOvery ;)
cs3-2023-bcn-ocm 001

Now I'm thinking if we put good capability discovery into the /.well-known/ocm / /ocm-provider document, then discovery doesn't even have to rely on the apiVersion field we have there. Older implementations will just have less entries in the capabilities object, or the object will be missing from the document altogether.

@michielbdejong
Copy link
Contributor Author

The current resourceTypes object is weird because it seems to be used both to advertise what types of resources a server can receive, and to advertise details about how this server acts as a resource server.

@glpatcern
Copy link
Member

Concerning apiVersion, IMHO its useful for humans, as the branding, and should NOT be relied upon to decide on caps. Yet it is used for that by Nextcloud.

@michielbdejong
Copy link
Contributor Author

Capabilities should be about being able to process something you receive.
Being able to send something or not is not so interesting to discover.

Copy link
Member

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I think we can include the full list in the spec as per #121 !

README.md Outdated Show resolved Hide resolved
@glpatcern glpatcern linked an issue Sep 25, 2024 that may be closed by this pull request
@glpatcern
Copy link
Member

@michielbdejong can you please rebase this on top of develop, as opposed to caps-disco-base ?

@michielbdejong michielbdejong changed the base branch from caps-disco-base to develop October 17, 2024 13:10
@michielbdejong
Copy link
Contributor Author

Rebased on develop.

@michielbdejong
Copy link
Contributor Author

I think we want to start off our capabilities list with all functionalities that we introduces since 1.0-proposal1 and for which one server needs to know whether the other one supports it. I can think of the following list:

  • ability to receive a federation-type share (but this should go into the share types object)
  • ability to receive a share without a 'protocol.name' being set to 'webdav' and without 'protocol.options' being present
  • ability to receive a share with more than one protocol (maybe merge this with the previous one?)
  • ability to receive a share with a webdav uri other than the base uri from discovery (do we think this is useful, actually?)
  • ability to use the code flow to obtain a bearer token (i.e. receive shares that do not have a sharedSecret)
  • ability to honour an MFA requirement (this is a super important one, we don't want requirements to get lost in translation!)

Functionalities which I think don't make sense to announce (these functionalities can just be present unannounced):

  • /ocm-provider being copied to /.well-known/ocm (it makes no sense to announce that in the doc itself)
  • ability to send a federation share (this is implicit when such a share is sent)
  • ability to send a share with an MFA requirement
  • ability to send an invite (this is implicit in the existence of the invite)
  • ability to receive an Invite Acceptance Request (this is implicit in the existence of the invite)
  • ability to sign a http request

@michielbdejong
Copy link
Contributor Author

so caps to add:

  • honour an MFA requirement
  • receive a share with new 'protocol' format
  • receive a share with a webdav uri that is a relative path to added to the discovered one (not an absolute URL)
  • receive a share with a code (and no sharedSecret)

webapp URI should probably also somehow be bound to the server domain name from discovery

@glpatcern
Copy link
Member

Let's not forget:

  • receive an invitation

As very first capability (in the form of endpoint)!

Copy link
Member

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few preliminary comments for some discussion

IETF-RFC.md Outdated
@@ -234,10 +234,10 @@ Step 7: The JSON response body is the data that was discovered.
The JSON response body offered by the Discoverable Server SHOULD contain the following information about its OCM API:

* REQUIRED: enabled (boolean) - Whether the OCM service is enabled at this endpoint
* REQUIRED: apiVersion (string) - The OCM API version this endpoint supports. Example: `"1.1.0"`
* REQUIRED: apiVersion (string) - The OCM API version this endpoint supports. MUST start with `"1."` and clients MUST ignore the rest of the string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As already commented, I'd not enforce any value, in particular the MUST start with 1. - we will have 2.0 :)

Maybe just It is provided for information purposes only: clients SHOULD NOT infer capabilities based on its value, or (better) say that when discussing capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that servers can announce which version of the spec they support. So if they support the version that says "announce me as 1.*", then that is what they should announce.

We could keep it as "anything goes" but we don't want a 1.* server to announce a 2.* API version, right?

we will have 2.0 :)

Exactly, and then we'll change the "MUST start with 1." to "MUST start with 2.", that's how we link the api version to the spec text. We can discuss it in today's meeting!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'll miss today's meeting)
Hmm I'm not sure - IMHO the spec's version is an attribute, a "payload", not an inherent feature ("MUST start with...").

And whether a server announces version A.B but actually implements C.D, this is more a matter of compliance that can (and should) be independently tested, e.g. by (y)our test suite, not of "adhering to a standard" I think.

IETF-RFC.md Show resolved Hide resolved
@michielbdejong
Copy link
Contributor Author

Let's not forget:
receive an invitation
As very first capability (in the form of endpoint)!

There are two sides to this and I think neither of them warrant a discoverable capability. When Alice sends an invite to Bob:

  • capability to receive an invitation - this is between Bob and Bob's server, not something Alice's server would, or even could discover. Remember, if Alice's server knew the FQDN of Bob's server then the invite would be unnecessary.
  • capability to receive a server-to-server invite acceptance request - if Alice's server allows Alice to mint invites containing its FQDN, then that in itself already implies that it is capable of also receiving acceptance notifications for it, right?

@michielbdejong michielbdejong marked this pull request as ready for review October 24, 2024 12:01
Copy link
Collaborator

@mickenordin mickenordin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@glpatcern
Copy link
Member

  • Remember, if Alice's server knew the FQDN of Bob's server then the invite would be unnecessary.

Are you sure? The principle of invite is for a user to add a trusted party, even if the Server URL of the other party is well known. I think discovering whether B's server supports acceptance of invites (which ok implies also issuance of invites, both being an optional feature, and thus a capability), even without a WAYF page, is necessary for A's server to expose the invite flow altogether.

@mickenordin
Copy link
Collaborator

Ok, my understanding is that the two points from @glpatcern is that:

  1. we should NOT do the change on line 237, that is not change the wording around versioning.
  2. We should leave invitations in as a capability.

For me I am fine with both these modifications to this pr, and if we do them, we can go ahead and merge this pr which would be a great step onwards to version 1.2. So what do you say @michielbdejong can you agree to those two changes?

Copy link
Member

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Do you want to patch the spec.yaml in this PR as well?

@michielbdejong
Copy link
Contributor Author

Checked that it looks OK on spec.yaml render of this branch:
Screenshot 2024-11-14 at 13 49 35

@michielbdejong michielbdejong merged commit 793c0c2 into develop Nov 14, 2024
@michielbdejong michielbdejong deleted the caps-disco branch November 14, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants