github-mirror/BookStack
2
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2024-11-22 12:55:14 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

Changed logout routes to POST instead of GET

Browse Source 
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.
This commit is contained in:
Dan Brown 2021-11-14 21:13:24 +00:00
parent fceb4ecc07
commit f910738a80
No known key found for this signature in database
GPG Key ID: 46D9F943C24A2EF9
7 changed files with 26 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/Auth/Access/Saml2Service.php
View File

@ -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.

10
app/Http/Controllers/Auth/Saml2Controller.php
View File

@ -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')]));

12
resources/views/common/header.blade.php
View File

@ -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>

10
routes/web.php
View File

@ -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

4
tests/Auth/AuthTest.php
View File

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

2
tests/Auth/OidcTest.php
View File

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

13
tests/Auth/Saml2Test.php
View File

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