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