Skip to content

Commit

Permalink
[BUGFIX] Restore and streamline auth log with MFA
Browse files Browse the repository at this point in the history
MFA was preventing the login log entry to be written
as after successful username/password auth MFA was
triggered without logging the first part.
With this patch, the order has been changed to allow
logging of the successful first log in step.
Additionally, the MFA controller is now also using
writeLog to have a complete auth logging including
failed and successful MFA attempts in the backend
log module. MFA related log messages are aligned
to also include the username.

Resolves: #98237
Releases: main, 11.5
Change-Id: I8d4ad0c73f9a8d0e4e2c9f3db85c6280d03c3fca
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/75863
Tested-by: core-ci <typo3@b13.com>
Tested-by: Oliver Bartsch <bo@cedev.de>
Reviewed-by: Oliver Bartsch <bo@cedev.de>
  • Loading branch information
susannemoog authored and o-ba committed Sep 23, 2022
1 parent a9d1d69 commit ac1703a
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 8 deletions.
20 changes: 15 additions & 5 deletions typo3/sysext/backend/Classes/Controller/MfaController.php
Expand Up @@ -33,6 +33,9 @@
use TYPO3\CMS\Core\Http\HtmlResponse;
use TYPO3\CMS\Core\Http\RedirectResponse;
use TYPO3\CMS\Core\Page\PageRenderer;
use TYPO3\CMS\Core\SysLog\Action\Login;
use TYPO3\CMS\Core\SysLog\Error as SystemLogErrorClassification;
use TYPO3\CMS\Core\SysLog\Type as SystemLogType;

/**
* Controller to provide a multi-factor authentication endpoint
Expand Down Expand Up @@ -131,7 +134,7 @@ public function verifyAction(ServerRequestInterface $request, MfaProviderManifes
}
// Call the provider to verify the request
if (!$mfaProvider->verify($request, $propertyManager)) {
$this->log('Multi-factor authentication failed');
$this->log('Multi-factor authentication failed for user ###USERNAME###');
// If failed, initiate a redirect back to the auth view
return new RedirectResponse($this->uriBuilder->buildUriWithRedirect(
'auth_mfa',
Expand All @@ -142,7 +145,7 @@ public function verifyAction(ServerRequestInterface $request, MfaProviderManifes
RouteRedirect::createFromRequest($request)
));
}
$this->log('Multi-factor authentication successful');
$this->log('Multi-factor authentication successful for user ###USERNAME###');
// If verified, store this information in the session
// and initiate a redirect back to the login view.
$this->getBackendUser()->setAndSaveSessionData('mfa', true);
Expand All @@ -159,7 +162,7 @@ public function verifyAction(ServerRequestInterface $request, MfaProviderManifes
*/
public function cancelAction(ServerRequestInterface $request): ResponseInterface
{
$this->log('Multi-factor authentication canceled');
$this->log('Multi-factor authentication canceled for user ###USERNAME###');
$this->getBackendUser()->logoff();
return new RedirectResponse($this->uriBuilder->buildUriWithRedirect('login', [], RouteRedirect::createFromRequest($request)));
}
Expand All @@ -183,10 +186,11 @@ protected function getAlternativeProviders(MfaProviderManifestInterface $mfaProv
protected function log(string $message, array $additionalData = [], ?MfaProviderManifestInterface $mfaProvider = null): void
{
$user = $this->getBackendUser();
$username = $user->user[$user->username_column];
$context = [
'user' => [
'uid' => $user->user[$user->userid_column],
'username' => $user->user[$user->username_column],
'username' => $username,
],
];
if ($mfaProvider !== null) {
Expand All @@ -195,7 +199,13 @@ protected function log(string $message, array $additionalData = [], ?MfaProvider
MfaProviderPropertyManager::create($mfaProvider, $user)
);
}
$this->logger->debug($message, array_replace_recursive($context, $additionalData));
$message = str_replace('###USERNAME###', $username, $message);
$data = array_replace_recursive($context, $additionalData);
$this->logger->debug($message, $data);
if ($user->writeStdLog) {
// Write to sys_log if enabled
$user->writelog(SystemLogType::LOGIN, Login::LOGIN, SystemLogErrorClassification::MESSAGE, 1, $message, $data);
}
}

protected function getMfaProviderFromRequest(ServerRequestInterface $request): ?MfaProviderManifestInterface
Expand Down
Expand Up @@ -636,9 +636,6 @@ public function checkAuthentication(ServerRequestInterface $request = null)
$this->regenerateSessionId();
}

// Check if multi-factor authentication is required
$this->evaluateMfaRequirements();

if ($activeLogin) {
// User logged in - write that to the log!
if ($this->writeStdLog) {
Expand All @@ -654,6 +651,8 @@ public function checkAuthentication(ServerRequestInterface $request = null)
'ip' => GeneralUtility::getIndpEnv('REMOTE_ADDR'),
]);
}
// Check if multi-factor authentication is required
$this->evaluateMfaRequirements();
} else {
// Mark the current login attempt as failed
if (empty($tempuserArr) && $activeLogin) {
Expand Down

0 comments on commit ac1703a

Please sign in to comment.