Closed Bug 119394 Opened 23 years ago Closed 22 years ago

[LDAP] support fetching certificates from LDAP servers

Categories

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

Other Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
psm2.3

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

(Keywords: regression, Whiteboard: [adt1 RTM] [ETA 6/20])

Attachments

(1 file, 7 obsolete files)

 
I think this depends on bug 119380
Assignee: ssaux → kaie
Depends on: 119380
Priority: -- → P2
Target Milestone: --- → 2.2
Is this a dupe of 103896?
*** Bug 103896 has been marked as a duplicate of this bug. ***
*** Bug 123020 has been marked as a duplicate of this bug. ***
S/MIME bugs are automatically nsbeta1 candidates. (this is a bulk update - there
may be some adjustment of the list).
Keywords: nsbeta1
Keywords: nsbeta1+
Keywords: nsbeta1
QA Contact: alam → carosendahl
remove nsbeta1
Keywords: nsbeta1+
Target Milestone: 2.2 → 2.3
Keywords: nsbeta1+
Whiteboard: [adt2 RTM]
Added [LDAP] to subject line, noted as regression from 4.7x
Keywords: regression
Summary: support fetching certificates from LDAP servers → [LDAP] support fetching certificates from LDAP servers
*** Bug 124046 has been marked as a duplicate of this bug. ***
This is an intermediate patch, not yet complete.

I'm using the patch from bug 119380. When I run the code, an internal error in
getBinaryValues is reached.

Some debug output, from both the ldap code and the patch:

6151[89e7618]: nsLDAPConnection::Run() entered
==> bound onLDAPInit
0
1024[80a0db8]: pending operation added; total pending operations now = 1
1024[80a0db8]: nsLDAPOperation::SearchExt(): called with aBaseDn = 'dc=com';
aFilter = '(mail=kin@netscape.com)', aAttrCounts = 1, aSizeLimit = -1
1024[80a0db8]: pending operation added; total pending operations now = 2
6151[89e7618]: InvokeMessageCallback entered
==> listener onLDAPMessage
[xpconnect wrapped nsILDAPMessage @ 0x88f6a40]
==>  errorCode: 0 type: 101 errorMessage:
1024[80a0db8]: nsLDAPMessage::GetBinaryValues(): called with aAttr =
'usercertificate;binary'
1024[80a0db8]: ###!!! ASSERTION: nsLDAPMessage::GetBinaryValues(): internal
error: 1: 'Error', file
/home/kaie/100/mozilla/directory/xpcom/base/src/nsLDAPMessage.cpp, line 573
###!!! ASSERTION: nsLDAPMessage::GetBinaryValues(): internal error: 1: 'Error',
file /home/kaie/100/mozilla/directory/xpcom/base/src/nsLDAPMessage.cpp, line
573
1024[80a0db8]: ###!!! Break: at file
/home/kaie/100/mozilla/directory/xpcom/base/src/nsLDAPMessage.cpp, line 573
###!!! Break: at file
/home/kaie/100/mozilla/directory/xpcom/base/src/nsLDAPMessage.cpp, line 573
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8000ffff
(NS_ERROR_UNEXPECTED) [nsILDAPMessage.getBinaryValues]"  nsresult: "0x8000ffff
(NS_ERROR_UNEXPECTED)"	location: "JS frame ::
chrome://messenger-smime/content/msgCompSecurityInfo.js :: anonymous :: line
180"  data: no]
************************************************************
6151[89e7618]: pending operation removed; total pending operations now = 1
6151[89e7618]: InvokeMessageCallback entered
==> listener onLDAPMessage
[xpconnect wrapped nsILDAPMessage @ 0x896cee8]
==>  errorCode: 0 type: 97 errorMessage:
6151[89e7618]: pending operation removed; total pending operations now = 0


Dan, am I making something wrong?
Kai: you need to call getBinaryValues on a RES_SEARCH_ENTRY message, not on the
RES_SEARCH_RESULT.  The RESULT is just a placeholder message which tells you
that the search has finished.  I'll try and remember to add an assertion to the
my patch which tests for this.
Thanks, Dan. So I guess, the fact that I'm only receiving a RES_SEARCH_RESULT,
but not a RES_SEARCH_ENTRY, means that no results where found?
I answer myself, yes, that's the reason.
I now have something working that is indeed able to fetch certificates!

