Skip to content

Commit

Permalink
Correctly handle denied access in the firewall (see #6805)
Browse files Browse the repository at this point in the history
Description
-----------

This now fixes and improves a lot of things 🙈 

1. Fixes the authentication system to correctly render the Contao login page even if the current route is not a Contao page (e.g. a custom controller)
1. Fixes the `ContaoLoginAuthenticator` from rendering the wrong page for login (if the user is not fully authenticated).
2. ~Fixes the remember me authentication system~ (see #6815)
3. Adds an `AccessDeniedHandler` to render the 403 page via the firewall exception handler
4. Correctly enforces fully authentication for changing personal data, changing the user password and configuring two-factor authentication
5. Removes the ExceptionConverter and PrettyErrorScreen from rendering 401 and 403 pages, because these are rendered by the firewall

There are two "behaviour changes" I can think of
-  obviously the fully-authentication works and is now enforces as described in point 4
- If no 401 or 403 pages are configured in root page, we now render the generic error (through the pretty error screen listener) instead of the generic Symfony 401/403 message. Since we are in Contao frontend scope, I think that should be fine.

### TODO:
- [x] ~Needs a migration for existing RememberMe tokens (the migration tool cannot migrate binary data to string value because the binary data is null-padded and therefore longer than the new string field).~

Commits
-------

9cba1e8 Correctly handle denied access in firewall
5d88701 Handle full authentication in login and user modules
8ffa520 Fix the rememberme functionality
45b2916 CS & Tests
ec43cfc CS & Tests
272f0ad Update the security configs
9dc8d9e Improved re-authentication in login module
a4f7264 Tests
b807be1 Update core-bundle/contao/languages/en/default.xlf
e917497 Do not treat the "please verify your identity" message as error
7e07f1c Handle re-authentication on the redirect page
d9d05c3 Correctly handle request object
90d5b70 Correctly handle request object

Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
  • Loading branch information
aschempp and leofeyer committed Feb 1, 2024
1 parent a8ef7ed commit 719479a
Show file tree
Hide file tree
Showing 22 changed files with 413 additions and 424 deletions.
1 change: 1 addition & 0 deletions core-bundle/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ security:
request_matcher: contao.routing.frontend_matcher
provider: contao.security.frontend_user_provider
user_checker: contao.security.user_checker
access_denied_handler: contao.security.access_denied_handler
switch_user: false
login_throttling: ~

Expand Down
1 change: 1 addition & 0 deletions core-bundle/config/listener.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ services:
arguments:
- '@contao.framework'
- '@contao.routing.scope_matcher'
- '@contao.routing.page_finder'
- '@contao.routing.content_url_generator'
- '@security.token_storage'
- '%scheb_two_factor.security_tokens%'
Expand Down
9 changes: 8 additions & 1 deletion core-bundle/config/services.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,13 @@ services:
- !tagged_iterator security.voter
- '@contao.security.priority_strategy'

contao.security.access_denied_handler:
class: Contao\CoreBundle\Security\Authentication\AccessDeniedHandler
arguments:
- '@contao.routing.page_finder'
- '@contao.routing.page_registry'
- '@http_kernel'

contao.security.authentication_failure_handler:
class: Contao\CoreBundle\Security\Authentication\AuthenticationFailureHandler
arguments:
Expand Down Expand Up @@ -974,7 +981,7 @@ services:
- '@contao.routing.scope_matcher'
- '@router.default'
- '@uri_signer'
- '@contao.framework'
- '@contao.routing.page_finder'
- '@security.token_storage'
- '@contao.routing.page_registry'
- '@http_kernel'
Expand Down
6 changes: 6 additions & 0 deletions core-bundle/contao/languages/en/default.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,12 @@
<trans-unit id="MSC.emptyField">
<source>Please enter your username and password!</source>
</trans-unit>
<trans-unit id="MSC.reauthenticate">
<source>Please enter your password to verify your identity.</source>
</trans-unit>
<trans-unit id="MSC.verify">
<source>Verify</source>
</trans-unit>
<trans-unit id="MSC.confirmation">
<source>Confirmation</source>
</trans-unit>
Expand Down
15 changes: 15 additions & 0 deletions core-bundle/contao/models/PageModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -475,9 +475,14 @@ public static function findFirstPublishedByTypeAndPid($strType, $intPid, array $
* @param array $arrOptions An optional options array
*
* @return PageModel|null The model or null if there is no 401 page
*
* @deprecated Deprecated since Contao 5.3, to be removed in Contao 6;
* use the contao.routing.page_finder service instead.
*/
public static function find401ByPid($intPid, array $arrOptions=array())
{
trigger_deprecation('contao/core-bundle', '5.3', 'Using "%s()" has been deprecated and will no longer work in Contao 6. Use the "contao.routing.page_finder" service instead.', __METHOD__);

$t = static::$strTable;
$arrColumns = array("$t.pid=? AND $t.type='error_401'");

Expand All @@ -502,9 +507,14 @@ public static function find401ByPid($intPid, array $arrOptions=array())
* @param array $arrOptions An optional options array
*
* @return PageModel|null The model or null if there is no 403 page
*
* @deprecated Deprecated since Contao 5.3, to be removed in Contao 6;
* use the contao.routing.page_finder service instead.
*/
public static function find403ByPid($intPid, array $arrOptions=array())
{
trigger_deprecation('contao/core-bundle', '5.3', 'Using "%s()" has been deprecated and will no longer work in Contao 6. Use the "contao.routing.page_finder" service instead.', __METHOD__);

$t = static::$strTable;
$arrColumns = array("$t.pid=? AND $t.type='error_403'");

Expand All @@ -529,9 +539,14 @@ public static function find403ByPid($intPid, array $arrOptions=array())
* @param array $arrOptions An optional options array
*
* @return PageModel|null The model or null if there is no 404 page
*
* @deprecated Deprecated since Contao 5.3, to be removed in Contao 6;
* use the contao.routing.page_finder service instead.
*/
public static function find404ByPid($intPid, array $arrOptions=array())
{
trigger_deprecation('contao/core-bundle', '5.3', 'Using "%s()" has been deprecated and will no longer work in Contao 6. Use the "contao.routing.page_finder" service instead.', __METHOD__);

$t = static::$strTable;
$arrColumns = array("$t.pid=? AND $t.type='error_404'");

Expand Down
11 changes: 10 additions & 1 deletion core-bundle/contao/modules/ModuleChangePassword.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

namespace Contao;

use Contao\CoreBundle\Exception\AccessDeniedException;

/**
* Front end module "change password".
*/
Expand Down Expand Up @@ -43,12 +45,19 @@ public function generate()
return $objTemplate->parse();
}

$security = $container->get('security.helper');

// Return if there is no logged-in user
if (!$container->get('contao.security.token_checker')->hasFrontendUser())
if (!$security->getUser() instanceof FrontendUser)
{
return '';
}

if (!$security->isGranted('IS_AUTHENTICATED_FULLY'))
{
throw new AccessDeniedException('Full authentication is required to change the password.');
}

return parent::generate();
}

Expand Down
49 changes: 35 additions & 14 deletions core-bundle/contao/modules/ModuleLogin.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ public function generate()
return $objTemplate->parse();
}

$user = System::getContainer()->get('security.helper')->getUser();

if ($user && !$user instanceof FrontendUser)
{
return '';
}

// If the form was submitted and the credentials were wrong, take the target
// path from the submitted data as otherwise it would take the current page
if ($request?->isMethod('POST'))
Expand Down Expand Up @@ -105,20 +112,18 @@ protected function compile()

$container = System::getContainer();
$request = $container->get('request_stack')->getCurrentRequest();
$security = $container->get('security.helper');
$user = $security->getUser();
$exception = null;
$lastUsername = '';
$isRemembered = $security->isGranted('IS_REMEMBERED');
$isTwoFactorInProgress = $security->isGranted('IS_AUTHENTICATED_2FA_IN_PROGRESS');

// Only call the authentication utils if there is an active session to prevent starting an empty session
if ($request?->hasSession() && ($request->hasPreviousSession() || $request->getSession()->isStarted()))
{
$authUtils = $container->get('security.authentication_utils');
$exception = $authUtils->getLastAuthenticationError();
$lastUsername = $authUtils->getLastUsername();
}

$authorizationChecker = $container->get('security.authorization_checker');
// The user can re-authenticate on the error_401 page or on the redirect page of the error_401 page
$canReauthenticate = $objPage->type == 'error_401' || $this->targetPath && $this->targetPath === $request?->query->get('redirect');

if ($authorizationChecker->isGranted('ROLE_MEMBER'))
// Show the logout button if the user is fully authenticated or cannot re-authenticate on the current page
if ($user instanceof FrontendUser && !$isTwoFactorInProgress && (!$isRemembered || !$canReauthenticate))
{
$strRedirect = Environment::get('uri');

Expand All @@ -134,12 +139,10 @@ protected function compile()
$strRedirect = Environment::get('base');
}

$user = FrontendUser::getInstance();

$this->Template->logout = true;
$this->Template->formId = 'tl_logout_' . $this->id;
$this->Template->slabel = StringUtil::specialchars($GLOBALS['TL_LANG']['MSC']['logout']);
$this->Template->loggedInAs = sprintf($GLOBALS['TL_LANG']['MSC']['loggedInAs'], $user->username);
$this->Template->loggedInAs = sprintf($GLOBALS['TL_LANG']['MSC']['loggedInAs'], $user->getUserIdentifier());
$this->Template->action = $container->get('security.logout_url_generator')->getLogoutPath();
$this->Template->targetPath = StringUtil::specialchars($strRedirect);

Expand All @@ -151,6 +154,14 @@ protected function compile()
return;
}

// Only call the authentication utils if there is an active session to prevent starting an empty session
if ($request?->hasSession() && ($request->hasPreviousSession() || $request->getSession()->isStarted()))
{
$authUtils = $container->get('security.authentication_utils');
$exception = $authUtils->getLastAuthenticationError();
$lastUsername = $authUtils->getLastUsername();
}

if ($exception instanceof TooManyLoginAttemptsAuthenticationException)
{
$this->Template->hasError = true;
Expand Down Expand Up @@ -187,7 +198,7 @@ protected function compile()
$this->Template->forceTargetPath = (int) $blnRedirectBack;
$this->Template->targetPath = StringUtil::specialchars(base64_encode($strRedirect));

if ($authorizationChecker->isGranted('IS_AUTHENTICATED_2FA_IN_PROGRESS'))
if ($isTwoFactorInProgress && $request)
{
// Dispatch 2FA form event to prepare 2FA providers
$token = $container->get('security.token_storage')->getToken();
Expand Down Expand Up @@ -216,5 +227,15 @@ protected function compile()
$this->Template->value = Input::encodeInsertTags(StringUtil::specialchars($lastUsername));
$this->Template->autologin = $this->autologin;
$this->Template->autoLabel = $GLOBALS['TL_LANG']['MSC']['autologin'];
$this->Template->remembered = false;

if ($isRemembered)
{
$this->Template->slabel = StringUtil::specialchars($GLOBALS['TL_LANG']['MSC']['verify']);
$this->Template->loggedInAs = sprintf($GLOBALS['TL_LANG']['MSC']['loggedInAs'], $user->getUserIdentifier());
$this->Template->reauthenticate = StringUtil::specialchars($GLOBALS['TL_LANG']['MSC']['reauthenticate']);
$this->Template->value = Input::encodeInsertTags(StringUtil::specialchars($user->getUserIdentifier()));
$this->Template->remembered = true;
}
}
}
18 changes: 16 additions & 2 deletions core-bundle/contao/modules/ModulePersonalData.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

namespace Contao;

use Contao\CoreBundle\Exception\AccessDeniedException;
use Contao\CoreBundle\Exception\ResponseException;

/**
Expand Down Expand Up @@ -49,12 +50,25 @@ public function generate()

$this->editable = StringUtil::deserialize($this->editable);

// Return if there are no editable fields or if there is no logged-in user
if (empty($this->editable) || !\is_array($this->editable) || !$container->get('contao.security.token_checker')->hasFrontendUser())
// Return if there are no editable fields
if (empty($this->editable) || !\is_array($this->editable))
{
return '';
}

$security = $container->get('security.helper');

// Return if there is no logged-in user
if (!$security->getUser() instanceof FrontendUser)
{
return '';
}

if (!$security->isGranted('IS_AUTHENTICATED_FULLY'))
{
throw new AccessDeniedException('Full authentication is required to edit the personal data.');
}

if ($this->memberTpl)
{
$this->strTemplate = $this->memberTpl;
Expand Down
18 changes: 13 additions & 5 deletions core-bundle/contao/templates/modules/mod_login.html5
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
<input type="hidden" name="REQUEST_TOKEN" value="<?= $this->requestToken ?>">
<input type="hidden" name="_target_path" value="<?= $this->targetPath ?>">
<input type="hidden" name="_always_use_target_path" value="<?= $this->forceTargetPath ?>">
<?php if ($this->remembered): ?>
<input type="hidden" name="username" value="<?= $this->value ?>">
<input type="hidden" name="autologin" value="<?= $this->autologin ? '1' : '' ?>">
<?php endif; ?>
<?php if ($this->logout): ?>
<p class="login_info"><?= $this->loggedInAs ?><br><?= $this->lastLogin ?></p>
<?php elseif ($this->twoFactorEnabled): ?>
Expand All @@ -28,15 +32,19 @@
<label for="trusted"><?= $this->trans('MSC.twoFactorTrustDevice') ?></label>
</div>
<?php else: ?>
<div class="widget widget-text">
<label for="username"><?= $this->username ?></label>
<input type="text" name="username" id="username" class="text" value="<?= $this->value ?>" autocapitalize="off" autocomplete="username" required>
</div>
<?php if ($this->remembered): ?>
<p class="login_info"><?= $this->loggedInAs ?><br><?= $this->reauthenticate ?></p>
<?php else: ?>
<div class="widget widget-text">
<label for="username"><?= $this->username ?></label>
<input type="text" name="username" id="username" class="text" value="<?= $this->value ?>" autocapitalize="off" autocomplete="username" required>
</div>
<?php endif; ?>
<div class="widget widget-password">
<label for="password"><?= $this->password ?></label>
<input type="password" name="password" id="password" class="text password" value="" autocomplete="current-password" required>
</div>
<?php if ($this->autologin): ?>
<?php if ($this->autologin && !$this->remembered): ?>
<div class="widget widget-checkbox">
<fieldset class="checkbox_container">
<span><input type="checkbox" name="autologin" id="autologin" value="1" class="checkbox"> <label for="autologin"><?= $this->autoLabel ?></label></span>
Expand Down

0 comments on commit 719479a

Please sign in to comment.