From b0aa17b13fe504445a3108e46a56031ca4b73bc6 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 13 Mar 2015 12:29:13 +0100 Subject: [PATCH 1/3] OCS Fixes to allow setting of password without removing additional settings - Added setPassword to share.php - Fixed OCS API call - Added unit tests --- apps/files_sharing/api/local.php | 43 ++---- lib/private/share/share.php | 80 +++++++++++ lib/public/share.php | 14 ++ tests/lib/share/share.php | 230 +++++++++++++++++++++++++++++++ 4 files changed, 333 insertions(+), 34 deletions(-) diff --git a/apps/files_sharing/api/local.php b/apps/files_sharing/api/local.php index 571982d153..1a5edbfd07 100644 --- a/apps/files_sharing/api/local.php +++ b/apps/files_sharing/api/local.php @@ -343,7 +343,7 @@ class Local { if(isset($params['_put']['permissions'])) { return self::updatePermissions($share, $params); } elseif (isset($params['_put']['password'])) { - return self::updatePassword($share, $params); + return self::updatePassword($params['id'], (int)$share['share_type'], $params['_put']['password']); } elseif (isset($params['_put']['publicUpload'])) { return self::updatePublicUpload($share, $params); } elseif (isset($params['_put']['expireDate'])) { @@ -457,47 +457,22 @@ class Local { /** * update password for public link share - * @param array $share information about the share - * @param array $params 'password' + * @param int $shareId + * @param int $shareType + * @param string $password * @return \OC_OCS_Result */ - private static function updatePassword($share, $params) { - - $itemSource = $share['item_source']; - $itemType = $share['item_type']; - - if( (int)$share['share_type'] !== \OCP\Share::SHARE_TYPE_LINK) { + private static function updatePassword($shareId, $shareType, $password) { + if($shareType !== \OCP\Share::SHARE_TYPE_LINK) { return new \OC_OCS_Result(null, 400, "password protection is only supported for public shares"); } - $shareWith = isset($params['_put']['password']) ? $params['_put']['password'] : null; - - if($shareWith === '') { - $shareWith = null; - } - - $items = \OCP\Share::getItemShared($itemType, $itemSource); - - $checkExists = false; - foreach ($items as $item) { - if($item['share_type'] === \OCP\Share::SHARE_TYPE_LINK) { - $checkExists = true; - $permissions = $item['permissions']; - } - } - - if (!$checkExists) { - return new \OC_OCS_Result(null, 404, "share doesn't exists, can't change password"); + if($password === '') { + $password = null; } try { - $result = \OCP\Share::shareItem( - $itemType, - $itemSource, - \OCP\Share::SHARE_TYPE_LINK, - $shareWith, - $permissions - ); + $result = \OCP\Share::setPassword($shareId, $password); } catch (\Exception $e) { return new \OC_OCS_Result(null, 403, $e->getMessage()); } diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 90f3f28f2e..38fd34e976 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -37,6 +37,10 @@ namespace OC\Share; +use OCP\IUserSession; +use OC\DB\Connection; +use OCP\IConfig; + /** * This class provides the ability for apps to share their content between users. * Apps must create a backend class that implements OCP\Share_Backend and register it with this class. @@ -1150,6 +1154,74 @@ class Share extends \OC\Share\Constants { return true; } + /** + * Retrieve the owner of a connection + * + * @param Connection $connection + * @param int $shareId + * @throws \Exception + * @return string uid of share owner + */ + private static function getShareOwner(Connection $connection, $shareId) { + $qb = $connection->createQueryBuilder(); + + $qb->select('`uid_owner`') + ->from('`*PREFIX*share`') + ->where($qb->expr()->eq('`id`', $shareId)); + $result = $qb->execute(); + $result = $result->fetch(); + + if (empty($result)) { + throw new \Exception('Share not found'); + } + + return $result['uid_owner']; + } + + /** + * Set expiration date for a share + * + * @param IUserSession $userSession + * @param Connection $connection + * @param IConfig $config + * @param int $shareId + * @param string $password + * @throws \Exception + * @return boolean + */ + public static function setPassword(IUserSession $userSession, + Connection $connection, + IConfig $config, + $shareId, $password) { + $user = $userSession->getUser(); + if (is_null($user)) { + throw new \Exception("User not logged in"); + } + + $uid = self::getShareOwner($connection, $shareId); + + if ($uid !== $user->getUID()) { + throw new \Exception('Cannot update share of a different user'); + } + + if ($password === '') { + $password = null; + } + + //If passwords are enforced the password can't be null + if (self::enforcePassword($config) && is_null($password)) { + throw new \Exception('Cannot remove password'); + } + + $qb = $connection->createQueryBuilder(); + $qb->update('`*PREFIX*share`') + ->set('`share_with`', is_null($password) ? 'NULL' : $qb->expr()->literal(\OC::$server->getHasher()->hash($password))) + ->where($qb->expr()->eq('`id`', $shareId)); + $qb->execute(); + + return true; + } + /** * Checks whether a share has expired, calls unshareItem() if yes. * @param array $item Share data (usually database row) @@ -2429,4 +2501,12 @@ class Share extends \OC\Share\Constants { return false; } + /** + * @param IConfig $config + * @return bool + */ + public static function enforcePassword(IConfig $config) { + $enforcePassword = $config->getAppValue('core', 'shareapi_enforce_links_password', 'no'); + return ($enforcePassword === "yes") ? true : false; + } } diff --git a/lib/public/share.php b/lib/public/share.php index d27c1825d6..fcdcc6a6d2 100644 --- a/lib/public/share.php +++ b/lib/public/share.php @@ -317,6 +317,20 @@ class Share extends \OC\Share\Constants { return \OC\Share\Share::setExpirationDate($itemType, $itemSource, $date, $shareTime); } + /** + * Set expiration date for a share + * @param int $shareId + * @param string $password + * @return boolean + */ + public static function setPassword($shareId, $password) { + $userSession = \OC::$server->getUserSession(); + $connection = \OC::$server->getDatabaseConnection(); + $config = \OC::$server->getConfig(); + return \OC\Share\Share::setPassword($userSession, $connection, $config, $shareId, $password); + } + + /** * Get the backend class for the specified item type * @param string $itemType diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index f35a0fa8e4..cd79abd205 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -1128,6 +1128,236 @@ class Test_Share extends \Test\TestCase { \OC_Appconfig::deleteKey('core', 'shareapi_expire_after_n_days'); \OC_Appconfig::deleteKey('core', 'shareapi_enforce_expire_date'); } + + /** + * Cannot set password is there is no user + * + * @expectedException Exception + * @expectedExceptionMessage User not logged in + */ + public function testSetPasswordNoUser() { + $userSession = $this->getMockBuilder('\OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + + $connection = $this->getMockBuilder('\OC\DB\Connection') + ->disableOriginalConstructor() + ->getMock(); + + $config = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + + \OC\Share\Share::setPassword($userSession, $connection, $config, 1, 'pass'); + } + + /** + * Test setting a password when everything is fine + */ + public function testSetPassword() { + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $user->method('getUID')->willReturn('user'); + + $userSession = $this->getMockBuilder('\OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + $userSession->method('getUser')->willReturn($user); + + + $ex = $this->getMockBuilder('\Doctrine\DBAL\Query\Expression\ExpressionBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb = $this->getMockBuilder('\Doctrine\DBAL\Query\QueryBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb->method('update')->will($this->returnSelf()); + $qb->method('set')->will($this->returnSelf()); + $qb->method('where')->will($this->returnSelf()); + $qb->method('andWhere')->will($this->returnSelf()); + $qb->method('select')->will($this->returnSelf()); + $qb->method('from')->will($this->returnSelf()); + $qb->method('expr')->willReturn($ex); + + $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement') + ->disableOriginalConstructor() + ->getMock(); + $ret->method('fetch')->willReturn(['uid_owner' => 'user']); + $qb->method('execute')->willReturn($ret); + + + $connection = $this->getMockBuilder('\OC\DB\Connection') + ->disableOriginalConstructor() + ->getMock(); + $connection->method('createQueryBuilder')->willReturn($qb); + + $config = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + + + $res = \OC\Share\Share::setPassword($userSession, $connection, $config, 1, 'pass'); + + $this->assertTrue($res); + } + + /** + * @expectedException Exception + * @expectedExceptionMessage Cannot remove password + * + * Test removing a password when password is enforced + */ + public function testSetPasswordRemove() { + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $user->method('getUID')->willReturn('user'); + + $userSession = $this->getMockBuilder('\OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + $userSession->method('getUser')->willReturn($user); + + + $ex = $this->getMockBuilder('\Doctrine\DBAL\Query\Expression\ExpressionBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb = $this->getMockBuilder('\Doctrine\DBAL\Query\QueryBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb->method('update')->will($this->returnSelf()); + $qb->method('select')->will($this->returnSelf()); + $qb->method('from')->will($this->returnSelf()); + $qb->method('set')->will($this->returnSelf()); + $qb->method('where')->will($this->returnSelf()); + $qb->method('andWhere')->will($this->returnSelf()); + $qb->method('expr')->willReturn($ex); + + $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement') + ->disableOriginalConstructor() + ->getMock(); + $ret->method('fetch')->willReturn(['uid_owner' => 'user']); + $qb->method('execute')->willReturn($ret); + + + $connection = $this->getMockBuilder('\OC\DB\Connection') + ->disableOriginalConstructor() + ->getMock(); + $connection->method('createQueryBuilder')->willReturn($qb); + + $config = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + $config->method('getAppValue')->willReturn('yes'); + + \OC\Share\Share::setPassword($userSession, $connection, $config, 1, ''); + } + + /** + * @expectedException Exception + * @expectedExceptionMessage Share not found + * + * Test modification of invaid share + */ + public function testSetPasswordInvalidShare() { + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $user->method('getUID')->willReturn('user'); + + $userSession = $this->getMockBuilder('\OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + $userSession->method('getUser')->willReturn($user); + + + $ex = $this->getMockBuilder('\Doctrine\DBAL\Query\Expression\ExpressionBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb = $this->getMockBuilder('\Doctrine\DBAL\Query\QueryBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb->method('update')->will($this->returnSelf()); + $qb->method('set')->will($this->returnSelf()); + $qb->method('where')->will($this->returnSelf()); + $qb->method('andWhere')->will($this->returnSelf()); + $qb->method('select')->will($this->returnSelf()); + $qb->method('from')->will($this->returnSelf()); + $qb->method('expr')->willReturn($ex); + + $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement') + ->disableOriginalConstructor() + ->getMock(); + $ret->method('fetch')->willReturn([]); + $qb->method('execute')->willReturn($ret); + + + $connection = $this->getMockBuilder('\OC\DB\Connection') + ->disableOriginalConstructor() + ->getMock(); + $connection->method('createQueryBuilder')->willReturn($qb); + + $config = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + + + \OC\Share\Share::setPassword($userSession, $connection, $config, 1, 'pass'); + } + + /** + * @expectedException Exception + * @expectedExceptionMessage Cannot update share of a different user + * + * Test modification of share of another user + */ + public function testSetPasswordShareOtherUser() { + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $user->method('getUID')->willReturn('user'); + + $userSession = $this->getMockBuilder('\OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + $userSession->method('getUser')->willReturn($user); + + + $ex = $this->getMockBuilder('\Doctrine\DBAL\Query\Expression\ExpressionBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb = $this->getMockBuilder('\Doctrine\DBAL\Query\QueryBuilder') + ->disableOriginalConstructor() + ->getMock(); + $qb->method('update')->will($this->returnSelf()); + $qb->method('set')->will($this->returnSelf()); + $qb->method('where')->will($this->returnSelf()); + $qb->method('andWhere')->will($this->returnSelf()); + $qb->method('select')->will($this->returnSelf()); + $qb->method('from')->will($this->returnSelf()); + $qb->method('expr')->willReturn($ex); + + $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement') + ->disableOriginalConstructor() + ->getMock(); + $ret->method('fetch')->willReturn(['uid_owner' => 'user2']); + $qb->method('execute')->willReturn($ret); + + + $connection = $this->getMockBuilder('\OC\DB\Connection') + ->disableOriginalConstructor() + ->getMock(); + $connection->method('createQueryBuilder')->willReturn($qb); + + $config = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + + + \OC\Share\Share::setPassword($userSession, $connection, $config, 1, 'pass'); + } + } class DummyShareClass extends \OC\Share\Share { From 0bf06d66913df92251ece31a14444e2975a0b5ca Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 30 Mar 2015 20:07:12 +0200 Subject: [PATCH 2/3] No injections --- lib/private/share/share.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 38fd34e976..d254b5a665 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -1167,7 +1167,8 @@ class Share extends \OC\Share\Constants { $qb->select('`uid_owner`') ->from('`*PREFIX*share`') - ->where($qb->expr()->eq('`id`', $shareId)); + ->where('`id` = :shareId') + ->setParameter(':shareId', $shareId); $result = $qb->execute(); $result = $result->fetch(); @@ -1215,8 +1216,11 @@ class Share extends \OC\Share\Constants { $qb = $connection->createQueryBuilder(); $qb->update('`*PREFIX*share`') - ->set('`share_with`', is_null($password) ? 'NULL' : $qb->expr()->literal(\OC::$server->getHasher()->hash($password))) - ->where($qb->expr()->eq('`id`', $shareId)); + ->set('`share_with`', ':pass') + ->where('`id` = :shareId') + ->setParameter(':pass', is_null($password) ? 'NULL' : $qb->expr()->literal(\OC::$server->getHasher()->hash($password))) + ->setParameter(':shareId', $shareId); + $qb->execute(); return true; From 3b1f0e60194aea021433031bd62b3173b912d712 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 30 Mar 2015 21:18:24 +0200 Subject: [PATCH 3/3] Fix unit test --- tests/lib/share/share.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index cd79abd205..523f7b379a 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -1178,6 +1178,7 @@ class Test_Share extends \Test\TestCase { $qb->method('andWhere')->will($this->returnSelf()); $qb->method('select')->will($this->returnSelf()); $qb->method('from')->will($this->returnSelf()); + $qb->method('setParameter')->will($this->returnSelf()); $qb->method('expr')->willReturn($ex); $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement') @@ -1232,6 +1233,7 @@ class Test_Share extends \Test\TestCase { $qb->method('set')->will($this->returnSelf()); $qb->method('where')->will($this->returnSelf()); $qb->method('andWhere')->will($this->returnSelf()); + $qb->method('setParameter')->will($this->returnSelf()); $qb->method('expr')->willReturn($ex); $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement') @@ -1284,6 +1286,7 @@ class Test_Share extends \Test\TestCase { $qb->method('andWhere')->will($this->returnSelf()); $qb->method('select')->will($this->returnSelf()); $qb->method('from')->will($this->returnSelf()); + $qb->method('setParameter')->will($this->returnSelf()); $qb->method('expr')->willReturn($ex); $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement') @@ -1336,6 +1339,7 @@ class Test_Share extends \Test\TestCase { $qb->method('andWhere')->will($this->returnSelf()); $qb->method('select')->will($this->returnSelf()); $qb->method('from')->will($this->returnSelf()); + $qb->method('setParameter')->will($this->returnSelf()); $qb->method('expr')->willReturn($ex); $ret = $this->getMockBuilder('\Doctrine\DBAL\Driver\ResultStatement')