Backport of #1643 to stable10

get rid of test warnings

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
cache loginName2UserName and cover the method with unit tests

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
adjust tests

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
index 299ad581644f8bba794f2a9efee98d5947982861..3b1370567ba327c40c0238c797cafb9eccfc6e15 100644 (file)
--- a/apps/user_ldap/lib/Access.php
+++ b/apps/user_ldap/lib/Access.php
@@ -54,6 +54,7 @@ class Access extends LDAPUtility implements IUserTools {
         * @var \OCA\User_LDAP\Connection
         */
        public $connection;
+       /** @var Manager */
        public $userManager;
        //never ever check this var directly, always use getPagedSearchResultState
        protected $pagedSearchedSuccessful;

diff --git a/apps/user_ldap/lib/Exceptions/NotOnLDAP.php b/apps/user_ldap/lib/Exceptions/NotOnLDAP.php
new file mode 100644 (file)
index 0000000..41fb6c9
--- /dev/null
+++ b/apps/user_ldap/lib/Exceptions/NotOnLDAP.php
@@ -0,0 +1,26 @@
+<?php
+/**
+ * @copyright Copyright (c) 2016 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\User_LDAP\Exceptions;
+
+class NotOnLDAP extends \Exception {}

diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php
index ce1aafc210e24dea6d33d4e77f162c39ef9001c9..3222c0cb3dba8902ad57bb5945ee3c430c947397 100644 (file)
--- a/apps/user_ldap/lib/User_LDAP.php
+++ b/apps/user_ldap/lib/User_LDAP.php
@@ -36,6 +36,7 @@
 namespace OCA\User_LDAP;
 
 use OC\User\NoUserException;
+use OCA\User_LDAP\Exceptions\NotOnLDAP;
 use OCA\User_LDAP\User\OfflineUser;
 use OCA\User_LDAP\User\User;
 use OCP\IConfig;
@@ -80,14 +81,26 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
         * @return string|false
         */
        public function loginName2UserName($loginName) {
+               $cacheKey = 'loginName2UserName-'.$loginName;
+               $username = $this->access->connection->getFromCache($cacheKey);
+               if(!is_null($username)) {
+                       return $username;
+               }
+
                try {
                        $ldapRecord = $this->getLDAPUserByLoginName($loginName);
                        $user = $this->access->userManager->get($ldapRecord['dn'][0]);
                        if($user instanceof OfflineUser) {
+                               // this path is not really possible, however get() is documented
+                               // to return User or OfflineUser so we are very defensive here.
+                               $this->access->connection->writeToCache($cacheKey, false);
                                return false;
                        }
-                       return $user->getUsername();
-               } catch (\Exception $e) {
+                       $username = $user->getUsername();
+                       $this->access->connection->writeToCache($cacheKey, $username);
+                       return $username;
+               } catch (NotOnLDAP $e) {
+                       $this->access->connection->writeToCache($cacheKey, false);
                        return false;
                }
        }
@@ -107,14 +120,14 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
         *
         * @param string $loginName
         * @return array
-        * @throws \Exception
+        * @throws NotOnLDAP
         */
        public function getLDAPUserByLoginName($loginName) {
                //find out dn of the user name
                $attrs = $this->access->userManager->getAttributes();
                $users = $this->access->fetchUsersByLoginName($loginName, $attrs);
                if(count($users) < 1) {
-                       throw new \Exception('No user available for the given login name on ' .
+                       throw new NotOnLDAP('No user available for the given login name on ' .
                                $this->access->connection->ldapHost . ':' . $this->access->connection->ldapPort);
                }
                return $users[0];

diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php
index 47e789142e51bf5b145a16f5d1511f711660d0b9..13a4f5854c34ea75b6b2413be5b9e957f003d208 100644 (file)
--- a/apps/user_ldap/tests/User_LDAPTest.php
+++ b/apps/user_ldap/tests/User_LDAPTest.php
@@ -28,9 +28,10 @@
 
 namespace OCA\User_LDAP\Tests;
 
+use OCA\User_LDAP\Access;
+use OCA\User_LDAP\Helper;
 use OCA\User_LDAP\User_LDAP as UserLDAP;
-use \OCA\User_LDAP\Access;
-use \OCA\User_LDAP\Connection;
+use Test\TestCase;
 
 /**
  * Class Test_User_Ldap_Direct
@@ -39,7 +40,7 @@ use \OCA\User_LDAP\Connection;
  *
  * @package OCA\User_LDAP\Tests
  */
-class User_LDAPTest extends \Test\TestCase {
+class User_LDAPTest extends TestCase {
        protected $backend;
        protected $access;
        protected $configMock;
@@ -51,24 +52,15 @@ class User_LDAPTest extends \Test\TestCase {
                \OC_Group::clearBackends();
        }
 
+       /**
+        * @return \PHPUnit_Framework_MockObject_MockObject|Access
+        */
        private function getAccessMock() {
-               static $conMethods;
-               static $accMethods;
-               static $uMethods;
-
-               if(is_null($conMethods) || is_null($accMethods)) {
-                       $conMethods = get_class_methods('\OCA\User_LDAP\Connection');
-                       $accMethods = get_class_methods('\OCA\User_LDAP\Access');
-                       unset($accMethods[array_search('getConnection', $accMethods)]);
-                       $uMethods   = get_class_methods('\OCA\User_LDAP\User\User');
-                       unset($uMethods[array_search('getUsername', $uMethods)]);
-                       unset($uMethods[array_search('getDN', $uMethods)]);
-                       unset($uMethods[array_search('__construct', $uMethods)]);
-               }
                $lw  = $this->getMock('\OCA\User_LDAP\ILDAPWrapper');
-               $connector = $this->getMock('\OCA\User_LDAP\Connection',
-                                                                       $conMethods,
-                                                                       array($lw, null, null));
+               $connector = $this->getMockBuilder('\OCA\User_LDAP\Connection')
+                       ->setMethodsExcept(['getConnection'])
+                       ->setConstructorArgs([$lw, null, null])
+                       ->getMock();
 
                $this->configMock = $this->getMock('\OCP\IConfig');
 
@@ -76,7 +68,7 @@ class User_LDAPTest extends \Test\TestCase {
                        ->disableOriginalConstructor()
                        ->getMock();
 
-               $um = $this->getMockBuilder('\OCA\User_LDAP\User\Manager')
+               $um = $this->getMockBuilder('OCA\User_LDAP\User\Manager')
                        ->setMethods(['getDeletedUser'])
                        ->setConstructorArgs([
                                $this->configMock,
@@ -93,11 +85,12 @@ class User_LDAPTest extends \Test\TestCase {
                        ->method('getDeletedUser')
                        ->will($this->returnValue($offlineUser));
 
-               $helper = new \OCA\User_LDAP\Helper();
-               
-               $access = $this->getMock('\OCA\User_LDAP\Access',
-                                                                $accMethods,
-                                                                array($connector, $lw, $um, $helper));
+               $helper = new Helper();
+
+               $access = $this->getMockBuilder('OCA\User_LDAP\Access')
+                       ->setMethodsExcept(['getConnection'])
+                       ->setConstructorArgs([$connector, $lw, $um, $helper])
+                       ->getMock();
 
                $um->setLdapAccess($access);
 
@@ -129,7 +122,7 @@ class User_LDAPTest extends \Test\TestCase {
 
        /**
         * Prepares the Access mock for checkPassword tests
-        * @param \OCA\User_LDAP\Access $access mock
+        * @param Access|\PHPUnit_Framework_MockObject_MockObject $access mock
         * @param bool $noDisplayName
         * @return void
         */
@@ -298,7 +291,7 @@ class User_LDAPTest extends \Test\TestCase {
 
        /**
         * Prepares the Access mock for getUsers tests
-        * @param \OCA\User_LDAP\Access $access mock
+        * @param Access $access mock
         * @return void
         */
        private function prepareAccessForGetUsers(&$access) {
@@ -842,4 +835,114 @@ class User_LDAPTest extends \Test\TestCase {
                $result = $backend->countUsers();
                $this->assertFalse($result);
        }
+
+       public function testLoginName2UserNameSuccess() {
+               $loginName = 'Alice';
+               $username  = 'alice';
+               $dn        = 'uid=alice,dc=what,dc=ever';
+
+               $access = $this->getAccessMock();
+               $access->expects($this->once())
+                       ->method('fetchUsersByLoginName')
+                       ->with($this->equalTo($loginName))
+                       ->willReturn([['dn' => [$dn]]]);
+               $access->expects($this->once())
+                       ->method('stringResemblesDN')
+                       ->with($this->equalTo($dn))
+                       ->willReturn(true);
+               $access->expects($this->once())
+                       ->method('dn2username')
+                       ->with($this->equalTo($dn))
+                       ->willReturn($username);
+
+               $access->connection->expects($this->exactly(2))
+                       ->method('getFromCache')
+                       ->with($this->equalTo('loginName2UserName-'.$loginName))
+                       ->willReturnOnConsecutiveCalls(null, $username);
+               $access->connection->expects($this->once())
+                       ->method('writeToCache')
+                       ->with($this->equalTo('loginName2UserName-'.$loginName), $this->equalTo($username));
+
+               $backend = new UserLDAP($access, $this->createMock('\OCP\IConfig'));
+               $name = $backend->loginName2UserName($loginName);
+               $this->assertSame($username, $name);
+
+               // and once again to verify that caching works
+               $backend->loginName2UserName($loginName);
+       }
+
+       public function testLoginName2UserNameNoUsersOnLDAP() {
+               $loginName = 'Loki';
+
+               $access = $this->getAccessMock();
+               $access->expects($this->once())
+                       ->method('fetchUsersByLoginName')
+                       ->with($this->equalTo($loginName))
+                       ->willReturn([]);
+               $access->expects($this->never())
+                       ->method('stringResemblesDN');
+               $access->expects($this->never())
+                       ->method('dn2username');
+
+               $access->connection->expects($this->exactly(2))
+                       ->method('getFromCache')
+                       ->with($this->equalTo('loginName2UserName-'.$loginName))
+                       ->willReturnOnConsecutiveCalls(null, false);
+               $access->connection->expects($this->once())
+                       ->method('writeToCache')
+                       ->with($this->equalTo('loginName2UserName-'.$loginName), false);
+
+               $backend = new UserLDAP($access, $this->createMock('\OCP\IConfig'));
+               $name = $backend->loginName2UserName($loginName);
+               $this->assertSame(false, $name);
+
+               // and once again to verify that caching works
+               $backend->loginName2UserName($loginName);
+       }
+
+       public function testLoginName2UserNameOfflineUser() {
+               $loginName = 'Alice';
+               $dn        = 'uid=alice,dc=what,dc=ever';
+
+               $offlineUser = $this->getMockBuilder('OCA\User_LDAP\User\OfflineUser')
+                       ->disableOriginalConstructor()
+                       ->getMock();
+
+               $access = $this->getAccessMock();
+               $access->expects($this->once())
+                       ->method('fetchUsersByLoginName')
+                       ->with($this->equalTo($loginName))
+                       ->willReturn([['dn' => [$dn]]]);
+               $access->expects($this->once())
+                       ->method('stringResemblesDN')
+                       ->with($this->equalTo($dn))
+                       ->willReturn(true);
+               $access->expects($this->once())
+                       ->method('dn2username')
+                       ->willReturn(false);    // this is fake, but allows us to force-enter the OfflineUser path
+
+               $access->connection->expects($this->exactly(2))
+                       ->method('getFromCache')
+                       ->with($this->equalTo('loginName2UserName-'.$loginName))
+                       ->willReturnOnConsecutiveCalls(null, false);
+               $access->connection->expects($this->once())
+                       ->method('writeToCache')
+                       ->with($this->equalTo('loginName2UserName-'.$loginName), $this->equalTo(false));
+
+               $access->userManager->expects($this->once())
+                       ->method('getDeletedUser')
+                       ->will($this->returnValue($offlineUser));
+
+               $this->configMock->expects($this->once())
+                       ->method('getUserValue')
+                       ->willReturn(1);
+
+               $backend = new UserLDAP($access, $this->createMock('\OCP\IConfig'));
+               $name = $backend->loginName2UserName($loginName);
+               $this->assertSame(false, $name);
+
+               // and once again to verify that caching works
+               $backend->loginName2UserName($loginName);
+       }
+
 }