Would love to see a plugin for email strait out of the box for two factor authentication. I understand that the security from an email solution would not be all that great but verifying that a user actually owns the email that they entered on the account would be fantastic. Drupal provides only entry level functionality on this but my requirement is to elevate security for privileged authors. These users will have to enter a user and password and proceed to an email validation every time they login to continue to verify valid contact addresses for them. Would that be something that can be easily done?
Comments
Comment #1
coltraneAn email provider is already written in the example api code, it'd just be a matter of elevating it to the module code. Let me think on this some more.
Comment #2
gregglesI think https://drupal.org/project/email_confirm solves the problem described in the original post.
I agree that having email as a real provider would be helpful though it shouldn't be the "default."
Comment #3
coolestdude1 CreditAttribution: coolestdude1 commentedgreggles++
I agree on the default changes to the original post.
We are now investigating the email_confirm module for when a user changes an email address, however I believe my client in question will request this functionality be done every single log in for a certain untrusted role. Part of it is testing the email out to make sure they can receive emails from the server (email_confirm) but the other part is verifying that the email is still under their control at the time of login (tfa) to verify passwords were not compromised. After seeing the module you recommended I believe both modules will be a good combination of a solution.
Thanks :)
Comment #4
banviktor CreditAttribution: banviktor at CARD.com commented+1 on this one. When I started working on TFA one of my first few questions to Ben was whether this functionality is implemented somewhere. Steam uses this for new devices btw.
I don't know where that example code is, but if I'm directed to it I'm happy to provide a patch.
Comment #5
gregglesI think ben's comment about email being in the example code is no longer relevant because the code has been rewritten.
I'd say that this is probably most similar to the the sms code.
Comment #6
banviktor CreditAttribution: banviktor at CARD.com commentedOkay, thanks Greg!
I'm working on it.
Comment #7
banviktor CreditAttribution: banviktor at CARD.com commentedComment #8
banviktor CreditAttribution: banviktor at CARD.com commentedThis was pretty useful as I got more familiar with the setup process of TFA Basic.
The texts could probably be better.
Comment #9
monstrfolk CreditAttribution: monstrfolk commentedTFA Email setup with no other TFA options set does not work. I can log directly into the site after requiring TFA for my account.
Comment #10
monstrfolk CreditAttribution: monstrfolk commentedThis would be a really nice feature.
Comment #11
aneel11 CreditAttribution: aneel11 commentedHi,
I have deployed TFA module on my localhost machine and also integrate this email patch, but in setup configuration, when I add security code which I received on email, it asks me to enter admin password and then it again redirects to same page for entering verification code, it is not confirming the email setup account, also I received verification code again. Can anyone help me to sort out this issue.
Thanks
Aneel
Comment #12
dhampeters CreditAttribution: dhampeters as a volunteer commentedI am attempting to develop my own patch (for D7) in response to subsequent comments regarding the https://www.drupal.org/files/issues/2090033-8-tfa-basic-email.patch file on the https://www.drupal.org/node/2090033 issue.
What I notice is that the
ready()
function, when invoked, does not recognizeisset($data['plugins'])
orisset($data['data']['plugins'])
as being TRUE, even when manually addingtfa_basic_email
to the JSON stored in the appropriate row in thetfa_user_settings
table, which causes WSOD. Thetfa_basic_setup_save_data()
function is never invoked when utilizing the E-mail authentication method provided by the patch. Ifready()
returns FALSE, thentfa_user_login()
won't be invoked with conditions for adrupal_goto('system/tfa/' . $account->uid . '/' . $login_hash, array('query' => $query))
iftfa_user_login()
is invoked at all.How may I invoke
tfa_user_login()
in thetfa_basic_email
plugin?Comment #13
johnhanley CreditAttribution: johnhanley as a volunteer commented+1patchComment #14
nullkernel CreditAttribution: nullkernel commentedI made a module "tfa_email" in 2015 and it's been working fine for me since then. It was developed standalone (outside of tfa_basic). Since there is some interest and this issue is "Needs Work", I'll leave a .patch here since it may be useful.
Basically, I just created a "modules" directory in tfa_basic, moved my module into it, and did a git diff to create a patch. It might be worth more tightly integrating it with tfa_basic (rather than having it as a separate module).
Comment #15
nullkernel CreditAttribution: nullkernel commentedI realized that I messed up the directories in the patch I previously uploaded. So I'm uploading a new patch. The module files should now be under "modules/tfa_email" instead of just "modules". I also fixed whitespace warnings.
I wasn't able to delete my old patch, so I hid it from displaying. I've also provided an interdiff between my two patches (which probably isn't that useful).
Comment #16
johnhanley CreditAttribution: johnhanley as a volunteer commented@nullkernel, thanks for sharing your patch. It works as expected and plays nicely with TFA Basic.
Comment #17
johnhanley CreditAttribution: johnhanley as a volunteer commentedThe following code adds email subject and message fields to the TFA Basic UI for both default and fallback states.
This code can be rolled into nullkernel's patched module (with function names prefixed accordingly) or used separately in a custom companion module with a dependency on the tfa_basic_email module.
Comment #18
nullkernel CreditAttribution: nullkernel commentedI've refactored my standalone module's code so that it is integrated with tfa_basic (instead of existing as a separate module). Please review.
Thanks for the admin form code @johnhanley. The patch I'm attaching includes your code as well.
Anyone who enabled the "tfa_email" module should disable/uninstall/remove it before applying this patch. Otherwise fatal errors will occur due to some functions being defined twice (if the tfa_email module is still enabled).
Comment #19
nullkernel CreditAttribution: nullkernel commentedI noticed a problem after integrating with tfa_basic, and have updated the patch to fix this. Whether or not a user had tfa_basic_email enabled in their configuration didn't matter - email would still be used as a TFA method. This is because TfaBasicEmail::ready() wasn't implemented, and simply used the TfaBasePlugin::ready() method (which always returns TRUE). With this patch, users shouldn't get email challenges unless they have set up tfa_basic_email.
Comment #20
parasolx CreditAttribution: parasolx as a volunteer commentedWow.. thanks for your big improvement. I already used it in my production site and it works as expected. I'm not sure why testing for php5.5 undergoes as a failure but it works fine in my environment using PHP7.0.
Comment #21
5818959 CreditAttribution: 5818959 commentedSome little improvements such email code expiration, support code case insensitive (useful for code with letters) and configure plugin by config variables.
Comment #22
monstrfolk CreditAttribution: monstrfolk commentedOne small fix for title. Verification Code should be Email Verification Code.
Comment #23
gisleUnassigning - looks like there is three years since banviktor worked on this.
I will also add that I agree that patch in #22 is RTBC.
Comment #24
gregglesThanks for your work on this, everyone! I have some feedback, some of which seems like it should be fixed prior to the commit of this patch, so I'm moving that status to needs work.
The TfaBasicEmail::generate code generation code uses the old Drupal-5-era password generation code that can lead to insecure codes being generated. That was the basis of https://www.drupal.org/sa-contrib-2018-044 The change to match more modern style of generating a random string is in this commit.
The TfaBasicEmail::validate function uses string comparison that can lead to timing attacks. It would be better to use hash_equals for the comparison.
A few functions are missing php documentation.
I believe the string below causes problems for translations and that it's better to use double quotes around the string so that the ' on "account's" does not need to be escaped with a \.
The code comment in TfaBasicEmailSetup::begin is missing terminating punctuation. Generally it seems like it would be good to review this with the php code style rules.
Comment #25
monstrfolk CreditAttribution: monstrfolk commentedChanges for comments from #24.
Comment #26
monstrfolk CreditAttribution: monstrfolk commentedComment #27
monstrfolk CreditAttribution: monstrfolk commentedhash_code is not part of (PHP 5 >= 5.6.0).
Added embedded hash_code check from https://github.com/facebook/php-graph-sdk/blob/5.x/src/Facebook/polyfill...
Comment #28
monstrfolk CreditAttribution: monstrfolk commentedUploaded wrong patch. :(
Comment #29
coltraneThank you monstrfolk, your patch looks good and worked for me. There are two "@todo"s in it, do you want feedback on those and to address before commit? IMO neither need addressing but curious to know what you think.
Comment #30
monstrfolk CreditAttribution: monstrfolk commentedColtrane, completed one TODO. Left second TODO since is same as SMS code. All changed label text to be more consistent with other TFA code. Please send feedback, or commit. I believe there is nothing pressing that must be completed before commit.
Comment #31
parasolx CreditAttribution: parasolx as a volunteer commentedDear monstrfolk, just applied your latest patch at #30 on my production site. Looks really good and working without any single error occur. Run using PHP7.1 and it works smoothly. I suggest it should be committed to dev version.
Comment #32
monstrfolk CreditAttribution: monstrfolk commentedComment #33
hoegrammer CreditAttribution: hoegrammer commentedHi, +1 for this, I'm using it and it works well.
Comment #34
nottaken CreditAttribution: nottaken commentedI'm going to be using this patch in #30, curious when/if it will be included in a release soon.
Comment #36
MustangGB CreditAttribution: MustangGB commentedCan remove:
Because it was added to tfa 7.x-2.x as
timingSafeEquals()
in #3183248: Use hash_equals instead of ===.