Closed (fixed)
Project:
TFA Basic plugins
Version:
7.x-1.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
5 Nov 2015 at 12:21 UTC
Updated:
18 Sep 2018 at 01:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
klausiPatch.
Comment #3
klausiAnd I noticed that the SMS verification is still triggering even if TFA is disabled, so let's also consider that.
Comment #4
coltraneGood catch, but I'd prefer status be enforced in tfa_basic_tfa_context_alter() than within a plugin since any plugins shouldn't run if status is disabled.
Comment #5
banviktor commentedEnforcing status should be implemented, but I don't think it's possible (in a clean way) in tfa_basic_tfa_context_alter(). The problem is that $context['validate'] can't be empty
so @klausi's patch in #3 is the way this can be done with the least amount of code.If TFA is enabled it expects a validate plugin to exist. TFA process can only be skipped if that validate plugin is not ready, which is usually (i.e. with the patch in #3) the case if TFA hasn't been set up for the user or it was disabled properly. So if the server crashes during disabling TFA for an account it can get messy.
A workaround would be creating a dummy plugin in TFA Basic which is never ready, and if status is false for the user and there are no plugins other than TFA Basic's then we set the validate plugin to be the said dummy plugin.
Other way would be creating a hook in TFA which tells if there is an active plugin for the user, or checking whether the validate plugin is empty. If we choose to modify TFA too I think we'll need some notes on the project page about version compatibility.
Edit: actually no, I think this particular case can be solved without checking status in TfaBasicSms::ready(), but the thing about enforcing status in tfa_context_alter is still true. I can provide a patch tomorrow.
Comment #6
banviktor commentedAbout my edited comment:
Nope, @klausi's patch is good. My thought was that during disabling TFA the 'sms' field is not emptied, but actually it is. So maybe we could replace that one line that checks for 'status' in TfaBasicSms::ready() with a line that checks for the 'sms' field which is contextually more appropriate but in the end the behavior is the same.
Comment #7
gisleIf you set up TFA Basic to use Twilio SMS as the default validation plugin the bug reported in the issue render a site unusable (as login results in a fatal error with no workaround). I think this situation warrents critical priority.
@klausi's patch is good, and since nobody has come up with any alternative solution for a year, I'm setting this to RTBC.
While there may be even better ways to fix this (re comments #4, #5, #6), I think the time is ripe for pushing this to the repo so that the project is in a usable state with Twilio SMS.
Comment #8
apmsooner commentedI came across this issue after reporting https://www.drupal.org/node/2890759 and after applying the patch, it seems to work now. I wouldn't think anything should happen at all though regarding plugins if the "Enable TFA" is unchecked. Something needs to be reworked regarding the access checks and redirect to first check the "Enable TFA" variable i think.... and additionally this patch is pretty critical as none of the plugins allow login entry even if not specify twilio sms as active or fallback. I'm surprised this issue with patch has sat in review for so long with no new release.Update: My issue has resurfaced regardless of the patch being applied.
Comment #10
coltraneFinally committed patch #3. Will review for making new release soon so stable doesn't have the error.