From ca2fd3007343dc4ba10d2b9d5e44ada0340d90cc Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 19 Oct 2015 15:39:39 +0200 Subject: [PATCH 1/3] Remove shares where the parent does not exist anymore --- lib/repair/repairinvalidshares.php | 32 ++++++++ tests/lib/repair/repairinvalidsharestest.php | 78 ++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/lib/repair/repairinvalidshares.php b/lib/repair/repairinvalidshares.php index 4b0aeb70c1..5a4cb445ce 100644 --- a/lib/repair/repairinvalidshares.php +++ b/lib/repair/repairinvalidshares.php @@ -70,11 +70,43 @@ class RepairInvalidShares extends BasicEmitter implements \OC\RepairStep { } } + /** + * Remove shares where the parent share does not exist anymore + */ + private function removeSharesNonExistingParent() { + $deletedEntries = 0; + + $query = $this->connection->getQueryBuilder(); + $query->select('s1.parent') + ->from('share', 's1') + ->where($query->expr()->isNotNull('s1.parent')) + ->andWhere($query->expr()->isNull('s2.id')) + ->leftJoin('s1', 'share', 's2', $query->expr()->eq('s1.parent', 's2.id')) + ->groupBy('s1.parent'); + + $deleteQuery = $this->connection->getQueryBuilder(); + $deleteQuery->delete('share') + ->where($query->expr()->eq('parent', $deleteQuery->createParameter('parent'))); + + $result = $query->execute(); + while ($row = $result->fetch()) { + $deletedEntries += $deleteQuery->setParameter('parent', (int) $row['parent']) + ->execute(); + } + $result->closeCursor(); + + if ($deletedEntries) { + $this->emit('\OC\Repair', 'info', array('Removed ' . $deletedEntries . ' shares where the parent did not exist')); + } + } + public function run() { $ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); if (version_compare($ocVersionFromBeforeUpdate, '8.2.0.7', '<')) { // this situation was only possible before 8.2 $this->removeExpirationDateFromNonLinkShares(); } + + $this->removeSharesNonExistingParent(); } } diff --git a/tests/lib/repair/repairinvalidsharestest.php b/tests/lib/repair/repairinvalidsharestest.php index 89a5ba470e..7aaf273e3c 100644 --- a/tests/lib/repair/repairinvalidsharestest.php +++ b/tests/lib/repair/repairinvalidsharestest.php @@ -119,5 +119,83 @@ class RepairInvalidSharesTest extends TestCase { $this->assertNull($userShare['expiration'], 'bogus expiration date was removed'); $this->assertNotNull($linkShare['expiration'], 'valid link share expiration date still there'); } + + /** + * Test remove shares where the parent share does not exist anymore + */ + public function testSharesNonExistingParent() { + $qb = $this->connection->getQueryBuilder(); + $shareValues = [ + 'share_type' => $qb->expr()->literal(Constants::SHARE_TYPE_USER), + 'share_with' => $qb->expr()->literal('recipientuser1'), + 'uid_owner' => $qb->expr()->literal('user1'), + 'item_type' => $qb->expr()->literal('folder'), + 'item_source' => $qb->expr()->literal(123), + 'item_target' => $qb->expr()->literal('/123'), + 'file_source' => $qb->expr()->literal(123), + 'file_target' => $qb->expr()->literal('/test'), + 'permissions' => $qb->expr()->literal(1), + 'stime' => $qb->expr()->literal(time()), + 'expiration' => $qb->expr()->literal('2015-09-25 00:00:00') + ]; + + // valid share + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share') + ->values($shareValues) + ->execute(); + $parent = $this->getLastShareId(); + + // share with existing parent + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share') + ->values(array_merge($shareValues, [ + 'parent' => $qb->expr()->literal($parent), + ]))->execute(); + $validChild = $this->getLastShareId(); + + // share with non-existing parent + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share') + ->values(array_merge($shareValues, [ + 'parent' => $qb->expr()->literal($parent + 100), + ]))->execute(); + $invalidChild = $this->getLastShareId(); + + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->orderBy('id', 'ASC') + ->execute(); + $rows = $result->fetchAll(); + $this->assertSame([['id' => $parent], ['id' => $validChild], ['id' => $invalidChild]], $rows); + $result->closeCursor(); + + $this->repair->run(); + + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->orderBy('id', 'ASC') + ->execute(); + $rows = $result->fetchAll(); + $this->assertSame([['id' => $parent], ['id' => $validChild]], $rows); + $result->closeCursor(); + } + + /** + * @return int + */ + protected function getLastShareId() { + // select because lastInsertId does not work with OCI + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->orderBy('id', 'DESC') + ->execute(); + $row = $result->fetch(); + $result->closeCursor(); + return $row['id']; + } } From e5a7e3124ae4054f5fdf99e5dd5cca373b4d83ad Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 19 Oct 2015 16:41:43 +0200 Subject: [PATCH 2/3] Add a repair step that checks for group membership on shares --- core/command/maintenance/repair.php | 17 ++- lib/private/repair.php | 13 ++ lib/repair/oldgroupmembershipshares.php | 117 +++++++++++++++ .../repair/oldgroupmembershipsharestest.php | 138 ++++++++++++++++++ 4 files changed, 284 insertions(+), 1 deletion(-) create mode 100644 lib/repair/oldgroupmembershipshares.php create mode 100644 tests/lib/repair/oldgroupmembershipsharestest.php diff --git a/core/command/maintenance/repair.php b/core/command/maintenance/repair.php index 5df362f811..f7c0cc4604 100644 --- a/core/command/maintenance/repair.php +++ b/core/command/maintenance/repair.php @@ -26,6 +26,7 @@ namespace OC\Core\Command\Maintenance; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; class Repair extends Command { @@ -49,10 +50,24 @@ class Repair extends Command { protected function configure() { $this ->setName('maintenance:repair') - ->setDescription('repair this installation'); + ->setDescription('repair this installation') + ->addOption( + 'include-expensive', + null, + InputOption::VALUE_NONE, + 'Use this option when you want to include resource and load expensive tasks' + ) + ; } protected function execute(InputInterface $input, OutputInterface $output) { + $includeExpensive = $input->getOption('include-expensive'); + if ($includeExpensive) { + foreach ($this->repair->getExpensiveRepairSteps() as $step) { + $this->repair->addStep($step); + } + } + $maintenanceMode = $this->config->getSystemValue('maintenance', false); $this->config->setSystemValue('maintenance', true); diff --git a/lib/private/repair.php b/lib/private/repair.php index 20219e313f..f6ac7ebe65 100644 --- a/lib/private/repair.php +++ b/lib/private/repair.php @@ -34,6 +34,7 @@ use OC\Repair\AssetCache; use OC\Repair\CleanTags; use OC\Repair\Collation; use OC\Repair\DropOldJobs; +use OC\Repair\OldGroupMembershipShares; use OC\Repair\RemoveGetETagEntries; use OC\Repair\SqliteAutoincrement; use OC\Repair\DropOldTables; @@ -118,6 +119,18 @@ class Repair extends BasicEmitter { ]; } + /** + * Returns expensive repair steps to be run on the + * command line with a special option. + * + * @return array of RepairStep instances + */ + public static function getExpensiveRepairSteps() { + return [ + new OldGroupMembershipShares(\OC::$server->getDatabaseConnection(), \OC::$server->getGroupManager()), + ]; + } + /** * Returns the repair steps to be run before an * upgrade. diff --git a/lib/repair/oldgroupmembershipshares.php b/lib/repair/oldgroupmembershipshares.php new file mode 100644 index 0000000000..2d701ac9fb --- /dev/null +++ b/lib/repair/oldgroupmembershipshares.php @@ -0,0 +1,117 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @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 OC\Repair; + + +use OC\Hooks\BasicEmitter; +use OC\RepairStep; +use OCP\IDBConnection; +use OCP\IGroupManager; +use OCP\Share; + +class OldGroupMembershipShares extends BasicEmitter implements RepairStep { + + /** @var \OCP\IDBConnection */ + protected $connection; + + /** @var \OCP\IGroupManager */ + protected $groupManager; + + /** + * @var array [gid => [uid => (bool)]] + */ + protected $memberships; + + /** + * @param IDBConnection $connection + * @param IGroupManager $groupManager + */ + public function __construct(IDBConnection $connection, IGroupManager $groupManager) { + $this->connection = $connection; + $this->groupManager = $groupManager; + } + + /** + * Returns the step's name + * + * @return string + */ + public function getName() { + return 'Remove shares of old group memberships'; + } + + /** + * Run repair step. + * Must throw exception on error. + * + * @throws \Exception in case of failure + */ + public function run() { + $deletedEntries = 0; + + $query = $this->connection->getQueryBuilder(); + $query->select(['s1.id', $query->createFunction('s1.`share_with` AS `user`'), $query->createFunction('s2.`share_with` AS `group`')]) + ->from('share', 's1') + ->where($query->expr()->isNotNull('s1.parent')) + // \OC\Share\Constant::$shareTypeGroupUserUnique === 2 + ->andWhere($query->expr()->eq('s1.share_type', $query->expr()->literal(2))) + ->andWhere($query->expr()->isNotNull('s2.id')) + ->andWhere($query->expr()->eq('s2.share_type', $query->expr()->literal(Share::SHARE_TYPE_GROUP))) + ->leftJoin('s1', 'share', 's2', $query->expr()->eq('s1.parent', 's2.id')); + + $deleteQuery = $this->connection->getQueryBuilder(); + $deleteQuery->delete('share') + ->where($query->expr()->eq('id', $deleteQuery->createParameter('share'))); + + $result = $query->execute(); + while ($row = $result->fetch()) { + if (!$this->isMember($row['group'], $row['user'])) { + $deletedEntries += $deleteQuery->setParameter('share', (int) $row['id']) + ->execute(); + } + } + $result->closeCursor(); + + if ($deletedEntries) { + $this->emit('\OC\Repair', 'info', array('Removed ' . $deletedEntries . ' shares where user is not a member of the group anymore')); + } + } + + /** + * @param string $gid + * @param string $uid + * @return bool + */ + protected function isMember($gid, $uid) { + if (isset($this->memberships[$gid][$uid])) { + return $this->memberships[$gid][$uid]; + } + + $isMember = $this->groupManager->isInGroup($uid, $gid); + if (!isset($this->memberships[$gid])) { + $this->memberships[$gid] = []; + } + $this->memberships[$gid][$uid] = $isMember; + + return $isMember; + } +} diff --git a/tests/lib/repair/oldgroupmembershipsharestest.php b/tests/lib/repair/oldgroupmembershipsharestest.php new file mode 100644 index 0000000000..74f68bd789 --- /dev/null +++ b/tests/lib/repair/oldgroupmembershipsharestest.php @@ -0,0 +1,138 @@ + + * This file is licensed under the Affero General Public License version 3 or + * later. + * See the COPYING-README file. + */ + +namespace Test\Repair; + +use OC\Repair\OldGroupMembershipShares; +use OC\Share\Constants; + +class OldGroupMembershipSharesTest extends \Test\TestCase { + + /** @var OldGroupMembershipShares */ + protected $repair; + + /** @var \OCP\IDBConnection */ + protected $connection; + + /** @var \OCP\IGroupManager|\PHPUnit_Framework_MockObject_MockObject */ + protected $groupManager; + + protected function setUp() { + parent::setUp(); + + /** \OCP\IGroupManager|\PHPUnit_Framework_MockObject_MockObject */ + $this->groupManager = $this->getMockBuilder('OCP\IGroupManager') + ->disableOriginalConstructor() + ->getMock(); + $this->connection = \OC::$server->getDatabaseConnection(); + + $this->deleteAllShares(); + } + + protected function tearDown() { + $this->deleteAllShares(); + + parent::tearDown(); + } + + protected function deleteAllShares() { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('share')->execute(); + } + + public function testRun() { + $repair = new OldGroupMembershipShares( + $this->connection, + $this->groupManager + ); + + $this->groupManager->expects($this->exactly(2)) + ->method('isInGroup') + ->willReturnMap([ + ['member', 'group', true], + ['not-a-member', 'group', false], + ]); + + $parent = $this->createShare(Constants::SHARE_TYPE_GROUP, 'group', null); + $group2 = $this->createShare(Constants::SHARE_TYPE_GROUP, 'group2', $parent); + $user1 = $this->createShare(Constants::SHARE_TYPE_USER, 'user1', $parent); + + // \OC\Share\Constant::$shareTypeGroupUserUnique === 2 + $member = $this->createShare(2, 'member', $parent); + $notAMember = $this->createShare(2, 'not-a-member', $parent); + + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->orderBy('id', 'ASC') + ->execute(); + $rows = $result->fetchAll(); + $this->assertSame([['id' => $parent], ['id' => $group2], ['id' => $user1], ['id' => $member], ['id' => $notAMember]], $rows); + $result->closeCursor(); + + $repair->run(); + + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->orderBy('id', 'ASC') + ->execute(); + $rows = $result->fetchAll(); + $this->assertSame([['id' => $parent], ['id' => $group2], ['id' => $user1], ['id' => $member]], $rows); + $result->closeCursor(); + } + + /** + * @param string $shareType + * @param string $shareWith + * @param null|int $parent + * @return int + */ + protected function createShare($shareType, $shareWith, $parent) { + $qb = $this->connection->getQueryBuilder(); + $shareValues = [ + 'share_type' => $qb->expr()->literal($shareType), + 'share_with' => $qb->expr()->literal($shareWith), + 'uid_owner' => $qb->expr()->literal('user1'), + 'item_type' => $qb->expr()->literal('folder'), + 'item_source' => $qb->expr()->literal(123), + 'item_target' => $qb->expr()->literal('/123'), + 'file_source' => $qb->expr()->literal(123), + 'file_target' => $qb->expr()->literal('/test'), + 'permissions' => $qb->expr()->literal(1), + 'stime' => $qb->expr()->literal(time()), + 'expiration' => $qb->expr()->literal('2015-09-25 00:00:00'), + ]; + + if ($parent) { + $shareValues['parent'] = $qb->expr()->literal($parent); + } + + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share') + ->values($shareValues) + ->execute(); + + return $this->getLastShareId(); + } + + /** + * @return int + */ + protected function getLastShareId() { + // select because lastInsertId does not work with OCI + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->orderBy('id', 'DESC') + ->execute(); + $row = $result->fetch(); + $result->closeCursor(); + return $row['id']; + } +} From 6043f0afdb88caf667cc7ace2c5615735de71ac3 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 20 Oct 2015 10:11:25 +0200 Subject: [PATCH 3/3] Use the method --- tests/lib/repair/repairinvalidsharestest.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/lib/repair/repairinvalidsharestest.php b/tests/lib/repair/repairinvalidsharestest.php index 7aaf273e3c..005a2db234 100644 --- a/tests/lib/repair/repairinvalidsharestest.php +++ b/tests/lib/repair/repairinvalidsharestest.php @@ -77,13 +77,7 @@ class RepairInvalidSharesTest extends TestCase { ]) ->execute(); - // select because lastInsertId does not work with OCI - $results = $this->connection->getQueryBuilder() - ->select('id') - ->from('share') - ->execute() - ->fetchAll(); - $bogusShareId = $results[0]['id']; + $bogusShareId = $this->getLastShareId(); // link share with expiration date $qb = $this->connection->getQueryBuilder();