jakartaee / mail-api

Jakarta Mail Specification project
https://jakartaee.github.io/mail-api
Other
247 stars 101 forks source link

UniqueValue should use UUID::randomUUID for boundary and messageid #460

Open sonersivri opened 4 years ago

sonersivri commented 4 years ago

Is your feature request related to a problem? Please describe. In our third party application uses to generate uniqueId with other than System user.name and we cannot set session property. If new System property jakarta.mail.user added it will ok

Describe the solution you'd like in jakarta.mail.internet.InternetAddress._getLocalAddress

        String user = null, host = null, address = null;
    if (session == null) {
            user = System.getProperty("jakarta.mail.user");
            if (user == null || user.length() == 0)
               user = System.getProperty("user.name");
        host = getLocalHostName();
    } else {
        address = session.getProperty("mail.from");
        if (address == null) {
        user = session.getProperty("mail.user");
        if (user == null || user.length() == 0)
            user = session.getProperty("user.name");
        if (user == null || user.length() == 0)
            user = System.getProperty("jakarta.mail.user");
        if (user == null || user.length() == 0)
            user = System.getProperty("user.name");
        host = session.getProperty("mail.host");
        if (host == null || host.length() == 0)
            host = getLocalHostName();
        }
    }

Describe alternatives you've considered No alternative solution

Additional context no additional context

jmehrens commented 4 years ago

What version of JavaMail/JakartaMail are you using? Unique ids for boundaries haven't included the username since JavaMail 1.5.3: The UniqueValue source code shows that the user is removed from the local address. https://github.com/eclipse-ee4j/mail/blob/master/mail/src/main/java/jakarta/mail/internet/UniqueValue.java

sonersivri commented 4 years ago

What version of JavaMail/JakartaMail are you using? Unique ids for boundaries haven't included the username since JavaMail 1.5.3: The UniqueValue source code shows that the user is removed from the local address. https://github.com/eclipse-ee4j/mail/blob/master/mail/src/main/java/jakarta/mail/internet/UniqueValue.java

1.4.7 version is used. as mentioned at header of this issue https://github.com/eclipse-ee4j/mail/blob/38da59413d50f0946127974116ef927c8e7aa210/mail/src/main/java/jakarta/mail/internet/InternetAddress.java#L599 InternetAddress class _getLocalAddress has this.

jmehrens commented 4 years ago

I think I'm missing additional context that should be placed in the description of this ticket. You provided the solution in the form of an implementation but I'm unable to reason about that implementation since there is not enough information about why you are needing to control the username this way. Can you provide more background on root problem, constraints we are operating under, and why they can't be changed? How is the session being created, is it null session, getDefaultInstance, or getInstance? What properties object is used when generating the session?

I'll do my best to answer you on what I can gather so far. Apologies if I'm way off track.

From the old JavaMail repo this what changed 1.5.3: https://github.com/javaee/javamail/commit/92d5ce7781ce2ed9f1b3fd290696066cc968f695#diff-43579024f2b1e29e49faa985d5264357

If your problem is leaking username in boundaries or messageid then simply replace your JavaMail artifact with a newer version and the userid is not included and therefore tweaking the user name is not needed because that part of the local address is not used in building the unique ids. If you can't upgrade JavaMail versions then any enhancement doesn't make sense because you would never be able to use the patch with you own enhancement. If you can upgrade and leaking the userid is your concern then this ticket should be closed as it is already fixed.

If the issue is you need to control the MimeMessage.setFrom() which relies on the local address computation then you should use the MimeMessage.setFrom(Address) method to construct a custom envelope. If you are just relying on creating local address you can create your own session and pass it to

If you need to control the user name because it is related to logging into the email server then that is a little more tricky.

  1. Create a matching account on the O/S and run your JVM under the user account. Doesn't work if you have more than one account or you can't create accounts.

  2. Not my favorite option but you can set the user name yourself using comand line or System.setProperty. See: https://stackoverflow.com/questions/29647529/override-message-id-by-configuration/

  3. The first caller of the getDefaultInstance is forever the single instance. Therefore you can construct a default instance before you start invoking your mailer methods and control what is going on. For instance:

Properties props = new Properties(System.getProperties()); //Inhert System properties (Yes/no?) Session.getDefaultInstance(props); .... props.put("user.name", "foo"); try { //do some mail action. finally { props.remove("user.name"); }

So even though you don't have control over the session directly you can control the properties that session will use by barging to the front of the line by creating them first.

sonersivri commented 4 years ago

zimbra uses this for generating unique headerid and we want to change header

jmehrens commented 4 years ago

Assuming I've found the correct product, it appears that Zimbra has an incomplete ticket from 2013 for upgrading to a newer version of JavaMail: https://bugzilla.zimbra.com/show_bug.cgi?id=84767

Unless there is a really good compatibility reason they should upgrade to the last official JavaMail version before the switch to JakartaMail. You should comment and vote on that issue.

As far as boundary lines go they have an issue for that: https://bugzilla.zimbra.com/show_bug.cgi?id=94875 It appears they have created their own MimeMessage subclass to do so which is the correct workaround.

Circling back to JakartaMail, in my mind it seems like Zimbra makes a good case for updating the format of the boundary lines and message-ids to use UUID. Assuming that is compatible with the various RFCs that seems like a reasonable enhancement. The orginal code wouldn't have used UUIDs because JavaMail supported JDK 1.4 for so long it was never revisited when we upgraded to support JDK 5 and later.

So we are getting closer to the root of this request. Do you need to hide the user name from the message id or are you wanting to use the user name to inject a specific token into the message id? If we just updated the message-id format to use a UUID would that solve your root problem?

https://github.com/eclipse-ee4j/mail/pull/435 as some interesting information on how custom message ids are approached in JakartaMail.

sonersivri commented 4 years ago

Yes message-id resolves our case. But if other users uses user.name property it will be problem for them.

jmehrens commented 3 years ago

@lukasj

I think the summary if this issue is that:

https://github.com/eclipse-ee4j/mail/blob/master/mail/src/main/java/jakarta/mail/internet/UniqueValue.java should consider replacing

(s.hashCode()).append('.').
      append(id.getAndIncrement()).append('.').
      append(System.currentTimeMillis()

with a call to: https://docs.oracle.com/javase/8/docs/api/java/util/UUID.html#randomUUID--

According to https://bugzilla.zimbra.com/show_bug.cgi?id=94875 other MTAs are already doing something similar. Optionally we could define a system property to toggle between the old format and the UUID format for compatibility. UUID uses SecureRandom and from what I can tell this shouldn't run into entropy pool running dry issues under the default JDK configuration as it is calling nextBytes.

If users are wanting to change the messageid that can be done by a few ways. If they want to change the part boundary that could be done by changing the content type.

If there is an interest in changing this I'll code up a pull request. Otherwise we should consider just closing this issue.

lukasj commented 3 years ago

@jmehrens

I think it makes sense to switch over to UUID.randomUUID() - both variants (current one as well as proposed one) seem to generate valid left part of the message-id.

Wrt a property for compatibility - what can go wrong after changing the way we generate message IDs? Only broken message threading in email clients is coming to my mind - and that's something driving me crazy, therefore I'd add a property but that can be just me. Is there anything else?