Lock information coming from collaboration service is shown empty

jvillafanez commented 4 months ago

Describe the bug

The lock information shown for the file details is empty if the file is locked with the new collaboration service

Steps to reproduce

Setup oCIS with the collaboration service
Setup a space so userA and userB can access to the same office file (.docx for example)
UserA opens the file with the collaboration service (using OnlyOffice or Collabora)
UserB see the file is locked and opens the details

Expected behavior

The details should show a "Locked by" with some information

Actual behavior

The "Locked by" is empty

Screenshot from 2024-06-07 09-50-15

Setup

Please describe how you started the server and provide a list of relevant environment variables or configuration files.

```console OCIS_XXX=somevalue OCIS_YYY=somevalue PROXY_XXX=somevalue ```

Additional context

The locked file is the "Untitled 3.docx" file. Propfind below.

<?xml version="1.0"?>
<d:multistatus xmlns:s="http://sabredav.org/ns" xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
  <d:response>
    <d:href>/remote.php/dav/spaces/1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43/</d:href>
    <d:propstat>
      <d:prop>
        <oc:permissions>RDNVCKZP</oc:permissions>
        <oc:favorite>0</oc:favorite>
        <oc:fileid>1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43!18dfc958-1f23-41bc-987e-0643b69dfc43</oc:fileid>
        <oc:file-parent>1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43</oc:file-parent>
        <oc:name>bananana</oc:name>
        <oc:owner-id>admin</oc:owner-id>
        <oc:owner-display-name>Admin</oc:owner-display-name>
        <oc:privatelink>https://ocis.jp.solidgear.prv/f/1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43%2118dfc958-1f23-41bc-987e-0643b69dfc43</oc:privatelink>
        <oc:size>7330</oc:size>
        <d:getlastmodified>Wed, 05 Jun 2024 13:41:44 GMT</d:getlastmodified>
        <d:getetag>"3b8d646497d419d654e887647f4a51e6"</d:getetag>
        <d:resourcetype>
          <d:collection/>
        </d:resourcetype>
        <oc:tags/>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
    <d:propstat>
      <d:prop>
        <d:lockdiscovery/>
        <d:activelock/>
        <oc:shareroot/>
        <oc:share-types/>
        <d:getcontentlength/>
        <d:getcontenttype/>
        <oc:downloadURL/>
        <oc:audio/>
        <oc:location/>
      </d:prop>
      <d:status>HTTP/1.1 404 Not Found</d:status>
    </d:propstat>
  </d:response>
  <d:response>
    <d:href>/remote.php/dav/spaces/1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43/.space/</d:href>
    <d:propstat>
      <d:prop>
        <oc:permissions>RDNVCKZP</oc:permissions>
        <oc:favorite>0</oc:favorite>
        <oc:fileid>1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43!300cdccb-2681-4f5d-aa7b-58c0c9b2a4fc</oc:fileid>
        <oc:file-parent>1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43!18dfc958-1f23-41bc-987e-0643b69dfc43</oc:file-parent>
        <oc:name>.space</oc:name>
        <oc:owner-id>admin</oc:owner-id>
        <oc:owner-display-name>Admin</oc:owner-display-name>
        <oc:privatelink>https://ocis.jp.solidgear.prv/f/1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43%21300cdccb-2681-4f5d-aa7b-58c0c9b2a4fc</oc:privatelink>
        <oc:size>46</oc:size>
        <d:getlastmodified>Wed, 05 Jun 2024 13:41:17 GMT</d:getlastmodified>
        <d:getetag>"f1084d476827788aeadb94cf0df5544c"</d:getetag>
        <d:resourcetype>
          <d:collection/>
        </d:resourcetype>
        <oc:tags/>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
    <d:propstat>
      <d:prop>
        <d:lockdiscovery/>
        <d:activelock/>
        <oc:shareroot/>
        <oc:share-types/>
        <d:getcontentlength/>
        <d:getcontenttype/>
        <oc:downloadURL/>
        <oc:audio/>
        <oc:location/>
      </d:prop>
      <d:status>HTTP/1.1 404 Not Found</d:status>
    </d:propstat>
  </d:response>
  <d:response>
    <d:href>/remote.php/dav/spaces/1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43/Untitled%203.docx</d:href>
    <d:propstat>
      <d:prop>
        <oc:permissions>RDNVWZP</oc:permissions>
        <oc:favorite>0</oc:favorite>
        <oc:fileid>1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43!c11b8c11-3a56-47b2-8afe-74535ceb63ba</oc:fileid>
        <oc:file-parent>1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43!18dfc958-1f23-41bc-987e-0643b69dfc43</oc:file-parent>
        <oc:name>Untitled 3.docx</oc:name>
        <d:lockdiscovery>
          <d:activelock>
            <d:locktype>
              <d:write/>
            </d:locktype>
            <d:lockscope>
              <d:exclusive/>
            </d:lockscope>
            <d:depth>Infinity</d:depth>
            <d:owner>com.github.owncloud.collaboration.Collabora</d:owner>
            <d:timeout>Second-1747</d:timeout>
            <d:locktoken>
              <d:href>cool-lock02e739bf</d:href>
            </d:locktoken>
          </d:activelock>
        </d:lockdiscovery>
        <oc:owner-id>admin</oc:owner-id>
        <oc:owner-display-name>Admin</oc:owner-display-name>
        <oc:privatelink>https://ocis.jp.solidgear.prv/f/1d238a14-6d02-495d-9f1d-1d134a7473d4$18dfc958-1f23-41bc-987e-0643b69dfc43%21c11b8c11-3a56-47b2-8afe-74535ceb63ba</oc:privatelink>
        <d:getcontentlength>7284</d:getcontentlength>
        <oc:size>7284</oc:size>
        <d:getlastmodified>Fri, 17 Mar 2023 09:54:01 GMT</d:getlastmodified>
        <d:getetag>"87d4e8bd8d3b52005e400523c625a2b3"</d:getetag>
        <d:getcontenttype>application/vnd.openxmlformats-officedocument.wordprocessingml.document</d:getcontenttype>
        <d:resourcetype/>
        <oc:tags/>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
    <d:propstat>
      <d:prop>
        <d:activelock/>
        <oc:shareroot/>
        <oc:share-types/>
        <oc:downloadURL/>
        <oc:audio/>
        <oc:location/>
      </d:prop>
      <d:status>HTTP/1.1 404 Not Found</d:status>
    </d:propstat>
  </d:response>
