Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions apps/files_external/css/settings.css
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,13 @@ td.mountPoint, td.backend { width:160px; }
#externalStorage .mountOptionsDropdown {
margin-right: 40px;
}

#externalStorage td.configuration .error-invalid,
#externalStorage td.authentication .error-invalid {
white-space: normal;

}

#externalStorage tr.invalid {
background-color: #F2DEDE;
}
33 changes: 33 additions & 0 deletions apps/files_external/js/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,19 @@ MountConfigListView.prototype = _.extend({
newStorage: function(storageConfig, onCompletion) {
var mountPoint = storageConfig.mountPoint;
var backend = this._allBackends[storageConfig.backend];
var isInvalidAuth = false;

if (!backend) {
backend = {
name: 'Unknown backend: ' + storageConfig.backend,
invalid: true
};
}
if (backend && storageConfig.authMechanism && !this._allAuthMechanisms[storageConfig.authMechanism]) {
// mark invalid to prevent editing
backend.invalid = true;
isInvalidAuth = true;
}

// FIXME: Replace with a proper Handlebar template
var $tr = this.$el.find('tr#addMountPoint');
Expand Down Expand Up @@ -836,6 +849,26 @@ MountConfigListView.prototype = _.extend({
$tr.addClass(backend.identifier);
$tr.find('.backend').data('identifier', backend.identifier);

if (backend.invalid || isInvalidAuth) {
$tr.addClass('invalid');
$tr.find('[name=mountPoint]').prop('disabled', true);
$tr.find('.applicable,.mountOptionsToggle').empty();
this.updateStatus($tr, false, 'Unknown backend: ' + backend.name);
if (isInvalidAuth) {
$tr.find('td.authentication').append('<span class="error-invalid">' +
t('files_external', 'Unknown auth backend "{b}"', {b: storageConfig.authMechanism}) +
'</span>'
);
}

$tr.find('td.configuration').append('<span class="error-invalid">' +
t('files_external', 'Please make sure that the app that provides this backend is installed and enabled') +
'</span>'
);

return $tr;
}

var selectAuthMechanism = $('<select class="selectAuthMechanism"></select>');
var neededVisibility = (this._isPersonal) ? StorageConfig.Visibility.PERSONAL : StorageConfig.Visibility.ADMIN;
$.each(this._allAuthMechanisms, function(authIdentifier, authMechanism) {
Expand Down
16 changes: 15 additions & 1 deletion apps/files_external/lib/Command/ListCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use OCP\Files\External\Backend\InvalidBackend;
use OCP\Files\External\Auth\InvalidAuth;

class ListCommand extends Base {
/**
Expand Down Expand Up @@ -192,7 +194,11 @@ public function listMounts($userId, array $mounts, InputInterface $input, Output
'enable_sharing' => false,
'encoding_compatibility' => false
];
$rows = array_map(function (IStorageConfig $config) use ($userId, $defaultMountOptions, $full) {
$countInvalid = 0;
$rows = array_map(function (IStorageConfig $config) use ($userId, $defaultMountOptions, $full, &$countInvalid) {
if ($config->getBackend() instanceof InvalidBackend || $config->getAuthMechanism() instanceof InvalidAuth) {
$countInvalid++;
}
$storageConfig = $config->getBackendOptions();
$keys = array_keys($storageConfig);
$values = array_values($storageConfig);
Expand Down Expand Up @@ -256,6 +262,14 @@ public function listMounts($userId, array $mounts, InputInterface $input, Output
$table->setHeaders($headers);
$table->setRows($rows);
$table->render();

if ($countInvalid > 0) {
$output->writeln(
"<error>Number of invalid storages found: $countInvalid.\n" .
"The listed configuration details are likely incomplete.\n" .
"Please make sure that all related apps that provide these storages are enabled or delete these.</error>"
);
}
}
}

Expand Down
23 changes: 23 additions & 0 deletions apps/files_external/tests/Command/ListCommandTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use OC\Files\External\StorageConfig;
use OCP\Files\External\IStorageConfig;
use Symfony\Component\Console\Output\BufferedOutput;
use OCP\Files\External\Backend\InvalidBackend;

class ListCommandTest extends CommandTest {
/**
Expand Down Expand Up @@ -71,4 +72,26 @@ public function testListAuthIdentifier() {

$this->assertNotEquals($output[0]['authentication_type'], $output[1]['authentication_type']);
}

public function testDisplayWarningForIncomplete() {
$l10n = $this->createMock('\OCP\IL10N', null, [], '', false);
$session = $this->createMock('\OCP\ISession');
$crypto = $this->createMock('\OCP\Security\ICrypto');
$instance = $this->getInstance();
// FIXME: use mock of IStorageConfig
$mount1 = new StorageConfig();
$mount1->setAuthMechanism(new Password());
$mount1->setBackend(new InvalidBackend('InvalidId'));
$mount2 = new StorageConfig();
$mount2->setAuthMechanism(new SessionCredentials($session, $crypto));
$mount2->setBackend(new Local($l10n, new NullMechanism()));
$input = $this->getInput($instance);
$output = new BufferedOutput();

$instance->listMounts('', [$mount1, $mount2], $input, $output);
$output = $output->fetch();

$lines = explode($output, "\n");
$this->assertRegexp('/Number of invalid storages found/', $output);
}
}
31 changes: 31 additions & 0 deletions lib/private/Files/External/InvalidStorage.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php
/**
* @author Vincent Petry <[email protected]>
*
* @copyright Copyright (c) 2017, ownCloud GmbH.
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace OC\Files\External;

use OCP\Files\StorageNotAvailableException;
use OC\Files\Storage\FailedStorage;

class InvalidStorage extends FailedStorage {
public function __construct($params) {
throw new StorageNotAvailableException();
Copy link
Member

Choose a reason for hiding this comment

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

This seems too "artificial" for me. Creating a class that would throw an exception in the constructor implies that you want to throw the exception in the first place, which makes me wonder why do we need to create a class for that.

It probably makes more sense if the exception is thrown in specific methods even if those methods are almost all of them. My point is that the InvalidStorage should behave as another storage, but that exception doesn't allow it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual reason is that at the location where we specify the class name we don't actually instantiate it. Remember that the external storage backend class must specify a name for the storage class to be instantiated, it doesn't instantiate it itself, but MountPoint->createStorage will instantiate it later. Throwing an exception in the constructor is to make sure this never happens.

Copy link
Member

Choose a reason for hiding this comment

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

My worry is that this is something that wasn't originally planned.

Due to there isn't an oficial storage factory, other apps like WND have to create the storages on their own. Throwing the exception like that will likely affect them even if the storage won't be used. I'll have to recheck the behaviour.
Creating the storage and throwing the exception in the rest of the methods seems a more compatible option.

Copy link
Member

Choose a reason for hiding this comment

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

ok, it seems WND will check the backend class and won't try to create an storage instance if it isn't for the WND backend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a StorageFactory class: https://github.com/owncloud/core/blob/v10.0.3/lib/private/Files/Storage/StorageFactory.php and here is the code that instantiates storages: https://github.com/owncloud/core/blob/v10.0.3/lib/private/Files/Mount/MountPoint.php#L138 (the $loader is actually an instance of StorageFactory.

In general third party apps should never instantiate the storage class themselves.

The use case of this InvalidStorage class should also never be accessible for apps as it's designed for use by core when accessing a storage for which the actual storage class does not exist due to the app being disabled.

Copy link
Member

Choose a reason for hiding this comment

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

I'm out of arguments against it, but it still feels like adding tomato to the spaghetti code... 🤕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hahaha, it does.

Unfortunately the backends came after the storages. If we designed storages to have backends first, we could have designed it so that the backend is responsible of instantiating storages, in which case this mess above wouldn't required.

}
}
12 changes: 9 additions & 3 deletions lib/private/Files/External/Service/StoragesService.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
use OCP\Files\External\IStoragesBackendService;
use OCP\Files\External\NotFoundException;
use OCP\Files\External\Service\IStoragesService;
use OCP\Files\External\Backend\InvalidBackend;
use OCP\Files\External\Auth\InvalidAuth;

/**
* Service class to manage external storages
Expand Down Expand Up @@ -307,11 +309,11 @@ public function createStorage(
) {
$backend = $this->backendService->getBackend($backendIdentifier);
if (!$backend) {
throw new \InvalidArgumentException('Unable to get backend for ' . $backendIdentifier);
$backend = new InvalidBackend($backendIdentifier);
}
$authMechanism = $this->backendService->getAuthMechanism($authMechanismIdentifier);
if (!$authMechanism) {
throw new \InvalidArgumentException('Unable to get authentication mechanism for ' . $authMechanismIdentifier);
$authMechanism = new InvalidAuth($authMechanismIdentifier);
}
$newStorage = $this->createConfig();
$newStorage->setMountPoint($mountPoint);
Expand Down Expand Up @@ -389,11 +391,15 @@ public function updateStorage(IStorageConfig $updatedStorage) {
$existingMount = $this->dbConfig->getMountById($id);

if (!is_array($existingMount)) {
throw new NotFoundException('Storage with id "' . $id . '" not found while updating storage');
throw new NotFoundException('Storage config with id "' . $id . '" not found while updating storage');
}

$oldStorage = $this->getStorageConfigFromDBMount($existingMount);

if ($oldStorage->getBackend() instanceof InvalidBackend) {
throw new NotFoundException('Storage config with id "' . $id . '" and backend id "' . $oldStorage->getBackend()->getInvalidId() . '" cannot be edited due to missing backend');
}

$removedUsers = array_diff($oldStorage->getApplicableUsers(), $updatedStorage->getApplicableUsers());
$removedGroups = array_diff($oldStorage->getApplicableGroups(), $updatedStorage->getApplicableGroups());
$addedUsers = array_diff($updatedStorage->getApplicableUsers(), $oldStorage->getApplicableUsers());
Expand Down
47 changes: 47 additions & 0 deletions lib/public/Files/External/Auth/InvalidAuth.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php
/**
* @author Vincent Petry <[email protected]>
*
* @copyright Copyright (c) 2017, ownCloud GmbH.
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace OCP\Files\External\Auth;

/**
* Invalid authentication representing an auth mechanism
* that could not be resolved
*
* @since 10.0.5
*/
class InvalidAuth extends AuthMechanism {

/**
* Constructs a new InvalidAuth with the id of the invalid auth
* for display purposes
*
* @param string $invalidId invalid id
*
* @since 10.0.5
*/
public function __construct($invalidId) {
$this->setIdentifier($invalidId)
->setScheme(self::SCHEME_NULL)
->setText('Unknown auth mechanism backend ' . $invalidId)
;
}

}
64 changes: 64 additions & 0 deletions lib/public/Files/External/Backend/InvalidBackend.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php
/**
* @author Vincent Petry <[email protected]>
*
* @copyright Copyright (c) 2017, ownCloud GmbH.
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace OCP\Files\External\Backend;

use \OCP\IL10N;

/**
* Invalid storage backend representing a backend
* that could not be resolved
*
* @since 10.0.5
*/
class InvalidBackend extends Backend {

/** @var string Invalid backend id */
private $invalidId;

/**
* Constructs a new InvalidBackend with the id of the invalid backend
* for display purposes
*
* @param string $invalidId id of the backend that did not exist
*
* @since 10.0.5
*/
function __construct($invalidId) {
$this->invalidId = $invalidId;
$this->setIdentifier($invalidId)
->setStorageClass('\OC\Files\External\InvalidStorage')
->setText('Unknown storage backend ' . $invalidId)
;
}

/**
* Returns the invalid backend id
*
* @return string invalid backend id
*
* @since 10.0.5
*/
public function getInvalidId() {
return $this->invalidId;
}
}

34 changes: 34 additions & 0 deletions tests/lib/Files/External/InvalidStorageTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php
/**
* @author Vincent Petry <[email protected]>
*
* @copyright Copyright (c) 2017, ownCloud GmbH.
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace Test\Files\External;

use Test\TestCase;
use OC\Files\External\InvalidStorage;

class InvalidStorageTest extends TestCase {

/**
* @expectedException OCP\Files\StorageNotAvailableException
*/
public function testCannotInstantiate() {
new InvalidStorage([]);
}
}
Loading