Added ability to escape role "External Auth ID" commas

- Using a backslash in this field before a comma.
- Could potentially (Although unlikely) be a breaking change.

For #3405

diff --git a/app/Auth/Access/GroupSyncService.php b/app/Auth/Access/GroupSyncService.php
index db19b007ac32b31f5a2bb6112514b6aa7b271826..74f0539d82ffcce72d8261be3e02ca6dcd2dddef 100644 (file)
--- 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 a952e0487f1677e2b2876c360eb2970cfe6c9a64..6dbc1394bef273f14fe899e799f057442470bcda 100644 (file)
--- 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 (file)
index 0000000..ee8dee0
--- /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