Preparing v1.1

[glpatcern]

This PR is to tag v1.1 of the OCM standard, and add a changelog to the project.

The current specifications in the develop branch have been agreed across all stakeholders, therefore this is to formally mark them as public and supported.

[glpatcern]

Quoting all people involved with OCM in the past, i.e. @schiessle, @moscicki , @michielbdejong, @gmgigi96, @labkode, @smesterheide, @andreasfk: if you have any comments please react, otherwise this PR will be merged and a tag produced in 10 days from now, on Monday, 15/05/2023.

[yasharpm]

I assume this pull request concerns every change made since release v1.0.0 and not only the latest commit. In which case I managed to do a full review and here are my comments:

1. expiration on new shares is ambiguous as it is an optional field that can only have a time. Seems like the cases where a share does not expire is not covered. At least it must be explicitly mentioned in the specification that the absence of the field means it doesn't expire. I would prefer for this field to be nullable and also required.
2. It would be nice to a have a link to a resource that describes what a federation share type is. Although I can understand if we decide to ignore this one.
3. protocol.name also has to be deprecated. This is the only point I have to insist on. I have left a comment with complete reasoning on this commit.
4. A minor almost typo which is nice to be fixed. Also commented on its relevant commit.

[glpatcern]

Thanks @yasharpm for the extensive review. Replies inline:

1. expiration on new shares is ambiguous as it is an optional field that can only have a time. Seems like the cases where a share does not expire is not covered. At least it must be explicitly mentioned in the specification that the absence of the field means it doesn't expire. I would prefer for this field to be nullable and also required.

Well spotted, I fixed the description. As the field is new compared to v1.0, I think it's better to have it optional.

2. It would be nice to a have a link to a resource that describes what a federation share type is. Although I can understand if we decide to ignore this one.

I agree, @smesterheide introduced this feature: could you provide an example how this is implemented in Nextcloud?

3. protocol.name also has to be deprecated. This is the only point I have to insist on. I have left a comment with complete reasoning on this commit.

Of course this is the most controversial point... What you are suggesting is what Gianmaria and I initially proposed prior to #69, but that was making the jump from v1.0 to v1.1 too hard. In particular, deprecating a field that in v1.0 is required felt like a breaking change. Therefore, I came up with the idea of using the protocol.name as a switch, so that e.g. Nextcloud will be able to handle the old and the new format at the same time in the code with an easy if $name === 'multi' or something like that.

Sure, the current state is a compromise, but it paves the way for a v2.0, where the breaking change will be to completely remove protocol.name and protocol.options in favor of the new properties. As such, I would not revert again the decision taken in #69.

4. A minor almost typo which is nice to be fixed. Also commented on its relevant commit.

Yeah this was almost a joke and I actually agree to not go that far, so I fixed it.

Could you please have another look?

[yasharpm]

I think v1.1 with protocol name being multi is breaking. Just imagine the case where NextCloud has adopted v1.1 before OwnCloud. NextCloud users won't be able to share a file with OwnCloud users. Simply because NC sends multi and OC expects webdav. So everyone will have to update to v1.1 at the same time or there will be unhappy users. But v1.1 had to be backward compatible in the first place.

On the other hand if name is optional. NC can still send webdav for backward compatibility and OC handles it. A v1.1 OCM server doesn't look at name at all and instead happily finds the webdav key. So it works for everyone.

[glpatcern]

I think this is to be managed by the implementation, and the issue if you like is even within Nextcloud itself, as already discussed several weeks ago: NC v.26 is OCM v1.0 only, and let's pretend NC v.next is OCM v1.1 enabled. NC v.next MUST offer OCMv1.0 shares to endpoints that - by their /ocm-provider endpoint - advertise OCM v1.0, and MAY offer OCMv1.1 shares to endpoints that advertise v1.1 shares. After some (potentially very long) time, NC may decide to publish a NC v.ultimate version that drops support for v1.0 shares.

The protocol is changing, there's no way around it, but v1.1 provides ways to manage both. Also let me repeat that the v1.0 protocol was never fully specified: the access to a remote share has just been reverse engineered by Michiel, and documented in #72. At least, the protocol.name (that is required, let's not forget that) eases the implementation.

[yasharpm]

It's true that the v1.1 implementations can check the version of the receiver and if it says v1.0 they can behave differently and make sure everything works. However I would not call this backward compatible as the same logic could be used for any future versions with completely different specifications. Even if the discovery endpoint gets non-existent in some version in the far future, still you can check if the discovery end point exists and make adjustments according to that. I would call this supporting multiple versions and not backward compatibility.

I also don't understand why you insist on keeping name field. If you mean it to always have the constant value of multi it is already equivalent to calling it deprecated because it will be irrelevant and no one needs to read it from v1.1 onward. Meantime my suggestion doesn't break anything. It covers every scenario you are concerned about but also my concerns as well. People would be more mad at us by breaking backward compatibility than making a required field deprecated.

I rest my case here. Let's allow others to join in on the conversation.

[glpatcern]

Yes, I'd really love to have others' opinions as well. Please note that it's not me insisting with keeping name here, we had removed it completely and it was agreed to put it back - I'm just reporting the reasoning, you can read the discussions through the PR #69 I already mentioned.

We can shortly discuss this at our next call on Thursday, but I think it's not in the interest of anyone to spend time again arguing about how to support an unspecified protocol moving forward.

