Introduce BackendService for managing external storage backends #15914

RobinMcCorkell · 2015-04-28T12:37:23Z

Backends are registered to the BackendService through new data structures:

BackendConfig specifies the basic information for a backend: class, human-readable name, visibility to personal/admin, initial priority. It takes an array of BackendParameter's to specifiy the parameters

BackendParameter stores a parameter configuration for an external storage: name of parameter, human-readable name, type of parameter (text, password, hidden, checkbox), flags (optional or not).

When a BackendConfig has checkDependencies() called on it, it will return a (possibly empty) array of BackendDependency's that specify a dependency of the backend. If an empty array is returned it means there are no unhandled dependencies, so the backend can be used.

Storages in the StoragesController now get their parameters validated server-side (fixes a TODO).

TODO:

cc @PVince81 @icewind1991 @DeepDiver1975

DeepDiver1975 · 2015-04-28T12:46:09Z

apps/files_external/lib/backendconfig.php

+/**
+ * External storage backend configuration
+ */
+class BackendConfig implements \JsonSerializable {


@PVince81 shall we move this into core? In the long run files_external will die and continue to live as isolated apps.
The UI and necessary APIs have to be in core.

Yes, makes sense.

@DeepDiver1975 That was one of the main reasons for implementing this - moving BackendService and StorageService over to core is much easier than trying to port OC_Mount_Config over. We just need to make sure we get the API right - that's what the [WIP] tag is for! 😄

PVince81 · 2015-07-03T16:22:39Z

CC @jmaciasportela as this will be useful for future work on external storage

RobinMcCorkell · 2015-07-24T16:00:02Z

This is becoming quite large... if anyone wants to comment on the API, please do so! I'm trying to avoid too many inter-class dependencies, while still providing a clean interface that doesn't involve passing class names everywhere.

Tests are quite comprehensive at this point.

cc @PVince81 @icewind1991 @DeepDiver1975 @MorrisJobke @LukasReschke

LukasReschke · 2015-07-25T15:55:16Z

apps/files_external/lib/auth/imechanism.php

+	 *
+	 * @param array $params
+	 */
+	public function __construct($params);


array $params

PVince81 · 2015-07-27T13:13:41Z

apps/files_external/lib/backendparameter.php

+	const VALUE_TEXT = 0;
+	const VALUE_BOOLEAN = 1;
+	const VALUE_PASSWORD = 2;
+	const VALUE_HIDDEN = 3;


We might want another type VALUE_CUSTOM = 4.
A backend should be then be able to specify a custom type name, and then provide JS code to render and handle it.
This could be done separately, but keep it in mind in case it affects the way how params are rendered. I guess the PHP could only render a placeholder "div" and then trigger an event somehow to trigger the custom JS code to render it.

One example could be a dropdown or multiselect. Not sure what other kind of parameters we need, @jmaciasportela do you know ?

I don't see how a custom value constant here would be useful - do you mean creating a new placeholder prefix (eww) that is exposed to JS when that value type is set? Or that backends can specify the placeholder prefix they want to use (ewwwwww)? Custom stuff should be done in JS, IMHO.

Imagine if there was a time travelling storage where you can specify a date. That one would want to have a date picker, which is currently not possible. So it would specify the custom flag so no field would be rendered, but a value would still be read/written to the config. But maybe that could be achieved with a simple text field, which would be automatically replaced with a date picker by the custom JS code.

Ahh, I see, so no rendering of an input at all! Brilliant! /me scurries away to change it

Just noticed we have the "HIDDEN" one, maybe that one will already do.
So never mind.

Actually on second thoughts, it's not going to work very well. A custom field still needs positioning in the configuration area, so there needs to be some kind of placeholder element that the custom JS can replace, preserving the placement of the inputs. I think the easiest way to accomplish this is via the VALUE_HIDDEN type, since that won't be rendered yet still 'allocates' a space in the configuration area.

Thoughts?

Whether space is allocated isn't that bad. With jquery you can just insert elements anywhere.
Most important is that the value is properly read/written in the field.
If using a hidden field, the JS code for that field should make sure to update the value in the hidden field so it can be saved properly. (similar to how we do mount options)

Well, it's not necessary to keep the field as a hidden field - the custom JS can replace the element entirely. The advantage of having an element to start with is that the ordering of the fields is guaranteed, and no magic is required to figure out how to insert between the URL and Root fields for example

PVince81 · 2015-07-27T13:15:49Z

We might need a fallback OC_Mount_Config::registerBackend() for older / third party ext storage implementations. I'd consider this method public API even though the naming doesn't match.
The implementation would just be an adapter from the old format to the new one.
Then add @deprecated so we can remove it later.

PVince81 · 2015-07-27T13:16:50Z

apps/files_external/service/backendservice.php

+
+use \OCA\Files_External\Lib\BackendConfig;
+use \OCA\Files_External\Lib\AuthMechConfig;
+use \OCA\Files_External\Lib\BackendParameter as Param;


Remove as Param ? I didn't find any occurrence

RobinMcCorkell · 2015-07-27T14:15:28Z

Pluggable authentication mechanisms are here!

The 'Static password' mechanism is just a quick hack I threw together, which always returns the same password (in this case, the correct one for the user). Note that the password never gets stored in mount.json!

Selecting a different authentication mechanism from the dropdown re-creates the relevant configuration fields.

Next up: working with hooks. Some authentication mechanisms might want to listen for hooks (login, namely) and perform some action, such as updating mount.json, as a result. This will be tough without using static methods...

RobinMcCorkell · 2015-07-27T15:05:51Z

@PVince81 Awww, I hoped to avoid compatibility shims in OC_Mount_Config - eventually I want to kill that class completely.

PVince81 · 2015-07-27T15:07:53Z

We all do 😉
The problem is that without shim we can't merge your PR until all external storage backends have been adapted, even stuff like irods... so better keep compatible for now.

RobinMcCorkell · 2015-07-27T15:11:38Z

/me sees 'iRODS', runs away.

RobinMcCorkell · 2015-07-27T15:12:12Z

Actually to be fair, I stole my original implementation idea of SMB_OC from iRODS, so it had some use

RobinMcCorkell · 2015-07-27T19:34:11Z

15:00:13 PhantomJS 1.9.8 (Linux 0.0.0) ERROR
15:00:13   SyntaxError: Parse error
15:00:13   at /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files_external/js/settings.js:9

Line 9 is the beginning (function() {, and running JSHint + some other checkers locally doesn't flag any errors...

Firefox is happy with the file too

PVince81 · 2015-08-19T07:54:09Z

The blocking ticket #18109 was merged.

This is good to go 👍

MorrisJobke · 2015-08-19T08:08:39Z

apps/files_external/appinfo/routes.php

@@ -28,8 +28,7 @@
 /**
 * @var $this \OC\Route\Router
 **/
-$application = new Application();
-$application->registerRoutes(
+\OC_Mount_Config::$app->registerRoutes(


This causes an error on my side.

{"reqId":"\/ic4rLtLHtT1DuBLflnU","remoteAddr":"127.0.0.1","app":"PHP","message":"Class 'OC_Mount_Config' not found at \/home\/mjob\/Projekte\/owncloud\/master\/apps\/files_external\/appinfo\/routes.php#31","level":3,"time":"2015-08-19T08:05:22+00:00","method":"GET","url":"\/master\/index.php"}

I guess this is caused by the order of loaded files. The class file is hardcoded in the app.php.

Somehow this worked just very well for a long time, until I reset my whole instance and enabled the files_external from this branch without having the old one enabled.

You might need to merge with master to get the other fix.

Fixed with rebase

MorrisJobke · 2015-08-19T08:15:51Z

BUG: adding a local mount via personal page I got following error:

{"reqId":"oQb7DRZkUfcunqWk0Sy+","remoteAddr":"127.0.0.1","app":"PHP","message":"Argument 1 passed to OCA\\Files_External\\Lib\\PersonalMount::__construct() must be an instance of OCA\\Files_external\\Service\\UserStoragesService, instance of OC\\Files\\Storage\\Local given, called in \/home\/mjob\/Projekte\/owncloud\/master\/apps\/files_external\/lib\/config\/configadapter.php on line 148 and defined at \/home\/mjob\/Projekte\/owncloud\/master\/apps\/files_external\/lib\/personalmount.php#49","level":0,"time":"2015-08-19T08:13:01+00:00","method":"GET","url":"\/master\/index.php"}
{"reqId":"oQb7DRZkUfcunqWk0Sy+","remoteAddr":"127.0.0.1","app":"PHP","message":"Object of class OC\\Files\\Storage\\StorageFactory could not be converted to string at \/home\/mjob\/Projekte\/owncloud\/master\/lib\/private\/files\/filesystem.php#795","level":0,"time":"2015-08-19T08:13:01+00:00","method":"GET","url":"\/master\/index.php"}
{"reqId":"oQb7DRZkUfcunqWk0Sy+","remoteAddr":"127.0.0.1","app":"PHP","message":"Call to undefined method OCA\\Files_External\\Lib\\PersonalMount::attachStoragesService() at \/home\/mjob\/Projekte\/owncloud\/master\/apps\/files_external\/lib\/config\/configadapter.php#149","level":3,"time":"2015-08-19T08:13:01+00:00","method":"GET","url":"\/master\/index.php"}

$ cat data/mjob/mount.json
{
    "user": {
        "mjob": {
            "\/mjob\/files\/abc": {
                "id": 1,
                "backend": "\\OC\\Files\\Storage\\Local",
                "authMechanism": "builtin::builtin",
                "options": {
                    "datadir": "\/home\/mjob\/tm"
                },
                "storage_id": "3"
            }
        }
    }
}

PVince81 · 2015-08-19T08:24:00Z

@MorrisJobke weird, you shouldn't be able to select "Local" in the personal page.

MorrisJobke · 2015-08-19T08:28:22Z

@MorrisJobke weird, you shouldn't be able to select "Local" in the personal page.

I also wondered about it, because it was not available some time ago.

RobinMcCorkell · 2015-08-19T08:56:09Z

@PVince81 @MorrisJobke Unfortunately due to the way backend registration now works, all legacy storages are (by default) available to everyone. I'll patch in a quick fix for that, but it'll be ugly and will touch the legacy registration code a bit.

Backends are registered to the BackendService through new data structures: Backends are concrete classes, deriving from \OCA\Files_External\Lib\Backend\Backend. During construction, the various configuration parameters of the Backend can be set, in a design similar to Symfony Console. DefinitionParameter stores a parameter configuration for an external storage: name of parameter, human-readable name, type of parameter (text, password, hidden, checkbox), flags (optional or not). Storages in the StoragesController now get their parameters validated server-side (fixes a TODO).

UserGlobalStoragesService reads the global storage configuration, cherry-picking storages applicable to a user. Writing storages through this service is forbidden, on punishment of throwing an exception. Storage IDs may also be config hashes when retrieved from this service, as it is unable to update the storages with real IDs. As UserGlobalStoragesService and UserStoragesService share a bit of code relating to users, that has been split into UserTrait. UserTrait also allows for the user set to be overridden, rather than using the user from IUserSession. Config\ConfigAdapter has been reworked to use UserStoragesService and UserGlobalStoragesService instead of OC_Mount_Config::getAbsoluteMountPoints(), further reducing dependance on that horrible static class.

A backend can now specify generic authentication schemes that it supports, instead of specifying the parameters for its authentication method directly. This allows multiple authentication mechanisms to be implemented for a single scheme, providing altered functionality. This commit introduces the backend framework for this feature, and so at this point the UI will be broken as the frontend does not specify the required information. Terminology: - authentication scheme Parameter interface for the authentication method. A backend supporting the 'password' scheme accepts two parameters, 'user' and 'password'. - authentication mechanism Specific mechanism implementing a scheme. Basic mechanisms may forward configuration options directly to the backend, more advanced ones may lookup parameters or retrieve them from the session New dropdown selector for external storage configurations to select the authentication mechanism to be used. Authentication mechanisms can have visibilities, just like backends. The API was extended too to make it easier to add/remove visibilities. In addition, the concept of 'allowed visibility' has been introduced, so a backend/auth mechanism can force a maximum visibility level (e.g. Local storage type) that cannot be overridden by configuration in the web UI. An authentication mechanism is a fully instantiated implementation. This allows an implementation to have dependencies injected into it, e.g. an \OCP\IDB for database operations. When a StorageConfig is being prepared for mounting, the authentication mechanism implementation has manipulateStorage() called, which inserts the relevant authentication method options into the storage ready for mounting.

Prior to this, the storage class name was stored in mount.json under the "class" parameter, and the auth mechanism class name under the "authMechanism" parameter. This decouples the class name from the identifier used to retrieve the backend or auth mechanism. Now, backends/auth mechanisms have a unique identifier, which is saved in the "backend" or "authMechanism" parameter in mount.json respectively. An identifier is considered unique for the object it references, but the underlying class may change (e.g. files_external gets pulled into core and namespaces are modified).

The following functions have been removed: - addMountPoint() - removeMountPoint() - movePersonalMountPoint() registerBackend() has been rewritten as a shim around BackendService, allowing legacy code to interact with the new API seamlessly addMountPoint() was already disconnected from all production code, so this commit completes the job and removes the function itself, along with disconnecting and removing related functions. Unit tests have likewise been removed. getAbsoluteMountPoints(), getSystemMountPoints() and getPersonalMountPoints() have been rewritten to use the StoragesServices

The same Application must be used in the settings templates and in routes, so that any registered backends are correctly seen

Failure to prepare the storage during backend or auth mechanism manipulation will throw an InsufficientDataForMeaningfulAnswerException, which is propagated to StorageNotAvailableException in the filesystem layer via the FailedStorage helper class. When a storage is unavailable not due to failure, but due to insufficient data being available, a special 'indeterminate' status is returned to the configuration UI.

Loading custom JS on a per-backend basis added needless complexity and made dealing with async required. Now all backends/auth mechanisms load custom JS in PHP

scrutinizer-notifier · 2015-08-19T09:10:26Z

A new inspection was created.

RobinMcCorkell · 2015-08-19T09:11:31Z

@MorrisJobke That bug you found was caused by me changing PersonalMount in response to @icewind1991 's comment, but then forgetting to change the point that it actually gets used. Fixed now.

ghost · 2015-08-19T10:53:43Z

🚀 Test PASSed.🚀

RobinMcCorkell · 2015-08-19T11:18:26Z

@PVince81 Could you quickly review that last commit I pushed? Everything else is the same.

PVince81 · 2015-08-19T11:19:19Z

👍

PVince81 · 2015-08-19T11:19:30Z

@MorrisJobke retry?

MorrisJobke · 2015-08-19T12:15:25Z

Tested and works so far:+1:

Introduce BackendService for managing external storage backends

RobinMcCorkell · 2015-08-19T12:36:32Z

plzdontbreak plzdontbreak plzdontbreak 🙈

RobinMcCorkell added enhancement 2 - Developing app:files_external labels Apr 28, 2015

RobinMcCorkell added this to the 8.2-next milestone Apr 28, 2015

DeepDiver1975 reviewed Apr 28, 2015
View reviewed changes

RobinMcCorkell mentioned this pull request Jun 5, 2015

External Storage Interface: Selecting Username and Password Options #16305

Closed

5 tasks

RobinMcCorkell force-pushed the ext-backend-register branch from ffce5eb to f2e4a22 Compare June 9, 2015 18:58

RobinMcCorkell force-pushed the ext-backend-register branch from f2e4a22 to 7455ba1 Compare July 23, 2015 16:41

LukasReschke reviewed Jul 25, 2015
View reviewed changes

apps/files_external/lib/auth/imechanism.php

*

* @param array $params

*/

public function __construct($params);

Copy link

Member

LukasReschke Jul 25, 2015

Choose a reason for hiding this comment

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

array $params

RobinMcCorkell force-pushed the ext-backend-register branch from fa33486 to 717680e Compare July 27, 2015 13:10

PVince81 reviewed Jul 27, 2015
View reviewed changes

RobinMcCorkell force-pushed the ext-backend-register branch from 717680e to 2dd3668 Compare July 27, 2015 14:51

MorrisJobke reviewed Aug 19, 2015
View reviewed changes

Robin McCorkell added 10 commits August 19, 2015 10:05

Unit tests for new backend API

c572631

Make Application a singleton

c592e24

The same Application must be used in the settings templates and in routes, so that any registered backends are correctly seen

Load custom JS for all backends/auth mechanisms

df8f542

Loading custom JS on a per-backend basis added needless complexity and made dealing with async required. Now all backends/auth mechanisms load custom JS in PHP

Restrict Local backend to admin-only

eb0e5ce

RobinMcCorkell force-pushed the ext-backend-register branch from 496165c to eb0e5ce Compare August 19, 2015 09:10

PVince81 mentioned this pull request Aug 19, 2015

Exception when updating storage availability #18420

Closed

MorrisJobke added a commit that referenced this pull request Aug 19, 2015

Merge pull request #15914 from owncloud/ext-backend-register

a9bb6be

Introduce BackendService for managing external storage backends

MorrisJobke merged commit a9bb6be into master Aug 19, 2015

MorrisJobke deleted the ext-backend-register branch August 19, 2015 12:15

lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019

Introduce BackendService for managing external storage backends #15914

Introduce BackendService for managing external storage backends #15914

Conversation

RobinMcCorkell commented Apr 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 3, 2015

RobinMcCorkell commented Jul 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 27, 2015

Choose a reason for hiding this comment

RobinMcCorkell commented Jul 27, 2015

RobinMcCorkell commented Jul 27, 2015

PVince81 commented Jul 27, 2015

RobinMcCorkell commented Jul 27, 2015

RobinMcCorkell commented Jul 27, 2015

RobinMcCorkell commented Jul 27, 2015

PVince81 commented Aug 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Aug 19, 2015

PVince81 commented Aug 19, 2015

MorrisJobke commented Aug 19, 2015

RobinMcCorkell commented Aug 19, 2015

scrutinizer-notifier commented Aug 19, 2015

RobinMcCorkell commented Aug 19, 2015

ghost commented Aug 19, 2015

RobinMcCorkell commented Aug 19, 2015

PVince81 commented Aug 19, 2015

PVince81 commented Aug 19, 2015

MorrisJobke commented Aug 19, 2015

RobinMcCorkell commented Aug 19, 2015