Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Zend Mail with PHPMailer #15891

Merged
merged 22 commits into from May 14, 2020
Merged

Replace Zend Mail with PHPMailer #15891

merged 22 commits into from May 14, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Apr 30, 2020

fixes #14841

@sgiehl sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Apr 30, 2020
@sgiehl sgiehl added this to the 4.0.0 milestone Apr 30, 2020
@Findus23
Copy link
Member

If I could wish for one more minor feature:

Would it be possible to add the verbose logging ($mail->SMTPDebug = SMTP::DEBUG_SERVER; ) to the output of the core:test-email command? This way debugging issues with E-Mail delivery would be quite a bit easier.

@sgiehl
Copy link
Member Author

sgiehl commented Apr 30, 2020

@Findus23 thanks for the hint. Did not yet manually test anything only tried to get the tests passing. Guess we will need to do some manual testing with various settings before merging this. Could you maybe give me some common scenarios that should be tested? And maybe also some that didn't work with Zend_Mail if you know any?

core/Mail.php Outdated Show resolved Hide resolved
@Findus23
Copy link
Member

Findus23 commented May 4, 2020

I would at least test the following:

@sgiehl
Copy link
Member Author

sgiehl commented May 4, 2020

@Findus23 Adding support for Gmail XOAuth would be possible using PHPMailer, but the required packages are not included by default. We would need to require thephpleague/oauth2-client, which pulls in some other libs and polyfills. Wondering if it might be better, to put that into a separate plugin as I don't think using gmail is a common use case, is it?
(The Mail.send event could be actually used to inject the oauth settings before sending)

@mattab @tsteur what do you think re OAuth support?

@Findus23
Copy link
Member

Findus23 commented May 4, 2020

@sgiehl I think this XOAuth support is not needed as it would add a lot of overhead and potential confusion to users for little gain.
Sending E-Mails via Gmail should still be possible "the normal way": https://github.com/PHPMailer/PHPMailer/blob/master/examples/gmail.phps

If people really need it, creating a plugin for it is probably easier.

@sgiehl
Copy link
Member Author

sgiehl commented May 4, 2020

Ok. So far I have tested:

  • use Gmail with app specific token
  • use custom smtp host with TLS (don't have one still working with ssl)
  • use local sendmail
  • check attachments are correct (inline images and pdfs)
  • check some errors with invalid configs
  • ensure phpmailer errors are in current language

Seems to work so far and should be ready for a review now.

@sgiehl sgiehl marked this pull request as ready for review May 4, 2020 10:30
@sgiehl sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 4, 2020
@Findus23
Copy link
Member

Findus23 commented May 4, 2020

Sounds good. The rest could probably be done with general Matomo 4 testing duing the beta releases.

@sgiehl sgiehl mentioned this pull request May 4, 2020
core/Mail.php Outdated Show resolved Hide resolved
@@ -121,6 +121,14 @@ public function doUpdate(Updater $updater)
// switch to default provider if GeoIp Legacy was still in use
LocationProvider::setCurrentProvider(LocationProvider\DefaultProvider::ID);
}

// @todo migrate that to a config migration. See utf8mb4 branch
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per comment. I've already built a config migration for the utf8mb4 branch. We should change that once that's merged. See 8907203

@tsteur
Copy link
Member

tsteur commented May 11, 2020

I'm also seeing another problem re WordPress. See https://developer.wordpress.org/plugins/wordpress-org/detailed-plugin-guidelines/#13-plugins-must-use-wordpress-default-libraries

We will need to test that in Matomo for WordPress we can safely remove the vendor/phpmailer library and things still work because we're not allowed to ship our plugin with PHPMailer dependency. Ideally, we would have something like

class Mail {
public function send() {
   $phpmailer = new PHPMailer(); $phpmailer->.... send();
}
}

instead of class Mail extends PHPMailer{}. This would make our mailer implementation more independent of the actual library but of course this has potentially some downsides. Not sure how easy that would be doable...

Did a quick test... by default WP does not find PHPMailer
image

It does work when using include ABSPATH . '/wp-includes/class-phpmailer.php';

WP is currently using PHPMailer 5.2.27 in their latest release. However, older versions of WP might use different versions and our PHPMailer version might have slightly different definitions causing issues potentially like these (could happen in the future)
image

Ideally, we'd not extend the PHPMailer class and ideally, if possible, only use PHPMailer as part of the send method or so.

@sgiehl
Copy link
Member Author

sgiehl commented May 12, 2020

@tsteur completely refactored the code now. The Piwik\Mail class is now independent and uses a Transport class to send the mail using PHPMailer. The transport class can be replaced, which can be used by wordpress or cloud plugins....

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sgiehl I think that's something I will be able to work with (hopefully). I'm expecting some trouble where we might need to add quite some code in Matomo for WordPress around attachments but will try to workaround and can always make further changes later if needed. Left a few other comments to look at. Cheers

@tsteur
Copy link
Member

tsteur commented May 13, 2020

@sgiehl just tested it but noticed there's now a method addBcc missing.

The setReplyTo method might be good to not deprecate it but instead actually make it a set (meaning overwrite any previously set replyTo)?

Otherwise looks good and things worked. Also text mails and various attachments. Be good to make the two changes and then merge 👍

@sgiehl
Copy link
Member Author

sgiehl commented May 14, 2020

@tsteur applied the feedback. will merge once the tests passed...

@sgiehl sgiehl merged commit 0187b71 into 4.x-dev May 14, 2020
@sgiehl sgiehl deleted the phpmailer branch May 14, 2020 07:52
@hahayidu
Copy link

hahayidu commented Dec 15, 2020

Hi,

If anyone need to disable $SMTPAutoTLS functionalty because of a getting an error: SMTP Error: Could not connect to SMTP host.

PHPMailer.php

/**
 * Whether to enable TLS encryption automatically if a server supports it,
 * even if `SMTPSecure` is not set to 'tls'.
 * Be aware that in PHP >= 5.6 this requires that the server's certificates are valid.
 *
 * @var bool
 */
public $SMTPAutoTLS = true;

You can set true to false.

This change may cause security vulnerability that we cannot predict. Please also read the ongoing messages.

@Findus23
Copy link
Member

Hi @hahayidu,

If I understand it correctly setting this setting to false, means PHPMailer will accept any invalid certificate which means anyone can man-in-the-middle your E-Mail connection and read and modify it and phpmailer will just accept their faked certificate as correct.

So I'd recommend you to fix the actual error instead of disabling security checks.

@hahayidu
Copy link

Hi @Findus23,

Yes you are right if matomo send mail directly to wide area network without encryption someone can be listen. In particular, sending a password reset request can be more dangerous than reports.

In my case, I wasn't even getting a certificate error. Because our internal smtp relay server doesn't a valid TLS configuration. (Probably doesn't have an any best practices configuration) I know this kind of case is not common.

I think real problem is PHPMailer override the mail configuration.

This is my configuration.

[mail]
transport = "smtp"
port = "25"
host = "XXXX.YYYY.com"

If $SMTPAutoTLS is true matomo can't send e-mail. I expect if I don't select any encryption, phpmailer shouldn't encrypt to connection. Empty is also an option in matomo ui.

So, Opportunistic TLS (which is default for phpmailler) is not useful some case.

Thank you for your response. I will update first post with your concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use a modern PHP library for sending emails and SMTP
4 participants