Closed Bug 155671 Opened 22 years ago Closed 22 years ago

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

Categories

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

Product:
MailNews Core
Component:
Security: S/MIME
Version:
Other Branch
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: KaiE, Assigned: bugzilla)

References

Details

(Whiteboard: [adt2 rtm])

Attachments

(1 file, 2 obsolete files)

Proposed fix, v3
16.00 KB, patch
bugzilla
: review+
Bienvenu
: superreview+
chofmann
: approval+
Details | Diff | Splinter Review 
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".
Depends on: 137071
Attached patch Proposed fix, v1 (obsolete) — — Splinter Review 
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).
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"

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

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...
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: have fix
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.

I've tested the patch on Mac and it works fine too.
Kaie, can you review the patch. Thanks
Comment on attachment 90152 [details] [diff] [review]
Proposed fix, v1

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

r=kaie
Attachment #90152 - Flags: review+
+  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?
>+  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.
Attached patch Proposed fix, v2 (obsolete) — — Splinter Review 
I addressed bienvenu's concerns by replacing PR_FREEIF by PR_Free
Attachment #90152 - Attachment is obsolete: true
Comment on attachment 90229 [details] [diff] [review]
Proposed fix, v2

carrying over R=kaie
Attachment #90229 - Flags: review+
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.
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.
Attached patch Proposed fix, v3 — — Splinter Review 
fix those 2 PR_FREEIF I missed in the previous patch and modified the WARNING
comment.
Attachment #90229 - Attachment is obsolete: true
Comment on attachment 90249 [details] [diff] [review]
Proposed fix, v3

sr=bienvenu, thx.
Attachment #90249 - Flags: superreview+
Comment on attachment 90249 [details] [diff] [review]
Proposed fix, v3

carrying over R=kaie
Attachment #90249 - Flags: review+
Fixed in the trunk. Nominating nsbeta1
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: nsbeta1
Resolution: --- → FIXED
Whiteboard: have fix
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.

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.

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. 
cc iqa

verified on 07-02-07-1.0 on Linux RedHat7.1 JA
Status: RESOLVED → VERIFIED
The last comment was a verification for intl related issue only.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
The last comment was a verification for intl related issue only.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
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.
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.
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.
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.
Verified.
Status: RESOLVED → VERIFIED
Keywords: nsbeta1 → adt1.0.1, nsbeta1+
Whiteboard: [adt2 rtm]
adding adt1.0.1+.  Please get drivers approval before checking in.
Keywords: adt1.0.1 → adt1.0.1+, mozilla1.0.1
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.
Attachment #90249 - Flags: approval+
Keywords: mozilla1.0.1 → mozilla1.0.1+
Fix checked in the branch.
Keywords: mozilla1.0.1+ → fixed1.0.1
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.


So far, so good. OK using july10 commercial branch, win98.
Still need to check other platforms.
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.

That's a separate issue. Do we  have a bug for that?
Yes, that's a separate issue, bug 141730.
comment 37 refers to the draft/template problem.

Bug 156958 covers the Mac bug forwarding inline encrypted messages.

Marking this verified1.0.1
Keywords: fixed1.0.1 → verified1.0.1
Product: PSM → Core
Product: Core → MailNews Core
QA Contact: carosendahl → s.mime
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: