See http://www.mozilla.org/mailnews/specs/security/ This bug tracks implementation of the "mummy" button. Icons will be replaced when they are available.

Please note that the images that I have prepared are not very good looking. However, they are of the correct size, and the toolbar buttons looks correctly (besides the face that will change as the icons from bug 104502 get ready).

Stephane or David, can you please review?

This should now have all the other UI logic that is required for the message compose window, as required by the spec. - three different states for the send button - three different states for the new security button. - the status bar icons (clickable) - the dropdown menu on the security button - changed the text "always encrypt" to "require encryption" as mentioned in the spec (The logic, that will detect whether the message is currently in state "crypto possible" or "crypto not possible" will not be implemented with this bug. This also applies to enabling/disabling the send button, when sending is not possible.)

I notice all my changes are below mailnews/ and themes/. Scott, can you please review? Thanks. (Please note that I have been unable to make the toolbar work correctly, by using only overlays. Only if I add securityButton directly inside messengercompose.xul, the button appears at the correct location - otherwise it appears even behind the throbber icon.)

Comment on attachment 72720 [details] [diff] [review] Updated patch Sorry for the spam, I take my request for review back. First, I have to make sure, the security button will only appear, if crypto is compiled in.

I've come to the opinion that it does not make sense to post intermediate patches or request reviews half way.

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

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

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

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

Javi, the latest patch contains changes to files in mozilla/security. Can you please review them? The changes are: - cert picker no longer supports dynamic setting of the info and prompt labels, it's not required. I removed those to make the dialog look closer to what has been specified in the UI spec. - cert picker for cert configuration no longer has the first entry preselected, but the one that is given as a parameter. This is used to preselect the cert that is currently configured. - I implemented the new method nsIX509Cert::verifyForUsage, that verifies the requested usage only and returns the verification result code. This was produced by cloning code from other methods in nsNSSCertificate.cpp, so it should be correct.

security changes look good. r=javi

Code in security/manager did not change, so r=javi does still apply. The UI strings in this patch have r=cotter

The latest patch has an enhancement: The message security info window, when composing a message, will now show whether it is possible to send the message or not. Signed: Yes => Signing is enabled and certificates are configured Signed: No => Signing is disabled Signed: Not possible => User requested to sign, but no certs have been configured. Encrypted: Yes => Valid certs for all recipients are available Encrypted: No => Encryption is disabled Encrypted: Not possible => There is no valid cert for at least one recipient, or there are no recipients specified at all.

In order to be consistent with the other mail windows, which already show a "security info" command in the view menu, I added that command to the composer window's view menu, too. But by doing so, I realized that this menu command is already contained below another menu command, "Options/Security". With this patch, I have removed that command from the Options/Security menu. I think it makes sense for the following reasons: - the user expects only changeable items inside the options menu. - the message security dialog does not allow any options to be changed, it only displays information. - moving the command to "View/Message Security Info" makes all mail window's menus consistent However, the dropdown menu that opens when the security toolbar button gets clicked, still shows that menu command.

John suggested, we don't need Cancel buttons in the security info dialogs. Sean suggested to add help buttons to the security info dialogs.

Comment on attachment 74967 [details] [diff] [review] Adding help buttons / removing unnecessary cancel buttons 1) In nsISMimeJSHelper.idl, you'll want to change the license file so that you are the listed contributor instead of me. The copyright in that file says 1998 to. Should be 2002. 2) now that we cache one compose window and recycle it for subsequent windows, does that expose any holes with us showing stale security info? Just asking as it could be an interesting test case to make sure we have working. 3) Doesn't the following mean we'll always send an encrypted message or am I misreading the JS assignment? + var encryptionPolicy = gCurrentIdentity.getIntAttribute("encryptionpolicy"); + // 0 == never, 1 == if possible, 2 == always Encrypt. + gSMFields.requireEncryptMessage = encryptionPolicy == 2; 5) In msgCompSecurityInfo, it looks like we use a tree widget to represent the security info. I believe tree is going away. Joe Hewitt has been converting all tree widgets to either outliners or list boxes. I bet we'll need to touch base with him to coordinate potential landing conflicts. See "phaseOutXULTrees" on the mozilla landing plan: http://komodo.mozilla.org/planning/branches.cgi for more details. The only thing that worried me was the secure compose stuff in MsgComposeCommands.js. I wonder if we can abstract the security stuff that went in there a little bit. I need to sleep on that part and maybe talk to JF about it in the morning. Nice work laying all this out Kai. Code looked really good.