Because of some issues with mail/news, we currently don't have the luxury of
getting guaranteed change reports to the recipient lists. Therefore we want to
do certificate fetching only when the message security info is requested, or
when the user finally decides to send the message. That includes, while
composing one message, remembering for which email addresses we already tried to
fetch, so that we don't try to query LDAP repeatedly.


As a result the user has to wait while we are fetching certificates.

I think we need to give feedback while we are fetching. And the user should be
able to cancel searching if he does not want to wait any longer.

What about the following plan:
When the user hits send or requests security info, and encryption is requested
for the current message, we check whether LDAP directory is configured, whether
certs for recipients are missing, and whether we actually have recipients
addresses we did not already search before. If all that is true, we show a
search status dialog, saying "downloading certs from LDAP". That dialog has a
"stop" button.

The user can cancel at any time. If he does, sending the message will probably
fail, and security info will probably report missing certs.

If the user is patiently enough to wait until the search is finished, the dialog
goes away automatically.


The above will take a moment to implement, unlikely to be finished tomorrow. So
I suggest we try to get the status string in before the next UI freeze.


Sean, which wording would you recommend? What about:
  "Please wait while trying to obtain message recipient certificates from the
directory server."
and for the stop button
  "stop searching"
?

I'll produce a patch with that wording for checkin this week.


With regards to storing certs, I think our plan was to store all found
certificates in the cert database permanently, as the user might want to send
encrypted to those recipients again.


Another question: If the user chooses to download the LDAP directory for offline
usage, will certificates also be available offline?
*** Bug 118772 has been marked as a duplicate of this bug. ***
Blocks: 74157
> Because of some issues with mail/news, we currently don't have the luxury of
> getting guaranteed change reports to the recipient lists

Wouldn't it be better to fix whatever is causing this problem rather than coding
around it?  The workaround sounds ugly.

> Another question: If the user chooses to download the LDAP directory for 
> offline usage, will certificates also be available offline?

No.  For offline use, everything is stored as a local mork addressbook, and the
LDAP API isn't used when offline at all.
> Wouldn't it be better to fix whatever is causing this problem rather than coding
> around it?  The workaround sounds ugly.

It's not really a problem but rather a missing feature. Right now, the recipient
list in the message compose window can be changed by a variety of ways, and the
list is stored only in the UI. I already had some discussions with ducarroz
about that a while ago, because I wanted to implement 144402. Basically, the
recipient list is obtained from the UI on demand only.

An always current up to date list of all recipient seemed to be complex to do,
and so we decided to future bug 144402. If you want, I can place more details
from our discussion in here (or maybe better in bug 144402).

As long as we don't have an always current recipient list, i.e. as long as we
don't have a notification mechanism that will be called, whatever action
triggered a change of the recipient list, we can't fetch certificates early, but
need to do that on demand, too, when we are sending the message out or trying to
show the security info.

Because we want that LDAP cert fetching feature really soon, and the general
composer window change seems to be to huge for the current timeframe, I fear the
workaround is needed, even though it seems ugly.
I'll be attaching a patch that is already close to the final patch.
- fetching certificates when opening security info window already works
- fetching certificates when clicking send is not yet implemented

In order to get the necessary strings in, I'll be attaching the patch in two
pieces. The first piece are the locale-only changes, which should be checked in
this week, after Sean has reviewed it. The second piece is the snapshot of what
I have currently, and will change.

Status: NEW → ASSIGNED
Attached patch Snapshot of work in progress (obsolete) — Splinter Review
Attachment #85223 - Attachment is obsolete: true
Attachment #85586 - Attachment is obsolete: true
Attachment #85586 - Attachment is obsolete: false
Sean, can you please review the User Interface Strings patch?

- title of status dialog
- message shown in status dialog
- stop button label 

Thanks
Here are my suggestions for the strings:

Title: Downloading Certificates

Message: Searching the directory for recipients' certificates. This may take a
few minutes.

