Store user_webdavauth users in DB + code cleanup #12799

LukasReschke · 2014-12-11T19:56:52Z

This commit introduces the storing of user_webdavauth users in the database which is a pre-requisite of #12620.

Furthermore, I refactored the code and removed deprecated code, as a little gimmick I added unit tests for the backend. - Especially considering that some people are using this as reference implementation for an authentication backend I consider it important that this backend follows best-practises. (still not perfect - but way better)

@MorrisJobke Please review.

@karlitschek Thought you might like that since you created that app. Care to review as well? - Don't worry, most of the change are unit tests :-)

karlitschek · 2014-12-11T20:04:22Z

I forgot this app already. ;-)
looks good 👍

LukasReschke · 2014-12-11T20:05:19Z

@DeepDiver1975 Our CI is running amok again :-(

LukasReschke · 2014-12-11T20:09:56Z

apps/user_webdavauth/user_webdavauth.php

+		if(substr($returnCode, 0, 1) === '2') {
+			if(!$this->userExists($uid)) {
+				$query = $this->db->prepareQuery('INSERT INTO `'.$this->tableName.'` (`uid`) VALUES (?)');
+				$query->bindValue(1, $uid);


@MorrisJobke @DeepDiver1975 I wanted to mock bindValue also in the tests. But somehow it never worked. Any idea? - I just noticed that it is not really within the defined class and is just annotated:

core/lib/private/db/statementwrapper.php

Lines 9 to 17 in 8fd90e0

/**

* small wrapper around \Doctrine\DBAL\Driver\Statement to make it behave, more like an MDB2 Statement

*

* @method boolean bindValue(mixed $param, mixed $value, integer $type = null);

* @method string errorCode();

* @method array errorInfo();

* @method integer rowCount();

* @method array fetchAll(integer $fetchMode = null);

*/

Any idea?

LukasReschke · 2014-12-11T20:17:44Z

@blizzz I guess this makes user_ldap compatible again with user_webdavauth?

blizzz · 2014-12-11T20:37:06Z

@LukasReschke did not test, but code-wise you fixed the issue.

A potential thing: in checkPassword(), if webdav does not care about case sensitivity, $uid could have different cases, resulting in different users. If this is the case, consider lowercasing it.

Also, since webdav users can show up out of nothing, the returned $uid in checkPassword could collide with an existing user. It's like creating a user without name checks. I.e. it could be crafted to get access to the data of a local or other user. In LDAP, I check available names before returning a new username.

LukasReschke · 2014-12-11T20:43:28Z

A potential thing: in checkPassword(), if webdav does not care about case sensitivity, $uid could have different cases, resulting in different users. If this is the case, consider lowercasing it.

True. Needs investigation.

Also, since webdav users can show up out of nothing, the returned $uid in checkPassword could collide with an existing user. It's like creating a user without name checks. I.e. it could be crafted to get access to the data of a local or other user. In LDAP, I check available names before returning a new username.

I guess you do that by querying the usermanager before and asking if the user exists?

blizzz · 2014-12-11T22:29:54Z

Also, since webdav users can show up out of nothing, the returned $uid in
checkPassword could collide with an existing user. It's like creating a
user without name checks. I.e. it could be crafted to get access to the
data of a local or other user. In LDAP, I check available names before
returning a new username.
I guess you do that by querying the usermanager before and asking if the
user exists?

You know my methods, Watson.

LukasReschke · 2014-12-12T10:28:46Z

@blizzz THX for the heads-up. Incorporated your comments.

LukasReschke · 2014-12-12T13:34:15Z

(failing test is unrelated)

DeepDiver1975 · 2014-12-15T07:24:23Z

Well - in the past months almost any user backend we implemented in some point in time introduced it's own table to store the users. We might want to think about some consolidation .....

DeepDiver1975 · 2014-12-15T07:28:43Z

apps/user_webdavauth/user_webdavauth.php

+	 */
+	public function getHome($uid) {
+		if ($this->userExists($uid)) {
+			return $this->config->getSystemValue('datadirectory', $this->serverRoot . '/data') . '/' . $uid;


wouldn't it make more sense to have an api to get the data dir instead of implementing this logic in several places? THX

just return false or don't implement it – getHome() is only to override ownCloud default.

@MorrisJobke as blizzz explained - no need to implement this function - just kill it.

serverroot should no longer be required as well - THX

LukasReschke · 2014-12-15T10:10:25Z

Well - in the past months almost any user backend we implemented in some point in time introduced it's own table to store the users. We might want to think about some consolidation .....

The problem I see there is that we will loose a lot of flexibility with that approach. User backends might allow different login names (for example the LDAP app does this) and also might require the need to store additional information about the backend. (for example the remote server if we're going to support such scenarios in the future)

I'm not quite sure how we can tackle this with a single table.

MorrisJobke · 2014-12-19T09:26:31Z

@LukasReschke rebase needed

DeepDiver1975 · 2014-12-19T12:23:19Z

apps/user_webdavauth/appinfo/database.xml

+			</field>
+
+			<index>
+				<name>webdav_uid</name>


We require PKs on all tables - please add

<primary>true</primary>

Reminder to myself: Learn more about that mysterious database.xml - THX! ;-)

I thought I added a commit to this branch o.O

Database noob here: Isn't the unique one line below exactly the same? 🙈

Unique != primary ;)

MorrisJobke · 2014-12-23T10:20:03Z

I rebase, fixed the merge conflict and added the primary key. But I'm not able to fix the other stuff. @LukasReschke Can you have a look at this?

MorrisJobke · 2014-12-23T10:37:51Z

I also dropped the method getHome

LukasReschke · 2015-01-05T09:26:46Z

I think it's better to keep this for 8.1 now.

LukasReschke · 2015-02-12T14:09:56Z

Rebased. Ready to review.

DeepDiver1975 · 2015-02-13T10:55:27Z

lib/private/server.php

-	 * @return \OCP\IDateTimeZone
+	 * Get the timezone of the current user, based on his session information and config data
+	 *
+	 * @return \DateTimeZone


the server is meant to return ocp interfaces

This commit introduces the storing of user_webdavauth users in the database which is a pre-requisite of #12620. Furthermore, I refactored the code and removed deprecated code, as a little gimmick I added unit tests for the backend. Conflicts: lib/private/server.php Conflicts: apps/user_webdavauth/appinfo/app.php apps/user_webdavauth/user_webdavauth.php

LukasReschke · 2015-02-24T12:06:01Z

Rebased as well.

ghost · 2015-02-24T12:34:15Z

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

scrutinizer-notifier · 2015-02-24T13:01:54Z

The inspection completed: 51 new issues, 32 updated code elements

canepan · 2015-03-10T10:41:22Z

Hello.
I'm wondering if it is possible to make this patch (and external users management) more useful.
Right now, an user which logs in via webdav does not belong to any group, has no email or display name, so apart from storing files and sharing via link or directly with other users.
It's not possible to assign quota, or do anything with external users.

I think this is the same for all external authentication methods (apart from LDAP, which can map LDAP groups to OC groups, I guess)

MorrisJobke · 2015-03-25T23:37:37Z

@DeepDiver1975 @LukasReschke Still something for 8.1?

ghost · 2015-04-30T21:33:54Z

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

MorrisJobke · 2015-08-28T17:53:16Z

@LukasReschke You said, that you want to have a look at this :)

DeepDiver1975 · 2015-09-21T09:21:09Z

@LukasReschke 9.0 ????

LukasReschke · 2015-09-21T15:52:41Z

Replaced by #19229 then I guess.

DeepDiver1975 · 2015-09-22T07:40:55Z

Replaced by #19229 then I guess.

yep

LukasReschke added the 3 - To Review label Dec 11, 2014

LukasReschke reviewed Dec 11, 2014
View reviewed changes

LukasReschke mentioned this pull request Dec 11, 2014

Creates a user with random password upon login with webdav backend, so to be able to assign user to groups #10577

Closed

DeepDiver1975 reviewed Dec 15, 2014
View reviewed changes

MorrisJobke added 1 - To develop and removed 3 - To Review labels Dec 19, 2014

LukasReschke force-pushed the store-users-for-webdav-auth branch from 7fc1b66 to 50deaab Compare December 19, 2014 09:44

LukasReschke added 3 - To Review and removed 1 - To develop labels Dec 19, 2014

LukasReschke mentioned this pull request Dec 19, 2014

User Management - ownCloud 8.0 #12620

Closed

23 tasks

LukasReschke added 2 - Developing and removed 3 - To Review labels Dec 19, 2014

DeepDiver1975 reviewed Dec 19, 2014
View reviewed changes

MorrisJobke mentioned this pull request Dec 22, 2014

User Management #12998

Closed

27 tasks

DeepDiver1975 added this to the 8.0-current milestone Dec 22, 2014

MorrisJobke force-pushed the store-users-for-webdav-auth branch from 50deaab to b5c62d7 Compare December 23, 2014 10:19

LukasReschke added 3 - To Review and removed 2 - Developing labels Feb 12, 2015

LukasReschke force-pushed the store-users-for-webdav-auth branch from a3408f6 to d25340c Compare February 12, 2015 14:09

DeepDiver1975 reviewed Feb 13, 2015
View reviewed changes

LukasReschke mentioned this pull request Feb 18, 2015

WebDAV Authentication backend inverts authentication #14323

Closed

LukasReschke force-pushed the store-users-for-webdav-auth branch from d25340c to 7723c53 Compare February 24, 2015 12:02

LukasReschke force-pushed the store-users-for-webdav-auth branch from 7723c53 to d4fc829 Compare February 24, 2015 12:05

MorrisJobke added 2 - Developing and removed 3 - To Review labels Mar 11, 2015

DeepDiver1975 modified the milestones: 8.2-next, 8.1-current Apr 7, 2015

LukasReschke modified the milestones: 9.0-next, 8.2-current Sep 21, 2015

LukasReschke closed this Sep 21, 2015

LukasReschke deleted the store-users-for-webdav-auth branch September 21, 2015 15:52

MorrisJobke removed this from the 9.0-next milestone Sep 21, 2015

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

	/**
	* small wrapper around \Doctrine\DBAL\Driver\Statement to make it behave, more like an MDB2 Statement
	*
	* @method boolean bindValue(mixed $param, mixed $value, integer $type = null);
	* @method string errorCode();
	* @method array errorInfo();
	* @method integer rowCount();
	* @method array fetchAll(integer $fetchMode = null);
	*/

Store user_webdavauth users in DB + code cleanup #12799

Store user_webdavauth users in DB + code cleanup #12799

Conversation

LukasReschke commented Dec 11, 2014

karlitschek commented Dec 11, 2014

LukasReschke commented Dec 11, 2014

Choose a reason for hiding this comment

LukasReschke commented Dec 11, 2014

blizzz commented Dec 11, 2014

LukasReschke commented Dec 11, 2014

blizzz commented Dec 11, 2014

LukasReschke commented Dec 12, 2014

LukasReschke commented Dec 12, 2014

DeepDiver1975 commented Dec 15, 2014

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

LukasReschke commented Dec 15, 2014

MorrisJobke commented Dec 19, 2014

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

MorrisJobke commented Dec 23, 2014

MorrisJobke commented Dec 23, 2014

LukasReschke commented Jan 5, 2015

LukasReschke commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasReschke commented Feb 24, 2015

ghost commented Feb 24, 2015

scrutinizer-notifier commented Feb 24, 2015

canepan commented Mar 10, 2015

MorrisJobke commented Mar 25, 2015

ghost commented Apr 30, 2015

MorrisJobke commented Aug 28, 2015

DeepDiver1975 commented Sep 21, 2015

LukasReschke commented Sep 21, 2015

DeepDiver1975 commented Sep 22, 2015