Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Remove TFA-wide "require TFA" and change the "skip TFA" permission to be a "require TFA".
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 2239949-tfa-required-14.patch | 623 bytes | zopa |
| #9 | 2239949-tfa-required-tfa-3.patch | 6 KB | greggles |
| #2 | 2239949-tfa-required-tfa-2.patch | 5.91 KB | coltrane |
| #1 | 2239949_require_tfa_permission.patch | 4.61 KB | greggles |
Comments
Comment #1
gregglesSomething like this?
Comment #2
coltraneSwapping 'skip' to 'require' also needed to change logic of when the TFA process should start. Tests pass with this.
Comment #3
gregglesThere's one hunk of this that no longer applies after #2241729: Plugin flood tests always fail but it's OK because it's not relevant/necessary.
The automated tests pass. I'll try to do some manual testing shortly as well.
Comment #4
gregglesSo, I tested 2 scenarios:
Scenario 1:
1. Enable module + totp + trusted device
2. Require tfa for a role
3. Configure tfa for a user
4. Log out and log in - everything works fine
5. Disable tfa for that user
6. Log out and log in - it will present the verification code form, which kind of makes sense. If you enter nonsense it won't let you in. If you enter the current TOTP value from step 3 (which was disabled in step 5) then it still lets you in. If I look in tfa_totp_seed then I see the seed is still present.
Scenario 2:
1. Same as above up through step 5, except I went in the DB and
delete from tfa_totp_seed2. When I try to log in as uid 2 with a role that I get a proper message "Login disallowed. You are required to setup two-factor authentication. Please contact a site administrator."
I think there's opportunity for usability improvements and a bug fix:
1. If TFA is required for your role then it should advise you more before you can log out (e.g. a dsm on every page).
2. The disable in step 5 should actually delete the value from tfa_totp_seed
If you agree I can work on those.
Comment #6
coltraneYes, I agree on both your points. Thank you Greg.
Comment #8
gregglesThe bugs from #4 seemed more relevant in tfa_basic.module. I split them off to #2243655: Warn if removing totp and tfa is required, removing doesn't actually remove.
So, I think this is now back to needs review.
Comment #9
gregglesA reroll for testbot.
Comment #10
coltraneCommitted. Thank you! http://drupalcode.org/project/tfa.git/commit/ef0978c
Comment #11
coltraneMade a followup issue to document how to handle required TFA for accounts who haven't set it up #2273603: Document require TFA and UX challenge
Comment #13
subhojit777I am sure not whether this is working. I have checked "Require TFA process" for two roles, but it is asking for authentication code during admin login, also for normal users who do not belong to any roles.
I am using Twilio SMS for authentication.
Comment #14
zopa CreditAttribution: zopa at Chromatic commentedI am also using Twilio SMS and couldn't get the latest tfa (7.x-2.0) and tfa_basic (7.x-1.0-beta3) modules to work out of the box. In order to bypass TFA for the roles not marked as required, I had to add a check for required roles in tfa_login_allowed() of tfa.module. Patch attached.
I also had to patch tfa_basic to get this to work. See https://www.drupal.org/node/2798713#comment-11615295.
Comment #15
zopa CreditAttribution: zopa at Chromatic commented