diff --git a/apps/files_external/lib/Controller/GlobalStoragesController.php b/apps/files_external/lib/Controller/GlobalStoragesController.php index 9ee3c91783d9..14abdafa2bc2 100644 --- a/apps/files_external/lib/Controller/GlobalStoragesController.php +++ b/apps/files_external/lib/Controller/GlobalStoragesController.php @@ -86,15 +86,6 @@ public function create( $applicableGroups, $priority ) { - $canCreateNewLocalStorage = \OC::$server->getConfig()->getSystemValue('files_external_allow_create_new_local', false); - - if ($backend === 'local' && $canCreateNewLocalStorage === false) { - return new DataResponse( - null, - Http::STATUS_FORBIDDEN - ); - } - $newStorage = $this->createStorage( $mountPoint, $backend, diff --git a/apps/files_external/lib/Controller/StoragesController.php b/apps/files_external/lib/Controller/StoragesController.php index 9b4fdf85ab9e..21de089ab642 100644 --- a/apps/files_external/lib/Controller/StoragesController.php +++ b/apps/files_external/lib/Controller/StoragesController.php @@ -183,7 +183,7 @@ protected function validate(IStorageConfig $storage) { $backend->getIdentifier() ]) ], - Http::STATUS_UNPROCESSABLE_ENTITY + Http::STATUS_FORBIDDEN ); } if (!$authMechanism->isVisibleFor($this->service->getVisibilityType())) { @@ -194,7 +194,7 @@ protected function validate(IStorageConfig $storage) { $authMechanism->getIdentifier() ]) ], - Http::STATUS_UNPROCESSABLE_ENTITY + Http::STATUS_FORBIDDEN ); } @@ -308,7 +308,7 @@ public function show($id, $testOnly = true) { } catch (NotFoundException $e) { return new DataResponse( [ - 'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id]) + 'message' => (string)$this->l10n->t('Storage with id "%d" not found', [$id]) ], Http::STATUS_NOT_FOUND ); @@ -335,7 +335,7 @@ public function destroy($id) { } catch (NotFoundException $e) { return new DataResponse( [ - 'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id]) + 'message' => (string)$this->l10n->t('Storage with id "%d" not found', [$id]) ], Http::STATUS_NOT_FOUND ); diff --git a/apps/files_external/lib/Controller/UserStoragesController.php b/apps/files_external/lib/Controller/UserStoragesController.php index 8c7a1e4efaae..126483a0dc45 100644 --- a/apps/files_external/lib/Controller/UserStoragesController.php +++ b/apps/files_external/lib/Controller/UserStoragesController.php @@ -31,7 +31,6 @@ use OCP\Files\External\IStorageConfig; use OCP\Files\External\NotFoundException; use OCP\Files\External\Service\IUserStoragesService; -use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; use OCP\IRequest; @@ -45,7 +44,6 @@ class UserStoragesController extends StoragesController { * @var IUserSession */ private $userSession; - private IConfig $config; public function __construct( $AppName, @@ -53,7 +51,6 @@ public function __construct( IL10N $l10n, IUserStoragesService $userStoragesService, IUserSession $userSession, - IConfig $config, ILogger $logger ) { parent::__construct( @@ -64,7 +61,6 @@ public function __construct( $logger ); $this->userSession = $userSession; - $this->config = $config; } protected function manipulateStorageConfig(IStorageConfig $storage) { @@ -82,12 +78,6 @@ protected function manipulateStorageConfig(IStorageConfig $storage) { * @return DataResponse */ public function index() { - if (!$this->isUserMountingAllowed()) { - return new DataResponse( - null, - Http::STATUS_FORBIDDEN - ); - } return parent::index(); } @@ -122,20 +112,6 @@ public function create( $backendOptions, $mountOptions ) { - if (!$this->isUserMountingAllowed()) { - return new DataResponse( - null, - Http::STATUS_FORBIDDEN - ); - } - $canCreateNewLocalStorage = \OC::$server->getConfig()->getSystemValue('files_external_allow_create_new_local', false); - if ($backend === 'local' && $canCreateNewLocalStorage === false) { - return new DataResponse( - null, - Http::STATUS_FORBIDDEN - ); - } - $newStorage = $this->createStorage( $mountPoint, $backend, @@ -188,12 +164,6 @@ public function update( $mountOptions, $testOnly = true ) { - if (!$this->isUserMountingAllowed()) { - return new DataResponse( - null, - Http::STATUS_FORBIDDEN - ); - } $storage = $this->createStorage( $mountPoint, $backend, @@ -241,17 +211,6 @@ public function update( * {@inheritdoc} */ public function destroy($id) { - if (!$this->isUserMountingAllowed()) { - return new DataResponse( - null, - Http::STATUS_FORBIDDEN - ); - } - return parent::destroy($id); } - - private function isUserMountingAllowed(): bool { - return $this->config->getAppValue('files_external', 'allow_user_mounting', 'no') === 'yes'; - } } diff --git a/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php b/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php index 6e45b6ca927f..3417a3b0695e 100644 --- a/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php +++ b/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php @@ -111,6 +111,38 @@ public function testCreate() { $this->assertEquals($expectedStorage, $actual); } + public function testCreateLocal() { + $mount = 'randomMount'; + $backend = 'local'; + $auth = 'identifier:\Random\Missing\Auth\Class'; + $backendOpts = [ + 'datadir' => '/tmp', + ]; + $priority = 3; + + // there is already a teardown in the parent class setting this value to false + \OC::$server->getSystemConfig()->setValue('files_external_allow_create_new_local', false); + + $result = $this->controller->create($mount, $backend, $auth, $backendOpts, [], [], [], $priority); + $this->assertEquals(Http::STATUS_FORBIDDEN, $result->getStatus()); + } + + public function testCreateLocalClassname() { + $mount = 'randomMount'; + $backend = '\OC\Files\Storage\Local'; + $auth = 'identifier:\Random\Missing\Auth\Class'; + $backendOpts = [ + 'datadir' => '/tmp', + ]; + $priority = 3; + + // there is already a teardown in the parent class setting this value to false + \OC::$server->getSystemConfig()->setValue('files_external_allow_create_new_local', false); + + $result = $this->controller->create($mount, $backend, $auth, $backendOpts, [], [], [], $priority); + $this->assertEquals(Http::STATUS_FORBIDDEN, $result->getStatus()); + } + public function testUpdate() { $mount = 'randomMount'; $backend = 'identifier:\This\Doesnt\Exist'; diff --git a/apps/files_external/tests/Controller/StoragesControllerTest.php b/apps/files_external/tests/Controller/StoragesControllerTest.php index 97cc11a0c3a8..3df57c9d04df 100644 --- a/apps/files_external/tests/Controller/StoragesControllerTest.php +++ b/apps/files_external/tests/Controller/StoragesControllerTest.php @@ -170,7 +170,7 @@ public function testAddStorageWithoutConfig() { $data = $response->getData(); $this->assertEquals(Http::STATUS_FORBIDDEN, $response->getStatus()); - $this->assertNull($data); + $this->assertEquals(['message' => ''], $data); // message comes from translatable error string, which isn't set here. } public function testUpdateStorage() { diff --git a/apps/files_external/tests/Controller/UserStoragesControllerTest.php b/apps/files_external/tests/Controller/UserStoragesControllerTest.php index c20722b73c4f..be4bfade78d5 100644 --- a/apps/files_external/tests/Controller/UserStoragesControllerTest.php +++ b/apps/files_external/tests/Controller/UserStoragesControllerTest.php @@ -32,7 +32,6 @@ use OCP\Files\External\Service\IStoragesService; use OCP\Files\StorageNotAvailableException; use OCP\ILogger; -use OCP\IConfig; use OCP\IUserSession; use OCP\IL10N; use OCP\IRequest; @@ -45,44 +44,16 @@ public function setUp(): void { $this->service->method('getVisibilityType') ->willReturn(IStoragesBackendService::VISIBILITY_PERSONAL); - $this->config = $this->createMock(IConfig::class); - $this->config->method('getAppValue')->willReturn('yes'); - $this->controller = new UserStoragesController( 'files_external', $this->createMock(IRequest::class), $this->createMock(IL10N::class), $this->service, $this->createMock(IUserSession::class), - $this->config, $this->createMock(ILogger::class) ); } - public function testApiWhenDisabled(): void { - $config = $this->createMock(IConfig::class); - $config->method('getAppValue')->willReturn('no'); - - $controller = new UserStoragesController( - 'files_external', - $this->createMock(IRequest::class), - $this->createMock(IL10N::class), - $this->service, - $this->createMock(IUserSession::class), - $config, - $this->createMock(ILogger::class) - ); - - $resp = $controller->create( - '', - '', - '', - [], - [], - ); - $this->assertEquals(Http::STATUS_FORBIDDEN, $resp->getStatus()); - } - public function testAddOrUpdateStorageDisallowedBackend() { $backend = $this->getBackendMock(); $backend->method('isVisibleFor') @@ -115,7 +86,7 @@ public function testAddOrUpdateStorageDisallowedBackend() { null ); - $this->assertEquals(Http::STATUS_UNPROCESSABLE_ENTITY, $response->getStatus()); + $this->assertEquals(Http::STATUS_FORBIDDEN, $response->getStatus()); $response = $this->controller->update( 1, @@ -129,7 +100,7 @@ public function testAddOrUpdateStorageDisallowedBackend() { null ); - $this->assertEquals(Http::STATUS_UNPROCESSABLE_ENTITY, $response->getStatus()); + $this->assertEquals(Http::STATUS_FORBIDDEN, $response->getStatus()); } public function testCreate() { @@ -195,6 +166,114 @@ public function testCreate() { $this->assertEquals($expectedStorage, $actual); } + public function localBackendNameProvider() { + return [ + ['local'], + ['\OC\Files\Storage\Local'], + ]; + } + + /** + * @dataProvider localBackendNameProvider + */ + public function testCreateLocal($localBackendName) { + $mount = 'randomMount'; + $backend = "identifier:{$localBackendName}"; + $auth = 'identifier:\Random\Missing\Auth\Class'; + $backendOpts = [ + 'datadir' => '/tmp', + ]; + $priority = 3; + + $storageConfig = $this->getNewStorageConfigMock([ + 'id' => 30, + 'backendClass' => '\OCA\Files_External\Lib\Backend', + 'backendStorageClass' => '\OC\Files\Storage\Local', + 'authClass' => '\Random\Missing\Auth\Class', + 'mountPoint' => $mount, + 'backendOpts' => $backendOpts, + 'priority' => $priority, + 'type' => IStorageConfig::MOUNT_TYPE_ADMIN, + ]); + + $backendMock = $storageConfig->getBackend(); + $backendMock->method('isVisibleFor')->willReturn(true); + $backendMock->method('validateStorage')->willReturn(true); + + $authMock = $storageConfig->getAuthMechanism(); + $authMock->method('isVisibleFor')->willReturn(true); + $authMock->method('validateStorage')->willReturn(true); + + $this->service->expects($this->once()) + ->method('createStorage') + ->willReturn($storageConfig); + $this->service->expects($this->once()) + ->method('addStorage') + ->will($this->returnArgument(0)); + + $expectedStorage = [ + 'id' => 30, + 'mountPoint' => '/randomMount', + 'backend' => "identifier:\OCA\Files_External\Lib\Backend", + 'authMechanism' => 'identifier:\Random\Missing\Auth\Class', + 'backendOptions' => [ + 'datadir' => '/tmp', + ], + 'priority' => 3, + 'userProvided' => false, + 'type' => 'system', + 'status' => StorageNotAvailableException::STATUS_SUCCESS, // status check for the storage is skipped and always returns this value + ]; + + $result = $this->controller->create($mount, $backend, $auth, $backendOpts, [], [], [], $priority); + $actual = $result->getData()->jsonSerialize(); + $this->assertEquals(Http::STATUS_CREATED, $result->getStatus()); + $this->assertEquals($expectedStorage, $actual); + } + + /** + * @dataProvider localBackendNameProvider + */ + public function testCreateLocalNotAllowed($localBackendName) { + $mount = 'randomMount'; + $backend = $localBackendName; + $auth = 'identifier:\Random\Missing\Auth\Class'; + $backendOpts = [ + 'datadir' => '/tmp', + ]; + $priority = 3; + + $storageConfig = $this->getNewStorageConfigMock([ + 'id' => 30, + 'backendClass' => '\OCA\Files_External\Lib\Backend', + 'backendStorageClass' => '\OC\Files\Storage\Local', + 'authClass' => '\Random\Missing\Auth\Class', + 'mountPoint' => $mount, + 'backendOpts' => $backendOpts, + 'priority' => $priority, + 'type' => IStorageConfig::MOUNT_TYPE_ADMIN, + ]); + + // if not allowed, the backend must return false for its visibility + $backendMock = $storageConfig->getBackend(); + $backendMock->method('isVisibleFor')->willReturn(false); + $backendMock->method('validateStorage')->willReturn(true); + + $authMock = $storageConfig->getAuthMechanism(); + $authMock->method('isVisibleFor')->willReturn(true); + $authMock->method('validateStorage')->willReturn(true); + + $this->service->expects($this->once()) + ->method('createStorage') + ->willReturn($storageConfig); + $this->service->expects($this->never()) + ->method('addStorage') + ->will($this->returnArgument(0)); + + $result = $this->controller->create($mount, $backend, $auth, $backendOpts, [], [], [], $priority); + $this->assertEquals(Http::STATUS_FORBIDDEN, $result->getStatus()); + } + public function testUpdate() { $mount = 'randomMount'; $backend = 'identifier:\This\Doesnt\Exist'; diff --git a/changelog/unreleased/41538 b/changelog/unreleased/41538 new file mode 100644 index 000000000000..167725ad0666 --- /dev/null +++ b/changelog/unreleased/41538 @@ -0,0 +1,8 @@ +Bugfix: Prevent mounting local storage if not allowed. + +Mounting a local storage was possible if the internal class name was used as +backend, despite local storage not allowed to be mounted. This problem is +fixed and the local storage can't be mounted if it was explicitly disallowed in +the configuration. + +https://github.com/owncloud/core/pull/41538 diff --git a/lib/private/Files/External/StoragesBackendChecker.php b/lib/private/Files/External/StoragesBackendChecker.php new file mode 100644 index 000000000000..3b48bb0b4f87 --- /dev/null +++ b/lib/private/Files/External/StoragesBackendChecker.php @@ -0,0 +1,87 @@ +config = $config; + } + + /** + * Checks if the regular users are allowed to mount external storages + * @return bool + */ + public function isUserMountingAllowed(): bool { + if ($this->allowUserMounting === null) { + $this->allowUserMounting = $this->config->getAppValue('files_external', 'allow_user_mounting', 'no') === 'yes'; + // if no backend is in the list an empty string is in the array and user mounting is disabled + if ($this->allowedBackendsForUsers() === ['']) { + $this->allowUserMounting = false; + } + } + return $this->allowUserMounting; + } + + private function allowedBackendsForUsers() { + if ($this->userMountingBackends === null) { + $user_mounting_backends = $this->config->getAppValue('files_external', 'user_mounting_backends', ''); + $this->userMountingBackends = \explode( + ',', + $user_mounting_backends + ); + } + return $this->userMountingBackends; + } + + /** + * Checks if the regular users are allowed to mount the specified backend. + * Note that the admin might still mount the backend. + * @param Backend $backend + * @return bool + */ + public function isAllowedUserBackend(Backend $backend): bool { + // Allowing users to mount local storage is a security risk, so it's + // blacklisted regardless of admin's decision. The admin can still + // setup a local mount for everyone (or chosen users) if he wants. + $blacklistedBackendsForUsers = ['\OC\Files\Storage\Local']; + if (\in_array($backend->getStorageClass(), $blacklistedBackendsForUsers, true)) { + return false; + } + + if ($this->isUserMountingAllowed() && \array_intersect($backend->getIdentifierAliases(), $this->allowedBackendsForUsers())) { + return true; + } + return false; + } + + /** + * Checks if the admin is allowed to mount the specified backend. + * @param Backend $backend + * @return bool + */ + public function isAllowedAdminBackend(Backend $backend): bool { + if ($this->canCreateNewLocalStorage === null) { + $this->canCreateNewLocalStorage = $this->config->getSystemValue('files_external_allow_create_new_local', false); + } + + if ($backend->getStorageClass() === '\OC\Files\Storage\Local' && !$this->canCreateNewLocalStorage) { + return false; + } + return true; + } +} diff --git a/lib/private/Files/External/StoragesBackendService.php b/lib/private/Files/External/StoragesBackendService.php index 712a6a286a16..0accca21db33 100644 --- a/lib/private/Files/External/StoragesBackendService.php +++ b/lib/private/Files/External/StoragesBackendService.php @@ -25,6 +25,7 @@ use OCP\IConfig; +use OC\Files\External\StoragesBackendChecker; use OCP\Files\External\Backend\Backend; use OCP\Files\External\Auth\AuthMechanism; use OCP\Files\External\Config\IBackendProvider; @@ -35,14 +36,8 @@ * Service class to manage backend definitions */ class StoragesBackendService implements IStoragesBackendService { - /** @var IConfig */ - protected $config; - - /** @var bool */ - private $userMountingAllowed = true; - - /** @var string[] */ - private $userMountingBackends = []; + /** @var StoragesBackendChecker */ + protected $storagesBackendChecker; /** @var Backend[] */ private $backends = []; @@ -57,27 +52,12 @@ class StoragesBackendService implements IStoragesBackendService { private $authMechanismProviders = []; /** - * @param IConfig $config + * @param StoragesBackendChecker $storagesBackendChecker */ public function __construct( - IConfig $config + StoragesBackendChecker $storagesBackendChecker ) { - $this->config = $config; - - // Load config values - if ($this->config->getAppValue('files_external', 'allow_user_mounting', 'no') !== 'yes') { - $this->userMountingAllowed = false; - } - $user_mounting_backends = $this->config->getAppValue('files_external', 'user_mounting_backends', ''); - $this->userMountingBackends = \explode( - ',', - $user_mounting_backends - ); - - // if no backend is in the list an empty string is in the array and user mounting is disabled - if ($this->userMountingBackends === ['']) { - $this->userMountingAllowed = false; - } + $this->storagesBackendChecker = $storagesBackendChecker; } /** @@ -124,6 +104,9 @@ public function registerBackend(Backend $backend) { if (!$this->isAllowedUserBackend($backend)) { $backend->removeVisibility(IStoragesBackendService::VISIBILITY_PERSONAL); } + if (!$this->isAllowedAdminBackend($backend)) { + $backend->removeVisibility(IStoragesBackendService::VISIBILITY_ADMIN); + } foreach ($backend->getIdentifierAliases() as $alias) { $this->backends[$alias] = $backend; } @@ -244,7 +227,7 @@ public function getAuthMechanism($identifier) { * @return bool */ public function isUserMountingAllowed() { - return $this->userMountingAllowed; + return $this->storagesBackendChecker->isUserMountingAllowed(); } /** @@ -254,12 +237,16 @@ public function isUserMountingAllowed() { * @return bool */ protected function isAllowedUserBackend(Backend $backend) { - if ($this->userMountingAllowed && - \array_intersect($backend->getIdentifierAliases(), $this->userMountingBackends) - ) { - return true; - } - return false; + return $this->storagesBackendChecker->isAllowedUserBackend($backend); + } + + /** + * Checks if the admin is allowed to mount the backend + * @param Backend $backend + * @return bool + */ + protected function isAllowedAdminBackend(Backend $backend) { + return $this->storagesBackendChecker->isAllowedAdminBackend($backend); } /** diff --git a/lib/private/Server.php b/lib/private/Server.php index 3cc8bfa8cd78..142cb6812d77 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -114,6 +114,7 @@ use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use OC\Files\External\StoragesBackendService; +use OC\Files\External\StoragesBackendChecker; use OC\Files\External\Service\UserStoragesService; use OC\Files\External\Service\UserGlobalStoragesService; use OC\Files\External\Service\GlobalStoragesService; @@ -844,7 +845,7 @@ public function __construct($webRoot, \OC\Config $config) { ); }); $this->registerService('StoragesBackendService', function (Server $c) { - $service = new StoragesBackendService($c->query('AllConfig')); + $service = new StoragesBackendService($c->query(StoragesBackendChecker::class)); // register auth mechanisms provided by core $provider = new \OC\Files\External\Auth\CoreAuthMechanismProvider($c, [ diff --git a/tests/lib/Files/External/Service/GlobalStoragesServiceDeleteUserTest.php b/tests/lib/Files/External/Service/GlobalStoragesServiceDeleteUserTest.php index 67e36a61510f..3fa744dfaeca 100644 --- a/tests/lib/Files/External/Service/GlobalStoragesServiceDeleteUserTest.php +++ b/tests/lib/Files/External/Service/GlobalStoragesServiceDeleteUserTest.php @@ -23,6 +23,7 @@ use OC\Files\Config\UserMountCache; use OC\Files\External\Service\DBConfigService; use OC\Files\External\Service\GlobalStoragesService; +use OC\Files\External\StoragesBackendChecker; use OC\Files\External\StoragesBackendService; use OCP\Files\External\Backend\Backend; use OCP\IUser; @@ -125,7 +126,7 @@ public function providesDeleteAllUser() { * @param $storageParams */ public function testDeleteAllForUser($storageParams, $userId) { - $backendService = new StoragesBackendService(\OC::$server->getConfig()); + $backendService = new StoragesBackendService(new StoragesBackendChecker(\OC::$server->getConfig())); $dbConfigService = new DBConfigService(\OC::$server->getDatabaseConnection(), \OC::$server->getCrypto()); $userManager = \OC::$server->getUserManager(); $userMountCache = new UserMountCache(\OC::$server->getDatabaseConnection(), $userManager, \OC::$server->getLogger()); diff --git a/tests/lib/Files/External/StoragesBackendCheckerTest.php b/tests/lib/Files/External/StoragesBackendCheckerTest.php new file mode 100644 index 000000000000..8197687380af --- /dev/null +++ b/tests/lib/Files/External/StoragesBackendCheckerTest.php @@ -0,0 +1,125 @@ +config = $this->createMock(IConfig::class); + + $this->storagesBackendChecker = new StoragesBackendChecker($this->config); + } + + public function isUserMountingAllowedProvider() { + return [ + ['no', '', false], + ['no', 'local,smb', false], + ['yes', '', false], + ['yes', 'local,smb', true], + ['yes', 'smb', true], + ['', 'local,smb', false], + ['random', 'local,smb', false], + ]; + } + + /** + * @dataProvider isUserMountingAllowedProvider + */ + public function testIsUserMountingAllowed($allowUserMounting, $allowedBackends, $expectedResult) { + $this->config->method('getAppValue')->willReturnCallback(function ($app, $key, $default) use ($allowUserMounting, $allowedBackends) { + $map = [ + 'allow_user_mounting' => $allowUserMounting, + 'user_mounting_backends' => $allowedBackends, + ]; + if ($app === 'files_external') { + if (isset($map[$key])) { + return $map[$key]; + } + } + return $default; + }); + + $this->assertEquals($expectedResult, $this->storagesBackendChecker->isUserMountingAllowed()); + } + + public function isAllowedUserBackendProvider() { + $backend1 = $this->createMock(Backend::class); + $backend1->method('getStorageClass')->willReturn('\OC\Files\Storage\DAV'); + $backend1->method('getIdentifierAliases')->willReturn(['\OCA\Files_External\Lib\Backend\DAV', 'DAV']); + $backend2 = $this->createMock(Backend::class); + $backend2->method('getStorageClass')->willReturn('\OC\Files\Storage\Local'); + $backend2->method('getIdentifierAliases')->willReturn(['\OCA\Files_External\Lib\Backend\Local', 'local']); + return [ + ['no', '', $backend1, false], + ['no', '', $backend2, false], + ['no', 'DAV,local', $backend1, false], + ['no', 'DAV,local', $backend2, false], + ['no', '\OCA\Files_External\Lib\Backend\DAV,\OCA\Files_External\Lib\Backend\Local', $backend1, false], + ['no', '\OCA\Files_External\Lib\Backend\DAV,\OCA\Files_External\Lib\Backend\Local', $backend2, false], + ['yes', '', $backend1, false], + ['yes', '', $backend2, false], + ['yes', 'DAV,local', $backend1, true], + ['yes', 'DAV,local', $backend2, false], // local storage always disallowed + ['yes', '\OCA\Files_External\Lib\Backend\DAV,\OCA\Files_External\Lib\Backend\Local', $backend1, true], + ['yes', '\OCA\Files_External\Lib\Backend\DAV,\OCA\Files_External\Lib\Backend\Local', $backend2, false], + ]; + } + + /** + * @dataProvider isAllowedUserBackendProvider + */ + public function testIsAllowedUserBackend($allowUserMounting, $allowedBackends, $backend, $expectedResult) { + $this->config->method('getAppValue')->willReturnCallback(function ($app, $key, $default) use ($allowUserMounting, $allowedBackends) { + $map = [ + 'allow_user_mounting' => $allowUserMounting, + 'user_mounting_backends' => $allowedBackends, + ]; + if ($app === 'files_external') { + if (isset($map[$key])) { + return $map[$key]; + } + } + return $default; + }); + + $this->assertEquals($expectedResult, $this->storagesBackendChecker->isAllowedUserBackend($backend)); + } + + public function isAllowedAdminBackendProvider() { + $backend1 = $this->createMock(Backend::class); + $backend1->method('getStorageClass')->willReturn('\OC\Files\Storage\DAV'); + $backend1->method('getIdentifierAliases')->willReturn(['\OCA\Files_External\Lib\Backend\DAV', 'DAV']); + $backend2 = $this->createMock(Backend::class); + $backend2->method('getStorageClass')->willReturn('\OC\Files\Storage\Local'); + $backend2->method('getIdentifierAliases')->willReturn(['\OCA\Files_External\Lib\Backend\Local', 'local']); + return [ + [false, $backend1, true], + [false, $backend2, false], + [true, $backend1, true], + [true, $backend2, true], + ]; + } + + /** + * @dataProvider isAllowedAdminBackendProvider + */ + public function testIsAllowedAdminBackend($isLocalAllowed, $backend, $expectedResult) { + $this->config->method('getSystemValue')->willReturnCallback(function ($key, $default) use ($isLocalAllowed) { + if ($key === 'files_external_allow_create_new_local') { + return $isLocalAllowed; + } + return $default; + }); + + $this->assertEquals($expectedResult, $this->storagesBackendChecker->isAllowedAdminBackend($backend)); + } +} diff --git a/tests/lib/Files/External/StoragesBackendServiceTest.php b/tests/lib/Files/External/StoragesBackendServiceTest.php index f61dacd8dcda..31e2e5fb4590 100644 --- a/tests/lib/Files/External/StoragesBackendServiceTest.php +++ b/tests/lib/Files/External/StoragesBackendServiceTest.php @@ -20,22 +20,21 @@ */ namespace Test\Files\External; +use OC\Files\External\StoragesBackendChecker; use OC\Files\External\StoragesBackendService; use OCP\Files\External\IStoragesBackendService; -use OCP\IConfig; +use OCP\Files\External\Backend\Backend; use PHPUnit\Framework\MockObject\MockObject; class StoragesBackendServiceTest extends \Test\TestCase { - /** @var \OCP\IConfig */ - protected $config; + /** @var StoragesBackendChecker */ + protected $storagesBackendChecker; protected function setUp(): void { - $this->config = $this->createMock('\OCP\IConfig'); - $this->config - ->method('getAppValue') - ->willReturnMap([ - ['files_external', 'user_mounting_backends', '', ''] - ]); + $this->storagesBackendChecker = $this->createMock(StoragesBackendChecker::class); + $this->storagesBackendChecker->method('isUserMountingAllowed')->willReturn(false); + $this->storagesBackendChecker->method('isAllowedUserBackend')->willReturn(false); + $this->storagesBackendChecker->method('isAllowedAdminBackend')->willReturn(true); } /** @@ -67,7 +66,7 @@ protected function getAuthMechanismMock($class) { } public function testRegisterBackend() { - $service = new StoragesBackendService($this->config); + $service = new StoragesBackendService($this->storagesBackendChecker); $backend = $this->getBackendMock('\Foo\Bar'); @@ -94,7 +93,7 @@ public function testRegisterBackend() { } public function testBackendProvider() { - $service = new StoragesBackendService($this->config); + $service = new StoragesBackendService($this->storagesBackendChecker); $backend1 = $this->getBackendMock('\Foo\Bar'); $backend2 = $this->getBackendMock('\Bar\Foo'); @@ -112,7 +111,7 @@ public function testBackendProvider() { } public function testAuthMechanismProvider() { - $service = new StoragesBackendService($this->config); + $service = new StoragesBackendService($this->storagesBackendChecker); $backend1 = $this->getAuthMechanismMock('\Foo\Bar'); $backend2 = $this->getAuthMechanismMock('\Bar\Foo'); @@ -130,7 +129,7 @@ public function testAuthMechanismProvider() { } public function testMultipleBackendProviders() { - $service = new StoragesBackendService($this->config); + $service = new StoragesBackendService($this->storagesBackendChecker); $backend1a = $this->getBackendMock('\Foo\Bar'); $backend1b = $this->getBackendMock('\Bar\Foo'); @@ -156,15 +155,18 @@ public function testMultipleBackendProviders() { } public function testUserMountingBackends() { - $config = $this->createMock(IConfig::class); - $config->expects($this->exactly(2)) - ->method('getAppValue') - ->willReturnMap([ - ['files_external', 'allow_user_mounting', 'no', 'yes'], - ['files_external', 'user_mounting_backends', '', 'identifier:\User\Mount\Allowed,identifier_alias'] - ]); - - $service = new StoragesBackendService($config); + $storagesBackendChecker = $this->createMock(StoragesBackendChecker::class); + $storagesBackendChecker->method('isUserMountingAllowed')->willReturn(true); + $storagesBackendChecker->method('isAllowedAdminBackend')->willReturn(true); + $storagesBackendChecker->method('isAllowedUserBackend')->willReturnCallback(function (Backend $backend) { + $backendAliases = $backend->getIdentifierAliases(); + if (\in_array('identifier:\User\Mount\Allowed', $backendAliases, true) || \in_array('identifier_alias', $backendAliases, true)) { + return true; + } + return false; + }); + + $service = new StoragesBackendService($storagesBackendChecker); $backendAllowed = $this->getBackendMock('\User\Mount\Allowed'); $backendAllowed->expects($this->never()) @@ -187,8 +189,89 @@ public function testUserMountingBackends() { $service->registerBackend($backendAlias); } + public function testAdminMountingBackends() { + $storagesBackendChecker = $this->createMock(StoragesBackendChecker::class); + $storagesBackendChecker->method('isUserMountingAllowed')->willReturn(true); + $storagesBackendChecker->method('isAllowedUserBackend')->willReturn(true); + $storagesBackendChecker->method('isAllowedAdminBackend')->willReturnCallback(function (Backend $backend) { + $backendAliases = $backend->getIdentifierAliases(); + if (\in_array('identifier:\User\Mount\Allowed', $backendAliases, true) || \in_array('identifier_alias', $backendAliases, true)) { + return true; + } + return false; + }); + + $service = new StoragesBackendService($storagesBackendChecker); + + $backendAllowed = $this->getBackendMock('\User\Mount\Allowed'); + $backendAllowed->expects($this->never()) + ->method('removeVisibility'); + $backendNotAllowed = $this->getBackendMock('\User\Mount\NotAllowed'); + $backendNotAllowed->expects($this->once()) + ->method('removeVisibility') + ->with(IStoragesBackendService::VISIBILITY_ADMIN); + + $backendAlias = $this->getMockBuilder('\OCP\Files\External\Backend\Backend') + ->disableOriginalConstructor() + ->getMock(); + $backendAlias->method('getIdentifierAliases') + ->willReturn(['identifier_real', 'identifier_alias']); + $backendAlias->expects($this->never()) + ->method('removeVisibility'); + + $service->registerBackend($backendAllowed); + $service->registerBackend($backendNotAllowed); + $service->registerBackend($backendAlias); + } + + public function testAdminAndUserMountingBackends() { + $storagesBackendChecker = $this->createMock(StoragesBackendChecker::class); + $storagesBackendChecker->method('isUserMountingAllowed')->willReturn(true); + $storagesBackendChecker->method('isAllowedUserBackend')->willReturnCallback(function (Backend $backend) { + $backendAliases = $backend->getIdentifierAliases(); + if (\in_array('identifier:\User\Mount\Allowed', $backendAliases, true) || \in_array('identifier_alias', $backendAliases, true)) { + return true; + } + return false; + }); + $storagesBackendChecker->method('isAllowedAdminBackend')->willReturnCallback(function (Backend $backend) { + $backendAliases = $backend->getIdentifierAliases(); + if (\in_array('identifier:\User\Mount\Allowed', $backendAliases, true) || \in_array('identifier_alias', $backendAliases, true)) { + return true; + } + return false; + }); + + $service = new StoragesBackendService($storagesBackendChecker); + + $backendAllowed = $this->getBackendMock('\User\Mount\Allowed'); + $backendAllowed->expects($this->never()) + ->method('removeVisibility'); + $backendNotAllowed = $this->getBackendMock('\User\Mount\NotAllowed'); + $backendNotAllowed->expects($this->exactly(2)) + ->method('removeVisibility') + ->with( + $this->logicalOr( + $this->identicalTo(IStoragesBackendService::VISIBILITY_ADMIN), + $this->identicalTo(IStoragesBackendService::VISIBILITY_PERSONAL) + ) + ); + + $backendAlias = $this->getMockBuilder('\OCP\Files\External\Backend\Backend') + ->disableOriginalConstructor() + ->getMock(); + $backendAlias->method('getIdentifierAliases') + ->willReturn(['identifier_real', 'identifier_alias']); + $backendAlias->expects($this->never()) + ->method('removeVisibility'); + + $service->registerBackend($backendAllowed); + $service->registerBackend($backendNotAllowed); + $service->registerBackend($backendAlias); + } + public function testGetAvailableBackends() { - $service = new StoragesBackendService($this->config); + $service = new StoragesBackendService($this->storagesBackendChecker); $backendAvailable = $this->getBackendMock('\Backend\Available'); $backendAvailable->expects($this->once())