210870 - LDAP Roaming

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Profile: Roaming, enhancement)

Description

[Pete Zha]

Enable LDAP roaming.
This is a feature that can let user store profile on LDAP server. Based on bug
124029 which implemented HTTP and FTP roaming, This bug is focuing on LDAP
implementation.

Comment 1

[Ben Bucksch (:BenB)]

Pete, that's great :).

Did you use the progress dialog or did you implement another "roaming protocol"
(mozSRoamingProtocol.h)?

Comment 2

[Peter Lairo]

Is it entended that the ENIRE profile be stored on LDAP, or will the user be
able to select which parts of his profile are stored on LDAP?

e.g., ONLY bookmarks, passwords and prefs.js settings, but NOT "Local Folders".

Comment 3

[Pete Zha]

Ben, I reused the progress dialog in your patch. In fact, what I want to do is
just extend your patch to add LDAP support.

>e.g., ONLY bookmarks, passwords and prefs.js settings, but NOT "Local Folders".
Yes, you will be able to select which file need to store on server. It's just
same as HTTP/FTP roaming since I reused the UI and the implementation. I'm now
working on this and will give out a patch for testing soon.

Comment 4

[Peter Van der Beken [:peterv]]

You probably want the patch in bug 124897.

Comment 5

[Pete Zha]

->1.8b. Will spend sometime on this recently.

Comment 6

[Pete Zha]

Attachment: patch v1

Comment 7

[Pete Zha]

not depend on 124897 since we can resolve this bug without it.

Comment 8

[Ben Bucksch (:BenB)]

LDAP support is great.

I can't review the LDAP stuff (most of the patch), please ask dmose for that.

Roaming parts:

+        if (isLDAP)
+          uploadChannel.setUploadStream(bis, "application/roaming-file", -1);
+        else
+          uploadChannel.setUploadStream(bis, file.mimetype, -1);

Why that? Can't LDAP deal with the real mimetype (e.g. text/plain)?

