When a message is received that was encrypted using S/MIME, any reply to that message should also default to "Always encrypt" or "Encrypt if possible". Currently this has to be set manually. This was the normal action for Communicator 4, and is also true of other mail clients. I consider this fairly important as it could lead a user to respond in plaintext.

Looks like a dupe of bug 29753

I don't believe it is. 29753 is to do with replying to multiple recipients where a certificate for one of them is not held. This bug is to do with the default setting of the UI being "No Encryption" when you hit reply.

Grouping related bugs. These bugs, whether it be compose, reply, forward, single/multiple recipients, need a whenever possible option for encryption. Additionally, 4.7x and OE allow the user the option to send the message unencrypted if one of the intended recipients does not have a cert available to the sender at the time of composition. The interaction should be: under mail&newsgroup Account settings: - never, whenever possible, always (currently only never, always) within composition: Upon sending the message, a check, then dialog (for whenever possible/always options) -list of people missing certs (instead of the current one at a time dialogs) -option to send unencrypted

Additionally, there are some workflow/logistics that need to be defined in the algorithms as pointed out in the defect. In addition to set preferences, if a message comes in signed and encrypted, then replies should be encrypted as well. Forwarding follows a different, yet similar path, depending on the nature of the message being forwarded.

*** Bug 140163 has been marked as a duplicate of this bug. ***

regression from 4.7x functionality. - added regression keyword. Raising severity (not priority) as this will be a common complaint of users using smime capabilities.

kai

I believe it is not a regression. With 4.7x, if your default is to send non-encrypted, when replying to an encrypted message, the new compose window will not have the encryption option enabled. I just tested.

There is an exception to this bug. ========= Imagine you have received an encrypted message, but you are unable to decrypt. Maybe it's an archived message, and you don't have the private key anymore. Maybe somebody else is still using an old certificate to encrypt, for which you lost the matching private key. Maybe you are reading the message on a machine, where you don't have your private key installed. In all those situations, you will see a broken encryption icon. You will not see the message contents. The user might want to reply, maybe telling the other person, reading the message was not possible. What should happen? Some time back, Stephane suggested, sending encrypted is not required in that situation.

adding myself and mscott to cc list. I'd like mscott to review, if he can, since he's much more familiar with this code. The patch is large, but it's not as scary as it looks. I think (blasphemy coming up) that using pldhash is a bit of an overkill, because we're hardly ever going to have more than one entry to keep track of so a simpler data structure might be the way to go.

Comment on attachment 88040 [details] [diff] [review] Suggested Implementation in msgCompSMIMEOverlay.js: gEncryptedURIService is not checked for null before beeing used! that could result is the abort of the JS and potentially miss imortant stuff. Apart that, looks good. R=ducarroz

This patch adds the null check, as requested. Thanks for catching this!

Comment on attachment 88155 [details] [diff] [review] Patch v2 carrying forward r=ducarroz

Comment on attachment 88155 [details] [diff] [review] Patch v2 In msgHdrViewSMIMEOverlay.js, the variable gMyLastEncryptedURI seems superfluous, we only use it in one place... you could probably just do: gEncryptedURIService.rememberEncrypted(GetLoadedMessage()); then get rid of gMyLastEncryptedURI. David mentioned that the hash table seems like overkill in nsEncryptedsMIMEURIsService, I tend to agree. Most users only have a single 3-pane open which means there would be only a single entry in this table. Even if a user has multiple 3-panes and multiple stand alone windows open, we are still talking about 3 or 4 entries IMHO. I bet it would simplify your patch quite a bit from an ADT point of view if you just used a void array of msg URIs. Other than that, everything looks good.

Ok, you convinced me to use a simpler datastructure. I assume you recommended nsCStringArray (not nsVoidArray), because I have to find the entries by string. I'm attaching a new patch. The only change is the implementation of the service, which is now much simpler. While the code is now shorter, it is a little bit slower, if I understand correclty, because every time we insert, remove or lookup, we create an additional copy of the input string. That is necessary because the type used by nsCStringArray is incompatible with the type used when defining the interface to use AUTF8String - this type was suggested to be used for URIs by dmose. The new patch is 125 lines shorter.

Comment on attachment 88185 [details] [diff] [review] Patch v3 r=ducarroz

