Complex webdav properties support

apps/dav/appinfo/Migrations/Version20200427142541.php

@@ -0,0 +1,56 @@
+<?php
+/**
+ * @author Viktar Dubiniuk <[email protected]>
+ *
+ * @copyright Copyright (c) 2020, ownCloud GmbH
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCA\DAV\Migrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\DBAL\Types\Type;
+use OCA\DAV\DAV\AbstractCustomPropertiesBackend;
+use OCP\Migration\ISchemaMigration;
+
+/*
+ * Adds a new property type column to properties and dav_properties tables
+ */
+class Version20200427142541 implements ISchemaMigration {
+
+	/**
+	 * @param Schema $schema
+	 * @param array $options
+	 */
+	public function changeSchema(Schema $schema, array $options) {
+		$prefix = $options['tablePrefix'];
+		$tableNames = ['properties', 'dav_properties'];
+
+		foreach ($tableNames as $tableName) {
+			$table = $schema->getTable("{$prefix}{$tableName}");
+			if ($table->hasColumn('propertytype') === false) {
+				$table->addColumn(
+					'propertytype',
+					Type::SMALLINT,
+					[
+						'notnull' => true,
+						'default' => AbstractCustomPropertiesBackend::VT_STRING,
+					]
+				);
+			}
+		}
+	}
+}

apps/dav/appinfo/info.xml

@@ -5,7 +5,7 @@
 	<description>ownCloud WebDAV endpoint</description>
 	<licence>AGPL</licence>
 	<author>owncloud.org</author>
-	<version>0.5.0</version>
+	<version>0.6.0</version>
 	<default_enable/>
 	<use-migrations>true</use-migrations>
 	<types>

apps/dav/lib/DAV/AbstractCustomPropertiesBackend.php

@@ -32,8 +32,23 @@
 use Sabre\DAV\PropFind;
 use Sabre\DAV\PropPatch;
 use Sabre\DAV\Tree;
+use Sabre\DAV\Xml\Property\Complex;
 
 abstract class AbstractCustomPropertiesBackend implements BackendInterface {
+	/**
+	 * Value is stored as string.
+	 */
+	const VT_STRING = 1;
+
+	/**
+	 * Value is stored as XML fragment.
+	 */
+	const VT_XML = 2;
+
+	/**
+	 * Value is stored as a property object.
+	 */
+	const VT_OBJECT = 3;
 
 	/**
 	 * Ignored properties
@@ -230,7 +245,7 @@ protected function fetchProperties($sql, $whereValues, $whereTypes) {
 
 		$props = [];
 		while ($row = $result->fetch()) {
-			$props[$row['propertyname']] = $row['propertyvalue'];
+			$props[$row['propertyname']] = $this->decodeValue($row['propertyvalue'], (int) $row['propertytype']);
 		}
 
 		$result->closeCursor();
@@ -264,4 +279,39 @@ protected function getNodeForPath($path) {
 		}
 		return null;
 	}
+
+	/**
+	 * @param mixed|Complex $value
+	 * @return array
+	 */
+	protected function encodeValue($value) {
+		if (\is_scalar($value)) {
+			$valueType = self::VT_STRING;
+		} elseif ($value instanceof Complex) {
+			$valueType = self::VT_XML;
+			$value = $value->getXml();
+		} else {
+			$valueType = self::VT_OBJECT;
+			$value = \serialize($value);
+		}
+		return [
+			'value' => $value,
+			'type' => $valueType
+		];
+	}
+
+	/**
+	 * @param string $value
+	 * @param int $valueType
+	 * @return mixed|Complex
+	 */
+	protected function decodeValue($value, $valueType) {
+		if ($valueType === self::VT_STRING) {
+			return $value;
+		} elseif ($valueType === self::VT_XML) {
+			return new Complex($value);
+		} elseif ($valueType === self::VT_OBJECT) {
+			return \unserialize($value);
+		}
+	}
 }

apps/dav/lib/DAV/FileCustomPropertiesBackend.php

