Skip to content

Commit

Permalink
Rework Authentication into separate services
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jtojnar committed Nov 24, 2024
1 parent 9f117a0 commit cf74581
Show file tree
Hide file tree
Showing 11 changed files with 299 additions and 107 deletions.
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
9 changes: 9 additions & 0 deletions src/common.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/About.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 9 additions & 14 deletions src/controllers/Authentication.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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';
}

Expand All @@ -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,
]);
Expand All @@ -64,7 +59,7 @@ public function login(): void {
* json
*/
public function logout(): void {
$this->authentication->logout();
$this->authenticationService->destroy();
$this->view->jsonSuccess([
'success' => true,
]);
Expand Down
101 changes: 13 additions & 88 deletions src/helpers/Authentication.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

namespace helpers;

use Monolog\Logger;
use helpers\Authentication\AuthenticationService;

/**
* Helper class for user authentication.
Expand All @@ -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();
}
}
Expand All @@ -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;
}
Expand All @@ -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();
}
}
49 changes: 49 additions & 0 deletions src/helpers/Authentication/AuthenticationFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

// SPDX-FileCopyrightText: 2024 Jan Tojnar <jtojnar@gmail.com>
// 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;
}
}
34 changes: 34 additions & 0 deletions src/helpers/Authentication/AuthenticationService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

// SPDX-FileCopyrightText: 2024 Jan Tojnar <jtojnar@gmail.com>
// 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;
}
Loading

0 comments on commit cf74581

Please sign in to comment.