</d:multistatus>

I'd expect the <d:owner>com.github.owncloud.collaboration.Collabora</d:owner> info to be shown as "locked by".

As far as I've seen in the code, the oc:ownername property is used instead, which is missing in the propfind. The property comes from opaque information (https://github.com/cs3org/reva/blob/edge/internal/http/services/owncloud/ocdav/propfind/propfind.go#L1767-L1771), so I'm not sure if we should use that because it seems to be considered as optional information that might not be present.

kulmann commented 4 months ago

Good point 🤔 For the lock information shown in the details panel I would actually like to have both. owner for the application owning the lock, and if present also the ownername (as in the username).

@jvillafanez could you please make sure that the ownername is being added to the propfind response as well? We can make it show only if present, that's not an issue. For the application name, we'll fix that. But could you provide a human readable display name of the application in that field? Because I don't want to show com.github.owncloud.collaboration.Collabora in the UI. I want to show Collabora instead.

cc @tbsbdr

jvillafanez commented 4 months ago

Maybe @micbar can double-check what information I should provide...

As far as I know, the lock is hold by the app (Collabora in this case). Including a user would show something like userA@idp via com.github.owncloud.collaboration.Collabora, but if userB is also editing the file and then userA leaves, then having that lock information might be confusing because it seems userA is holding the lock although he isn't editing. In addition, I'm not sure how it will behave because userB might attempt to refresh the lock (which I'm not sure if it will cause issues).

For the ownername, as said, the information comes from the opaque. There are several things to consider:

If it's opaque information, I doubt an external service should use or modify it. It could be fine if I would be both the producer and the consumer of that information, but it isn't the case.
The information hold inside the opaque is undocumented (probably intended).
Since the information that I write there will be used by a different service, I need to know how that service is going to access and use that information. This means that there will be an implicit (and undocumented) dependency between the services. In particular, if reva decides to change the key used now, we'll have a bug that we won't notice.

This is why I'm not sure if it's a good idea to use the ownername. I think we'd be relying on the code not changing (at least that piece), but there is nothing preventing it.

For the application name, we'll fix that. But could you provide a human readable display name of the application in that field? Because I don't want to show com.github.owncloud.collaboration.Collabora in the UI. I want to show Collabora instead.

This is also something to discuss. Right now, the appname comes from the configured COLLABORATION_APP_LOCKNAME, which is com.github.owncloud.collaboration by default. Since people won't likely change the variable, I appended the COLLABORATION_APP_NAME so we could have several WOPI apps deployed at the same time.

I think we can remove the COLLABORATION_APP_LOCKNAME from the configuration, and just use the COLLABORATION_APP_NAME also for the locks. That would show the name we want and also allow parallel deployment of WOPI apps.

kulmann commented 4 months ago

Thanks for the context, that's really helpful! I agree, showing the ownername (as in: username) is a bad idea then. So let's not do that.

For the owner prop: What about keeping it like is and instead writing the COLLABORATION_APP_NAME into a new field to be consumed by clients for displaying the info? Propfind field something like owner-displayname or alike?

micbar commented 4 months ago

Please let us check that we are in line with the rfc. The locking info is part of the WebDAV RFC where we should be compatible.

owncloud / web