With Twilio SMS as the plugin, the check in tfa_basic_tfa_context_alter() of tfa_basic.module uses '||' to remove the SMS plugin if the mobile number is not available. First time login of users with a TFA required role and valid mobile number are not able to access the TFA login flow - the constructor blows an error because the check returns true and removes the SMS plugin.

CommentFileSizeAuthor
#2 2798713-tfa_basic_sms_first_login-2.patch626 byteszopa

Comments

zopa created an issue. See original summary.

zopa’s picture

zopa commented
StatusFileSize
new 2798713-tfa_basic_sms_first_login-2.patch626 bytes

Here's a patch for updating the check in tfa_basic_tfa_context_alter() - I had to use this is in conjunction with this patch and a custom module that overrides the error messages in order for first time login of users with a TFA required role and a valid mobile number to enter the TFA login flow.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Version: 7.x-1.0-beta3 » 7.x-1.x-dev
Status: Active » Needs review

Thanks for the patch! BTW when you upload a patch please try to remember to set the issue status to "needs review", it triggers the testbot and lets others know that there's something to look at.

poker10’s picture

poker10 commented
Status: Needs review » Postponed (maintainer needs more info)

Thanks for working on this @zopa.

Can you please post an error which is thrown without the patch #2? I do not have Twillio enabled and available for testing now. However reading the code, the comment could be a bit misleading, but the condition itself looks good to me: 

if ((empty($tfa_data['data']['sms']) || !$number)) {

The condition is checking the output from tfa_basic_get_tfa_data() and will disable the validation in case the SMS is FALSE (e.g. disabled) or the phone number does not exists. I think this is correct.

Can you also please check, if this issue #2609090: SMS TFA pretends to be ready when it is not have not fixed the potential error mentioned by this issue?

Thanks!