External storage 'Login credentials' auth mechanism

[RobinMcCorkell]

This auth mechanism saves user credentials in the database, encrypted. This is done with the new CredentialsManager class.

⚠️ The setValues() method on IDBConnection might break with Oracle. Hopefully the unit tests will pass, but if not, the fix lies in to_char(...) in the WHERE clause when using clobs. ⚠️

Please test and review @PVince81 @MorrisJobke @DeepDiver1975 @icewind1991 @LukasReschke

@MTRichards ref #16305

[PVince81]

Please rebase

[icewind1991]

needs a beter name

[DeepDiver1975]

@PVince81 @Xenopathic schedule for 9.0

[PVince81]

add comment, what is this ?

[PVince81]

does it link to the mount id ?

[PVince81]

Please rebase 😄

[RobinMcCorkell]

I'd rather we wait for the config to live in the DB fully before merging this, for cleaner, better code.

[PVince81]

@Xenopathic the config is already in the DB. Please rebase 😄

@icewind1991 please notice this PR about auth mechanism, to make sure we're not duplicating work 😄

[icewind1991]

rebased and added more tests for setValues

[icewind1991]

Fixed checking the external storage status (owncloud/index.php/apps/files_external/userglobalstorages/$id) by passing the user when manipulating storage config

[icewind1991]

After applying #21467 everything seems to work fine for an admin defined mount using login credentials.

Storage is accessible both trough the WI/DAV and occ

[RobinMcCorkell]

I still don't like the approach of the CredentialsManager in core, ideally the configuration row in the files_external tables should be updated/created, but that approach isn't so simple with multiple users access to a global storage.

I had an idea (completely conceptual at this stage) to create a wrapping storage per-user that contains the credentials, along with a 'pointer' to the global storage. Then during mounting, that wrapped storage overrides the global storage and can apply the credentials as necessary. That would also form a nice basis for the 'custom credentials' authentication mechanism (as required by the WND merger) which would work in a similar way, but with those custom credentials directly modifiable in the configuration UI.

[icewind1991]

I think the current idea is better and less complex.

The CredentialsManager can also be used to handle the custom credentials

[PVince81]

Okay, it works from scratch.

@icewind1991 please increase the app version

[icewind1991]

please increase the app version

Done

[PVince81]

👍

Maybe @Xenopathic and @icewind1991 could review each other's code ?

Unless @jmaciasportela wants to chime in for the final review ?

[icewind1991]

👍 for @Xenopathic's part

[icewind1991]

06:44:45 There was 1 error:
06:44:45
06:44:45 1) Test\TestAllConfig::testSetUserValueWithPreCondition
06:44:45 Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'UPDATE "oc_preferences" SET "configvalue" = ? WHERE ("userid" = ?) AND ("appid" = ?) AND ("configkey" = ?) AND ("configvalue" = ?)' with params ["valuePreCond2", "userPreCond", "appPreCond", "keyPreCond", "valuePreCond"]:
06:44:45
06:44:45 ORA-00932: inconsistent datatypes: expected - got CLOB

Will look into it

(jenkins-oci actually reporting real failures, amazing)

[RobinMcCorkell]

Yup, as predicted in the original PR post, Oracle broke. @nickvergessen had a PR a while back that simply removes the WHERE clause on Oracle, making things slower sure, but at least not breaking.

[nickvergessen]

clob fields can not be compared on OCI

[icewind1991]

Fixed by adding adding support for comparing against clobs on oci if you explicitly define you're trying to compare strings

cc @DeepDiver1975 for the query builder stuff (last commit)

[icewind1991]

all green, can this be merged?

[PVince81]

👍 from me for the last changes

@nickvergessen @DeepDiver1975 is the query builder change acceptable ? 58afddf

[nickvergessen]

I think if we do this, we should do it for all comparisons, or at least neq()

Should also then add unit tests for this directly.
Can also do this in another PR, up to @DeepDiver1975

[nickvergessen]

In order to keep the methods from doctrine untouched, we could also add a new method, that would make the comparison explicit, while not touching the interface of doctrine methods

[icewind1991]

I would prefer to keep it as arguments to the existing methods

[PVince81]

Can you add neq then too and the unit test for both methods, if possible ?

[nickvergessen]

Well we can do that once we decide on the API and how we want to handle it. Until then lets waste as few effort as possible

[PVince81]

In order to keep the methods from doctrine untouched, we could also add a new method, that would make the comparison explicit, while not touching the interface of doctrine methods

@icewind1991 can you make the change + add unit tests ? (or make it a separate PR so we can review it separately)

[icewind1991]

In order to keep the methods from doctrine untouched, we could also add a new method, that would make the comparison explicit, while not touching the interface of doctrine methods

I prefer to merge this as is and we can discuss whether to make it a new method or keep it as argument in a seperate issue/pr.
It's a shame to keep this PR blocked over a minor issue like that

cc @nickvergessen

[PVince81]

@jmaciasportela @DeepDiver1975 can you help with the second review ?
or @Xenopathic review @icewind1991's latest changes so we can merge this

[jmaciasportela]

Except this comment only to take into account: https://github.com/owncloud/core/pull/18531/files#r49991237
LGTM 👍