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

app/Auth/Access/Oidc/OidcException.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace BookStack\Auth\Access\Oidc;
+
+use Exception;
+
+class OidcException extends Exception {}

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 {}

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 RegistrationService $registrationService;
-    protected $loginService;
+    protected LoginService $loginService;
-    protected $httpClient;
+    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
+     * return the matching, or new if registration active, user matched to the
-     * the authorization server.
+     * authorization server. Throws if the user cannot be auth if not authenticated.
-     * Returns null if not authenticated.
      *
-     * @throws Exception
+     * @throws JsonDebugException
-     * @throws ClientExceptionInterface
+     * @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 OidcException
-     * @throws ClientExceptionInterface
      */
     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(
+        try {
-            $userDetails['name'],
+            $user = $this->registrationService->findOrRegister(
-            $userDetails['email'],
+                $userDetails['name'],
-            $userDetails['external_id']
+                $userDetails['email'],
-        );
+                $userDetails['external_id']
-
+            );
-        if ($user === null) {
+        } catch (UserRegistrationException $exception) {
-            throw new OpenIdConnectException(trans('errors.oidc_user_not_registered', ['name' => $userDetails['external_id']]), '/login');
+            throw new OidcException($exception->getMessage());
         }
 
         $this->loginService->login($user, 'oidc');

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);
     }

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;

app/Exceptions/OpenIdConnectException.php

@@ -1,7 +0,0 @@
-<?php
-
-namespace BookStack\Exceptions;
-
-class OpenIdConnectException extends NotifyException
-{
-}

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();
     }

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()