From: Dan Brown <redacted>
Date: Thu, 24 Feb 2022 14:16:09 +0000 (+0000)
Subject: Updated OIDC error handling for better error reporting
X-Git-Tag: v22.02~1^2~7
X-Git-Url: http://source.bookstackapp.com/bookstack/commitdiff_plain/ce566bea2a2f5e120cc09808f9d3a4aee79589c2?ds=inline

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
index 000000000..d65661d63
--- /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 e2f364e89..c14be6892 100644
--- 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 b8e017b4b..b4a6a0f08 100644
--- 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 e037fcb8e..e8d61305e 100644
--- 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 ced478090..307916db7 100644
--- 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
index 7bbc4bdaf..000000000
--- 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 ff93dd803..571caa3c7 100644
--- 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 9fa4d0012..9aebb4d04 100644
--- 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()