Email.getMailSession() is a mess
--------------------------------
Key: EMAIL-96
URL: https://issues.apache.org/jira/browse/EMAIL-96
Project: Commons Email
Issue Type: Bug
Affects Versions: 1.2
Reporter: NC
Email.getMailSession() is a mess
This bug initially was going to address only the excessive permissions required
by Email.getMailSession(). My examination of how this might best be remedied
led me to the revelation that the problems with this method, and their effects
on the entire class are quite extraordinary. (To be fair, some of the blame
should be placed squarely upon other members of this class.)
Excess permission requirements
--------------------------------------------
The Email.getMailSession() method, when running under a SecurityManager,
requires permission to be granted to read and write ALL permissions
(java.util.PropertyPermission * read, write). This, despite the fact that only
a handful of permissions are applicable to sending mail.
The permission requirement stems from the following line:
Properties properties = new Properties(System.getProperties());
As far as I can tell, Commons-Email reads only "mail.smtp.from" and
"mail.smtp.host" from the system properties for use as default values.
Commons-Email does not appear to write any system properties.
Therefore, something along these lines would greatly reduce the permissions
required to send mail using Commons-Email:
Properties properties = new Properties();
properties.setProperty(MAIL_SMTP_FROM,
System.getProperty(MAIL_SMTP_FROM));
properties.setProperty(MAIL_HOST, System.getProperty(MAIL_HOST));
On top of all of this, getMailSession() does not use
AccessController.doPrivileged() to isolate the code requiring the permission
grants.
My thought was to submit a patch refactoring the class, adding a
createDefaultProperties() method to do all of this.
But then...
--------------
During my examination of the code, I noticed that getMailSession() is
documented with the following JavaDoc comment:
Initialise a mailsession object
That would be an appropriate description for an initMailSession() method. For
getMailSession(), I would have expected something more along the lines of:
Determines the Session used when sending this Email, creating the Session if
necessary.
(I would also refactor the class such that getMailSession() delegates creation
and initialization of a mail Session to a createMailSession() method.)
However, buildMimeMessage() *does* treat getMailSession() as though it is
initMailSession(), e.g.:
this.getMailSession();
this.message = this.createMimeMessage(this.session);
One might have expected a simple createMimeMessage(getMailSession()), rather
than using getMailSession() for its side effect and then accessing the data
member directly. A bit confusing, but it got me thinking.
Suppose I do something like this:
System.getProperties().setProperty("mail.smtp.host",
"smtp.example.com");
Email email =new SimpleEmail();
email.getMailSession();
email.setHostName("mail.example.com");
email.setSmtpPort(26);
email.setSsl(true);
email.addTo("[email protected]", "Someone");
email.setFrom("[email protected]", "Me");
email.setSubject("Test");
email.setMsg("This is a test.");
email.send();
Q. What SMTP server is contacted to relay the mail?
A. smtp.example.com
Q. On what port is the SMTP server contacted?
A. 25
Q. Is SSL used when communicating with the SMTP server?
A. No.
Q. What mail host and port will be reported in any error message?
A. mail.example.com: 26
The problems here are:
a) The call to getMailSession() initializes the Session used by Email
b) subsequent setXxxx() calls don't write through to the already-initialized
Session
c) any EmailException thrown by sendMimeMessage() uses values obtained from
the getter methods, which prefer local data members rather than Session
properties
I suppose one could argue that getMailSession(), despite its misleading name,
clearly states that it initializes the Session, thereby sealing the attributes
despite subsequent setXxxx() calls. In that case the JavaDoc comments should
clearly note this. That would leave Commons Email with a confusing and stinky
API. Problem 'c' listed above would still need to be addressed.
How truly fixable this class is without breaking the API is probably dependent
upon whether javax.mail.Session accepts changes written to the Properties
object returned by its getProperties() method. The JavaMail API does not seem
to specify that behavior.
I will cheerfully accept any corrections, major or minor. I imagine anyone
reading this has spent far more time with the code involved than I have.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
