diff --git a/.env.example b/.env.example index 4bb2555b0..d09db93d3 100644 --- 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 index c4c7fe972..5695705d0 100644 --- a/app/Http/Controllers/PermissionController.php +++ b/app/Http/Controllers/PermissionController.php @@ -78,6 +78,7 @@ class PermissionController extends Controller * @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 index e86854e79..54a5bb180 100644 --- 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 index 7606f1271..4936b2da8 100644 --- a/app/Services/LdapService.php +++ b/app/Services/LdapService.php @@ -5,6 +5,7 @@ use BookStack\Repos\UserRepo; use BookStack\Role; use BookStack\User; use Illuminate\Contracts\Auth\Authenticatable; +use Illuminate\Database\Eloquent\Builder; /** * Class LdapService @@ -299,15 +300,13 @@ class LdapService * 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 @@ class LdapService } else { $user->roles()->syncWithoutDetaching($ldapGroupsAsRoles); } - - // make the user an admin? - // TODO - Remove - if (in_array($this->config['admin'], $userLdapGroups)) { - $this->userRepo->attachSystemRole($user, 'admin'); - } } /** - * 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 + * 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 */ - private function groupNameFilter(array $groups) + protected function matchLdapGroupsToSystemsRoles(array $groupNames) { - $return = []; - foreach ($groups as $groupName) { - $return[] = str_replace(' ', '-', strtolower($groupName)); + foreach ($groupNames as $i => $groupName) { + $groupNames[$i] = str_replace(' ', '-', trim(strtolower($groupName))); } - return $return; + + $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'); } + + /** + * 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 + */ + protected function roleMatchesGroupNames(Role $role, array $groupNames) + { + 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; + } + + $roleName = str_replace(' ', '-', trim(strtolower($role->display_name))); + return in_array($roleName, $groupNames); + } + } diff --git a/config/services.php b/config/services.php index daa160643..9c550f2fa 100644 --- a/config/services.php +++ b/config/services.php @@ -120,7 +120,6 @@ return [ '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 new file mode 100644 index 000000000..706a883a3 --- /dev/null +++ b/database/migrations/2018_07_15_173514_add_role_external_auth_id.php @@ -0,0 +1,33 @@ +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 index 30abbc1b9..d6fbb6107 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -82,6 +82,7 @@ return [ '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 index 15ce7e6d7..6a8e27487 100644 --- a/resources/views/settings/roles/form.blade.php +++ b/resources/views/settings/roles/form.blade.php @@ -15,6 +15,14 @@ @include('form/text', ['name' => 'description']) + + @if(config('auth.method') === 'ldap') +
+ + @include('form/text', ['name' => 'external_auth_id']) +
+ @endif +
{{ trans('settings.role_system') }}
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index ada33d692..64f3fd742 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -148,8 +148,8 @@ class LdapTest extends BrowserKitTest 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 @@ class LdapTest extends BrowserKitTest $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 @@ class LdapTest extends BrowserKitTest 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,7 +238,7 @@ class LdapTest extends BrowserKitTest $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, @@ -246,4 +246,56 @@ class LdapTest extends BrowserKitTest ]); } + 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 + ]); + } + }