Skip to content

Commit 2ff42cb

Browse files
JamesAliasJamesAlias
committed
TASK: Fix regression regarding deletion of second factor and new enforcment conditions
1 parent 47ef940 commit 2ff42cb

File tree

4 files changed

+98
-54
lines changed

4 files changed

+98
-54
lines changed

‎Classes/Controller/BackendController.php

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Sandstorm\NeosTwoFactorAuthentication\Domain\Model\SecondFactor;
1919
use Sandstorm\NeosTwoFactorAuthentication\Domain\Model\Dto\SecondFactorDto;
2020
use Sandstorm\NeosTwoFactorAuthentication\Domain\Repository\SecondFactorRepository;
21+
use Sandstorm\NeosTwoFactorAuthentication\Service\SecondFactorService;
2122
use Sandstorm\NeosTwoFactorAuthentication\Service\SecondFactorSessionStorageService;
2223
use Sandstorm\NeosTwoFactorAuthentication\Service\TOTPService;
2324

@@ -71,10 +72,10 @@ class BackendController extends AbstractModuleController
7172
protected $defaultViewObjectName = FusionView::class;
7273

7374
/**
74-
* @Flow\InjectConfiguration(path="enforceTwoFactorAuthentication")
75-
* @var bool
75+
* @Flow\Inject
76+
* @var SecondFactorService
7677
*/
77-
protected $enforceTwoFactorAuthentication;
78+
protected $secondFactorService;
7879

