From 8b211ed4618299a22bb2a3dda9bb317d8cde65b6 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 21 Jun 2022 15:32:18 +0100 Subject: [PATCH] Review and update of login auto initiation PR For PR #3406 - Updated naming from 'redirect' to 'initate/initation'. - Updated phpunit.xml and .env.example.complete files with the new option. - Cleaned up controller logic a bit. - Added content and design to the new initation view to not leave user on a blank view for a while. - Added non-JS button to initiation view as fallback option for progression. - Moved new test to it's own Test class and expanded with additional scenario tests for better functionality coverage. --- .env.example.complete | 4 + app/Config/auth.php | 7 +- app/Http/Controllers/Auth/LoginController.php | 28 +++++-- phpunit.xml | 1 + resources/lang/en/auth.php | 5 ++ resources/sass/_buttons.scss | 3 + resources/views/auth/login-initiate.blade.php | 37 +++++++++ resources/views/auth/login-redirect.blade.php | 16 ---- tests/Auth/LoginAutoInitiateTest.php | 80 +++++++++++++++++++ tests/Auth/OidcTest.php | 14 ---- 10 files changed, 153 insertions(+), 42 deletions(-) create mode 100644 resources/views/auth/login-initiate.blade.php delete mode 100644 resources/views/auth/login-redirect.blade.php create mode 100644 tests/Auth/LoginAutoInitiateTest.php diff --git a/.env.example.complete b/.env.example.complete index 0c7f8f6a5..c40ab1380 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -143,6 +143,10 @@ STORAGE_URL=false # Can be 'standard', 'ldap', 'saml2' or 'oidc' AUTH_METHOD=standard +# Automatically initiate login via external auth system if it's the only auth method. +# Works with saml2 or oidc auth methods. +AUTH_AUTO_INITIATE=false + # Social authentication configuration # All disabled by default. # Refer to https://www.bookstackapp.com/docs/admin/third-party-auth/ diff --git a/app/Config/auth.php b/app/Config/auth.php index ec4389a6c..37190156a 100644 --- a/app/Config/auth.php +++ b/app/Config/auth.php @@ -13,10 +13,9 @@ return [ // Options: standard, ldap, saml2, oidc 'method' => env('AUTH_METHOD', 'standard'), - // Automatically redirect to external login provider if only one provider is being used - // instead of displaying a single-button login page and requiring users to click through - // Supported methods: saml2, oidc - 'auto_redirect' => env('AUTH_AUTO_REDIRECT', false), + // Automatically initiate login via external auth system if it's the sole auth method. + // Works with saml2 or oidc auth methods. + 'auto_initiate' => env('AUTH_AUTO_INITIATE', false), // Authentication Defaults // This option controls the default authentication "guard" and password diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 695bfa28d..f1a1a8bd6 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -32,10 +32,9 @@ class LoginController extends Controller */ protected $redirectTo = '/'; protected $redirectPath = '/'; - protected $redirectAfterLogout = '/'; - protected $socialAuthService; - protected $loginService; + protected SocialAuthService $socialAuthService; + protected LoginService $loginService; /** * Create a new controller instance. @@ -50,7 +49,6 @@ class LoginController extends Controller $this->loginService = $loginService; $this->redirectPath = url('/'); - $this->redirectAfterLogout = url(config('auth.auto_redirect') ? '/login?logout=1' : '/'); } public function username() @@ -73,7 +71,7 @@ class LoginController extends Controller { $socialDrivers = $this->socialAuthService->getActiveDrivers(); $authMethod = config('auth.method'); - $autoRedirect = config('auth.auto_redirect'); + $preventInitiation = $request->get('prevent_auto_init') === 'true'; if ($request->has('email')) { session()->flashInput([ @@ -85,8 +83,8 @@ class LoginController extends Controller // Store the previous location for redirect after login $this->updateIntendedFromPrevious(); - if ($autoRedirect && !($request->has('logout') && $request->get('logout') == '1') && count($socialDrivers) == 0 && in_array($authMethod, ['oidc', 'saml2'])) { - return view('auth.login-redirect', [ + if (!$preventInitiation && $this->shouldAutoInitiate()) { + return view('auth.login-initiate', [ 'authMethod' => $authMethod, ]); } @@ -259,6 +257,18 @@ class LoginController extends Controller redirect()->setIntendedUrl($previous); } + /** + * Check if login auto-initiate should be valid based upon authentication config. + */ + protected function shouldAutoInitiate(): bool + { + $socialDrivers = $this->socialAuthService->getActiveDrivers(); + $authMethod = config('auth.method'); + $autoRedirect = config('auth.auto_initiate'); + + return $autoRedirect && count($socialDrivers) === 0 && in_array($authMethod, ['oidc', 'saml2']); + } + /** * Logout user and perform subsequent redirect. * @@ -270,6 +280,8 @@ class LoginController extends Controller { $this->traitLogout($request); - return redirect($this->redirectAfterLogout); + $redirectUri = $this->shouldAutoInitiate() ? '/login?prevent_auto_init=true' : '/'; + + return redirect($redirectUri); } } diff --git a/phpunit.xml b/phpunit.xml index 90320ff41..56a510b10 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -29,6 +29,7 @@ + diff --git a/resources/lang/en/auth.php b/resources/lang/en/auth.php index ad0d516bb..c670106f9 100644 --- a/resources/lang/en/auth.php +++ b/resources/lang/en/auth.php @@ -38,6 +38,11 @@ return [ 'registration_email_domain_invalid' => 'That email domain does not have access to this application', 'register_success' => 'Thanks for signing up! You are now registered and signed in.', + // Login auto-initiation + 'auto_init_starting' => 'Attempting Login', + 'auto_init_starting_desc' => 'We\'re contacting your authentication system to start the login process. If there\'s no progress after 5 seconds you can try clicking the link below.', + 'auto_init_start_link' => 'Proceed with authentication', + // Password Reset 'reset_password' => 'Reset Password', 'reset_password_send_instructions' => 'Enter your email below and you will be sent an email with a password reset link.', diff --git a/resources/sass/_buttons.scss b/resources/sass/_buttons.scss index 850443d9a..714dfc42c 100644 --- a/resources/sass/_buttons.scss +++ b/resources/sass/_buttons.scss @@ -99,6 +99,9 @@ button { fill: var(--color-primary); } } +.text-button.hover-underline:hover { + text-decoration: underline; +} .button.block { width: 100%; diff --git a/resources/views/auth/login-initiate.blade.php b/resources/views/auth/login-initiate.blade.php new file mode 100644 index 000000000..520175f09 --- /dev/null +++ b/resources/views/auth/login-initiate.blade.php @@ -0,0 +1,37 @@ +@extends('layouts.simple') + +@section('content') + +
+ +
 
