Updated OIDC error handling for better error reporting

Fixes issue where certain errors would not show to the user
due to extra navigation jumps which lost the error message
in the process.
This simplifies and aligns exceptions with more directly
handled exception usage at the controller level.

Fixes #3264

diff --git a/app/Auth/Access/Oidc/OidcException.php b/app/Auth/Access/Oidc/OidcException.php
new file mode 100644 (file)
index 0000000..d65661d
--- /dev/null
+++ b/app/Auth/Access/Oidc/OidcException.php
@@ -0,0 +1,7 @@
+<?php
+
+namespace BookStack\Auth\Access\Oidc;
+
+use Exception;
+
+class OidcException extends Exception {}

diff --git a/app/Auth/Access/Oidc/OidcIssuerDiscoveryException.php b/app/Auth/Access/Oidc/OidcIssuerDiscoveryException.php
index e2f364e89db97a198be3116a3b17e9b8f2e272a0..c14be689253783c040f045cf05618066b242c412 100644 (file)
--- a/app/Auth/Access/Oidc/OidcIssuerDiscoveryException.php
+++ b/app/Auth/Access/Oidc/OidcIssuerDiscoveryException.php
@@ -2,6 +2,6 @@
 
 namespace BookStack\Auth\Access\Oidc;
 
-class OidcIssuerDiscoveryException extends \Exception
-{
-}
+use Exception;
+
+class OidcIssuerDiscoveryException extends Exception {}

diff --git a/app/Auth/Access/Oidc/OidcService.php b/app/Auth/Access/Oidc/OidcService.php
index b8e017b4b135c85fd3b7aee9afbce252b0b372c1..b4a6a0f089d8952f8fc15042bc54e9a2c86c47e2 100644 (file)
--- a/app/Auth/Access/Oidc/OidcService.php
+++ b/app/Auth/Access/Oidc/OidcService.php
@@ -2,20 +2,18 @@
 
 namespace BookStack\Auth\Access\Oidc;
 
-use function auth;
 use BookStack\Auth\Access\LoginService;
 use BookStack\Auth\Access\RegistrationService;
 use BookStack\Auth\User;
 use BookStack\Exceptions\JsonDebugException;
-use BookStack\Exceptions\OpenIdConnectException;
 use BookStack\Exceptions\StoppedAuthenticationException;
 use BookStack\Exceptions\UserRegistrationException;
-use function config;
-use Exception;
 use Illuminate\Support\Facades\Cache;
 use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider;
-use Psr\Http\Client\ClientExceptionInterface;
+use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
 use Psr\Http\Client\ClientInterface as HttpClient;
+use function auth;
+use function config;
 use function trans;
 use function url;
 
