From a8c11451e8d885a243c1ad52012093ba8d121e2c Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Tue, 21 Jul 2020 20:33:33 +0200 Subject: [PATCH] Process login through Slim controller --- application/front/ShaarliMiddleware.php | 4 +- .../controller/visitor/LoginController.php | 127 +++++++- .../front/exceptions/CantLoginException.php | 10 + application/security/SessionManager.php | 30 ++ index.php | 84 +----- tests/front/ShaarliMiddlewareTest.php | 7 +- .../visitor/FrontControllerMockHelper.php | 1 + .../visitor/LoginControllerTest.php | 278 +++++++++++++++++- 8 files changed, 442 insertions(+), 99 deletions(-) create mode 100644 application/front/exceptions/CantLoginException.php diff --git a/application/front/ShaarliMiddleware.php b/application/front/ShaarliMiddleware.php index 595182ac..e9f5552d 100644 --- a/application/front/ShaarliMiddleware.php +++ b/application/front/ShaarliMiddleware.php @@ -62,7 +62,9 @@ class ShaarliMiddleware return $response->write($this->container->pageBuilder->render('error')); } catch (UnauthorizedException $e) { - return $response->withRedirect($this->container->basePath . '/login'); + $returnUrl = urlencode($this->container->environment['REQUEST_URI']); + + return $response->withRedirect($this->container->basePath . '/login?returnurl=' . $returnUrl); } catch (\Throwable $e) { // Unknown error encountered $this->container->pageBuilder->reset(); diff --git a/application/front/controller/visitor/LoginController.php b/application/front/controller/visitor/LoginController.php index a257766f..c40b8cc4 100644 --- a/application/front/controller/visitor/LoginController.php +++ b/application/front/controller/visitor/LoginController.php @@ -4,8 +4,12 @@ declare(strict_types=1); namespace Shaarli\Front\Controller\Visitor; +use Shaarli\Front\Exception\CantLoginException; use Shaarli\Front\Exception\LoginBannedException; +use Shaarli\Front\Exception\WrongTokenException; use Shaarli\Render\TemplatePage; +use Shaarli\Security\CookieManager; +use Shaarli\Security\SessionManager; use Slim\Http\Request; use Slim\Http\Response; @@ -19,29 +23,132 @@ use Slim\Http\Response; */ class LoginController extends ShaarliVisitorController { + /** + * GET /login - Display the login page. + */ public function index(Request $request, Response $response): Response { - if ($this->container->loginManager->isLoggedIn() - || $this->container->conf->get('security.open_shaarli', false) - ) { + try { + $this->checkLoginState(); + } catch (CantLoginException $e) { return $this->redirect($response, '/'); } - $userCanLogin = $this->container->loginManager->canLogin($request->getServerParams()); - if ($userCanLogin !== true) { - throw new LoginBannedException(); + if ($request->getParam('login') !== null) { + $this->assignView('username', escape($request->getParam('login'))); } - if ($request->getParam('username') !== null) { - $this->assignView('username', escape($request->getParam('username'))); - } + $returnUrl = $request->getParam('returnurl') ?? $this->container->environment['HTTP_REFERER'] ?? null; $this - ->assignView('returnurl', escape($request->getServerParam('HTTP_REFERER'))) + ->assignView('returnurl', escape($returnUrl)) ->assignView('remember_user_default', $this->container->conf->get('privacy.remember_user_default', true)) ->assignView('pagetitle', t('Login') .' - '. $this->container->conf->get('general.title', 'Shaarli')) ; return $response->write($this->render(TemplatePage::LOGIN)); } + + /** + * POST /login - Process login + */ + public function login(Request $request, Response $response): Response + { + if (!$this->container->sessionManager->checkToken($request->getParam('token'))) { + throw new WrongTokenException(); + } + + try { + $this->checkLoginState(); + } catch (CantLoginException $e) { + return $this->redirect($response, '/'); + } + + if (!$this->container->loginManager->checkCredentials( + $this->container->environment['REMOTE_ADDR'], + client_ip_id($this->container->environment), + $request->getParam('login'), + $request->getParam('password') + ) + ) { + $this->container->loginManager->handleFailedLogin($this->container->environment); + + $this->container->sessionManager->setSessionParameter( + SessionManager::KEY_ERROR_MESSAGES, + [t('Wrong login/password.')] + ); + + // Call controller directly instead of unnecessary redirection + return $this->index($request, $response); + } + + $this->container->loginManager->handleSuccessfulLogin($this->container->environment); + + $cookiePath = $this->container->basePath . '/'; + $expirationTime = $this->saveLongLastingSession($request, $cookiePath); + $this->renewUserSession($cookiePath, $expirationTime); + + // Force referer from given return URL + $this->container->environment['HTTP_REFERER'] = $request->getParam('returnurl'); + + return $this->redirectFromReferer($request, $response, ['login']); + } + + /** + * Make sure that the user is allowed to login and/or displaying the login page: + * - not already logged in + * - not open shaarli + * - not banned + */ + protected function checkLoginState(): bool + { + if ($this->container->loginManager->isLoggedIn() + || $this->container->conf->get('security.open_shaarli', false) + ) { + throw new CantLoginException(); + } + + if (true !== $this->container->loginManager->canLogin($this->container->environment)) { + throw new LoginBannedException(); + } + + return true; + } + + /** + * @return int Session duration in seconds + */ + protected function saveLongLastingSession(Request $request, string $cookiePath): int + { + if (empty($request->getParam('longlastingsession'))) { + // Standard session expiration (=when browser closes) + $expirationTime = 0; + } else { + // Keep the session cookie even after the browser closes + $this->container->sessionManager->setStaySignedIn(true); + $expirationTime = $this->container->sessionManager->extendSession(); + } + + $this->container->cookieManager->setCookieParameter( + CookieManager::STAY_SIGNED_IN, + $this->container->loginManager->getStaySignedInToken(), + $expirationTime, + $cookiePath + ); + + return $expirationTime; + } + + protected function renewUserSession(string $cookiePath, int $expirationTime): void + { + // Send cookie with the new expiration date to the browser + $this->container->sessionManager->destroy(); + $this->container->sessionManager->cookieParameters( + $expirationTime, + $cookiePath, + $this->container->environment['SERVER_NAME'] + ); + $this->container->sessionManager->start(); + $this->container->sessionManager->regenerateId(true); + } } diff --git a/application/front/exceptions/CantLoginException.php b/application/front/exceptions/CantLoginException.php new file mode 100644 index 00000000..cd16635d --- /dev/null +++ b/application/front/exceptions/CantLoginException.php @@ -0,0 +1,10 @@ +savePath; } + + /* + * Next public functions wrapping native PHP session API. + */ + + public function destroy(): bool + { + $this->session = []; + + return session_destroy(); + } + + public function start(): bool + { + if (session_status() === PHP_SESSION_ACTIVE) { + $this->destroy(); + } + + return session_start(); + } + + public function cookieParameters(int $lifeTime, string $path, string $domain): bool + { + return session_set_cookie_params($lifeTime, $path, $domain); + } + + public function regenerateId(bool $deleteOldSession = false): bool + { + return session_regenerate_id($deleteOldSession); + } } diff --git a/index.php b/index.php index 4627438e..1a121f37 100644 --- a/index.php +++ b/index.php @@ -159,89 +159,6 @@ header("Pragma: no-cache"); $loginManager->checkLoginState($clientIpId); -// ------------------------------------------------------------------------------------------ -// Process login form: Check if login/password is correct. -if (isset($_POST['login'])) { - if (! $loginManager->canLogin($_SERVER)) { - die(t('I said: NO. You are banned for the moment. Go away.')); - } - if (isset($_POST['password']) - && $sessionManager->checkToken($_POST['token']) - && $loginManager->checkCredentials($_SERVER['REMOTE_ADDR'], $clientIpId, $_POST['login'], $_POST['password']) - ) { - $loginManager->handleSuccessfulLogin($_SERVER); - - $cookiedir = ''; - if (dirname($_SERVER['SCRIPT_NAME']) != '/') { - // Note: Never forget the trailing slash on the cookie path! - $cookiedir = dirname($_SERVER["SCRIPT_NAME"]) . '/'; - } - - if (!empty($_POST['longlastingsession'])) { - // Keep the session cookie even after the browser closes - $sessionManager->setStaySignedIn(true); - $expirationTime = $sessionManager->extendSession(); - - setcookie( - CookieManager::STAY_SIGNED_IN, - $loginManager->getStaySignedInToken(), - $expirationTime, - WEB_PATH - ); - } else { - // Standard session expiration (=when browser closes) - $expirationTime = 0; - } - - // Send cookie with the new expiration date to the browser - session_destroy(); - session_set_cookie_params($expirationTime, $cookiedir, $_SERVER['SERVER_NAME']); - session_start(); - session_regenerate_id(true); - - // Optional redirect after login: - if (isset($_GET['post'])) { - $uri = './?post='. urlencode($_GET['post']); - foreach (array('description', 'source', 'title', 'tags') as $param) { - if (!empty($_GET[$param])) { - $uri .= '&'.$param.'='.urlencode($_GET[$param]); - } - } - header('Location: '. $uri); - exit; - } - - if (isset($_GET['edit_link'])) { - header('Location: ./?edit_link='. escape($_GET['edit_link'])); - exit; - } - - if (isset($_POST['returnurl'])) { - // Prevent loops over login screen. - if (strpos($_POST['returnurl'], '/login') === false) { - header('Location: '. generateLocation($_POST['returnurl'], $_SERVER['HTTP_HOST'])); - exit; - } - } - header('Location: ./?'); - exit; - } else { - $loginManager->handleFailedLogin($_SERVER); - $redir = '?username='. urlencode($_POST['login']); - if (isset($_GET['post'])) { - $redir .= '&post=' . urlencode($_GET['post']); - foreach (array('description', 'source', 'title', 'tags') as $param) { - if (!empty($_GET[$param])) { - $redir .= '&' . $param . '=' . urlencode($_GET[$param]); - } - } - } - // Redirect to login screen. - echo ''; - exit; - } -} - // ------------------------------------------------------------------------------------------ // Token management for XSRF protection // Token should be used in any form which acts on data (create,update,delete,import...). @@ -283,6 +200,7 @@ $app->group('', function () { $this->get('/', '\Shaarli\Front\Controller\Visitor\BookmarkListController:index'); $this->get('/shaare/{hash}', '\Shaarli\Front\Controller\Visitor\BookmarkListController:permalink'); $this->get('/login', '\Shaarli\Front\Controller\Visitor\LoginController:index')->setName('login'); + $this->post('/login', '\Shaarli\Front\Controller\Visitor\LoginController:login')->setName('processLogin'); $this->get('/picture-wall', '\Shaarli\Front\Controller\Visitor\PictureWallController:index'); $this->get('/tags/cloud', '\Shaarli\Front\Controller\Visitor\TagCloudController:cloud'); $this->get('/tags/list', '\Shaarli\Front\Controller\Visitor\TagCloudController:list'); diff --git a/tests/front/ShaarliMiddlewareTest.php b/tests/front/ShaarliMiddlewareTest.php index 20090d8b..09bebd04 100644 --- a/tests/front/ShaarliMiddlewareTest.php +++ b/tests/front/ShaarliMiddlewareTest.php @@ -38,6 +38,8 @@ class ShaarliMiddlewareTest extends TestCase $this->container->loginManager = $this->createMock(LoginManager::class); + $this->container->environment = ['REQUEST_URI' => 'http://shaarli/subfolder/path']; + $this->middleware = new ShaarliMiddleware($this->container); } @@ -127,7 +129,10 @@ class ShaarliMiddlewareTest extends TestCase $result = $this->middleware->__invoke($request, $response, $controller); static::assertSame(302, $result->getStatusCode()); - static::assertSame('/subfolder/login', $result->getHeader('location')[0]); + static::assertSame( + '/subfolder/login?returnurl=' . urlencode('http://shaarli/subfolder/path'), + $result->getHeader('location')[0] + ); } /** diff --git a/tests/front/controller/visitor/FrontControllerMockHelper.php b/tests/front/controller/visitor/FrontControllerMockHelper.php index 7f560662..e0bd4ecf 100644 --- a/tests/front/controller/visitor/FrontControllerMockHelper.php +++ b/tests/front/controller/visitor/FrontControllerMockHelper.php @@ -80,6 +80,7 @@ trait FrontControllerMockHelper 'SERVER_NAME' => 'shaarli', 'SERVER_PORT' => '80', 'REQUEST_URI' => '/daily-rss', + 'REMOTE_ADDR' => '1.2.3.4', ]; $this->container->basePath = '/subfolder'; diff --git a/tests/front/controller/visitor/LoginControllerTest.php b/tests/front/controller/visitor/LoginControllerTest.php index e57f44b9..0a21f938 100644 --- a/tests/front/controller/visitor/LoginControllerTest.php +++ b/tests/front/controller/visitor/LoginControllerTest.php @@ -7,6 +7,10 @@ namespace Shaarli\Front\Controller\Visitor; use PHPUnit\Framework\TestCase; use Shaarli\Config\ConfigManager; use Shaarli\Front\Exception\LoginBannedException; +use Shaarli\Front\Exception\WrongTokenException; +use Shaarli\Render\TemplatePage; +use Shaarli\Security\CookieManager; +use Shaarli\Security\SessionManager; use Slim\Http\Request; use Slim\Http\Response; @@ -21,13 +25,25 @@ class LoginControllerTest extends TestCase { $this->createContainer(); + $this->container->cookieManager = $this->createMock(CookieManager::class); + $this->container->sessionManager->method('checkToken')->willReturn(true); + $this->controller = new LoginController($this->container); } + /** + * Test displaying login form with valid parameters. + */ public function testValidControllerInvoke(): void { $request = $this->createMock(Request::class); - $request->expects(static::once())->method('getServerParam')->willReturn('> referer'); + $request + ->expects(static::atLeastOnce()) + ->method('getParam') + ->willReturnCallback(function (string $key) { + return 'returnurl' === $key ? '> referer' : null; + }) + ; $response = new Response(); $assignedVariables = []; @@ -46,18 +62,32 @@ class LoginControllerTest extends TestCase static::assertInstanceOf(Response::class, $result); static::assertSame(200, $result->getStatusCode()); - static::assertSame('loginform', (string) $result->getBody()); + static::assertSame(TemplatePage::LOGIN, (string) $result->getBody()); static::assertSame('> referer', $assignedVariables['returnurl']); static::assertSame(true, $assignedVariables['remember_user_default']); static::assertSame('Login - Shaarli', $assignedVariables['pagetitle']); } + /** + * Test displaying login form with username defined in the request. + */ public function testValidControllerInvokeWithUserName(): void { + $this->container->environment = ['HTTP_REFERER' => '> referer']; + $request = $this->createMock(Request::class); - $request->expects(static::once())->method('getServerParam')->willReturn('> referer'); - $request->expects(static::exactly(2))->method('getParam')->willReturn('myUser>'); + $request + ->expects(static::atLeastOnce()) + ->method('getParam') + ->willReturnCallback(function (string $key, $default) { + if ('login' === $key) { + return 'myUser>'; + } + + return $default; + }) + ; $response = new Response(); $assignedVariables = []; @@ -84,6 +114,9 @@ class LoginControllerTest extends TestCase static::assertSame('Login - Shaarli', $assignedVariables['pagetitle']); } + /** + * Test displaying login page while being logged in. + */ public function testLoginControllerWhileLoggedIn(): void { $request = $this->createMock(Request::class); @@ -98,6 +131,9 @@ class LoginControllerTest extends TestCase static::assertSame(['/subfolder/'], $result->getHeader('Location')); } + /** + * Test displaying login page with open shaarli configured: redirect to homepage. + */ public function testLoginControllerOpenShaarli(): void { $request = $this->createMock(Request::class); @@ -119,6 +155,9 @@ class LoginControllerTest extends TestCase static::assertSame(['/subfolder/'], $result->getHeader('Location')); } + /** + * Test displaying login page while being banned. + */ public function testLoginControllerWhileBanned(): void { $request = $this->createMock(Request::class); @@ -131,4 +170,235 @@ class LoginControllerTest extends TestCase $this->controller->index($request, $response); } + + /** + * Test processing login with valid parameters. + */ + public function testProcessLoginWithValidParameters(): void + { + $parameters = [ + 'login' => 'bob', + 'password' => 'pass', + ]; + $request = $this->createMock(Request::class); + $request + ->expects(static::atLeastOnce()) + ->method('getParam') + ->willReturnCallback(function (string $key) use ($parameters) { + return $parameters[$key] ?? null; + }) + ; + $response = new Response(); + + $this->container->loginManager->method('canLogin')->willReturn(true); + $this->container->loginManager->expects(static::once())->method('handleSuccessfulLogin'); + $this->container->loginManager + ->expects(static::once()) + ->method('checkCredentials') + ->with('1.2.3.4', '1.2.3.4', 'bob', 'pass') + ->willReturn(true) + ; + $this->container->loginManager->method('getStaySignedInToken')->willReturn(bin2hex(random_bytes(8))); + + $this->container->sessionManager->expects(static::never())->method('extendSession'); + $this->container->sessionManager->expects(static::once())->method('destroy'); + $this->container->sessionManager + ->expects(static::once()) + ->method('cookieParameters') + ->with(0, '/subfolder/', 'shaarli') + ; + $this->container->sessionManager->expects(static::once())->method('start'); + $this->container->sessionManager->expects(static::once())->method('regenerateId')->with(true); + + $result = $this->controller->login($request, $response); + + static::assertSame(302, $result->getStatusCode()); + static::assertSame('/subfolder/', $result->getHeader('location')[0]); + } + + /** + * Test processing login with return URL. + */ + public function testProcessLoginWithReturnUrl(): void + { + $parameters = [ + 'returnurl' => 'http://shaarli/subfolder/admin/shaare', + ]; + $request = $this->createMock(Request::class); + $request + ->expects(static::atLeastOnce()) + ->method('getParam') + ->willReturnCallback(function (string $key) use ($parameters) { + return $parameters[$key] ?? null; + }) + ; + $response = new Response(); + + $this->container->loginManager->method('canLogin')->willReturn(true); + $this->container->loginManager->expects(static::once())->method('handleSuccessfulLogin'); + $this->container->loginManager->expects(static::once())->method('checkCredentials')->willReturn(true); + $this->container->loginManager->method('getStaySignedInToken')->willReturn(bin2hex(random_bytes(8))); + + $result = $this->controller->login($request, $response); + + static::assertSame(302, $result->getStatusCode()); + static::assertSame('/subfolder/admin/shaare', $result->getHeader('location')[0]); + } + + /** + * Test processing login with remember me session enabled. + */ + public function testProcessLoginLongLastingSession(): void + { + $parameters = [ + 'longlastingsession' => true, + ]; + $request = $this->createMock(Request::class); + $request + ->expects(static::atLeastOnce()) + ->method('getParam') + ->willReturnCallback(function (string $key) use ($parameters) { + return $parameters[$key] ?? null; + }) + ; + $response = new Response(); + + $this->container->loginManager->method('canLogin')->willReturn(true); + $this->container->loginManager->expects(static::once())->method('handleSuccessfulLogin'); + $this->container->loginManager->expects(static::once())->method('checkCredentials')->willReturn(true); + $this->container->loginManager->method('getStaySignedInToken')->willReturn(bin2hex(random_bytes(8))); + + $this->container->sessionManager->expects(static::once())->method('destroy'); + $this->container->sessionManager + ->expects(static::once()) + ->method('cookieParameters') + ->with(42, '/subfolder/', 'shaarli') + ; + $this->container->sessionManager->expects(static::once())->method('start'); + $this->container->sessionManager->expects(static::once())->method('regenerateId')->with(true); + $this->container->sessionManager->expects(static::once())->method('extendSession')->willReturn(42); + + $this->container->cookieManager = $this->createMock(CookieManager::class); + $this->container->cookieManager + ->expects(static::once()) + ->method('setCookieParameter') + ->willReturnCallback(function (string $name): CookieManager { + static::assertSame(CookieManager::STAY_SIGNED_IN, $name); + + return $this->container->cookieManager; + }) + ; + + $result = $this->controller->login($request, $response); + + static::assertSame(302, $result->getStatusCode()); + static::assertSame('/subfolder/', $result->getHeader('location')[0]); + } + + /** + * Test processing login with invalid credentials + */ + public function testProcessLoginWrongCredentials(): void + { + $parameters = [ + 'returnurl' => 'http://shaarli/subfolder/admin/shaare', + ]; + $request = $this->createMock(Request::class); + $request + ->expects(static::atLeastOnce()) + ->method('getParam') + ->willReturnCallback(function (string $key) use ($parameters) { + return $parameters[$key] ?? null; + }) + ; + $response = new Response(); + + $this->container->loginManager->method('canLogin')->willReturn(true); + $this->container->loginManager->expects(static::once())->method('handleFailedLogin'); + $this->container->loginManager->expects(static::once())->method('checkCredentials')->willReturn(false); + + $this->container->sessionManager + ->expects(static::once()) + ->method('setSessionParameter') + ->with(SessionManager::KEY_ERROR_MESSAGES, ['Wrong login/password.']) + ; + + $result = $this->controller->login($request, $response); + + static::assertSame(200, $result->getStatusCode()); + static::assertSame(TemplatePage::LOGIN, (string) $result->getBody()); + } + + /** + * Test processing login with wrong token + */ + public function testProcessLoginWrongToken(): void + { + $request = $this->createMock(Request::class); + $response = new Response(); + + $this->container->sessionManager = $this->createMock(SessionManager::class); + $this->container->sessionManager->method('checkToken')->willReturn(false); + + $this->expectException(WrongTokenException::class); + + $this->controller->login($request, $response); + } + + /** + * Test processing login with wrong token + */ + public function testProcessLoginAlreadyLoggedIn(): void + { + $request = $this->createMock(Request::class); + $response = new Response(); + + $this->container->loginManager->method('isLoggedIn')->willReturn(true); + $this->container->loginManager->expects(static::never())->method('handleSuccessfulLogin'); + $this->container->loginManager->expects(static::never())->method('handleFailedLogin'); + + $result = $this->controller->login($request, $response); + + static::assertSame(302, $result->getStatusCode()); + static::assertSame('/subfolder/', $result->getHeader('location')[0]); + } + + /** + * Test processing login with wrong token + */ + public function testProcessLoginInOpenShaarli(): void + { + $request = $this->createMock(Request::class); + $response = new Response(); + + $this->container->conf = $this->createMock(ConfigManager::class); + $this->container->conf->method('get')->willReturnCallback(function (string $key, $value) { + return 'security.open_shaarli' === $key ? true : $value; + }); + + $this->container->loginManager->expects(static::never())->method('handleSuccessfulLogin'); + $this->container->loginManager->expects(static::never())->method('handleFailedLogin'); + + $result = $this->controller->login($request, $response); + + static::assertSame(302, $result->getStatusCode()); + static::assertSame('/subfolder/', $result->getHeader('location')[0]); + } + + /** + * Test processing login while being banned + */ + public function testProcessLoginWhileBanned(): void + { + $request = $this->createMock(Request::class); + $response = new Response(); + + $this->container->loginManager->method('canLogin')->willReturn(false); + $this->container->loginManager->expects(static::never())->method('handleSuccessfulLogin'); + $this->container->loginManager->expects(static::never())->method('handleFailedLogin'); + + $this->expectException(LoginBannedException::class); + + $this->controller->login($request, $response); + } } -- 2.41.0