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

Move Java Mail configuration to application #7424

Closed
poikilotherm opened this issue Nov 18, 2020 · 1 comment · Fixed by #9939
Closed

Move Java Mail configuration to application #7424

poikilotherm opened this issue Nov 18, 2020 · 1 comment · Fixed by #9939
Assignees
Labels
Feature: Installation Guide Feature: Installer Feature: Messaging Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Milestone

Comments

@poikilotherm
Copy link
Contributor

poikilotherm commented Nov 18, 2020

As with JDBC #7418 and JMS #7423, we can move the mail configuration to the application. This further reduces the necessary bash glue to install Dataverse or use in containers. It eases reconfiguration, too.

Using Java EE 7 @MailSessionDefinition for this.

This should make things much easier in cases like #3061, #5986, #5723, #4969. #5707

Let's ping some people from the mail service code base: @pdurbin @qqmyers @landreev @sekmiller

@poikilotherm poikilotherm self-assigned this Nov 18, 2020
@poikilotherm poikilotherm added Feature: Installer Feature: Installation Guide Feature: Messaging Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh labels Nov 18, 2020
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Nov 18, 2020

OK while this is fairly easy to do (add annotation to MailServiceBean), I stumbled over something.

The database setting :SystemEmail is used to set the "From" address of the message. It is even used to explicitly avoid blocking for mails. (Thx @qqmyers !)

MailServiceBean.sendSystemMail():

MailServiceBean.sendMail():

//Always send from system address to avoid email being blocked
InternetAddress fromAddress=getSystemAddress();

What I find very confusing: why should we do this? The "From" address is already configured for the JavaMail resource:

./asadmin $ASADMIN_OPTS create-javamail-resource --mailhost "$SMTP_SERVER" --mailuser "dataversenotify" --fromaddress "do-not-reply@${HOST_ADDRESS}" --property mail.smtp.port="${SMTP_SERVER_PORT}" mail/notifyMailSession
else
./asadmin $ASADMIN_OPTS create-javamail-resource --mailhost "$SMTP_SERVER" --mailuser "dataversenotify" --fromaddress "do-not-reply@${HOST_ADDRESS}" mail/notifyMailSession

When a sysadmin configures a mail account from, lets say GMail, this might cause a situation where the authenticated user may not send a mail with a differing mail from :SystemEmail.

So while I add the annotation, I'd vote to ease the situation by:

  1. Remove the DB setting
  2. Make the annotation easily configurable via MicroProfile Config API
  3. Rely on the Session to set the from mail for us (as configured).

After wrting all of this I found a prior issue on this: #4210 (thanks @donsizemore !) So we might as well choose to keep these two code changes separate. Whatever you like best.

poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Dec 1, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Dec 1, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Dec 1, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Dec 3, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Dec 3, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Dec 3, 2020
@mreekie mreekie added the bk2211 label Nov 1, 2022
@mreekie mreekie removed the bk2211 label Jan 11, 2023
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 24, 2023
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 24, 2023
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
…7424

Besides adding the JVM option, the logic to receive the setting in MailServiceBean has changed.
The method signature is now returning an optional to enforce the optional nature of the setting.
This replaces the "null" contract from before and requires more changes to code using the lookup.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
…dress IQSS#7424

As we changed the lookup function to use Optional<InternetAddress> to enforce the optional
nature of the setting, we now have to change the code using the function.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
To ease looking up the (also optional) setting of a support team mail address,
the mail service is extended with another lookup function. This is intended
to replace many manual, error prone lookups, also streamlining the fall-through
behavior when not set, etc.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
…#7424

As we now have proper functions to lookup the mail addresses, replace manual lookup and parsing with them.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
Document in code how to replace usages, too. (There aren't any,
but in case someone is adding it again in the future, it helps to
have docs)
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
As we replaced the lookups and parsing with a streamlined version of it all
in MailServiceBean, we don't need this helper function anymore.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 6, 2023
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 10, 2023
- With JavaMail 1.6+, we have support for UTF-8 mail addresses and don't need to parse these ourselves
- Remove some C-style coding and duplications
- Make logging eat less cycles
- Add missing Javadocs
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 10, 2023
- Make support configurable using new setting, defaulting to true
  (most MTAs today should support SMTPUTF8)
- If need be and an admin disables the support, make email validator
  deny UTF-8 chars (otherwise no mails could be sent!)
- Add logging message to send method to give hint about necessary
  UTF-8 support everywhere in the chain
- Add (extensible) integration test for MailServiceBean to check
  sending mails actually works
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Oct 10, 2023
Mail notification are optional (mostly to avoid setting up mail
services in dev envs). Do not enforce MTA host config and do not
pester logs about missing configuration.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Nov 8, 2023
Using @resource on the field triggers deployments to fail if the resource is not provided by the app server.
Using a programmatic lookup, we can catch and ignore the exception.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Nov 8, 2023
Without this change, the javamail maps would not be included in the artifact and trigger error messages in the logs about them being missed.

The error message will still be present as long as payara/Payara#6254 is
not fixed, released and we updated to a newer version of Payara.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Nov 8, 2023
During the deployment of Dataverse we check for conditions of the mail system that might not be done as people intend to use it.
We'll only issue warnings in the log messages, nothing critical here.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Feb 20, 2024
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Feb 20, 2024
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 25, 2024
pdurbin added a commit to poikilotherm/dataverse that referenced this issue Mar 25, 2024
pdurbin added a commit to poikilotherm/dataverse that referenced this issue Mar 25, 2024
pdurbin added a commit to poikilotherm/dataverse that referenced this issue Mar 26, 2024
pdurbin added a commit to poikilotherm/dataverse that referenced this issue Mar 26, 2024
@pdurbin pdurbin added this to the 6.2 milestone Mar 26, 2024
jeromeroucou pushed a commit to Recherche-Data-Gouv/dataverse that referenced this issue Sep 17, 2024
jeromeroucou pushed a commit to Recherche-Data-Gouv/dataverse that referenced this issue Sep 18, 2024
Without this change, the javamail maps would not be included in the artifact and trigger error messages in the logs about them being missed.

The error message will still be present as long as payara/Payara#6254 is
not fixed, released and we updated to a newer version of Payara.

(cherry picked from commit d650725)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Installation Guide Feature: Installer Feature: Messaging Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants