diff --git a/apps/dav/appinfo/Migrations/Version20200427142541.php b/apps/dav/appinfo/Migrations/Version20200427142541.php new file mode 100644 index 000000000000..2b5162d018fd --- /dev/null +++ b/apps/dav/appinfo/Migrations/Version20200427142541.php @@ -0,0 +1,56 @@ + + * + * @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 + * + */ + +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, + ] + ); + } + } + } +} diff --git a/apps/dav/appinfo/info.xml b/apps/dav/appinfo/info.xml index 125ff50f72bb..f722a3c732c3 100644 --- a/apps/dav/appinfo/info.xml +++ b/apps/dav/appinfo/info.xml @@ -5,7 +5,7 @@ ownCloud WebDAV endpoint AGPL owncloud.org - 0.5.0 + 0.6.0 true diff --git a/apps/dav/lib/DAV/AbstractCustomPropertiesBackend.php b/apps/dav/lib/DAV/AbstractCustomPropertiesBackend.php index eeceaedcd6fc..c65e2b045a2c 100644 --- a/apps/dav/lib/DAV/AbstractCustomPropertiesBackend.php +++ b/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); + } + } } diff --git a/apps/dav/lib/DAV/FileCustomPropertiesBackend.php b/apps/dav/lib/DAV/FileCustomPropertiesBackend.php index ab62e5a3f7d3..bd865b8b6ad4 100644 --- a/apps/dav/lib/DAV/FileCustomPropertiesBackend.php +++ b/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,7 +176,8 @@ 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 @@ -187,24 +185,23 @@ protected function updateProperties($path, INode $node, $changedProperties) { ); } } 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(); diff --git a/apps/dav/lib/DAV/MiscCustomPropertiesBackend.php b/apps/dav/lib/DAV/MiscCustomPropertiesBackend.php index e0706f84c203..f210fd50eab5 100644 --- a/apps/dav/lib/DAV/MiscCustomPropertiesBackend.php +++ b/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,7 +118,8 @@ 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 @@ -129,18 +127,23 @@ protected function updateProperties($path, INode $node, $changedProperties) { ); } } 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 ] diff --git a/apps/dav/tests/unit/DAV/FileCustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/FileCustomPropertiesBackendTest.php index 5ee8bd6e4f4e..c3ef2c213294 100644 --- a/apps/dav/tests/unit/DAV/FileCustomPropertiesBackendTest.php +++ b/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('') ]); $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('', $complexProp->getXml()); $this->assertEquals(['unsetprop'], $propFind->get404Properties()); } diff --git a/changelog/unreleased/37314 b/changelog/unreleased/37314 new file mode 100644 index 000000000000..71c05b41b2d4 --- /dev/null +++ b/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 diff --git a/tests/Core/Command/Apps/AppsListTest.php b/tests/Core/Command/Apps/AppsListTest.php index 2d7b364088f2..72b93a36dfad 100644 --- a/tests/Core/Command/Apps/AppsListTest.php +++ b/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'] ]; } } diff --git a/tests/acceptance/features/apiWebdavProperties/setFileProperties.feature b/tests/acceptance/features/apiWebdavProperties/setFileProperties.feature index 1219a905deaf..29ebfac0d1e0 100644 --- a/tests/acceptance/features/apiWebdavProperties/setFileProperties.feature +++ b/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 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 "" 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 "" + Then the response should contain a custom "very-custom-prop" property with namespace "x1='http://whatever.org/ns'" and complex value "" Examples: | dav_version | | old | diff --git a/tests/acceptance/features/bootstrap/WebDavPropertiesContext.php b/tests/acceptance/features/bootstrap/WebDavPropertiesContext.php index 004c2c6eb148..2123869cf4ed 100644 --- a/tests/acceptance/features/bootstrap/WebDavPropertiesContext.php +++ b/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 *