> 1) In nsISMimeJSHelper.idl, you'll want to change the license file so that you > are the listed contributor instead of me. The copyright in that file says 1998 > to. Should be 2002. Done, changed it with a MPL license. > 2) now that we cache one compose window and recycle it for subsequent windows, > does that expose any holes with us showing stale security info? Just asking as > it could be an interesting test case to make sure we have working. We rely on the the gComposeRecyclingListener in MsgComposeCommands.js to be called. I rely that both onClose and onReopen are called and calls are forwarded to the security overlay. For enhanced safety, I now added another deletion of the security info to the onReopen function, just before we try to create a new instance, which ensures cleared info when new object creation goes wrong. I'm also relying that either gMsgCompose.compFields stays around between compose window recycles, or, that on new compFields creation it is assured that securityInfo will be null, which I assume it is by default. > 3) Doesn't the following mean we'll always send an encrypted message or am I > misreading the JS assignment? > + var encryptionPolicy = > gCurrentIdentity.getIntAttribute("encryptionpolicy"); > + // 0 == never, 1 == if possible, 2 == always Encrypt. > + gSMFields.requireEncryptMessage = encryptionPolicy == 2; Hmm, maybe that line is not obvious enough. My understanding is, first the comparison will be evaluated, and the boolean result will be assigned. So, only if encryptionPolicy is set to "2", we'll require encrpytion, all other cases will not use encryption. Value "2" is set by the preferences dialog. I wanted to leave that value for now, until we implement the "if possible" encryption in a future version. > 5) In msgCompSecurityInfo, it looks like we use a tree widget to represent the > security info. I believe tree is going away. Joe Hewitt has been converting all > tree widgets to either outliners or list boxes. I bet we'll need to touch base > with him to coordinate potential landing conflicts. See "phaseOutXULTrees" on > the mozilla landing plan: > http://komodo.mozilla.org/planning/branches.cgi > for more details. Hmm. I asked around a bit and found postings on the newsgroups. Looks like <tree>s must be replaced with either <listbox> or <outliner>. I liked <listbox> better. The fact, that currently not a single xul or js file is using <listcell> is not very encouraging. However, I changed my code to use <listbox> and all the various suggested new tags, and it seems to work. > The only thing that worried me was the secure compose stuff in > MsgComposeCommands.js. I wonder if we can abstract the security stuff that went > in there a little bit. I need to sleep on that part and maybe talk to JF about > it in the morning. If in the future, multiple encryption engines were used, the code in MsgComposeCommands can just be extended to store a list of registered secure compose overlays that wish to be notified. > Nice work laying all this out Kai. Code looked really good. I'm glad to hear that, thanks a lot, but note that I had good teachers ;)

Now using <listbox> instead of <tree>. Now also includes the changes for Mac makefiles.

Comment on attachment 75564 [details] [diff] [review] Updated patch Here's an idea I had last night for the secureCompose stuff in MsgComposeCommands.js. Why don't we just fire custom events in MsgComposeCommands.js: This is how I notify the smime code that the message pane has been loaded. I created a custom event and fired it from the generic mailnews code. Then the smime code was registered as an event listener for this custom event so it gets the notification. Here's a snippet: i.e. var event = document.createEvent('Events'); event.initEvent('messagepane-loaded', false, true); var headerViewElement = document.getElementById("msgHeaderView"); headerViewElement.dispatchEvent(event); We can do the same thing here with 2 custom events: compose-window-close and compose-window-reopen What do you think?

> What do you think? This patch implements what you suggested.

Comment on attachment 76030 [details] [diff] [review] Updated patch sr=mscott. Thanks for making that additional change. You'll still need another r= from a mailnews person. Also, any packaging issues we need because of this change? I don't think there were but I thought I'd mention it.

> Also, any packaging issues we need because of this change? I don't think there > were but I thought I'd mention it. I agree. I added a lot of files, but no new libraries or archives. I changed the jar.mn files and the makefiles. I also gave a Win32 test build to Stephane, and he was able to use it. My primary browser is currently a Linux build which includes this patch, and this build has been installed using the installer, which I built myself. In addition, Javi tested that the patch compiles on Mac, if he uses the GIFs that I gave him separately.

Comment on attachment 76030 [details] [diff] [review] Updated patch Looks ok to me. R=ducarroz

Comment on attachment 76030 [details] [diff] [review] Updated patch a=asa (on behalf of drivers) for checkin to the 1.0 trunk

patch checked in, fixed.

Verified on 3/28 Win2000 trunk build.