Comment on attachment 88185 [details] [diff] [review] Patch v3 sr=mscott

Checked in to trunk, marking fixed. I did not check in the patch exactly as reviewed. Before I checked in, I realized that one filename was too long for Mac.

Comment on attachment 88302 [details] [diff] [review] Patch v4, slightly corrected version for Mac compatibility carrying forward r=ducarroz and sr=mscott

Looking at the patch, this bug only applies to replies to encrypted messages, not forwarding, correct?

No, it should also work with forwarding. Both when replying and forwarding, the URI of the original message gets passed on to the new compose window.

Reopening bug. Unfortunately, the patch does not yet cover the case "forward inline".

Case #0: Forwarding encrypted messages is not defaulting to encrypt when the forward message is inline. Case #1: Hitting reply all to the encrypted message defaults to encrypt. Sending succeeds. However, none of the recipients are able to read the message (broken key icon). I created a message and sent it to John, Jimmy, and myself encrypted I deleted their certs after sending the orignial message and restarted the browser bfore replying. I replied all to the message. The default was set to encrypt. The message seemed to go out successfully encrypted. They both replied back that they were not able to decrypt the message. Case #2: I did not have a recipient cert for Kaie in my Cert Manager. I created an encrypted mail message to myself. I replied all to the message. I added Kaie to the list of recipients (in this order: carosendahl,kaie) the message sent successfully encrypted (supposedly). However, If I hit reply all to the message, made kaie (invalid recipient) the first user in the address list, the failure is detected and the message is not sent. (in this order: kaie, carosendahl) Case #1 and #2 probably need a separate bug filed against it so that we can fix it should it exist in the branch now as well, since I'm not sure if this is the patch that caused this problem.

This patch is not from me, it is from ducarroz, who was kind enough to write it for me. The problem is: When forwarding inline, the "original message uri" param does not get set. But the other patch in 137071 requires it to be set. This patch sets the param for inline forwarded messages, too.

Comment on attachment 88571 [details] [diff] [review] Additional patch I tested this patch. It works and fixes the forward inline problem. I reviewed this patch. It looks good to me. r=kaie

Comment on attachment 88571 [details] [diff] [review] Additional patch sr=mscott

Additional patch has been checked in to trunk. Marking fixed again.

Charles, in reply to your comment 25: case #0: Now fixed by the additional patch, can you please verify? (builds starting 2002-06-21-08-trunk should have the fix) case #1: Thanks for finding that nasty bug, it is a separate problem. I filed 153243 and it's meanwhile fixed. case #2: Is the same as case #1

adding adt1.0.1 keyword. Charles is verifying right now.

Verified on 0621 Trunk builds. Forwarded inline, as attachment, reply, reply all. Original and additional recipients used in replies. Original, subset of original, and new recipients used in forwarding of mail messages.

