272149 - misleading question when selecting multiple certificates for signing and encrypting mail

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[simon annear]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041122
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041122

The aim is to have two certificates, one for signing and one for encrypting
mail.  This works, but the msgbox is poorly worded

It should change from:
Do you want to use the same certificate to digitally sign your email? (when
changing the encryption cert)  [OK]  [Cancel]

Do you want to use the same certificate to digitally sign your email? (when
changing the encryption cert)  [YES]  [NO]

I would have assumed that when I select a cert, and then choose cancel, it
cancels the change.  Instead what it is doing is either changing (or not) the
other cert so that you are using the same certificate.

Reproducible: Always
Steps to Reproduce:
1.load two certs into your certificate store
2.go to edit, mail and newsgroup account settings
3.select a mail account
4.select security
5.select one of your certs for digital signing
6.when it asks if you want to use the cert for encrypting, select OK
7.select another cert for encrypting
8.when it asks you if you want to use the cert for signing, select cancel
9.Note that the certs have changed

Actual Results:  
certs changed

Expected Results:  
either certs not changed, or better still, wording of msgbox should be updated
to reflect actual results

Comment 1

[:Gijs (he/him)]

Attachment: patch v1

First try on a patch. A couple of questions:
* Is there any code outside this file which uses askUser?
* If there isn't, should I still be more subtle than defining the same buttons
for every caller? (ie, add parameters for buttons to the askUser function)
* How can I (myself) test if this is actually fixed? Right now I'm assuming
that my javascript is correct, and I can't seem to get the same dialog. Any
ideas how I should do that?
* Should I file a new bug (or is there one already?) to do the same in
Thunderbird? It seems they have the same dialogs.


Requesting review. (Do I need sr? From who would I request it?)
This is my first patch for mailnews, if I'm doing anything really really wrong,
please let me know.

Comment 2

[David :Bienvenu]

Comment on attachment 179710 [details] [diff] [review]
patch v1

+  if (rv == 0)
+    return true;
+  else
+    return false;

this can just be return (rv == 0); or return !rv ;

Does this just change the dialog from an OK/Cancel to a Yes/No dialog?

Comment 3

[David :Bienvenu]

to answer some of your questions:

there don't look to be any callers of askUser outside this file 
Have you rebuilt the smime extension with your change? if so, is the problem
that you can't reproduce the bug scenario?
I think thunderbird has forked this file, so we'd need a new bug for this (but
let's make sure we have a seamonkey fix first)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsIPromptService.idl#203
?

Comment 5

[:Gijs (he/him)]

Attachment: Patch v2

(In reply to comment #2)
> this can just be return (rv == 0); or return !rv ;
Changed.

> Does this just change the dialog from an OK/Cancel to a Yes/No dialog?
Yes.

(In reply to comment #3)
> Have you rebuilt the smime extension with your change? if so, is the problem
> that you can't reproduce the bug scenario?
I unjarred, replaced the file, rejarred. And yes, the problem is that I can't
reproduce the bug scenario. Sorry for being unclear.

> I think thunderbird has forked this file, so we'd need a new bug for this
(but
> let's make sure we have a seamonkey fix first)
Sure, I'll file one once this bug is fixed :-).


(In reply to comment #4)
>
http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsIPromptService.idl#203


Hrm, I should have remembered that. Did a previous patch for ChatZilla,
couldn't use that constant because it wasn't present in all versions of Mozilla
/ Firefox (ChatZilla aims to be quite backwards compatible). Fixed now.

Comment 6

[simon annear]

Gijs and others

I'm happy to test the patch, but don't have a compile environment - is there any
way for me to test the patch without having to compile up Mozilla (myself)?

Thanks for your work so far

Simon

Comment 7

[[e-mail]]

Comment on attachment 179735 [details] [diff] [review]
Patch v2

>+  var buttons = ps.STD_YES_NO_BUTTONS;
I don't see the point of an extra declaration for this (and it would have to be
const anyway), it should be obvious what passing ps.STD_YES_NO_BUTTONS as the
fourth parameter does.

>+  var rv = ps.confirmEx(
Don't call this rv, that name is reserved for nsresult codes. Call it something
like "button" instead (that also makes the test for button == 0 clearer).

sr=me with those nits fixed.

Comment 8

[:Gijs (he/him)]

Attachment: Patch with nits fixed

Patch with nits fixed.
I was told I could set sr+ myself, since neil said sr would be given if the
nits were fixed. If this is in error, I apologize.

Comment 9

[David :Bienvenu]

Comment on attachment 179834 [details] [diff] [review]
Patch with nits fixed

thx for fixing this.

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Has the patch been checked in?

Comment 11

[:Gijs (he/him)]

(In reply to comment #10)
> Has the patch been checked in?

http://lxr.mozilla.org/seamonkey/source/mailnews/extensions/smime/resources/content/am-smime.js

Not yet. 

As this is non-critical code change, I'm presuming it will have to wait until
the tree unfreezes? Otherwise, it would need approval before being checked in, I
think :-).

Comment 12

[:Gijs (he/him)]

Comment on attachment 179834 [details] [diff] [review]
Patch with nits fixed

Requesting approval for branch.

<CTho>	Hannibal: it's not just the critical stuff that makes a good experience

^ perhaps I should have realised that earlier. Anyhow, five months onwards,
here we go :-)

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Landed on trunk.  I'd like to see it in 1.8 too.

Comment 14

[Asa Dotzler [:asa]]

I think scott needs to look at this because it looks like files that are shared
with Tbird.

Comment 15

[:Gijs (he/him)]

(In reply to comment #14)
> I think scott needs to look at this because it looks like files that are shared
> with Tbird.

comment #3 wrote:
> I think thunderbird has forked this file, so we'd need a new bug for this (but
> let's make sure we have a seamonkey fix first)

See:
http://lxr.mozilla.org/seamonkey/source/mail/extensions/smime/content/am-smime.js
http://lxr.mozilla.org/seamonkey/source/mailnews/extensions/smime/resources/content/am-smime.js