Button: [ Stop Searching ]
Raising priority.
Whiteboard: [adt2 RTM] → [adt1 RTM]
Severity: normal → critical
Attached patch UI strings v2 (obsolete) — Splinter Review
New patch with user interface strings as suggested by Sean.
Attachment #85587 - Attachment is obsolete: true
Comment on attachment 85735 [details] [diff] [review]
UI strings v2

Because it uses Sean's strings, r=cotter
Attachment #85735 - Flags: review+
This patch works for me.

It only tries to fetch, when encryption is actually enabled to be used in the
currently composed message.
Attachment #85586 - Attachment is obsolete: true
Comment on attachment 85735 [details] [diff] [review]
UI strings v2

sr=alecf
Attachment #85735 - Flags: superreview+
Comment on attachment 85773 [details] [diff] [review]
Suggested implementation (excluding UI strings)

Don't you need an equivalent modification for the modern theme that corresponds
with this change?

mozilla/themes/classic/jar.mn

Other than that r=javi
Attachment #85773 - Flags: review+
Thanks for the review, Javi. I think you have overlooked that the patch actually
already contains a change for the mentioned themes/classic/jar.mn file.
Comment on attachment 85773 [details] [diff] [review]
Suggested implementation (excluding UI strings)

I'm not all the way through the patch yet, but here are some comments that
could use a look:

nsISMimeJSHelper.idl
--------------------
a) please add doxygen-style comments for getNoCertAddresses

b) wstring is obsolete; please use AString

search()
--------
c) the call to gLdapConnection.init is using gLdapServerURL.dn as the
bind DN.  However, that property of the URL is the base DN.  For
details on the bind DN, see bug 135778 and its dependants.  If you
don't implement authentication immediately, this param be changed to null
before checkin.

kickOffSearches()
---------------
d) Instead of kicking off a bunch of separate searches, why not just
OR all your search terms together and do a single search?

e) gLdapServerURL.filter needs to be ANDed with whatever search is being
done so that only that part of the tree defined in the prefs gets
searched.  See nsLDAPAutoCompleteSession for an example.  

f) the sizelimit param to searchExt should be taken from the preferences; see
MsgComposeCommands.js for an example.

generateBoundListener()
-----------------------
g) XPConnect should be able to automagically generate QI in many
cases, at the very least for objects which only implement one other
interface than nsISupports.  So you shouldn't need to implement QI
here at all.

h) the only reason to have a "generate*" wrapper for an object is if
you need to exploit the lexical closure of JS scopes.  The only thing
used in this particular object at all is |kickOffSearches|, and that's
already in the global scope.  So there's no need for this wrapper
here.

I think these will change the code non-trivially, so if you could address these
first, I'll then have another look.  Thanks!
Attachment #85773 - Flags: review+ → needs-work+
> a) please add doxygen-style comments for getNoCertAddresses

done


> b) wstring is obsolete; please use AString

Dan revoked that request, because he says, arrays of AString are not yet working
properly


> c) bind dn

Ok, setting to null for now, will look into the bind details soon.


> d) ORing

done


> e) ANDing url filter

done


> f) sizelimit to searchExt

Actually, I think I should not take the limit from the prefs, but do something
else. I'm not doing a wildcard search, but an explicit search. I want a
certificate for each queried email address. And because the directory might list
two certificates, I should double that number.
What do you think?


> g) remove QI

I tried without the QI, it did not work.
FYI, I see the following error:
[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE)
[nsIProxyObjectManager.getProxyForObject]"  nsresult: "0x80004002
(NS_NOINTERFACE)"  location: "JS frame ::
chrome://messenger-smime/content/certFetchingStatus.js :: getProxyOnUIThread ::
line 312"  data: no]

Works fine with having the QI implemented.


> h) unnecessary generate* wrapper

ok, removed
Attached patch Implementation v2 (obsolete) — Splinter Review
New patch, addressing dmose's comments.
Attachment #85773 - Attachment is obsolete: true
You have approval to checkin the string changes from the ADT.  You also need to
get drivers approval.
Comment on attachment 85735 [details] [diff] [review]
UI strings v2

