Skip to content
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

Jump to bottom

Allow blocking of group sharing #23398

Merged
merged 6 commits into from Mar 22, 2016
Merged

Allow blocking of group sharing #23398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
35024be
Add `allow sharing with groups` checkbox to admin page
rullzer Mar 18, 2016
195efc1
Add config to sharemanager
rullzer Mar 18, 2016
52826d0
Block group sharing in API and in share manager
rullzer Mar 18, 2016
e69a097
Respect disabled group sharing in sharee endpoint
rullzer Mar 18, 2016
6719f8c
Add intergration tests
rullzer Mar 21, 2016
cf3e740
Fix js strings if group sharing is disabled
rullzer Mar 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions apps/files_sharing/api/share20ocs.php
View file
Open in desktop
Expand Up @@ -264,6 +264,10 @@ public function createShare() {
$share->setSharedWith($shareWith);
$share->setPermissions($permissions);
} else if ($shareType === \OCP\Share::SHARE_TYPE_GROUP) {
if (!$this->shareManager->allowGroupSharing()) {
return new \OC_OCS_Result(null, 404, 'group sharing is disabled by the administrator');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the administrator part, that's obvious for each "disabled"setting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just there out of consistency... (it is basically the same message as for disabled public upload etc)

}

// Valid group is required to share
if ($shareWith === null || !$this->groupManager->groupExists($shareWith)) {
return new \OC_OCS_Result(null, 404, 'please specify a valid group');
Expand Down
17 changes: 14 additions & 3 deletions apps/files_sharing/api/sharees.php
View file
Open in desktop
Expand Up @@ -62,6 +62,9 @@ class Sharees {
/** @var ILogger */
protected $logger;

/** @var \OCP\Share\IManager */
protected $shareManager;

/** @var bool */
protected $shareWithGroupOnly = false;

Expand Down Expand Up @@ -97,6 +100,7 @@ class Sharees {
* @param IURLGenerator $urlGenerator
* @param IRequest $request
* @param ILogger $logger
* @param \OCP\Share\IManager $shareManager
*/
public function __construct(IGroupManager $groupManager,
IUserManager $userManager,
Expand All @@ -105,7 +109,8 @@ public function __construct(IGroupManager $groupManager,
IUserSession $userSession,
IURLGenerator $urlGenerator,
IRequest $request,
ILogger $logger) {
ILogger $logger,
\OCP\Share\IManager $shareManager) {
$this->groupManager = $groupManager;
$this->userManager = $userManager;
$this->contactsManager = $contactsManager;
Expand All @@ -114,6 +119,7 @@ public function __construct(IGroupManager $groupManager,
$this->urlGenerator = $urlGenerator;
$this->request = $request;
$this->logger = $logger;
$this->shareManager = $shareManager;
}

/**
Expand Down Expand Up @@ -411,9 +417,14 @@ public function search() {

$shareTypes = [
Share::SHARE_TYPE_USER,
Share::SHARE_TYPE_GROUP,
Share::SHARE_TYPE_REMOTE,
];

if ($this->shareManager->allowGroupSharing()) {
$shareTypes[] = Share::SHARE_TYPE_GROUP;
}

$shareTypes[] = Share::SHARE_TYPE_REMOTE;

if (isset($_GET['shareType']) && is_array($_GET['shareType'])) {
$shareTypes = array_intersect($shareTypes, $_GET['shareType']);
sort($shareTypes);
Expand Down
3 changes: 2 additions & 1 deletion apps/files_sharing/appinfo/routes.php
View file
Open in desktop
Expand Up @@ -126,7 +126,8 @@ function() {
\OC::$server->getUserSession(),
\OC::$server->getURLGenerator(),
\OC::$server->getRequest(),
\OC::$server->getLogger());
\OC::$server->getLogger(),
\OC::$server->getShareManager());

API::register('get',
'/apps/files_sharing/api/v1/sharees',
Expand Down
50 changes: 49 additions & 1 deletion apps/files_sharing/tests/api/share20ocstest.php
View file
Open in desktop
Expand Up @@ -759,9 +759,12 @@ public function testCreateShareGroup() {
->with('valid-path')
->willReturn($path);

$group = $this->getMock('\OCP\IGroup');
$this->groupManager->method('groupExists')->with('validGroup')->willReturn(true);

$this->shareManager->expects($this->once())
->method('allowGroupSharing')
->willReturn(true);

$share->method('setPath')->with($path);
$share->method('setPermissions')->with(\OCP\Constants::PERMISSION_ALL);
$share->method('setShareType')->with(\OCP\Share::SHARE_TYPE_GROUP);
Expand All @@ -775,6 +778,51 @@ public function testCreateShareGroup() {
$this->assertEquals($expected->getData(), $result->getData());
}

public function testCreateShareGroupNotAllowed() {
$share = $this->getMock('\OCP\Share\IShare');
$this->shareManager->method('newShare')->willReturn($share);

$this->request
->method('getParam')
->will($this->returnValueMap([
['path', null, 'valid-path'],
['permissions', null, \OCP\Constants::PERMISSION_ALL],
['shareType', '-1', \OCP\Share::SHARE_TYPE_GROUP],
['shareWith', null, 'validGroup'],
]));

$userFolder = $this->getMock('\OCP\Files\Folder');
$this->rootFolder->expects($this->once())
->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);

$path = $this->getMock('\OCP\Files\Folder');
$storage = $this->getMock('OCP\Files\Storage');
$storage->method('instanceOfStorage')
->with('OCA\Files_Sharing\External\Storage')
->willReturn(false);
$path->method('getStorage')->willReturn($storage);
$userFolder->expects($this->once())
->method('get')
->with('valid-path')
->willReturn($path);

$this->groupManager->method('groupExists')->with('validGroup')->willReturn(true);

$this->shareManager->expects($this->once())
->method('allowGroupSharing')
->willReturn(false);

$share->method('setPath')->with($path);

$expected = new \OC_OCS_Result(null, 404, 'group sharing is disabled by the administrator');
$result = $this->ocs->createShare();

$this->assertEquals($expected->getMeta(), $result->getMeta());
$this->assertEquals($expected->getData(), $result->getData());
}

public function testCreateShareLinkNoLinksAllowed() {
$this->request
->method('getParam')
Expand Down
84 changes: 53 additions & 31 deletions apps/files_sharing/tests/api/shareestest.php
View file
Open in desktop
Expand Up @@ -55,6 +55,9 @@ class ShareesTest extends TestCase {
/** @var \OCP\IRequest|\PHPUnit_Framework_MockObject_MockObject */
protected $request;

/** @var \OCP\Share\IManager|\PHPUnit_Framework_MockObject_MockObject */
protected $shareManager;

protected function setUp() {
parent::setUp();

Expand All @@ -78,6 +81,10 @@ protected function setUp() {
->disableOriginalConstructor()
->getMock();

$this->shareManager = $this->getMockBuilder('OCP\Share\IManager')
->disableOriginalConstructor()
->getMock();

$this->sharees = new Sharees(
$this->groupManager,
$this->userManager,
Expand All @@ -86,7 +93,8 @@ protected function setUp() {
$this->session,
$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
$this->request,
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock()
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
$this->shareManager
);
}

Expand Down Expand Up @@ -966,89 +974,95 @@ public function dataSearch() {
$allTypes = [Share::SHARE_TYPE_USER, Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_REMOTE];

return [
[[], '', 'yes', true, '', null, $allTypes, 1, 200, false, true],
[[], '', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],

// Test itemType
[[
'search' => '',
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],
[[
'search' => 'foobar',
], '', 'yes', true, 'foobar', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, 'foobar', null, $allTypes, 1, 200, false, true, true],
[[
'search' => 0,
], '', 'yes', true, '0', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, '0', null, $allTypes, 1, 200, false, true, true],

// Test itemType
[[
'itemType' => '',
], '', 'yes', true, '', '', $allTypes, 1, 200, false, true],
], '', 'yes', true, '', '', $allTypes, 1, 200, false, true, true],
[[
'itemType' => 'folder',
], '', 'yes', true, '', 'folder', $allTypes, 1, 200, false, true],
], '', 'yes', true, '', 'folder', $allTypes, 1, 200, false, true, true],
[[
'itemType' => 0,
], '', 'yes', true, '', '0', $allTypes, 1, 200, false, true],
], '', 'yes', true, '', '0', $allTypes, 1, 200, false, true, true],

// Test shareType
[[
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],
[[
'shareType' => 0,
], '', 'yes', true, '', null, [0], 1, 200, false, true],
], '', 'yes', true, '', null, [0], 1, 200, false, true, true],
[[
'shareType' => '0',
], '', 'yes', true, '', null, [0], 1, 200, false, true],
], '', 'yes', true, '', null, [0], 1, 200, false, true, true],
[[
'shareType' => 1,
], '', 'yes', true, '', null, [1], 1, 200, false, true],
], '', 'yes', true, '', null, [1], 1, 200, false, true, true],
[[
'shareType' => 12,
], '', 'yes', true, '', null, [], 1, 200, false, true],
], '', 'yes', true, '', null, [], 1, 200, false, true, true],
[[
'shareType' => 'foobar',
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],
[[
'shareType' => [0, 1, 2],
], '', 'yes', true, '', null, [0, 1], 1, 200, false, true],
], '', 'yes', true, '', null, [0, 1], 1, 200, false, true, true],
[[
'shareType' => [0, 1],
], '', 'yes', true, '', null, [0, 1], 1, 200, false, true],
], '', 'yes', true, '', null, [0, 1], 1, 200, false, true, true],
[[
'shareType' => $allTypes,
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],
[[
'shareType' => $allTypes,
], '', 'yes', false, '', null, [0, 1], 1, 200, false, true],
], '', 'yes', false, '', null, [0, 1], 1, 200, false, true, true],
[[
'shareType' => $allTypes,
], '', 'yes', true, '', null, [0, 6], 1, 200, false, true, false],
[[
'shareType' => $allTypes,
], '', 'yes', false, '', null, [0], 1, 200, false, true, false],