+        this.baseURL.spec = this.baseURL.spec.replace(

this.baseURL.spec shoudl equal this.remoteDir, right?

+      if (this.baseURL.scheme == "ldap") {

/Maybe/ turn this around, so that the normal (non-LDAP) case comes first.

+        // attribute
+        mRemoteBaseUrl.Append("?nslidata");
+        // scope
+        mRemoteBaseUrl.Append("?base");
(in a row)

huch? Why not
mRemoteBaseUrl.Append("?nslidata?base");
? This also makes clear that you are adding 2 "?". Is that syntactically valid?
shouldn't you use "?...&..."?

+ this.baseURL.spec = this.baseURL.spec.replace(
+                     "nsLIProfileName",
+                     "nsLIElementType=" + file.filename + ", nsLIProfileName"

Tsn't there a separator missing? And can you reply on nsLIProfileName to be in
the URL or can you use something else as anchor/position for insertion?

Comment 9

[Ben Bucksch (:BenB)]

s/reply/rely/

> Tsn't there a separator missing?

Nevermind that, there's a ", " which I overlooked.

Comment 10

[Pete Zha]

Thanks for the quick review :-)

> I can't review the LDAP stuff (most of the patch), please ask dmose for that.
Yeah, I will find him to review it.
> 
> Why that? Can't LDAP deal with the real mimetype (e.g. text/plain)?
LDAP can not deal with content type. Actually I set this content type for LDAP
module that can get to know I'm uploading roaming data. So, the
nsRoamingLDAPOperation can create necessary object and set it to server.
> 
> +        this.baseURL.spec = this.baseURL.spec.replace(
> 
> this.baseURL.spec shoudl equal this.remoteDir, right?
Yes, they are same, and I can change it to use this.remoteDir. It's more clear.
> 
> +      if (this.baseURL.scheme == "ldap") {
> 
> /Maybe/ turn this around, so that the normal (non-LDAP) case comes first.
OK.

> huch? Why not
> mRemoteBaseUrl.Append("?nslidata?base");
OK, I can do that.

> shouldn't you use "?...&..."?
No, we don't use that format.

Comment 11

[Ben Bucksch (:BenB)]

> Actually I set this content type for LDAP
> module that can get to know I'm uploading roaming data

Can you use something else for signalling? Seems like a hack, and I didn't
understand it.

> Yes, they are same, and I can change it to use this.remoteDir.

You don't have to, but maybe make a comment to clarify, and keep them in sync.

>> shouldn't you use "?...&..."?

> No, we don't use that format.

Who is "we?" That's a URL, right? IIRC, in URLs (no matter which one), the query
string starts with "?" and any additional parameters are added with "&", not
"?". Or isn't that a query (syntactically).

Comment 12

[Pete Zha]

> Who is "we?" That's a URL, right? IIRC, in URLs (no matter which one), the query
> string starts with "?" and any additional parameters are added with "&", not
> "?". Or isn't that a query (syntactically).
In LDAP URL spec, the first parameter after "?" in URL is the attribute, the
second is the scop. You can look into the nsLDAPURL, nsLDAPChannel to find the
detail.

Comment 13

[Ben Bucksch (:BenB)]

hm, you are right, RFC 2255 <http://asg.web.cmu.edu/rfc/rfc2255.html> agrees
with you, but as I read RFC2396 (path/query component, reserved characters),
that's invalid. Anyways, it's surely OK to have that in our code.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

The LDAP XPCOM SDK itself shouldn't need to know anything about roaming
specifically.  The nsIUploadChannel changes make some sense, but much of the
rest of the LDAP code should live in the sroaming code and use the new
interfaces proposed in 124897.  I'll try and start reviewing those interfaces soon.

Comment 15

[Pete Zha]

(In reply to comment #14)
> rest of the LDAP code should live in the sroaming code and use the new
> interfaces proposed in 124897.
It's hard to implement Roaming Upload based on the interface proposed in 124897.
Unless we implement another channel(maybe LDAPRoamingChannel) in Roaming
module... Do you suggest so?

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

I think we should be able to do it by keeping most of the channel code where it
is.  My thought here is that we may need some hooks that allow one to define how
the upload channel does its work.  This might require a new interface (eg
nsILDAPChannel) so that code in the sroaming directory can tweak specific bits
of how the nsLDAPChannel does its work.  Also, I don't mean "exactly the patch
in 124897", I just mean along those general lines, since I still need to review
that patch.  But if doing this along those lines requires that you propose some
changes to the patch in 124897, that's fine.  I'll try and do some of the
reviewing on that patch in the next week or two, so you may want to just hold
off until that happens.

Comment 17

[Pander]

Would be fantastic if profile can be stored in LDAP.

When specifiying an LDAP schema for this, perhaps define one which is also usable for both Thunderbird and Evolution. The difference in Evolution and Thunderbird addressbook LDAP schema is already different. Perhaps here start with a common definition. Both are mail clients and store much of the same information on POP/IMAP, SMTP server, bookmarks etc.

It would be very welcome to be able to share your bookmarks at home and at work via LDAP (even between different mail clients).

Comment 18

[Ben Bucksch (:BenB)]

Is there still interest in this?

Comment 19

[Ivan Beveridge]

[Sorry for the delay in responding .. I tried replying but it bounced :( ]

Most definitely, yes! I keep waiting/hoping/checking for this in every
major/point release :/

Cheers

Ivan

Comment 20

[Ben Bucksch (:BenB)]

Pete Zha, are you still working on this?

I don't have time nor interest to review this, though. In my capacity as module owner, I can delegate, though. Does anybody (who has contributed patches before) want to review the patch?

Comment 21

[Pete Zha]

Sorry... I don't have time on this either.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

The percentage of users (of any of the Mozilla products, really) who would benefit from LDAP roaming support strikes me as likely to be pretty small (5% or less, as a wild guess).  As such, I'd suggest that it actually makes more sense for this code to live in an extension rather than the core (naturally adding any extension points necessary for this to work).

Comment 23

[Ben Bucksch (:BenB)]

Comment on attachment 162688 [details] [diff] [review]
patch v1

dmose, sroaming already is an extension.

To the patch: The majority of this patch is in the core LDAP code, e.g. in
nsLDAPChannel.cpp :
+    if (mUploadStream && mContentType.Find("application/roaming-file") >=0 ) {
+        mCurrentOperation = new nsRoamingLDAPOperation();
+        NS_ENSURE_TRUE(mCurrentOperation, NS_ERROR_OUT_OF_MEMORY);
+    } else {

Some other changes to LDAP code seem to make sense.

I think the changes to LDAP roaming should be confined to the sroaming code (or even separated from that, as dmose suggests), and not be in the core LDAP code - unless core LDAP has bugs or limitations that prevent it, that would be fair to change. But otherwise, only general LDAP write operations should be used.

So, I'll have to give this an r-, unfortunately.

Comment 24

[MegaZone]

I suspect most users have moved on from waiting to using extensions like Foxmarks/Xmarks.

Comment 25

[Benjamin Smedberg]

This bug is filed in a bugzilla component related to pre-Firefox code which no longer exists. I believe it is no longer relevant and I am therefore closing it INCOMPLETE.

If you believe that this bug is still valid and needs to be fixed, please reopen it and move it to the Toolkit:Startup and Profile System product/component.