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.

Sign up for GitHub

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

Jump to bottom

Add note to dav endpoint #12978

Merged
merged 2 commits into from
Mar 4, 2019
Merged

Add note to dav endpoint #12978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78e6bf0
Add note to dav endpoint
tobiasKaminsky Dec 5, 2018
aee348c
Update apps/dav/lib/Connector/Sabre/Node.php
ChristophWurst Mar 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions apps/dav/lib/Connector/Sabre/FilesPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class FilesPlugin extends ServerPlugin {
const HAS_PREVIEW_PROPERTYNAME = '{http://nextcloud.org/ns}has-preview';
const MOUNT_TYPE_PROPERTYNAME = '{http://nextcloud.org/ns}mount-type';
const IS_ENCRYPTED_PROPERTYNAME = '{http://nextcloud.org/ns}is-encrypted';
const SHARE_NOTE = '{http://nextcloud.org/ns}note';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also share-note maybe... as just note is a bit generic?


/**
* Reference to main server object
Expand Down Expand Up @@ -161,6 +162,7 @@ public function initialize(\Sabre\DAV\Server $server) {
$server->protectedProperties[] = self::HAS_PREVIEW_PROPERTYNAME;
$server->protectedProperties[] = self::MOUNT_TYPE_PROPERTYNAME;
$server->protectedProperties[] = self::IS_ENCRYPTED_PROPERTYNAME;
$server->protectedProperties[] = self::SHARE_NOTE;

// normally these cannot be changed (RFC4918), but we want them modifiable through PROPPATCH
$allowedProperties = ['{DAV:}getetag'];
Expand Down Expand Up @@ -359,6 +361,12 @@ public function handleGetProperties(PropFind $propFind, \Sabre\DAV\INode $node)
$propFind->handle(self::MOUNT_TYPE_PROPERTYNAME, function () use ($node) {
return $node->getFileInfo()->getMountPoint()->getMountType();
});

$propFind->handle(self::SHARE_NOTE, function() use ($node, $httpRequest) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comments as to #14429

I think it would make sense to move this to the share plugin we have.
Because in it current state. If you have N entries in the folder you propfind. This will fire off N*4 queries. This won't scale.

return $node->getNoteFromShare(
$httpRequest->getRawServerValue('PHP_AUTH_USER')
);
});
}

if ($node instanceof \OCA\DAV\Connector\Sabre\Node) {
Expand Down
31 changes: 31 additions & 0 deletions apps/dav/lib/Connector/Sabre/Node.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
use OCP\Files\StorageNotAvailableException;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IManager;
use OCP\Share;
use OCP\Share\IShare;


abstract class Node implements \Sabre\DAV\INode {
Expand Down Expand Up @@ -290,6 +292,35 @@ public function getSharePermissions($user) {
return $permissions;
}

/**
* @param string $user
* @return string
*/
public function getNoteFromShare($user) {
ChristophWurst marked this conversation as resolved.
Show resolved Hide resolved
if ($user === null) {
return '';
}

$types = [
Share::SHARE_TYPE_USER,
Share::SHARE_TYPE_GROUP,
Share::SHARE_TYPE_CIRCLE,
Share::SHARE_TYPE_ROOM
];

foreach ($types as $shareType) {
$shares = $this->shareManager->getSharedWith($user, $shareType, $this, -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt this works... The third argument expects a node for sure... but this is a different note that the sharing one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so it works because the actual function e use int he share amanger is also present. But this will do 💥 at some point.

Either we revert this or we fix it. Because this is not really something we can leave around and have do 💥 when we move to more strict typing. Especially since this isn't covered by any tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can fix it by putting an instanceof check against $this beforehand, in line 300. Would be the easiest thing to do, but perhaps not cleanest one.

Alternative would be to retrieve always correct Node instance, if possible. There's no straight way from View or FileInfo, and I did not look deeper by now.

foreach ($shares as $share) {
$note = $share->getNote();
if($share->getShareOwner() !== $user && !empty($note)) {
return $note;
}
}
}

return '';
}

/**
* @return string
*/
Expand Down