// Test pagination
[[
'page' => 1,
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],
[[
'page' => 10,
], '', 'yes', true, '', null, $allTypes, 10, 200, false, true],
], '', 'yes', true, '', null, $allTypes, 10, 200, false, true, true],

// Test perPage
[[
'perPage' => 1,
], '', 'yes', true, '', null, $allTypes, 1, 1, false, true],
], '', 'yes', true, '', null, $allTypes, 1, 1, false, true, true],
[[
'perPage' => 10,
], '', 'yes', true, '', null, $allTypes, 1, 10, false, true],
], '', 'yes', true, '', null, $allTypes, 1, 10, false, true, true],

// Test $shareWithGroupOnly setting
[[], 'no', 'yes', true, '', null, $allTypes, 1, 200, false, true],
[[], 'yes', 'yes', true, '', null, $allTypes, 1, 200, true, true],
[[], 'no', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],
[[], 'yes', 'yes', true, '', null, $allTypes, 1, 200, true, true, true],

// Test $shareeEnumeration setting
[[], 'no', 'yes', true, '', null, $allTypes, 1, 200, false, true],
[[], 'no', 'no', true, '', null, $allTypes, 1, 200, false, false],
[[], 'no', 'yes', true, '', null, $allTypes, 1, 200, false, true, true],
[[], 'no', 'no', true, '', null, $allTypes, 1, 200, false, false, true],

