155671 - When "forwarding inline" an encrypted message, enable encryption.

Status: VERIFIED FIXED

(MailNews Core :: Security: S/MIME, defect, P2)

Description

[Kai Engert [:KaiE:]]

Bug 137071 is the original bug that made the attempt to implement this feature.
However, the part to make the "forward inline" portion work gets backed out
because it caused regressions. Because bug 137071 already looks confusing
enough, we track the additional logic in this separate bug.

The purpose of this bug is:

When reading an encrypted message, and forwarding it inline, a new message
compose window will open. In the new opening window, the encryption pref should
be pre-set to "encrypt this message".

Comment 1

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

to correcly fix the problem, we need to pass the original message URI received
by the message compose service to mime draft. I have also fix a memory leak in
mime draft (mdd->url_name).

Comment 2

[Kai Engert [:KaiE:]]

I tested this patch (using a branch tree, having backed out 137071/154734
partially as done by 155638).

The attached patch works very good for me.

The inteded behaviour of 137071 works for all cases (reply/forward
attachment/forward inline)

It works for messages stored in 
- IMAP folders
- folders where the name of the folder contains a space
- "Local Folders"

Comment 3

[Kai Engert [:KaiE:]]

Opening Draft messages from IMAP/Local Folders works, too.

Comment 4

[Jean-Francois Ducarroz]

I tested successfully Edit message as new and Forward Inline from a newsgroup.

Naoki, can you give a try to be sure we don't have I18n issue with...

Comment 5

[nhottanscp]

I applied the patch to the trunk and tested for the charset problem, bug 155253.
I don't see the incorrect charset for forward inline and edit as new. 

But bug 155253 is already fixed on the trunk, so I removed my fix checked in
yesterday and tested again. The charset problem is fixed by the patch of this
bug. But with this patch only, the charset problem is not completely fixed (e.g.
after the first foward, the following reply/forward inline fail to set a correct
charset,  which exists in PR1 also). Anyway, the patch of this bug fixes the
recent regression of incorrect charset.

Comment 6

[Jean-Francois Ducarroz]

I've tested the patch on Mac and it works fine too.
Kaie, can you review the patch. Thanks

Comment 7

[Kai Engert [:KaiE:]]

Comment on attachment 90152 [details] [diff] [review]
Proposed fix, v1

Patch looks good to me. Good cleanup work, too.

r=kaie

Comment 8

[David :Bienvenu]

+  PR_FREEIF ( stream );
+  PR_FREEIF ( obj );
should be PR_Free, right? You don't care about nulling out obj and stream
because you're returning right away.

similarly, here:
+  PR_FREEIF(mdd->url_name)
+  PR_FREEIF(mdd->originalMsgURI)
   PR_Free (mdd);
since you're freeing mdd, I don't think you care if mdd->url_name is null or not
before you do that, so you can use PR_Free.

How come you can do this?
-  if (aMsgToReplace)
-     GetMsgDBHdrFromURI(msgURI, aMsgToReplace);
Did you make sure that saving a draft replaces the old copy of the draft?

Comment 9

[Jean-Francois Ducarroz]

>+  PR_FREEIF ( stream );
>+  PR_FREEIF ( obj );
>should be PR_Free, right? You don't care about nulling out obj and stream
>because you're returning right away.
>
>similarly, here:
>+  PR_FREEIF(mdd->url_name)
>+  PR_FREEIF(mdd->originalMsgURI)
>   PR_Free (mdd);
>since you're freeing mdd, I don't think you care if mdd->url_name is null or
>notbefore you do that, so you can use PR_Free.

right, I should be able to use PR_Free, I'll change that


>How come you can do this?
>-  if (aMsgToReplace)
>-     GetMsgDBHdrFromURI(msgURI, aMsgToReplace);
>Did you make sure that saving a draft replaces the old copy of the draft?

Using LXR, the parameter aMsgToReplace is alway passed as null therefore I
removed it (in fact I replaced it by the originalMsgURI). I tested draft
replacement and it works, the logic for that is during the save process, not in
mime.

Comment 10

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2

I addressed bienvenu's concerns by replacing PR_FREEIF by PR_Free

Comment 11

[Jean-Francois Ducarroz]

Comment on attachment 90229 [details] [diff] [review]
Proposed fix, v2

carrying over R=kaie

Comment 12

[David :Bienvenu]

Comment on attachment 90229 [details] [diff] [review]
Proposed fix, v2

+  PR_FREEIF(mdd->url_name)
+  PR_FREEIF(mdd->originalMsgURI)
these are still here

re this comment:
+  /* WARNING: You cannot use an c++ object in that structure which is
dependent on its constructor
+	    as mime_draft_data is not created using the new operator. COMPtr
however seems to survive
+	    to this limitation.
+  */
+
the comptr will not be released so it has to be done by hand - there is no
destructor for the struct.

Comment 13

[Jean-Francois Ducarroz]

the comptr is set to null manually when we free the structure. Therefore we are
fine. idealy, we should initialize the whole structure with "new" and free it
with "delete" but this is out of the scoop of this fix. I just add this warning
because I forget about it and tried to use a nsCString in the structure which
cause various wierd crash later.

