Tickets :: Comment on [#551] Forwards LDAP driver

* Your Email Address
* Spam protection	Enter the letters below: .__ . ..__ __ .__. [__)\|_/ [__)/ `\| \| \| \| \\| \__.\|__\|
Comment	> I offer no warrantee on this code -- I'm not a native PHP speaker. > But it seems to work so far. > > > > The patches below address several issues that I found in the LDAP > driver for Forwards. Each of the main issues is noted in the code > with a comment beginning // :ptf: (PTF is my initials). > > > > First, a small patch to forwards/main.php. It seems to me that > "forwarding is enabled" should not be an error. > > > > --- main.php.orig 2004-09-07 20:01:11.904287000 -0400 > > +++ main.php 2004-09-07 17:01:47.684079000 -0400 > > @@ -85,8 +85,9 @@ > > // this fails, it could be because it is disabled, or just because we > > // can't tell, so just be quiet about it. > > if ($driver->isEnabledForwarding($user, $realm, > Auth::getCredential('password'))) { > > + // :ptf: Why was this an ERROR? > > Horde::raiseMessage( _("Forwarding is currently enabled."), > > - HORDE_ERROR); > > + HORDE_SUCCESS); > > } > > > > include_once $registry->getTemplatePath('horde') . > '/javascript/open_help_win.js'; > > > > > > > > Now for the meat of it -- changes to forwards/lib/Driver/ldap.php. > In summary: > > 1) There was no function Forwards_Driver_ldap() > > 2) The local delivery flag was ignored. Added a config param > (localEmail) to support this function. > > 3) Errors such as "insufficient access" from the EnableForwarding() > function were not properly returned to the calling routine > > 4) isEnabledForwarding() needed to return Boolean values, not 'Y' and 'N' > > 5) Renamed _getForwards to _getFirstValue, since the name is more > apt, and changed filter from 'uid=' to 'objectClass=' > > 6) Added a utility function to remove the 'count' from arrays > returned by the ldap_get() functions. > > > > Local Delivery still needs a bit of work. I added an attribute > (localEmail) to the config file to support this function. (The > alternative -- building an address out of the user and realm -- > doesn't work well if the realm is 'default'.) > > > > To properly handle local delivery, I need (1) to properly identify if > the local address is part of the forwarding string, and (2) if it is > there, strip it out when displaying the page and check off the "local > delivery" box. I should also handle the case where local delivery is > the only remaining address on the list. > > > > > > --- ldap.php.orig 2004-09-07 16:13:51.668142000 -0400 > > +++ ldap.php 2004-09-07 19:44:26.845636000 -0400 > > @@ -22,6 +22,17 @@ > > / > > var $_ds; > > > > + // :ptf: The function Forwards_Driver_ldap was missing ... (?) > > + /* > > + * Constructs a new ldap Forwards_Driver object. > > + * > > + * @param array $params A hash containing connection parameters. > > + / > > + function Forwards_Driver_ldap($params = array()) > > + { > > + $this->_params = $params; > > + } > > + > > /* > > * Check if the realm has a specific configuration. If not, try to fall > > * back on the default configuration. If still not a valid > configuration > > @@ -41,6 +52,7 @@ > > } > > > > // If still no host/port, then we have a misconfigured module. > > + > > if (empty($this->_params[$realm]['host']) \|\| > > empty($this->_params[$realm]['port']) ) { > > $this->_error = _("The module is not properly configured!"); > > @@ -55,7 +67,7 @@ > > * @param string $user The username to enable forwards for. > > * @param string $realm The realm of the user. > > * @param string $target The message to install. > > - * @param string $keeplocal Never used. > > + * @param string $keeplocal Deliver message to box and forward > it as well. > > * > > * @return boolean Returns true on success, false on error. > > / > > @@ -84,17 +96,64 @@ > > return false; > > } > > > > + > > + // :ptf: Handle "Local Delivery" flag > > + // Local delivery? > > + if ($keeplocal === 'on') { > > + > > + // What is the local mailbox? > > + if ( $this->_params[$realm]['localEmail']) { > > + $attribs = array($this->_params[$realm]['localEmail']); > > + $local_target = $this->_getFirstValue($userdn, $attribs); > > + // > > + } else { > > + $res = PEAR::raiseError(_("Cannot determine local > mailbox")); > > + $this->err_str = "Cannot determine local mailbox"; > > + $this->_disconnect(); > > + return false; > > + } > > + // An alternative would be to guess what the local box > > + // might be (see below). But guessing badly is a bad thing. > > + // } elseif ($realm == 'default' ) { > > + // $local_target = $user; > > + // } else { > > + // $local_target = $user . '@' . $realm; > > + // } > > + // > > + > > + > > + // :ptf: LocalDelivery handling still has a two bugs > > + // BUG: I copied this code from the SQL driver -- but > > + // it ain't perfect: > > + // if the local target is foo@bar.com, and the > > + // forwards list contains bigfoo@bar.com, then > > + // it will get the wrong answer > > + // > > + // Assumes that Forwards attribute is comma-delimited list > > + if($local_target AND !strstr($message,$local_target)) { > > + $message .= ', ' . $local_target; > > + } > > + > > + } else { // keeplocal is off > > + // BUG: We need to pull out the local target(s) if > > + // keeplocal is off > > + } > > + > > + > > // Change the user's forwards. > > $newDetails[$this->_params[$realm]['forwards']] = $message; > > $res = ldap_mod_replace($this->_ds, $userdn, $newDetails); > > + > > + // :ptf: Errors here were being ignored > > if (!$res) { > > $res = PEAR::raiseError(ldap_error($this->_ds)); > > + $this->err_str = ldap_error($this->_ds); > > + $this->_disconnect(); > > + return false; > > + } else { > > + $this->_disconnect(); > > + return true; > > } > > - > > - // Disconnect from the ldap server. > > - $this->_disconnect(); > > - > > - return true; > > } > > > > /* > > @@ -133,7 +192,7 @@ > > > > // Delete the user's forwards. > > $attribs = array($this->_params[$realm]['forwards']); > > - $value = $this->_getForwards($userdn, $attribs); > > + $value = $this->_getFirstValue($userdn, $attribs); > > if (!$value) { > > // Nothing to delete, so treat as success. > > return true; > > @@ -158,9 +217,10 @@ > > * > > * @param string $realm The realm of the user to check. > > * > > - * @return mixed Returns 'Y' if forwarding is enabled for the > > - * user or 'N' if forwarding is currently > > - * disabled. > > + * // :ptf: This function needs to return Boolean values, not strings > > + * @return boolean Returns true if forwarding is enabled for the > > + * user or false if forwarding is currently disabled or if > > + * an error occurs > > / > > function isEnabledForwarding($user, $realm, $pass) > > { > > @@ -175,7 +235,7 @@ > > } else { > > $userdn = $this->_lookupdn($user, $realm); > > if (is_a($userdn, 'PEAR_Error')) { > > - return $userdn; > > + return false; > > } > > } > > > > @@ -184,18 +244,18 @@ > > if (is_a($res, 'PEAR_Error')) { > > $this->_disconnect(); > > if ($res->getMessage() == _("Could not bind to ldap server")) { > > - return PEAR::raiseError(_("Incorrect Password")); > > + $res = PEAR::raiseError(_("Incorrect Password")); > > } > > - return $res; > > + return false; > > } > > > > $attribs = array($this->_params[$realm]['forwards']); > > - if ($this->_getForwards($userdn, $attribs)) { > > + if ($this->_getFirstValue($userdn, $attribs)) { > > $this->_disconnect(); > > - return 'Y'; > > + return true; > > } else { > > $this->_disconnect(); > > - return 'N'; > > + return false; > > } > > } > > > > @@ -236,7 +296,7 @@ > > > > $attribs = array($this->_params[$realm]['forwards']); > > > > - return $this->_getForwards($userdn, $attribs); > > + return $this->_getFirstValue($userdn, $attribs); > > } > > > > /* > > @@ -275,9 +335,20 @@ > > return $userdn; > > } > > > > - function _getForwards($userdn, $attribs) > > + // :ptf: Renamed this function > > + /** > > + * Get the first available attribute value for a particular DSN > > + * > > + * @param $userdn The ldap basedn. > > + * @param $attribs A list whose first entry is the attribute we seek > > + * > > + * @return string The value of the attribute, or false if not found > > + * > > + / > > + function _getFirstValue($userdn, $attribs) > > { > > - $sr = ldap_search($this->_ds, $userdn, 'uid=', $attribs); > > + // :ptf: Using objectClass, since we don't know if 'uid' exists > > + $sr = ldap_search($this->_ds, $userdn, 'objectClass=', $attribs); > > $entry = ldap_first_entry($this->_ds, $sr); > > $res = ldap_get_attributes($this->_ds, $entry); > > if ($res['count'] == 0) { > > @@ -288,6 +359,32 @@ > > return $values[0]; > > } > > > > + > > + // :ptf: Handy function to remove 'count' from ldap_get() results > > + /* > > + * Recursively remove the 'count' entry from an array > > + * > > + * Posted by mack@incise.org > > + * http://us4.php.net/manual/en/function.ldap-get-entries.php > > + * > > + * @param $arr The source array > > + * > > + * @return mixed The same array with no 'count' entries > > + * > > + / > > + function _rCountRemover($arr) > > + { > > + foreach($arr as $key=>$val) { > > + # (int)0 == "count", so we need to use === > > + if($key === "count") > > + unset($arr[$key]); > > + elseif(is_array($val)) > > + $arr[$key] = _rCountRemover($arr[$key]); > > + } > > + return $arr; > > + } > > + > > + > > /* > > * Do an ldap connect and bind as the guest user or as the > optional userdn. > > *
Attachment
Watch this ticket

Saved Queries