From bedbb845eec20363b928b424143787dbe988eefe Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Thu, 13 Aug 2020 11:08:13 +0200 Subject: Move all admin controller into a dedicated group Also handle authentication check in a new middleware for the admin group. --- application/front/ShaarliAdminMiddleware.php | 27 ++++++ application/front/ShaarliMiddleware.php | 12 ++- .../controller/admin/SessionFilterController.php | 13 +-- .../controller/admin/ShaarliAdminController.php | 9 -- .../visitor/PublicSessionFilterController.php | 13 +++ application/legacy/LegacyController.php | 2 +- index.php | 54 +++++------ tests/front/ShaarliAdminMiddlewareTest.php | 100 +++++++++++++++++++++ tests/front/ShaarliMiddlewareTest.php | 2 +- .../admin/SessionFilterControllerTest.php | 51 ----------- .../admin/ShaarliAdminControllerTest.php | 15 ---- .../visitor/PublicSessionFilterControllerTest.php | 51 +++++++++++ tests/legacy/LegacyControllerTest.php | 4 +- tpl/default/linklist.paging.html | 4 +- tpl/default/page.header.html | 4 +- tpl/vintage/linklist.paging.html | 2 +- tpl/vintage/page.header.html | 2 +- 17 files changed, 241 insertions(+), 124 deletions(-) create mode 100644 application/front/ShaarliAdminMiddleware.php create mode 100644 tests/front/ShaarliAdminMiddlewareTest.php diff --git a/application/front/ShaarliAdminMiddleware.php b/application/front/ShaarliAdminMiddleware.php new file mode 100644 index 00000000..35ce4a3b --- /dev/null +++ b/application/front/ShaarliAdminMiddleware.php @@ -0,0 +1,27 @@ +initBasePath($request); + + if (true !== $this->container->loginManager->isLoggedIn()) { + $returnUrl = urlencode($this->container->environment['REQUEST_URI']); + + return $response->withRedirect($this->container->basePath . '/login?returnurl=' . $returnUrl); + } + + return parent::__invoke($request, $response, $next); + } +} diff --git a/application/front/ShaarliMiddleware.php b/application/front/ShaarliMiddleware.php index 707489d0..a2a3837b 100644 --- a/application/front/ShaarliMiddleware.php +++ b/application/front/ShaarliMiddleware.php @@ -40,7 +40,7 @@ class ShaarliMiddleware */ public function __invoke(Request $request, Response $response, callable $next): Response { - $this->container->basePath = rtrim($request->getUri()->getBasePath(), '/'); + $this->initBasePath($request); try { if (!is_file($this->container->conf->getConfigFileExt()) @@ -125,4 +125,14 @@ class ShaarliMiddleware return true; } + + /** + * Initialize the URL base path if it hasn't been defined yet. + */ + protected function initBasePath(Request $request): void + { + if (null === $this->container->basePath) { + $this->container->basePath = rtrim($request->getUri()->getBasePath(), '/'); + } + } } diff --git a/application/front/controller/admin/SessionFilterController.php b/application/front/controller/admin/SessionFilterController.php index 081c0ba0..d9a7a2e0 100644 --- a/application/front/controller/admin/SessionFilterController.php +++ b/application/front/controller/admin/SessionFilterController.php @@ -17,7 +17,7 @@ use Slim\Http\Response; class SessionFilterController extends ShaarliAdminController { /** - * GET /visibility: allows to display only public or only private bookmarks in linklist + * GET /admin/visibility: allows to display only public or only private bookmarks in linklist */ public function visibility(Request $request, Response $response, array $args): Response { @@ -46,16 +46,5 @@ class SessionFilterController extends ShaarliAdminController return $this->redirectFromReferer($request, $response, ['visibility']); } - /** - * GET /untagged-only: allows to display only bookmarks without any tag - */ - public function untaggedOnly(Request $request, Response $response): Response - { - $this->container->sessionManager->setSessionParameter( - SessionManager::KEY_UNTAGGED_ONLY, - empty($this->container->sessionManager->getSessionParameter(SessionManager::KEY_UNTAGGED_ONLY)) - ); - return $this->redirectFromReferer($request, $response, ['untaggedonly', 'untagged-only']); - } } diff --git a/application/front/controller/admin/ShaarliAdminController.php b/application/front/controller/admin/ShaarliAdminController.php index 3bc5bb6b..3b5939bb 100644 --- a/application/front/controller/admin/ShaarliAdminController.php +++ b/application/front/controller/admin/ShaarliAdminController.php @@ -22,15 +22,6 @@ use Slim\Http\Request; */ abstract class ShaarliAdminController extends ShaarliVisitorController { - public function __construct(ShaarliContainer $container) - { - parent::__construct($container); - - if (true !== $this->container->loginManager->isLoggedIn()) { - throw new UnauthorizedException(); - } - } - /** * Any persistent action to the config or data store must check the XSRF token validity. */ diff --git a/application/front/controller/visitor/PublicSessionFilterController.php b/application/front/controller/visitor/PublicSessionFilterController.php index 35da0c5f..1a66362d 100644 --- a/application/front/controller/visitor/PublicSessionFilterController.php +++ b/application/front/controller/visitor/PublicSessionFilterController.php @@ -30,4 +30,17 @@ class PublicSessionFilterController extends ShaarliVisitorController return $this->redirectFromReferer($request, $response, ['linksperpage'], ['nb']); } + + /** + * GET /untagged-only: allows to display only bookmarks without any tag + */ + public function untaggedOnly(Request $request, Response $response): Response + { + $this->container->sessionManager->setSessionParameter( + SessionManager::KEY_UNTAGGED_ONLY, + empty($this->container->sessionManager->getSessionParameter(SessionManager::KEY_UNTAGGED_ONLY)) + ); + + return $this->redirectFromReferer($request, $response, ['untaggedonly', 'untagged-only']); + } } diff --git a/application/legacy/LegacyController.php b/application/legacy/LegacyController.php index a97b07b1..26465d2c 100644 --- a/application/legacy/LegacyController.php +++ b/application/legacy/LegacyController.php @@ -67,7 +67,7 @@ class LegacyController extends ShaarliVisitorController /** Legacy route: ?do=logout */ protected function logout(Request $request, Response $response): Response { - return $this->redirect($response, '/logout'); + return $this->redirect($response, '/admin/logout'); } /** Legacy route: ?do=picwall */ diff --git a/index.php b/index.php index 24c273be..e7471823 100644 --- a/index.php +++ b/index.php @@ -95,39 +95,41 @@ $app->group('', function () { $this->get('/add-tag/{newTag}', '\Shaarli\Front\Controller\Visitor\TagController:addTag'); $this->get('/remove-tag/{tag}', '\Shaarli\Front\Controller\Visitor\TagController:removeTag'); $this->get('/links-per-page', '\Shaarli\Front\Controller\Visitor\PublicSessionFilterController:linksPerPage'); + $this->get('/untagged-only', '\Shaarli\Front\Controller\Admin\PublicSessionFilterController:untaggedOnly'); +})->add('\Shaarli\Front\ShaarliMiddleware'); - /* -- LOGGED IN -- */ +$app->group('/admin', function () { $this->get('/logout', '\Shaarli\Front\Controller\Admin\LogoutController:index'); - $this->get('/admin/tools', '\Shaarli\Front\Controller\Admin\ToolsController:index'); - $this->get('/admin/password', '\Shaarli\Front\Controller\Admin\PasswordController:index'); - $this->post('/admin/password', '\Shaarli\Front\Controller\Admin\PasswordController:change'); - $this->get('/admin/configure', '\Shaarli\Front\Controller\Admin\ConfigureController:index'); - $this->post('/admin/configure', '\Shaarli\Front\Controller\Admin\ConfigureController:save'); - $this->get('/admin/tags', '\Shaarli\Front\Controller\Admin\ManageTagController:index'); - $this->post('/admin/tags', '\Shaarli\Front\Controller\Admin\ManageTagController:save'); - $this->get('/admin/add-shaare', '\Shaarli\Front\Controller\Admin\ManageShaareController:addShaare'); - $this->get('/admin/shaare', '\Shaarli\Front\Controller\Admin\ManageShaareController:displayCreateForm'); - $this->get('/admin/shaare/{id:[0-9]+}', '\Shaarli\Front\Controller\Admin\ManageShaareController:displayEditForm'); - $this->post('/admin/shaare', '\Shaarli\Front\Controller\Admin\ManageShaareController:save'); - $this->get('/admin/shaare/delete', '\Shaarli\Front\Controller\Admin\ManageShaareController:deleteBookmark'); - $this->get('/admin/shaare/visibility', '\Shaarli\Front\Controller\Admin\ManageShaareController:changeVisibility'); - $this->get('/admin/shaare/{id:[0-9]+}/pin', '\Shaarli\Front\Controller\Admin\ManageShaareController:pinBookmark'); + $this->get('/tools', '\Shaarli\Front\Controller\Admin\ToolsController:index'); + $this->get('/password', '\Shaarli\Front\Controller\Admin\PasswordController:index'); + $this->post('/password', '\Shaarli\Front\Controller\Admin\PasswordController:change'); + $this->get('/configure', '\Shaarli\Front\Controller\Admin\ConfigureController:index'); + $this->post('/configure', '\Shaarli\Front\Controller\Admin\ConfigureController:save'); + $this->get('/tags', '\Shaarli\Front\Controller\Admin\ManageTagController:index'); + $this->post('/tags', '\Shaarli\Front\Controller\Admin\ManageTagController:save'); + $this->get('/add-shaare', '\Shaarli\Front\Controller\Admin\ManageShaareController:addShaare'); + $this->get('/shaare', '\Shaarli\Front\Controller\Admin\ManageShaareController:displayCreateForm'); + $this->get('/shaare/{id:[0-9]+}', '\Shaarli\Front\Controller\Admin\ManageShaareController:displayEditForm'); + $this->post('/shaare', '\Shaarli\Front\Controller\Admin\ManageShaareController:save'); + $this->get('/shaare/delete', '\Shaarli\Front\Controller\Admin\ManageShaareController:deleteBookmark'); + $this->get('/shaare/visibility', '\Shaarli\Front\Controller\Admin\ManageShaareController:changeVisibility'); + $this->get('/shaare/{id:[0-9]+}/pin', '\Shaarli\Front\Controller\Admin\ManageShaareController:pinBookmark'); $this->patch( - '/admin/shaare/{id:[0-9]+}/update-thumbnail', + '/shaare/{id:[0-9]+}/update-thumbnail', '\Shaarli\Front\Controller\Admin\ThumbnailsController:ajaxUpdate' ); - $this->get('/admin/export', '\Shaarli\Front\Controller\Admin\ExportController:index'); - $this->post('/admin/export', '\Shaarli\Front\Controller\Admin\ExportController:export'); - $this->get('/admin/import', '\Shaarli\Front\Controller\Admin\ImportController:index'); - $this->post('/admin/import', '\Shaarli\Front\Controller\Admin\ImportController:import'); - $this->get('/admin/plugins', '\Shaarli\Front\Controller\Admin\PluginsController:index'); - $this->post('/admin/plugins', '\Shaarli\Front\Controller\Admin\PluginsController:save'); - $this->get('/admin/token', '\Shaarli\Front\Controller\Admin\TokenController:getToken'); - $this->get('/admin/thumbnails', '\Shaarli\Front\Controller\Admin\ThumbnailsController:index'); + $this->get('/export', '\Shaarli\Front\Controller\Admin\ExportController:index'); + $this->post('/export', '\Shaarli\Front\Controller\Admin\ExportController:export'); + $this->get('/import', '\Shaarli\Front\Controller\Admin\ImportController:index'); + $this->post('/import', '\Shaarli\Front\Controller\Admin\ImportController:import'); + $this->get('/plugins', '\Shaarli\Front\Controller\Admin\PluginsController:index'); + $this->post('/plugins', '\Shaarli\Front\Controller\Admin\PluginsController:save'); + $this->get('/token', '\Shaarli\Front\Controller\Admin\TokenController:getToken'); + $this->get('/thumbnails', '\Shaarli\Front\Controller\Admin\ThumbnailsController:index'); $this->get('/visibility/{visibility}', '\Shaarli\Front\Controller\Admin\SessionFilterController:visibility'); - $this->get('/untagged-only', '\Shaarli\Front\Controller\Admin\SessionFilterController:untaggedOnly'); -})->add('\Shaarli\Front\ShaarliMiddleware'); +})->add('\Shaarli\Front\ShaarliAdminMiddleware'); + // REST API routes $app->group('/api/v1', function () { diff --git a/tests/front/ShaarliAdminMiddlewareTest.php b/tests/front/ShaarliAdminMiddlewareTest.php new file mode 100644 index 00000000..7451330b --- /dev/null +++ b/tests/front/ShaarliAdminMiddlewareTest.php @@ -0,0 +1,100 @@ +container = $this->createMock(ShaarliContainer::class); + + touch(static::TMP_MOCK_FILE); + + $this->container->conf = $this->createMock(ConfigManager::class); + $this->container->conf->method('getConfigFileExt')->willReturn(static::TMP_MOCK_FILE); + + $this->container->loginManager = $this->createMock(LoginManager::class); + $this->container->updater = $this->createMock(Updater::class); + + $this->container->environment = ['REQUEST_URI' => 'http://shaarli/subfolder/path']; + + $this->middleware = new ShaarliAdminMiddleware($this->container); + } + + public function tearDown(): void + { + unlink(static::TMP_MOCK_FILE); + } + + /** + * Try to access an admin controller while logged out -> redirected to login page. + */ + public function testMiddlewareWhileLoggedOut(): void + { + $this->container->loginManager->expects(static::once())->method('isLoggedIn')->willReturn(false); + + $request = $this->createMock(Request::class); + $request->method('getUri')->willReturnCallback(function (): Uri { + $uri = $this->createMock(Uri::class); + $uri->method('getBasePath')->willReturn('/subfolder'); + + return $uri; + }); + + $response = new Response(); + + /** @var Response $result */ + $result = $this->middleware->__invoke($request, $response, function () {}); + + static::assertSame(302, $result->getStatusCode()); + static::assertSame( + '/subfolder/login?returnurl=' . urlencode('http://shaarli/subfolder/path'), + $result->getHeader('location')[0] + ); + } + + /** + * Process controller while logged in. + */ + public function testMiddlewareWhileLoggedIn(): void + { + $this->container->loginManager->method('isLoggedIn')->willReturn(true); + + $request = $this->createMock(Request::class); + $request->method('getUri')->willReturnCallback(function (): Uri { + $uri = $this->createMock(Uri::class); + $uri->method('getBasePath')->willReturn('/subfolder'); + + return $uri; + }); + + $response = new Response(); + $controller = function (Request $request, Response $response): Response { + return $response->withStatus(418); // I'm a tea pot + }; + + /** @var Response $result */ + $result = $this->middleware->__invoke($request, $response, $controller); + + static::assertSame(418, $result->getStatusCode()); + } +} diff --git a/tests/front/ShaarliMiddlewareTest.php b/tests/front/ShaarliMiddlewareTest.php index 09bebd04..d435f506 100644 --- a/tests/front/ShaarliMiddlewareTest.php +++ b/tests/front/ShaarliMiddlewareTest.php @@ -43,7 +43,7 @@ class ShaarliMiddlewareTest extends TestCase $this->middleware = new ShaarliMiddleware($this->container); } - public function tearDown() + public function tearDown(): void { unlink(static::TMP_MOCK_FILE); } diff --git a/tests/front/controller/admin/SessionFilterControllerTest.php b/tests/front/controller/admin/SessionFilterControllerTest.php index 7d5511ed..d306c6e9 100644 --- a/tests/front/controller/admin/SessionFilterControllerTest.php +++ b/tests/front/controller/admin/SessionFilterControllerTest.php @@ -174,55 +174,4 @@ class SessionFilterControllerTest extends TestCase static::assertSame(302, $result->getStatusCode()); static::assertSame(['/subfolder/controller/?searchtag=abc'], $result->getHeader('location')); } - - /** - * Untagged only - valid call - */ - public function testUntaggedOnly(): void - { - $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc']; - - $request = $this->createMock(Request::class); - $response = new Response(); - - $this->container->sessionManager - ->expects(static::once()) - ->method('setSessionParameter') - ->with(SessionManager::KEY_UNTAGGED_ONLY, true) - ; - - $result = $this->controller->untaggedOnly($request, $response); - - static::assertInstanceOf(Response::class, $result); - static::assertSame(302, $result->getStatusCode()); - static::assertSame(['/subfolder/controller/?searchtag=abc'], $result->getHeader('location')); - } - - /** - * Untagged only - toggle off - */ - public function testUntaggedOnlyToggleOff(): void - { - $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc']; - - $request = $this->createMock(Request::class); - $response = new Response(); - - $this->container->sessionManager - ->method('getSessionParameter') - ->with(SessionManager::KEY_UNTAGGED_ONLY) - ->willReturn(true) - ; - $this->container->sessionManager - ->expects(static::once()) - ->method('setSessionParameter') - ->with(SessionManager::KEY_UNTAGGED_ONLY, false) - ; - - $result = $this->controller->untaggedOnly($request, $response); - - static::assertInstanceOf(Response::class, $result); - static::assertSame(302, $result->getStatusCode()); - static::assertSame(['/subfolder/controller/?searchtag=abc'], $result->getHeader('location')); - } } diff --git a/tests/front/controller/admin/ShaarliAdminControllerTest.php b/tests/front/controller/admin/ShaarliAdminControllerTest.php index 7c5f50a6..fff427cb 100644 --- a/tests/front/controller/admin/ShaarliAdminControllerTest.php +++ b/tests/front/controller/admin/ShaarliAdminControllerTest.php @@ -5,9 +5,7 @@ declare(strict_types=1); namespace Shaarli\Front\Controller\Admin; use PHPUnit\Framework\TestCase; -use Shaarli\Front\Exception\UnauthorizedException; use Shaarli\Front\Exception\WrongTokenException; -use Shaarli\Security\LoginManager; use Shaarli\Security\SessionManager; use Slim\Http\Request; @@ -52,19 +50,6 @@ class ShaarliAdminControllerTest extends TestCase }; } - /** - * Creating an instance of an admin controller while logged out should raise an exception. - */ - public function testInstantiateWhileLoggedOut(): void - { - $this->expectException(UnauthorizedException::class); - - $this->container->loginManager = $this->createMock(LoginManager::class); - $this->container->loginManager->method('isLoggedIn')->willReturn(false); - - $this->controller = new class($this->container) extends ShaarliAdminController {}; - } - /** * Trigger controller's checkToken with a valid token. */ diff --git a/tests/front/controller/visitor/PublicSessionFilterControllerTest.php b/tests/front/controller/visitor/PublicSessionFilterControllerTest.php index 3aa1cb99..06352750 100644 --- a/tests/front/controller/visitor/PublicSessionFilterControllerTest.php +++ b/tests/front/controller/visitor/PublicSessionFilterControllerTest.php @@ -68,4 +68,55 @@ class PublicSessionFilterControllerTest extends TestCase static::assertSame(302, $result->getStatusCode()); static::assertSame(['/subfolder/'], $result->getHeader('location')); } + + /** + * Untagged only - valid call + */ + public function testUntaggedOnly(): void + { + $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc']; + + $request = $this->createMock(Request::class); + $response = new Response(); + + $this->container->sessionManager + ->expects(static::once()) + ->method('setSessionParameter') + ->with(SessionManager::KEY_UNTAGGED_ONLY, true) + ; + + $result = $this->controller->untaggedOnly($request, $response); + + static::assertInstanceOf(Response::class, $result); + static::assertSame(302, $result->getStatusCode()); + static::assertSame(['/subfolder/controller/?searchtag=abc'], $result->getHeader('location')); + } + + /** + * Untagged only - toggle off + */ + public function testUntaggedOnlyToggleOff(): void + { + $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc']; + + $request = $this->createMock(Request::class); + $response = new Response(); + + $this->container->sessionManager + ->method('getSessionParameter') + ->with(SessionManager::KEY_UNTAGGED_ONLY) + ->willReturn(true) + ; + $this->container->sessionManager + ->expects(static::once()) + ->method('setSessionParameter') + ->with(SessionManager::KEY_UNTAGGED_ONLY, false) + ; + + $result = $this->controller->untaggedOnly($request, $response); + + static::assertInstanceOf(Response::class, $result); + static::assertSame(302, $result->getStatusCode()); + static::assertSame(['/subfolder/controller/?searchtag=abc'], $result->getHeader('location')); + } } diff --git a/tests/legacy/LegacyControllerTest.php b/tests/legacy/LegacyControllerTest.php index ff4520a3..759a5b2a 100644 --- a/tests/legacy/LegacyControllerTest.php +++ b/tests/legacy/LegacyControllerTest.php @@ -73,8 +73,8 @@ class LegacyControllerTest extends TestCase ['addlink', [], '/login', false], ['login', [], '/login', true], ['login', [], '/login', false], - ['logout', [], '/logout', true], - ['logout', [], '/logout', false], + ['logout', [], '/admin/logout', true], + ['logout', [], '/admin/logout', false], ['picwall', [], '/picture-wall', false], ['picwall', [], '/picture-wall', true], ['tagcloud', [], '/tags/cloud', false], diff --git a/tpl/default/linklist.paging.html b/tpl/default/linklist.paging.html index e1952b79..7b320eaf 100644 --- a/tpl/default/linklist.paging.html +++ b/tpl/default/linklist.paging.html @@ -6,10 +6,10 @@ {'Filters'|t} {if="$is_logged_in"} - - {/if} diff --git a/tpl/default/page.header.html b/tpl/default/page.header.html index aa634a66..a71464c7 100644 --- a/tpl/default/page.header.html +++ b/tpl/default/page.header.html @@ -56,7 +56,7 @@ {if="$is_logged_in"}
  • - {'Logout'|t} + {'Logout'|t}
    • {else}
  • @@ -88,7 +88,7 @@
    • {else}
  • - +
    • diff --git a/tpl/vintage/linklist.paging.html b/tpl/vintage/linklist.paging.html index ea6a5ea2..b9396df6 100644 --- a/tpl/vintage/linklist.paging.html +++ b/tpl/vintage/linklist.paging.html @@ -1,7 +1,7 @@
    {if="$is_logged_in"}
    - + {if="$visibility=='private'"} Click to see all links {else} diff --git a/tpl/vintage/page.header.html b/tpl/vintage/page.header.html index 1c00d19b..0a33523b 100644 --- a/tpl/vintage/page.header.html +++ b/tpl/vintage/page.header.html @@ -18,7 +18,7 @@ {else}
  • Home
    • {if="$is_logged_in"} -
  • Logout
    • +
  • Logout
  • Tools
  • Add link
    • {elseif="$openshaarli"} -- cgit v1.2.3