OFBiz
  1. OFBiz
  2. OFBIZ-3379

Email sending process using one connection for To/CC/BCC causing issues

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: Release Branch 09.04, Trunk
    • Fix Version/s: None
    • Component/s: framework
    • Labels:
      None

      Description

      Typically BCCs are handled via the sending mail client. That is, when the client sees a BCC in an email, it will open up two connections to the mail server, the first for the To/CC fields, the second for BCC fields, this way the addresses are masked from the headers and there is that layer of anonymity that BCC is used for.

      What appears to be happening is that OFBiz is sending all of the information in one connection to the mail server and having the mail server sort out the details. So when sendTo encountering an invalid email, and then terminating the remaining execution of the outgoing process and no email sent to BCC address which is usually going to be a valid address from email settings for the company.

      To fix the issue, we need to send this via two connection to mail client.

      1. OFBIZ-3379-2.patch
        4 kB
        Scott Gray
      2. OFBIZ-3379.patch
        5 kB
        Pranay Pandey
      3. OFBIZ-3379.patch
        8 kB
        Scott Gray
      4. OFBIZ-3379.patch
        9 kB
        Scott Gray

        Activity

        Hide
        Pranay Pandey added a comment -

        Here is the patch for review.

        Show
        Pranay Pandey added a comment - Here is the patch for review.
        Hide
        Ruth Hoffman added a comment -

        I'm wondering why you would want to send the bcc if the original failed. If your intention is to keep a record of success or failure of the email event - then isn't this better done somewhere else? IMO, BCC were never intended to be used as email telemetry.

        Please do not make this change. If desired, make this a configuration option....

        Show
        Ruth Hoffman added a comment - I'm wondering why you would want to send the bcc if the original failed. If your intention is to keep a record of success or failure of the email event - then isn't this better done somewhere else? IMO, BCC were never intended to be used as email telemetry. Please do not make this change. If desired, make this a configuration option....
        Hide
        Tim Ruppert added a comment -

        Ruth, read the comment above. It's from a senior engineer at one of the top hosting companies in the country - in response to a problem that has been reported by a customer who's really using OFBiz. Customers need to ALWAYS get their correspondence - especially for sales - regardless of what email address the customer puts in.

        This does not mean that we shouldn't look at a better way of tracking what happens with the To and CCs - because those are visible to everyone that is receiving the email. BCCs are not and should be treated differently - with multiple connections.

        Show
        Tim Ruppert added a comment - Ruth, read the comment above. It's from a senior engineer at one of the top hosting companies in the country - in response to a problem that has been reported by a customer who's really using OFBiz. Customers need to ALWAYS get their correspondence - especially for sales - regardless of what email address the customer puts in. This does not mean that we shouldn't look at a better way of tracking what happens with the To and CCs - because those are visible to everyone that is receiving the email. BCCs are not and should be treated differently - with multiple connections.
        Hide
        Ruth Hoffman added a comment -

        Tim,
        The comment about BCC anonymity is valid. And for that reason I now agree using alternate connections to send BCCs is a good idea. But, let me remind you, that was not the original intent of this change. We were discussing failed email delivery and not BCC anonymity.

        Show
        Ruth Hoffman added a comment - Tim, The comment about BCC anonymity is valid. And for that reason I now agree using alternate connections to send BCCs is a good idea. But, let me remind you, that was not the original intent of this change. We were discussing failed email delivery and not BCC anonymity.
        Hide
        Scott Gray added a comment -

        I've been thinking about this the last couple of days and here are thoughts.

        I believe BCC handling is the responsibility of the underlying JVM implementation and not of the API user.  The BCCs are correctly blinded using the current implementation and I don't think we need to change that.

        At a high level, emails can fail in one of two ways: 

        • they can be rejected outright by the initial smtp server 
        • they can subsequently be bounced further down the chain after having been initially accepted.  At this point the message is bounced per failed recipient and will still be delivered to any valid ones.

        I think our issue is that if a message is rejected by the smtp server then there is no feedback provided by the system (the comm event may be marked failed, I haven't checked).  With a regular mail client, direct feedback is provided in the form of some type of error message and email remains in the outbox or in draft state until the user rectifies the problem.  

        I would like to suggest that instead of sending the bcc regardless, we should send a failure message to the "from" email address with the reason for the failure.  If the comm event isn't marked as failed then we should also change the logic to do so (we could possibly link to the comm event from the failure notification to allow the user to rectify the situation easily).  The failure message logic should probably reside at a higher level than the sendmail service so that it can be templated and even turned off if needed (an example might be a webmail client where we have the ability to provide direct feedback).

        If everyone feels this approach is suitable then I'll take a closer look at the code tomorrow as see how we might go about implementing it.  

        Show
        Scott Gray added a comment - I've been thinking about this the last couple of days and here are thoughts. I believe BCC handling is the responsibility of the underlying JVM implementation and not of the API user.  The BCCs are correctly blinded using the current implementation and I don't think we need to change that. At a high level, emails can fail in one of two ways:  they can be rejected outright by the initial smtp server  they can subsequently be bounced further down the chain after having been initially accepted.  At this point the message is bounced per failed recipient and will still be delivered to any valid ones. I think our issue is that if a message is rejected by the smtp server then there is no feedback provided by the system (the comm event may be marked failed, I haven't checked).  With a regular mail client, direct feedback is provided in the form of some type of error message and email remains in the outbox or in draft state until the user rectifies the problem.   I would like to suggest that instead of sending the bcc regardless, we should send a failure message to the "from" email address with the reason for the failure.  If the comm event isn't marked as failed then we should also change the logic to do so (we could possibly link to the comm event from the failure notification to allow the user to rectify the situation easily).  The failure message logic should probably reside at a higher level than the sendmail service so that it can be templated and even turned off if needed (an example might be a webmail client where we have the ability to provide direct feedback). If everyone feels this approach is suitable then I'll take a closer look at the code tomorrow as see how we might go about implementing it.  
        Hide
        Tim Ruppert added a comment -

        Well, this doesn't solve the problem of the person who's on the BCC receiving the message regardless of the To/Cc - which I've checked with a number of different email programs and I ALWAYS get the BCC - even if the To and CC do not. I also get the message that Scott's talking about as well. So, I'm down with changing this to:

        1. Still sending it to the BCC person - since this is how all email sending programs seem to work.
        2. AND doing what Scott suggested and sending a failure message to the From.

        This is how all email clients seem to work - as well as the fact that it improves the interface for the From message senders on all messages.

        Show
        Tim Ruppert added a comment - Well, this doesn't solve the problem of the person who's on the BCC receiving the message regardless of the To/Cc - which I've checked with a number of different email programs and I ALWAYS get the BCC - even if the To and CC do not. I also get the message that Scott's talking about as well. So, I'm down with changing this to: Still sending it to the BCC person - since this is how all email sending programs seem to work. AND doing what Scott suggested and sending a failure message to the From. This is how all email clients seem to work - as well as the fact that it improves the interface for the From message senders on all messages.
        Hide
        Ruth Hoffman added a comment -

        Hello All:
        IMO this issue is a great example of determining, in advance, what problem you are trying to solve. Or, if there is even a problem. What you "believe" and what happens to be true...well...may not be the same. Still looks to me like we shouldn't change the email service, just fix the notification process.

        Show
        Ruth Hoffman added a comment - Hello All: IMO this issue is a great example of determining, in advance, what problem you are trying to solve. Or, if there is even a problem. What you "believe" and what happens to be true...well...may not be the same. Still looks to me like we shouldn't change the email service, just fix the notification process.
        Hide
        Scott Gray added a comment -

        Attaching a patch which generates a delivery failure notification which along with r894236 should resolve this issue.

        Pranay, I'm unable to test this patch because I couldn't reproduce the environment in which the SendFailedException occurs. Could you either test the patch for me or otherwise let me know (offline) how to setup my system in order to reproduce the problem?

        Thanks
        Scott

        Show
        Scott Gray added a comment - Attaching a patch which generates a delivery failure notification which along with r894236 should resolve this issue. Pranay, I'm unable to test this patch because I couldn't reproduce the environment in which the SendFailedException occurs. Could you either test the patch for me or otherwise let me know (offline) how to setup my system in order to reproduce the problem? Thanks Scott
        Hide
        Pranay Pandey added a comment -

        Thanks Scott, I will test the patch uploaded by you and will post the results here.

        Show
        Pranay Pandey added a comment - Thanks Scott, I will test the patch uploaded by you and will post the results here.
        Hide
        Scott Gray added a comment -

        Updated patch for testing, prevents infinite recursion when the failure notification also fails to be sent

        Show
        Scott Gray added a comment - Updated patch for testing, prevents infinite recursion when the failure notification also fails to be sent
        Hide
        Scott Gray added a comment -

        If there are no objections I'll likely commit this patch in the next day or so, here's are summary of the changes in case anyone hasn't been following closely:

        • Emails will now be sent to any valid recipients even if the SMTP server rejected any invalid ones. This can be turned off if desired in general.properties and also on a per service call basis.
        • A failure notification will be sent to the email's "from" address, listing the failed recipients and the reason for each failure. The notification can be turned off by setting the sendFailureNotification parameter to false in the service context. If a failure notification is sent then the service will return success even if there were failures (this prevents async calls from retrying and multiple failure notifications being sent).
        Show
        Scott Gray added a comment - If there are no objections I'll likely commit this patch in the next day or so, here's are summary of the changes in case anyone hasn't been following closely: Emails will now be sent to any valid recipients even if the SMTP server rejected any invalid ones. This can be turned off if desired in general.properties and also on a per service call basis. A failure notification will be sent to the email's "from" address, listing the failed recipients and the reason for each failure. The notification can be turned off by setting the sendFailureNotification parameter to false in the service context. If a failure notification is sent then the service will return success even if there were failures (this prevents async calls from retrying and multiple failure notifications being sent).
        Hide
        Tim Ruppert added a comment -

        Ruth, this is a good example of there being a REAL problem and addressing that problem directly - which, in the real world, is how bugs are found and problems are solved - real customers, real problems - not just trying to think like someone else might be thinking.

        Show
        Tim Ruppert added a comment - Ruth, this is a good example of there being a REAL problem and addressing that problem directly - which, in the real world, is how bugs are found and problems are solved - real customers, real problems - not just trying to think like someone else might be thinking.
        Hide
        Tim Ruppert added a comment -

        Scott, thanks for taking the extra time to abstract all of this so that everyone can have anything configured however they like. Please commit this fix so that the issues this has caused can be fixed for the entire community.

        Show
        Tim Ruppert added a comment - Scott, thanks for taking the extra time to abstract all of this so that everyone can have anything configured however they like. Please commit this fix so that the issues this has caused can be fixed for the entire community.
        Hide
        Ruth Hoffman added a comment -

        Tim:
        Really? And what was that problem? Email wasn't "broken". The process by which OFBiz sends notification - using email - does not work the way the original developers had intended.

        On another note: Don't waste your time presuming to lecture me on problem solving techniques. I've been working with computers and software since before Al Gore invented the internet if, on the other hand you feel compelled to vent because you don't agree with me, well then by all means - go for it!

        Show
        Ruth Hoffman added a comment - Tim: Really? And what was that problem? Email wasn't "broken". The process by which OFBiz sends notification - using email - does not work the way the original developers had intended. On another note: Don't waste your time presuming to lecture me on problem solving techniques. I've been working with computers and software since before Al Gore invented the internet if, on the other hand you feel compelled to vent because you don't agree with me, well then by all means - go for it!
        Hide
        Tim Ruppert added a comment -

        Read the subject and description of the error - they describe EXACTLY what the real world problem was and provide a solution for it. Thru the community process, and as a result of one of your comments, the original fix was improved to have more flexibility to allow people to configure if they don't want to get emails addressed to them (if there are other issues). So, as I stated earlier, the way that OFBiz sends notifications WAS broken for the way many customers use it - and yes it has now been fixed.

        On your other note - just because you've been doing it for a long time doesn't mean that you do it right - not now and no guarantee about then. But, at least you're now putting comments related to JIRA tasks in JIRA - instead of in email - so I guess we can all take that as a bonus and moving in the right direction together! Just follow the processes and be a part of intelligent communication in the projects and everyone will continue to be happy with your contributions.

        Show
        Tim Ruppert added a comment - Read the subject and description of the error - they describe EXACTLY what the real world problem was and provide a solution for it. Thru the community process, and as a result of one of your comments, the original fix was improved to have more flexibility to allow people to configure if they don't want to get emails addressed to them (if there are other issues). So, as I stated earlier, the way that OFBiz sends notifications WAS broken for the way many customers use it - and yes it has now been fixed. On your other note - just because you've been doing it for a long time doesn't mean that you do it right - not now and no guarantee about then. But, at least you're now putting comments related to JIRA tasks in JIRA - instead of in email - so I guess we can all take that as a bonus and moving in the right direction together! Just follow the processes and be a part of intelligent communication in the projects and everyone will continue to be happy with your contributions.
        Hide
        Scott Gray added a comment -

        Thanks all, fixed in trunk r896213

        Show
        Scott Gray added a comment - Thanks all, fixed in trunk r896213
        Hide
        Scott Gray added a comment -

        Pranay, if you get a chance could you please test this patch which addresses Adam's concerns about using Sun's SMTP classes? No rush, whenever you have time would be great.

        Thanks
        Scott

        Show
        Scott Gray added a comment - Pranay, if you get a chance could you please test this patch which addresses Adam's concerns about using Sun's SMTP classes? No rush, whenever you have time would be great. Thanks Scott
        Hide
        Pranay Pandey added a comment -

        Sure Scott, I will test it and will get back to you soon.

        Thanks

        Pranay Pandey

        Show
        Pranay Pandey added a comment - Sure Scott, I will test it and will get back to you soon. Thanks – Pranay Pandey
        Hide
        Pranay Pandey added a comment - - edited

        Hi Scott,

        Sorry for late reply.
        Thanks for the patch. I tested the patch given by you and it worked well except one issue that I found while calling sendMailMultiPart(in sendFailureNotification method) with newContext in which missing bodyPart gives service validation error. This is from the last patch on the issue OFBIZ-3379-2.patch.

        Please let me know if you have any questions.

        Show
        Pranay Pandey added a comment - - edited Hi Scott, Sorry for late reply. Thanks for the patch. I tested the patch given by you and it worked well except one issue that I found while calling sendMailMultiPart(in sendFailureNotification method) with newContext in which missing bodyPart gives service validation error. This is from the last patch on the issue OFBIZ-3379 -2.patch. Please let me know if you have any questions.
        Hide
        elan added a comment -
        1. handle BCC via another connection
          In RFC, the BCC shall be handled at MX (SMTP server). SMTP server will remove the BCC header while deliver the mail.
          As a client, it has no need to handle BCC via another connection.
        2. failure notification
          SMTP server will send failure message to client, client shall notice the sender/from .
          But for sender/from , BCC is not secret, so it will be sufficiently to handle it the same as TO/CC .
        Show
        elan added a comment - handle BCC via another connection In RFC, the BCC shall be handled at MX (SMTP server). SMTP server will remove the BCC header while deliver the mail. As a client, it has no need to handle BCC via another connection. failure notification SMTP server will send failure message to client, client shall notice the sender/from . But for sender/from , BCC is not secret, so it will be sufficiently to handle it the same as TO/CC .

          People

          • Assignee:
            Scott Gray
            Reporter:
            Pranay Pandey
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development