Skip to content

Commit

Permalink
refactor: migrated 2FA to controller (#3257)
Browse files Browse the repository at this point in the history
  • Loading branch information
thorsten committed Dec 29, 2024
1 parent 984b874 commit d4cd819
Show file tree
Hide file tree
Showing 9 changed files with 150 additions and 100 deletions.
2 changes: 1 addition & 1 deletion nginx.conf
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ server {
rewrite admin/api/(.*) /admin/api/index.php last;

# Administration pages
rewrite admin/(attachments|authenticate|backup|category|comments|configuration|elasticsearch|export|faq|faqs|forms|glossary|group|import|instance|instances|logout|login|media-browser|news|password|questions|session-keep-alive|statistics|sticky-faqs|stopwords|system|tags|update|user) /admin/front.php last;
rewrite admin/(2fa|attachments|authenticate|backup|category|check|comments|configuration|elasticsearch|export|faq|faqs|forms|glossary|group|import|instance|instances|logout|login|media-browser|news|password|questions|session-keep-alive|statistics|sticky-faqs|stopwords|system|token|tags|update|user) /admin/front.php last;

# REST API v3.0 and v3.1
rewrite ^api/v3\.[01]/(.*) /api/index.php last;
Expand Down
2 changes: 1 addition & 1 deletion phpmyfaq/.htaccess
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ Header set Access-Control-Allow-Headers "Content-Type, Authorization"
# Administration API
RewriteRule ^admin/api/(.*) admin/api/index.php [L,QSA]
# Administration pages
RewriteRule ^admin/(attachments|authenticate|backup|category|comments|configuration|elasticsearch|export|faq|faqs|forms|glossary|group|import|instance|instances|login|logout|media-browser|news|password|questions|session-keep-alive|statistics|sticky-faqs|stopwords|system|tags|update|user) admin/front.php [L,QSA]
RewriteRule ^admin/(2fa|attachments|authenticate|backup|category|check|comments|configuration|elasticsearch|export|faq|faqs|forms|glossary|group|import|instance|instances|login|logout|media-browser|news|password|questions|session-keep-alive|statistics|sticky-faqs|stopwords|system|tags|token|update|user) admin/front.php [L,QSA]
#RewriteRule ^admin/(.*) admin/front.php [L,QSA]
# Private APIs
RewriteRule ^api/(autocomplete|bookmark/delete|bookmark/create|user/data/update|user/password/update|user/request-removal|user/remove-twofactor|contact|voting|register|captcha|share|comment/create|faq/create|question/create|webauthn/prepare|webauthn/register|webauthn/prepare-login|webauthn/login) api/index.php [L,QSA]
Expand Down
46 changes: 0 additions & 46 deletions phpmyfaq/admin/twofactor.php

This file was deleted.

89 changes: 44 additions & 45 deletions phpmyfaq/assets/templates/admin/user/twofactor.twig
Original file line number Diff line number Diff line change
@@ -1,53 +1,52 @@
{% if ( requestIsSecure == true ) or ( security.useSslForLogins == false ) %}
<div class="container py-5">
<div class="row">
<div class="col-lg-12">
<div class="row">
<div class="col-lg-6 mx-auto">
<div class="card rounded-0" id="login-form">
<div class="card-header">
<h3 class="mb-0">
{{ msgTwofactorEnabled }}
</h3>
{% if actionIsLogout == true %}
<p class="alert alert-danger alert-dismissible fade show mt-3">
<button type="button" class="close" data-dismiss="alert"><span aria-hidden="true">&times;</span></button>
{{ ad_logout }}
</p>
{% endif %}
{% if ( error is defined ) and ( error|length > 0 ) %}
<p class="alert alert-danger alert-dismissible fade show mt-3">
<button type="button" class="close" data-dismiss="alert"><span aria-hidden="true">&times;</span></button>
{{ error }}
</p>
{% endif %}
{% extends '@admin/index.twig' %}

</div>
<div class="card-body">
<form action="{{ systemUri }}admin/index.php" method="post"
accept-charset="utf-8" role="form" class="pmf-form-login">
<input type="hidden" name="userid" id="userid" value="<?= $userid ?>">
<input type="hidden" name="redirect-action" value="<?= $action ?>">
<div class="form-group">
<label for="token">{{ msgEnterTwofactorToken }}</label>
<div class="col-4 mx-auto my-2">
<input type="text" class="form-control form-control-lg text-center rounded-0" name="token"
id="token" autocomplete="off" maxlength="6" autofocus required>
{% block content %}
{% if ( requestIsSecure == true ) or ( security.useSslForLogins == false ) %}
<div class="container py-5">
<div class="row">
<div class="col-lg-12">
<div class="row">
<div class="col-lg-6 mx-auto">
<div class="card rounded-0" id="login-form">
<div class="card-header">
<h3 class="mb-0">
{{ msgTwofactorEnabled }}
</h3>
{% if ( error is defined ) and ( error|length > 0 ) %}
xxx
<p class="alert alert-danger alert-dismissible fade show mt-3">
<button type="button" class="close" data-dismiss="alert"><span aria-hidden="true">&times;</span></button>
{{ error }}
</p>
{% endif %}

</div>
<div class="card-body">
<form action="{{ systemUri }}admin/check" method="post"
accept-charset="utf-8" role="form" class="pmf-form-login">
<input type="hidden" name="user-id" id="user-id" value="{{ userId }}">
<input type="hidden" name="redirect-action" value="{{ redirectAction }}">
<div class="form-group">
<label for="token">{{ msgEnterTwofactorToken }}</label>
<div class="col-4 mx-auto my-2">
<input type="text" class="form-control form-control-lg text-center rounded-0" name="token"
id="token" autocomplete="off" maxlength="6" autofocus required>
</div>
</div>
<div class="d-grid gap-2 col-6 mx-auto">
<button type="submit" class="btn btn-success btn-lg float-right" id="btnLogin">
{{ msgTwofactorCheck }}
</button>
</div>
</div>
<div class="d-grid gap-2 col-6 mx-auto">
<button type="submit" class="btn btn-success btn-lg float-right" id="btnLogin">
{{ msgTwofactorCheck }}
</button>
</div>
</form>
</form>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
{% else %}
<p><a href="https://{{ requestHost }}{{ requestUri }}">{{ msgSecureSwitch }}</a></p>
{% endif %}
{% else %}
<p><a href="https://{{ requestHost }}{{ requestUri }}">{{ msgSecureSwitch }}</a></p>
{% endif %}
{% endblock %}
11 changes: 11 additions & 0 deletions phpmyfaq/src/admin-routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@
'controller' => [AuthenticationController::class, 'authenticate'],
'methods' => 'POST'
],
'admin.auth.check' => [
'path' => '/check',
'controller' => [AuthenticationController::class, 'check'],
'methods' => 'POST'
],
'admin.auth.login' => [
'path' => '/login',
'controller' => [AuthenticationController::class, 'login'],
Expand All @@ -71,6 +76,12 @@
'controller' => [AuthenticationController::class, 'logout'],
'methods' => 'GET'
],
'admin.auth.token' => [
'path' => '/token',
'controller' => [AuthenticationController::class, 'token'],
'methods' => 'GET',

],
'admin.backup' => [
'path' => '/backup',
'controller' => [BackupController::class, 'index'],
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,20 @@
<?php

/**
* The Administration Authentication Controller
*
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at https://mozilla.org/MPL/2.0/.
*
* @package phpMyFAQ
* @author Thorsten Rinne <[email protected]>
* @copyright 2024 phpMyFAQ Team
* @license https://www.mozilla.org/MPL/2.0/ Mozilla Public License Version 2.0
* @link https://www.phpmyfaq.de
* @since 2024-12-28
*/

declare(strict_types=1);

namespace phpMyFAQ\Controller\Administration;
Expand Down Expand Up @@ -47,12 +62,12 @@ public function authenticate(Request $request): Response
try {
$this->currentUser = $userAuth->authenticate($username, $password);
if ($userAuth->hasTwoFactorAuthentication()) {
return new RedirectResponse('./2fa');
return new RedirectResponse('./token?user-id=' . $this->currentUser->getUserId());
}
} catch (Exception $e) {
$logging->log(
$this->currentUser,
'Login-error\nLogin: ' . $username . '\nErrors: ' . implode(', ', $this->configuration->errors)
'Login-error\nLogin: ' . $username . '\nErrors: ' . implode(', ', $this->currentUser->errors)
);
//$error = $e->getMessage();
return new RedirectResponse('./login');
Expand Down Expand Up @@ -116,6 +131,7 @@ public function logout(Request $request): Response

$csrfToken = Filter::filterVar($request->get('csrf'), FILTER_SANITIZE_SPECIAL_CHARS);
if (!Token::getInstance($this->container->get('session'))->verifyToken('admin-logout', $csrfToken)) {
// add an error message
return $redirect->send();
}

Expand All @@ -128,4 +144,68 @@ public function logout(Request $request): Response

return $redirect->send();
}

/**
* @throws \Exception
*/
#[Route('/token', name: 'admin.auth.token', methods: ['GET'])]
public function token(Request $request): Response
{
if ($this->currentUser->isLoggedIn()) {
return new RedirectResponse('./');
}

$userId = Filter::filterVar($request->get('user-id'), FILTER_VALIDATE_INT);

return $this->render(
'@admin/user/twofactor.twig',
[
... $this->getHeader($request),
... $this->getFooter(),
'msgTwofactorEnabled' => Translation::get('msgTwofactorEnabled'),
'msgTwofactorCheck' => Translation::get('msgTwofactorCheck'),
'msgEnterTwofactorToken' => Translation::get('msgEnterTwofactorToken'),
'requestIsSecure' => $request->isSecure(),
'security.useSslForLogins' => $this->configuration->get('security.useSslForLogins'),
'requestHost' => $request->getHost(),
'requestUri' => $request->getRequestUri(),
'userId' => $userId,
'msgSecureSwitch' => Translation::get('msgSecureSwitch'),
'systemUri' => $this->configuration->getDefaultUrl()
]
);
}

/**
* @throws \Exception
*/
#[Route('/check', name: 'admin.auth.check', methods: ['POST'])]
public function check(Request $request): Response
{
if ($this->currentUser->isLoggedIn()) {
return new RedirectResponse('./');
}

$token = Filter::filterVar($request->get('token'), FILTER_VALIDATE_INT);
$userId = Filter::filterVar($request->get('user-id'), FILTER_VALIDATE_INT);

$user = $this->container->get('phpmyfaq.user.current_user');
$user->getUserById($userId);

if (strlen((string) $token) === 6) {
$tfa = $this->container->get('phpmyfaq.user.two-factor');
$result = $tfa->validateToken($token, $userId);

if ($result) {
$user->twoFactorSuccess();
return new RedirectResponse('./');
} else {
// add an error message
return new RedirectResponse('./token?user-id=' . $userId);
}
} else {
// add an error message
return new RedirectResponse('./token?user-id=' . $userId);
}
}
}
6 changes: 3 additions & 3 deletions phpmyfaq/src/phpMyFAQ/User/TwoFactor.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ public function getSecret(CurrentUser $currentUser): string|null
/**
* Validates a given token. Returns true if the token is correct.
*/
public function validateToken(string $token, int $userId): bool
public function validateToken(int $token, int $userId): bool
{
if (strlen($token) !== 6) {
if (strlen((string)$token) !== 6) {
return false;
}

Expand All @@ -101,7 +101,7 @@ public function validateToken(string $token, int $userId): bool
*/
public function getQrCode(string $secret): string
{
$label = $this->configuration->getTitle() . ':' . $this->currentUser->getUserData('email');
$label = $this->configuration->getTitle() . 'XXXXX:' . $this->currentUser->getUserData('email');
$qrCodeText = sprintf(
'%s&image=%sassets/templates/images/logo.png',
$this->twoFactorAuth->getQrText($label, $secret),
Expand Down
8 changes: 7 additions & 1 deletion phpmyfaq/src/services.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
use phpMyFAQ\Tags;
use phpMyFAQ\User;
use phpMyFAQ\User\CurrentUser;
use phpMyFAQ\User\TwoFactor;
use phpMyFAQ\User\UserSession;
use phpMyFAQ\Visits;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
Expand Down Expand Up @@ -294,9 +295,14 @@
new Reference('phpmyfaq.configuration')
]);

$services->set('phpmyfaq.user.two-factor', TwoFactor::class)
->args([
new Reference('phpmyfaq.configuration'),
new Reference('phpmyfaq.user.current_user')
]);

$services->set('phpmyfaq.visits', Visits::class)
->args([
new Reference('phpmyfaq.configuration')
]);

};
2 changes: 1 addition & 1 deletion tests/phpMyFAQ/User/TwoFactorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public function testValidateToken(): void
$property->setAccessible(true);
$property->setValue($this->twoFactor, $twoFactorAuth);

$result = $this->twoFactor->validateToken('123456', 1);
$result = $this->twoFactor->validateToken(123456, 1);
$this->assertTrue($result);
}

Expand Down

0 comments on commit d4cd819

Please sign in to comment.