Do not sync user role if LDAP groups are not configured

5 files changed, 51 insertions, 2 deletions

diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md
index 7efe2128..2f0a0589 100644
--- a/CONTRIBUTORS.md
+++ b/CONTRIBUTORS.md
@@ -13,6 +13,7 @@ Contributors:
 - [Anjar Febrianto](https://github.com/Lasut)
 - [Ashbike](https://github.com/ashbike)
 - [Ashish Kulkarni](https://github.com/ashkulz)
+- [Bitcoin 333](https://github.com/bitcoin333)
 - [Busfreak](https://github.com/Busfreak)
 - [Christian González](https://github.com/nerdoc)
 - [Chorgroup](https://github.com/chorgroup)
diff --git a/ChangeLog b/ChangeLog
index ae028de1..64e446e3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -22,6 +22,8 @@ Improvements:
 
 Bug fixes:
 
+* Do not sync user role if LDAP groups are not configured
+* Fixed issue with unicode handling for letter based avatars and user initials
 * Do not send notifications to disabled users
 * Fix wrong redirect when removing a task from the task view page
 
diff --git a/app/Core/Ldap/User.php b/app/Core/Ldap/User.php
index c54aa1ac..91b48530 100644
--- a/app/Core/Ldap/User.php
+++ b/app/Core/Ldap/User.php
@@ -108,12 +108,18 @@ class User
     /**
      * Get role from LDAP groups
      *
+     * Note: Do not touch the current role if groups are not configured
+     *
      * @access protected
      * @param  string[] $groupIds
      * @return string
      */
     protected function getRole(array $groupIds)
     {
+        if ($this->hasGroupsNotConfigured()) {
+            return null;
+        }
+
         foreach ($groupIds as $groupId) {
             $groupId = strtolower($groupId);
 
@@ -272,6 +278,17 @@ class User
     }
 
     /**
+     * Return true if LDAP Group mapping is not configured
+     *
+     * @access public
+     * @return boolean
+     */
+    public function hasGroupsNotConfigured()
+    {
+        return !$this->getGroupAdminDn() && !$this->getGroupManagerDn();
+    }
+
+    /**
      * Get LDAP admin group DN
      *
      * @access public
diff --git a/tests/units/Core/Ldap/LdapUserTest.php b/tests/units/Core/Ldap/LdapUserTest.php
index 02b9331e..505b8a03 100644
--- a/tests/units/Core/Ldap/LdapUserTest.php
+++ b/tests/units/Core/Ldap/LdapUserTest.php
@@ -61,7 +61,7 @@ class LdapUserTest extends Base
             ->getMock();
     }
 
-    public function testGetUser()
+    public function testGetUserWithNoGroupConfigured()
     {
         $entries = new Entries(array(
             'count' => 1,
@@ -136,7 +136,7 @@ class LdapUserTest extends Base
         $this->assertEquals('my_ldap_user', $user->getUsername());
         $this->assertEquals('My LDAP user', $user->getName());
         $this->assertEquals('user1@localhost', $user->getEmail());
-        $this->assertEquals(Role::APP_USER, $user->getRole());
+        $this->assertEquals(null, $user->getRole());
         $this->assertSame('', $user->getPhoto());
         $this->assertEquals(array(), $user->getExternalGroupIds());
         $this->assertEquals(array('is_ldap_user' => 1), $user->getExtraAttributes());
@@ -746,6 +746,11 @@ class LdapUserTest extends Base
 
         $this->user
             ->expects($this->any())
+            ->method('getGroupManagerDn')
+            ->will($this->returnValue('cn=Kanboard Managers,ou=Groups,dc=kanboard,dc=local'));
+
+        $this->user
+            ->expects($this->any())
             ->method('getBasDn')
             ->will($this->returnValue('OU=Users,DC=kanboard,DC=local'));
 
diff --git a/tests/units/Core/User/UserPropertyTest.php b/tests/units/Core/User/UserPropertyTest.php
index a2f052f4..30e651cb 100644
--- a/tests/units/Core/User/UserPropertyTest.php
+++ b/tests/units/Core/User/UserPropertyTest.php
@@ -56,6 +56,30 @@ class UserPropertyTest extends Base
         );
 
         $this->assertEquals($expected, UserProperty::filterProperties($profile, $properties));
+
+        $profile = array(
+            'id' => 123,
+            'username' => 'bob',
+            'name' => null,
+            'email' => '',
+            'other_column' => 'myvalue',
+            'role' => Role::APP_ADMIN,
+        );
+
+        $properties = array(
+            'external_id' => '456',
+            'username' => 'bobby',
+            'name' => 'Bobby',
+            'email' => 'admin@localhost',
+            'role' => null,
+        );
+
+        $expected = array(
+            'name' => 'Bobby',
+            'email' => 'admin@localhost',
+        );
+
+        $this->assertEquals($expected, UserProperty::filterProperties($profile, $properties));
     }
 
     public function testFilterPropertiesOverrideExistingValueWhenNecessary()