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

status.php version not useful. #3788

Closed
dragotin opened this issue May 12, 2022 · 20 comments · Fixed by #3805
Closed

status.php version not useful. #3788

dragotin opened this issue May 12, 2022 · 20 comments · Fixed by #3805
Assignees
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug

Comments

@dragotin
Copy link
Contributor

An old "friend" is raising up again: The version/product strings in status.php do need a rework.

Currently uses the version field in the status.php to report the "real" ocis version. Unfortunately, at least the desktop clients use the version from that field to enable and disable certain functions, or refuse to work with the server at all. There is one exception to that: If the version string is 0.0.0.0, the client does not refuse it, but it might disable some features. If the ocis version string is 2.0.0.0 all released desktop clients will refuse to work with that server.

The suggested solution is to

  • Set the field version to a recent value of oC10, for example 10.11.0.0.
  • create a new field ocisversion and set that to the real ocis version

That way, future clients can check the product string to use the right version. "Old" clients use the version string by default and continue to work.

@micbar @TheOneRing @felix-schwarz @hosy please double check.

@felix-schwarz
Copy link

The iOS SDK requires these fields to pass a couple of checks before it connects:

  • productname: may not contain the name of known, incompatible server software (at the moment only nextcloud)
  • version: must be either empty (taken as a hint that the server uses version.hidden) - or >= 10.0. Older versions don't have all of the APIs / behavior required by the iOS SDK

The suggested solution is to

  • Set the field version to a recent value of oC10, for example 10.11.0.0.
  • create a new field ocisversion and set that to the real ocis version

That should work.

That way, future clients can check the product string to use the right version.

IIRC the product string can be customized in OC10, so I wouldn't depend on the contents of product.

Instead, I'd suggest that clients should simply check for the existence of ocisversion - and use that instead of version if it exists.

@dragotin dragotin added the Priority:p2-high Escalation, on top of current planning, release blocker label May 13, 2022
@micbar micbar added Priority:p1-urgent Consider a hotfix release with only that fix and removed Priority:p2-high Escalation, on top of current planning, release blocker labels May 13, 2022
@micbar micbar self-assigned this May 13, 2022
@micbar
Copy link
Contributor

micbar commented May 16, 2022

@dragotin Can we change the name to productversion ?
we should not inject the string ocis into reva.

@kulmann
Copy link
Member

kulmann commented May 16, 2022

The iOS SDK requires these fields to pass a couple of checks before it connects:

  • productname: may not contain the name of known, incompatible server software (at the moment only nextcloud)
  • version: must be either empty (taken as a hint that the server uses version.hidden) - or >= 10.0. Older versions don't have all of the APIs / behavior required by the iOS SDK

The suggested solution is to

  • Set the field version to a recent value of oC10, for example 10.11.0.0.
  • create a new field ocisversion and set that to the real ocis version

That should work.

That way, future clients can check the product string to use the right version.

IIRC the product string can be customized in OC10, so I wouldn't depend on the contents of product.

Instead, I'd suggest that clients should simply check for the existence of ocisversion - and use that instead of version if it exists.

@felix-schwarz is it possible/feasible to check for presence of certain capabilities instead of the doing a version check? Or to check a combination of version and product / productname, so that ocis can have any version? I mean, not as an immediate solution, but as something that we want to switch over to in a year from now? Keeping both a version and a productversion in the response (with different values) is really confusing.

@jnweiger
Copy link
Contributor

Fun fact: We have a long tradition of bad version numbers in ocis. Compare #310 and #531

@jnweiger
Copy link
Contributor

jnweiger commented May 16, 2022

Enhancement:
I'd suggest to also provide the entry point /status instead of /status.php so that we can deprecate status.php when classic server gets deprecated. The field ocisversion definitly does not belong into a php thing :-)

@micbar
Copy link
Contributor

micbar commented May 16, 2022

Ok, @jnweiger thanks for the suggestion, i added that route.

Now we need a decision on the naming. 2 PRs are ready.

@dragotin
Copy link
Contributor Author

No, it is not possible to check for a product name and version combination, because already released and running clients do not do that, and ignore the productname (when checking the version) and thus fail to run, because the version would be too small.

@phil-davis
Copy link
Contributor

And I think that it is possible to override the product name in oC10 with theming, so some installations that are themed may not reveal that they are oC10. Is that correct?

So, will future "theming" in oCIS mean that clients cannot rely on productname + productversion in order to "know" what server they are accessing?

i.e. productname + productversion should only be strings for clients to report in logs etc for humans to look at. Clients should never use these to try to make decisions/guesses about how the server might behave.

@micbar
Copy link
Contributor

micbar commented May 16, 2022

@phil-davis Puh we need another round trip to core and back for the testing pipelines.

https://drone.cernbox.cern.ch/cs3org/reva/7146/12/6

@phil-davis
Copy link
Contributor

@micbar you may as well decide in the same core PR if oC10 is going to start advertising productversion - and in that case it will be a copy of version. That will be convenient for clients to be able to get productversion on either oC10 or oCIS or reva implementations.

And do we want to add productversion to core release-10.10.0 ? It might be nice to have that "appear" in the release that is about to happen.

@micbar
Copy link
Contributor

micbar commented May 16, 2022

no, we do not want this in oc10

@dragotin
Copy link
Contributor Author

i.e. productname + productversion should only be strings for clients to report in logs etc for humans to look at. Clients should never use these to try to make decisions/guesses about how the server might behave.

This does not sound correct to me, as productversion is now the new "real" version, and version only the legacy from now on. Both are used by clients.

Is that common sense?

@felix-schwarz
Copy link

felix-schwarz commented May 16, 2022