[michielbdejong]

See https://github.com/cs3org/OCM-API/pull/72#pullrequestreview-1422562614

[yasharpm]

After an one hour long discussion with @glpatcern @smesterheide @michielbdejong we agreed to disagree and I remained on my position of being against setting the value for protocol.name to multi and not making it deprecated. But as the general opinion was to merge it as it is, the conclusion was that it will be merged and tagged as v1.1.

[smesterheide]

Thanks for your thorough review @yasharpm.

I just realized today that in v1.1 the default way of sending single-protocol shares (eg. webdav) uses multi as well. While #72 addresses the use case when v1.0 is sharing with v1.1 the other way around is not guaranteed to succeed (see discussion above).

The question is whether we put the burden on the receiving side or on the sending side. @yasharpm also proposed to always send a mixed v1.0/v1.1 share.

I would like to put forward #73.

[glpatcern]

The question is whether we put the burden on the receiving side or on the sending side

IMHO the burden remains on the sending side, no matter how the specifications are written, and this simply for managing UX.

I see that my earlier comment may suggest that a sender has to look up the receiver's version to decide: that's not accurate, the sender must really look at the offered capabilities (multi-entries in resourceTypes[0].protocols, or non-empty capabilities[]). Typical example: a sender wants to send an invitation to a receiver. The sender MUST ensure the receiver does understand the /invite-accepted call, and disable the feature to its users if not. Likewise with multi-protocol: the sender MUST ensure the receiver is capable to exploit a payload with multiple URIs, otherwise the receiver may parse part of it, the share will succeed but yet the user on the receiving side cannot exploit the share the way the sender exposed it - for a totally broken UX (this is what I tried to explain at the end of the call when Sandro had to leave, yep that was a long call...).

All in all, regardless the specifications, being v1.1 a proper superset of v1.0 with optional parts it means that v1.1 implementations have to manage the optional parts without assuming anything on the receivers.

[yasharpm]

A minor version occurs when there are added new features like the new /invite-accepted end point but everyone can still use it the way they were using it before with no worries. A major version occurs when otherwise happens.

I disagree with looking at it as a sender or receiver burden on the protocol level. A protocol specifies the message and the response as a whole. And should be versioned as such. Hence, as long as they are on the same major version, they need not worry about versions to enable the basic features.

A "good" service would implement multiple versions with proper UX for the user as @glpatcern is concerned with. We are using software that supports multiple versions every day. Think HTTP versions, SSL versions, IP versions. Hence a good OCM software would read the versions and provide proper UX to the user of what is possible no matter a minor or a major version. A good software should even cover and protect the user from bad protocol versioning or a bad protocol as NC and OC are doing it currently.

Looking only at the protocol versioning standards, it is observable that the proposed v1.1 adds new features but also changes the behavior of something that existed before. Hence it must be incremented as a major version and not minor. Not doing so CAN cause real life problems. An engineer might look only at the proposed v1.1 specification and since it is not a major version, does not implement the aspect that figures out the version (be it by directly checking the version or querying the capabilities) and always send multi. What will follow is sending messages to v1.0 will fail and the mistake is with the people who marked the version as v1.1!

If we only look at ourselves who are fully aware of every change in every version, it is hard to notice a problem here. But the versioning standards are there for a reason. It being to pass the essential information to outside unwise parties.

However, as we all truthfully feel that raising the major version should be avoided (the reason for that feeling is adding complexity to the software implementations that can be avoided), I have proposed a solution to stay on v1.1 and not move on to v2.0 just yet:

Always send webdav as the protocol.name and put relevant information in protocol.options. V1.0 implementations won't be affected. V1.1 implementations MUST look for protocol.webdav and other keys first and look for protocol.name as a fall back strategy.

We will still get all the feature that the new version adds. We can still have good software that is aware of the versions and does all the right UX.

[glpatcern]

Looking only at the protocol versioning standards, it is observable that the proposed v1.1 adds new features but also changes the behavior of something that existed before.

This was not the intention. @yasharpm could you please suggest an improved description in

https://github.com/cs3org/OCM-API/blob/develop/spec.yaml#LL420C1-L425C72

Such that it is made clear that the existing behavior is preserved in full, also looking at the examples?

[glpatcern]

And on the specific proposal:

Always send webdav as the protocol.name and put relevant information in protocol.options. V1.0 implementations won't be affected. V1.1 implementations MUST look for protocol.webdav and other keys first and look for protocol.name as a fall back strategy.

I think the consensus on the call was that it was suboptimal compared to the current one. Anyway, my issues here are:

• It would cause a repetition of the sharedSecret
• Stating that an implementation MUST look at an optional property does not seem correct.
• It implicitly endorses the (so far undocumented and out of spec, I must repeat again!) current practice of using options, whereas the idea was to be independent from those options altogether, in view of a future v2.0 where that field will disappear.

Edit: in v1.0 protocol.options was actually required and left unspecified, making v1.0 just incomplete from a pure "syntax" point of view (so kind of "symmetric" of my 2nd issue). That was/is the main reason why protocol.options should be deprecated (despite it was required), and v1.0 should not be used as a model. We're trying to fix that after all!

[glpatcern]

I have rebased on top of the latest changes, and added a commit on README.md amending how to access relative paths on shares.