markshust / magento2-module-disabletwofactorauth

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

Added new functionality which controlling behavior 2FA for admins #12

Closed artemii-karkusha closed 2 years ago

artemii-karkusha commented 2 years ago

Added

Change 2FA for user (admin)

Enables the bypass of 2FA for admin access. We have to disable 2FA for this user.

If you want disable 2FA for user

Visit Admin > System > Permissions > All Users > Pick user -> 2FA and set Enable 2FA to No. CLI: bin/magento admin:user:2fa:disable <username>

If you want enable 2FA for user

Visit Admin > System > Permissions > All Users > Pick user -> 2FA and set Enable 2FA to Yes. CLI: bin/magento admin:user:2fa:enable <username>

NOTE:

If 2FA is enabled for user, and it is disabled for website, always returns true, so all requests bypass 2FA.
If 2FA is disabled for user, and it is enabled for website, always returns true, so all requests bypass 2FA.
If 2FA is disabled for user, and it is disabled for website, always returns true, so all requests bypass 2FA.
If 2FA is enabled for user, and it is enabled for website, returns the original result.
markshust commented 2 years ago

Appreciate the huge PR 😅

That said, I'm not sure if it's a good idea to merge this in? This module was meant to be used in development environments, and I think this PR implies that it can/should used within staging & production environments.

This would sort of open a loophole in that a single account can be vulnerable to attacks which could have been prevented if it were enabled site-wide? This question is probably splitting hairs, but I still fail to see why we would need to use this in development. Please argue to prove me wrong.

Cheers, Mark

drpayyne commented 2 years ago

Great idea and PR - but out of curiosity, why would some users require 2FA when others don't? Is this meant for production/staging as @markshust mentioned? If yes, I think it's definitely a vulnerability.

artemii-karkusha commented 2 years ago

I want to disable 2FA for some users, but not all. This functionality intended more for staging than for production. I think when we disable 2fA for some users, that be more security than we disable 2FA for store. 2FA disabled for store create more hole in our security than 2Fa disable for several users which are actively working on staging.

artemii-karkusha commented 2 years ago

@markshust What do you think?

drpayyne commented 2 years ago

I understand your argument @artemii-karkusha, interesting thought indeed.

But here's what I think - A store owner wouldn't want all their logins without 2FA. So they would avoid installing this module on prod/staging as it's either full 2FA or no 2FA - and anyone would prefer full 2FA over no 2FA. But if this functionality is added, store owners can install this module on prod/staging and selectively apply 2FA as per their convenience, which I feel would be a security weakness.

I agree that anyone installing this feature is made aware of this weakness and isn't any hidden vulnerability. I'm not saying this great feature shouldn't be accepted, but these are my thoughts. But what do you think?

artemii-karkusha commented 2 years ago

First of all this module created for up convenience. We use that module for our convenience and even without this functionality we have weakness in our security. We can reduce count weakness in our security using this functionality.

artemii-karkusha commented 2 years ago

Hi @markshust, could you please review this? Thank you!

markshust commented 2 years ago

I'm thinking this is too much code to merge, for a feature that should probably not be included in the first place 🤷.

I'll leave this PR open indefinitely to hear what others things.

markshust commented 2 years ago

I'm going to close/deny this PR, just on the premise that I do not urge anyone to disable 2FA on prod for any reason. This goes against the warnings/notices/etc. I have every place about not disabling 2FA on production, as well as my general stance on security about this. That said, if you make a derivative work/fork of this module, please let me know and I can link to the module under an "alternatives" section in the README.