Merge branch 'development' into release

Author: ssddanbrown

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 @@ public function updateRememberToken(Authenticatable $user, $token)
      */
     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']);
     }
 
     /**

app/Access/Guards/ExternalBaseSessionGuard.php

@@ -30,7 +30,7 @@ class ExternalBaseSessionGuard implements StatefulGuard
     /**
      * The user we last attempted to retrieve.
      */
-    protected Authenticatable|null $lastAttempted;
+    protected Authenticatable|null $lastAttempted = null;
 
     /**
      * The session used by the guard.
@@ -203,7 +203,7 @@ protected function clearUserDataFromStorage(): void
     /**
      * Get the last user we attempted to authenticate.
      */
-    public function getLastAttempted(): Authenticatable
+    public function getLastAttempted(): Authenticatable|null
     {
         return $this->lastAttempted;
     }

app/Access/LoginService.php

@@ -13,6 +13,8 @@
 use BookStack\Theming\ThemeEvents;
 use BookStack\Users\Models\User;
 use Exception;
+use Illuminate\Contracts\Auth\Authenticatable;
+use Illuminate\Support\Facades\Hash;
 
 class LoginService
 {
@@ -171,10 +173,24 @@ public function attempt(array $credentials, string $method, bool $remember = fal
             } catch (LoginAttemptInvalidUserException $e) {
                 // Catch and return false for non-login accounts
                 // so it looks like a normal invalid login.
-                return false;
+                $result = false;
             }
         }
 
+        // Perform a dummy hash check to balance out the time of a login with an existing known user
+        // with that of a user not in the system (which we don't perform a hash check for in the above).
+        /** @var Authenticatable|null $lastAttempted */
+        $lastAttempted = auth()->getLastAttempted();
+        if (!$result && $lastAttempted === null) {
+            Hash::check($credentials['password'], '$2y$04$A.H9icXH4/lxLd9DHuaYqO/GVBd0OKetxyY0txmNfTAlPLVnTBx3y');
+        }
+
+        // Add some noise to request times on failed login attempts
+        if (!$result) {
+            $sleepMs = random_int(0, 250);
+            usleep($sleepMs * 1000);
+        }
+
         return $result;
     }
 

app/Access/RegistrationService.php

@@ -52,9 +52,7 @@ protected function registrationAllowed(): bool
      */
     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 = [

app/App/Providers/AuthServiceProvider.php

@@ -10,6 +10,7 @@
 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 @@ public function boot(): void
     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.

app/Exceptions/ApiAuthException.php

@@ -4,7 +4,7 @@
 
 use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
 
-class ApiAuthException extends \Exception implements HttpExceptionInterface
+class ApiAuthException extends \Exception implements HttpExceptionInterface, ShowsApiExceptionMessage
 {
     protected int $status;
 
@@ -23,4 +23,9 @@ public function getHeaders(): array
     {
         return [];
     }
+
+    public function getMessageForApi(): string
+    {
+        return $this->getMessage();
+    }
 }

app/Exceptions/Handler.php

@@ -126,16 +126,25 @@ protected function renderApiException(Throwable $e): JsonResponse
             $headers = $e->getHeaders();
         }
 
-        if ($e instanceof ModelNotFoundException) {
-            $code = 404;
-        }
-
         $responseData = [
             'error' => [
-                'message' => $e->getMessage(),
+                'message' => 'An error occurred',
             ],
         ];
 
+        if ($e instanceof ModelNotFoundException) {
+            $responseData['error']['message'] = 'The requested resource could not be found.';
+            $code = 404;
+        }
+
+        if ($e instanceof ShowsApiExceptionMessage) {
+            $responseData['error']['message'] = $e->getMessageForApi();
+        }
+
+        if (app()->hasDebugModeEnabled()) {
+            $responseData['error']['message'] = $e->getMessage();
+        }
+
         if ($e instanceof ValidationException) {
             $responseData['error']['message'] = 'The given data was invalid.';
             $responseData['error']['validation'] = $e->errors();

app/Exceptions/NotifyException.php

@@ -8,13 +8,13 @@
 
 /**
  * An exception that is thrown to notify the user of something which went wrong.
- * Typically these should be translated messages since they will be shown to the end user
- * via a pop up notification error message in the UI.
+ * Typically, these should be translated messages since they will be shown to the end user
+ * via a pop-up notification error message in the UI.
  *
  * This exception is not intended to be used for internal system/application errors,
  * and therefore will not be logged by the exception handler.
  */
-class NotifyException extends Exception implements Responsable, HttpExceptionInterface
+class NotifyException extends Exception implements Responsable, HttpExceptionInterface, ShowsApiExceptionMessage
 {
     public function __construct(
         string $message,
@@ -61,4 +61,9 @@ public function toResponse($request)
 
         return redirect($this->redirectLocation);
     }
+
+    public function getMessageForApi(): string
+    {
+        return $this->getMessage();
+    }
 }

app/Exceptions/PermissionsException.php

@@ -4,6 +4,10 @@
 
 use Exception;
 
-class PermissionsException extends Exception
+class PermissionsException extends Exception implements ShowsApiExceptionMessage
 {
+    public function getMessageForApi(): string
+    {
+        return $this->getMessage();
+    }
 }

app/Exceptions/PrettyException.php

@@ -6,7 +6,7 @@
 use Illuminate\Contracts\Support\Responsable;
 use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
 
-class PrettyException extends Exception implements Responsable, HttpExceptionInterface
+class PrettyException extends Exception implements Responsable, HttpExceptionInterface, ShowsApiExceptionMessage
 {
     protected ?string $subtitle = null;
     protected ?string $details = null;
@@ -56,4 +56,16 @@ public function getHeaders(): array
     {
         return [];
     }
+
+    public function getMessageForApi(): string
+    {
+        $message = $this->getMessage() . '.';
+        if ($this->subtitle) {
+            $message .= " {$this->subtitle}.";
+        }
+        if ($this->details) {
+            $message .= " {$this->details}.";
+        }
+        return $message;
+    }
 }