Added ability to set custom ldap group -> role mapping

Added input in role form to allow matching against custom names. Changed default mapping to use role display name instead of the hidden DB name.
rosetta-ai · Jul 15, 2018 · f421d83 · f421d83
1 parent be2ca9d
commit f421d83
Show file tree

Hide file tree

Showing 9 changed files with 146 additions and 26 deletions.
diff --git a/.env.example b/.env.example
@@ -71,8 +71,6 @@ LDAP_VERSION=false
 LDAP_USER_TO_GROUPS=false
 # What is the LDAP attribute for group memberships
 LDAP_GROUP_ATTRIBUTE="memberOf"
-# What LDAP group should the user be a part of to be an admin on BookStack
-LDAP_ADMIN_GROUP="Domain Admins"
 # Would you like to remove users from roles on BookStack if they do not match on LDAP
 # If false, the ldap groups-roles sync will only add users to roles
 LDAP_REMOVE_FROM_GROUPS=false

diff --git a/app/Http/Controllers/PermissionController.php b/app/Http/Controllers/PermissionController.php
@@ -78,6 +78,7 @@ public function editRole($id)
      * @param $id
      * @param Request $request
      * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector
+     * @throws PermissionsException
      */
     public function updateRole($id, Request $request)
     {

diff --git a/app/Role.php b/app/Role.php
@@ -3,7 +3,7 @@
 class Role extends Model
 {
 
-    protected $fillable = ['display_name', 'description'];
+    protected $fillable = ['display_name', 'description', 'external_auth_id'];
 
     /**
      * The roles that belong to the role.

diff --git a/app/Services/LdapService.php b/app/Services/LdapService.php
@@ -5,6 +5,7 @@
 use BookStack\Role;
 use BookStack\User;
 use Illuminate\Contracts\Auth\Authenticatable;
+use Illuminate\Database\Eloquent\Builder;
 
 /**
  * Class LdapService
@@ -299,15 +300,13 @@ protected function groupFilter($ldapSearchReturn)
      * Sync the LDAP groups to the user roles for the current user
      * @param \BookStack\User $user
      * @throws LdapException
-     * @throws \BookStack\Exceptions\NotFoundException
      */
     public function syncGroups(User $user)
     {
         $userLdapGroups = $this->getUserGroups($user->external_auth_id);
-        $userLdapGroups = $this->groupNameFilter($userLdapGroups);
 
         // Get the ids for the roles from the names
-        $ldapGroupsAsRoles = Role::query()->whereIn('name', $userLdapGroups)->pluck('id');
+        $ldapGroupsAsRoles = $this->matchLdapGroupsToSystemsRoles($userLdapGroups);
 
         // Sync groups
         if ($this->config['remove_from_groups']) {
@@ -316,26 +315,55 @@ public function syncGroups(User $user)
         } else {
             $user->roles()->syncWithoutDetaching($ldapGroupsAsRoles);
         }
+    }
 
-        // make the user an admin?
-        // TODO - Remove
-        if (in_array($this->config['admin'], $userLdapGroups)) {
-            $this->userRepo->attachSystemRole($user, 'admin');
+    /**
+     * Match an array of group names from LDAP to BookStack system roles.
+     * Formats LDAP group names to be lower-case and hyphenated.
+     * @param array $groupNames
+     * @return \Illuminate\Support\Collection
+     */
+    protected function matchLdapGroupsToSystemsRoles(array $groupNames)
+    {
+        foreach ($groupNames as $i => $groupName) {
+            $groupNames[$i] = str_replace(' ', '-', trim(strtolower($groupName)));
         }
+
+        $roles = Role::query()->where(function(Builder $query) use ($groupNames) {
+            $query->whereIn('name', $groupNames);
+            foreach ($groupNames as $groupName) {
+                $query->orWhere('external_auth_id', 'LIKE', '%' . $groupName . '%');
+            }
+        })->get();
+
+        $matchedRoles = $roles->filter(function(Role $role) use ($groupNames) {
+            return $this->roleMatchesGroupNames($role, $groupNames);
+        });
+
+        return $matchedRoles->pluck('id');
     }
 
     /**
-     * Filter to convert the groups from ldap to the format of the roles name on BookStack
-     * Spaces replaced with -, all lowercase letters
-     * @param array $groups
-     * @return array
+     * Check a role against an array of group names to see if it matches.
+     * Checked against role 'external_auth_id' if set otherwise the name of the role.
+     * @param Role $role
+     * @param array $groupNames
+     * @return bool
      */
-    private function groupNameFilter(array $groups)
+    protected function roleMatchesGroupNames(Role $role, array $groupNames)
     {
-        $return = [];
-        foreach ($groups as $groupName) {
-            $return[] = str_replace(' ', '-', strtolower($groupName));
+        if ($role->external_auth_id) {
+            $externalAuthIds = explode(',', strtolower($role->external_auth_id));
+            foreach ($externalAuthIds as $externalAuthId) {
+                if (in_array(trim($externalAuthId), $groupNames)) {
+                    return true;
+                }
+            }
+            return false;
         }
-        return $return;
+
+        $roleName = str_replace(' ', '-', trim(strtolower($role->display_name)));
+        return in_array($roleName, $groupNames);
     }
+
 }
diff --git a/config/services.php b/config/services.php
@@ -120,7 +120,6 @@
         'follow_referrals' => env('LDAP_FOLLOW_REFERRALS', false),
 		'user_to_groups' => env('LDAP_USER_TO_GROUPS',false),
 		'group_attribute' => env('LDAP_GROUP_ATTRIBUTE', 'memberOf'),
-		'admin' => env('LDAP_ADMIN_GROUP','Domain Admins'),
 		'remove_from_groups' => env('LDAP_REMOVE_FROM_GROUPS',false),
 	]
 

diff --git a/database/migrations/2018_07_15_173514_add_role_external_auth_id.php b/database/migrations/2018_07_15_173514_add_role_external_auth_id.php
@@ -0,0 +1,33 @@
+<?php
+
+use Illuminate\Support\Facades\Schema;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Database\Migrations\Migration;
+
+class AddRoleExternalAuthId extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::table('roles', function (Blueprint $table) {
+            $table->string('external_auth_id', 200)->default('');
+            $table->index('external_auth_id');
+        });
+    }
+
+    /**
+     * Reverse the migrations.
+     *
+     * @return void
+     */
+    public function down()
+    {
+        Schema::table('roles', function (Blueprint $table) {
+            $table->dropColumn('external_auth_id');
+        });
+    }
+}
diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php
@@ -82,6 +82,7 @@
     'role_details' => 'Role Details',
     'role_name' => 'Role Name',
     'role_desc' => 'Short Description of Role',
