Skip to content

Skip null groups in group manager #3884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nickvergessen merged 4 commits into from
Mar 20, 2017
Merged

Skip null groups in group manager #3884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
377fdf3
Skip null groups in group manager (#26871) (#26956)
Jan 19, 2017
bd97b7d
Use DI
nickvergessen Mar 17, 2017
ebabf81
Clean up the test
nickvergessen Mar 17, 2017
80d2717
Fix 5.6 duplicate class import
nickvergessen Mar 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ protected function canAccessShare(\OCP\Share\IShare $share, $checkGroups = true)
if ($checkGroups && $share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) {
$sharedWith = $this->groupManager->get($share->getSharedWith());
$user = $this->userManager->get($this->currentUser);
Copy link
Member

@nickvergessen nickvergessen Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also save this call for $sharedWith === null
But since this is only for an error case it's probably okay

if ($user !== null && $sharedWith->inGroup($user)) {
if ($user !== null && $sharedWith !== null && $sharedWith->inGroup($user)) {
return true;
}
}
Expand Down
18 changes: 17 additions & 1 deletion apps/files_sharing/lib/MountProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,23 @@ private function buildSuperShares(array $allShares, \OCP\IUser $user) {
if ($share->getTarget() !== $superShare->getTarget()) {
// adjust target, for database consistency
$share->setTarget($superShare->getTarget());
$this->shareManager->moveShare($share, $user->getUID());
try {
$this->shareManager->moveShare($share, $user->getUID());
} catch (\InvalidArgumentException $e) {
// ignore as it is not important and we don't want to
// block FS setup

// the subsequent code anyway only uses the target of the
// super share

// such issue can usually happen when dealing with
// null groups which usually appear with group backend
// caching inconsistencies
$this->logger->debug(
'Could not adjust share target for share ' . $share->getId() . ' to make it consistent: ' . $e->getMessage(),
['app' => 'files_sharing']
);
}
}
if (!is_null($share->getNodeCacheEntry())) {
$superShare->setNodeCacheEntry($share->getNodeCacheEntry());
Expand Down
7 changes: 6 additions & 1 deletion apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,14 +544,19 @@ public function testCanAccessShare() {
$this->groupManager->method('get')->will($this->returnValueMap([
['group', $group],
['group2', $group2],
['groupnull', null],
]));
$this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));

$share = $this->getMockBuilder('OCP\Share\IShare')->getMock();
$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP);
$share->method('getSharedWith')->willReturn('group2');
$this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));

$this->groupManager->method('get')->with('group2')->willReturn($group);
// null group
$share = $this->getMock('OCP\Share\IShare');
$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP);
$share->method('getSharedWith')->willReturn('groupnull');
$this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));

$share = $this->getMockBuilder('OCP\Share\IShare')->getMock();
Expand Down
22 changes: 21 additions & 1 deletion apps/files_sharing/tests/MountProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,20 @@ public function mergeSharesDataProvider() {
['1', 100, 'user2', '/share2-renamed', 31],
],
],
// #9: share as outsider with "nullgroup" and "user1" where recipient renamed in between
[
[
[2, 100, 'user2', '/share2', 31],
],
[
[1, 100, 'nullgroup', '/share2-renamed', 31],
],
[
// use target of least recent share
['1', 100, 'nullgroup', '/share2-renamed', 31],
],
true
],
];
}

