search

markshust / magento2-module-disabletwofactorauth

The DisableTwoFactorAuth module provides the ability to disable two-factor authentication.
MIT License
194 stars 39 forks source link

Disable 2FA for MFTF #5

Closed BorisovskiP closed 3 years ago

BorisovskiP commented 3 years ago

These changes are added to take care of the MFTF tests.

When 2FA is disabled, checking for the configuration in the CLI or in the tests will throw an exception.

This happens when the value is set to 0. When 2FA is enabled, running bin/magento config:show twofactorauth/general/enable will return 1, where as when its disabled it will return an exception which we use as an indicator to know that 2FA is disabled and that we should bypass the filling of the OTP key.

In this case, rewriting the action group and its helper classes is necessary because all helper classes must extend Magento\FunctionalTestingFramework\Helper\Helper

BorisovskiP commented 3 years ago

@markshust What do you think about the proposed changes?

markshust commented 3 years ago

Hi @BorisovskiP, thanks so much for the PR. Can you provide a couple scenarios that I can run to test this out?

BorisovskiP commented 3 years ago

Hi @markshust , you can easly test this with any MFTF that has the action group AdminLoginActionGroup.

If you have 2FA enabled and configured correctly, then all tests should use the predefined credentials and log in to the admin panel without issue.

If you have it disabled, then the tests will fail because in the action group 'AdminLoginActionGroup' (vendor/magento/module-two-factor-auth/Test/Mftf/ActionGroup/AdminLoginActionGroup.xml) there are two helper classes that fill in the credentials in any case.

This means they do not check whether or not 2FA is enabled or disabled. When you run these tests with 2FA disabled, an error with be thrown and the test will fail. In the changes proposed in the PR, we check if that error is thrown, and if it is we know that 2FA is disabled and just return false.

We tested in Magento 2.4.0

BorisovskiP commented 3 years ago

Hi @markshust, maybe some more concrete steps might help you test this better:

Now the test should be executed successfully

markshust commented 3 years ago

@BorisovskiP thank you very much! I apologize in my delay of responses, I mute GitHub notifications now and only check it once or twice a week. The testing criteria you just posted helps a ton. I’ll look into this shortly.

BorisovskiP commented 3 years ago

@markshust Did you find time to test this?

markshust commented 3 years ago

Not yet, will try to over this weekend. Sorry extremely busy time of year.

BorisovskiP commented 3 years ago

@markshust did you have time to test the PR? Could you give me an update on the current status of the PR?

BorisovskiP commented 3 years ago

Hey @markshust, were you able to test the PR?

markshust commented 3 years ago

Hi @BorisovskiP - I apologize for the delay. I haven't had time to test this out, but I don't believe merging any of this code in will cause any issues. The code looks good 👍 I'll trust it's all OK and merge this in.

markshust commented 3 years ago

Released with 1.1.2