Changed logout routes to POST instead of GET

As per #3047.

Also made some SAML specific fixes:
- IDP initiated login was broken due to forced default session value.
  Double checked against OneLogin lib docs that this reverted logic was fine.
- Changed how the saml login flow works to use 'withoutMiddleware' on
  the route instead of hacking out the session driver. This was due to
  the array driver (previously used for the hack) no longer being
  considered non-persistent.

diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
index ad889faf7c158767f74f8e20fcbbfce8268e0b70..f5d0cd7ccf57ee61fd863c8a85f5f9144d71db75 100644 (file)
--- a/app/Auth/Access/Saml2Service.php
+++ b/app/Auth/Access/Saml2Service.php
@@ -99,7 +99,7 @@ class Saml2Service
      * @throws JsonDebugException
      * @throws UserRegistrationException
      */
-    public function processAcsResponse(string $requestId, string $samlResponse): ?User
+    public function processAcsResponse(?string $requestId, string $samlResponse): ?User
     {
         // The SAML2 toolkit expects the response to be within the $_POST superglobal
         // so we need to manually put it back there at this point.

diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php
index bd3b25da770ab113e04c2298b6d0b4d3f7b6f907..b84483961127fb0bd5ee9bed41957499df4986f8 100644 (file)
--- a/app/Http/Controllers/Auth/Saml2Controller.php
+++ b/app/Http/Controllers/Auth/Saml2Controller.php
@@ -5,8 +5,7 @@ namespace BookStack\Http\Controllers\Auth;
 use BookStack\Auth\Access\Saml2Service;
 use BookStack\Http\Controllers\Controller;
 use Illuminate\Http\Request;
-use Illuminate\Support\Facades\Cache;
-use Str;
+use Illuminate\Support\Str;
 
 class Saml2Controller extends Controller
 {
@@ -79,11 +78,6 @@ class Saml2Controller extends Controller
      */
     public function startAcs(Request $request)
     {
-        // Note: This is a bit of a hack to prevent a session being stored
-        // on the response of this request. Within Laravel7+ this could instead
-        // be done via removing the StartSession middleware from the route.
-        config()->set('session.driver', 'array');
-
         $samlResponse = $request->get('SAMLResponse', null);
 
         if (empty($samlResponse)) {
@@ -114,7 +108,7 @@ class Saml2Controller extends Controller
             $samlResponse = decrypt(cache()->pull($cacheKey));
         } catch (\Exception $exception) {
         }
-        $requestId = session()->pull('saml2_request_id', 'unset');
+        $requestId = session()->pull('saml2_request_id', null);
 
         if (empty($acsId) || empty($samlResponse)) {
             $this->showErrorNotification(trans('errors.saml_fail_authed', ['system' => config('saml2.name')]));

diff --git a/resources/views/common/header.blade.php b/resources/views/common/header.blade.php
index 2311ce3e019e9f529ef2fbde6569a518db4f11c6..d55f3ae2dacd179a7749d362035a9ed0f2498922 100644 (file)
--- a/resources/views/common/header.blade.php
+++ b/resources/views/common/header.blade.php
@@ -71,11 +71,13 @@
                                 <a href="{{ $currentUser->getEditUrl() }}">@icon('edit'){{ trans('common.edit_profile') }}</a>
                             </li>
                             <li>
-                                @if(config('auth.method') === 'saml2')
-                                    <a href="{{ url('/saml2/logout') }}">@icon('logout'){{ trans('auth.logout') }}</a>
-                                @else
-                                    <a href="{{ url('/logout') }}">@icon('logout'){{ trans('auth.logout') }}</a>
-                                @endif
+                                <form action="{{ url(config('auth.method') === 'saml2' ? '/saml2/logout' : '/logout') }}"
+                                      method="post">
+                                    {{ csrf_field() }}
+                                    <button class="text-muted icon-list-item text-primary">
+                                        @icon('logout'){{ trans('auth.logout') }}
+                                    </button>
+                                </form>
                             </li>
                             <li><hr></li>
                             <li>

diff --git a/routes/web.php b/routes/web.php
index 653b5c227116e286d66af3b2f5e88280ef43fe39..c924ed68c9ee93a2a3ef05665901b14d0aaed692 100644 (file)
--- a/routes/web.php
+++ b/routes/web.php
@@ -277,7 +277,7 @@ Route::get('/register/service/{socialDriver}', [Auth\SocialController::class, 'r
 // Login/Logout routes
 Route::get('/login', [Auth\LoginController::class, 'getLogin']);
 Route::post('/login', [Auth\LoginController::class, 'login']);
-Route::get('/logout', [Auth\LoginController::class, 'logout']);
+Route::post('/logout', [Auth\LoginController::class, 'logout']);
 Route::get('/register', [Auth\RegisterController::class, 'getRegister']);
 Route::get('/register/confirm', [Auth\ConfirmEmailController::class, 'show']);
 Route::get('/register/confirm/awaiting', [Auth\ConfirmEmailController::class, 'showAwaiting']);
@@ -287,10 +287,14 @@ Route::post('/register', [Auth\RegisterController::class, 'postRegister']);
 
 // SAML routes
 Route::post('/saml2/login', [Auth\Saml2Controller::class, 'login']);
-Route::get('/saml2/logout', [Auth\Saml2Controller::class, 'logout']);
+Route::post('/saml2/logout', [Auth\Saml2Controller::class, 'logout']);
 Route::get('/saml2/metadata', [Auth\Saml2Controller::class, 'metadata']);
 Route::get('/saml2/sls', [Auth\Saml2Controller::class, 'sls']);
-Route::post('/saml2/acs', [Auth\Saml2Controller::class, 'startAcs']);
+Route::post('/saml2/acs', [Auth\Saml2Controller::class, 'startAcs'])->withoutMiddleware([
+    \Illuminate\Session\Middleware\StartSession::class,
+    \Illuminate\View\Middleware\ShareErrorsFromSession::class,
+    \BookStack\Http\Middleware\VerifyCsrfToken::class,
+]);
 Route::get('/saml2/acs', [Auth\Saml2Controller::class, 'processAcs']);
 
 // OIDC routes

diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php
index 66ab09d3c62dc02ebaec724c57c532f246b89a61..50f56bfb9c6d8d49926d29b1737aa0e4d8866390 100644 (file)
--- a/tests/Auth/AuthTest.php
+++ b/tests/Auth/AuthTest.php
@@ -192,7 +192,7 @@ class AuthTest extends TestCase
     public function test_logout()
     {
         $this->asAdmin()->get('/')->assertOk();
-        $this->get('/logout')->assertRedirect('/');
+        $this->post('/logout')->assertRedirect('/');
         $this->get('/')->assertRedirect('/login');
     }
 
@@ -204,7 +204,7 @@ class AuthTest extends TestCase
         $mfaSession->markVerifiedForUser($user);
         $this->assertTrue($mfaSession->isVerifiedForUser($user));
 
-        $this->asAdmin()->get('/logout');
+        $this->asAdmin()->post('/logout');
         $this->assertFalse($mfaSession->isVerifiedForUser($user));
     }
 

diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index e7665a679a44a892bbe743863aa01579c3e780c0..0b033ea812593bf462e09891624ada9b882696f3 100644 (file)
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -90,7 +90,7 @@ class OidcTest extends TestCase
     public function test_logout_route_functions()
     {
         $this->actingAs($this->getEditor());
-        $this->get('/logout');
+        $this->post('/logout');
         $this->assertFalse(auth()->check());
     }
 

diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php
index aac2710a889a4aa891d7df8fc9312f1d32087885..cb217585c5fc18f7018aa9a83a0bbdf4b984a361 100644 (file)
--- a/tests/Auth/Saml2Test.php
+++ b/tests/Auth/Saml2Test.php
@@ -157,8 +157,7 @@ class Saml2Test extends TestCase
         ]);
 
         $resp = $this->actingAs($this->getEditor())->get('/');
-        $resp->assertElementExists('a[href$="/saml2/logout"]');
-        $resp->assertElementContains('a[href$="/saml2/logout"]', 'Logout');
+        $resp->assertElementContains('form[action$="/saml2/logout"] button', 'Logout');
     }
 
     public function test_logout_sls_flow()
@@ -177,7 +176,7 @@ class Saml2Test extends TestCase
 
         $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
 
-        $req = $this->get('/saml2/logout');
+        $req = $this->post('/saml2/logout');
         $redirect = $req->headers->get('location');
         $this->assertStringStartsWith('http://saml.local/saml2/idp/SingleLogoutService.php', $redirect);
         $this->withGet(['SAMLResponse' => $this->sloResponseData], $handleLogoutResponse);
@@ -193,7 +192,7 @@ class Saml2Test extends TestCase
         $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
         $this->assertTrue($this->isAuthenticated());
 
-        $req = $this->get('/saml2/logout');
+        $req = $this->post('/saml2/logout');
         $req->assertRedirect('/');
         $this->assertFalse($this->isAuthenticated());
     }
@@ -216,13 +215,13 @@ class Saml2Test extends TestCase
     public function test_saml_routes_are_only_active_if_saml_enabled()
     {
         config()->set(['auth.method' => 'standard']);
-        $getRoutes = ['/logout', '/metadata', '/sls'];
+        $getRoutes = ['/metadata', '/sls'];
         foreach ($getRoutes as $route) {
             $req = $this->get('/saml2' . $route);
             $this->assertPermissionError($req);
         }
 
-        $postRoutes = ['/login', '/acs'];
+        $postRoutes = ['/login', '/acs', '/logout'];
         foreach ($postRoutes as $route) {
             $req = $this->post('/saml2' . $route);
             $this->assertPermissionError($req);
@@ -249,7 +248,7 @@ class Saml2Test extends TestCase
         $resp = $this->post('/login');
         $this->assertPermissionError($resp);
 
-        $resp = $this->get('/logout');
+        $resp = $this->post('/logout');
         $this->assertPermissionError($resp);
     }