From d795af04dfa69fe1fc95f13aa0b0d43f3bdac268 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Wed, 4 May 2022 21:03:13 +0100
Subject: [PATCH] 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
---
app/Auth/Access/GroupSyncService.php | 18 ++++++--
database/factories/Auth/RoleFactory.php | 1 +
tests/Auth/GroupSyncServiceTest.php | 59 +++++++++++++++++++++++++
3 files changed, 74 insertions(+), 4 deletions(-)
create mode 100644 tests/Auth/GroupSyncServiceTest.php
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