diff --git a/app/Access/ExternalBaseUserProvider.php b/app/Access/ExternalBaseUserProvider.php index 2165fd459..ef001289d 100644 --- a/app/Access/ExternalBaseUserProvider.php +++ b/app/Access/ExternalBaseUserProvider.php @@ -3,11 +3,17 @@ namespace BookStack\Access; use BookStack\Users\Models\User; +use BookStack\Users\UserRepo; use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Contracts\Auth\UserProvider; class ExternalBaseUserProvider implements UserProvider { + public function __construct( + protected UserRepo $userRepo, + ) { + } + /** * Retrieve a user by their unique identifier. */ @@ -44,9 +50,7 @@ class ExternalBaseUserProvider implements UserProvider */ public function retrieveByCredentials(array $credentials): ?Authenticatable { - return User::query() - ->where('external_auth_id', $credentials['external_auth_id']) - ->first(); + return $this->userRepo->getByExternalAuthId($credentials['external_auth_id']); } /** diff --git a/app/Access/RegistrationService.php b/app/Access/RegistrationService.php index e47479e79..e3ae9b202 100644 --- a/app/Access/RegistrationService.php +++ b/app/Access/RegistrationService.php @@ -52,9 +52,7 @@ class RegistrationService */ public function findOrRegister(string $name, string $email, string $externalId): User { - $user = User::query() - ->where('external_auth_id', '=', $externalId) - ->first(); + $user = $this->userRepo->getByExternalAuthId($externalId); if (is_null($user)) { $userData = [ diff --git a/app/App/Providers/AuthServiceProvider.php b/app/App/Providers/AuthServiceProvider.php index 6a8162521..8c71fee3a 100644 --- a/app/App/Providers/AuthServiceProvider.php +++ b/app/App/Providers/AuthServiceProvider.php @@ -10,6 +10,7 @@ use BookStack\Access\LoginService; use BookStack\Access\RegistrationService; use BookStack\Api\ApiTokenGuard; use BookStack\Users\Models\User; +use BookStack\Users\UserRepo; use Illuminate\Support\Facades\Auth; use Illuminate\Support\ServiceProvider; use Illuminate\Validation\Rules\Password; @@ -60,7 +61,7 @@ class AuthServiceProvider extends ServiceProvider public function register(): void { Auth::provider('external-users', function () { - return new ExternalBaseUserProvider(); + return new ExternalBaseUserProvider($this->app[UserRepo::class]); }); // Bind and provide the default system user as a singleton to the app instance when needed. diff --git a/app/Users/UserRepo.php b/app/Users/UserRepo.php index 2c0897cef..1643756c8 100644 --- a/app/Users/UserRepo.php +++ b/app/Users/UserRepo.php @@ -51,6 +51,25 @@ class UserRepo return User::query()->where('slug', '=', $slug)->firstOrFail(); } + /** + * Get a user by their external auth ID value. + * Returns null if no matching user found. + */ + public function getByExternalAuthId(string $externalId): User|null + { + // We only really expect at most one user from the search, but as an extra layer of defence against database + // normalisation we search possible matches exactly against the value. + $users = User::query()->where('external_auth_id', '=', $externalId)->get(); + + foreach ($users as $user) { + if ($user->external_auth_id === $externalId) { + return $user; + } + } + + return null; + } + /** * Create a new basic instance of user with the given pre-validated data. * diff --git a/database/migrations/2026_07_27_201402_update_users_external_auth_id_collation.php b/database/migrations/2026_07_27_201402_update_users_external_auth_id_collation.php new file mode 100644 index 000000000..9f9267c88 --- /dev/null +++ b/database/migrations/2026_07_27_201402_update_users_external_auth_id_collation.php @@ -0,0 +1,31 @@ +string('external_auth_id') + ->collation('utf8mb4_bin') + ->change(); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('users', function (Blueprint $table) { + $table->string('external_auth_id') + ->change(); + }); + } +}; diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index d1f128a50..5adbe11a1 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -207,6 +207,36 @@ class LdapTest extends TestCase $this->assertDatabaseHas('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => 'cooluser456']); } + public function test_login_uses_exact_match_for_external_auth_id_values() + { + config()->set(['services.ldap.id_attribute' => 'my_custom_id']); + // External auth id the same as we expect below but different casing + User::query()->forceCreate([ + 'email' => 'otheruser@example.com', + 'external_auth_id' => 'CoolUser456', + 'email_confirmed' => true, + 'name' => 'Barry Scott', + ]); + + $this->commonLdapMocks(1, 1, 1, 2, 1); + $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn'); + $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) + ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->andReturn(['count' => 1, 0 => [ + 'cn' => [$this->mockUser->name], + 'dn' => $ldapDn, + 'my_custom_id' => ['cooluser456'], + 'mail' => [$this->mockUser->email], + ]]); + + $resp = $this->mockUserLogin(); + $resp->assertRedirect('/'); + + $this->assertDatabaseHas('users', ['email' => $this->mockUser->email]); + $this->assertEquals($this->mockUser->email, user()->email); + $this->assertEquals('cooluser456', user()->external_auth_id); + } + public function test_user_filter_default_placeholder_format() { config()->set('services.ldap.user_filter', '(&(uid={user}))'); diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index 8508568f1..0fcfdc5ff 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -471,7 +471,28 @@ class OidcTest extends TestCase $this->assertEquals('xXBennyTheGeezXx', $user->external_auth_id); } - public function test_auth_uses_mulitple_display_name_claims_if_configured() + public function test_auth_uses_external_id_as_exact_value() + { + // External auth id the same as we expect below but different casing + User::query()->forceCreate([ + 'email' => 'otheruser@example.com', + 'external_auth_id' => 'Benni202', + 'email_confirmed' => true, + 'name' => 'Barry Scott', + ]); + + $resp = $this->runLogin([ + 'email' => 'benny@example.com', + 'sub' => 'benni202', + ]); + $resp->assertRedirect('/'); + + $this->assertDatabaseHas('users', ['email' => 'benny@example.com']); + $this->assertEquals('benny@example.com', user()->email); + $this->assertEquals('benni202', user()->external_auth_id); + } + + public function test_auth_uses_multiple_display_name_claims_if_configured() { config()->set(['oidc.display_name_claims' => 'first_name|last_name']); diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php index 6a3063bcf..fbaffceb3 100644 --- a/tests/Auth/Saml2Test.php +++ b/tests/Auth/Saml2Test.php @@ -419,6 +419,29 @@ class Saml2Test extends TestCase $acsPost->assertSee('A user with the email user@example.com already exists but with different credentials'); } + public function test_login_uses_exact_match_for_external_auth_values() + { + $this->post('/saml2/login'); + config()->set(['saml2.onelogin.strict' => false]); + + // Make the user pre-existing in DB with auth_id of different casing + User::query()->forceCreate([ + 'email' => 'otheruser@example.com', + 'external_auth_id' => 'UsEr', + 'email_confirmed' => true, + 'name' => 'Barry Scott', + ]); + + $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]); + + $this->assertTrue($this->isAuthenticated()); + $this->assertDatabaseHas('users', [ + 'email' => 'user@example.com', + ]); + $this->assertEquals('user@example.com', user()->email); + $this->assertEquals('user', user()->external_auth_id); + } + public function test_login_request_contains_expected_default_authncontext() { $authReq = $this->getAuthnRequest();