I added simple password authentification to the trunk ; can you check if that meets your needs?

(In reply to comment #1)
> I added simple password authentification to the trunk ; can you check if that
> meets your needs?

There's still no prompt for the LDAP DN to authenticate with, so it doesn't help.

I've pulled the SVN code and am looking at implementing more extensive LDAP support now. The code really needs to use LDAPURLDesc's instead of passing hostname/port# around. That will also simplify persisting the settings in the config, most of the items can go into the URL:

   ldap[s]://hostname:port/baseDN?attributes?scope?filter[?extensions]

The current code is pretty broken; while the input form lets you choose an alternate attribute to retrieve, the code that parses responses is hardcoded to use "telephoneNumber". In a proper LDAP client all of the relevant session information (everything that comprises the LDAP URL) should be fully configurable. I'll be fixing this telephoneNumber issue as well.

For the optional Extensions part, there probably ought to be an option to turn on StartTLS.

A decent client should also offer the option of authenticating with SASL, and specifying the SASL mechanism and any mech-specific parameters. For the moment, I'll try adding support to turn on SASL and choose a mech or let the client negotiate with the server.

The big question in my mind is how people will react to the UI changes that will be necessary to expose all these options.

It would probably be ugly to put too much of the URI into a single input field. I think a single input field for the server location "ldap[s]://hostname:port" would be OK. The BaseDN can remain as its own field; the attributes field can remain as its own (and should allow for a comma-separated list of attribute names). Scope can remain. Currently there's no option to customize the search filter, it just uses "(cn=*name*)". Probably a new input box should be added for that, but is it perhaps too "advanced" a feature to display by default?

I think a dropdown box for StartTLS[off|optional|required] should also be present, as well as a checkbox for SASL (instead of Simple Auth) and an input field for the mech name. Again, maybe too advanced a feature to display by default.

And of course, an input field for the userID: an LDAP DN for Simple Auth, or a SASL username for SASL Auth.

The current module is also hardcoded to use givenName to display the addressbook contact. At the very least, it needs to be able to fallback to cn in case the entry has no givenName. Most people probably want to see the full name anyway, because they probably have multiple contacts with the same first name. Again, it would be best to make this choice fully configurable. That may be too much hassle to handle at the moment.

Created attachment 119998 [details] [review]
Proposed patch

Add a field for entering the authentication ID, merge hostname/port into URI.

I'm glad you want to work on it : I must admit that code is full of compile-time decisions, and was only tested against ekiga.net.

Reading your code : it's pretty neat except for two things, one small, one big :
(*) the small one is that line: std::string new_authcID = result.text ("authcID");
   --> would it be possible to find something less scary for the average user?

(*) the big one is that your code works within itself -- it would be fine if it would be to enter 3.0. But the problem is that 3.0 is out already, so it would need to read&update 3.O-style configuration : if it finds a hostname/port configuration, it should remove it and replace it with a new-style configuration (remove *after* it got the information, of course).

Thanks!

Hmm, I was pretty sure that "authcID" doesn't show up in any dialogs; it uses "Bind ID" in the actual forms. authcID is the term used for SASL Binds... Would it be better to use Bind_ID throughout?

re: the config change, I'll take a look at that.

Some of my comments in #2 above also relate to Bug#351953...

Oups, sorry, I pasted the wrong line ; yes, the text is :
+ request.text ("authcID", _("Bind _ID"), authcID);
which isn't 100% clearer ;-)

For the SASL checkbox, I don't think we have code using that yet, but the infrastructure is in place :
- in lib/engine/framework/form.h, the "boolean" method allows to read such a setting from a form result : "use_sasl = result.boolean ("use_sasl");" should be enough ;
- in lib/engine/framework/form-request-simple.h, another "boolean" method allows adding such a setting control in a form -- something like "request.boolean ("use_sasl", _("Use SASL"), true);" should be enough.

I hope this helps.

Created attachment 120101 [details] [review]
With config migration

This patch is the same as before, but it also upgrades the config (reads old host/port and constructs uri from them, then deletes host/port from config).

If we're satisfied with this then I'll add SASL as a followon patch. I'm also still thinking about merging all of URI/base/attr/scope/filter into a single LDAP URL but that also seemed like too drastic a change for the moment. (But it may help our sanity by keeping the function argument lists shorter as we add the rest of the authentication options...)

Yesterday I added support in libldap for enabling StartTLS via a URL extension. As such, that would allow this code to use StartTLS without any further changes, so that's one less feature we need to write new support for.
E.g., if you configure the URI to "ldap://example.com/????StartTLS" then it will use TLS on the session. I figure by the time ekiga 3.2 is released the requisite OpenLDAP library version should be widely available...

You don't need the "upgraded_config" boolean : you just have to do "trigger_saving.emit ()" just like in the on_edit_form_submitted method.

If you think merging all in the uri is the future : I'm all for it (and marking the patch as "needs-work" for that reason)!

Indeed, if we don't do it now, that means changing the way the configuration is serialized once again in the future -- and that is bad!

This work doesn't look like it will be ready for 3.01 anyway, so if it takes a little long to get right, let's just take that long : that's that much less pain for later!

PS: I notice you use xmlNewChild -- check the code I committed today, where I fixed a problem where we didn't escape the strings good enough ; the modifications look like this :
   xmlNewChild (node, NULL,
-	       BAD_CAST "name", BAD_CAST name.c_str ());
+	       BAD_CAST "name",
+	       BAD_CAST robust_xmlEscape (node->doc, name).c_str ());