a=valeski obtained in AIM discussion
Attachment #85735 - Flags: approval+
Comment on attachment 85735 [details] [diff] [review]
UI strings v2

This UI strings only patch has been checked in to both the trunk and the 1_0
branch. Marking the patch obsolete.
Attachment #85735 - Attachment is obsolete: true
I filed bug 149013 as per dmose's request to implement authentication.
Blocks: 149013
Comment on attachment 86035 [details] [diff] [review]
Implementation v2

> Actually, I think I should not take the limit from the prefs, but do
> something else. I'm not doing a wildcard search, but an explicit
> search. I want a certificate for each queried email address. And
> because the directory might list two certificates, I should double
> that number.  What do you think?

I see your point.  I guess I don't really have a strong opinion here;
it wouldn't even bug me too much to set it to unlimited.  Do whatever
you think is right.

general
-------
Most of the error-handling seems to just involve closing the window
and giving up.	Is it worth filing another bug on popping up dialogs
with an error message?

Most of the stuff below is minor nitpicking; in general the patch
looks great.

search()
--------
i) Why the leading s on this variable name?

+  var sPrefs = prefService.getBranch(null);

kickOffSearches()
-----------------
j) Shouldn't this now be named kickOffSearch()?

k) This code:

+      if (urlFilter[0] != '(') {
+	 prefix1 = "(&(" + urlFilter + ")";
+      }
+      else {
+	 prefix1 = "(&" + urlFilter;
+      }
+      suffix1 = ")";

would be easier to read if the test were for == rather than !=

l) The "" string initializers (prefix?, suffix?, mailfilter) aren't
necessary; strings in JS default to that value.

m) use gLDAPServerURL.scope instead of hardcoding SCOPE_SUBTREE

onLDAPMessage
-------------
n) Yes, the search should be aborted if the bind failed.  How to tell
if it succeeded: compare aMessage.errorCode to
nsILDAPErrors::SUCCESS

onComposeSendMessage
--------------------
o) In this clause:

+  var helper =
Components.classes[gSMimeContractID].createInstance(gISMimeJSHelper);
+
+  if (!helper)
+    return;

the if (!helper) will never get hit, because the only way helper can
end up being NULL would cause an exception to be thrown.

p) Since all the catch clauses in this function seem to do more or
less the same thing, perhaps the right thing to do hoist them up into
a single try/catch which wraps all (or most) of the statements in the
function.

msgCompSecurityInfo.js
----------------------
q) If you start at jCount.value and count down to 0, then you don't
need to spend an extra variable just to avoid multiple getter calls.
Additionally, processors tend to have specific instructions for
comparing with zero, which could save a few cycles as well.

+    var jmax = gCount.value;
+    for (var j = 0; j < jmax; ++j)

getMailboxList()
--------------
r) Since this is really just wrapping an XPCOM method, how about making
this mirror that pattern and have the string be an out param and the
function return an nsresult?  Otherwise it's not clear why
mailbox_count and the return value are both needed. I think this will
also allow you to avoid pre-setting mailbox_list and mailbox_count to
nsnull/0.

s) Shouldn't the calls to the compFields getters be checking return
values?

t) I believe nsIMsgHeaderParser.idl lies, and nsnull is now
interpreted as UTF8, not us-ascii.  So the conversion calls should
change to NS_ConvertUCS82toUTF8.  

u) If you replace the four consecutive calls to .Append with one
statement of the following form:

all_recipients += string1 + string2 + string3 + string4;

unnecessary intermediate allocations will be avoided.  I think alecf
wrote a "best practices" doc on www.mozilla.org somewhere that talks
about this.

v) Since the data being returned is going the XPCOM interfaces,
nsMemory::Free() needs to be used instead of PR_FREEIF() to free
results of these calls.  I'm not sure if this requires first doing
your own null-check.

GetNoCertAddresses()
--------------------
w) why bother pre-setting *count to 0?	All the other code paths here
seem to set it to something reasonable when exiting this function...

x) See point v) about the delete [] and PR_FREEIF calls on
mailbox_list.

