When I try to disable TFA for an account, I get a WSOD with an ugly error message:
GuzzleHttp\Exception\ClientException: Client error: `POST https://api.sendgrid.com/api/mail.send.json` resulted in a `400 Bad Request` response: {"errors":["Missing email body"],"message":"error"} in GuzzleHttp\Exception\RequestException::create() (line 113 of /app/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php).
I am using SendGrid to send mail, which is why Guzzle gets involved. The symptom might be less drastic if we are relying on the default e-mail handler, but I am pretty sure that it still does not work. The code comment (in src/Form/BasicDisable.php) agrees with me:
// @todo Not working, not sure why though.
// E-mail account to inform user that it has been disabled.
$params = ['account' => $account];
\Drupal::service('plugin.manager.mail')->mail('tfa', 'tfa_disabled_configuration', $account->getEmail(), $account->getPreferredLangcode(), $params);
It looks to me as though the problem is that the module does not implement hook_mail().
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | interdiff-3-8.txt | 1.2 KB | benjifisher |
| #8 | 2931147-error-disabling-8.patch | 3.9 KB | benjifisher |
Comments
Comment #2
benjifisherI am working on this now.
Comment #3
benjifisherIt looks as though the @todo comment was added in Commit 122059f0, and that a lot of code was copied over from the tfa_basic module, but not
tfa_basic_mail(). This patch is based on that function.The API documentation for
hook_mail()usesstrtr()instead oft(). Maybehook_mail()is invoked in a partially bootstrapped environment? I am usingt(), but I have not tested on a multilingual site.This patch also un-comments and updates the lines that send an e-mail confirmation when setting up an account.
While testing this, I once received two copies of the e-mail when setting up TFA. Maybe that is an unreproducible glitch, but please keep an eye out for this when testing yourself.
Comment #4
daggerhart commentedThis applies cleanly and works as written, but it brings up some UX issues.
Concerning #2: This is happening because the recovery codes are currently a "fallback" plugin. The concept will be removed in #2924691: Remove fallback plugins, make recovery code concept into validation & setup plugins, but for now we probably need to make sure that the current plugin is not a fallback plugin before sending the email.
Attached is a patch that does a bit of work to make sure the current plugin is not a fallback plugin. I didn't use DI because this is temporary (hopefully). And the hacky-looking str_replace to find the validation plugin id is because the way setup plugins are currently defined is a hardcoded expectation of
validation_plugin_id . "_setup"Thoughts?
Comment #6
daggerhart commentedwow, my last patch was terrible. restoring #3. The interdiff is correct, but something went wrong w/ the actual patch roll.
Comment #7
daggerhart commentedTrying this patch thing again.
Comment #8
benjifisherI was wondering about the double e-mail. Usually I skip the recovery codes, sometimes I do not. That is why I only saw the duplicate once.
Here is a variant of your patch. The idea is to make it easier to update after #2924691: Remove fallback plugins, make recovery code concept into validation & setup plugins goes in.
I have attached interdiffs to compare with the patches from Comment #3 and Comment #6.
I tested this patch locally. I get only one confirmation message, but I see the message twice in the logs, as expected.
Comment #9
daggerhart commentedThis looks even better, thank you! RTBC, and adding reference to the new UI task for these emails.
Comment #11
nerdsteinThe patch in #8 looks great - thanks for catching this. Merged.