You'll probably want to do something similar.

OK. I've cleaned up those two points and am now working on the rest of the URI conversion...

I have a last question (which woke me up this night) : how dependant on openldap are your proposed changes?

I remember we had problem because on solaris they don't use openldap, but some other ldap lib -- which is mostly compatible, but still different.

I found the report I was thinking about : see bug #552765, where we had to add solaris-specific LDAP initialization in ldap-book.cpp.

(In reply to comment #12)
> I found the report I was thinking about : see bug #552765, where we had to add
> solaris-specific LDAP initialization in ldap-book.cpp.
 
Hm, that's interesting, what version of Solaris was that? ldap_initialize() has been part of libldap since December 1999.

re: "-lldap -llber" vs "-lldap": when libldap is correctly built as a shared library, it is explicitly linked against liblber already. As such, linking against -lldap will pull in -llber implicitly, so you really don't need to specify it explicitly in the Makefile.

One last note: Sun is abandoning their (obsolete) libldap and adopting OpenLDAP's library anyway, so this will all be a moot point soon. The OpenLDAP version of libldap is the only one being actively maintained (and enhanced) in the LDAP community; most other vendors (Novell, IBM, Apple, ISODE, etc...) adopted it ages ago.

http://mail.opensolaris.org/pipermail/sparks-discuss/2008-August/000727.html

Oh, and the direct answer to the question - I'm pretty sure we can move to URI-based config using only common ldap_url_*() functions, most of those have been around since 1995 at least...

Looks good.

Will the lde_url_* functions handle the new way of enabling SASL too, or will some distribution get stuck with an older lib which won't support it?

SASL still has to be handled explicitly. I guess we ought to handle TLS explicitly as well, if the library is too old.

The forms handling is fairly rudimentary. With the current code you can zero out several of the fields (including the address book name itself) and it will blithely accept them. I would imagine that things get pretty ugly if you try to set two books with the same name, or with NULL/zero length names. Can we do any kind of input validation before they submit the form, or can we put up an error dialog after it's submitted, if it's missing required fields?

Yes, you can do validation after-the-fact, and send the form back to the user with an error message :

if (something_is_wrong_with (result)) {

  Ekiga::FormRequestSimple request;

  result.visit (request);
  request.error (_("Explicit error message"));
  request.submitted.connect (sigc::mem_fun (this,
                                    &OPENLDAP::Book::on_edit_form_submitted));

  if (!questions.handle_request (&request)) {

    // FIXME: better error reporting
#ifdef __GNUC__
    std::cout << "Unhandled form request in "
              << __PRETTY_FUNCTION__ << std::endl;
#endif
  }
}

Created attachment 120244 [details] [review]
Almost complete

This is nearly complete. All of the form and config processing is done; I just have to plug in the hooks for SASL authentication and add the form validation checks. Just giving you an idea of where things are headed.

Pretty much everything is configurable and StartTLS is working. You can set a "filter template" where "$" in the template is replaced by whatever the user entered in the search_filter field. I've preserved the original behavior, where providing a name is treated as a substring match: foo + (cn=$) -> (cn=*foo*) and where no name is treated as a presence match: "" + (cn=$) -> (cn=*)

I've also extended it so that if the search_filter field begins with '(' and ends with ')' then it's used as the filter directly, ignoring the template. So an LDAP-savvy person can use whatever LDAP filter they want if the default/template isn't suitable.

Created attachment 120245 [details]
Screenshot of new LDAP Directory dialog

to give you an idea of how things look now

Created attachment 120246 [details] [review]
With form validation

This adds form validation to the previous patch.

I haven't read the patches yet, but the screenshot leads to two questions already:

- one for you : is "SASL Mechanism" just a text entry -- can't it be a choice in a closed list like the search scope?

- one for me : why isn't "Search Scope" sanely left aligned !? (reported as bug #555637)

Created attachment 120251 [details] [review]
Use a choicelist for SASL mechs

(In reply to comment #21)
> I haven't read the patches yet, but the screenshot leads to two questions
> already:
> 
> - one for you : is "SASL Mechanism" just a text entry -- can't it be a choice
> in a closed list like the search scope?

I was a little reluctant to go that route because it requires a couple explicit calls into the Cyrus SASL library. We probably need to add autoconf tests to detect them in that case. This patch adds the two SASL calls.

Probably we should have added an ldap_get_option() option in libldap to retrieve this list. I really don't like exposing the Cyrus SASL dependencies at this level...

Your patches look good -- only nitpick is that there was a FIXME comment about prepending "sip:" forcibly and unconditionally to the uris : it would be nice to add one too so it's easy to grep.

Do you want those patches to get in as-is or do you want to work on them a little more?

(In reply to comment #23)
> Your patches look good -- only nitpick is that there was a FIXME comment about
> prepending "sip:" forcibly and unconditionally to the uris : it would be nice
> to add one too so it's easy to grep.

OK, can do. But what's the actual desired fix there? It may be more sensible to just Do It Right now. Should we check first to see if there's already some kind of URI prefix and use the value directly if so?

> Do you want those patches to get in as-is or do you want to work on them a
> little more?

Well, everything except SASL is working in there. With the current patch, if you enable SASL you'll get no authentication at all (because it leads to an empty if block...) If you're cool with leaving things like that temporarily, then go ahead and commit. I'm working on filling out that functionality now.

By the way, do you know what the deal is with struct RefreshData in ldap-book.cpp? It seems to be totally unused, was there a future need for it?

Created attachment 120306 [details] [review]
Basic SASL Bind implementation

OK, this adds the missing SASL functionality. I've also reinserted the FIXME comment you mentioned above.

Note that this SASL Bind will work for the most common mechs (like DIGEST-MD5, GSSAPI, EXTERNAL) but not for something more interactive like OTP. For that we need to be able to put up a Challenge/Response dialog to the user and get their input *synchronously*, and I don't know how to do that with the current form-request-simple stuff.

I'll try to attack from the libldap side by writing a new ldap_sasl_*bind function that can operate asynchronously.

By the way, any guidelines on adding my name to the comment header at the top of the changed files?

Oh one more thing I forgot: there's a bug in Cyrus SASL releases up to and including the current 2.1.22, which will cause apps to get a SEGV when authenticating. The bug is described in this thread

http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-sasl&msg=8954

and a fix is in the current Cyrus SASL CVS. I don't know when the fix will be in any public release.

Created attachment 120311 [details] [review]
More error details

Display a slightly more specific error message if the LDAP session can't be started.

The comment header generally has a "begin: written in ... by ..."
You can make it look like:
begin: written in ... by ...
       completed in 2008 by Howard Chu

I'm pretty sure we have a list of authors somewhere for the about window, in which you would definitely fit... but I don't remember where.

Created attachment 120324 [details] [review]
Full SASL Bind support

This patch adds dialog handling to request any other SASL parameters that aren't normally used. (E.g., OTP challenge/response handling.)

That pretty much wraps it up. I'll follow up with one more patch for the comment headers...

Created attachment 120325 [details] [review]
author credits...

If I touched the wrong Authors files here feel free to ignore those bits...

All is in : thanks!