@@ -43,9 +43,9 @@
 class FileCustomPropertiesBackend extends AbstractCustomPropertiesBackend {
 	public const SELECT_BY_ID_STMT = 'SELECT * FROM `*PREFIX*properties` WHERE `fileid` = ?';
 	public const INSERT_BY_ID_STMT = 'INSERT INTO `*PREFIX*properties`'
-	. ' (`fileid`,`propertyname`,`propertyvalue`) VALUES(?,?,?)';
+	. ' (`fileid`,`propertyname`,`propertyvalue`, `propertytype`) VALUES(?,?,?,?)';
 	public const UPDATE_BY_ID_AND_NAME_STMT = 'UPDATE `*PREFIX*properties`'
-	. ' SET `propertyvalue` = ? WHERE `fileid` = ? AND `propertyname` = ?';
+	. ' SET `propertyvalue` = ?, `propertytype` = ? WHERE `fileid` = ? AND `propertyname` = ?';
 	public const DELETE_BY_ID_STMT = 'DELETE FROM `*PREFIX*properties` WHERE `fileid` = ?';
 	public const DELETE_BY_ID_AND_NAME_STMT = 'DELETE FROM `*PREFIX*properties`'
 	. ' WHERE `fileid` = ? AND `propertyname` = ?';
@@ -168,9 +168,6 @@ protected function updateProperties($path, INode $node, $changedProperties) {
 		$existingProperties = $this->getProperties($path, $node, []);
 		'@phan-var \OCA\DAV\Connector\Sabre\Node $node';
 		$fileId = $node->getId();
-		$deleteStatement = self::DELETE_BY_ID_AND_NAME_STMT;
-		$insertStatement = self::INSERT_BY_ID_STMT;
-		$updateStatement = self::UPDATE_BY_ID_AND_NAME_STMT;
 
 		// TODO: use "insert or update" strategy ?
 		$this->connection->beginTransaction();
@@ -179,32 +176,32 @@ protected function updateProperties($path, INode $node, $changedProperties) {
 			// If it was null, we need to delete the property
 			if ($propertyValue === null) {
 				if ($propertyExists) {
-					$this->connection->executeUpdate($deleteStatement,
+					$this->connection->executeUpdate(
+						self::DELETE_BY_ID_AND_NAME_STMT,
 						[
 							$fileId,
 							$propertyName
 						]
 					);
 				}
 			} else {
-				// FIXME: PHP 7.4 handles object serialization differently so we store 'Object' here
-				// to keep old (wrong) behavior and fix Oracle failure
-				// see https://github.com/owncloud/core/issues/32670
-				if ($propertyValue instanceof Complex) {
-					$propertyValue = 'Object';
-				}
+				$propertyData = $this->encodeValue($propertyValue);
 				if (!$propertyExists) {
-					$this->connection->executeUpdate($insertStatement,
+					$this->connection->executeUpdate(
+						self::INSERT_BY_ID_STMT,
 						[
 							$fileId,
 							$propertyName,
-							$propertyValue
+							$propertyData['value'],
+							$propertyData['type']
 						]
 					);
 				} else {
-					$this->connection->executeUpdate($updateStatement,
+					$this->connection->executeUpdate(
+						self::UPDATE_BY_ID_AND_NAME_STMT,
 						[
-							$propertyValue,
+							$propertyData['value'],
+							$propertyData['type'],
 							$fileId,
 							$propertyName
 						]
@@ -266,7 +263,7 @@ protected function loadChildrenProperties(INode $node, $requestedProperties) {
 			);
 			while ($row = $result->fetch()) {
 				$props = $this->offsetGet($row['fileid']) ?? [];
-				$props[$row['propertyname']] = $row['propertyvalue'];
+				$props[$row['propertyname']] = $this->decodeValue($row['propertyvalue'], (int) $row['propertytype']);
 				$this->offsetSet($row['fileid'], $props);
 			}
 			$result->closeCursor();

apps/dav/lib/DAV/MiscCustomPropertiesBackend.php

@@ -36,11 +36,11 @@
 class MiscCustomPropertiesBackend extends AbstractCustomPropertiesBackend {
 	const SELECT_BY_PATH_STMT = 'SELECT * FROM `*PREFIX*dav_properties` WHERE `propertypath` = ?';
 	const INSERT_BY_PATH_STMT = 'INSERT INTO `*PREFIX*dav_properties`'
-	. ' (`propertypath`, `propertyname`, `propertyvalue`) VALUES(?,?,?)';
+	. ' (`propertypath`, `propertyname`, `propertyvalue`, `propertytype`) VALUES(?,?,?,?)';
 	const UPDATE_BY_PATH_STMT = 'UPDATE `*PREFIX*dav_properties`'
 	. ' SET `propertypath` = ? WHERE `propertypath` = ?';
 	const UPDATE_BY_PATH_AND_NAME_STMT = 'UPDATE `*PREFIX*dav_properties` '
-	. 'SET `propertyvalue` = ? WHERE `propertypath` = ? AND `propertyname` = ?';
+	. 'SET `propertyvalue` = ?, `propertytype` = ? WHERE `propertypath` = ? AND `propertyname` = ?';
 	const DELETE_BY_PATH_STMT = 'DELETE FROM `*PREFIX*dav_properties` WHERE `propertypath` = ?';
 	const DELETE_BY_PATH_AND_NAME_STMT = 'DELETE FROM `*PREFIX*dav_properties`'
 	. ' WHERE `propertypath` = ? AND `propertyname` = ?';
@@ -110,9 +110,6 @@ protected function getProperties($path, INode $node, array $requestedProperties)
 	 */
 	protected function updateProperties($path, INode $node, $changedProperties) {
 		$existingProperties = $this->getProperties($path, $node, []);
-		$deleteStatement = self::DELETE_BY_PATH_AND_NAME_STMT;
-		$insertStatement = self::INSERT_BY_PATH_STMT;
-		$updateStatement = self::UPDATE_BY_PATH_AND_NAME_STMT;
 
 		// TODO: use "insert or update" strategy ?
 		$this->connection->beginTransaction();
@@ -121,26 +118,32 @@ protected function updateProperties($path, INode $node, $changedProperties) {
 			// If it was null, we need to delete the property
 			if ($propertyValue === null) {
 				if ($propertyExists) {
-					$this->connection->executeUpdate($deleteStatement,
+					$this->connection->executeUpdate(
+						self::DELETE_BY_PATH_AND_NAME_STMT,
 						[
 							$path,
 							$propertyName
 						]
 					);
 				}
 			} else {
+				$propertyData = $this->encodeValue($propertyValue);
 				if (!$propertyExists) {
-					$this->connection->executeUpdate($insertStatement,
+					$this->connection->executeUpdate(
+						self::INSERT_BY_PATH_STMT,
 						[
 							$path,
 							$propertyName,
-							$propertyValue
+							$propertyData['value'],
+							$propertyData['type']
 						]
 					);
 				} else {
-					$this->connection->executeUpdate($updateStatement,
+					$this->connection->executeUpdate(
+						self::UPDATE_BY_PATH_AND_NAME_STMT,
 						[
-							$propertyValue,
+							$propertyData['value'],
+							$propertyData['type'],
 							$path,
 							$propertyName
 						]

apps/dav/tests/unit/DAV/FileCustomPropertiesBackendTest.php

@@ -36,6 +36,7 @@
 use OCP\Files\Node;
 use Sabre\DAV\PropFind;
 use Sabre\DAV\SimpleCollection;
+use Sabre\DAV\Xml\Property\Complex;
 
 /**
  * Class FileCustomPropertiesBackendTest
@@ -146,6 +147,7 @@ private function applyDefaultProps($path = '/dummypath') {
 		$propPatch = new \Sabre\DAV\PropPatch([
 			'customprop' => 'value1',
 			'customprop2' => 'value2',
+			'customprop3' => new Complex('<foo xmlns="http://bar"/>')
 		]);
 
 		$this->backend->propPatch(
@@ -253,6 +255,7 @@ public function testSetGetPropertiesForFile() {
 			[
 				'customprop',
 				'customprop2',
+				'customprop3',
 				'unsetprop',
 			],
 			0
@@ -265,6 +268,10 @@ public function testSetGetPropertiesForFile() {
 
 		$this->assertEquals('value1', $propFind->get('customprop'));
 		$this->assertEquals('value2', $propFind->get('customprop2'));
+		/** @var Complex $complexProp */
+		$complexProp = $propFind->get('customprop3');
+		$this->assertInstanceOf(Complex::class, $complexProp);
+		$this->assertEquals('<foo xmlns="http://bar"/>', $complexProp->getXml());
 		$this->assertEquals(['unsetprop'], $propFind->get404Properties());
 	}
 

changelog/unreleased/37314

@@ -0,0 +1,8 @@
+Bugfix: Properly store complex Webdav properties
+
+Fixed: setting custom complex DAV property and reading it returned
+just an 'Object' string instead of the original property value.
+
+https://github.com/owncloud/core/pull/37314
+https://github.com/owncloud/core/issues/32670
+https://github.com/owncloud/core/issues/37027

tests/Core/Command/Apps/AppsListTest.php

@@ -58,9 +58,9 @@ public function testCommandInput($input, $expectedOutput) {
 	public function providesAppIds() {
 		return [
 			[[], '- files: 1.5'],
-			[['--shipped' => 'true'], '- dav: 0.5.0'],
+			[['--shipped' => 'true'], '- dav: 0.6.0'],
 			[['--shipped' => 'false'], '- comments:'],
-			[['search-pattern' => 'dav'], '- dav: 0.5.0']
+			[['search-pattern' => 'dav'], '- dav: 0.6.0']
 		];
 	}
 }

tests/acceptance/features/apiWebdavProperties/setFileProperties.feature

@@ -20,14 +20,12 @@ Feature: set file properties
       | old         |
       | new         |
 
-  @issue-32670
   Scenario Outline: Setting custom complex DAV property and reading it
     Given using <dav_version> DAV path
     And user "user0" has uploaded file "filesForUpload/textfile.txt" to "/testcustomprop.txt"
     And user "user0" has set property "very-custom-prop" with namespace "x1='http://whatever.org/ns'" of file "/testcustomprop.txt" to "<foo xmlns='http://bar'/>"
     When user "user0" gets a custom property "very-custom-prop" with namespace "x1='http://whatever.org/ns'" of file "/testcustomprop.txt"
-    Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and value "Object"
-    #Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and value "<foo xmlns='http://bar'/>"
+    Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and complex value "<x2:foo xmlns:x2=\"http://bar\"/>"
     Examples:
       | dav_version |
       | old         |

tests/acceptance/features/bootstrap/WebDavPropertiesContext.php

@@ -347,6 +347,46 @@ public function theResponseShouldContainACustomPropertyWithValue(
 		);
 	}
 
+	/**
+	 * @Then /^the response should contain a custom "([^"]*)" property with namespace "([^"]*)" and complex value "(([^"\\]|\\.)*)"$/
+	 *
+	 * @param string $propertyName
+	 * @param string $namespaceString
+	 * @param string $propertyValue
+	 *
+	 * @return void
+	 * @throws \Exception
+	 */
+	public function theResponseShouldContainACustomPropertyWithComplexValue(
+		$propertyName, $namespaceString, $propertyValue
+	) {
+		// let's unescape quotes first
+		$propertyValue = \str_replace('\"', '"', $propertyValue);
+		$this->featureContext->setResponseXmlObject(
+			HttpRequestHelper::getResponseXml($this->featureContext->getResponse())
+		);
+		$responseXmlObject = $this->featureContext->getResponseXmlObject();
+		//calculate the namespace prefix and namespace
+		$matches = [];
+		\preg_match("/^(.*)='(.*)'$/", $namespaceString, $matches);
+		$nameSpace = $matches[2];
+		$nameSpacePrefix = $matches[1];
+		$responseXmlObject->registerXPathNamespace(
+			$nameSpacePrefix, $nameSpace
+		);
+		$xmlPart = $responseXmlObject->xpath(
+			"//d:prop/" . "$nameSpacePrefix:$propertyName" . "/*"
+		);
+		Assert::assertArrayHasKey(
+			0, $xmlPart, "Cannot find property \"$propertyName\""
+		);
+		Assert::assertEquals(
+			$propertyValue, $xmlPart[0]->asXML(),
+			"\"$propertyName\" has a value \"" .
+			$xmlPart[0]->asXML() . "\" but \"$propertyValue\" expected"
+		);
+	}
+
 	/**
 	 * @Then the single response should contain a property :property with a child property :childProperty
 	 *