Skip to content

Commit 89b8512

Browse files
lfolcolfolco
committed
fixes from code review (#22833: Short-term admin accounts)
1 parent 171c4c9 commit 89b8512

File tree

10 files changed

+63
-45
lines changed

10 files changed

+63
-45
lines changed

‎app/code/Magento/Security/Model/Plugin/AuthSession.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public function __construct(
7272
public function aroundProlong(Session $session, \Closure $proceed)
7373
{
7474
if (!$this->sessionsManager->getCurrentSession()->isLoggedInStatus() ||
75-
$this->userExpirationManager->userIsExpired($session->getUser())) {
75+
$this->userExpirationManager->isUserExpired($session->getUser()->getId())) {
7676
$session->destroy();
7777
$this->addUserLogoutNotification();
7878
return null;

‎app/code/Magento/Security/Model/ResourceModel/UserExpiration.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class UserExpiration extends \Magento\Framework\Model\ResourceModel\Db\AbstractD
3535
public function __construct(
3636
\Magento\Framework\Model\ResourceModel\Db\Context $context,
3737
\Magento\Framework\Stdlib\DateTime\TimezoneInterface $timezone,
38-
$connectionName = null
38+
?string $connectionName = null
3939
) {
4040
parent::__construct($context, $connectionName);
4141
$this->timezone = $timezone;

‎app/code/Magento/Security/Model/ResourceModel/UserExpiration/Collection.php

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,21 +33,18 @@ protected function _construct()
3333
/**
3434
* Filter for expired, active users.
3535
*
36-
* @param string $now
3736
* @return $this
3837
*/
39-
public function addActiveExpiredUsersFilter($now = null): Collection
38+
public function addActiveExpiredUsersFilter(): Collection
4039
{
41-
if ($now === null) {
42-
$now = new \DateTime();
43-
$now->format('Y-m-d H:i:s');
44-
}
40+
$currentTime = new \DateTime();
41+
$currentTime->format('Y-m-d H:i:s');
4542
$this->getSelect()->joinLeft(
4643
['user' => $this->getTable('admin_user')],
4744
'main_table.user_id = user.user_id',
4845
['is_active']
4946
);
50-
$this->addFieldToFilter('expires_at', ['lt' => $now])
47+
$this->addFieldToFilter('expires_at', ['lt' => $currentTime])
5148
->addFieldToFilter('user.is_active', 1);
5249

5350
return $this;
@@ -59,7 +56,7 @@ public function addActiveExpiredUsersFilter($now = null): Collection
5956
* @param int[] $userIds
6057
* @return Collection
6158
*/
62-
public function addUserIdsFilter($userIds = []): Collection
59+
public function addUserIdsFilter(array $userIds = []): Collection
6360
{
6461
return $this->addFieldToFilter('main_table.user_id', ['in' => $userIds]);
6562
}

‎app/code/Magento/Security/Model/UserExpiration.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
/**
1111
* Admin User Expiration model.
12+
* @method string getUserId()
13+
* @method \Magento\Security\Model\UserExpiration setUserId($userId)
1214
* @method string getExpiresAt()
1315
* @method \Magento\Security\Model\UserExpiration setExpiresAt($value)
1416
*/

‎app/code/Magento/Security/Model/UserExpirationManager.php

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
use Magento\Security\Model\ResourceModel\UserExpiration\Collection as ExpiredUsersCollection;
1111

1212
/**
13-
* Class to handle admin user expirations.
13+
* Class to handle admin user expirations. Temporary admin users can be created with an expiration
14+
* date that, once past, will not allow them to login to the admin. A cron is run to periodically check for expired
15+
* users and, if found, will deactivate them.
1416
* @SuppressWarnings(PHPMD.CookieAndSessionMisuse)
1517
*/
1618
class UserExpirationManager
@@ -98,26 +100,26 @@ public function deactivateExpiredUsers(?array $userIds = null): void
98100

99101
// delete expired records
100102
$expiredRecordIds = $expiredRecords->getAllIds();
101-
$expiredRecords->walk('delete');
102103

103104
// set user is_active to 0
104105
$users = $this->userCollectionFactory->create()
105106
->addFieldToFilter('main_table.user_id', ['in' => $expiredRecordIds]);
106107
$users->setDataToAll('is_active', 0)->save();
108+
$expiredRecords->walk('delete');
107109
}
108110

109111
/**
110112
* Check if the given user is expired.
111113
*
112-
* @param \Magento\User\Model\User $user
114+
* @param string $userId
113115
* @return bool
114116
*/
115-
public function userIsExpired(\Magento\User\Model\User $user): bool
117+
public function isUserExpired(string $userId): bool
116118
{
117119
$isExpired = false;
118120
/** @var \Magento\Security\Model\UserExpiration $expiredRecord */
119121
$expiredRecord = $this->userExpirationCollectionFactory->create()
120-
->addExpiredRecordsForUserFilter($user->getId())
122+
->addExpiredRecordsForUserFilter($userId)
121123
->getFirstItem();
122124
if ($expiredRecord && $expiredRecord->getId()) {
123125
$expiresAt = $this->dateTime->timestamp($expiredRecord->getExpiresAt());

‎app/code/Magento/Security/Observer/AdminUserAuthenticateBefore.php

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,22 +25,22 @@ class AdminUserAuthenticateBefore implements ObserverInterface
2525
private $userExpirationManager;
2626

2727
/**
28-
* @var \Magento\User\Model\User
28+
* @var \Magento\User\Model\UserFactory
2929
*/
30-
private $user;
30+
private $userFactory;
3131

3232
/**
3333
* AdminUserAuthenticateBefore constructor.
3434
*
3535
* @param UserExpirationManager $userExpirationManager
36-
* @param \Magento\User\Model\User $user
36+
* @param \Magento\User\Model\UserFactory $userFactory
3737
*/
3838
public function __construct(
3939
\Magento\Security\Model\UserExpirationManager $userExpirationManager,
40-
\Magento\User\Model\User $user
40+
\Magento\User\Model\UserFactory $userFactory
4141
) {
4242
$this->userExpirationManager = $userExpirationManager;
43-
$this->user = $user;
43+
$this->userFactory = $userFactory;
4444
}
4545

4646
/**
@@ -53,10 +53,11 @@ public function __construct(
5353
public function execute(Observer $observer)
5454
{
5555
$username = $observer->getEvent()->getUsername();
56+
$user = $this->userFactory->create();
5657
/** @var \Magento\User\Model\User $user */
57-
$user = $this->user->loadByUsername($username);
58+
$user->loadByUsername($username);
5859

59-
if ($user->getId() && $this->userExpirationManager->userIsExpired($user)) {
60+
if ($user->getId() && $this->userExpirationManager->isUserExpired($user->getId())) {
6061
$this->userExpirationManager->deactivateExpiredUsers([$user->getId()]);
6162
throw new AuthenticationException(
6263
__(

‎app/code/Magento/Security/Test/Unit/Model/Plugin/AuthSessionTest.php

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public function setUp()
7979

8080
$this->userExpirationManagerMock = $this->createPartialMock(
8181
\Magento\Security\Model\UserExpirationManager::class,
82-
['userIsExpired']
82+
['isUserExpired']
8383
);
8484

8585
$this->userMock = $this->createMock(\Magento\User\Model\User::class);
@@ -183,6 +183,7 @@ public function testAroundProlongSessionIsActiveUserIsExpired()
183183
return $result;
184184
};
185185

186+
$adminUserId = '12345';
186187
$this->currentSessionMock->expects($this->once())
187188
->method('isLoggedInStatus')
188189
->willReturn(true);
@@ -191,9 +192,13 @@ public function testAroundProlongSessionIsActiveUserIsExpired()
191192
->method('getUser')
192193
->willReturn($this->userMock);
193194

195+
$this->userMock->expects($this->once())
196+
->method('getId')
197+
->willReturn($adminUserId);
198+
194199
$this->userExpirationManagerMock->expects($this->once())
195-
->method('userIsExpired')
196-
->with($this->userMock)
200+
->method('isUserExpired')
201+
->with($adminUserId)
197202
->willReturn(true);
198203

199204
$this->authSessionMock->expects($this->once())
@@ -225,6 +230,7 @@ public function testAroundProlongSessionIsActive()
225230
return $result;
226231
};
227232

233+
$adminUserId = '12345';
228234
$this->currentSessionMock->expects($this->any())
229235
->method('isLoggedInStatus')
230236
->willReturn(true);
@@ -233,9 +239,13 @@ public function testAroundProlongSessionIsActive()
233239
->method('getUser')
234240
->willReturn($this->userMock);
235241

242+
$this->userMock->expects($this->once())
243+
->method('getId')
244+
->willReturn($adminUserId);
245+
236246
$this->userExpirationManagerMock->expects($this->once())
237-
->method('userIsExpired')
238-
->with($this->userMock)
247+
->method('isUserExpired')
248+
->with($adminUserId)
239249
->willReturn(false);
240250

241251
$this->adminSessionsManagerMock->expects($this->any())

‎app/code/Magento/Security/Test/Unit/Observer/BeforeAdminUserAuthenticateTest.php renamed to ‎app/code/Magento/Security/Test/Unit/Observer/AdminUserAuthenticateBeforeTest.php

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
*
1313
* @package Magento\Security\Test\Unit\Observer
1414
*/
15-
class BeforeAdminUserAuthenticateTest extends \PHPUnit\Framework\TestCase
15+
class AdminUserAuthenticateBeforeTest extends \PHPUnit\Framework\TestCase
1616
{
1717
/**
1818
* @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager
@@ -29,6 +29,11 @@ class BeforeAdminUserAuthenticateTest extends \PHPUnit\Framework\TestCase
2929
*/
3030
private $userMock;
3131

32+
/**
33+
* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\User\Model\UserFactory
34+
*/
35+
private $userFactoryMock;
36+
3237
/**
3338
* @var \Magento\Security\Observer\AdminUserAuthenticateBefore
3439
*/
@@ -60,14 +65,15 @@ protected function setUp()
6065

6166
$this->userExpirationManagerMock = $this->createPartialMock(
6267
\Magento\Security\Model\UserExpirationManager::class,
63-
['userIsExpired', 'deactivateExpiredUsers']
68+
['isUserExpired', 'deactivateExpiredUsers']
6469
);
70+
$this->userFactoryMock = $this->createPartialMock(\Magento\User\Model\UserFactory::class, ['create']);
6571
$this->userMock = $this->createPartialMock(\Magento\User\Model\User::class, ['loadByUsername', 'getId']);
6672
$this->observer = $this->objectManager->getObject(
6773
\Magento\Security\Observer\AdminUserAuthenticateBefore::class,
6874
[
6975
'userExpirationManager' => $this->userExpirationManagerMock,
70-
'user' => $this->userMock,
76+
'userFactory' => $this->userFactoryMock,
7177
]
7278
);
7379
$this->eventObserverMock = $this->createPartialMock(\Magento\Framework\Event\Observer::class, ['getEvent']);
@@ -85,17 +91,18 @@ protected function setUp()
8591
*/
8692
public function testWithExpiredUser()
8793
{
88-
$adminUserId = 123;
94+
$adminUserId = '123';
8995
$username = 'testuser';
9096
$this->eventObserverMock->expects(static::once())->method('getEvent')->willReturn($this->eventMock);
9197
$this->eventMock->expects(static::once())->method('getUsername')->willReturn($username);
92-
$this->userMock->expects(static::once())->method('loadByUsername')->willReturn($this->userMock);
98+
$this->userFactoryMock->expects(static::once())->method('create')->willReturn($this->userMock);
99+
$this->userMock->expects(static::once())->method('loadByUsername')->willReturnSelf();
93100

94101
$this->userExpirationManagerMock->expects(static::once())
95-
->method('userIsExpired')
96-
->with($this->userMock)
102+
->method('isUserExpired')
103+
->with($adminUserId)
97104
->willReturn(true);
98-
$this->userMock->expects(static::exactly(2))->method('getId')->willReturn($adminUserId);
105+
$this->userMock->expects(static::exactly(3))->method('getId')->willReturn($adminUserId);
99106
$this->userExpirationManagerMock->expects(static::once())
100107
->method('deactivateExpiredUsers')
101108
->with([$adminUserId])
@@ -105,15 +112,16 @@ public function testWithExpiredUser()
105112

106113
public function testWithNonExpiredUser()
107114
{
108-
$adminUserId = 123;
115+
$adminUserId = '123';
109116
$username = 'testuser';
110117
$this->eventObserverMock->expects(static::once())->method('getEvent')->willReturn($this->eventMock);
111118
$this->eventMock->expects(static::once())->method('getUsername')->willReturn($username);
112-
$this->userMock->expects(static::once())->method('loadByUsername')->willReturn($this->userMock);
113-
$this->userMock->expects(static::once())->method('getId')->willReturn($adminUserId);
119+
$this->userFactoryMock->expects(static::once())->method('create')->willReturn($this->userMock);
120+
$this->userMock->expects(static::once())->method('loadByUsername')->willReturnSelf();
121+
$this->userMock->expects(static::exactly(2))->method('getId')->willReturn($adminUserId);
114122
$this->userExpirationManagerMock->expects(static::once())
115-
->method('userIsExpired')
116-
->with($this->userMock)
123+
->method('isUserExpired')
124+
->with($adminUserId)
117125
->willReturn(false);
118126
$this->observer->execute($this->eventObserverMock);
119127
}

‎app/code/Magento/User/etc/db_schema_whitelist.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,4 @@
4242
"ADMIN_PASSWORDS_USER_ID_ADMIN_USER_USER_ID": true
4343
}
4444
}
45-
}
45+
}

‎dev/tests/integration/testsuite/Magento/Security/Model/UserExpirationManagerTest.php

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
/**
1111
* Tests for \Magento\Security\Model\UserExpirationManager
12-
*
12+
* @magentoAppArea adminhtml
1313
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
1414
*/
1515
class UserExpirationManagerTest extends \PHPUnit\Framework\TestCase
@@ -42,8 +42,6 @@ class UserExpirationManagerTest extends \PHPUnit\Framework\TestCase
4242
protected function setUp()
4343
{
4444
$this->objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
45-
$this->objectManager->get(\Magento\Framework\Config\ScopeInterface::class)
46-
->setCurrentScope(\Magento\Backend\App\Area\FrontNameResolver::AREA_CODE);
4745
$this->auth = $this->objectManager->create(\Magento\Backend\Model\Auth::class);
4846
$this->authSession = $this->objectManager->create(\Magento\Backend\Model\Auth\Session::class);
4947
$this->adminSessionInfo = $this->objectManager->create(\Magento\Security\Model\AdminSessionInfo::class);
@@ -59,7 +57,7 @@ public function testUserIsExpired()
5957
{
6058
$adminUserNameFromFixture = 'adminUserExpired';
6159
$user = $this->loadUserByUsername($adminUserNameFromFixture);
62-
static::assertTrue($this->userExpirationManager->userIsExpired($user));
60+
static::assertTrue($this->userExpirationManager->isUserExpired($user->getId()));
6361
}
6462

6563
/**

0 commit comments

Comments
 (0)