131087 - Failure on sending S/Mime messages should display the correct reason

Status: VERIFIED FIXED

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

Description

[Kai Engert [:KaiE:]]

For examples, see bug 127364 or bug 121287.

The current code gives misleading information to the user when sending is not
possible. Currently, it just displays a general statement about "wrong email
settings".

It should be stating that there is a problem caused by certificates, if that's
the real reason.

Comment 1

[Kai Engert [:KaiE:]]

Assigning to me, nsbeta1+.

Comment 2

[Kai Engert [:KaiE:]]

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

Comment 3

[Kai Engert [:KaiE:]]

I'd like to land this before the UI freeze.

Although I'm just defining two additional strings, I guess it's better to land
today or tomorrow.

Will attach a patch in a minute.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

David, can you please review the code?

Sean, can you please review the two added strings? (search for "properties" in
the patch).

Thanks!

Comment 5

[Kai Engert [:KaiE:]]

David, in the code I'm changing, I'm removing comments which say
  XXX fix this later

The comment did not say what exactly should get fixed. Are those comments from
you and is the only thing that's missing, giving feedback to the user? Or is
there more to do?


A hint for reviewing: I'm removing a line that checks for "status < 0". I'm
removing it, because a couple of lines above, there is already the same line,
and the variable is not changed after and and before the removed check.

Comment 6

[David P. Drinan]

Comment on attachment 76586 [details] [diff] [review]
Suggested fix

r=ddrinan.

Comment 7

[Kai Engert [:KaiE:]]

Jag, can you please sr= ?

I would like to land this today or on Friday, because it is a UI relevant change.

Thanks!

Comment 8

[jag (Peter Annema)]

Comment on attachment 76586 [details] [diff] [review]
Suggested fix

>Index: mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp,v
>retrieving revision 1.11
>diff -u -d -r1.11 nsMsgComposeSecure.cpp
>--- mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp	27 Mar 2002 03:27:43 -0000	1.11
>+++ mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp	28 Mar 2002 17:47:25 -0000
...
>+  if (NS_SUCCEEDED(res) && errorString.Length())

!errorString.IsEmpty()

(Both places)

>@@ -572,17 +630,19 @@
>   PR_ASSERT(mSelfEncryptionCert);
>   PR_SetError(0,0);
>   mEncryptionCinfo = do_CreateInstance(NS_CMSMESSAGE_CONTRACTID, &rv);
>-  if (NS_FAILED(rv)) return 0;
>-  rv = mEncryptionCinfo->CreateEncrypted(mCerts); // XXX Fix this later XXX //
>+  if (NS_FAILED(rv)) return NS_ERROR_OUT_OF_MEMORY;

I would just return rv here instead of assuming we're out of memory. If you
suspect mEncryptionCinfo could be null while NS_SUCCEEDED(rv), add a ! check on
that and then return NS_ERROR_OUTOFMEMORY.

Also, you're effectively changing this case from NS_SUCCEEDED to NS_FAILED, are
all callers ready to deal with that?

(Both places)

Comment 9

[Kai Engert [:KaiE:]]

>>+  if (NS_SUCCEEDED(res) && errorString.Length())
> 
> !errorString.IsEmpty()
> 
> (Both places)

ok, changed.


>>  mEncryptionCinfo = do_CreateInstance(NS_CMSMESSAGE_CONTRACTID, &rv);
>>-  if (NS_FAILED(rv)) return 0;
>>+  if (NS_FAILED(rv)) return NS_ERROR_OUT_OF_MEMORY;
> 
> I would just return rv here instead of assuming we're out of memory. 

You're right, I changed that in both places.


> Also, you're effectively changing this case from NS_SUCCEEDED to NS_FAILED, are
> all callers ready to deal with that?

Yes, I think it should be that way.
Our only caller is in nsMsgSendPart, routed through nsMsgSend.
We arrive in this method only if there is actually encryption work to do.
If we are asked to encrypt, but we can't do our job, then we should return
failure, and cause the mail sending code to give up.

Comment 10

[Kai Engert [:KaiE:]]

Attachment: Updated Patch

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 76764 [details] [diff] [review]
Updated Patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 12

[Jaime Rodriguez, Jr.]

adt1.00+ per ADT triage, pending L10N approval. Adding mcarlson for L10N
approval for strings. 

Comment 13

[Michele Carlson]

l10n approved. thanks

Comment 14

[Sean Cotter]

Comments on the new strings:

+ErrorCanNotEncrypt=Unable to encrypt the message. Please check that you have a
valid email certificate for each recipient. Please check that your own
configured mail security certificates for this mail account are valid and trusted.

Change to: "Unable to encrypt message. Please check that you have a valid email
certificate for each recipient. Please check that the certificates specified in
Mail & Newsgroups Account Settings for this mail account are valid and trusted.

+ErrorCanNotSign=Unable to sign the message. Please check that your own
configured mail security certificates for this mail account are valid and trusted.

Change to: "Unable to sign message. Please check that the certificates specified
in Mail & Newsgroups Account Settings for this mail account are valid and trusted.

Reason in both cases is to give user a bit more of a clue re where to look, plus
consistency with usage elsewhere.

Comment 15

[Jean-Francois Ducarroz]

Comment on attachment 76764 [details] [diff] [review]
Updated Patch

looks good to me, R=ducarroz

Comment 16

[jag (Peter Annema)]

Comment on attachment 76764 [details] [diff] [review]
Updated Patch

sr=jag

Comment 17

[Kai Engert [:KaiE:]]

Checked in, using Sean's suggested text.

Comment 18

[Charles Rosendahl]

Verified.

Comment 19

[Charles Rosendahl]

Is this also in the branch yet?

Comment 20

[Kai Engert [:KaiE:]]

This landed on the trunk before the 1.0.0 branch was created, so the answer is yes.