Skip to content

Filter getStorage to make sure the user has access to it #21468

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
DeepDiver1975 merged 1 commit into from
Jan 8, 2016
Merged

Filter getStorage to make sure the user has access to it #21468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions apps/files_external/lib/storageconfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
* External storage configuration
*/
class StorageConfig implements \JsonSerializable {
const MOUNT_TYPE_ADMIN = 1;
const MOUNT_TYPE_PERSONAl = 2;

/**
* Storage config id
Expand Down Expand Up @@ -107,6 +109,13 @@ class StorageConfig implements \JsonSerializable {
*/
private $mountOptions = [];

/**
* Whether it's a personal or admin mount
*
* @var int
*/
private $type;

/**
* Creates a storage config
*
Expand Down Expand Up @@ -349,6 +358,20 @@ public function setStatus($status, $message = null) {
$this->statusMessage = $message;
}

/**
* @return int self::MOUNT_TYPE_ADMIN or self::MOUNT_TYPE_PERSONAl
*/
public function getType() {
return $this->type;
}

/**
* @param int $type self::MOUNT_TYPE_ADMIN or self::MOUNT_TYPE_PERSONAl
*/
public function setType($type) {
$this->type = $type;
}

/**
* Serialize config to JSON
*
Expand Down
4 changes: 4 additions & 0 deletions apps/files_external/service/globalstoragesservice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,8 @@ protected function triggerChangeHooks(StorageConfig $oldStorage, StorageConfig $
public function getVisibilityType() {
return BackendService::VISIBILITY_ADMIN;
}

protected function isApplicable(StorageConfig $config) {
return true;
}
}
19 changes: 15 additions & 4 deletions apps/files_external/service/storagesservice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,9 @@

namespace OCA\Files_external\Service;

use \OCP\IUserSession;
use \OC\Files\Filesystem;

use \OCA\Files_external\Lib\StorageConfig;
use \OCA\Files_external\NotFoundException;
use \OCA\Files_External\Service\BackendService;
use \OCA\Files_External\Lib\Backend\Backend;
use \OCA\Files_External\Lib\Auth\AuthMechanism;
use \OCP\Files\StorageNotAvailableException;
Expand Down Expand Up @@ -85,6 +82,7 @@ protected function getStorageConfigFromDBMount(array $mount) {
array_values($applicableGroups),
$mount['priority']
);
$config->setType($mount['type']);
$config->setId((int)$mount['mount_id']);
return $config;
} catch (\UnexpectedValueException $e) {
Expand Down Expand Up @@ -132,9 +130,22 @@ public function getStorage($id) {
throw new NotFoundException('Storage with id "' . $id . '" not found');
}

return $this->getStorageConfigFromDBMount($mount);
$config = $this->getStorageConfigFromDBMount($mount);
if ($this->isApplicable($config)) {
return $config;
} else {
throw new NotFoundException('Storage with id "' . $id . '" not found');

@RobinMcCorkell RobinMcCorkell Jan 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we not throw some sort of AccessDeniedException, rather than lying about the existence of a storage?

@icewind1991 icewind1991 Jan 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NotFoundException is consistent with 8.2 behavior

}
}

/**
* Check whether this storage service should provide access to a storage
*
* @param StorageConfig $config
* @return bool
*/
abstract protected function isApplicable(StorageConfig $config);

@LukasReschke LukasReschke Jan 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PHPDoc 😄


/**
* Gets all storages, valid or not
*
Expand Down
18 changes: 18 additions & 0 deletions apps/files_external/service/userglobalstoragesservice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,22 @@ protected function getPriorityType(StorageConfig $storage) {
return 0;
}

protected function isApplicable(StorageConfig $config) {
$applicableUsers = $config->getApplicableUsers();
$applicableGroups = $config->getApplicableGroups();

if (count($applicableUsers) === 0 && count($applicableGroups) === 0) {

@PVince81 PVince81 Jan 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd have used empty(), but it doesn't matter 😄

return true;
}
if (in_array($this->getUser()->getUID(), $applicableUsers, true)) {
return true;
}
$groupIds = $this->groupManager->getUserGroupIds($this->getUser());
foreach ($groupIds as $groupId) {
if (in_array($groupId, $applicableGroups, true)) {
return true;
}
}
return false;
}
}
4 changes: 4 additions & 0 deletions apps/files_external/service/userstoragesservice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,8 @@ public function updateStorage(StorageConfig $updatedStorage) {
public function getVisibilityType() {
return BackendService::VISIBILITY_PERSONAL;
}

protected function isApplicable(StorageConfig $config) {
return ($config->getApplicableUsers() === [$this->getUser()->getUID()]) && $config->getType() === StorageConfig::MOUNT_TYPE_PERSONAl;
}
}
8 changes: 8 additions & 0 deletions apps/files_external/tests/service/userglobalstoragesservicetest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
*/
namespace OCA\Files_External\Tests\Service;

use OCA\Files_external\NotFoundException;
use OCA\Files_external\Service\StoragesService;
use \OCA\Files_External\Service\UserGlobalStoragesService;
use \OCP\IGroupManager;
Expand Down Expand Up @@ -140,6 +141,13 @@ public function testGetStorageWithApplicable($applicableUsers, $applicableGroups
$this->assertEquals('/mountpoint', $retrievedStorage->getMountPoint());
} else {
$this->assertEquals(0, count($storages));

try {
$this->service->getStorage($newStorage->getId());
$this->fail('Failed asserting that storage can\'t be accessed by id');
} catch (NotFoundException $e) {

@PVince81 PVince81 Jan 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to make it less ugly I usually create a variable $thrown = false and then set it to true here. Then add an assert after the try block to assert that $thrown is true.

}
}

}
Expand Down
30 changes: 30 additions & 0 deletions apps/files_external/tests/service/userstoragesservicetest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

use \OC\Files\Filesystem;

use OCA\Files_external\Service\GlobalStoragesService;
use OCA\Files_external\Service\StoragesService;
use \OCA\Files_external\Service\UserStoragesService;
use \OCA\Files_external\NotFoundException;
use \OCA\Files_external\Lib\StorageConfig;
Expand All @@ -38,9 +40,16 @@ class UserStoragesServiceTest extends StoragesServiceTest {

private $userId;

/**
* @var StoragesService
*/
protected $globalStoragesService;

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

$this->globalStoragesService = new GlobalStoragesService($this->backendService, $this->dbConfig);

$this->userId = $this->getUniqueID('user_');
$this->createUser($this->userId, $this->userId);
$this->user = \OC::$server->getUserManager()->get($this->userId);
Expand Down Expand Up @@ -174,4 +183,25 @@ public function testHooksRenameMountPoint() {
$this->userId
);
}

/**
* @expectedException \OCA\Files_external\NotFoundException
*/
public function testGetAdminStorage() {
$backend = $this->backendService->getBackend('identifier:\OCA\Files_External\Lib\Backend\SMB');
$authMechanism = $this->backendService->getAuthMechanism('identifier:\Auth\Mechanism');

$storage = new StorageConfig();
$storage->setMountPoint('mountpoint');
$storage->setBackend($backend);
$storage->setAuthMechanism($authMechanism);
$storage->setBackendOptions(['password' => 'testPassword']);
$storage->setApplicableUsers([$this->userId]);

$newStorage = $this->globalStoragesService->addStorage($storage);

$this->assertInstanceOf('\OCA\Files_external\Lib\StorageConfig', $this->globalStoragesService->getStorage($newStorage->getId()));

$this->service->getStorage($newStorage->getId());
}
}