+ +
+

{{ trans('auth.auto_init_starting') }}

+ +
+ @include('auth.parts.login-form-' . $authMethod) +
+ +
+
+

{{ trans('auth.auto_init_starting_desc') }}

+

+ +

+
+
+ @include('common.loading-icon') +
+
+ + + +
+
+ +@stop diff --git a/resources/views/auth/login-redirect.blade.php b/resources/views/auth/login-redirect.blade.php deleted file mode 100644 index 08501c6b3..000000000 --- a/resources/views/auth/login-redirect.blade.php +++ /dev/null @@ -1,16 +0,0 @@ - - - - - - - - - - - diff --git a/tests/Auth/LoginAutoInitiateTest.php b/tests/Auth/LoginAutoInitiateTest.php new file mode 100644 index 000000000..a2941e21b --- /dev/null +++ b/tests/Auth/LoginAutoInitiateTest.php @@ -0,0 +1,80 @@ +set([ + 'auth.auto_initiate' => true, + 'services.google.client_id' => false, + 'services.github.client_id' => false, + ]); + } + + public function test_with_oidc() + { + config()->set([ + 'auth.method' => 'oidc', + ]); + + $req = $this->get('/login'); + $req->assertSeeText('Attempting Login'); + $req->assertElementExists('form[action$="/oidc/login"][method=POST][id="login-form"] button'); + $req->assertElementExists('button[form="login-form"]'); + } + + public function test_with_saml2() + { + config()->set([ + 'auth.method' => 'saml2', + ]); + + $req = $this->get('/login'); + $req->assertSeeText('Attempting Login'); + $req->assertElementExists('form[action$="/saml2/login"][method=POST][id="login-form"] button'); + $req->assertElementExists('button[form="login-form"]'); + } + + public function test_it_does_not_run_if_social_provider_is_active() + { + config()->set([ + 'auth.method' => 'oidc', + 'services.google.client_id' => 'abc123a', + 'services.google.client_secret' => 'def456', + ]); + + $req = $this->get('/login'); + $req->assertDontSeeText('Attempting Login'); + $req->assertSee('Log In'); + } + + public function test_it_does_not_run_if_prevent_query_string_exists() + { + config()->set([ + 'auth.method' => 'oidc', + ]); + + $req = $this->get('/login?prevent_auto_init=true'); + $req->assertDontSeeText('Attempting Login'); + $req->assertSee('Log In'); + } + + public function test_logout_with_auto_init_leads_to_login_page_with_prevention_query() + { + config()->set([ + 'auth.method' => 'oidc', + ]); + $this->actingAs($this->getEditor()); + + $req = $this->post('/logout'); + $req->assertRedirect('/login?prevent_auto_init=true'); + } + +} \ No newline at end of file diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index 34fd70115..9aebb4d04 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -26,7 +26,6 @@ class OidcTest extends TestCase config()->set([ 'auth.method' => 'oidc', - 'auth.auto_redirect' => false, 'auth.defaults.guard' => 'oidc', 'oidc.name' => 'SingleSignOn-Testing', 'oidc.display_name_claims' => ['name'], @@ -112,19 +111,6 @@ class OidcTest extends TestCase $this->assertPermissionError($resp); } - public function test_automatic_redirect_on_login() - { - config()->set([ - 'auth.auto_redirect' => true, - 'services.google.client_id' => false, - 'services.github.client_id' => false, - ]); - $req = $this->get('/login'); - $req->assertSeeText('SingleSignOn-Testing'); - $req->assertElementExists('form[action$="/oidc/login"][method=POST] button'); - $req->assertElementExists('div#loginredirect-wrapper'); - } - public function test_login() { $req = $this->post('/oidc/login');