I think this depends on bug 119380

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).

remove nsbeta1

Added [LDAP] to subject line, noted as regression from 4.7x

*** 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. ***

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

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.

New patch with user interface strings as suggested by Sean.

Comment on attachment 85735 [details] [diff] [review] UI strings v2 Because it uses Sean's strings, r=cotter

This patch works for me. It only tries to fetch, when encryption is actually enabled to be used in the currently composed message.

Comment on attachment 85735 [details] [diff] [review] UI strings v2 sr=alecf

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

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!

> 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

New patch, addressing dmose's comments.

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

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.

I filed bug 149013 as per dmose's request to implement authentication.

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).

> 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

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

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(",");

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.

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?

Checked in to trunk.

Let's verify this with 119380 asap so we can try to land this on the branch.

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.

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.

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.

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.