+    'role_external_auth_id' => 'External Authentication IDs',
     'role_system' => 'System Permissions',
     'role_manage_users' => 'Manage users',
     'role_manage_roles' => 'Manage roles & role permissions',

diff --git a/resources/views/settings/roles/form.blade.php b/resources/views/settings/roles/form.blade.php
@@ -15,6 +15,14 @@
                         <label for="name">{{ trans('settings.role_desc') }}</label>
                         @include('form/text', ['name' => 'description'])
                     </div>
+
+                    @if(config('auth.method') === 'ldap')
+                        <div class="form-group">
+                            <label for="name">{{ trans('settings.role_external_auth_id') }}</label>
+                            @include('form/text', ['name' => 'external_auth_id'])
+                        </div>
+                    @endif
+
                     <h5>{{ trans('settings.role_system') }}</h5>
                     <label>@include('settings/roles/checkbox', ['permission' => 'users-manage']) {{ trans('settings.role_manage_users') }}</label>
                     <label>@include('settings/roles/checkbox', ['permission' => 'user-roles-manage']) {{ trans('settings.role_manage_roles') }}</label>

diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
@@ -148,8 +148,8 @@ public function test_non_admins_cannot_change_auth_id()
 
     public function test_login_maps_roles_and_retains_existsing_roles()
     {
-        $roleToRecieve = factory(Role::class)->create(['name' => 'ldaptester']);
-        $roleToRecieve2 = factory(Role::class)->create(['name' => 'ldaptester-second']);
+        $roleToReceive = factory(Role::class)->create(['name' => 'ldaptester', 'display_name' => 'LdapTester']);
+        $roleToReceive2 = factory(Role::class)->create(['name' => 'ldaptester-second', 'display_name' => 'LdapTester Second']);
         $existingRole = factory(Role::class)->create(['name' => 'ldaptester-existing']);
         $this->mockUser->forceFill(['external_auth_id' => $this->mockUser->name])->save();
         $this->mockUser->attachRole($existingRole);
@@ -187,11 +187,11 @@ public function test_login_maps_roles_and_retains_existsing_roles()
         $user = User::where('email', $this->mockUser->email)->first();
         $this->seeInDatabase('role_user', [
             'user_id' => $user->id,
-            'role_id' => $roleToRecieve->id
+            'role_id' => $roleToReceive->id
         ]);
         $this->seeInDatabase('role_user', [
             'user_id' => $user->id,
-            'role_id' => $roleToRecieve2->id
+            'role_id' => $roleToReceive2->id
         ]);
         $this->seeInDatabase('role_user', [
             'user_id' => $user->id,
@@ -201,7 +201,7 @@ public function test_login_maps_roles_and_retains_existsing_roles()
 
     public function test_login_maps_roles_and_removes_old_roles_if_set()
     {
-        $roleToRecieve = factory(Role::class)->create(['name' => 'ldaptester']);
+        $roleToReceive = factory(Role::class)->create(['name' => 'ldaptester', 'display_name' => 'LdapTester']);
         $existingRole = factory(Role::class)->create(['name' => 'ldaptester-existing']);
         $this->mockUser->forceFill(['external_auth_id' => $this->mockUser->name])->save();
         $this->mockUser->attachRole($existingRole);
@@ -238,12 +238,64 @@ public function test_login_maps_roles_and_removes_old_roles_if_set()
         $user = User::where('email', $this->mockUser->email)->first();
         $this->seeInDatabase('role_user', [
             'user_id' => $user->id,
-            'role_id' => $roleToRecieve->id
+            'role_id' => $roleToReceive->id
         ]);
         $this->dontSeeInDatabase('role_user', [
             'user_id' => $user->id,
             'role_id' => $existingRole->id
         ]);
     }
 
+    public function test_external_auth_id_visible_in_roles_page_when_ldap_active()
+    {
+        $role = factory(Role::class)->create(['name' => 'ldaptester', 'external_auth_id' => 'ex-auth-a, test-second-param']);
+        $this->asAdmin()->visit('/settings/roles/' . $role->id)
+            ->see('ex-auth-a');
+    }
+
+    public function test_login_maps_roles_using_external_auth_ids_if_set()
+    {
+        $roleToReceive = factory(Role::class)->create(['name' => 'ldaptester', 'external_auth_id' => 'test-second-param, ex-auth-a']);
+        $roleToNotReceive = factory(Role::class)->create(['name' => 'ldaptester-not-receive', 'display_name' => 'ex-auth-a', 'external_auth_id' => 'test-second-param']);
+
+        app('config')->set([
+            'services.ldap.user_to_groups' => true,
+            'services.ldap.group_attribute' => 'memberOf',
+            'services.ldap.remove_from_groups' => true,
+        ]);
+        $this->mockLdap->shouldReceive('connect')->times(2)->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->times(2);
+        $this->mockLdap->shouldReceive('setOption')->times(4);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+            ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
+            ->andReturn(['count' => 1, 0 => [
+                'uid' => [$this->mockUser->name],
+                'cn' => [$this->mockUser->name],
+                'dn' => ['dc=test' . config('services.ldap.base_dn')],
+                'mail' => [$this->mockUser->email],
+                'memberof' => [
+                    'count' => 1,
+                    0 => "cn=ex-auth-a,ou=groups,dc=example,dc=com",
+                ]
+            ]]);
+        $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);
+
+        $this->visit('/login')
+            ->see('Username')
+            ->type($this->mockUser->name, '#username')
+            ->type($this->mockUser->password, '#password')
+            ->press('Log In')
+            ->seePageIs('/');
+
+        $user = User::where('email', $this->mockUser->email)->first();
+        $this->seeInDatabase('role_user', [
+            'user_id' => $user->id,
+            'role_id' => $roleToReceive->id
+        ]);
+        $this->dontSeeInDatabase('role_user', [
+            'user_id' => $user->id,
+            'role_id' => $roleToNotReceive->id
+        ]);
+    }
+
 }