From 5632fef6212001ec8357c854f565b5e6a1df9c0f Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Wed, 11 Dec 2024 14:22:48 +0000
Subject: [PATCH] Auth: Added specific guards against guest account login

Hardened things to enforce the intent that the guest account should not
be used for logins.
Currently this would not be allowed due to empty set password, and no
password fields on user edit forms, but an error could occur if the
login was attempted.

This adds:
- Handling to show normal invalid user warning on login instead of a
  hash check error.
- Prevention of guest user via main login route, in the event that
  inventive workarounds would be used by admins to set a password for
  this account.
- Test for guest user login.
---
 app/Access/LoginService.php                   | 35 +++++++++++++++++--
 .../LoginAttemptInvalidUserException.php      |  7 ++++
 tests/Auth/AuthTest.php                       | 20 +++++++++++
 3 files changed, 59 insertions(+), 3 deletions(-)
 create mode 100644 app/Exceptions/LoginAttemptInvalidUserException.php

diff --git a/app/Access/LoginService.php b/app/Access/LoginService.php
index cc48e0f9b..6607969af 100644
--- a/app/Access/LoginService.php
+++ b/app/Access/LoginService.php
@@ -5,6 +5,7 @@ namespace BookStack\Access;
 use BookStack\Access\Mfa\MfaSession;
 use BookStack\Activity\ActivityType;
 use BookStack\Exceptions\LoginAttemptException;
+use BookStack\Exceptions\LoginAttemptInvalidUserException;
 use BookStack\Exceptions\StoppedAuthenticationException;
 use BookStack\Facades\Activity;
 use BookStack\Facades\Theme;
@@ -29,10 +30,14 @@ class LoginService
      * a reason to (MFA or Unconfirmed Email).
      * Returns a boolean to indicate the current login result.
      *
-     * @throws StoppedAuthenticationException
+     * @throws StoppedAuthenticationException|LoginAttemptInvalidUserException
      */
     public function login(User $user, string $method, bool $remember = false): void
     {
+        if ($user->isGuest()) {
+            throw new LoginAttemptInvalidUserException('Login not allowed for guest user');
+        }
+
         if ($this->awaitingEmailConfirmation($user) || $this->needsMfaVerification($user)) {
             $this->setLastLoginAttemptedForUser($user, $method, $remember);
 
@@ -58,7 +63,7 @@ class LoginService
      *
      * @throws Exception
      */
-    public function reattemptLoginFor(User $user)
+    public function reattemptLoginFor(User $user): void
     {
         if ($user->id !== ($this->getLastLoginAttemptUser()->id ?? null)) {
             throw new Exception('Login reattempt user does align with current session state');
@@ -152,16 +157,40 @@ class LoginService
      */
     public function attempt(array $credentials, string $method, bool $remember = false): bool
     {
+        if ($this->areCredentialsForGuest($credentials)) {
+            return false;
+        }
+
         $result = auth()->attempt($credentials, $remember);
         if ($result) {
             $user = auth()->user();
             auth()->logout();
-            $this->login($user, $method, $remember);
+            try {
+                $this->login($user, $method, $remember);
+            } catch (LoginAttemptInvalidUserException $e) {
+                // Catch and return false for non-login accounts
+                // so it looks like a normal invalid login.
+                return false;
+            }
         }
 
         return $result;
     }
 
+    /**
+     * Check if the given credentials are likely for the system guest account.
+     */
+    protected function areCredentialsForGuest(array $credentials): bool
+    {
+        if (isset($credentials['email'])) {
+            return User::query()->where('email', '=', $credentials['email'])
+                ->where('system_name', '=', 'public')
+                ->exists();
+        }
+
+        return false;
+    }
+
     /**
      * Logs the current user out of the application.
      * Returns an app post-redirect path.
diff --git a/app/Exceptions/LoginAttemptInvalidUserException.php b/app/Exceptions/LoginAttemptInvalidUserException.php
new file mode 100644
index 000000000..163484c5a
--- /dev/null
+++ b/app/Exceptions/LoginAttemptInvalidUserException.php
@@ -0,0 +1,7 @@
+<?php
+
+namespace BookStack\Exceptions;
+
+class LoginAttemptInvalidUserException extends LoginAttemptException
+{
+}
diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php
index 0164978d8..bffd8bbdb 100644
--- a/tests/Auth/AuthTest.php
+++ b/tests/Auth/AuthTest.php
@@ -3,6 +3,7 @@
 namespace Tests\Auth;
 
 use BookStack\Access\Mfa\MfaSession;
+use Illuminate\Support\Facades\Hash;
 use Illuminate\Testing\TestResponse;
 use Tests\TestCase;
 
@@ -144,6 +145,25 @@ class AuthTest extends TestCase
         $resp->assertSee('Too many login attempts. Please try again in');
     }
 
+    public function test_login_specifically_disabled_for_guest_account()
+    {
+        $guest = $this->users->guest();
+
+        $resp = $this->post('/login', ['email' => $guest->email, 'password' => 'password']);
+        $resp->assertRedirect('/login');
+        $resp = $this->followRedirects($resp);
+        $resp->assertSee('These credentials do not match our records.');
+
+        // Test login even with password somehow set
+        $guest->password = Hash::make('password');
+        $guest->save();
+
+        $resp = $this->post('/login', ['email' => $guest->email, 'password' => 'password']);
+        $resp->assertRedirect('/login');
+        $resp = $this->followRedirects($resp);
+        $resp->assertSee('These credentials do not match our records.');
+    }
+
     /**
      * Perform a login.
      */