7980
/**
8081
* used to list all second factors of the current user
@@ -177,14 +178,11 @@ public function deleteAction(SecondFactor $secondFactor): void
177178
{
178179
$account = $this->securityContext->getAccount();
179180

180-
if (
181-
$this->securityContext->hasRole('Neos.Neos:Administrator')
182-
|| $secondFactor->getAccount() === $account
183-
) {
184-
if (
185-
$this->enforceTwoFactorAuthentication
186-
&& count($this->secondFactorRepository->findByAccount($account)) <= 1
187-
) {
181+
$isAdministrator = $this->securityContext->hasRole('Neos.Neos:Administrator');
182+
$isOwner = $secondFactor->getAccount() === $account;
183+
184+
if ($isAdministrator || $isOwner) {
185+
if (!$this->secondFactorService->canOneSecondFactorBeDeletedForAccount($account)) {
188186
$this->addFlashMessage(
189187
$this->translator->translateById(
190188
'module.index.delete.flashMessage.cannotRemoveLastSecondFactor',

‎Classes/Domain/Repository/SecondFactorRepository.php

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,6 @@
1616
*/
1717
class SecondFactorRepository extends Repository
1818
{
19-
public function isEnabledForAccount(Account $account): bool
20-
{
21-
$factors = $this->findByAccount($account);
22-
return count($factors) > 0;
23-
}
24-
2519
/**
2620
* @throws IllegalObjectTypeException
2721
*/

‎Classes/Http/Middleware/SecondFactorMiddleware.php

Lines changed: 10 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
use Psr\Http\Server\RequestHandlerInterface;
1616
use Psr\Log\LoggerInterface;
1717
use Sandstorm\NeosTwoFactorAuthentication\Domain\AuthenticationStatus;
18-
use Sandstorm\NeosTwoFactorAuthentication\Domain\Repository\SecondFactorRepository;
18+
use Sandstorm\NeosTwoFactorAuthentication\Service\SecondFactorService;
1919
use Sandstorm\NeosTwoFactorAuthentication\Service\SecondFactorSessionStorageService;
2020

2121
class SecondFactorMiddleware implements MiddlewareInterface
@@ -30,12 +30,6 @@ class SecondFactorMiddleware implements MiddlewareInterface
3030
*/
3131
protected $securityContext;
3232

33-
/**
34-
* @Flow\Inject
35-
* @var SecondFactorRepository
36-
*/
37-
protected $secondFactorRepository;
38-
3933
/**
4034
* @Flow\Inject
4135
* @var ActionRequestFactory
@@ -55,23 +49,10 @@ class SecondFactorMiddleware implements MiddlewareInterface
5549
protected $secondFactorSessionStorageService;
5650

5751
/**
58-
* @Flow\InjectConfiguration(path="enforceTwoFactorAuthentication")
59-
* @var bool
60-
*/
61-
protected $enforceTwoFactorAuthentication;
62-
63-
/**
64-
* @Flow\InjectConfiguration(path="enforce2FAForAuthenticationProviders")
65-
* @var array
66-
*/
67-
protected $enforce2FAForAuthenticationProviders;
68-
69-
/**
70-
* @Flow\InjectConfiguration(path="enforce2FAForRoles")
71-
* @var array
52+
* @Flow\Inject
53+
* @var SecondFactorService
7254
*/
73-
protected $enforce2FAForRoles;
74-
55+
protected SecondFactorService $secondFactorService;
7556

7657
/**
7758
* This middleware checks if the user is authenticated with a second factor "if necessary".
@@ -167,13 +148,11 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
167148

168149
$account = $this->securityContext->getAccount();
169150

151+
$isEnabledForAccount = $this->secondFactorService->isSecondFactorEnabledForAccount($account);
152+
$isEnforcedForAccount = $this->secondFactorService->isSecondFactorEnforcedForAccount($account);
153+
170154
// 4. Skip, if second factor is not set up for account and not enforced via settings.
171-
if (
172-
!$this->secondFactorRepository->isEnabledForAccount($account)
173-
&& !$this->enforceTwoFactorAuthentication
174-
&& !count(array_intersect(array_map(fn($item) => $item->getIdentifier(),$account->getRoles()),$this->enforce2FAForRoles))
175-
&& !in_array($account->getAuthenticationProviderName(),$this->enforce2FAForAuthenticationProviders)
176-
) {
155+
if (!$isEnabledForAccount && !$isEnforcedForAccount) {
177156
$this->log('Second factor not enabled for account and not enforced by system, skipping second factor.');
178157

179158
return $handler->handle($request);
@@ -193,7 +172,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
193172
// 6. Redirect to 2FA login, if second factor is set up for account but not authenticated.
194173
// Skip, if already on 2FA login route.
195174
if (
196-
$this->secondFactorRepository->isEnabledForAccount($account)
175+
$isEnabledForAccount
197176
&& $authenticationStatus === AuthenticationStatus::AUTHENTICATION_NEEDED
198177
) {
199178
// WHY: We use the request URI as state here to keep the middleware from entering a redirect loop.
@@ -213,13 +192,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
213192

214193
// 7. Redirect to 2FA setup, if second factor is not set up for account but is enforced by system.
215194
// Skip, if already on 2FA setup route.
216-
if (
217-
($this->enforceTwoFactorAuthentication
218-
|| count(array_intersect(array_map(fn($item) => $item->getIdentifier(),$account->getRoles()),$this->enforce2FAForRoles))
219-
|| in_array($account->getAuthenticationProviderName(),$this->enforce2FAForAuthenticationProviders)
220-
)
221-
&& !$this->secondFactorRepository->isEnabledForAccount($account)
222-
) {
195+
if ($isEnforcedForAccount && !$isEnabledForAccount) {
223196
// WHY: We use the request URI as state here to keep the middleware from entering a redirect loop.
224197
$isSettingUp2FA = str_ends_with($request->getUri()->getPath(), self::SECOND_FACTOR_SETUP_URI);
225198
if ($isSettingUp2FA) {

‎Classes/Service/SecondFactorService.php

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
<?php
2+
3+
namespace Sandstorm\NeosTwoFactorAuthentication\Service;
4+
5+
use Neos\Flow\Annotations as Flow;
6+
use Neos\Flow\Security\Account;
7+
use Sandstorm\NeosTwoFactorAuthentication\Domain\Repository\SecondFactorRepository;
8+
9+
class SecondFactorService
10+
{
11+
/**
12+
* @Flow\InjectConfiguration(path="enforceTwoFactorAuthentication")
13+
* @var bool
14+
*/
15+
protected $enforceTwoFactorAuthentication;
16+
17+
/**
18+
* @Flow\InjectConfiguration(path="enforce2FAForAuthenticationProviders")
19+
* @var array
20+
*/
21+
protected $enforce2FAForAuthenticationProviders;
22+
23+
/**
24+
* @Flow\InjectConfiguration(path="enforce2FAForRoles")
25+
* @var array
26+
*/
27+
protected $enforce2FAForRoles;
28+
29+
/**
30+
* @Flow\Inject
31+
* @var SecondFactorRepository
32+
*/
33+
protected $secondFactorRepository;
34+
35+
/**
36+
* Check if the second factor is enforced for the given account.
37+
*
38+
* The second factor is enforced if:
39+
* - it is enforced for all accounts or
40+
* - it is enforced for a role of the account or
41+
* - it is enforced for the authentication provider of the account
42+
*/
43+
public function isSecondFactorEnforcedForAccount(Account $account): bool
44+
{
45+
$isEnforcedForAll = $this->enforceTwoFactorAuthentication;
46+
$isEnforcedForRoles = count(array_intersect(
47+
array_map(fn($item) => $item->getIdentifier(), $account->getRoles()),
48+
$this->enforce2FAForRoles
49+
));
50+
$isEnforcedForAuthenticationProviders = in_array(
51+
$account->getAuthenticationProviderName(),
52+
$this->enforce2FAForAuthenticationProviders
53+
);
54+
55+
return $isEnforcedForAll || $isEnforcedForRoles || $isEnforcedForAuthenticationProviders;
56+
}
57+
58+
/**
59+
* Check if the account has setup at least 1 second factor.
60+
*/
61+
public function isSecondFactorEnabledForAccount(Account $account): bool
62+
{
63+
$factors = $this->secondFactorRepository->findByAccount($account);
64+
return count($factors) > 0;
65+
}
66+
67+
/**
68+
* Check if the account can delete 1 second factor.
69+
*
70+
* Second factor can only be deleted if it is not enforced for the account or if the account has multiple factors.
71+
*/
72+
public function canOneSecondFactorBeDeletedForAccount(Account $account): bool
73+
{
74+
$isEnforcedForAccount = $this->isSecondFactorEnforcedForAccount($account);
75+
$hasMultipleFactors = count($this->secondFactorRepository->findByAccount($account)) > 1;
76+
77+
return !$isEnforcedForAccount || $hasMultipleFactors;
78+
}
79+
}

0 commit comments

Comments
 (0)