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

Add allow self singed in mail setting #20046

Merged
merged 12 commits into from Dec 7, 2022
Merged

Add allow self singed in mail setting #20046

merged 12 commits into from Dec 7, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Nov 24, 2022

Description:

Fixes: #18563
add allow self singed in mail setting

Review

add allow self singed
@peterhashair peterhashair marked this pull request as ready for review November 24, 2022 22:53
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 24, 2022
@peterhashair peterhashair added this to the 4.12.5 milestone Nov 25, 2022
update change log
@peterhashair peterhashair added the Needs Review PRs that need a code review label Nov 27, 2022
@sgiehl
Copy link
Member

sgiehl commented Nov 28, 2022

@peterhashair Weren't @mattab's requirements defined in #18563 (comment) clear enough? Wondering why you implemented only one ini setting instead of the three settings he requested 🤔

@Findus23
Copy link
Member

I would also change the wording of the setting. allow_self_signed implies this allows an additional mode of operation and something that has a limited impact of security (it feels like "only one more thing to support").
But what the setting in fact does is completely remove all TLS verification, allowing anyone in-between Matomo and the mail server to intercept, read and modify e-mails without any issues. (and the fact this also makes self signed certs work is just a side-effect)
So the setting should make it clear that this has strong security implications and should only be set if the person exactly understands what it means and that it is fine in their environment (e.g. the mail server is on localhost)

update config options
@peterhashair
Copy link
Contributor Author

@sgiehl ops update it.

@justinvelluppillai justinvelluppillai modified the milestones: 4.12.5, 4.13.2 Nov 28, 2022
CHANGELOG.md Outdated
@@ -4,6 +4,10 @@ This is the Developer Changelog for Matomo platform developers. All changes in o

The Product Changelog at **[matomo.org/changelog](https://matomo.org/changelog)** lets you see more details about any Matomo release, such as the list of new guides and FAQs, security fixes, and links to all closed issues.

## Matomo 4.12.5
Copy link
Contributor

Choose a reason for hiding this comment

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

This would actually be 4.13.0 if this is merged before the next release. Otherwise potentially 4.13.1

Peter added 4 commits November 30, 2022 11:22
update changelog version number
# Conflicts:
#	CHANGELOG.md
resolve conflic
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Added some suggestions for text improvements. Otherwise guess this should work as expected.

config/global.ini.php Outdated Show resolved Hide resolved
config/global.ini.php Outdated Show resolved Hide resolved
config/global.ini.php Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Dec 1, 2022
Peter Zhang and others added 5 commits December 2, 2022 10:31
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@peterhashair peterhashair added the Needs Review PRs that need a code review label Dec 6, 2022
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Dec 7, 2022
@peterhashair peterhashair merged commit 52eb229 into 4.x-dev Dec 7, 2022
@peterhashair peterhashair deleted the m18563 branch December 7, 2022 21:25
@mattab
Copy link
Member

mattab commented Dec 8, 2022

Sorry i forgot to mention, but here I would expect that we surface the existence of these settings to the user when they get the error, say change the message from:

SMTP Error: Could not connect to SMTP host.

(as found in the original issue)

to something helpful to users to find it directly like:

SMTP Error: Could not connect to SMTP host. If you use a SMTP server with a self-signed certificate, see this FAQ: LINK_TO_FAQ

and additionally create a new short FAQ to document the settings.

Note: Just seeing i had mentioned it in #18563 (comment)

@sgiehl
Copy link
Member

sgiehl commented Dec 8, 2022

@mattab That might be not that easy to implement. The error message is coming from PHPMailer, which throws a normal \Exception. We could look at the error message, but PHPMailer has messages that are translated to the requested language. Also those messages might change with every release of PHPMailer, so if we look for a certain message, that might start failing with each release of PHPMailer in any language.
What we could do is provide some sort of fake language file for PHPMailer, that will be used. That will ensure that we always get the same error messages. But it would mean, that we would need to do the translation manually afterwards, or even provide all of them through our translation system.
Not sure if that's worth the effort. If you think that's important enough to spent the time on it, feel free to reopen the related issue or create a new one for it.

@mattab
Copy link
Member

mattab commented Dec 9, 2022

Sounds good - then we just need to Create a FAQ that mentions the console error message + the solution with the new INI setting (so that people searching for the error will find it) eg. "How do I configure Matomo to send emails when a self-served certificate is used, or SSL doesn't work?"

@sgiehl sgiehl modified the milestones: 4.13.2, 4.13.1 Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to send mails through SMTP server with self-signed cert (and undescriptive error msg)
5 participants