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

Expose share owner id and display name via files webdav #20188

Merged
merged 2 commits into from
Nov 17, 2015

Conversation

PVince81
Copy link
Contributor

  • Required by the Webdav files UI: we need the share owner to be able to display it in the UI. It was formerly available in the "ajax/list.php" response.
  • This PR brings this value to the files' Webdav response, when requested explicitly.
  • Whether a folder is an incoming share is already available in the "S" permission (oc:permissions)
  • This does not cause a performance penalty because the share info has already been resolved at the time the mount point was created. So we're basically reading information that is already available.

To test:

  1. Create two users "root" and "user1"
  2. Login as "user1"
  3. Share a folder "test" with "root"
  4. Save the following into "propfind.txt"
<?xml version="1.0"?>
<a:propfind xmlns:a="DAV:" xmlns:oc="http://owncloud.org/ns">
        <a:prop><oc:permissions/></a:prop>
        <a:prop><oc:owner-id/></a:prop>
        <a:prop><oc:owner-display-name/></a:prop>
</a:propfind>
  1. curl -X PROPFIND -H "Content-Type: text/xml" --data-binary "@propfind.txt" http://root:admin@localhost/owncloud/remote.php/webdav/test | xmllint --format -

This PR will output two additional properties, when requested as above:

<d:response>
    <d:href>/owncloud/remote.php/webdav/test/</d:href>
    <d:propstat>
      <d:prop>
        <oc:owner-id>user1</oc:share-owner-id>
        <oc:owner-display-name>User One</oc:share-owner-display-name>
        <oc:permissions>SRDNVCK</oc:permissions>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>

Edit: now renamed to "owner-id" and "owner-display-name"

Open questions:

  • Should we rename "share owner" to simply "owner" ? If yes:
    • In ownCloud's terms, the owner of a file is always either the current user or the sharer.
    • Note that is we rename it to "owner" we'll need to return the current user.
    • It is still possible to detect if this is a shared mount by checking the "S" permission.
    • We'd then remove SharesPlugin and move the attribute to FilesPlugin, as it is more generic
  • @icewind1991 do you think we should cache the getOwner() for better performance ? The code path looks like it goes up to the SharedStorage which has the share cached
  • TODO: add unit tests once everything is clarified

Let's discuss @DeepDiver1975 @icewind1991 @schiesbn @rullzer @nickvergessen

@PVince81 PVince81 changed the title [WIP] Expose share owner is and display name via files webdav [WIP] Expose share owner id and display name via files webdav Oct 30, 2015
@PVince81 PVince81 added this to the 9.0-current milestone Oct 30, 2015
@PVince81
Copy link
Contributor Author

I realized that this PR already returns the current user "root" when doing a PROPFIND on a non-shared item. If we really want this only for shares, then the property would need to be adjusted.

I think I'm more in favor of having a more generic "owner-id" and "owner-display-name" attribute.
Thinking further, I wonder how this would fit in with principals in the future. But we need a short term solution.

@PVince81 PVince81 self-assigned this Oct 30, 2015
@PVince81
Copy link
Contributor Author

PVince81 commented Nov 2, 2015

  • TODO: make sure to return 403 if someone tries to PROPPATCH these new properties

@icewind1991
Copy link
Contributor

I agree with moving to owner-id

@icewind1991
Copy link
Contributor

@icewind1991 do you think we should cache the getOwner() for better performance ? The code path looks like it goes up to the SharedStorage which has the share cached

Might be good to add getOwner to the FileInfo which could then be used to cache it

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 2, 2015

@icewind1991 I added getOwner to the FileInfo interface and also Node API, please check my implementation: f563648

And I renamed the properties to "owner-id" and "owner-display-name".

Please review

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 2, 2015

Regarding getOwner on the interface: I checked that the Common storage, SharedStorage and all the external storages have that method implemented. External storages all directly or indirectly extend Common.

@PVince81 PVince81 changed the title [WIP] Expose share owner id and display name via files webdav Expose share owner id and display name via files webdav Nov 2, 2015
@icewind1991
Copy link
Contributor

@icewind1991 I added getOwner to the FileInfo interface and also Node API, please check my implementation: f563648

Since we also want the display name it makes more sense imo to return a user object.

See #20224

@rullzer
Copy link
Contributor

rullzer commented Nov 12, 2015

@PVince81 rebase now that #20224 is in.

@PVince81
Copy link
Contributor Author

I'll fix it to use the "new" API, as we're now getting a user object.

@PVince81
Copy link
Contributor Author

I have updated the API usage and the unit tests to reflect the fact that getOwner() now returns user objects.

Please review @icewind1991 @rullzer @DeepDiver1975 @blizzz @schiesbn

@icewind1991
Copy link
Contributor

👍 looks good

@PVince81
Copy link
Contributor Author

CC @dragotin @guruz @rperezb in case desktop and mobile clients are interested in this property too.
Note that the original reason I added it is because the web UI will need it when using Webdav.

@DeepDiver1975
Copy link
Member

👍

DeepDiver1975 added a commit that referenced this pull request Nov 17, 2015
Expose share owner id and display name via files webdav
@DeepDiver1975 DeepDiver1975 merged commit 20c251a into master Nov 17, 2015
@DeepDiver1975 DeepDiver1975 deleted the webdav-exposeshareowner branch November 17, 2015 12:40
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants