[prev in list] [next in list] [prev in thread] [next in thread] 

List:       jakarta-commons-dev
Subject:    [jira] Commented: (EMAIL-96) Email.getMailSession() is a mess
From:       "Siegfried Goeschl (JIRA)" <jira () apache ! org>
Date:       2010-10-31 16:24:24
Message-ID: 20306359.162971288542264898.JavaMail.jira () thor
[Download RAW message or body]


    [ 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("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.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


[prev in list] [next in list] [prev in thread] [next in thread]