Expand All @@ -278,7 +292,7 @@ public function mergeSharesDataProvider() {
* @param array $groupShares array of group share specs
* @param array $expectedShares array of expected supershare specs
*/
public function testMergeShares($userShares, $groupShares, $expectedShares) {
public function testMergeShares($userShares, $groupShares, $expectedShares, $moveFails = false) {
$rootFolder = $this->createMock(IRootFolder::class);
$userManager = $this->createMock(IUserManager::class);

Expand Down Expand Up @@ -307,6 +321,12 @@ public function testMergeShares($userShares, $groupShares, $expectedShares) {
return new \OC\Share20\Share($rootFolder, $userManager);
}));

if ($moveFails) {
$this->shareManager->expects($this->any())
->method('moveShare')
->will($this->throwException(new \InvalidArgumentException()));
}

$mounts = $this->provider->getMountsForUser($this->user, $this->loader);

$this->assertCount(count($expectedShares), $mounts);
Expand Down
30 changes: 24 additions & 6 deletions lib/private/Group/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@

use OC\Hooks\PublicEmitter;
use OCP\GroupInterface;
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\ILogger;
use OCP\IUser;

/**
* Class Manager
Expand Down Expand Up @@ -78,11 +81,16 @@ class Manager extends PublicEmitter implements IGroupManager {
/** @var \OC\SubAdmin */
private $subAdmin = null;

/** @var ILogger */
private $logger;

/**
* @param \OC\User\Manager $userManager
* @param ILogger $logger
*/
public function __construct(\OC\User\Manager $userManager) {
public function __construct(\OC\User\Manager $userManager, ILogger $logger) {
$this->userManager = $userManager;
$this->logger = $logger;
$cachedGroups = & $this->cachedGroups;
$cachedUserGroups = & $this->cachedUserGroups;
$this->listen('\OC\Group', 'postDelete', function ($group) use (&$cachedGroups, &$cachedUserGroups) {
Expand Down Expand Up @@ -186,15 +194,15 @@ protected function getGroupObject($gid, $displayName = null) {
* @return bool
*/
public function groupExists($gid) {
return !is_null($this->get($gid));
return $this->get($gid) instanceof IGroup;
}

/**
* @param string $gid
* @return \OC\Group\Group
*/
public function createGroup($gid) {
if ($gid === '' || is_null($gid)) {
if ($gid === '' || $gid === null) {
return false;
} else if ($group = $this->get($gid)) {
return $group;
Expand Down Expand Up @@ -223,7 +231,12 @@ public function search($search, $limit = null, $offset = null) {
foreach ($this->backends as $backend) {
$groupIds = $backend->getGroups($search, $limit, $offset);
foreach ($groupIds as $groupId) {
$groups[$groupId] = $this->get($groupId);
$aGroup = $this->get($groupId);
if ($aGroup instanceof IGroup) {
$groups[$groupId] = $aGroup;
} else {
$this->logger->debug('Group "' . $groupId . '" was returned by search but not found through direct access', ['app' => 'core']);
}
}
if (!is_null($limit) and $limit <= 0) {
return array_values($groups);
Expand All @@ -237,7 +250,7 @@ public function search($search, $limit = null, $offset = null) {
* @return \OC\Group\Group[]
*/
public function getUserGroups($user) {
if (is_null($user)) {
if (!$user instanceof IUser) {
return [];
}
return $this->getUserIdGroups($user->getUID());
Expand All @@ -256,7 +269,12 @@ public function getUserIdGroups($uid) {
$groupIds = $backend->getUserGroups($uid);
if (is_array($groupIds)) {
foreach ($groupIds as $groupId) {
$groups[$groupId] = $this->get($groupId);
$aGroup = $this->get($groupId);
if ($aGroup instanceof IGroup) {
$groups[$groupId] = $aGroup;
} else {
$this->logger->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', ['app' => 'core']);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ public function __construct($webRoot, \OC\Config $config) {
return new \OC\User\Manager($config);
});
$this->registerService('GroupManager', function (Server $c) {
$groupManager = new \OC\Group\Manager($this->getUserManager());
$groupManager = new \OC\Group\Manager($this->getUserManager(), $this->getLogger());
$groupManager->listen('\OC\Group', 'preCreate', function ($gid) {
\OC_Hook::emit('OC_Group', 'pre_createGroup', array('run' => true, 'gid' => $gid));
});
Expand Down
4 changes: 4 additions & 0 deletions lib/private/Share20/DefaultShareProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,10 @@ public function deleteFromSelf(\OCP\Share\IShare $share, $recipient) {
$group = $this->groupManager->get($share->getSharedWith());
$user = $this->userManager->get($recipient);

if (is_null($group)) {
throw new ProviderException('Group "' . $share->getSharedWith() . '" does not exist');
}

if (!$group->inGroup($user)) {
throw new ProviderException('Recipient not in receiving group');
}
Expand Down
13 changes: 9 additions & 4 deletions lib/private/Share20/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,10 +393,12 @@ protected function userCreateChecks(\OCP\Share\IShare $share) {
// The share is already shared with this user via a group share
if ($existingShare->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) {
$group = $this->groupManager->get($existingShare->getSharedWith());
$user = $this->userManager->get($share->getSharedWith());
if (!is_null($group)) {
$user = $this->userManager->get($share->getSharedWith());

if ($group->inGroup($user) && $existingShare->getShareOwner() !== $share->getShareOwner()) {
throw new \Exception('Path already shared with this user');
if ($group->inGroup($user) && $existingShare->getShareOwner() !== $share->getShareOwner()) {
throw new \Exception('Path already shared with this user');
}
}
}
}
Expand All @@ -418,7 +420,7 @@ protected function groupCreateChecks(\OCP\Share\IShare $share) {
if ($this->shareWithGroupMembersOnly()) {
$sharedBy = $this->userManager->get($share->getSharedBy());
$sharedWith = $this->groupManager->get($share->getSharedWith());
if (!$sharedWith->inGroup($sharedBy)) {
if (is_null($sharedWith) || !$sharedWith->inGroup($sharedBy)) {
throw new \Exception('Only sharing within your own groups is allowed');
}
}
Expand Down Expand Up @@ -886,6 +888,9 @@ public function moveShare(\OCP\Share\IShare $share, $recipientId) {

if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) {
$sharedWith = $this->groupManager->get($share->getSharedWith());
if (is_null($sharedWith)) {
throw new \InvalidArgumentException('Group "' . $share->getSharedWith() . '" does not exist');
}
$recipient = $this->userManager->get($recipientId);
if (!$sharedWith->inGroup($recipient)) {
throw new \InvalidArgumentException('Invalid recipient');
Expand Down
Loading