Short-term admin accounts #22833

[lfolco]

Description (*)

Provides the ability to set an expiration date on admin user accounts, after which the account will be disabled.

Fixed Issues (if relevant)

1. Short-term admin accounts #22833: Short-term admin accounts

Manual testing scenarios (*)

Manual testing scenario 1

1. Create an admin user
2. Set their expiration date to 5 minutes from now
3. Login as that user
4. Wait 5 minutes
5. Navigate to another page
6. User should be logged out

Manual testing scenario 2

1. Create an admin user
2. Set their expiration date to 5 minutes from now
3. In 5 minutes, try to login as that user
4. Login should fail with message: "The account sign-in was incorrect or your account is disabled temporarily. Please wait and try again later"

Manual testing scenario 3

1. Create an admin user
2. Set their expiration date to 5 minutes from now
3. In 5 minutes, run the Magento cron manually
4. Check the admin user: the account should be disabled and the expiration date should now be empty

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @lfolco. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[sidolov]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @sidolov. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @sidolov, here is your new Magento instance.
Admin access: https://pr-22837.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[phoenix128]

Hello @lfolco ,
are you still on this PR?

[lfolco]

Yes, I'm running into issues with my tests failing here but not locally. Could use some help!

[lfolco]

I know that this PR has issues with the semantic version checker build, but it's also failing on the static tests build and the functional tests build for stuff that I haven't changed.

[kokoc]

This model has no interface. The fields are defined in phpdoc. Suggest to introduce and use the interface for expiration model

[kokoc]

@lfolco

[lfolco]

Do you mean create an interface class or define the methods in UserExpiration?

[kokoc]

Just a regular php interface with implementation inside UserExpiration class.

E.g.

interface UserExpirationInterface {
    public function getExpiresAt(): string, date or timestamp type
}

class UserExpiration implements UserExpirationInterface {
public function getExpiresAt() {
 return 'something';
}


//Some other place

class OtherClass {
    public function save(UserExpirationInterface $expiration) {
         $expiration->getExpiresAt();
    }

}

[lfolco]

How would I use that in \Magento\Security\Observer\AfterAdminUserSave, for example? This relies on the abstract model/resource model:

public function execute(Observer $observer)
    {
        /* @var $user \Magento\User\Model\User */
        $user = $observer->getEvent()->getObject();
        if ($user->getId()) {
            $expiresAt = $user->getExpiresAt();
            /** @var \Magento\Security\Model\UserExpiration $userExpiration */
            $userExpiration = $this->userExpirationFactory->create();
            $this->userExpirationResource->load($userExpiration, $user->getId());

            if (empty($expiresAt)) {
                // delete it if the admin user clears the field
                if ($userExpiration->getId()) {
                    $this->userExpirationResource->delete($userExpiration);
                }
            } else {
                if (!$userExpiration->getId()) {
                    $userExpiration->setId($user->getId());
                }
                $userExpiration->setExpiresAt($expiresAt);
                $this->userExpirationResource->save($userExpiration);
            }
        }
    }

Unless I create a repository as well? Or should I not use the new interface in the scenario where I need to load or save the UserExpiration object directly?

[lfolco]

So I guess I don't need to do this anymore? I was only asking for clarification, I would have preferred to implement it. I never heard back about my question.

[VladimirZaets]

Hi @lfolco. Sorry for the delay and this accident, I start working on this PR and really want to delivery it.
That is ok that you extend \Magento\Framework\Model\AbstractModel, all that new need it create the interface for this class. We should have a strict contract, currently, contract provided just in phpdoc block, it isn't strict and we have just one option to provide strict contract - it's an interface

[VladimirZaets]

@magento run all tests

[VladimirZaets]

@magento run all tests

[VladimirZaets]

@magento run all tests

[magento-engcom-team]

Hi @VladimirZaets, thank you for the review.
ENGCOM-6014 has been created to process this Pull Request

[m2-assistant]

Hi @lfolco, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.