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

Fix SMTP Mail send #490 #524

Closed
wants to merge 272 commits into from
Closed

Conversation

mrflobow
Copy link
Contributor

This commit fixes #490.
For accomplishing the goal I added the libary phpmailer (as also suggested).
No big issues with adding it to composer.
I believe with using the library lansuite mail module gets future proof and can be used in wider server configuration.

Tested following combinations:

  • No Auth against local docker smtp container ->Success
  • With Auth against MailTrap - >Success
  • With TLS against Outlook. - >Success

For public SMTP you can't send as different user so I added an optional configuration to set the sender instead of the user itself. Example could be used with noreply@something.com .
If the configuration is left blank it will use the user E-Mail instead.

Summary:

  • Refactored Mail Module
  • Used widely use PHPMailer for sending mails instead of reimplenting from scratch.
  • Added new settings to make SMTP configurable as needed.
  • PHP Composer lock has been updated.

@sonarcloud
Copy link

sonarcloud bot commented Jan 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@M4LuZ
Copy link
Collaborator

M4LuZ commented Jan 17, 2023

I'm a bit on the fence for this one.
On the one hand this allows a bit of more variety in setups that we support.
On the other hand I have not yet seen a setup where mail is not handed off to a local sendmail instance that then is configured correctly.
Using personal mail accounts as in #490 appears to me more as something that should be prevented than a functionality that should be provided.

But as said: undecided, so it would be good to hear your thoughts.

@mrflobow
Copy link
Contributor Author

mrflobow commented Jan 17, 2023

In general the support for TLS has been established and the possibile to change the default port from port 25.
We can discuss about the from, but it seems to be the logical place for me. It would be confusing not to have the setting there.

I belive setting up a sendmail smtp relay is not trivial if you want to use your existing smtp relay on a linux server.
However the function was already implemented, I slightly improved it.

Are you questioning if SMTP should be supported at all?

@M4LuZ
Copy link
Collaborator

M4LuZ commented Jan 17, 2023

Sorry, let me rephrase that:
This was not about the implementation details - these are fine and I'm happy to merge as-is, but about the necessity and implications of it.

As we have generally the following use-cases/usergroups:

  • Somebody wanting to run this on managed webspace: mail is usually transmitted via preconfigured sendmail instance and we're fine, as we are now
  • Somebody wanting to run this on their own box: normally same applies, we are also fine
  • Somebody that runs an thrown-together setup locally with W/XAMPP (also there mercury acts as local sendmail and needs separate config) in a local network as intranet system. That normally does not send any mails at all

That leaves the following use-cases where this would help:

  • somebody wants to run LanSuite, but does not have the technical skills to do create, maintain and supervise the required environment
  • Enterprise setups with large networks between Webserver and Mailserver, requiring encryption in between
  • the odd case in a hosted environment where an remote mailserver AND encryption is required, or where an intranet instance should send mail to external recipients

And I worry that this enables especially the first group to do more harm than good.
But I could also worry for nothing and to support remote servers and TLS is a fair one.
Thus asking the round if that makes sense

@mrflobow
Copy link
Contributor Author

@M4LuZ thanks for providing your elaborated thoughts.

I don't believe the operator would need to use mail/smtp at all for internal communication. So maybe it would be good to disable this at all.
Only thing I see necessary is using mail / smtp to send password resets or newsletters. This would reduce the use case to a noreply at something adress + sendmail/smtp relay.

Would that lower your concerns?

My guess e.g. in AWS you would be happy to use SMTP directly and from my exp. it's a pain to setup sendmail/other centos magic :D to forward to SMTP Relay. So I'm always happy to have this in an application. Just my 2cents on this :-)