@felix-schwarz is it possible/feasible to check for presence of certain capabilities instead of the doing a version check? Or to check a combination of version and product / productname, so that ocis can have any version? I mean, not as an immediate solution, but as something that we want to switch over to in a year from now? Keeping both a version and a productversion in the response (with different values) is really confusing.

@kulmann: Looking at future versions of clients, anything is possible.

To achieve compatibility with older versions of clients, however, there's no alternative to fulfilling the requirements of those old versions. In this case: return a version field with the expected version number.

@phil-davis Yeah, IIRC the initial implementation of the iOS SDK was checking for ownCloud in the productname to ensure it's a supported/compatible server - and that fell apart with a customer's customized server. Since then it does the inverse: check only for the name of known incompatible server software - and connect otherwise.

@phil-davis
Copy link
Contributor

phil-davis commented May 16, 2022

Is that common sense?

My thought is that:

version is a data item that indicates a "level of functionality" of the server implementation. Existing clients want to see 9.x 10.x etc in that to make the client "feel comfortable" to run, or to switch on/off some functionality. It is (already) used as an "added extra" with capabilities when clients "decide what to do". So it has that legacy usage. New implementations (reva, oCIS, another implementation) had better support at least "basically what oC10 does" and put 10.x in version

productversion can be whatever version an individual implementation is running - the versioning system of that implementation. The purpose would be to allow someone/some-client that queries the productversion to be able to then have a chance to find the release tag of that "product/implementation" in GitHub. We should document that clients MUST NOT use productversion to "decide what to do". All future functionality enhancements/changes should be decidable by parsing the capabilities. Note: if an installation has changed productname in their theme, then someone who looks at productversion is not going to have a nicely-computable way to get to the matching product release in GitHub. They will need to be a person who "knows" or has an "educated guess" about what the product is and where to find it in a source-code repo.

@felix-schwarz
Copy link

felix-schwarz commented May 16, 2022

Quick summary / brain dump of how the server software / server version info could be shared via /status / /status.php:

  1. naming the version numbers (think ocisversion): rejected, because of merging in reva
  2. using the name of the product (using productname): rejected, because it's not backwards-compatible / may conflict with branding
  3. using a different name for different software (think version for OC10 clients / productversion for ocis clients)
  4. using a numeric value instead of a software's name (think productid, 1/missing for OC10, 2 for ocis, …) + productversion whose content can be interpreted based on productid.
  5. add a new productcompatibility field that works similar to User-Agent HTTP headers, specifying version/compatibility by product,
    f.ex. oc/10.11 ocis/2.0.0

Personally, I'd prefer option 4, because it allows for custom productnames (=> branding / security by obscurity), doesn't include the actual product name (=> which shouldn't go into reva), provides explicit info on the server software used (=> oc10/ocis), is easy to implement by clients and future proof.

@felix-schwarz
Copy link

@phil-davis Yeah, clients should use supported capabilities instead of productversion to determine feature availability.

Only exceptions I can see:

  • bugs in specific server versions that require special handling on the client-side (=> identifiable only through server version)
  • change in behaviour / functionality that can be tied to a version number but doesn't have an associated capability (=> only as a temporary workaround, of course, until it is covered by a capability)

@kulmann
Copy link
Member

kulmann commented May 16, 2022

@felix-schwarz is it possible/feasible to check for presence of certain capabilities instead of the doing a version check? Or to check a combination of version and product / productname, so that ocis can have any version? I mean, not as an immediate solution, but as something that we want to switch over to in a year from now? Keeping both a version and a productversion in the response (with different values) is really confusing.

@kulmann: Looking at future versions of clients, anything is possible.

To achieve compatibility with older versions of clients, however, there's no alternative to fulfilling the requirements of those old versions. In this case: return a version field with the expected version number.

Maybe my initial assumption was wrong (@dragotin for confirmation/disproof): I assumed that we don't need older client versions to support oCIS. I.e. if we would find a good solution now how to let new client versions across all platforms work with the version being something smaller than 10.x.x (in this case: 2.0.0), that would be fine in my opinion. We anyway have to encourage people to install new clients if they want to use project spaces. Introducing an entirely new version field feels wrong to me. And letting existing client versions fail with oCIS could be intentional, no? Again: project spaces. They anyway need new clients across all platforms.

@phil-davis Yeah, IIRC the initial implementation of the iOS SDK was checking for ownCloud in the productname to ensure it's a supported/compatible server - and that fell apart with a customer's customized server. Since then it does the inverse: check only for the name of known incompatible server software - and connect otherwise.

Good-to-know context, thank you for that! 😇

@TheOneRing
Copy link
Contributor

The new client will not have any version checks determining its behavior.
However we still check during the account setup, for now looking at the product name.
https://github.com/owncloud/client/blob/c4500ba9dc97f397f7c99a2f9dc8aeceadb3ed28/src/libsync/account.cpp#L312-L316

As I expected we would fix the version number issue.
How am I supposed to check for unsupported server versions if the version number is allowed to be anything, in combination with an arbitrary product name?

PS:
Fun fact, Microsoft skipped Windows 9 as there was to much software out there still checking for if version.startswith("9") (95,98)

@micbar
Copy link
Contributor

micbar commented May 19, 2022

@kulmann @fschade That needs changes in web, once #3805 is merged.

@dragotin
Copy link
Contributor Author

@kulmann oh no, your assumption is wrong: Old clients (desktop and mobile) need to run with oCIS. They will not support spaces of course, but should continue to sync the existing sync connects (desktop). That has always been the goal.

And that is the reason why we have to kind of "protect" the version field, because already installed clients out there in the field use that field. And if that drops to 2.0.0 for oCIS they assume that is ownCloud 10 version 2 and refuse to talk to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants