[
https://issues.apache.org/jira/browse/EMAIL-96?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12926731#action_12926731
]
Siegfried Goeschl commented on EMAIL-96:
----------------------------------------
Throwing an IllegalStateException when the user tries to modify mai session
settings when the mail session is already supplied or created. This might break
invalid but existing code.
> 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
> Assignee: Siegfried Goeschl
> Fix For: 1.3
>
>
> 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("[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.
