refactor user searching

add additional user searching tests Signed-off-by: Robin Appelman <robin@icewind.nl>
2017-12-20 15:51:37 +01:00 · 2017-12-20 15:51:37 +01:00 · aad01894e3
parent ac14d02e1e
commit aad01894e3
9 changed files with 101 additions and 52 deletions
--- a/lib/private/DB/Connection.php
+++ b/lib/private/DB/Connection.php
@ -401,7 +401,7 @@ class Connection extends \Doctrine\DBAL\Connection implements IDBConnection {
 	}

 	/**
-	 * Espace a parameter to be used in a LIKE query
+	 * Escape a parameter to be used in a LIKE query
 	 *
 	 * @param string $param
 	 * @return string
--- a/lib/private/DB/QueryBuilder/ExpressionBuilder/ExpressionBuilder.php
+++ b/lib/private/DB/QueryBuilder/ExpressionBuilder/ExpressionBuilder.php
@ -26,6 +26,7 @@ namespace OC\DB\QueryBuilder\ExpressionBuilder;

 use Doctrine\DBAL\Query\Expression\ExpressionBuilder as DoctrineExpressionBuilder;
 use OC\DB\QueryBuilder\CompositeExpression;
+use OC\DB\QueryBuilder\FunctionBuilder\FunctionBuilder;
 use OC\DB\QueryBuilder\Literal;
 use OC\DB\QueryBuilder\QueryFunction;
 use OC\DB\QueryBuilder\QuoteHelper;
@ -45,6 +46,9 @@ class ExpressionBuilder implements IExpressionBuilder {
 	/** @var IDBConnection */
 	protected $connection;

+	/** @var FunctionBuilder */
+	protected $functionBuilder;
+
 	/**
 	 * Initializes a new <tt>ExpressionBuilder</tt>.
 	 *
@ -54,6 +58,7 @@ class ExpressionBuilder implements IExpressionBuilder {
 		$this->connection = $connection;
 		$this->helper = new QuoteHelper();
 		$this->expressionBuilder = new DoctrineExpressionBuilder($connection);
+		$this->functionBuilder = $connection->getQueryBuilder()->func();
 	}

 	/**
@ -298,9 +303,7 @@ class ExpressionBuilder implements IExpressionBuilder {
 	 * @since 9.0.0
 	 */
 	public function iLike($x, $y, $type = null) {
-		$x = $this->helper->quoteColumnName($x);
-		$y = $this->helper->quoteColumnName($y);
-		return $this->expressionBuilder->comparison("LOWER($x)", 'LIKE', "LOWER($y)");
+		return $this->expressionBuilder->like($this->functionBuilder->lower($x), $this->functionBuilder->lower($y));
 	}

 	/**
--- a/lib/private/DB/QueryBuilder/ExpressionBuilder/SqliteExpressionBuilder.php
+++ b/lib/private/DB/QueryBuilder/ExpressionBuilder/SqliteExpressionBuilder.php
@ -31,4 +31,8 @@ class SqliteExpressionBuilder extends ExpressionBuilder {
 	public function like($x, $y, $type = null) {
 		return parent::like($x, $y, $type) . " ESCAPE '\\'";
 	}
+
+	public function iLike($x, $y, $type = null) {
+		return $this->like($this->functionBuilder->lower($x), $this->functionBuilder->lower($y), $type);
+	}
 }
--- a/lib/private/DB/QueryBuilder/FunctionBuilder/FunctionBuilder.php
+++ b/lib/private/DB/QueryBuilder/FunctionBuilder/FunctionBuilder.php
@ -59,4 +59,8 @@ class FunctionBuilder implements IFunctionBuilder {
 	public function sum($field) {
 		return new QueryFunction('SUM(' . $this->helper->quoteColumnName($field) . ')');
 	}
+
+	public function lower($field) {
+		return new QueryFunction('LOWER(' . $this->helper->quoteColumnName($field) . ')');
+	}
 }
--- a/lib/private/User/Database.php
+++ b/lib/private/User/Database.php
@ -40,6 +40,7 @@
 * along with this program.  If not, see <http://www.gnu.org/licenses/>
 *
 */
+
 /*
 *
 * The following SQL statement is just a help for developers and will not be
@ -56,6 +57,7 @@
 namespace OC\User;

 use OC\Cache\CappedMemoryCache;
+use OC\DB\QueryBuilder\Literal;
 use OCP\IUserBackend;
 use OCP\Util;
 use Symfony\Component\EventDispatcher\EventDispatcher;
@ -83,6 +85,7 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * Create a new user
+	 *
 	 * @param string $uid The username of the user to create
 	 * @param string $password The password of the new user
 	 * @return bool
@ -112,6 +115,7 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * delete a user
+	 *
 	 * @param string $uid The username of the user to delete
 	 * @return bool
 	 *
@ -131,6 +135,7 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * Set password
+	 *
 	 * @param string $uid The username
 	 * @param string $password The new password
 	 * @return bool
@ -152,6 +157,7 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * Set display name
+	 *
 	 * @param string $uid The username
 	 * @param string $displayName The new display name
 	 * @return bool
@ -172,6 +178,7 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * get display name of the user
+	 *
 	 * @param string $uid user ID of the user
 	 * @return string display name
 	 */