Comment 14

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v3

fix those 2 PR_FREEIF I missed in the previous patch and modified the WARNING
comment.

Comment 15

[David :Bienvenu]

Comment on attachment 90249 [details] [diff] [review]
Proposed fix, v3

sr=bienvenu, thx.

Comment 16

[Jean-Francois Ducarroz]

Comment on attachment 90249 [details] [diff] [review]
Proposed fix, v3

carrying over R=kaie

Comment 17

[Jean-Francois Ducarroz]

Fixed in the trunk. Nominating nsbeta1

Comment 18

[Kai Engert [:KaiE:]]

Charles, Esther, can you please verify this on the trunk?
Now, that this bug has been fixed, the intended functionality of 137071 should
now work.

Comment 19

[Stephane Saux]

as per ADT meeting:
suggestions of what needs to be tested:
   - forward inline and as attachment.
   - reply inline and as attachment.
   - Source folder name has one or more of the following characteristics:
     - mixed-case.
     - spaces
     - other characters liable for url-encoding (e.g., "/:,+=") (not sure
whether such characters are allowed in folder names).
     - i18n names.
   - folder can be top-level or sub-folder.
   - edit draft functionality from above combination of folders.
   - edit message as new functionality from above combination of folders.

Comment 20

[Gayatri Rimola]

Laurel, Please verify using Stephane's test cases and update the bug with
comments. Charles will mark it VERIFIED when he is done with his testing. 

Comment 21

[nhottanscp]

cc iqa

Comment 22

[Kasumi]

verified on 07-02-07-1.0 on Linux RedHat7.1 JA

Comment 23

[nhottanscp]

The last comment was a verification for intl related issue only.

Comment 24

[nhottanscp]

The last comment was a verification for intl related issue only.

Comment 25

[Charles Rosendahl]

20020708 Trunk Builds: OSX, Linux7.3, and Win2k.

I've verified that the encryption characteristic of a message is preserved when
replying, forwarding as attachment, and forwarding inline.

I have tested drafts, templates, strangely named folders at both the top and
leaf level of the folder directory tree for both encrypted and non-encrypted
messages.

Everything appears to be working.  Once Laurel or someone in the mail group
confirms the same, we can mark this bug verified.

Comment 26

[laurel]

So far I've done the same things as Charles mentioned with july08 commercial
trunk on win98.  
The only thing I noticed so far: if you have saved a draft, closed the draft,
come back and use Edit Draft the composition window will show no signs of
encryption (no key icon and the Options|Security menu item will be set to "do
not encrypt this message).  However, once sent (without changing options
setting) the message received will show as encrypted.

Comment 27

[Charles Rosendahl]

Thanks Laurel,

Actually you mention a separate bug - right now drafts and templates aren't
fully supported/functioning in a consistent manner.

The problem is that we encrypt with the recipient cert, save as a draft, and
then later when we attempt to edit it, it is not possible to do so since it is
already encrypted with the recipient cert.

templates are also not fully functioning at the moment.  

I will file a separate bug specific to draft/template encryption - it has been a
known issue within our small circle that has never been noticed by anyone else
until now ;)  Not related to this specific bug though.

Other than that, just let me know when you guys think you are finished.

Comment 28

[laurel]

I tested with today's commercial trunk builds on linux rh6.2, mac OS 9.2 and
didn't see any problems other than the draft/template/edit as new issue I
previously mentioned. (I did notice with that issue that Mac operates a bit
differently and won't encrypt unless you set the option again.)

I'm done testing for this bug.

Comment 29

[Charles Rosendahl]

Verified.

Comment 30

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking in.

Comment 31

[chris hofmann]

Comment on attachment 90249 [details] [diff] [review]
Proposed fix, v3

a=chofmann for 1.0.1.  add the fixed1.0.1 keyword after checking in on the
branch.

Comment 32

[Jean-Francois Ducarroz]

Fix checked in the branch.

Comment 33

[Charles Rosendahl]

20020710 Branch Builds: OSX, Linux7.3, and Win2k.

I've verified that the encryption characteristic of a message is preserved when
replying, forwarding as attachment, and forwarding inline.

I have tested drafts, templates, strangely named folders at both the top and
leaf level of the folder directory tree for both encrypted and non-encrypted
messages.

Everything appears to be working.  Once someone in the mail group
confirms the same, we can mark this bug verified1.0.1.

Comment 34

[laurel]

So far, so good. OK using july10 commercial branch, win98.
Still need to check other platforms.

Comment 35

[Charles Rosendahl]

On second time around, Laurel mentioned something to me that I took another look at.

Forwarding encrypted messages inline on the Mac (osX, 9.2) creates a p7m
attachment, and the body of the forwarded inline message is not readable. 
Forwarding as an attachment on Mac works.

Comment 36

[Jean-Francois Ducarroz]

That's a separate issue. Do we  have a bug for that?

Comment 37

[Kai Engert [:KaiE:]]

Yes, that's a separate issue, bug 141730.

Comment 38

[Charles Rosendahl]

comment 37 refers to the draft/template problem.

Bug 156958 covers the Mac bug forwarding inline encrypted messages.

Marking this verified1.0.1