Closed
Bug 419595
Opened 16 years ago
Closed 15 years ago
nsLDAPURL implementation is incorrect
Categories
(Directory :: LDAP XPCOM SDK, defect, P2)
Directory
LDAP XPCOM SDK
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file, 8 obsolete files)
66.69 KB,
patch
|
standard8
:
review+
dmosedale
:
superreview+
|
Details | Diff | Splinter Review |
nsLDAPURL has at least the following problems: * .path returns just the DN and not the full path, it additionally doesn't have a "/" on the front of it. * .prePath doesn't return a value (just truncates) * .hostPort implementation is wrong. We need also to check the implementation is at least reasonable to RFC 4516. I'm attaching a basic xpcshell test that we need to pass (we fail at the moment for the reasons mentioned above), though I'd like to see it expanded to test more cases etc.
Assignee | ||
Comment 1•16 years ago
|
||
I'm now working on this.
Assignee: nobody → bugzilla
Keywords: helpwanted
Assignee | ||
Comment 2•16 years ago
|
||
Here's the fix. To save reimplementing parts of nsStandardURL we now keep a local copy and use that to handle the "core" parts. This is the same way that nsMsgMailNewsUrl does things. I decided to keep the "path" section of the nsStandardURL up to date, hence when we set items that are local, we have to update the path as well. I'm open to comments about this; the alternative would be to ignore the path part that's in the nsStandardURL and just handle it in nsLDAPURL, so when we GetSpec we'd have to get the Pre-Path part and then append the locally held path. Also clarified/corrected a couple of comments in nsILDAPURL.idl. Test case included that should just about cover all of the functions we support.
Attachment #305710 -
Attachment is obsolete: true
Attachment #320900 -
Flags: superreview?(dmose)
Attachment #320900 -
Flags: review?(dmose)
Assignee | ||
Comment 3•16 years ago
|
||
Assignee | ||
Comment 4•16 years ago
|
||
Comment on attachment 320900 [details] [diff] [review] The fix (diff -w) I've a feeling this may have broken something address book side. Cancelling review till I check it out tomorrow.
Attachment #320900 -
Flags: superreview?(dmose)
Attachment #320900 -
Flags: review?(dmose)
Comment 5•16 years ago
|
||
(Any update ?)
Assignee | ||
Updated•16 years ago
|
Priority: -- → P2
Assignee | ||
Comment 6•16 years ago
|
||
Revised version. This works with both the old and new password managers so its safe to put in once reviewed. The main things we are changing in this patch: - Provide a protocol handler for ldap and ldaps protocols. For this I dropped the c++ version, implemented one in js, and did the appropriate support changes to allow the channel to be created if the experimental code is switched on. - Reimplement nsLDAPURL to things to the same standard as per nsStandardURL, mostly by re-using the same code. See comment 0 and comment 2 for more info. - Implement a test case for nsLDAPURL dropping it into the existing tests directory that the directory/xpcom code has.
Attachment #320900 -
Attachment is obsolete: true
Attachment #320901 -
Attachment is obsolete: true
Attachment #334889 -
Flags: superreview?(dmose)
Attachment #334889 -
Flags: review?(bienvenu)
Comment 7•16 years ago
|
||
Comment on attachment 334889 [details] [diff] [review] The fix v2 In general, this looks good. The tests are great; I like the switch of the protocol handler to JS. I do think nsLDAPURL.h needs some documentation about exactly how mBaseURL is (and isn't) intended to be used; without that, it's difficult to review whether the code is doing exactly the right thing or not. >+++ b/directory/xpcom/base/src/nsLDAPProtocolHandler.js > > [...] > >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); >+ >+const kNetworkProtocolCIDPrefix = "@mozilla.org/network/protocol;1?name="; >+const nsIProtocolHandler = Components.interfaces.nsIProtocolHandler; >+ >+function makeProtocolHandler(aProtocol, aDefaultPort) { >+ return { >+ classDescription: "LDAP Protocol Handler", >+ contractID: kNetworkProtocolCIDPrefix + aProtocol, The lines after return would be more readable with another level of indent, I think. >+ >+ newURI: function (aSpec, aOriginaCharset, aBaseURI) { s/aOrigina/aOrigin/, not that it really matters. >diff --git a/directory/xpcom/base/src/nsLDAPURL.cpp b/directory/xpcom/base/src/nsLDAPURL.cpp >--- a/directory/xpcom/base/src/nsLDAPURL.cpp >+++ b/directory/xpcom/base/src/nsLDAPURL.cpp >@@ -43,329 +43,283 @@ > #include "netCore.h" > #include "plstr.h" > #include "nsCOMPtr.h" >+#include "nsNetCID.h" >+#include "nsComponentManagerUtils.h" > > // The two schemes we support, LDAP and LDAPS > // >-static const char kLDAPScheme[] = "ldap"; >-static const char kLDAPSSLScheme[] = "ldaps"; >+#define LDAP_SCHEME NS_LITERAL_CSTRING("ldap") >+#define LDAP_SSL_SCHEME NS_LITERAL_CSTRING("ldaps") Assuming the frozen string API still supports it, NS_NAMED_LITERAL_CSTRING is preferred. > NS_IMETHODIMP > nsLDAPURL::SetSpec(const nsACString &aSpec) > { >+ nsresult rv = mBaseURL->SetSpec(aSpec); >+ NS_ENSURE_SUCCESS(rv, rv); > >+ return SetPathInternal(nsPromiseFlatCString(aSpec)); If the SetPathInternal fails, won't the entire nsLDAPURL be in an unknown/inconsistent state? I think we need to fail without corrupting the object, if we can. >diff --git a/mailnews/addrbook/prefs/resources/content/pref-directory-add.js b/mailnews/addrbook/prefs/resources/content/pref-directory-add.js > >+ // We have to form this by hand because the standard URL methods don't >+ // allow us to build it up in a URL object from scratch. I don't understand why this would be true. Can't you just QI the object returned from the IO service to an nsILDAPURL and then build it up through that?
Attachment #334889 -
Flags: superreview?(dmose) → superreview-
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #7) > >diff --git a/mailnews/addrbook/prefs/resources/content/pref-directory-add.js b/mailnews/addrbook/prefs/resources/content/pref-directory-add.js > > > >+ // We have to form this by hand because the standard URL methods don't > >+ // allow us to build it up in a URL object from scratch. > > I don't understand why this would be true. Can't you just QI the object > returned from the IO service to an nsILDAPURL and then build it up through > that? Well I we could do it that way, but I'd have to make the protocol handler, fill in a dummy URL into the new nsILDAPURL. If you try and do it by bits, then nsStandardURL complains saying its not initialised: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsStandardURL.cpp&rev=1.111&mark=1093-1096#1080 and its init function requires the full URL.
Assignee | ||
Comment 9•16 years ago
|
||
Updated patch for Dan's comment, see also my response to one of them in comment 8.
Attachment #334889 -
Attachment is obsolete: true
Attachment #335582 -
Flags: superreview?(dmose)
Attachment #335582 -
Flags: review?(bienvenu)
Attachment #334889 -
Flags: review?(bienvenu)
Comment 10•16 years ago
|
||
Comment on attachment 335582 [details] [diff] [review] The fix v3 this braces style is inconsistent with the rest of the file/patch + rv = SetPathInternal(nsPromiseFlatCString(aSpec)); + if (NS_FAILED(rv)) + { + mBaseURL->SetSpec(originalSpec); + } typo - "convenient" + * URI locally (to allow convinent get/set), but always updates the mBaseURL
Attachment #335582 -
Flags: review?(bienvenu) → review+
Assignee | ||
Comment 11•16 years ago
|
||
Changed quite a few things from the last patch, so requesting review again. Highlights of changes: - nsILDAPURL now provides an init function, reflect that which nsIStandardURL has. - nsLDAPProtocolHandler now calls the init function - nsLDAPProtocolHandler will set up the default port to the correct value (rather than -1) if USE_TK_LOGIN_MANAGER is defined in the environment. This is so that we don't break existing logins whilst using wallet. When we do the full switch-over, I have verified the toolkit password manager will correctly migrate the passwords and "strip" the default port numbers from the URLs, and hence we'll continue picking up the saved passwords. - We now return NS_ERROR_NOT_INITIALIZED if the base URL isn't set up. - The preference pane now sets the URLs up bits at a time. We still have to provide a temporary URL because the c-sdk requires it (not just nsStandardURL as I first thought). This is potentially a c-sdk bug, but I don't want to fix that here. - nsAbLDAPDirectory::GetLDAPURL now uses the IO Service to create and get the URL. - Defined nsLDAPURL::Clone, nsAbLDAPDirectoryQuery now clones the URL rather than getting all the individual attributes, formulating a string and then forming a URL from the string. - Updated nsAbLDAPListenerBase to the toolkit interfaces (via an ifdef), so that code is ready to go as well.
Attachment #335582 -
Attachment is obsolete: true
Attachment #337451 -
Flags: superreview?(dmose)
Attachment #337451 -
Flags: review?(bienvenu)
Attachment #335582 -
Flags: superreview?(dmose)
Assignee | ||
Comment 12•16 years ago
|
||
Minor change to fix the test_ldap1.js test that this patch had broken.
Attachment #337451 -
Attachment is obsolete: true
Attachment #337657 -
Flags: superreview?(dmose)
Attachment #337657 -
Flags: review?(bienvenu)
Attachment #337451 -
Flags: superreview?(dmose)
Attachment #337451 -
Flags: review?(bienvenu)
Updated•16 years ago
|
Attachment #337657 -
Flags: review?(bienvenu) → review+
Comment 13•16 years ago
|
||
Comment on attachment 337657 [details] [diff] [review] The fix v4a nit: +nsLDAPURL::SetPathInternal(const nsCString &aPath) +{ + PRUint32 rv, count; this code seems to be mixing ldap rvs with nsresults, i.e., rv starts out as an ldap sdk result, and then ends up as an nsresult. // don't quite line up here: + nsCString mFilter; // LDAP search filter + PRUint32 mOptions; // Options + nsCStringArray mAttributes; // List of attributes typo in comment: + do_check_eq(attrs.length, 0); + + // Nothing should happend if the attribute doesn't exist + url.removeAttribute("abc");
Assignee | ||
Comment 14•16 years ago
|
||
Fixes bienvenu's nits.
Attachment #337657 -
Attachment is obsolete: true
Attachment #342127 -
Flags: superreview?(dmose)
Attachment #342127 -
Flags: review+
Attachment #337657 -
Flags: superreview?(dmose)
Assignee | ||
Comment 15•16 years ago
|
||
Very minor change. I noticed whilst working on one of the other password manager patches that nsIProtocolHandler.URI_NORELATIVE had an extra underscore in it in the v4b patch.
Attachment #342127 -
Attachment is obsolete: true
Attachment #351448 -
Flags: superreview?(dmose)
Attachment #351448 -
Flags: review+
Attachment #342127 -
Flags: superreview?(dmose)
Comment 16•15 years ago
|
||
Comment on attachment 351448 [details] [diff] [review] The fix v4c Looks good; just some minor stuff: >-[scriptable, uuid(7310562d-1567-43c7-a123-633c1ba3663e)] >+[scriptable, uuid(1018ee06-f708-4bd7-b7df-11285332a404)] > interface nsILDAPURL : nsIURI { > > /** >@@ -152,4 +152,29 @@ interface nsILDAPURL : nsIURI { > */ > const unsigned long OPT_SECURE = 0x01; > >+ /** >+ * Initialize an LDAP URL >+ * While putting stuff at the end of an interface is often good for C++ binary compatibility reasons, that battle is already lost here since the semantics have changed. I think it's probably worth putting this at the top of the interface and being explicit that it must be called for one of the implementing objects can be used. >+ * @param aUrlType - one of the URLTYPE_ flags listed above. >diff --git a/directory/xpcom/base/src/nsLDAPProtocolHandler.js b/directory/xpcom/base/src/nsLDAPProtocolHandler.js > >+ protocolFlags: nsIProtocolHandler.URI_NORELATIVE | >+ nsIProtocolHandler.URI_DANGEROUS_TO_LOAD, I suspect that, if we actually cared about newChannel in normal builds, we'd also want ALLOWS_PROXY here. More realistically, I think that at some future time, we should probably just get rid of the nsLDAPChannel code entirely. >diff --git a/directory/xpcom/tests/unit/test_nsLDAPURL.js b/directory/xpcom/tests > > [...] > >+ // XXX Still need to check ldaps when the protocol handler can create one Looks to me like the protocol handler is now capable. Maybe just clone one of the existing tests but s/ldap/ldaps/ ? >+function run_test() { > > [...] > >+ url = url.QueryInterface(Components.interfaces.nsILDAPURL); Really, instead of having to QI here, nsLDAPURL ought to have classinfo. Maybe add an XXX comment to that effect? Very thorough tests, btw; nice work! >diff --git a/mailnews/addrbook/prefs/resources/content/pref-directory-add.js b/mailnews/addrbook/prefs/resources/content/pref-directory-add.js >@@ -201,9 +200,15 @@ function fillSettings() > sub.radioGroup.selectedItem = sub; > break; > } >- >- if (ldapUrl.options & ldapUrl.OPT_SECURE) >+ >+ var secure = ldapUrl.options & ldapUrl.OPT_SECURE >+ if (secure) > document.getElementById("secure").setAttribute("checked", "true"); >+ >+ document.getElementById("port").value = >+ ldapUrl.port == -1 ? >+ (secure ? kDefaultSecureLDAPPort : kDefaultLDAPPort) : >+ ldapUrl.port; Nested ternary operations make code hard to read; please get rid of that. >+ // Due to the LDAP c-sdk pass a dummy url to the IO service, then >+ // update the parts. Please make this comment a little more explicit. It sounds like this is actually a workaround to a bug; is that true? >diff --git a/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp b/mailnews/addrbook >+ >+ rv = url->SetAttributes(returnAttrsCount, (const char**)returnAttrsArray); Prefer C++-style casts: they express intent more explicitly to the compiler, which allows it to catch more bugs at build time. Thanks for the patch, and sorry for the review delay. sr=dmose with nits picked.
Attachment #351448 -
Flags: superreview?(dmose) → superreview+
Comment 17•15 years ago
|
||
>
> >+ * @param aUrlType - one of the URLTYPE_ flags listed above.
>
This was supposed to be accompanied by the comment: there is no "above" here, maybe add an @seealso pointing to nsIStandardURL?
Assignee | ||
Comment 18•15 years ago
|
||
(In reply to comment #16) > >+ // Due to the LDAP c-sdk pass a dummy url to the IO service, then > >+ // update the parts. > > Please make this comment a little more explicit. It sounds like this is > actually a workaround to a bug; is that true? I raised bug 473351 and referenced it in the code. Other comments addressed. Patch checked in: http://hg.mozilla.org/comm-central/rev/f13a45c4f1f9
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
You need to log in
before you can comment on or make changes to this bug.
Description
•