// Test keep case for search
[[
'search' => 'foo@example.com/ownCloud',
], '', 'yes', true, 'foo@example.com/ownCloud', null, $allTypes, 1, 200, false, true],
], '', 'yes', true, 'foo@example.com/ownCloud', null, $allTypes, 1, 200, false, true, true],
];
}

Expand All @@ -1066,8 +1080,9 @@ public function dataSearch() {
* @param int $perPage
* @param bool $shareWithGroupOnly
* @param bool $shareeEnumeration
* @param bool $allowGroupSharing
*/
public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEnabled, $search, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly, $shareeEnumeration) {
public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEnabled, $search, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly, $shareeEnumeration, $allowGroupSharing) {
$oldGet = $_GET;
$_GET = $getData;

Expand All @@ -1082,6 +1097,10 @@ public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEn
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', $enumSetting],
]);

$this->shareManager->expects($this->once())
->method('allowGroupSharing')
->willReturn($allowGroupSharing);

$sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees')
->setConstructorArgs([
$this->groupManager,
Expand All @@ -1091,7 +1110,8 @@ public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEn
$this->session,
$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock()
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
$this->shareManager
])
->setMethods(array('searchSharees', 'isRemoteSharingAllowed'))
->getMock();
Expand Down Expand Up @@ -1175,7 +1195,8 @@ public function testSearchInvalid($getData, $message) {
$this->session,
$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock()
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
$this->shareManager
])
->setMethods(array('searchSharees', 'isRemoteSharingAllowed'))
->getMock();
Expand Down Expand Up @@ -1327,7 +1348,8 @@ public function testSearchSharees($searchTerm, $itemType, array $shareTypes, $pa
$this->session,
$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock()
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
$this->shareManager
])
->setMethods(array('getShareesForShareIds', 'getUsers', 'getGroups', 'getRemote'))
->getMock();
Expand Down
1 change: 1 addition & 0 deletions build/integration/features/bootstrap/ShareesContext.php
View file
Open in desktop
Expand Up @@ -68,5 +68,6 @@ public function getArrayOfShareesResponded(ResponseInterface $response, $shareeT
protected function resetAppConfigs() {
$this->modifyServerConfig('core', 'shareapi_only_share_with_group_members', 'no');
$this->modifyServerConfig('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes');
$this->modifyServerConfig('core', 'shareapi_allow_group_sharing', 'yes');
}
}
16 changes: 16 additions & 0 deletions build/integration/sharees_features/sharees.feature
View file
Open in desktop
Expand Up @@ -222,3 +222,19 @@ Feature: sharees
Then "groups" sharees returned is empty
Then "exact remotes" sharees returned is empty
Then "remotes" sharees returned is empty

