[epic] collaboration service

[jvillafanez]

Check list:

• Initial version (feat: add new collaboration service (WOPI) #8374)
• SSE event triggers
  Checked with OnlyOffice, and it likely happens with any other apps. When the file is saved after editing, the app (OnlyOffice) waits 5 seconds before starting to convert the document and upload it into the storage. This will take some time, specially if the document is big. From the web UI, after the closing the editor, the file will still appear as locked because the app hasn't unlocked the file yet, but there is no "unlock" event that notifies the web UI
• Explicit support of specific apps
  The contents of the CheckFileInfo response varies depending on the target WOPI app (MS, OnlyOffice, Collabora, etc). Right now, the response is basically a superset of the properties of those three, and it's adapted to MS in order for the WOPI validator to pass the tests.
  Some of the information in the CheckFileInfo response is used by the apps to customize the editor's behavior, for example, to disable printing. This isn't homogeneous and it depends on the specific apps.
• Support for proof keys (info in https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/online/scenarios/proofkeys)
• Additional / missing file operations ("putRelativeFile", "deleteFile", "renameFile")
  Unlikely to be implemented: "putUserInfo", "getShareUrls" and "enumerateAncestors"
• Use gateway client pool selector for Kubernetes [full-ci] fix: always select next gateway client #10133
• WOPI Host Urls feat: add wopi host and sharing urls #10174
• Custom request / response headers (list for MS in https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/common-headers)
  Most of them seem to be optional, and probably dependent on the app.
• Clean startup / tear-down of the service
  The service currently crashes if there are dependent services not running. It also seems to have problems with the shutdown process, so right now you need to kill the service.
• Implement ecosystem endpoint (https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/ecosystem/checkecosystem)
• Implement bootstrap endpoint (https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/bootstrapper/bootstrap)

To be investigated:

• Direct access to file contents via fileUrl (https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/checkfileinfo/checkfileinfo-response#fileurl)
  As performance improvement, it might not be worthy for us.
• Additional / MS-only? file operations (list in https://learn.microsoft.com/en-us/openspecs/office_protocols/ms-wopi/f52e753e-fa08-4ba4-a68b-2f8801992cf0)
  The WOPI validator has some tests for the "addActivities" operation. I haven't seen any reference of some of them in OnlyOffice nor Collabora documentation, so it's probably MS-only features.
• Coauth locks (https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/plus/coauthoring/locks-about)
  Likely MS-only, and I'm not sure if reva supports somethings similar

[micbar]

From #8855
the current ocis collaboration service also needs a few changes

• it may have to learn the wopi proxy trick unless we use the wopi proxy secret to sign the wopi JWT
• it currently uses one JWT secret for three things: 1. reva access token signature verification, 2. wopi JWT creation and 3. access token encryption

[jvillafanez]

Refactor plans around the connector package

Originally, the operations returned a string representing the expected lockId (if any) the file had in case of a conflict response. This had been enough because the lockId was the only relevant header we needed to use.
With the introduction of new operations ("putRelativeFile" and "renameFile"), the responses are more complex: they need additional headers as well as a JSON body.

Furthermore, some of the returned errors came with an HTTP status code that we'd need to use. While this isn't a big problem, we need to check and convert that error, so the code complexity increases.

---

There are 2 main goals for the refactor:

• Reduce code complexity (there are several methods with a 15+ cyclomatic complexity)
• Homogenize the connector calls.

Connector calls will return a pointer to a Response and an error, something like GetLock(...) (*Response, error) {....}.
Any "generic" error that could happen inside, such as network errors, invalid parameters, etc, should be forwarded and returned in the error. These "generic" errors will be converted in HTTP 500 in the "httpAdapter".
Valid responses will return a *Response with a nil error.

The response will contain the following:

type Response struct {
  Status int
  Headers map[string]string
  Body map[string]interface{}
}

Both headers and body can be nil if there is nothing to be returned, so only the Status would be somewhat mandatory.

The expected response handling should be similar to the following:

response, err := fc.PutRelativeFile(....)
if err != nil {
  http.Error(w, http,StatusText(500), 500)
  return
}

jsonBody, err := json.Marshal(response.Body)
if err != nil {
  http.Error(w, http,StatusText(500), 500)
  return
}

for key, value := range response.Headers {
  w.Header.Set(key, value)
}
w.WriteHeader(response.Status)
w.Write(jsonBody)

That should reduce the complexity from around 8 to 2 (manually counting the complexity of the current similar code piece).
It takes advantage of 2 things: if nil is returned as headers, the headers loop will be skipped; and we have some knowledge about the method returning a meaningful body or not, so for most of the cases if not all, we can skip the nil check on the response.Body

---

Additional things to consider:

• Error handling inside the connector is dense.
  Performing any operation usually consist in at least 3 checks: check for error in the operation, check for the response status of the operation and check for the actual result. The problem comes when, in case of error, we need to perform an additional operation (such as get the lock in case of conflict)
• Some operations involve a name generation. This shouldn't be a big issue because the complexity comes from the error handling. Following the refactor plans should help with this.

[jvillafanez]

Refactor plans around the connector package (part 2)

For error handling, I think the best option is to just split the code into smaller pieces and move the pieces to custom private functions. For example, the UnLock method in the fileConnector needs to get the current lock if the unlock operation fails with a "locked" error. This is currently done inline, so it's adding extra complexity. Just moving this handling to a new function would reduce the complexity.

Some additional discarded solutions:

• Setting up custom handlers per possible error code in each call against the gateway.
  It could reduce the complexity a lot, but setting it up would also reduce the readability of the code. In addition, we'd need custom handlers for any operation, and the handlers might become quite complex by themselves. It would be extra messy if we consider nested handlers, that would be needed in some situations.
  Overall, it doesn't seem to be a net positive outcome.
• Wrapping up gateway calls in order to simplify error handling.
  We might do something with this if we could reuse the wrapping function, but the type of the parameters and the response from the gateway calls are different. This means we'd likely need one wrapping method per each gateway call we use.
  Handling the error cases in the wrapping method is likely a challenge. Despite the gateway call to be the same, each caller might need a different error handling: some might return right away, but others might need to call additional methods (in order to retrieve locks, for example).
  In the end, it seems too much effort for very little gain.

[micbar]

@jvillafanez Adding a new feature, like already discussed.

Access Logging

Because of the nature of the Collaboration server, that it bypasses the ocis-proxy service, we are currently "not in the loop" what happens on the endpoints of the collaboration service.

From our current experience with a large SaaS Service, a proper access log is vital to provide support to the end users.

Requirements

• When we set COLLABORATION_LOG_LEVEL=info, the collaboration service logs all http requests
• It would be perfect if we could re-use this middleware https://github.com/owncloud/ocis/blob/master/services/proxy/pkg/middleware/accesslog.go It logs exactly the params that we need.
• The access log always logs a RequestID and a traceID, which makes later debugging easier, because the traceID is forwarded across service boundaries

[hodyroff]

Hope this is the right place, when I use "Files" and then rename ... there is no way to do that as of course the file is locked at this time by WOPI.
@tbsbdr UX wise - enable this or eliminate the button and its ability? If that is possible via customizing? Or make this a WOPI function and allow it?
In this context I am getting after some clicks here

a wrong alignment and a weird long path I have no idea why thats presented to the user ...

Doing two failed renames ... error message = check your permissions = not good as I am editing it at this time = locked.
I get this

Does not revocer.
Setup used is https://ocis.aaitest.owncloud.works/ as we have COOL 24 there @nicholas-wilson-au anything in the logs?

[hodyroff]

I am missing the "save as" function in collabora @jvillafanez is this by purpose not enabled yet?
Expectation would be that a second file in the same folder with a different name (prompt for name) would be created and a second tab with that content would be given to me - provided I have write access to the folder and the file.

[hodyroff]

#8769 (comment)
Looks like this is part of an open item for @jvillafanez above = rename, save as etc. will work when extented WOPI attributes/functions might be in the next roling ...

[micbar]

Correct. We are already working on it.

[jvillafanez]

"putRelativeFile", "deleteFile", "renameFile" operations have been merged, so they'll be available for the next rolling release.

Couple of things to know:

• The "deleteFile" operation was needed for the WOPI validator. As far as I know, neither Collabora nor OnlyOffice deletes files (don't expect a "deleteFile" button anywhere), and it's unclear whether office365 has something like that.
• Filenames used for the "putRelativeFile" and "renameFile" operation are usually "suggested". This means that the host can use a different name.
  • The collaboration service will try to use the proposed filename if possible. If a file already exists with same name or there is a conflict, a "not-so-random" prefix will be used (it's time-based so a second name collision shouldn't happen). It should be easy to spot and it can be manually renamed afterwards.
• "putUserInfo", "getShareUrls" and "enumerateAncestors" operations are NOT planned to be implemented. These seems to be MS-only operations, and we might also need support for them at storage level (need some research)

[jvillafanez]

Quick refactor plans for the OpenInApp implementation in the collaboration service

This is mostly moving things around and simplifying the code. 3 main points, which should be easy to do:

• Logs are dense, having the same 3 parameters being added. A custom logger, with those same parameters added at the beginning of the function will compact the code to a one log line in most of the cases.
• Choosing the final app URL that will be sent is chaotic at the moment. Moving this to a new function should simplify the openInApp function and might also clarify why that specific URL is chosen.
  I think the code related to this has a cyclomatic complexity of around 15, which is very high (sonarcloud might complain), so moving the code to another function seems right. Nevertheless, we'll also need to keep the complexity in check inside the new function.
• Query parameters in the final app URL. This is currently weird because we're building the final app URL (with the query parameters) for 2 URLs and then choose one of them, wasting time.
  This can also be moved out to another function.