adt1.0.1 (on ADT's behalf) approval for checkin to the 1.0 branch, pending drivers' approval. pls check this in asap, then add the "fixed1.0.1" keyword.

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.

Comment on attachment 88571 [details] [diff] [review] Additional patch Marking a=valeski He gave me approval online in a chat, and we were talking about both patches.

Both patches checked in to 1_0 branch.

Same tests as comment #32 on the 06/25 branch builds.

I believe the fix to this bug has regressed http://bugzilla.mozilla.org/show_bug.cgi?id=154734.

Heads Up: I think I found two more problems related to this bug. I will file bugs soon, after I know more details. Issue 1: I have one profile with one message, only with that particular message I see a problem. When I forward that message, in the opening window encryption is not enabled. I need to analyze why. Issue 2: Copy an encrypted message to one of your "Local Folders". Open that message from there. Do "Forward Inline". No composition window opens, but an error message is shown instead.

Issue 2 is probably related to the space in "Local Folders". happens for all messages, not just signed/encrypted messages.

this fix cause bug 155476

other regression caused by this fix: bug 155253

CC'ing ducarroz FYI. In response to my own comment 39, and to Charles follow up comment 40, only talking about issue #2, the error message: - I'm not sure yet whether it is indeed caused by the space in the folder name, because I had the space fix from bug 154734 applied when testing. - I confirm that it happens for any message, not only encrypted messages. - It only happens when forwarding inline. - I believe what I see is bug 155476, because I see the same error message as reported in that bug, and JF told me, forwarding inline works by using the same logic as opening a draft message. - trying to understand what causes the problem, I tried to back out the patch from this bug named "Additional Patch" (attachment 88571 [details] [diff] [review]), and after backing it out, I no longer see my problem #2. I will have a closer look at what is happening, and will comment in bug 155476. One hint I have already is, the debug version shows the following warning in addition to the UI error message: ARNING: malformed hostname, file /home/kaie/100/mozilla/netwerk/base/src/nsURLParsers.cpp, line 574 WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /home/kaie/100/mozilla/mailnews/local/src/nsMailboxService.cpp, line 568 WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /home/kaie/100/mozilla/mailnews/compose/src/nsMsgCompose.cpp, line 1454 WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /home/kaie/100/mozilla/mailnews/compose/src/nsMsgComposeService.cpp, line 645

Follow up to my comment 39, issue 1 - the problem that the original implemented functionality does not work for one particular message in a POP3 folder. I think what I see is caused by the fix introduced with bug 154734. In the forward inline case, the URI that S/Mime logic remembers has a different escaping from what will be seen later during message compose init. Therefore, we do not find the URI in our lookup table. Note that bug 154734 has caused regression bug 155476. Anyway, if we continue to stay with the patch from bug 154734, we need an additional patch for bug 137071. In this small additional patch, we need to achieve escaping consistency in what we store and lookup in our internal S/Mime URI table.

Another issue. Because of bug 155476 (caused by 154734), you currently can not see the following problem. You need a build which has the patch from 154734 backed out, or which is from a date before 154734 was checked in. I found the new bug 155513. Because of bug 155513, the intended behaviour of this bug 137071 does not yet work when forwarding inline a message stored in "Local Folders". In order to fix 137071 completely, we need to either fix bug 137071, or must use an additional patch for 137071, in order to ignore the upper/lowercase of mail URIs we remember in our table.

> In order to fix 137071 completely, we need to either fix bug 137071, or must use > an additional patch for 137071, in order to ignore the upper/lowercase of mail > URIs we remember in our table. This should have said: In order to fix 137071 completely, we need to either fix bug 155513, or must use an additional patch for 137071, in order to ignore the upper/lowercase of mail URIs we remember in our table.

>we need to achieve escaping consistency in what we store and lookup in our >internal S/Mime URI table. Kaie, can you give me example of escaping inconsistency?

JF: isn't bug 155513 what Kai is talking about?

bug 155513 is about capitalization, not escaping problem!

bug 155638 has been file to partially backup this fix on the branch until we get the correct fix.

> Kaie, can you give me example of escaping inconsistency? Let me explain what consistency would be: We only have "escaping consistency" if all S/Mime code dealing with message URIs can expect that the URI is - either always escaped - or never escaped Only if the S/Mime code knows in advance, that the "URI is escaped" property is always true or always false, it can store the URIs directly in its lookup table, without having to "normalize" it first. Right now, the 137071 S/Mime code does not try to normalize the escaping, and if an URI is sometimes escaped, but sometimes it is not, the lookup fails, because the string comparison in the lookup table fails. So, we need to know the answer to this questions: 1) Is the URI stored in originalMsgURI - always escaped - always not escaped - sometimes escaped, sometimes not escaped 2) Is the URI returned by GetSelectedMessage() always in the same escaping format as the URI as it is seen in gMsgCompose.originalMsgURI?

We opened bug 155671 to track the work required to re-implement the logic to fix the forward inline case.

kai, did 137071 really cause regressions or did it expose fundamental design problems and defects at another level? From everything that I looked at and have tested, I still think that your fix was correct with the side effect of exposing the underlying escaping/unescaping inconsistencies. No mind - I take it 155671 will find a solution all the way around post RTM or pre RTM?

Charles, I don't think, nor have I heard anybody say, that there are "fundamental design problems". Just defects here and there in the implementation. We may have a partial 137071 functionality for RTM. If Kai and JF can come up with a fix that resolves the issues, and that patch passes the smell test, adt will look at it. Otherwise, we'll have a great solution for post-RTM.