occ check-code compliance

[violoncelloCH]

as of #6

• change discouraged operators
• change deprecated logging method
• make appinfo.xml compliant
• OC_DB static method of private class must not be called and private class must not be imported with a use statement

cc @nextcloud/user_external

1. occ check-code says for the appinfo.xml that the element types would not be expected. In NC12 documentation I can find types, in any newer version it's gone and I can't find any information with what I could replace / change this.

2. I would also need help with OC_DB - private class must not be imported with a use statement and OC_DB - Static method of private class must not be called. What has to be changed there?

[violoncelloCH]

@MorrisJobke @rullzer @nickvergessen could someone help me with these two points?

[MorrisJobke]

2. I would also need help with OC_DB - private class must not be imported with a use statement and OC_DB - Static method of private class must not be called. What has to be changed there?

Go with a OCP\DB\IController instead ;)

[violoncelloCH]

So what would be your suggestion @nickvergessen ? (as you reacted on #30 (comment))

[nickvergessen]

Ah, sorry to create confusion. The interface name was wrong: OCP\IDBConnection is what you should use. Not OCP\DB\IController because our databases dont have a controller class 💃

[MorrisJobke]

Ah, sorry to create confusion. The interface name was wrong: OCP\IDBConnection is what you should use. Not OCP\DB\IController because our databases dont have a controller class 💃

Yes - it was late and I mixed up the terms :/ @nickvergessen is right 👍

[violoncelloCH]

thank you for the clarification!
I tried using it with use OCP\IDBConnection; and then the following later on in the code:

		OCP\IDBConnection::executeQuery(
			'DELETE FROM `*PREFIX*users_external` WHERE `uid` = ? AND `backend` = ?',
			array($uid, $this->backend)
		);

but this fails with Message: Class 'OCA\user_external\OCP\IDBConnection' not found -- any idea?

[nickvergessen]

For now use \OC::$server->getDatabaseConnection()->executeQuery(…)

[violoncelloCH]

thank you @nickvergessen this worked in three places!
however everywhere ->fetchRow() is called or $limit and $offset are passed this doesn't work; e.g.:

user_external/lib/base.php

Lines 58 to 62 in 05fb0e2

	$user = OC_DB::executeAudited(
	'SELECT `displayname` FROM `*PREFIX*users_external`'
	. ' WHERE `uid` = ? AND `backend` = ?',
	array($uid, $this->backend)
	)->fetchRow();

user_external/lib/base.php

Lines 77 to 86 in 05fb0e2

	$result = OC_DB::executeAudited(
	array(
	'sql' => 'SELECT `uid`, `displayname` FROM `*PREFIX*users_external`'
	. ' WHERE (LOWER(`displayname`) LIKE LOWER(?) '
	. ' OR LOWER(`uid`) LIKE LOWER(?)) AND `backend` = ?',
	'limit' => $limit,
	'offset' => $offset
	),
	array('%' . $search . '%', '%' . $search . '%', $this->backend)
	);

user_external/lib/base.php

Lines 173 to 178 in 05fb0e2

	$result = OC_DB::executeAudited(
	'SELECT COUNT(*) FROM `*PREFIX*users_external`'
	. ' WHERE LOWER(`uid`) = LOWER(?) AND `backend` = ?',
	array($uid, $this->backend)
	);
	return $result->fetchOne() > 0;

what can I use there?

[nickvergessen]

Can you push your changes here? Then I will have a look

[violoncelloCH]

I pushed the changes for where \OC::$server->getDatabaseConnection()->executeQuery() works :)

[nickvergessen]

