We forgot to implement TfaBasicSms::ready().

Symptom: Recoverable fatal error: Argument 1 passed to Tfa::__construct() must be an instance of TfaBasePlugin, boolean given, called in tfa.module on line 186 and defined in Tfa->__construct() (line 68 of tfa.inc).

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
StatusFileSize
new946 bytes

Patch.

klausi’s picture

StatusFileSize
new1.23 KB

And I noticed that the SMS verification is still triggering even if TFA is disabled, so let's also consider that.

coltrane’s picture

Status: Needs review » Needs work

Good 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.

banviktor’s picture

Status: Needs work » Active

Enforcing 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.

banviktor’s picture

Status: Active » Needs review

About 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.

gisle’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

If 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.

apmsooner’s picture

I 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.

  • klausi authored e35b13c on 7.x-1.x
    Issue #2609090 by klausi: SMS TFA pretends to be ready when it is not
    
coltrane’s picture

Status: Reviewed & tested by the community » Fixed

Finally committed patch #3. Will review for making new release soon so stable doesn't have the error.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.