y) The for loop appears to work the way it does largely because of the
extremely goofy, non-XPCOM-standard semantics of
nsIMsgHeaderParser::ParseHeaderAddresses().  Can you add a comment
here explaining exactly what's going on?

z) Prefer strlen() to nsCRT::strlen() for c strings; some
compiler/libc combinations either have hand-optimized code for this or
do sneaky inlining tricks.

A) In the call to nsMemory::Alloc, how about using a C++ style cast (ie
NS_STATIC_CAST macro) rather than a C-style cast?

B) It'd be good to check for ToNewUnicode failure.  One possibility is
to use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY if there's a failure; see
mozilla/xpcom/glue/nsMemory.h for usage details.

nsIX509CERTDB.idl
-----------------
C) doxygen comments here, please.

ImportEmailCertificate2
-----------------------
D) C++-style cast, please (NS_REINTERPRET_CAST, in this case).
Attachment #86035 - Flags: needs-work+
Attached patch Implementation v3 (obsolete) — Splinter Review
> Most of the error-handling seems to just involve closing the window
> and giving up.	Is it worth filing another bug on popping up dialogs
> with an error message?

Bug 149534 filed.


> i) Why the leading s on this variable name?
> +  var sPrefs = prefService.getBranch(null);

Because I copied that code over from mail/news.
I remember, it was a convention to indicate global variables, that will stay
around while composer windows are being re-used.
But in fact, this makes no sense here, I have renamed it to "prefs".


> j) Shouldn't this now be named kickOffSearch()?

renamed


> k) This code:
> [urlfilter]
> would be easier to read if the test were for == rather than !=

rearranged


> l) The "" string initializers (prefix?, suffix?, mailfilter) aren't
> necessary; strings in JS default to that value.

I did, and it cost me 15 minutes to find out why my code did no longer work :(
I'm using += to add data. If I have not initialized with "", that += code does
not seem work.
In addition, not all code paths are actually assigning values to the prefix and
suffix variables. However, I'm trying to combine all of them to a larger string
later.
By assigning "" I do not only set them to empty, I also define those variables
should have type string.
Only if I have defined the type, the concatenation code seems to work.
I'm not removing any of the assignments.


> m) use gLDAPServerURL.scope instead of hardcoding SCOPE_SUBTREE

changed


> n) Yes, the search should be aborted if the bind failed.  How to tell
> if it succeeded: compare aMessage.errorCode to
> nsILDAPErrors::SUCCESS

Thanks, error handling added.


> o) In this clause:
> the if (!helper) will never get hit, because the only way helper can
> end up being NULL would cause an exception to be thrown.

removed


> p) Since all the catch clauses in this function seem to do more or
> less the same thing, perhaps the right thing to do hoist them up into
> a single try/catch which wraps all (or most) of the statements in the
> function.

Agreed, all code currently protected with try/catch put into a single wrapper.


> q) If you start at jCount.value and count down to 0, then you don't
> need to spend an extra variable just to avoid multiple getter calls.
> Additionally, processors tend to have specific instructions for
> comparing with zero, which could save a few cycles as well.
> 
> +    var jmax = gCount.value;
> +    for (var j = 0; j < jmax; ++j)

Ok, although I usually try to avoid index variables to become negative.
I changed the continue condition to be >= 0. This inclusion of zero is
necessary, if I want to avoid having to use (j-1) as the array index, which
would look ugly.


> r) Since this is really just wrapping an XPCOM method, how about making
> this mirror that pattern and have the string be an out param and the
> function return an nsresult?	Otherwise it's not clear why
> mailbox_count and the return value are both needed. I think this will
> also allow you to avoid pre-setting mailbox_list and mailbox_count to
> nsnull/0.

I don't see a real advantage, but you win. :)


> s) Shouldn't the calls to the compFields getters be checking return
> values?

done


> t) I believe nsIMsgHeaderParser.idl lies, and nsnull is now
> interpreted as UTF8, not us-ascii.  So the conversion calls should
> change to NS_ConvertUCS82toUTF8.  

I looked at the implementation, too, and believe you are right. I filed bug
149546.
While this patch is not addding that conversion, but is just moving it around
in the file, I made the change to use UTF8.


