wbrawner/server
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix reshare permission change to not impair other deletion code

Browse source 
A recent change that prevents reshare permission changes to delete group
share children had the side-effect of also preventing group share
children deletion when it needed to be done.

This fix adds an extra flag to isolate the "reshare permission change"
deletion case and keep the other ones as they were before, not only to
fix the regression but also fix other potential regressions in code that
uses this method.

Also updated the comment because now Helper::delete() is no longer
limited to reshares but also applies to group share children.
This commit is contained in:
Vincent Petry 2015-01-20 13:09:39 +01:00 committed by Morris Jobke
parent 4894a2c458
commit 9f137ac259
3 changed files with 68 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

27
lib/private/share/helper.php
View file

@ -79,13 +79,14 @@ class Helper extends \OC\Share\Constants {
}
/**
* Delete all reshares of an item
* Delete all reshares and group share children of an item
* @param int $parent Id of item to delete
* @param bool $excludeParent If true, exclude the parent from the delete (optional)
* @param string $uidOwner The user that the parent was shared with (optional)
* @param int $newParent new parent for the childrens
* @param bool $excludeGroupChildren exclude group children elements
*/
public static function delete($parent, $excludeParent = false, $uidOwner = null, $newParent = null) {
public static function delete($parent, $excludeParent = false, $uidOwner = null, $newParent = null, $excludeGroupChildren = false) {
$ids = array($parent);
$deletedItems = array();
$changeParent = array();
@ -94,15 +95,25 @@ class Helper extends \OC\Share\Constants {
$parents = "'".implode("','", $parents)."'";
// Check the owner on the first search of reshares, useful for
// finding and deleting the reshares by a single user of a group share
$params = array();
if (count($ids) == 1 && isset($uidOwner)) {
$query = \OC_DB::prepare('SELECT `id`, `share_with`, `item_type`, `share_type`, `item_target`, `file_target`, `parent`'
.' FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') AND `uid_owner` = ? AND `share_type` != ?');
$result = $query->execute(array($uidOwner, self::$shareTypeGroupUserUnique));
// FIXME: don't concat $parents, use Docrine's PARAM_INT_ARRAY approach
$queryString = 'SELECT `id`, `share_with`, `item_type`, `share_type`, ' .
'`item_target`, `file_target`, `parent` ' .
'FROM `*PREFIX*share` ' .
'WHERE `parent` IN ('.$parents.') AND `uid_owner` = ? ';
$params[] = $uidOwner;
} else {
$query = \OC_DB::prepare('SELECT `id`, `share_with`, `item_type`, `share_type`, `item_target`, `file_target`, `parent`, `uid_owner`'
.' FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') AND `share_type` != ?');
$result = $query->execute(array(self::$shareTypeGroupUserUnique));
$queryString = 'SELECT `id`, `share_with`, `item_type`, `share_type`, ' .
'`item_target`, `file_target`, `parent`, `uid_owner` ' .
'FROM `*PREFIX*share` WHERE `parent` IN ('.$parents.') ';
}
if ($excludeGroupChildren) {
$queryString .= ' AND `share_type` != ?';
$params[] = self::$shareTypeGroupUserUnique;
}
$query = \OC_DB::prepare($queryString);
$result = $query->execute($params);
// Reset parents array, only go through loop again if items are found
$parents = array();
while ($item = $result->fetchRow()) {

3
lib/private/share/share.php
View file

@ -971,7 +971,8 @@ class Share extends \OC\Share\Constants {
if ($item['permissions'] & ~$permissions) {
// If share permission is removed all reshares must be deleted
if (($item['permissions'] & \OCP\Constants::PERMISSION_SHARE) && (~$permissions & \OCP\Constants::PERMISSION_SHARE)) {
Helper::delete($item['id'], true);
// delete all shares, keep parent and group children
Helper::delete($item['id'], true, null, null, true);
} else {
$ids = array();
$parents = array($item['id']);

47
tests/lib/share/share.php
View file

@ -44,11 +44,13 @@ class Test_Share extends \Test\TestCase {
$this->user2 = $this->getUniqueID('user2_');
$this->user3 = $this->getUniqueID('user3_');
$this->user4 = $this->getUniqueID('user4_');
$this->user5 = $this->getUniqueID('user5_');
$this->groupAndUser = $this->getUniqueID('groupAndUser_');
OC_User::createUser($this->user1, 'pass');
OC_User::createUser($this->user2, 'pass');
OC_User::createUser($this->user3, 'pass');
OC_User::createUser($this->user4, 'pass');
OC_User::createUser($this->user5, 'pass');
OC_User::createUser($this->groupAndUser, 'pass');
OC_User::setUserId($this->user1);
OC_Group::clearBackends();
@ -610,6 +612,51 @@ class Test_Share extends \Test\TestCase {
$this->assertEquals(array(), OCP\Share::getItemsShared('test'));
}
/**
* Test that unsharing from group will also delete all
* child entries
*/
public function testShareWithGroupThenUnshare() {
OC_User::setUserId($this->user5);
OCP\Share::shareItem(
'test',
'test.txt',
OCP\Share::SHARE_TYPE_GROUP,
$this->group1,
\OCP\Constants::PERMISSION_ALL
);
$targetUsers = array($this->user1, $this->user2, $this->user3);
foreach($targetUsers as $targetUser) {
OC_User::setUserId($targetUser);
$items = OCP\Share::getItemsSharedWithUser(
'test',
$targetUser,
Test_Share_Backend::FORMAT_TARGET
);
$this->assertEquals(1, count($items));
}
OC_User::setUserId($this->user5);
OCP\Share::unshare(
'test',
'test.txt',
OCP\Share::SHARE_TYPE_GROUP,
$this->group1
);
// verify that all were deleted
foreach($targetUsers as $targetUser) {
OC_User::setUserId($targetUser);
$items = OCP\Share::getItemsSharedWithUser(
'test',
$targetUser,
Test_Share_Backend::FORMAT_TARGET
);
$this->assertEquals(0, count($items));
}
}
public function testShareWithGroupAndUserBothHaveTheSameId() {