You should be able to change those calls from OC_DB::executeAudited( to \OC::$server->getDatabaseConnection()->executeQuery( and fetchRow( to fetch(

[violoncelloCH]

thank you! I changed this, where it worked for me this way. However there are still to places where this fails; these are the functions in which it fails:

user_external/lib/base.php

Line 76 in 05fb0e2

public function getDisplayNames($search = '', $limit = null, $offset = null) {

user_external/lib/base.php

Line 101 in 05fb0e2

public function getUsers($search = '', $limit = null, $offset = null) {

it fails on the second one on loading the user list (if a user from user_external should be loaded) and errors with: {"reqId":"BruTN3o0cCaCpdjXBLfC","level":3,"time":"2019-01-10T22:19:46+00:00","remoteAddr":"127.0.0.1","user":"jonas","app":"PHP","method":"GET","url":"\/nextcloud\/ocs\/v2.php\/cloud\/users\/details?offset=0&limit=25&search=","message":"Error: Call to a member function execute() on boolean at \/home\/jonas\/nextcloud\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Connection.php#849","userAgent":"Mozilla\/5.0 (X11; Ubuntu; Linux x86_64; rv:64.0) Gecko\/20100101 Firefox\/64.0","version":"16.0.0.0"}

[nickvergessen]

Ah, opsy
change:

		$result = OC_DB::executeAudited(
			array(
				'sql' => 'SELECT `uid`, `displayname` FROM `*PREFIX*users_external`'
					. ' WHERE (LOWER(`displayname`) LIKE LOWER(?) '
					. ' OR LOWER(`uid`) LIKE LOWER(?)) AND `backend` = ?',
				'limit'  => $limit,
				'offset' => $offset
			),
			array('%' . $search . '%', '%' . $search . '%', $this->backend)
		);
		$displayNames = array();
		while ($row = $result->fetchRow()) {
			$displayNames[$row['uid']] = $row['displayname'];
		}

to

		$stmt = \OC::$server->getDatabaseConnection()->prepare(
			'SELECT `uid`, `displayname` FROM `*PREFIX*users_external`'
			. ' WHERE (LOWER(`displayname`) LIKE LOWER(?) '
			. ' OR LOWER(`uid`) LIKE LOWER(?)) AND `backend` = ?',
			$limit, $offset
		);

		$stmt->execute(['%' . $search . '%', '%' . $search . '%', $this->backend]);

		$displayNames = array();
		while ($row = $stmt->fetch()) {
			$displayNames[$row['uid']] = $row['displayname'];
		}

and similar for the second one.

[violoncelloCH]

thank you very much @nickvergessen ! This is now finally completely working!

remains my first question (for this to be complete):

1. occ check-code says for the appinfo.xml that the element types would not be expected. In NC12 documentation I can find types, in any newer version it's gone and I can't find any information with what I could replace / change this.

[nickvergessen]

fixed

[violoncelloCH]

ahh, that simply needs to be after "author"... thanks!

[nickvergessen]

this code is really strange, when there is not exactly one user, the database result is being ignored. Is this intended?

[violoncelloCH]

hmm, maybe @kosli can help us here?

[violoncelloCH]

or maybe @pierreozoux ?

[violoncelloCH]

@nickvergessen the database returns as many users as there are with the same email address (if this emailaddress is entered in the login form), so this should be zero or one normally, shouldn't it?
It makes it possible to login for normal users with their email address even though in config.php for imap backend another domain is specified. This code as it is written now wouldn't work if there are multiple users with the same email address anyway. If we would change it to if(count($users) >= 1) it would always try to login the first user with the given email address... so I think it's better leave it as it is now and not to try to log in the user at all, if the email address is assigned to multiple users. How does Nextcloud's own user backend handle this case? Shouldn't it be restricted that only one user can have a specific email address?

[nickvergessen]

If there are more than 2 users you can not login with email:
https://github.com/nextcloud/server/blob/43d6ae7476bbf4d08991b405a63a0f8dbc2ac25a/core/Controller/LoginController.php#L311-L312

[nickvergessen]

I don't suggest to use if(count($users) >= 1), I just wanted to know if that is intended behaviour. Also as per the code I liked above, the imap user backend should not need to do this. It will be done by the user manager. So actually this method (checkPassword) will be called twice already for email alike logins, so the user_external app does not need to check this.

[nickvergessen]

I commited accordingly, if you still want to keep this, just rebase and drop the commit

[violoncelloCH]

okay, thank you for clarifying! I'm fine with removing it; login with email for a normal user with another domain still works...