@ -189,23 +196,29 @@ class Database extends Backend implements IUserBackend {
 	 * @return array an array of all displayNames (value) and the corresponding uids (key)
 	 */
 	public function getDisplayNames($search = '', $limit = null, $offset = null) {
-		$parameters = [];
-		$searchLike = '';
-		if ($search !== '') {
-			$parameters[] = '%' . \OC::$server->getDatabaseConnection()->escapeLikeParameter($search) . '%';
-			$parameters[] = '%' . \OC::$server->getDatabaseConnection()->escapeLikeParameter($search) . '%';
-			$parameters[] = '%' . \OC::$server->getDatabaseConnection()->escapeLikeParameter($search) . '%';
-			$searchLike .= ' LEFT JOIN `*PREFIX*preferences` ON `userid` = `uid` AND `appid` = \'settings\' AND `configkey` = \'email\'';
-			$searchLike .= ' WHERE LOWER(`configvalue`) LIKE LOWER(?)';
-			$searchLike .= ' OR LOWER(`displayname`) LIKE LOWER(?) OR '
-				. 'LOWER(`uid`) LIKE LOWER(?)';
-		}
+		$connection = \OC::$server->getDatabaseConnection();

-		$displayNames = array();
-		$query = \OC_DB::prepare('SELECT `uid`, `displayname` FROM `*PREFIX*users`'
-			. $searchLike .' ORDER BY LOWER(`displayname`), LOWER(`uid`) ASC', $limit, $offset);
-		$result = $query->execute($parameters);
-		while ($row = $result->fetchRow()) {
+		$query = $connection->getQueryBuilder();
+
+		$query->select('uid', 'displayname')
+			->from('users', 'u')
+			->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
+				$query->expr()->eq('userid', 'uid')),
+				$query->expr()->eq('appid', new Literal('settings')),
+				$query->expr()->eq('configkey', new Literal('email'))
+			)
+			// sqlite doesn't like re-using a single named parameter here
+			->where($query->expr()->iLike('uid', $query->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')))
+			->orWhere($query->expr()->iLike('displayname', $query->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')))
+			->orWhere($query->expr()->iLike('configvalue', $query->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')))
+			->orderBy($query->func()->lower('displayname'), 'ASC')
+			->orderBy($query->func()->lower('uid'), 'ASC')
+			->setMaxResults($limit)
+			->setFirstResult($offset);
+
+		$result = $query->execute();
+		$displayNames = [];
+		while ($row = $result->fetch()) {
 			$displayNames[$row['uid']] = $row['displayname'];
 		}

@ -214,6 +227,7 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * Check if the password is correct
+	 *
 	 * @param string $uid The username
 	 * @param string $password The password
 	 * @return string
@ -229,8 +243,8 @@ class Database extends Backend implements IUserBackend {
 		if ($row) {
 			$storedHash = $row['password'];
 			$newHash = '';
-			if(\OC::$server->getHasher()->verify($password, $storedHash, $newHash)) {
-				if(!empty($newHash)) {
+			if (\OC::$server->getHasher()->verify($password, $storedHash, $newHash)) {
+				if (!empty($newHash)) {
 					$this->setPassword($uid, $password);
 				}
 				return $row['uid'];
@ -243,15 +257,16 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * Load an user in the cache
+	 *
 	 * @param string $uid the username
 	 * @return boolean true if user was found, false otherwise
 	 */
 	private function loadUser($uid) {
-		$uid = (string) $uid;
+		$uid = (string)$uid;
 		if (!isset($this->cache[$uid])) {
 			//guests $uid could be NULL or ''
 			if ($uid === '') {
-				$this->cache[$uid]=false;
+				$this->cache[$uid] = false;
 				return true;
 			}

@ -288,32 +303,15 @@ class Database extends Backend implements IUserBackend {
 	 * @return string[] an array of all uids
 	 */
 	public function getUsers($search = '', $limit = null, $offset = null) {
-		$parameters = [];
-		$searchLike = '';
-		if ($search !== '') {
-			// Email
-			$parameters[] = '%' . \OC::$server->getDatabaseConnection()->escapeLikeParameter($search) . '%';
-			$searchLike .= ' LEFT JOIN `*PREFIX*preferences` ON `userid` = `uid` AND `appid` = \'settings\' AND `configkey` = \'email\'';
-			$searchLike .= ' WHERE LOWER(`configvalue`) LIKE LOWER(?)';
-			// UID
-			$parameters[] = '%' . \OC::$server->getDatabaseConnection()->escapeLikeParameter($search) . '%';
-			$searchLike .= ' OR LOWER(`uid`) LIKE LOWER(?)';
-			// Display name
-			$parameters[] = '%' . \OC::$server->getDatabaseConnection()->escapeLikeParameter($search) . '%';
-			$searchLike .= ' OR LOWER(`displayname`) LIKE LOWER(?)';
-		}
-
-		$query = \OC_DB::prepare('SELECT `uid` FROM `*PREFIX*users`' . $searchLike . ' ORDER BY LOWER(`uid`) ASC', $limit, $offset);
-		$result = $query->execute($parameters);
-		$users = array();
-		while ($row = $result->fetchRow()) {
-			$users[] = $row['uid'];
-		}
-		return $users;
+		$users = $this->getDisplayNames($search, $limit, $offset);
+		$userIds = array_keys($users);
+		sort($userIds, SORT_STRING | SORT_FLAG_CASE);
+		return $userIds;
 	}

 	/**
 	 * check if a user exists
+	 *
 	 * @param string $uid the username
 	 * @return boolean
 	 */
@ -324,6 +322,7 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * get the user's home directory
+	 *
 	 * @param string $uid the username
 	 * @return string|false
 	 */
@ -373,14 +372,15 @@ class Database extends Backend implements IUserBackend {

 	/**
 	 * Backend name to be shown in user management
+	 *
 	 * @return string the name of the backend to be shown
 	 */
-	public function getBackendName(){
+	public function getBackendName() {
 		return 'Database';
 	}

 	public static function preLoginNameUsedAsUserName($param) {
-		if(!isset($param['uid'])) {
+		if (!isset($param['uid'])) {
 			throw new \Exception('key uid is expected to be set in $param');
 		}

--- a/lib/public/DB/QueryBuilder/IFunctionBuilder.php
+++ b/lib/public/DB/QueryBuilder/IFunctionBuilder.php
@ -71,4 +71,13 @@ interface IFunctionBuilder {
 	 * @since 12.0.0
 	 */
 	public function sum($field);
+
+	/**
+	 * Transforms a string field or value to lower case
+	 *
+	 * @param mixed $field
+	 * @return IQueryFunction
+	 * @since 13.0.0
+	 */
+	public function lower($field);
 }
--- a/tests/lib/DB/QueryBuilder/FunctionBuilderTest.php
+++ b/tests/lib/DB/QueryBuilder/FunctionBuilderTest.php
@ -79,4 +79,14 @@ class FunctionBuilderTest extends TestCase {

 		$this->assertEquals('oobar', $query->execute()->fetchColumn());
 	}
+
+	public function testLower() {
+		$query = $this->connection->getQueryBuilder();
+
+		$query->select($query->func()->lower($query->createNamedParameter('FooBar')));
+		$query->from('appconfig')
+			->setMaxResults(1);
+
+		$this->assertEquals('foobar', $query->execute()->fetchColumn());
+	}
 }
--- a/tests/lib/User/Backend.php
+++ b/tests/lib/User/Backend.php
@ -103,15 +103,23 @@ abstract class Backend extends \Test\TestCase {
 		$name1 = 'foobarbaz';
 		$name2 = 'bazbarfoo';
 		$name3 = 'notme';
+		$name4 = 'under_score';

 		$this->backend->createUser($name1, 'pass1');
 		$this->backend->createUser($name2, 'pass2');
 		$this->backend->createUser($name3, 'pass3');
+		$this->backend->createUser($name4, 'pass4');

 		$result = $this->backend->getUsers('bar');
-		$this->assertSame(2, count($result));
+		$this->assertCount(2, $result);

 		$result = $this->backend->getDisplayNames('bar');
-		$this->assertSame(2, count($result));
+		$this->assertCount(2, $result);
+
+		$result = $this->backend->getUsers('under_');
+		$this->assertCount(1, $result);
+
+		$result = $this->backend->getUsers('not_');
+		$this->assertCount(0, $result);
 	}
 }
--- a/tests/lib/User/DatabaseTest.php
+++ b/tests/lib/User/DatabaseTest.php
@ -129,13 +129,24 @@ class DatabaseTest extends Backend {
 		$emailAddr1 = "$user1@nextcloud.com";
 		$emailAddr2 = "$user2@nextcloud.com";

+		$user1Obj->setDisplayName('User 1 Display');
+
+		$result = $this->backend->getDisplayNames('display');
+		$this->assertCount(1, $result);
+
+		$result = $this->backend->getDisplayNames(strtoupper($user1));
+		$this->assertCount(1, $result);
+
 		$user1Obj->setEMailAddress($emailAddr1);
 		$user2Obj->setEMailAddress($emailAddr2);

 		$result = $this->backend->getUsers('@nextcloud.com');
-		$this->assertSame(2, count($result));
+		$this->assertCount(2, $result);

 		$result = $this->backend->getDisplayNames('@nextcloud.com');
-		$this->assertSame(2, count($result));
+		$this->assertCount(2, $result);
+
+		$result = $this->backend->getDisplayNames('@nextcloud.COM');
+		$this->assertCount(2, $result);
 	}
 }