Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Fixed
-
1.2
-
None
Description
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 properties (java.util.PropertyPermission * read, write). This, despite the fact that only a handful of properties 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("someone@example.com", "Someone");
email.setFrom("me@example.com", "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.