> u) If you replace the four consecutive calls to .Append with one
> statement of the following form:
> 
> all_recipients += string1 + string2 + string3 + string4;

That code was wrong, because it forgot to add commas, with the added complexity
that adding commas should only be done when necessary.
Yesterday the patch to add the commas landed. Now that code looks much more
complex, and your suggestion to simplify is no longer applicable.


> v) Since the data being returned is going the XPCOM interfaces,
> nsMemory::Free() needs to be used instead of PR_FREEIF() to free
> results of these calls.  I'm not sure if this requires first doing
> your own null-check.

Changed, again this was just moved. I originally cloned the calls to free from
nsMsgComposeSecure.cpp.
If you are right, we should probably change the original location to.
I did that, and I'm including that additional change in the new patch (file
nsMsgComposeSecure.cpp).


> w) why bother pre-setting *count to 0?	All the other code paths here
> seem to set it to something reasonable when exiting this function...

It's just that I'm used to always initialize my variables.
But removed.


> x) See point v) about the delete [] and PR_FREEIF calls on
> mailbox_list.

I don't see you mention delete [] in v) ?

All calls to PR_FREEIF replaced with nsMemory::Free.


> y) The for loop appears to work the way it does largely because of the
> extremely goofy, non-XPCOM-standard semantics of
> nsIMsgHeaderParser::ParseHeaderAddresses().  Can you add a comment
> here explaining exactly what's going on?

When one follows the code, one arrives at the documentation of the
implemenation of ParseHeaderAddresses, which already gives a detailed
explanation of the returned data.
I'm a bit unwilling to comment that again, because all we do is forwarding and
processing that data, not manipulating it.
Where would the right place to comment it? There are multiple locations in
extensions/smims that deal with that data. Should we add the same explaining
comment whenever we have a for loop processing it?
Or should we just add the comment to the common function getMailboxList? But
that function is just one step before the other function with the comment, and
I believe programmers should be able to find that other comment.


> z) Prefer strlen() to nsCRT::strlen() for c strings; some
> compiler/libc combinations either have hand-optimized code for this or
> do sneaky inlining tricks.

Hmm, changing that everywhere in extensions/smime, too.
Can you explain why nsCRT::strlen does not get removed from the codebase, then?



> A) In the call to nsMemory::Alloc, how about using a C++ style cast (ie
> NS_STATIC_CAST macro) rather than a C-style cast?

changed


> B) It'd be good to check for ToNewUnicode failure.  One possibility is
> to use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY if there's a failure; see
> mozilla/xpcom/glue/nsMemory.h for usage details.

done. This made the for loops significantly more complex, because freeing the
arrays up after a failure requires that everything has been initialized to
zero, and that currently requires that each iteration of the loop gets
executed. I changed the initial code in the loops to do that first, and only
continue that iteration if no memory failure has been detected.


> nsIX509CERTDB.idl
> C) doxygen comments here, please.

This file does not currently use doxygen comments.
I was told that one should generally keep the style that is already used in a
file.
Therefore, I added a simple non-doxygen comment.


> ImportEmailCertificate2
> D) C++-style cast, please (NS_REINTERPRET_CAST, in this case).

done
Attachment #86035 - Attachment is obsolete: true
>> r) Since this is really just wrapping an XPCOM method, how about making
>> this mirror that pattern and have the string be an out param and the
>> function return an nsresult?	Otherwise it's not clear why
>> mailbox_count and the return value are both needed. I think this will
>> also allow you to avoid pre-setting mailbox_list and mailbox_count to
>> nsnull/0.
>
>I don't see a real advantage, but you win. :)

Readability for people new to the code.  Especially important if someone else
ends up maintaining it at some future point.

>> t) I believe nsIMsgHeaderParser.idl lies, and nsnull is now
>> interpreted as UTF8, not us-ascii.  So the conversion calls should
>> change to NS_ConvertUCS82toUTF8.  
>
>I looked at the implementation, too, and believe you are right. I filed bug
>149546.

Great; thanks.

