From 12338e0ef07c409156fa9cd1008bb981bda20461 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 13 May 2014 15:22:18 +0200 Subject: [PATCH] allow admin to disable sharing for specific groups of users --- apps/files/css/files.css | 4 ++ apps/files_sharing/js/share.js | 32 ++++++++--- apps/files_sharing/lib/permissions.php | 14 +++-- apps/files_sharing/lib/sharedstorage.php | 8 +++ apps/files_sharing/tests/api.php | 72 ++++++++++++++++++++++++ apps/files_sharing/tests/base.php | 2 + apps/files_sharing/tests/proxy.php | 4 +- core/js/config.php | 1 + lib/private/files/cache/permissions.php | 19 ++++++- lib/private/files/storage/common.php | 4 ++ lib/private/share/share.php | 10 +++- lib/private/util.php | 23 ++++++++ lib/public/util.php | 9 +++ settings/admin.php | 18 ++++++ settings/ajax/excludegroups.php | 18 ++++++ settings/css/settings.css | 9 +++ settings/js/admin.js | 49 ++++++++++++++++ settings/routes.php | 2 + settings/templates/admin.php | 22 ++++++-- tests/lib/util.php | 55 ++++++++++++++++++ 20 files changed, 352 insertions(+), 23 deletions(-) create mode 100644 settings/ajax/excludegroups.php diff --git a/apps/files/css/files.css b/apps/files/css/files.css index 009cb355ba..731dd7a23e 100644 --- a/apps/files/css/files.css +++ b/apps/files/css/files.css @@ -358,6 +358,10 @@ table td.filename form { font-size:14px; margin-left:48px; margin-right:48px; } padding: 28px 14px 19px !important; } +#fileList .action.action-share-notification span, img, a { + cursor: default !important; +} + a.action>img { max-height:16px; max-width:16px; vertical-align:text-bottom; } /* Actions for selected files */ diff --git a/apps/files_sharing/js/share.js b/apps/files_sharing/js/share.js index 7d68a8d886..1b04097ccb 100644 --- a/apps/files_sharing/js/share.js +++ b/apps/files_sharing/js/share.js @@ -27,13 +27,29 @@ $(document).ready(function() { } $('#fileList').on('fileActionsReady',function(){ - var $fileList = $(this); - var allShared = $fileList.find('[data-share-owner] [data-Action="Share"]'); - allShared.addClass('permanent'); - allShared.find('span').text(function(){ - var $owner = $(this).closest('tr').attr('data-share-owner'); - return ' ' + t('files_sharing', 'Shared by {owner}', {owner: $owner}); - }); + // if no share action exists because the admin disabled sharing for this user + // we create a share notification action to inform the user about files + // shared with him otherwise we just update the existing share action. + var allShared; + if (oc_appconfig.core.sharingDisabledForUser) { + var $fileList = $(this); + allShared = $fileList.find('[data-share-owner]'); + var shareNotification = '' + + ' '; + $(allShared).find('.fileactions').append(function() { + var owner = $(this).closest('tr').attr('data-share-owner'); + var shareBy = t('files_sharing', 'Shared by {owner}', {owner: owner}); + return shareNotification + ' ' + shareBy + ''; + }); + } else { + allShared = $fileList.find('[data-share-owner] [data-Action="Share"]'); + allShared.addClass('permanent'); + allShared.find('span').text(function(){ + var $owner = $(this).closest('tr').attr('data-share-owner'); + return ' ' + t('files_sharing', 'Shared by {owner}', {owner: $owner}); + }); + } // FIXME: these calls are also working on hard-coded // list selectors... @@ -48,7 +64,7 @@ $(document).ready(function() { } }); - FileActions.register('all', 'Share', OC.PERMISSION_READ, OC.imagePath('core', 'actions/share'), function(filename) { + FileActions.register('all', 'Share', OC.PERMISSION_SHARE, OC.imagePath('core', 'actions/share'), function(filename) { var tr = FileList.findFileEl(filename); var itemType = 'file'; if ($(tr).data('type') == 'dir') { diff --git a/apps/files_sharing/lib/permissions.php b/apps/files_sharing/lib/permissions.php index c3ad63e2fd..f32ebabe40 100644 --- a/apps/files_sharing/lib/permissions.php +++ b/apps/files_sharing/lib/permissions.php @@ -30,6 +30,7 @@ class Shared_Permissions extends Permissions { * @return int (-1 if file no permissions set) */ public function get($fileId, $user) { + if ($fileId == -1) { // if we ask for the mount point return -1 so that we can get the correct // permissions by the path, with the root fileId we have no idea which share is meant @@ -37,11 +38,14 @@ class Shared_Permissions extends Permissions { } $source = \OCP\Share::getItemSharedWithBySource('file', $fileId, \OC_Share_Backend_File::FORMAT_SHARED_STORAGE, null, true); + + $permission = -1; + if ($source) { - return $source['permissions']; - } else { - return -1; + $permission = $this->updatePermissions($source['permissions']); } + + return $permission; } /** @@ -55,7 +59,7 @@ class Shared_Permissions extends Permissions { $source = \OCP\Share::getItemSharedWithBySource('file', $fileId, \OC_Share_Backend_File::FORMAT_SHARED_STORAGE, null, false); if ($source) { - return $source['permissions']; + return $this->updatePermissions($source['permissions']); } else { return -1; } @@ -106,7 +110,7 @@ class Shared_Permissions extends Permissions { $result = $query->execute(array($parentId)); $filePermissions = array(); while ($row = $result->fetchRow()) { - $filePermissions[$row['fileid']] = $permissions; + $filePermissions[$row['fileid']] = $this->updatePermissions($permissions); } return $filePermissions; } diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index c18e30966f..07a0acf00a 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -108,6 +108,11 @@ class Shared extends \OC\Files\Storage\Common { if (pathinfo($target, PATHINFO_EXTENSION) === 'part') { $permissions |= \OCP\PERMISSION_DELETE; } + + if (\OC_Util::isSharingDisabledForUser()) { + $permissions &= ~\OCP\PERMISSION_SHARE; + } + return $permissions; } @@ -198,6 +203,9 @@ class Shared extends \OC\Files\Storage\Common { } public function isSharable($path) { + if (\OCP\Util::isSharingDisabledForUser()) { + return false; + } return ($this->getPermissions($path) & \OCP\PERMISSION_SHARE); } diff --git a/apps/files_sharing/tests/api.php b/apps/files_sharing/tests/api.php index dc07c6fc62..6d0ed434ef 100644 --- a/apps/files_sharing/tests/api.php +++ b/apps/files_sharing/tests/api.php @@ -171,6 +171,78 @@ class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { $appConfig->setValue('core', 'shareapi_enforce_links_password', 'no'); } + /** + * @medium + */ + function testSharePermissions() { + + // sharing file to a user should work if shareapi_exclude_groups is set + // to no + \OC_Appconfig::setValue('core', 'shareapi_exclude_groups', 'no'); + $_POST['path'] = $this->filename; + $_POST['shareWith'] = \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2; + $_POST['shareType'] = \OCP\Share::SHARE_TYPE_USER; + + $result = Share\Api::createShare(array()); + + $this->assertTrue($result->succeeded()); + $data = $result->getData(); + + $share = $this->getShareFromId($data['id']); + + $items = \OCP\Share::getItemShared('file', $share['item_source']); + + $this->assertTrue(!empty($items)); + + $fileinfo = $this->view->getFileInfo($this->filename); + + $result = \OCP\Share::unshare('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); + + $this->assertTrue($result); + + // exclude groups, but not the group the user belongs to. Sharing should still work + \OC_Appconfig::setValue('core', 'shareapi_exclude_groups', 'yes'); + \OC_Appconfig::setValue('core', 'shareapi_exclude_groups_list', 'admin,group1,group2'); + + $_POST['path'] = $this->filename; + $_POST['shareWith'] = \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2; + $_POST['shareType'] = \OCP\Share::SHARE_TYPE_USER; + + $result = Share\Api::createShare(array()); + + $this->assertTrue($result->succeeded()); + $data = $result->getData(); + + $share = $this->getShareFromId($data['id']); + + $items = \OCP\Share::getItemShared('file', $share['item_source']); + + $this->assertTrue(!empty($items)); + + $fileinfo = $this->view->getFileInfo($this->filename); + + $result = \OCP\Share::unshare('file', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER, + \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2); + + $this->assertTrue($result); + + // now we exclude the group the user belongs to ('group'), sharing should fail now + \OC_Appconfig::setValue('core', 'shareapi_exclude_groups_list', 'admin,group'); + + $_POST['path'] = $this->filename; + $_POST['shareWith'] = \Test_Files_Sharing_Api::TEST_FILES_SHARING_API_USER2; + $_POST['shareType'] = \OCP\Share::SHARE_TYPE_USER; + + $result = Share\Api::createShare(array()); + + $this->assertFalse($result->succeeded()); + + // cleanup + \OC_Appconfig::setValue('core', 'shareapi_exclude_groups', 'no'); + \OC_Appconfig::setValue('core', 'shareapi_exclude_groups_list', ''); + } + /** * @medium diff --git a/apps/files_sharing/tests/base.php b/apps/files_sharing/tests/base.php index 7cd36b9d41..34ec4a36ed 100644 --- a/apps/files_sharing/tests/base.php +++ b/apps/files_sharing/tests/base.php @@ -109,6 +109,8 @@ abstract class Test_Files_Sharing_Base extends \PHPUnit_Framework_TestCase { if ($create) { \OC_User::createUser($user, $password); + \OC_Group::createGroup('group'); + \OC_Group::addToGroup($user, 'group'); } \OC_Util::tearDownFS(); diff --git a/apps/files_sharing/tests/proxy.php b/apps/files_sharing/tests/proxy.php index 402402082d..634ed86db5 100644 --- a/apps/files_sharing/tests/proxy.php +++ b/apps/files_sharing/tests/proxy.php @@ -25,9 +25,9 @@ require_once __DIR__ . '/base.php'; use OCA\Files\Share; /** - * Class Test_Files_Sharing_Api + * Class Test_Files_Sharing_Proxy */ -class Test_Files_Sharing_Api extends Test_Files_Sharing_Base { +class Test_Files_Sharing_Proxy extends Test_Files_Sharing_Base { const TEST_FOLDER_NAME = '/folder_share_api_test'; diff --git a/core/js/config.php b/core/js/config.php index 33665b8401..80b1b6d242 100644 --- a/core/js/config.php +++ b/core/js/config.php @@ -80,6 +80,7 @@ $array = array( 'defaultExpireDate' => $defaultExpireDate, 'defaultExpireDateEnforced' => $enforceDefaultExpireDate, 'enforcePasswordForPublicLink' => \OCP\Util::isPublicLinkPasswordRequired(), + 'sharingDisabledForUser' => \OCP\Util::isSharingDisabledForUser(), ) ) ), diff --git a/lib/private/files/cache/permissions.php b/lib/private/files/cache/permissions.php index 2e2bdb20b7..eba18af386 100644 --- a/lib/private/files/cache/permissions.php +++ b/lib/private/files/cache/permissions.php @@ -36,7 +36,7 @@ class Permissions { $sql = 'SELECT `permissions` FROM `*PREFIX*permissions` WHERE `user` = ? AND `fileid` = ?'; $result = \OC_DB::executeAudited($sql, array($user, $fileId)); if ($row = $result->fetchRow()) { - return $row['permissions']; + return $this->updatePermissions($row['permissions']); } else { return -1; } @@ -78,7 +78,7 @@ class Permissions { $result = \OC_DB::executeAudited($sql, $params); $filePermissions = array(); while ($row = $result->fetchRow()) { - $filePermissions[$row['fileid']] = $row['permissions']; + $filePermissions[$row['fileid']] = $this->updatePermissions($row['permissions']); } return $filePermissions; } @@ -99,7 +99,7 @@ class Permissions { $result = \OC_DB::executeAudited($sql, array($parentId, $user)); $filePermissions = array(); while ($row = $result->fetchRow()) { - $filePermissions[$row['fileid']] = $row['permissions']; + $filePermissions[$row['fileid']] = $this->updatePermissions($row['permissions']); } return $filePermissions; } @@ -140,4 +140,17 @@ class Permissions { } return $users; } + + /** + * check if admin removed the share permission for the user and update the permissions + * + * @param int $permissions + * @return int + */ + protected function updatePermissions($permissions) { + if (\OCP\Util::isSharingDisabledForUser()) { + $permissions &= ~\OCP\PERMISSION_SHARE; + } + return $permissions; + } } diff --git a/lib/private/files/storage/common.php b/lib/private/files/storage/common.php index fef33cabd8..b03ae7d051 100644 --- a/lib/private/files/storage/common.php +++ b/lib/private/files/storage/common.php @@ -81,6 +81,10 @@ abstract class Common implements \OC\Files\Storage\Storage { } public function isSharable($path) { + if (\OC_Util::isSharingDisabledForUser()) { + return false; + } + return $this->isReadable($path); } diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 16bc492d38..46796c2637 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -485,15 +485,23 @@ class Share extends \OC\Share\Constants { $itemSourceName = $itemSource; } - // verify that the file exists before we try to share it + // check if file can be shared if ($itemType === 'file' or $itemType === 'folder') { $path = \OC\Files\Filesystem::getPath($itemSource); + // verify that the file exists before we try to share it if (!$path) { $message = 'Sharing %s failed, because the file does not exist'; $message_t = $l->t('Sharing %s failed, because the file does not exist', array($itemSourceName)); \OC_Log::write('OCP\Share', sprintf($message, $itemSourceName), \OC_Log::ERROR); throw new \Exception($message_t); } + // verify that the user has share permission + if (!\OC\Files\Filesystem::isSharable($path)) { + $message = 'You are not allowed to share %s'; + $message_t = $l->t('You are not allowed to share %s', array($itemSourceName)); + \OC_Log::write('OCP\Share', sprintf($message, $itemSourceName), \OC_Log::ERROR); + throw new \Exception($message_t); + } } //verify that we don't share a folder which already contains a share mount point diff --git a/lib/private/util.php b/lib/private/util.php index c018721afe..23c7053002 100755 --- a/lib/private/util.php +++ b/lib/private/util.php @@ -96,6 +96,29 @@ class OC_Util { return ($enforcePassword === 'yes') ? true : false; } + /** + * check if sharing is disabled for the current user + * + * @return boolean + */ + public static function isSharingDisabledForUser() { + if (\OC_Appconfig::getValue('core', 'shareapi_exclude_groups', 'no') === 'yes') { + $user = \OCP\User::getUser(); + $groupsList = \OC_Appconfig::getValue('core', 'shareapi_exclude_groups_list', ''); + $excludedGroups = explode(',', $groupsList); + $usersGroups = \OC_Group::getUserGroups($user); + if (!empty($usersGroups)) { + $remainingGroups = array_diff($usersGroups, $excludedGroups); + // if the user is only in groups which are disabled for sharing then + // sharing is also disabled for the user + if (empty($remainingGroups)) { + return true; + } + } + } + return false; + } + /** * Get the quota of a user * @param string $user diff --git a/lib/public/util.php b/lib/public/util.php index 3166d4040d..d1faec3997 100644 --- a/lib/public/util.php +++ b/lib/public/util.php @@ -116,6 +116,15 @@ class Util { } } + /** + * check if sharing is disabled for the current user + * + * @return boolean + */ + public static function isSharingDisabledForUser() { + return \OC_Util::isSharingDisabledForUser(); + } + /** * get l10n object * @param string $application diff --git a/settings/admin.php b/settings/admin.php index f9406246e7..699ba97edd 100755 --- a/settings/admin.php +++ b/settings/admin.php @@ -10,6 +10,7 @@ OC_Util::checkAdminUser(); OC_Util::addStyle( "settings", "settings" ); OC_Util::addScript( "settings", "admin" ); OC_Util::addScript( "settings", "log" ); +OC_Util::addScript( 'core', 'multiselect' ); OC_App::setActiveNavigationEntry( "admin" ); $tmpl = new OC_Template( 'settings', 'admin', 'user'); @@ -48,6 +49,23 @@ $tmpl->assign('shareAPIEnabled', OC_Appconfig::getValue('core', 'shareapi_enable $tmpl->assign('shareDefaultExpireDateSet', OC_Appconfig::getValue('core', 'shareapi_default_expire_date', 'no')); $tmpl->assign('shareExpireAfterNDays', OC_Appconfig::getValue('core', 'shareapi_expire_after_n_days', '7')); $tmpl->assign('shareEnforceExpireDate', OC_Appconfig::getValue('core', 'shareapi_enforce_expire_date', 'no')); +$excludeGroups = OC_Appconfig::getValue('core', 'shareapi_exclude_groups', 'no') === 'yes' ? true : false; +$tmpl->assign('shareExcludeGroups', $excludeGroups); +$allGroups = OC_Group::getGroups(); +$excludedGroupsList = OC_Appconfig::getValue('core', 'shareapi_exclude_groups_list', ''); +$excludedGroups = $excludedGroupsList !== '' ? explode(',', $excludedGroupsList) : array(); +$groups = array(); +foreach ($allGroups as $group) { + if (in_array($group, $excludedGroups)) { + $groups[$group] = array('gid' => $group, + 'excluded' => true); + } else { + $groups[$group] = array('gid' => $group, + 'excluded' => false); + } +} +ksort($groups); +$tmpl->assign('groups', $groups); // Check if connected using HTTPS diff --git a/settings/ajax/excludegroups.php b/settings/ajax/excludegroups.php new file mode 100644 index 0000000000..2934a448a6 --- /dev/null +++ b/settings/ajax/excludegroups.php @@ -0,0 +1,18 @@ +' + + escapeHTML(group) + ''); + } + }); + }; + + var label = null; + element.multiSelect({ + createCallback: addGroup, + createText: label, + selectedFirst: true, + checked: checked, + oncheck: checkHandeler, + onuncheck: checkHandeler, + minWidth: 100 + }); + + } + } +}; + $(document).ready(function(){ + + $('select#excludedGroups[multiple]').each(function (index, element) { + SharingGroupList.applyMultipleSelect($(element)); + }); + + $('#loglevel').change(function(){ $.post(OC.filePath('settings','ajax','setloglevel.php'), { level: $(this).val() },function(){ OC.Log.reload(); @@ -84,4 +129,8 @@ $(document).ready(function(){ OC.msg.finishedAction('#sendtestmail_msg', data); }); }); + + $('#shareapiExcludeGroups').change(function() { + $("#selectExcludedGroups").toggleClass('hidden', !this.checked); + }); }); diff --git a/settings/routes.php b/settings/routes.php index 21d406beec..0e0f293b9b 100644 --- a/settings/routes.php +++ b/settings/routes.php @@ -84,3 +84,5 @@ $this->create('settings_admin_mail_test', '/settings/admin/mailtest') ->action('OC\Settings\Admin\Controller', 'sendTestMail'); $this->create('settings_ajax_setsecurity', '/settings/ajax/setsecurity.php') ->actionInclude('settings/ajax/setsecurity.php'); +$this->create('settings_ajax_excludegroups', '/settings/ajax/excludegroups.php') + ->actionInclude('settings/ajax/excludegroups.php'); diff --git a/settings/templates/admin.php b/settings/templates/admin.php index d1f519a072..a4a3698d3b 100644 --- a/settings/templates/admin.php +++ b/settings/templates/admin.php @@ -240,9 +240,6 @@ if (!$_['internetconnectionworking']) { t('Allow users to share items to the public with links')); ?> - - - @@ -271,7 +268,24 @@ if (!$_['internetconnectionworking']) { t('Allow users to send mail notification for shared files')); ?> - + + > + /> +
+
+ +
+ t('These groups will still be able to receive shares, but not to initiate them.')); ?> + + diff --git a/tests/lib/util.php b/tests/lib/util.php index c4780cc5f4..4dc7813d91 100644 --- a/tests/lib/util.php +++ b/tests/lib/util.php @@ -235,4 +235,59 @@ class Test_Util extends PHPUnit_Framework_TestCase { array(' .', false), ); } + + /** + * @dataProvider dataProviderForTestIsSharingDisabledForUser + * @param array $groups existing groups + * @param array $membership groups the user belong to + * @param array $excludedGroups groups which should be excluded from sharing + * @param bool $expected expected result + */ + function testIsSharingDisabledForUser($groups, $membership, $excludedGroups, $expected) { + $uid = "user1"; + \OC_User::setUserId($uid); + + \OC_User::createUser($uid, "passwd"); + + foreach($groups as $group) { + \OC_Group::createGroup($group); + } + + foreach($membership as $group) { + \OC_Group::addToGroup($uid, $group); + } + + $appConfig = \OC::$server->getAppConfig(); + $appConfig->setValue('core', 'shareapi_exclude_groups_list', implode(',', $excludedGroups)); + $appConfig->setValue('core', 'shareapi_exclude_groups', 'yes'); + + $result = \OCP\Util::isSharingDisabledForUser(); + + $this->assertSame($expected, $result); + + // cleanup + \OC_User::deleteUser($uid); + \OC_User::setUserId(''); + + foreach($groups as $group) { + \OC_Group::deleteGroup($group); + } + + $appConfig->setValue('core', 'shareapi_exclude_groups_list', ''); + $appConfig->setValue('core', 'shareapi_exclude_groups', 'no'); + + } + + public function dataProviderForTestIsSharingDisabledForUser() { + return array( + // existing groups, groups the user belong to, groups excluded from sharing, expected result + array(array('g1', 'g2', 'g3'), array(), array('g1'), false), + array(array('g1', 'g2', 'g3'), array(), array(), false), + array(array('g1', 'g2', 'g3'), array('g2'), array('g1'), false), + array(array('g1', 'g2', 'g3'), array('g2'), array(), false), + array(array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1'), false), + array(array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1', 'g2'), true), + array(array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1', 'g2', 'g3'), true), + ); + } }