From 1712110623b655ded9ecf6e46e246865bea92757 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Wed, 10 Jul 2024 22:01:33 +0200 Subject: [PATCH 1/5] helpers\View: Determine base URL lazily So that it does not try to access `$_SERVER['SERVER_NAME']` in CLI SAPI. Though this is not sufficient, since the Authentication helper will try to access it strictly too. --- src/controllers/Rss.php | 4 ++-- src/helpers/View.php | 11 +++++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/controllers/Rss.php b/src/controllers/Rss.php index f43a599705..50228a5204 100644 --- a/src/controllers/Rss.php +++ b/src/controllers/Rss.php @@ -42,9 +42,9 @@ public function rss(): void { $this->feedWriter->setTitle($this->configuration->rssTitle); $this->feedWriter->setChannelElement('description', ''); - $this->feedWriter->setSelfLink($this->view->base . 'feed'); + $this->feedWriter->setSelfLink($this->view->getBaseUrl() . 'feed'); - $this->feedWriter->setLink($this->view->base); + $this->feedWriter->setLink($this->view->getBaseUrl()); // get sources $lastSourceId = 0; diff --git a/src/helpers/View.php b/src/helpers/View.php index 5c4c5ba3bd..8e1a89000b 100644 --- a/src/helpers/View.php +++ b/src/helpers/View.php @@ -21,7 +21,7 @@ */ class View { /** Current base url */ - public string $base = ''; + private ?string $baseUrl = null; private Configuration $configuration; @@ -30,7 +30,6 @@ class View { */ public function __construct(Configuration $configuration) { $this->configuration = $configuration; - $this->base = $this->getBaseUrl(); } /** @@ -39,6 +38,14 @@ public function __construct(Configuration $configuration) { * globale server variables ($_SERVER). */ public function getBaseUrl(): string { + if ($this->baseUrl === null) { + $this->baseUrl = $this->makeBaseUrl(); + } + + return $this->baseUrl; + } + + private function makeBaseUrl(): string { // base url in config.ini file $base = $this->configuration->baseUrl; if ($base !== '') { From 5f9b4979ff4823e924b35cc57fa7e1da5fd1ffbf Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Thu, 11 Jul 2024 02:30:41 +0200 Subject: [PATCH 2/5] helpers\Session: Start session lazily So that we do not need to care about it in the Authentication rewrite later. --- src/helpers/Authentication.php | 1 - src/helpers/Session.php | 10 ++++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/helpers/Authentication.php b/src/helpers/Authentication.php index 4d18d0cf59..1fb7ed20f6 100644 --- a/src/helpers/Authentication.php +++ b/src/helpers/Authentication.php @@ -36,7 +36,6 @@ public function __construct(Configuration $configuration, Logger $logger, Sessio return; } - $this->session->start(); if ($this->session->getBool('loggedin', false)) { $this->loggedin = true; $this->logger->debug('logged in using valid session'); diff --git a/src/helpers/Session.php b/src/helpers/Session.php index ce56ba93d3..d7bde5c6fe 100644 --- a/src/helpers/Session.php +++ b/src/helpers/Session.php @@ -16,6 +16,8 @@ /** * Helper class for session management. + * + * This must be the only place to call `session_start()`. */ class Session { private bool $started = false; @@ -61,6 +63,8 @@ public function start(): void { } public function getBool(string $name, bool $default): bool { + $this->start(); + if (array_key_exists($name, $_SESSION) && is_bool($_SESSION[$name])) { return $_SESSION[$name]; } @@ -69,10 +73,16 @@ public function getBool(string $name, bool $default): bool { } public function setBool(string $name, bool $value): void { + $this->start(); + $_SESSION[$name] = $value; } public function destroy(): void { + if (!$this->started) { + return; + } + session_destroy(); } } From bba2a8c209f9581d94b99027cdcfa73111bfd663 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sat, 23 Nov 2024 18:24:38 +0100 Subject: [PATCH 3/5] tests: Fix login assertion Otherwise unexpected success will not be caught. --- tests/integration/run.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/run.py b/tests/integration/run.py index 330d43d6c4..dea6251432 100644 --- a/tests/integration/run.py +++ b/tests/integration/run.py @@ -23,8 +23,8 @@ def test_basic_workflow(self): url=fibonacci_feed_uri, ) assert ( - "Adding source is privileged operation and should fail without login." - ) + False + ), "Adding source is privileged operation and should fail without login." except requests.exceptions.HTTPError as e: assert ( e.response.status_code == 403 From 9f117a04c1e0384d48f940b48c71f5ebc4d17017 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sun, 24 Nov 2024 14:43:40 +0100 Subject: [PATCH 4/5] Standardize authorization levels Rename authorization levels to `read`/`update`/`privileged` and explicitly document them. --- src/controllers/Helpers/HashPassword.php | 2 +- src/controllers/Index.php | 2 +- src/controllers/Items.php | 10 ++++----- src/controllers/Items/Stats.php | 2 +- src/controllers/Items/Sync.php | 4 ++-- src/controllers/Opml/Export.php | 2 +- src/controllers/Opml/Import.php | 2 +- src/controllers/Rss.php | 2 +- src/controllers/Sources.php | 14 ++++++------ src/controllers/Sources/Write.php | 2 +- src/controllers/Tags.php | 4 ++-- src/helpers/Authentication.php | 27 +++++++++++++----------- 12 files changed, 38 insertions(+), 35 deletions(-) diff --git a/src/controllers/Helpers/HashPassword.php b/src/controllers/Helpers/HashPassword.php index 77998797d1..3588efd2f8 100644 --- a/src/controllers/Helpers/HashPassword.php +++ b/src/controllers/Helpers/HashPassword.php @@ -24,7 +24,7 @@ public function __construct(Authentication $authentication, View $view) { * json */ public function hash(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); if (!isset($_POST['password'])) { $this->view->jsonError([ diff --git a/src/controllers/Index.php b/src/controllers/Index.php index a920b26c89..8a9559501d 100644 --- a/src/controllers/Index.php +++ b/src/controllers/Index.php @@ -60,7 +60,7 @@ public function home(): void { return; } - $this->authentication->needsLoggedInOrPublicMode(); + $this->authentication->ensureCanRead(); // load tags $tags = $this->tagsDao->getWithUnread(); diff --git a/src/controllers/Items.php b/src/controllers/Items.php index ce8316b297..0d02e5c0a8 100644 --- a/src/controllers/Items.php +++ b/src/controllers/Items.php @@ -44,7 +44,7 @@ public function __construct( * @param ?string $itemId ID of item to mark as read */ public function mark(?string $itemId = null): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); $ids = null; if ($itemId !== null) { @@ -84,7 +84,7 @@ public function mark(?string $itemId = null): void { * @param string $itemId id of an item to mark as unread */ public function unmark(string $itemId): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); try { $itemId = Misc::forceId($itemId); @@ -106,7 +106,7 @@ public function unmark(string $itemId): void { * @param string $itemId id of an item to starr */ public function starr(string $itemId): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); try { $itemId = Misc::forceId($itemId); @@ -127,7 +127,7 @@ public function starr(string $itemId): void { * @param string $itemId id of an item to unstarr */ public function unstarr(string $itemId): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); try { $itemId = Misc::forceId($itemId); @@ -146,7 +146,7 @@ public function unstarr(string $itemId): void { * json */ public function listItems(): void { - $this->authentication->needsLoggedInOrPublicMode(); + $this->authentication->ensureCanRead(); // parse params $options = new ItemOptions($_GET); diff --git a/src/controllers/Items/Stats.php b/src/controllers/Items/Stats.php index 2454a50925..b50955ffd8 100644 --- a/src/controllers/Items/Stats.php +++ b/src/controllers/Items/Stats.php @@ -30,7 +30,7 @@ public function __construct(Authentication $authentication, \daos\Items $itemsDa * json */ public function stats(): void { - $this->authentication->needsLoggedInOrPublicMode(); + $this->authentication->ensureCanRead(); $stats = $this->itemsDao->stats(); diff --git a/src/controllers/Items/Sync.php b/src/controllers/Items/Sync.php index e808afce00..3b1a47eeeb 100644 --- a/src/controllers/Items/Sync.php +++ b/src/controllers/Items/Sync.php @@ -39,7 +39,7 @@ public function __construct(Authentication $authentication, Configuration $confi * json */ public function sync(): void { - $this->authentication->needsLoggedInOrPublicMode(); + $this->authentication->ensureCanRead(); if (isset($_GET['since'])) { $params = $_GET; @@ -113,7 +113,7 @@ public function sync(): void { * Items statuses bulk update. */ public function updateStatuses(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); if (isset($_POST['updatedStatuses'])) { $updatedStatuses = $_POST['updatedStatuses']; diff --git a/src/controllers/Opml/Export.php b/src/controllers/Opml/Export.php index 944ae31cf8..39c3e1192f 100644 --- a/src/controllers/Opml/Export.php +++ b/src/controllers/Opml/Export.php @@ -79,7 +79,7 @@ private function writeSource(array $source): void { * @note Uses the selfoss namespace to store selfoss-specific information */ public function export(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); $this->logger->debug('start OPML export'); $this->writer->openMemory(); diff --git a/src/controllers/Opml/Import.php b/src/controllers/Opml/Import.php index c5eca9f179..7f0a562048 100644 --- a/src/controllers/Opml/Import.php +++ b/src/controllers/Opml/Import.php @@ -42,7 +42,7 @@ public function __construct(Authentication $authentication, Logger $logger, \dao * @note Borrows from controllers/Sources.php:write */ public function add(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); http_response_code(400); diff --git a/src/controllers/Rss.php b/src/controllers/Rss.php index 50228a5204..27165501bf 100644 --- a/src/controllers/Rss.php +++ b/src/controllers/Rss.php @@ -38,7 +38,7 @@ public function __construct(Authentication $authentication, Configuration $confi * rss feed */ public function rss(): void { - $this->authentication->needsLoggedInOrPublicMode(); + $this->authentication->ensureCanRead(); $this->feedWriter->setTitle($this->configuration->rssTitle); $this->feedWriter->setChannelElement('description', ''); diff --git a/src/controllers/Sources.php b/src/controllers/Sources.php index 0490a5850c..2eefecd9ce 100644 --- a/src/controllers/Sources.php +++ b/src/controllers/Sources.php @@ -37,7 +37,7 @@ public function __construct(Authentication $authentication, \daos\Sources $sourc * json */ public function show(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); // get available spouts $spouts = $this->spoutLoader->all(); @@ -61,7 +61,7 @@ public function show(): void { * json */ public function add(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); $spouts = $this->spoutLoader->all(); @@ -75,7 +75,7 @@ public function add(): void { * json */ public function params(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); if (!isset($_GET['spout'])) { $this->view->error('no spout type given'); @@ -102,7 +102,7 @@ public function params(): void { * @param string $id ID of source to remove */ public function remove(string $id): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); try { $id = Misc::forceId($id); @@ -126,7 +126,7 @@ public function remove(string $id): void { * json */ public function listSources(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); // load sources $sources = $this->sourcesDao->getWithIcon(); @@ -145,7 +145,7 @@ public function listSources(): void { * json */ public function spouts(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); $spouts = $this->spoutLoader->all(); $this->view->jsonSuccess($spouts); @@ -156,7 +156,7 @@ public function spouts(): void { * json */ public function stats(): void { - $this->authentication->needsLoggedInOrPublicMode(); + $this->authentication->ensureCanRead(); // load sources $sources = $this->sourcesDao->getWithUnread(); diff --git a/src/controllers/Sources/Write.php b/src/controllers/Sources/Write.php index 2ade2628b1..66a69f39d2 100644 --- a/src/controllers/Sources/Write.php +++ b/src/controllers/Sources/Write.php @@ -51,7 +51,7 @@ public function __construct( * @param ?string $id ID of source to update, or null to create a new one */ public function write(?string $id = null): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); $data = $this->request->getData(); diff --git a/src/controllers/Tags.php b/src/controllers/Tags.php index 6b4d26d542..a57e053061 100644 --- a/src/controllers/Tags.php +++ b/src/controllers/Tags.php @@ -70,7 +70,7 @@ public function tagsAddColors(array $itemTags, ?array $tags = null): StringKeyed * set tag color */ public function color(): void { - $this->authentication->needsLoggedIn(); + $this->authentication->ensureIsPrivileged(); $data = $this->request->getData(); @@ -101,7 +101,7 @@ public function color(): void { * html */ public function listTags(): void { - $this->authentication->needsLoggedInOrPublicMode(); + $this->authentication->ensureCanRead(); $tags = $this->tagsDao->getWithUnread(); diff --git a/src/helpers/Authentication.php b/src/helpers/Authentication.php index 1fb7ed20f6..f6a300a3d9 100644 --- a/src/helpers/Authentication.php +++ b/src/helpers/Authentication.php @@ -16,6 +16,12 @@ /** * Helper class for user authentication. + * + * selfoss currently has three kinds of controlled resources that user can be authorized to access: + * + * - **Read**: Read-only access available to unauthenticated users if *public mode* is enabled. + * - **Update**: Allows triggering source updates when *public update mode* is enabled. + * - **Privileged**: Any other operation (admin) user, full access without any limitations. */ class Authentication { private bool $loggedin = false; @@ -88,10 +94,7 @@ public function login(string $username, string $password): bool { return true; } - /** - * isloggedin - */ - public function isLoggedin(): bool { + private function isPrivileged(): bool { if ($this->enabled() === false) { return true; } @@ -103,7 +106,7 @@ public function isLoggedin(): bool { * showPrivateTags */ public function showPrivateTags(): bool { - return $this->isLoggedin(); + return $this->isPrivileged(); } /** @@ -117,19 +120,19 @@ public function logout(): void { } /** - * send 403 if not logged in and not public mode + * If user is not authorized to read, force them to authenticate. */ - public function needsLoggedInOrPublicMode(): void { - if ($this->isLoggedin() !== true && !$this->configuration->public) { + public function ensureCanRead(): void { + if ($this->isPrivileged() !== true && !$this->configuration->public) { $this->forbidden(); } } /** - * send 403 if not logged in + * If user is not authorized to privileged operations, force them to authenticate. */ - public function needsLoggedIn(): void { - if ($this->isLoggedin() !== true) { + public function ensureIsPrivileged(): void { + if ($this->isPrivileged() !== true) { $this->forbidden(); } } @@ -151,7 +154,7 @@ public function forbidden(): void { * or public update must be allowed in the config. */ public function allowedToUpdate(): bool { - return $this->isLoggedin() === true + return $this->isPrivileged() === true || $_SERVER['REMOTE_ADDR'] === $_SERVER['SERVER_ADDR'] || $_SERVER['REMOTE_ADDR'] === '127.0.0.1' || $this->configuration->allowPublicUpdateAccess; From cf74581ca66466ba205bf760ebe36821ba9b5360 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Thu, 11 Jul 2024 00:13:00 +0200 Subject: [PATCH 5/5] Rework Authentication into separate services This is a from-scratch rewrite, moving a bit closer to Single Responsibility Principle. We split handling of credentials-in-config and always-open authentication systems. In the future, we will be able implement more methods this way. This was motivated by session code being called in constructor, which would break in CLI with Tracy strict mode. For now, we are just porting the Authentication helper and controller. Additionally: - Session verification now also checks if the credentials in the config did not change. - Requests from loopback IP address now give full access to all operations, not just update. - IPv6 loopback address is recognized as well. - Requests forwarded by proxies are filtered out since local reverse proxies might come from loopback as well. One thing I do not like that any request with credentials will automatically persist the login to session but removing that feature can be done later. --- NEWS.md | 3 + src/common.php | 9 ++ src/controllers/About.php | 2 +- src/controllers/Authentication.php | 23 ++- src/helpers/Authentication.php | 101 ++----------- .../Authentication/AuthenticationFactory.php | 49 +++++++ .../Authentication/AuthenticationService.php | 34 +++++ .../Services/RequestOrSession.php | 137 ++++++++++++++++++ src/helpers/Authentication/Services/Trust.php | 33 +++++ src/helpers/Session.php | 8 +- tests/integration/helpers/selfoss_api.py | 7 + 11 files changed, 299 insertions(+), 107 deletions(-) create mode 100644 src/helpers/Authentication/AuthenticationFactory.php create mode 100644 src/helpers/Authentication/AuthenticationService.php create mode 100644 src/helpers/Authentication/Services/RequestOrSession.php create mode 100644 src/helpers/Authentication/Services/Trust.php diff --git a/NEWS.md b/NEWS.md index d1ca3df832..69598c4720 100644 --- a/NEWS.md +++ b/NEWS.md @@ -40,6 +40,9 @@ - Source filters are stricter, they need to start and end with a `/`. ([#1423](https://github.com/fossar/selfoss/pull/1423)) - OPML importer has been merged into the React client. ([#1442](https://github.com/fossar/selfoss/pull/1442)) - Web requests will send `Accept-Encoding` header. ([#1482](https://github.com/fossar/selfoss/pull/1482)) +- Authentication system has been rewritten to allow more methods in the future. ([#1491](https://github.com/fossar/selfoss/pull/1491)) +- Authentication will now also log user out when the credentials in the config change. ([#1491](https://github.com/fossar/selfoss/pull/1491)) +- Requests from loopback IP address now give full access to all operations, not just update. Additionally, IPv6 loopback address is recognized and proxies are ignored. ([#1491](https://github.com/fossar/selfoss/pull/1491)) #### For developers - Back-end source code is now checked using [PHPStan](https://phpstan.org/). ([#1409](https://github.com/fossar/selfoss/pull/1409)) diff --git a/src/common.php b/src/common.php index a9e226e320..efbc65f9bf 100644 --- a/src/common.php +++ b/src/common.php @@ -69,6 +69,15 @@ function boot_error(string $message) { ->register(helpers\Authentication::class) ->setShared(true) ; + +$container + ->register( + helpers\Authentication\AuthenticationService::class, + [new Slince\Di\Reference(helpers\Authentication\AuthenticationFactory::class), 'create'] + ) + ->setShared(true) +; + $container ->register(helpers\Session::class) ->setShared(true) diff --git a/src/controllers/About.php b/src/controllers/About.php index 5e0af5f716..7ed1864aa3 100644 --- a/src/controllers/About.php +++ b/src/controllers/About.php @@ -54,7 +54,7 @@ public function about(): void { 'htmlTitle' => trim($this->configuration->htmlTitle), // string 'allowPublicUpdate' => $this->configuration->allowPublicUpdateAccess, // bool 'publicMode' => $this->configuration->public, // bool - 'authEnabled' => $this->authentication->enabled() === true, // bool + 'authEnabled' => $this->authentication->enabled(), // bool 'readingSpeed' => $this->configuration->readingSpeedWpm > 0 ? $this->configuration->readingSpeedWpm : null, // ?int 'language' => $this->configuration->language === '0' ? null : $this->configuration->language, // ?string 'userCss' => file_exists(BASEDIR . '/user.css') ? filemtime(BASEDIR . '/user.css') : null, // ?int diff --git a/src/controllers/Authentication.php b/src/controllers/Authentication.php index bafd37776e..1d24125c11 100644 --- a/src/controllers/Authentication.php +++ b/src/controllers/Authentication.php @@ -4,18 +4,18 @@ namespace controllers; -use helpers; +use helpers\Authentication\AuthenticationService; use helpers\View; /** * Controller for user related tasks */ class Authentication { - private helpers\Authentication $authentication; + private AuthenticationService $authenticationService; private View $view; - public function __construct(helpers\Authentication $authentication, View $view) { - $this->authentication = $authentication; + public function __construct(AuthenticationService $authenticationService, View $view) { + $this->authenticationService = $authenticationService; $this->view = $view; } @@ -26,17 +26,11 @@ public function __construct(helpers\Authentication $authentication, View $view) public function login(): void { $error = null; - if (isset($_REQUEST['username'])) { - $username = $_REQUEST['username']; - } else { - $username = ''; + if (!isset($_REQUEST['username'])) { $error = 'no username given'; } - if (isset($_REQUEST['password'])) { - $password = $_REQUEST['password']; - } else { - $password = ''; + if (!isset($_REQUEST['password'])) { $error = 'no password given'; } @@ -47,7 +41,8 @@ public function login(): void { ]); } - if ($this->authentication->login($username, $password)) { + // The function automatically checks the request for credentials. + if ($this->authenticationService->isPrivileged()) { $this->view->jsonSuccess([ 'success' => true, ]); @@ -64,7 +59,7 @@ public function login(): void { * json */ public function logout(): void { - $this->authentication->logout(); + $this->authenticationService->destroy(); $this->view->jsonSuccess([ 'success' => true, ]); diff --git a/src/helpers/Authentication.php b/src/helpers/Authentication.php index f6a300a3d9..893b6c7680 100644 --- a/src/helpers/Authentication.php +++ b/src/helpers/Authentication.php @@ -12,7 +12,7 @@ namespace helpers; -use Monolog\Logger; +use helpers\Authentication\AuthenticationService; /** * Helper class for user authentication. @@ -24,106 +24,31 @@ * - **Privileged**: Any other operation (admin) user, full access without any limitations. */ class Authentication { - private bool $loggedin = false; + private AuthenticationService $authenticationService; - private Configuration $configuration; - private Logger $logger; - private Session $session; - - /** - * start session and check login - */ - public function __construct(Configuration $configuration, Logger $logger, Session $session) { - $this->configuration = $configuration; - $this->logger = $logger; - $this->session = $session; - - if ($this->enabled() === false) { - return; - } - - if ($this->session->getBool('loggedin', false)) { - $this->loggedin = true; - $this->logger->debug('logged in using valid session'); - } else { - $this->logger->debug('session does not contain valid auth'); - } - - // autologin if request contains unsername and password - if ($this->loggedin === false - && isset($_REQUEST['username']) - && isset($_REQUEST['password'])) { - $this->login($_REQUEST['username'], $_REQUEST['password']); - } + public function __construct(AuthenticationService $authenticationService) { + $this->authenticationService = $authenticationService; } /** * login enabled */ public function enabled(): bool { - return strlen($this->configuration->username) != 0 && strlen($this->configuration->password) != 0; - } - - /** - * login user - */ - public function login(string $username, string $password): bool { - if ($this->enabled()) { - $usernameCorrect = $username === $this->configuration->username; - $hashedPassword = $this->configuration->password; - // Passwords hashed with password_hash start with $, otherwise use the legacy path. - $passwordCorrect = - $hashedPassword !== '' && $hashedPassword[0] === '$' - ? password_verify($password, $hashedPassword) - : hash('sha512', $this->configuration->salt . $password) === $hashedPassword; - $credentialsCorrect = $usernameCorrect && $passwordCorrect; - - if ($credentialsCorrect) { - $this->loggedin = true; - $this->session->setBool('loggedin', true); - $this->logger->debug('logged in with supplied username and password'); - - return true; - } else { - $this->logger->debug('failed to log in with supplied username and password'); - - return false; - } - } - - return true; - } - - private function isPrivileged(): bool { - if ($this->enabled() === false) { - return true; - } - - return $this->loggedin; + return !$this->authenticationService instanceof Authentication\Services\Trust; } /** * showPrivateTags */ public function showPrivateTags(): bool { - return $this->isPrivileged(); - } - - /** - * logout - */ - public function logout(): void { - $this->loggedin = false; - $this->session->setBool('loggedin', false); - $this->session->destroy(); - $this->logger->debug('logged out and destroyed session'); + return $this->authenticationService->isPrivileged(); } /** * If user is not authorized to read, force them to authenticate. */ public function ensureCanRead(): void { - if ($this->isPrivileged() !== true && !$this->configuration->public) { + if (!$this->authenticationService->canRead()) { $this->forbidden(); } } @@ -132,16 +57,19 @@ public function ensureCanRead(): void { * If user is not authorized to privileged operations, force them to authenticate. */ public function ensureIsPrivileged(): void { - if ($this->isPrivileged() !== true) { + if (!$this->authenticationService->isPrivileged()) { $this->forbidden(); } } /** * send 403 if not logged in + * + * @return never */ - public function forbidden(): void { + private function forbidden(): void { header('HTTP/1.0 403 Forbidden'); + header('Content-type: text/plain'); echo 'Access forbidden!'; exit; } @@ -154,9 +82,6 @@ public function forbidden(): void { * or public update must be allowed in the config. */ public function allowedToUpdate(): bool { - return $this->isPrivileged() === true - || $_SERVER['REMOTE_ADDR'] === $_SERVER['SERVER_ADDR'] - || $_SERVER['REMOTE_ADDR'] === '127.0.0.1' - || $this->configuration->allowPublicUpdateAccess; + return $this->authenticationService->canUpdate(); } } diff --git a/src/helpers/Authentication/AuthenticationFactory.php b/src/helpers/Authentication/AuthenticationFactory.php new file mode 100644 index 0000000000..4f3a2f44db --- /dev/null +++ b/src/helpers/Authentication/AuthenticationFactory.php @@ -0,0 +1,49 @@ + +// SPDX-License-Identifier: GPL-3.0-or-later + +declare(strict_types=1); + +namespace helpers\Authentication; + +use helpers\Configuration; +use Psr\Container\ContainerInterface; + +/** + * Factory that creates `AuthenticationService` based on the configuration. + */ +final class AuthenticationFactory { + private Configuration $configuration; + private ContainerInterface $container; + + public function __construct(Configuration $configuration, ContainerInterface $container) { + $this->configuration = $configuration; + $this->container = $container; + } + + public function create(): AuthenticationService { + if (!$this->useCredentials() || $this->isCli() || $this->isLocalIp()) { + return $this->container->get(Services\Trust::class); + } + + return $this->container->get(Services\RequestOrSession::class); + } + + private function isCli(): bool { + return PHP_SAPI === 'cli'; + } + + private function isLocalIp(): bool { + // We cannot trust these IP addresses but we know they are likely not local. + if (isset($_SERVER['HTTP_X_FORWARDED_FOR']) || isset($_SERVER['HTTP_FORWARDED'])) { + return false; + } + + return $_SERVER['REMOTE_ADDR'] === '::1' || $_SERVER['REMOTE_ADDR'] === '127.0.0.1'; + } + + private function useCredentials(): bool { + return strlen($this->configuration->username) > 0 && strlen($this->configuration->password) > 0; + } +} diff --git a/src/helpers/Authentication/AuthenticationService.php b/src/helpers/Authentication/AuthenticationService.php new file mode 100644 index 0000000000..7fac450805 --- /dev/null +++ b/src/helpers/Authentication/AuthenticationService.php @@ -0,0 +1,34 @@ + +// SPDX-License-Identifier: GPL-3.0-or-later + +declare(strict_types=1); + +namespace helpers\Authentication; + +/** + * Must be implemented by any authentication service. + */ +interface AuthenticationService { + /** + * Checks whether user is authorized to read (logged in/public mode). + */ + public function canRead(): bool; + + /** + * Checks whether user is authorized to update sources (logged in/public update mode). + */ + public function canUpdate(): bool; + + /** + * Checks whether user is authorized to perform a privileged action + * or access privileged information. + */ + public function isPrivileged(): bool; + + /** + * Give up authorization. + */ + public function destroy(): void; +} diff --git a/src/helpers/Authentication/Services/RequestOrSession.php b/src/helpers/Authentication/Services/RequestOrSession.php new file mode 100644 index 0000000000..146c262542 --- /dev/null +++ b/src/helpers/Authentication/Services/RequestOrSession.php @@ -0,0 +1,137 @@ + +// SPDX-License-Identifier: GPL-3.0-or-later + +declare(strict_types=1); + +namespace helpers\Authentication\Services; + +use helpers\Authentication\AuthenticationService; +use helpers\Configuration; +use helpers\Session; +use Monolog\Logger; + +/** + * Authentication method that verifies credentials given using + * the following means against those specified in configuration file: + * + * - `username` and `password` in `POST` data + * - `username` and `password` in `GET` query string + * + * Additionally, it will persist the authorization in a session. + */ +final class RequestOrSession implements AuthenticationService { + private const SESSION_KEY = 'authorized'; + + private Configuration $configuration; + private Logger $logger; + private Session $session; + + private ?bool $authorized = null; + + public function __construct(Configuration $configuration, Logger $logger, Session $session) { + $this->configuration = $configuration; + $this->logger = $logger; + $this->session = $session; + } + + public function canRead(): bool { + return $this->configuration->public || $this->isPrivileged(); + } + + public function canUpdate(): bool { + return $this->configuration->allowPublicUpdateAccess || $this->isPrivileged(); + } + + public function isPrivileged(): bool { + if ($this->authorized === null) { + $this->authorized = $this->checkSession() || $this->checkRequest(); + } + + return $this->authorized; + } + + /** + * Checks if there is a authorized session and matches it against the credentials in the config. + */ + private function checkSession(): bool { + $authorization = $this->session->getString(self::SESSION_KEY); + + if ($authorization === null) { + $this->logger->debug('Session does not contain authorization string'); + } elseif ($authorization === $this->getExpectedAuthorizationSession()) { + $this->logger->debug('Access granted based on a session'); + + return true; + } + + $this->logger->debug('Session does not contain valid auth (credentials changed)'); + + return false; + } + + /** + * Checks if the request contains credentials and stores the authorization in a session if it does. + */ + private function checkRequest(): bool { + if (!isset($_REQUEST['username']) || !isset($_REQUEST['password'])) { + return false; + } + + if ($this->verifyCredentials($_REQUEST['username'], $_REQUEST['password'])) { + $this->logger->debug('Access granted based on request credentials'); + $this->saveAuthorization(); + + return true; + } + + $this->logger->debug('Request credentials not valid'); + + return false; + } + + /** + * Generates a session value that will serve as a proof of authentication. + * + * We are storing the username & password hash to ensure clients + * are logged out when credentials change. + */ + private function getExpectedAuthorizationSession(): string { + // We are using the password hash from the config for simplicity. + // This will cause users to be signed out if they rehash the password but that should be rare. + return $this->configuration->username . ':::' . $this->configuration->password; + } + + /** + * Checks credentials against config. + */ + private function verifyCredentials(string $username, string $password): bool { + return $username === $this->configuration->username && $this->verifyPassword($password); + } + + /** + * Checks password against config. + */ + private function verifyPassword(string $password): bool { + $hashedPassword = $this->configuration->password; + + // Passwords hashed with password_hash start with $, otherwise use the legacy path. + return + $hashedPassword !== '' && $hashedPassword[0] === '$' + ? password_verify($password, $hashedPassword) + : hash('sha512', $this->configuration->salt . $password) === $hashedPassword; + } + + private function saveAuthorization(): void { + $authorization = $this->getExpectedAuthorizationSession(); + $this->session->setString(self::SESSION_KEY, $authorization); + } + + public function destroy(): void { + $this->authorized = false; + $this->session->setString(self::SESSION_KEY, null); + $this->session->destroy(); + $this->logger->debug('Logged out and destroyed session'); + } +} diff --git a/src/helpers/Authentication/Services/Trust.php b/src/helpers/Authentication/Services/Trust.php new file mode 100644 index 0000000000..84f8bd7768 --- /dev/null +++ b/src/helpers/Authentication/Services/Trust.php @@ -0,0 +1,33 @@ + +// SPDX-License-Identifier: GPL-3.0-or-later + +declare(strict_types=1); + +namespace helpers\Authentication\Services; + +use helpers\Authentication\AuthenticationService; + +/** + * Trivial authentication service that allows any access. + * + * To be used for CLI or when authentication is disabled. + */ +final class Trust implements AuthenticationService { + public function canRead(): bool { + return true; + } + + public function canUpdate(): bool { + return true; + } + + public function isPrivileged(): bool { + return true; + } + + public function destroy(): void { + // Nothing to rescind. + } +} diff --git a/src/helpers/Session.php b/src/helpers/Session.php index d7bde5c6fe..3e8779170c 100644 --- a/src/helpers/Session.php +++ b/src/helpers/Session.php @@ -62,17 +62,17 @@ public function start(): void { } } - public function getBool(string $name, bool $default): bool { + public function getString(string $name): ?string { $this->start(); - if (array_key_exists($name, $_SESSION) && is_bool($_SESSION[$name])) { + if (array_key_exists($name, $_SESSION) && is_string($_SESSION[$name])) { return $_SESSION[$name]; } - return $default; + return null; } - public function setBool(string $name, bool $value): void { + public function setString(string $name, ?string $value): void { $this->start(); $_SESSION[$name] = $value; diff --git a/tests/integration/helpers/selfoss_api.py b/tests/integration/helpers/selfoss_api.py index 1561401384..84af951524 100644 --- a/tests/integration/helpers/selfoss_api.py +++ b/tests/integration/helpers/selfoss_api.py @@ -7,6 +7,13 @@ def __init__(self, base_uri: str): # We still use cookies for authentication so let’s persist them across requests. self.session = requests.Session() + self.session.headers.update( + { + # Pretend that we are coming from a different computer so that local authentication bypass does not happen. + "X-Forwarded-For": "1.2.3.4", + } + ) + def login(self, username, password): r = self.session.post( f"{self.base_uri}/login",