web icon indicating copy to clipboard operation
web copied to clipboard
verified publisher icon owncloud

Lock information coming from collaboration service is shown empty

Open jvillafanez opened this issue 1 year ago • 4 comments

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

  1. Setup oCIS with the collaboration service
  2. Setup a space so userA and userB can access to the same office file (.docx for example)
  3. UserA opens the file with the collaboration service (using OnlyOffice or Collabora)
  4. 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.

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.

jvillafanez avatar Jun 07 '24 08:06 jvillafanez

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

kulmann avatar Jun 11 '24 07:06 kulmann

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.

jvillafanez avatar Jun 11 '24 09:06 jvillafanez

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?

kulmann avatar Jun 11 '24 09:06 kulmann

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.

micbar avatar Jun 11 '24 10:06 micbar

@jvillafanez just checked this again together with @JammingBen and @AlexAndBear and we wanted to do the following:

  1. use the lock owner from the propfind response in d:lockdiscovery.d:activelock.d:owner
  2. look up the app (by the id provided in lock-owner field) in the app providers (/app/list response, which I extract as a unique set from all the mime types in the respective app_providers array)
  3. Show the app provider name field as the human readable lock owner.

I have the following issue now: the fields in 1. and 2. don't match. For OnlyOffice the values are:

  1. com.github.owncloud.collaboration.OnlyOffice (lock info in propfind response)
  2. com.owncloud.api.collaboration.OnlyOffice (app provider)

Is there any chance that we can have these 2 matching? I.e. use the com.owncloud.api.collaboration.OnlyOffice app provider id in the lockdiscovery, so that I can do a reverse-lookup of the app provider name in web?

kulmann avatar Oct 30 '24 10:10 kulmann

We could change the default value in https://github.com/owncloud/ocis/blob/master/services/collaboration/pkg/config/defaults/defaultconfig.go#L29 in order to match, but I'm not sure if it makes sense. The additional problem is that the values are configurable, so the admin can change them.

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.

Maybe we should try this and check if it's good enough. In this case, the lock info would be just OnlyOffice and not com.github.owncloud.collaboration.OnlyOffice.

jvillafanez avatar Oct 31 '24 08:10 jvillafanez

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.

Maybe we should try this and check if it's good enough. In this case, the lock info would be just OnlyOffice and not com.github.owncloud.collaboration.OnlyOffice.

That would be nice, thank you!

kulmann avatar Oct 31 '24 08:10 kulmann

https://github.com/owncloud/ocis/pull/10447 should do for the server side. There is a little detail regarding what to do with the corresponding config option, but the code should work.

jvillafanez avatar Oct 31 '24 10:10 jvillafanez