Scenario: Group sharees not returned when group sharing is disabled
Given As an "test"
And parameter "shareapi_allow_group_sharing" of app "core" is set to "no"
When getting sharees for
| search | sharee |
| itemType | file |
Then the OCS status code should be "100"
And the HTTP status code should be "200"
And "exact users" sharees returned is empty
And "users" sharees returned are
| Sharee1 | 0 | Sharee1 |
And "exact groups" sharees returned is empty
And "groups" sharees returned is empty
And "exact remotes" sharees returned is empty
And "remotes" sharees returned is empty
3 changes: 2 additions & 1 deletion core/js/config.php
View file
Open in desktop
Expand Up @@ -162,7 +162,8 @@
'sharingDisabledForUser' => \OCP\Util::isSharingDisabledForUser(),
'resharingAllowed' => \OCP\Share::isResharingAllowed(),
'remoteShareAllowed' => $outgoingServer2serverShareEnabled,
'federatedCloudShareDoc' => \OC::$server->getURLGenerator()->linkToDocs('user-sharing-federated')
'federatedCloudShareDoc' => \OC::$server->getURLGenerator()->linkToDocs('user-sharing-federated'),
'allowGroupSharing' => \OC::$server->getShareManager()->allowGroupSharing()
)
)
),
Expand Down
3 changes: 2 additions & 1 deletion core/js/shareconfigmodel.js
View file
Open in desktop
Expand Up @@ -26,7 +26,8 @@
isDefaultExpireDateEnabled: oc_appconfig.core.defaultExpireDateEnabled === true,
isRemoteShareAllowed: oc_appconfig.core.remoteShareAllowed,
defaultExpireDate: oc_appconfig.core.defaultExpireDate,
isResharingAllowed: oc_appconfig.core.resharingAllowed
isResharingAllowed: oc_appconfig.core.resharingAllowed,
allowGroupSharing: oc_appconfig.core.allowGroupSharing
},

/**
Expand Down