diff --git a/app/Auth/Access/GroupSyncService.php b/app/Auth/Access/GroupSyncService.php index db19b007a..74f0539d8 100644 --- a/app/Auth/Access/GroupSyncService.php +++ b/app/Auth/Access/GroupSyncService.php @@ -28,10 +28,8 @@ class GroupSyncService */ protected function externalIdMatchesGroupNames(string $externalId, array $groupNames): bool { - $externalAuthIds = explode(',', strtolower($externalId)); - - foreach ($externalAuthIds as $externalAuthId) { - if (in_array(trim($externalAuthId), $groupNames)) { + foreach ($this->parseRoleExternalAuthId($externalId) as $externalAuthId) { + if (in_array($externalAuthId, $groupNames)) { return true; } } @@ -39,6 +37,18 @@ class GroupSyncService return false; } + protected function parseRoleExternalAuthId(string $externalId): array + { + $inputIds = preg_split('/(?<!\\\),/', $externalId); + $cleanIds = []; + + foreach ($inputIds as $inputId) { + $cleanIds[] = str_replace('\,', ',', trim($inputId)); + } + + return $cleanIds; + } + /** * Match an array of group names to BookStack system roles. * Formats group names to be lower-case and hyphenated. diff --git a/database/factories/Auth/RoleFactory.php b/database/factories/Auth/RoleFactory.php index a952e0487..6dbc1394b 100644 --- a/database/factories/Auth/RoleFactory.php +++ b/database/factories/Auth/RoleFactory.php @@ -23,6 +23,7 @@ class RoleFactory extends Factory return [ 'display_name' => $this->faker->sentence(3), 'description' => $this->faker->sentence(10), + 'external_auth_id' => '', ]; } } diff --git a/tests/Auth/GroupSyncServiceTest.php b/tests/Auth/GroupSyncServiceTest.php new file mode 100644 index 000000000..ee8dee008 --- /dev/null +++ b/tests/Auth/GroupSyncServiceTest.php @@ -0,0 +1,59 @@ +<?php + +namespace Tests\Auth; + +use BookStack\Auth\Access\GroupSyncService; +use BookStack\Auth\Role; +use BookStack\Auth\User; +use Tests\TestCase; + +class GroupSyncServiceTest extends TestCase +{ + + public function test_user_is_assigned_to_matching_roles() + { + $user = $this->getViewer(); + + $roleA = Role::factory()->create(['display_name' => 'Wizards']); + $roleB = Role::factory()->create(['display_name' => 'Gremlins']); + $roleC = Role::factory()->create(['display_name' => 'ABC123', 'external_auth_id' => 'sales']); + $roleD = Role::factory()->create(['display_name' => 'DEF456', 'external_auth_id' => 'admin-team']); + + foreach([$roleA, $roleB, $roleC, $roleD] as $role) { + $this->assertFalse($user->hasRole($role->id)); + } + + (new GroupSyncService())->syncUserWithFoundGroups($user, ['Wizards', 'Gremlinz', 'Sales', 'Admin Team'], false); + + $user = User::query()->find($user->id); + $this->assertTrue($user->hasRole($roleA->id)); + $this->assertFalse($user->hasRole($roleB->id)); + $this->assertTrue($user->hasRole($roleC->id)); + $this->assertTrue($user->hasRole($roleD->id)); + } + + public function test_multiple_values_in_role_external_auth_id_handled() + { + $user = $this->getViewer(); + $role = Role::factory()->create(['display_name' => 'ABC123', 'external_auth_id' => 'sales, engineering, developers, marketers']); + $this->assertFalse($user->hasRole($role->id)); + + (new GroupSyncService())->syncUserWithFoundGroups($user, ['Developers'], false); + + $user = User::query()->find($user->id); + $this->assertTrue($user->hasRole($role->id)); + } + + public function test_commas_can_be_used_in_external_auth_id_if_escaped() + { + $user = $this->getViewer(); + $role = Role::factory()->create(['display_name' => 'ABC123', 'external_auth_id' => 'sales\,-developers, marketers']); + $this->assertFalse($user->hasRole($role->id)); + + (new GroupSyncService())->syncUserWithFoundGroups($user, ['Sales, Developers'], false); + + $user = User::query()->find($user->id); + $this->assertTrue($user->hasRole($role->id)); + } + +} \ No newline at end of file