>> v) Since the data being returned is going the XPCOM interfaces,
>> nsMemory::Free() needs to be used instead of PR_FREEIF() to free
>> results of these calls.  I'm not sure if this requires first doing
>> your own null-check.
>
>Changed, again this was just moved. I originally cloned the calls to free from
>nsMsgComposeSecure.cpp. If you are right, we should probably change the
>original location too. I did that, and I'm including that additional change in
>the new patch (file nsMsgComposeSecure.cpp).

Cool. The general rule is documented here: 

http://www.mozilla.org/scriptable/interface-rules-we-break.html

I believe this doesn't happen to hurt us too much at the moment because nsMemory
happens to use PR_Alloc under the hood.  But that's not guaranteed to remain
true, and every so often there is some discussion about pluggable allocators.

>> y) The for loop appears to work the way it does largely because of the
>> extremely goofy, non-XPCOM-standard semantics of
>> nsIMsgHeaderParser::ParseHeaderAddresses().  Can you add a comment
>> here explaining exactly what's going on?

>When one follows the code, one arrives at the documentation of the
>implemenation of ParseHeaderAddresses, which already gives a detailed
>explanation of the returned data.
>I'm a bit unwilling to comment that again, because all we do is forwarding and
>processing that data, not manipulating it.
>Where would the right place to comment it? There are multiple locations in
>extensions/smims that deal with that data. Should we add the same explaining
>comment whenever we have a for loop processing it?

That sounds like a good idea to me.  I'd be happy with a short comment just
pointing out this lack of convention and referring readers to the IDL file
documentation.  No need to re-iterate everything here.

>> z) Prefer strlen() to nsCRT::strlen() for c strings; some
>> compiler/libc combinations either have hand-optimized code for this or
>> do sneaky inlining tricks.
>
>Hmm, changing that everywhere in extensions/smime, too.
>Can you explain why nsCRT::strlen does not get removed from the codebase, then?

It should be. Bug 150073 filed. See the comments in nsCRT.h about the various
unsupported functions.

>> nsIX509CERTDB.idl
>> C) doxygen comments here, please.
>
>This file does not currently use doxygen comments.
>I was told that one should generally keep the style that is already used in a
>file.  Therefore, I added a simple non-doxygen comment.

In general, that's a good policy, especially for things like indentation, etc. 
However, I don't think it really applies to doxygen comments for IDL.  They
should always be used because they're necessary to drive
http://unstable.elemental.com/mozilla/

I'll re-review your patch shortly.



Comment on attachment 86593 [details] [diff] [review]
Implementation v3

Looks good.  Make the following minor tweaks, as well as my modified
suggestion for y) above, and you've got r=dmose.  No need for me to
re-review after these tweaks are made.

msgCompSMIMEOverlay.js
----------------------
E) These should be "const", not "var:

 var gISMimeCompFields = Components.interfaces.nsIMsgSMIMECompFields;
 var gSMimeCompFieldsContractID =
"@mozilla.org/messenger-smime/composefields;1";
+var gSMimeContractID = "@mozilla.org/messenger-smime/smimejshelper;1";
+var gISMimeJSHelper = Components.interfaces.nsISMimeJSHelper;


F) nsSMimeJSHelper::GetNoCertAddresses

This shouldn't be delete[]d, because it will have come through the
nsIMsgHeaderParser interface and therefore will have been created with
nsMemory::Alloc.  Use nsMemory::Free().

+    delete [] mailbox_list;

nsSMimeJSHelper::getMailboxList:

G) I really meant that this should be a normal XPCOM style signature:

nsresult nsSMimeJSHelper::GetMailboxList(nsIMsgCompFields *compFields, PRUint32
*mailbox_count, char **mailbox_list)

H) .Appends can still be concatenated to avoid unnecessary allocs.  Eg:

+      all_recipients.Append(utf8_to.get());
+      all_recipients.Append(",");

should be replacable with this:

+      all_recipients += utf8_to.get() + ','; 

or if that doesn't work, at least:

+      all_recipients += utf8_to.get() + ",";

