Dynamically update stored credentials in mount config #8167

RobinMcCorkell · 2014-04-11T17:41:33Z

Instead of storing credentials in a session variable, on login the credentials are used to update any dynamic personal mount points (or create them if system-wide ones are encountered) with the new credentials.

Sharing for SMB_OC has been enabled.

The following needs testing:

iRODS
Sharing support ~~(this is a little broken at the moment)~~ ~~Separate bug affecting SMB~~ This was a separate issue, fixed in Fix issue with personal mount points and sharing #8293
Non-dynamic mount points are working as expected

21/04/2014 WIP status removed

@PVince81 @icewind1991 @DeepDiver1975

LukasReschke · 2014-04-11T17:47:26Z

apps/files_external/lib/config.php

+	 * @param array $params
+	 */
+	public static function updateDynamicMountPoints($credentials) {
+		$username = \OC::$session->get('loginname');


I'd prefer to use the function getLoginName() here.

icewind1991 · 2014-04-16T09:05:20Z

Works and the code looks good but I'm not sure if this is a good approach since it requires the users password to be written to disk (in a non one-way encrypted way)

RobinMcCorkell · 2014-04-17T10:30:54Z

Writing the password to disk is no different to how every other external storage works - this just updates it every time the user logs in for dynamic storages.

Still not sure if the sharing bug I mentioned is actually a regression from these changes, it would be really handy if someone else could test:

Create an external storage (doesn't need to be dynamic)
Share something inside it with another user
Attempt to view the shared file/folder as the other user

What I saw was a recursive loop of ownCloud refusing to go into the directory.

RobinMcCorkell · 2014-04-21T20:50:52Z

Any chance of getting an iRODS test done?

RobinMcCorkell · 2014-04-23T18:33:29Z

I'm fairly confident that iRODS will work. I tested a configuration both with 'Use ownCloud login' and without, and as expected the username and password was dynamically adjusted for the 'Use ownCloud login' config while for the one without no such adjustment was made. Ideally someone who uses iRODS needs to test this, but I'm happy that it is working.

PVince81 · 2014-04-28T08:32:59Z

@Xenopathic nice one! I'll test this later.
Would you mind adding a unit test for the new function in config.php ?

PVince81 · 2014-04-28T08:33:12Z

@DeepDiver1975 do you have an iRODS env to test this ?

DeepDiver1975 · 2014-04-28T08:55:51Z

@DeepDiver1975 do you have an iRODS env to test this ?

No - not really - we need to split files_external asap and move iRODS into it's own repo.
The overall need for iRODS connectivity is almost zero - maybe we even drop it.

DeepDiver1975 · 2014-04-28T08:56:09Z

@karlitschek @MTRichards FYI: on iRODS - THX

karlitschek · 2014-04-28T09:38:48Z

Let's also split out the irods into a separate app for now. Then we can see if someone actually needs it and if we ship it.

DeepDiver1975 · 2014-04-28T09:45:20Z

Let's also split out the irods into a separate app for now.

I'll take care of this - I'll move that app (let's name it files_irods) into the apps repo.
Once the development will continue we can move it into it's own repo.

@karlitschek agreed?

karlitschek · 2014-04-28T10:20:11Z

yes. perfect

PVince81 · 2014-04-29T07:48:54Z

apps/files_external/lib/irods.php

@@ -59,8 +51,8 @@ public function __construct($params) {

 	}

-	public static function login( $params ) {
-		\OC::$session->set('irods-credentials', $params);
+	public function isDynamic() {


I suggest to rename this to what it actually is: useLogonCredentials which is more explicit and clearer than isDynamic [minor]

Never mind, I saw that this method is added to the generic Storage interface where we probably don't want to mention logon credentials.

PVince81 · 2014-04-29T07:56:11Z

Nice works @Xenopathic ! Please see my comments.

RobinMcCorkell · 2014-04-29T09:12:35Z

Wow, everything needs rebasing today 😆

Also, I have no idea how to go about writing a unit test. The only time I tried performing a test the whole thing broke on me, so I've been reluctant to try again. Any tips?

Instead of storing credentials in a session variable, on login the credentials are used to update any dynamic personal mount points (or create them if system-wide ones are encountered) with the new credentials. Sharing for SMB_OC has been enabled.

PVince81 · 2014-04-29T10:11:32Z

@Xenopathic rebase your rebased rebase 😉

For the unit test, have a look here: http://doc.owncloud.org/server/7.0/developer_manual/core/unit-testing.html#running-unit-tests-for-the-owncloud-core-project

You can add additional tests in apps/files_external/tests/mountconfig.php.
There are some examples there already that read/check the saved attributes.
You could there simulate a logged in user with different passwords and check whether the config was auto-changed.

RobinMcCorkell · 2014-05-05T19:39:30Z

PHPUnit tests are in. I tried using \OC_User::login() to perform the login instead of manually invoking the hook, but I ran into problems with the hook not being called for some reason (the hook was emitted OK, it just wasn't picked up). Is this intended behaviour, or is that a bug in ownCloud's implementation of unit testing?

PVince81 · 2014-05-06T07:41:15Z

@Xenopathic I'm not sure why the hook wasn't trigger.
The trouble with unit tests is that some things are auto-inited when a PHP class is loaded, which doesn't fit with the setUp/tearDown scheme. I guess that in your case the static init of the hooks didn't happen.
I think it's fine to trigger the hooks manually.

PVince81 · 2014-05-06T07:52:41Z

lib/private/files/storage/common.php

+	 *
+	 * @return bool
+	 */
+	public function isDynamic() {


I'm still not convinced about the isDynamic function. First its name is too generic: what does a "dynamic" storage class mean ? You could rename it to "hasDynamicCredentials".
But still, I think the concept of credentials should not be included in the generic/base class of storages.

I can't think of a better solution right now so I suggest to rename all occurrences of "dynamic" to something more explicit like "hasDynamicConfig()" and "updateDynamicMountPointsConfig()".

@icewind1991 do you have any suggestions ?

I agree that the wording isDynamic is to generic, maybe something like useUserCredentails

PVince81 · 2014-05-06T07:53:37Z

The unit tests look great! Thanks.
Please have a look at my last comment regarding the name "Dynamic".

isDynamic() -> needCredentialsUpdate() updateDynamicMountPoints() -> updateMountPointCredentials() testNonDynamicMountPoint() -> testMountPointNoCredentialsUpdate() testDynamicMountPoint() -> testMountPointCredentialsUpdate()

RobinMcCorkell · 2014-05-06T16:16:31Z

Hopefully needCredentialsUpdate is suitable. This is basically the same issue I ran into when trying to name the SMB_OC class...

scrutinizer-notifier · 2014-05-06T16:42:45Z

The inspection completed: 1 new issues, 7 updated code elements

PVince81 · 2014-05-06T17:24:24Z

The code looks good, thanks.
I'll test this later.

ghost · 2014-05-06T18:31:37Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4592/

PVince81 · 2014-05-07T10:37:26Z

mount SMB_OC storage as admin, login as user, mount appears
mount SMB_OC storage as personal, login as user, mount appears: 🚫 FAIL. When creating it from scratch it doesn't show any status (neither red nor green)

Also, try this:

As admin, create a SMB_OC mount point
As user, go to the personal page: see the auto-generated mount point
As admin, delete the mount point
As user, go to the personal page: see that the mount is still there

It does makes sense technically to auto-generate a personal mount point as this is where the password is saved. But from a UX point of view, if the admin chooses to delete or modify it, it would need to update all the personal mount points. And currently there is no concept of "inherited" mount points.

In an ideal world the user should be able to have "inherited mount points" that are read-only and not shown in the personal page, and those would auto-update based on the "root" entry. But that's tricky to implement and I'm not sure whether we want that level of complexity.

Needs further discussion.

PVince81 · 2014-05-07T10:43:30Z

Also having an auto-generated personal mount point creates an additional storage entry:

 smb::vincent@test//vincent//temp/smbmount/ |          3
 smb::root@test//vincent//temp/smbmount/    |          4

I don't think this is wanted.

RobinMcCorkell · 2014-05-07T11:26:28Z

@PVince81 I'll check out that status bug when mounting as personal.

I expected the deleting bug to hit. There is currently no way to avoid it, as you say with no concept of inherited or read-only mount points. This also has the disadvantage of requiring SMB_OC to be a valid personal mount class to work.

I don't think there is any way to avoid getting a redundant storage entry - this is a side effect of autogeneration.

RobinMcCorkell · 2014-05-10T13:07:35Z

@PVince81 I had a look at mounting SMB_OC as personal... and couldn't replicate the issues you were having. Both with the branch itself and when merged into master, I got the status correctly shown and the mount point mounted (after a logout and login, of course).

RobinMcCorkell · 2014-05-15T10:59:57Z

Is there any point in making a separate SMB_OC class? Other classes could benefit from getting updated credentials when the user logs in - either we make *_OC classes for every class that could benefit or we modify all needed classes to have a new 'Use ownCloud credentials' option (similar to how iRODS did it before).

PVince81 · 2014-05-15T11:05:42Z

Not necessarily. Some classes like Dropbox will not use a password but a token.
The SMB case might be more common though.
I suggest to keep it as is for now.

RobinMcCorkell · 2014-05-15T12:24:32Z

Once the addMountPoint changes have gone through (#8583) I will start work on a proper inheritance model for mount points. That should fix all issues with dynamic mount points like this

ghost · 2014-10-06T08:15:45Z

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/83/

ghost · 2014-10-15T09:20:02Z

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/155/
💣 Test FAILed. 💣

ghost · 2014-11-21T16:17:44Z

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3042/
💣 Test FAILed. 💣

PVince81 · 2014-11-26T11:19:29Z

@Xenopathic in the light of #12216 (the part about capturing credentials) what should we do with this PR ?

RobinMcCorkell · 2014-11-26T13:31:13Z

@PVince81 This approach was fundementally flawed from its inception, so I'll close it. Is there any point in keeping the branch available though?

LukasReschke reviewed Apr 11, 2014
View reviewed changes

RobinMcCorkell mentioned this pull request Apr 21, 2014

Fix issue with personal mount points and sharing #8293

Merged

RobinMcCorkell changed the title ~~[WIP] Dynamically update stored credentials in mount config~~ Dynamically update stored credentials in mount config Apr 21, 2014

PVince81 reviewed Apr 29, 2014
View reviewed changes

Robin McCorkell added 3 commits April 29, 2014 10:17

Use getLoginName instead of reading from the session

9f81778

Add PHPDoc and better comments

f90846b

Add PHPUnit tests for dynamic credential updating

5f5c2ed

Remove un-needed variable assignment

3e8694d

PVince81 reviewed May 6, 2014
View reviewed changes

Change 'dynamic' wording

11cd013

isDynamic() -> needCredentialsUpdate() updateDynamicMountPoints() -> updateMountPointCredentials() testNonDynamicMountPoint() -> testMountPointNoCredentialsUpdate() testDynamicMountPoint() -> testMountPointCredentialsUpdate()

RobinMcCorkell mentioned this pull request Jul 16, 2014

UI improvements for external storage configuration #9663

Closed

butonic mentioned this pull request Aug 8, 2014

[6.0.4] Share external mount in CentOS #10045

Closed

RobinMcCorkell mentioned this pull request Sep 2, 2014

Missing 'Share' button for 'Share by Link' on 'External Storage' using 'SMB / CIFS using OC login' #10802

Closed

LukasReschke added the 2 - Developing label Sep 9, 2014

scolebrook force-pushed the master branch from 4a4ea1d to 9dea79e Compare October 10, 2014 13:32

RobinMcCorkell closed this Nov 26, 2014

RobinMcCorkell mentioned this pull request May 11, 2015

file sync - external storage SMB / CIFS using OC login #16239

Closed

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

Dynamically update stored credentials in mount config #8167

Dynamically update stored credentials in mount config #8167

Conversation

RobinMcCorkell commented Apr 11, 2014

LukasReschke Apr 11, 2014

Choose a reason for hiding this comment

icewind1991 commented Apr 16, 2014

RobinMcCorkell commented Apr 17, 2014

RobinMcCorkell commented Apr 21, 2014

RobinMcCorkell commented Apr 23, 2014

PVince81 commented Apr 28, 2014

PVince81 commented Apr 28, 2014

DeepDiver1975 commented Apr 28, 2014

DeepDiver1975 commented Apr 28, 2014

karlitschek commented Apr 28, 2014

DeepDiver1975 commented Apr 28, 2014

karlitschek commented Apr 28, 2014

PVince81 Apr 29, 2014

Choose a reason for hiding this comment

PVince81 Apr 29, 2014

Choose a reason for hiding this comment

PVince81 commented Apr 29, 2014

RobinMcCorkell commented Apr 29, 2014

PVince81 commented Apr 29, 2014

RobinMcCorkell commented May 5, 2014

PVince81 commented May 6, 2014

PVince81 May 6, 2014

Choose a reason for hiding this comment

icewind1991 May 6, 2014

Choose a reason for hiding this comment

PVince81 commented May 6, 2014

RobinMcCorkell commented May 6, 2014

scrutinizer-notifier commented May 6, 2014

PVince81 commented May 6, 2014

ghost commented May 6, 2014

PVince81 commented May 7, 2014

PVince81 commented May 7, 2014

RobinMcCorkell commented May 7, 2014

RobinMcCorkell commented May 10, 2014

RobinMcCorkell commented May 15, 2014

PVince81 commented May 15, 2014

RobinMcCorkell commented May 15, 2014

ghost commented Oct 6, 2014

ghost commented Oct 15, 2014

ghost commented Nov 21, 2014

PVince81 commented Nov 26, 2014

RobinMcCorkell commented Nov 26, 2014