Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2
    • Fix Version/s: 1.3
    • Labels:
      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.

        Activity

        Hide
        Siegfried Goeschl added a comment -

        Well, the confusing and stinky API has hopefully some value ...

        +) the assumption that javax.mail.session will honor those changes is risky
        +) I will have a look at the remaining points in two weeks time

        Show
        Siegfried Goeschl added a comment - Well, the confusing and stinky API has hopefully some value ... +) the assumption that javax.mail.session will honor those changes is risky +) I will have a look at the remaining points in two weeks time
        Hide
        Siegfried Goeschl added a comment -

        It took more than two weeks but some points you mentioned are rather difficult

        +) The underlying JavaMail use more than the to system properties but the relevant properies are prefixed with "mail" - not sure if that helps your security manager problem - we are talking about thirty different properties and different mail providers might add their own.

        +) can you help with AccessController.doPrivileged()?! I have no experience with that and also no way to test it

        +) I updated the javadoc of getMailSession()

        +) change MimeMessage creation to "this.createMimeMessage(this.getMailSession())"

        Show
        Siegfried Goeschl added a comment - It took more than two weeks but some points you mentioned are rather difficult +) The underlying JavaMail use more than the to system properties but the relevant properies are prefixed with "mail" - not sure if that helps your security manager problem - we are talking about thirty different properties and different mail providers might add their own. +) can you help with AccessController.doPrivileged()?! I have no experience with that and also no way to test it +) I updated the javadoc of getMailSession() +) change MimeMessage creation to "this.createMimeMessage(this.getMailSession())"
        Hide
        Siegfried Goeschl added a comment -

        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.

        Show
        Siegfried Goeschl added a comment - 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.

          People

          • Assignee:
            Siegfried Goeschl
            Reporter:
            NC
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development