@@ -25,9 +23,9 @@ use function url;
  */
 class OidcService
 {
-    protected $registrationService;
-    protected $loginService;
-    protected $httpClient;
+    protected RegistrationService $registrationService;
+    protected LoginService $loginService;
+    protected HttpClient $httpClient;
 
     /**
      * OpenIdService constructor.
@@ -43,6 +41,7 @@ class OidcService
      * Initiate an authorization flow.
      *
      * @return array{url: string, state: string}
+     * @throws OidcException
      */
     public function login(): array
     {
@@ -57,14 +56,15 @@ class OidcService
 
     /**
      * Process the Authorization response from the authorization server and
-     * return the matching, or new if registration active, user matched to
-     * the authorization server.
-     * Returns null if not authenticated.
+     * return the matching, or new if registration active, user matched to the
+     * authorization server. Throws if the user cannot be auth if not authenticated.
      *
-     * @throws Exception
-     * @throws ClientExceptionInterface
+     * @throws JsonDebugException
+     * @throws OidcException
+     * @throws StoppedAuthenticationException
+     * @throws IdentityProviderException
      */
-    public function processAuthorizeResponse(?string $authorizationCode): ?User
+    public function processAuthorizeResponse(?string $authorizationCode): User
     {
         $settings = $this->getProviderSettings();
         $provider = $this->getProvider($settings);
@@ -77,9 +77,9 @@ class OidcService
         return $this->processAccessTokenCallback($accessToken, $settings);
     }
 
+
     /**
-     * @throws OidcIssuerDiscoveryException
-     * @throws ClientExceptionInterface
+     * @throws OidcException
      */
     protected function getProviderSettings(): OidcProviderSettings
     {
@@ -100,7 +100,11 @@ class OidcService
 
         // Run discovery
         if ($config['discover'] ?? false) {
-            $settings->discoverFromIssuer($this->httpClient, Cache::store(null), 15);
+            try {
+                $settings->discoverFromIssuer($this->httpClient, Cache::store(null), 15);
+            } catch (OidcIssuerDiscoveryException $exception) {
+                throw new OidcException('OIDC Discovery Error: ' . $exception->getMessage());
+            }
         }
 
         $settings->validate();
@@ -161,9 +165,8 @@ class OidcService
      * Processes a received access token for a user. Login the user when
      * they exist, optionally registering them automatically.
      *
-     * @throws OpenIdConnectException
+     * @throws OidcException
      * @throws JsonDebugException
-     * @throws UserRegistrationException
      * @throws StoppedAuthenticationException
      */
     protected function processAccessTokenCallback(OidcAccessToken $accessToken, OidcProviderSettings $settings): User
@@ -182,28 +185,28 @@ class OidcService
         try {
             $idToken->validate($settings->clientId);
         } catch (OidcInvalidTokenException $exception) {
-            throw new OpenIdConnectException("ID token validate failed with error: {$exception->getMessage()}");
+            throw new OidcException("ID token validate failed with error: {$exception->getMessage()}");
         }
 
         $userDetails = $this->getUserDetails($idToken);
         $isLoggedIn = auth()->check();
 
         if (empty($userDetails['email'])) {
-            throw new OpenIdConnectException(trans('errors.oidc_no_email_address'));
+            throw new OidcException(trans('errors.oidc_no_email_address'));
         }
 
         if ($isLoggedIn) {
-            throw new OpenIdConnectException(trans('errors.oidc_already_logged_in'), '/login');
+            throw new OidcException(trans('errors.oidc_already_logged_in'));
         }
 
-        $user = $this->registrationService->findOrRegister(
-            $userDetails['name'],
-            $userDetails['email'],
-            $userDetails['external_id']
-        );
-
-        if ($user === null) {
-            throw new OpenIdConnectException(trans('errors.oidc_user_not_registered', ['name' => $userDetails['external_id']]), '/login');
+        try {
+            $user = $this->registrationService->findOrRegister(
+                $userDetails['name'],
+                $userDetails['email'],
+                $userDetails['external_id']
+            );
+        } catch (UserRegistrationException $exception) {
+            throw new OidcException($exception->getMessage());
         }
 
         $this->loginService->login($user, 'oidc');

diff --git a/app/Exceptions/JsonDebugException.php b/app/Exceptions/JsonDebugException.php
index e037fcb8e9041dd7bcb4f8115af350b299cb148d..e8d61305e1232eb748b007d8c506e85ae39a0385 100644 (file)
--- a/app/Exceptions/JsonDebugException.php
+++ b/app/Exceptions/JsonDebugException.php
@@ -3,23 +3,25 @@
 namespace BookStack\Exceptions;
 
 use Exception;
+use Illuminate\Http\JsonResponse;
 
 class JsonDebugException extends Exception
 {
-    protected $data;
+    protected array $data;
 
     /**
      * JsonDebugException constructor.
      */
-    public function __construct($data)
+    public function __construct(array $data)
     {
         $this->data = $data;
+        parent::__construct();
     }
 
     /**
      * Covert this exception into a response.
      */
-    public function render()
+    public function render(): JsonResponse
     {
         return response()->json($this->data);
     }

diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php
index ced4780900669897d2a9a8b18b5791968287613f..307916db7a7e7e3fc5635fd9bf76085854b7439d 100644 (file)
--- a/app/Exceptions/NotifyException.php
+++ b/app/Exceptions/NotifyException.php
@@ -11,9 +11,6 @@ class NotifyException extends Exception implements Responsable
     public $redirectLocation;
     protected $status;
 
-    /**
-     * NotifyException constructor.
-     */
     public function __construct(string $message, string $redirectLocation = '/', int $status = 500)
     {
         $this->message = $message;

diff --git a/app/Exceptions/OpenIdConnectException.php b/app/Exceptions/OpenIdConnectException.php
deleted file mode 100644 (file)
index 7bbc4bd..0000000
--- a/app/Exceptions/OpenIdConnectException.php
+++ /dev/null
@@ -1,7 +0,0 @@
-<?php
-
-namespace BookStack\Exceptions;
-
-class OpenIdConnectException extends NotifyException
-{
-}

diff --git a/app/Http/Controllers/Auth/OidcController.php b/app/Http/Controllers/Auth/OidcController.php
index ff93dd803aa822b82138d81332cd28125821b504..571caa3c7b64e4a91c5fc967bc7c3e01503556a9 100644 (file)
--- a/app/Http/Controllers/Auth/OidcController.php
+++ b/app/Http/Controllers/Auth/OidcController.php
@@ -3,12 +3,13 @@
 namespace BookStack\Http\Controllers\Auth;
 
 use BookStack\Auth\Access\Oidc\OidcService;
+use BookStack\Auth\Access\Oidc\OidcException;
 use BookStack\Http\Controllers\Controller;
 use Illuminate\Http\Request;
 
 class OidcController extends Controller
 {
-    protected $oidcService;
+    protected OidcService $oidcService;
 
     /**
      * OpenIdController constructor.
@@ -24,7 +25,13 @@ class OidcController extends Controller
      */
     public function login()
     {
-        $loginDetails = $this->oidcService->login();
+        try {
+            $loginDetails = $this->oidcService->login();
+        } catch (OidcException $exception) {
+            $this->showErrorNotification($exception->getMessage());
+            return redirect('/login');
+        }
+
         session()->flash('oidc_state', $loginDetails['state']);
 
         return redirect($loginDetails['url']);
@@ -45,7 +52,12 @@ class OidcController extends Controller
             return redirect('/login');
         }
 
-        $this->oidcService->processAuthorizeResponse($request->query('code'));
+        try {
+            $this->oidcService->processAuthorizeResponse($request->query('code'));
+        } catch (OidcException $oidcException) {
+            $this->showErrorNotification($oidcException->getMessage());
+            return redirect('/login');
+        }
 
         return redirect()->intended();
     }

diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index 9fa4d0012a41c9460b0cf35f5d9b9e89fa832b58..9aebb4d04a6694c812c6d2a57f74f1f3660257bf 100644 (file)
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -6,14 +6,13 @@ use BookStack\Actions\ActivityType;
 use BookStack\Auth\User;
 use GuzzleHttp\Psr7\Request;
 use GuzzleHttp\Psr7\Response;
-use Illuminate\Filesystem\Cache;
 use Tests\Helpers\OidcJwtHelper;
 use Tests\TestCase;
 use Tests\TestResponse;
 
 class OidcTest extends TestCase
 {
-    protected $keyFilePath;
+    protected string $keyFilePath;
     protected $keyFile;
 
     protected function setUp(): void
@@ -236,22 +235,24 @@ class OidcTest extends TestCase
 
         $this->assertFalse(auth()->check());
 
-        $this->runLogin([
+        $resp = $this->runLogin([
             'email' => $editor->email,
             'sub'   => 'benny505',
         ]);
+        $resp = $this->followRedirects($resp);
 
-        $this->assertSessionError('A user with the email ' . $editor->email . ' already exists but with different credentials.');
+        $resp->assertSeeText('A user with the email ' . $editor->email . ' already exists but with different credentials.');
         $this->assertFalse(auth()->check());
     }
 
     public function test_auth_login_with_invalid_token_fails()
     {
-        $this->runLogin([
+        $resp = $this->runLogin([
             'sub' => null,
         ]);
+        $resp = $this->followRedirects($resp);
 
-        $this->assertSessionError('ID token validate failed with error: Missing token subject value');
+        $resp->assertSeeText('ID token validate failed with error: Missing token subject value');
         $this->assertFalse(auth()->check());
     }
 
@@ -287,9 +288,9 @@ class OidcTest extends TestCase
             new Response(404, [], 'Not found'),
         ]);
 
-        $this->runLogin();
+        $resp = $this->followRedirects($this->runLogin());
         $this->assertFalse(auth()->check());
-        $this->assertSessionError('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
+        $resp->assertSeeText('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
     }
 
     public function test_autodiscovery_calls_are_cached()