commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "NC (JIRA)" <j...@apache.org>
Subject [jira] Updated: (EMAIL-96) Email.getMailSession() is a mess
Date Mon, 28 Jun 2010 03:10:50 GMT

     [ https://issues.apache.org/jira/browse/EMAIL-96?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

NC updated EMAIL-96:
--------------------

    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.

  was:
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 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("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.


> 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 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.


Mime
View raw message