Same for the other .Appends there.
Attachment #86593 - Flags: review+
This is a rather enormous patch.  What is being done to ensure adequate test
coverage?  Is it possible to limit any damage to the actual attempted use of
this feature?  (That is, don't have anything bad happen unless an actual
certificate exists and is referenced.)
This is the last piece of functionality that we are waiting to come in.  There
are some bug fixes still trickling in, but by and large they are pretty isolated
to security code (S/MIME & PSM), and I have a good handle on what's in the pipe.

I will be testing the functionality on commercial platforms.

For now I think Kai and dan are doing it right by taking their time to review
the patches.  I am more comfortable waiting a few more days until they are
completely comfortable.  I assume Kai is doing his usual unit testing as he's going.

After the patches are accepted, reviewed, etc., kai can check the code into
trunk.  I will test it there.  At the same time, he can create some test builds
with his changes and the branch code.  After I have completed testing on the
trunk, I will check the test builds.  If the test builds pass, then we can move
the code into the official branch line.
Dan, this patch addresses all of your requests.
Only in one case I was required to do something else, your suggested string
logic did not work correctly, the first version produced strings with random
content, the second suggested alternative did not compile.
After some testing, I changed the code to:
  all_recipients += utf8_to + nsDependentCString(",");
Attachment #86593 - Attachment is obsolete: true
Attachment #87208 - Flags: review+
Comment on attachment 87208 [details] [diff] [review]
Implementation v4

Carrying forward r=dmose
Comment on attachment 87208 [details] [diff] [review]
Implementation v4

sr=mscott. Dan did a great job on the review phase. The patch looked good to
me.
Attachment #87208 - Flags: superreview+
adding eta based on conversation with ssaux.  It looks like this is ready to
land on the trunk.  Once that happens, carosendahl, can you verify this?
Whiteboard: [adt1 RTM] → [adt1 RTM][ETA 6/14]
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Let's verify this with 119380 asap so we can try to land this on the branch.
Keywords: approval
Whiteboard: [adt1 RTM][ETA 6/14] → [adt1 RTM][ETA 6/18]
I've verified this on the 20020614 Trunk builds.  It looks good.

I performed the following tests:
Local cert database combined with global directory setup;
Local cert database combined with global directory setup and account specific
override;
Multiple recipients of different recipient types in a combination of valid and
invalid settings in a variety of ways;
Encrypted and Signed/Encrypted messages;

Please move to the branch, where I will complete some edge case testing.
Status: RESOLVED → VERIFIED
I have completed all of the tests on this functionality (whether it be trunk or
branch), so going forward I would like to focus on my branch builds and thus
would like to see it landed there.

I can continue to test it on the trunk, but at this point it would serve little
value beyond what I have already done over the last two days.
adding adt1.0.1+.  Please get drivers approval before checking into the branch.
Keywords: adt1.0.1adt1.0.1+
Adding:  Performed all of the tests in comment 48 under an SSL connection as well.
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Whiteboard: [adt1 RTM][ETA 6/18] → [adt1 RTM] [ETA 6/20]
Checked in to 1_0 branch.
charles - can you verify this fix on the branch tomorrow? thanks!
I'm working on it.  I should be done sometime Friday, 6/21.
Verified on the branch using the same test cases that I used on the trunk.
Kai, do you remember what the motivation was for this bug to use the ldap
attribute: "usercertificate" when looking up the certificate? Apparently
Netscape 4.x used the ldap attribute "usersmimecertificate" so this feature
doesn't work for customers migrating from 4.x in their enterprise environment. I
was just curious if you remember the reason for choosing that ldap attribute.
Thanks!
Scott: I seem to remember there being a snarl related to communicator and the
cert server using different attribute names for this.  I don't remember all the
details, but I suspect Bob Lord does, as he was in IS and had to deploy that
stuff back in the day, so you might try asking him.  Another possibility is to
ask someone still at Netscape if they can search old closed bugscape bugs about
this for more history.

Poking around in the RFCs, this seems to be documented to some extent in section
2.8 of RFC 2798.  My reading of that section makes it seem as though perhaps
usersmimecertificate;binary should be searched first, and then usercertificate
should used as a fallback.
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

Created:
Updated:
Size: