Closed Bug 419595 Opened 14 years ago Closed 13 years ago

nsLDAPURL implementation is incorrect

Categories

(Directory :: LDAP XPCOM SDK, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file, 8 obsolete files)

Attached patch Basic Unit Test (obsolete) — 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.
I'm now working on this.
Assignee: nobody → bugzilla
Keywords: helpwanted
Attached patch The fix (diff -w) (obsolete) — Splinter Review
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)
Attached patch The fix (Normal patch) (obsolete) — Splinter Review
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)
(Any update ?)
Priority: -- → P2
Attached patch The fix v2 (obsolete) — Splinter Review
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 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-
(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.
Attached patch The fix v3 (obsolete) — Splinter Review
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 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+
Attached patch The fix v4 (obsolete) — Splinter Review
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)
Attached patch The fix v4a (obsolete) — Splinter Review
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)
Attachment #337657 - Flags: review?(bienvenu) → review+
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");
Attached patch The fix v4b (obsolete) — Splinter Review
Fixes bienvenu's nits.
Attachment #337657 - Attachment is obsolete: true
Attachment #342127 - Flags: superreview?(dmose)
Attachment #342127 - Flags: review+
Attachment #337657 - Flags: superreview?(dmose)
Attached patch The fix v4cSplinter Review
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)
Blocks: 433316
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+
> 
> >+     * @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?
(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: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Depends on: 473594
Depends on: 473715
Depends on: 475170
Depends on: 474439
Depends on: 479438
You need to log in before you can comment on or make changes to this bug.