dependabot bot and others added 25 commits March 1, 2023 05:00
Bumps [symfony/cache](https://github.com/symfony/cache) from 5.4.18 to 5.4.21.
- [Release notes](https://github.com/symfony/cache/releases)
- [Changelog](https://github.com/symfony/cache/blob/6.2/CHANGELOG.md)
- [Commits](symfony/cache@v5.4.18...v5.4.21)

---
updated-dependencies:
- dependency-name: symfony/cache
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
…fony/cache-5.4.21

Bump symfony/cache from 5.4.18 to 5.4.21
…izlabs/php_codesniffer-3.7.2

Bump squizlabs/php_codesniffer from 3.7.1 to 3.7.2
Bumps [phan/phan](https://github.com/phan/phan) from 5.4.1 to 5.4.2.
- [Release notes](https://github.com/phan/phan/releases)
- [Changelog](https://github.com/phan/phan/blob/v5/NEWS.md)
- [Commits](phan/phan@5.4.1...5.4.2)

---
updated-dependencies:
- dependency-name: phan/phan
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
…n/phan-5.4.2

Bump phan/phan from 5.4.1 to 5.4.2
Bumps [symfony/cache](https://github.com/symfony/cache) from 5.4.21 to 5.4.23.
- [Release notes](https://github.com/symfony/cache/releases)
- [Changelog](https://github.com/symfony/cache/blob/6.2/CHANGELOG.md)
- [Commits](symfony/cache@v5.4.21...v5.4.23)

---
updated-dependencies:
- dependency-name: symfony/cache
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
…fony/cache-5.4.23

Bump symfony/cache from 5.4.21 to 5.4.23
Bumps [phpstan/phpstan](https://github.com/phpstan/phpstan) from 1.9.13 to 1.10.15.
- [Release notes](https://github.com/phpstan/phpstan/releases)
- [Changelog](https://github.com/phpstan/phpstan/blob/1.11.x/CHANGELOG.md)
- [Commits](phpstan/phpstan@1.9.13...1.10.15)

---
updated-dependencies:
- dependency-name: phpstan/phpstan
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 3.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2...v3)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updates the basic development env to the minimum versions of latest official supported versions:

* PHP: 7.2.34 -> 8.0.28
* Composer: 1.8.0 -> 2.5.5
* xDebug: 2.9.0 -> 3.2.1
* phpunit/phpunit: ^8.5
* smarty/smarty: ^4.0
* youthweb/bbcode-parser: ^1.6
* nginx: Pinned at nginx:1.23.4-alpine
* MySQL: 5.6 -> 5.7.42

The application is not fully usable/runnable in this configuration, but it starts.
andygrunwald and others added 8 commits June 2, 2023 08:19
…ion3

PHP8: Fix Warnings of undefined variables, early access, unknown array keys, etc. (Iteration 3)
Replace symfony/debug with symfony/error-handler, because the debug package is is abandoned
Bumps [phpstan/phpstan](https://github.com/phpstan/phpstan) from 1.10.15 to 1.10.16.
- [Release notes](https://github.com/phpstan/phpstan/releases)
- [Changelog](https://github.com/phpstan/phpstan/blob/1.11.x/CHANGELOG.md)
- [Commits](phpstan/phpstan@1.10.15...1.10.16)

---
updated-dependencies:
- dependency-name: phpstan/phpstan
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
GitHub Actions: Label Pull Requests if they have a merge conflict
dependabot bot and others added 14 commits June 8, 2023 08:25
Bumps [phpstan/phpstan](https://github.com/phpstan/phpstan) from 1.10.16 to 1.10.18.
- [Release notes](https://github.com/phpstan/phpstan/releases)
- [Changelog](https://github.com/phpstan/phpstan/blob/1.11.x/CHANGELOG.md)
- [Commits](phpstan/phpstan@1.10.16...1.10.18)

---
updated-dependencies:
- dependency-name: phpstan/phpstan
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps composer from 2.5.7 to 2.5.8.

---
updated-dependencies:
- dependency-name: composer
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps php from 8.0.28-fpm-bullseye to 8.0.29-fpm-bullseye.

---
updated-dependencies:
- dependency-name: php
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [phpunit/phpunit](https://github.com/sebastianbergmann/phpunit) from 9.6.8 to 9.6.9.
- [Changelog](https://github.com/sebastianbergmann/phpunit/blob/9.6.9/ChangeLog-9.6.md)
- [Commits](sebastianbergmann/phpunit@9.6.8...9.6.9)

---
updated-dependencies:
- dependency-name: phpunit/phpunit
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [phpstan/phpstan](https://github.com/phpstan/phpstan) from 1.10.18 to 1.10.19.
- [Release notes](https://github.com/phpstan/phpstan/releases)
- [Changelog](https://github.com/phpstan/phpstan/blob/1.11.x/CHANGELOG.md)
- [Commits](phpstan/phpstan@1.10.18...1.10.19)

---
updated-dependencies:
- dependency-name: phpstan/phpstan
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [rector/rector](https://github.com/rectorphp/rector) from 0.17.0 to 0.17.1.
- [Release notes](https://github.com/rectorphp/rector/releases)
- [Commits](rectorphp/rector@0.17.0...0.17.1)

---
updated-dependencies:
- dependency-name: rector/rector
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [phpstan/phpstan](https://github.com/phpstan/phpstan) from 1.10.19 to 1.10.20.
- [Release notes](https://github.com/phpstan/phpstan/releases)
- [Changelog](https://github.com/phpstan/phpstan/blob/1.11.x/CHANGELOG.md)
- [Commits](phpstan/phpstan@1.10.19...1.10.20)

---
updated-dependencies:
- dependency-name: phpstan/phpstan
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [phpstan/phpstan](https://github.com/phpstan/phpstan) from 1.10.20 to 1.10.21.
- [Release notes](https://github.com/phpstan/phpstan/releases)
- [Changelog](https://github.com/phpstan/phpstan/blob/1.11.x/CHANGELOG.md)
- [Commits](phpstan/phpstan@1.10.20...1.10.21)

---
updated-dependencies:
- dependency-name: phpstan/phpstan
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Several PHP 8-Warning fixes (no logic changes)
… Installation page (lansuite#650)

* Fix lansuite#344: Switching to English during Installation breaks the Installation page

Fix lansuite#344
Related lansuite#415

* Fix usage of wrong variable name

* EnvIsConfigured: Renamed File according function

---------

Co-authored-by: M4LuZ <MaLuZ@gmx.net>
Bumps [setasign/fpdf](https://github.com/Setasign/FPDF) from 1.8.5 to 1.8.6.
- [Release notes](https://github.com/Setasign/FPDF/releases)
- [Changelog](https://github.com/Setasign/FPDF/blob/master/changelog.htm)
- [Commits](Setasign/FPDF@1.8.5...1.8.6)

---
updated-dependencies:
- dependency-name: setasign/fpdf
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 36 to 37.
- [Release notes](https://github.com/tj-actions/changed-files/releases)
- [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md)
- [Commits](tj-actions/changed-files@v36...v37)

---
updated-dependencies:
- dependency-name: tj-actions/changed-files
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@andygrunwald
Copy link
Collaborator

Thanks @mrflobow, for your work on this.

I am a big fan of using established libraries for standard use cases like sending emails.
I understand that many people prefer using their self-hosted sendmail or similar service. However, a big fraction of people like to delegate this to a SaaS like one of the Cloud Hyperscalers or Mailgun.

My plan is to bring this PR to the latest and mergeable state.
I think it enables more people to use the module and all current use cases are also covered.

This might enable better usecases in the future. At least, we rely on properly maintained PHP packages for the heavy lifting.

…obow-fix/STARTTLS

* 'fix/STARTTLS' of github.com:mrflobow/lansuite:
  phpcs fix
  - Sonar fixes - Reduced complexity - Added validator
  * Sonar Fixes
  * Sonar Fixes
  * Sonar Fixes * Improved Mail Pattern * Fixed wrong Encryption Pattern , explicit use TLS
  Removed unused code
  * Refactored Mail Module * Used widely use PHPMailer for sending mails instead of reimplenting from scratch. * Added new settings ** Set Port, Sender (opt) , TLS Yes/No * PHP Composer lock has been updated.
@sonarcloud
Copy link

sonarcloud bot commented Jun 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

@andygrunwald
Copy link
Collaborator

@mrflobow @M4LuZ I fucked this branch up. Sorry.
My goal was to rebase it and make it mergable again. Instead I pushed way to many commits.

I opened a new PR and cherry-picked the original changes.
Look at #658

This will be the Pull Request to make this a success :)

@mrflobow mrflobow deleted the fix/STARTTLS branch November 5, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E-Mail communication does not support STARTTLS
5 participants