Remove UID 1 enforcement of TFA and allow hooks to require

Hi, thanks for raising this issue. It's true that because the 'require TFA' permission defaults to enabled for the administrator role that once TFA is enabled it can lock out the UID 1 user.

One proposal might be to expose a UID1 TFA bypass option on the TFA admin settings page. Something like "Allow UID 1 to skip TFA login requirement when they haven't set up TFA".

What do think of an option like that?

@morseCode I've done similar code before with TFA so there's definitely need for something like that. Are you using that with TFA Basic module? If so perhaps that's a worthy feature to add to it. Otherwise it could be a separate module.

I've just been testing the TFA module and got the same issue with user 1.

I was thinking the permission "Require TFA" should instead be "Bypass TFA if not configured"

There should then also be a module setting called 'Require TFA' which redirects a user to the user/security page after they login until they have setup their TFA unless they have the permission "Bypass TFA if not configured".

This means User 1 always has to opt into TFA.

Please find attached a patch which does the following:

1. Gives special consideration to user 1 so it is never locked out by the 'require tfa' permission. User 1 must opt into TFA by completing the TFA setup.
2. Adds a workflow such that once a user logs in they must then complete TFA setup using the code @morseCode provided as a base. This is a less aggressive option than 'require tfa' which prevents login if TFA is not setup.
3. Adds a new permission 'bypass tfa setup' to prevent the action introduced by 2 above on a per role basis

This solves my particular workflow and I hope it is of interest to others.

Also, I shouldn't the permission 'setup own tfa' be provided by the TFA module rather than tfa_basic?

Please find a better patch attached with some typos corrected.

Patch #9 works perfectly for my workflow too.

Thanks !

This fix the problem for me. Made some doc changes.

+++ b/tfa.module
@@ -380,3 +390,60 @@ function tfa_login_hash($account) {
+  $setting_pages = variable_get('tfa_unforced_pages', TFA_UNFORCED_PAGES);

I think we should add some kind of setting to tfa_admin_settings page for that.

@jibran
Great !!!... it worked for you too.
One thing, your patch file seems to be empty.

Thanks for pointing that.

At least interdiff was there :P

Patch in #14 seems to force all users to use TFA even if they do not have the require TFA permission. Updated patch to fix that attached.

In the long term is a permission really the right place to be defining who is required to use TFA? It is not really a permission and setting it in another manner would clear up this whole ugly deal with UID 1.

I could be wrong but I believe the tests are only failing because the user in the test does not have the require tfa permission set. Adding that to patch.

Thanks all for the code and reviews here, but I think this needs to be split up. The User 1 login without TFA setup permission change is right for TFA module but the user/security path is defined by TFA Basic module so needs to be there.

@coltrane: That makes sense to me. I would like to get this patched and working properly so that we don't have to worry about our site breaking when upgrading to future versions of your modules.

What course would you recommend in the short term? Sticking with the logic from this patch of completely excluding UID 1 (maybe this should be a setting?) and then go with the same type of bypass permission in this patch but moved to the appropriate place in the TFA Basic module?

Long term what do you think about the idea of moving require TFA out of permissions entirely and avoiding the whole UID 1 issue? I think this might make sense as you could open up the ability to require TFA for a single user, with a hook, etc. "require TFA" doesn't really feel like a permission to me.

Thanks!
kevin

Fair points @koshea2 and I think you're right, it is making more sense to remove the 'require tfa' permission.

Is anyone opposed to this plan?

Since TFA module does not provide any TFA plugins and the require tfa permission is immediately applied to UID 1 the permission has the unintended affect of blocking UID 1 from logging in without providing any way to do so. Instead, required TFA-setup should be provided by plugin modules.

• remove the 'require tfa' permission from tfa.module
• alter the permission check in tfa_login_submit() and tfa_user_login() to call a hook for whether login is blocked
• update tfa.test to remove requirement check
• document this new hook in the readme

Let me know any thoughts or I'll update the summary here and create a new TFA Basic issue to match the latest functionality proposed in #17.

+1 to the plan in #21.

I am also happy with approach suggested in #21

+1 to #21

Thanks for the feedback! Updating summary with steps needed for TFA module and created https://www.drupal.org/node/2457823 to handle requirement for TFA Basic.

interdiff please.

Attached patch invokes new hook_tfa_require() and if any implementations return TRUE will disallow login. Implementers can now set the visible message.

Updated patch to simplify code flow in login handlers and minor updates to documentation. Moves tfa_login_complete() code within tfa_login_allowed().

Patch in 28 missed a few other minor doc changes.

And minor update to change hook name to 'tfa_ready_require' to be more explicit on what's being required. Patches 27-29 also removed a test on plugin readiness so this restores that.

I'm planning to commit this soon so any reviews/testing is greatly appreciated in case there are needed changes.

Just a re-roll for clean apply.

Updating title since redirect isn't handled here.

Committed #31.