Mailing List Archive

LDAP fixes
Hi! I've been setting up DAViCal and ran into some problems with LDAP synchronization. The following patch got me up and running. The first couple of edits clean up the code and solve a problem with groups with no members at all getting the same members as the previous group with members. The last two changes format the timestamps in the right way. Note that I haven't looked at the else-parts following

$valid[$mapping['modified']] = "$Y-$m-$d $H:$M:$S";

as I don't understand why they are there at all, since in two other places in the code timestamps are converted in a similar manner without testing if $c->authenticate_hook['config']['format_updated'] is empty.

Best regards,

Rasmus

-----------------------------------------------------------------
Hansen, Rasmus Borup Intomics - from data to biology
System Administrator Diplomvej 377
Scientific Programmer DK-2800 Kgs. Lyngby
Denmark
E: rbh@intomics.com W: http://www.intomics.com/
P: +45 5167 7972 P: +45 8880 7979




---
inc/drivers_ldap.php | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/inc/drivers_ldap.php b/inc/drivers_ldap.php
index a784e48..c4eb46c 100644
--- a/inc/drivers_ldap.php
+++ b/inc/drivers_ldap.php
@@ -127,7 +127,6 @@ class ldapDrivers
join(', ', $attributes),
$baseDNUsers);
}
- $row = array();
for($i = ldap_first_entry($this->connect,$entry);
$i && $arr = ldap_get_attributes($this->connect,$i);
$i = ldap_next_entry($this->connect,$i) ) {
@@ -158,10 +157,10 @@ class ldapDrivers
join(', ', $attributes),
$baseDNGroups);
}
- $row = array();
for($i = ldap_first_entry($this->connect,$entry);
$i && $arr = ldap_get_attributes($this->connect,$i);
$i = ldap_next_entry($this->connect,$i) ) {
+ $row = array();
for ($j=0; $j < $arr['count']; $j++) {
$row[$arr[$j]] = count($arr[$arr[$j]])>2?$arr[$arr[$j]]:$arr[$arr[$j]][0];
}
@@ -549,6 +548,7 @@ function sync_LDAP(){
foreach($c->authenticate_hook['config']['format_updated'] as $k => $v)
$$k = substr($ldap_timestamp,$v[0],$v[1]);
$ldap_timestamp = $Y.$m.$d.$H.$M.$S;
+ $valid[$mapping['modified']] = "$Y-$m-$d $H:$M:$S";
}
else if ( preg_match('{^(\d{8})(\d{6})(Z)?$', $ldap_timestamp, $matches ) ) {
$ldap_timestamp = $matches[1].'T'.$matches[2].$matches[3];
@@ -556,7 +556,6 @@ function sync_LDAP(){
else if ( empty($ldap_timestamp) ) {
$ldap_timestamp = date('c');
}
- $valid[$mapping['modified']] = $ldap_timestamp;

sync_user_from_LDAP( $principal, $mapping, $valid );
}
--
1.7.9.5



_______________________________________________
DAViCal-dev mailing list
DAViCal-dev@lists.davical.org
http://lists.davical.org/listinfo/davical-dev
Re: LDAP fixes [ In reply to ]
On Fri, 2012-05-11 at 15:21 +0200, Rasmus Borup Hansen wrote:
> Hi! I've been setting up DAViCal and ran into some problems with LDAP
> synchronization. The following patch got me up and running. The first
> couple of edits clean up the code and solve a problem with groups with
> no members at all getting the same members as the previous group with
> members. The last two changes format the timestamps in the right way.
> Note that I haven't looked at the else-parts following
>
> $valid[$mapping['modified']] = "$Y-$m-$d $H:$M:$S";
>
> as I don't understand why they are there at all, since in two other
> places in the code timestamps are converted in a similar manner
> without testing if $c->authenticate_hook['config']['format_updated']
> is empty.

Hi Rasmus,

Thanks for your patch. It seems that the original author of the LDAP
driver wrote the date-handling in some possibly over-complicated way
that let you specify a format for the date that was returned from the
LDAP server...

Given that I've never actually got around to setting up an LDAP server
myself, either for testing, or for some real life need, I just have to
depend on whatever people send me for this so the code quality gets
pretty uneven. It seems to me that it might be possible different LDAP
servers to return dates in different formats though, so that code might
be needed. Maybe.

Of course if I've made it this far through life without desperately
needing an LDAP server for something the odds of me actually running one
end up being pretty low, so unless someone volunteers to look after this
code (including responding to the mailing list and IRC) it'll continue
to be a second class citizen.

As a result I'm quite tempted to apply your patch, but I'm not sure that
I believe moving an assignment like:
$row = array();
into a loop that does not consume $row seems either necessary or
expected. Could you perhaps provide a better justification than " it
works for me", or a more carefully scoped patch?

Cheers,
Andrew.

>
>
> ---
> inc/drivers_ldap.php | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/inc/drivers_ldap.php b/inc/drivers_ldap.php
> index a784e48..c4eb46c 100644
> --- a/inc/drivers_ldap.php
> +++ b/inc/drivers_ldap.php
> @@ -127,7 +127,6 @@ class ldapDrivers
> join(', ', $attributes),
> $baseDNUsers);
> }
> - $row = array();
> for($i = ldap_first_entry($this->connect,$entry);
> $i && $arr = ldap_get_attributes($this->connect,$i);
> $i = ldap_next_entry($this->connect,$i) ) {
> @@ -158,10 +157,10 @@ class ldapDrivers
> join(', ', $attributes),
> $baseDNGroups);
> }
> - $row = array();
> for($i = ldap_first_entry($this->connect,$entry);
> $i && $arr = ldap_get_attributes($this->connect,$i);
> $i = ldap_next_entry($this->connect,$i) ) {
> + $row = array();
> for ($j=0; $j < $arr['count']; $j++) {
> $row[$arr[$j]] = count($arr[$arr[$j]])>2?$arr[$arr[$j]]:$arr[$arr[$j]][0];
> }
> @@ -549,6 +548,7 @@ function sync_LDAP(){
> foreach($c->authenticate_hook['config']['format_updated'] as $k => $v)
> $$k = substr($ldap_timestamp,$v[0],$v[1]);
> $ldap_timestamp = $Y.$m.$d.$H.$M.$S;
> + $valid[$mapping['modified']] = "$Y-$m-$d $H:$M:$S";
> }
> else if ( preg_match('{^(\d{8})(\d{6})(Z)?$', $ldap_timestamp, $matches ) ) {
> $ldap_timestamp = $matches[1].'T'.$matches[2].$matches[3];
> @@ -556,7 +556,6 @@ function sync_LDAP(){
> else if ( empty($ldap_timestamp) ) {
> $ldap_timestamp = date('c');
> }
> - $valid[$mapping['modified']] = $ldap_timestamp;
>
> sync_user_from_LDAP( $principal, $mapping, $valid );
> }

--
------------------------------------------------------------------------
andrew (AT) morphoss (DOT) com +64(272)DEBIAN
Building more free and open source software for everyone.
------------------------------------------------------------------------
Re: LDAP fixes [ In reply to ]
On May 14, 2012, at 9:29 , Andrew McMillan wrote:

> As a result I'm quite tempted to apply your patch, but I'm not sure that
> I believe moving an assignment like:
> $row = array();
> into a loop that does not consume $row seems either necessary or
> expected. Could you perhaps provide a better justification than " it
> works for me", or a more carefully scoped patch?

The outer loop (the one using $i as iterator) loops through all groups. The inner loop (the one using $j) loops through all the attributes of a group, ie. its members, and modifies $row accordingly. If a group does not have any members, the inner loop will never be entered and $row will still have the value that was computed for the previous group. Please compare the logic with getAllUsers where $row is correctly reset inside the $i-loop just as I suggest it be done in getAllGroups.

Best regards,

Rasmus

-----------------------------------------------------------------
Hansen, Rasmus Borup Intomics - from data to biology
System Administrator Diplomvej 377
Scientific Programmer DK-2800 Kgs. Lyngby
Denmark
E: rbh@intomics.com W: http://www.intomics.com/
P: +45 5167 7972 P: +45 8880 7979

_______________________________________________
DAViCal-dev mailing list
DAViCal-dev@lists.davical.org
http://lists.davical.org/listinfo/davical-dev