Home » Authentication » Auth » Bug #6959

[2006-02-28 11:15 UTC] hugues dot peeters at claroline dot net

Description:
------------
Dear Pear contributors,

Christophe Gesché (aka Moosh) has already forwarded one of my message concerning Auth::LDAP (see http://news.php.net/php.pear.qa/3510).

Last week, I worked on the Auth LDAP container to make it work again in our own Application. I forward in attachment the adapted container code for 1.3 Auth branches.

Here are the main changes :
- Fix a detected bug in _quoteFilterString() (Note : I just noted that it has been fixed by someone else since then ...)

- Rename checkGroup() method in _checkGroup() to follow the convention of other private class methods.

- Set a default value for $this->options['url'] rather than test later in the code if it is set.

- Limit the _deconnect() method calls by refactoring a bit the code.

- Refactor fetchData() method extracting the code for the additional attributes retrieval and put it in a new method called '_fetchAdditionalAttributes()'.

- Add a new method called _setV12OptionsToV13() who manage the compatibility problems that could arise when user will pass from Auth LDAP 1.2 to 1.3.

Sources are availbale at this url : http://www.claroline.net/dlarea/misc/Pear.Auth.LDAP.phps

I've read that Adam Ashley aim to release Auth 1.3 stable bery soon. I hope that the following remarks concerning the Auth LDAP container don't come too late, but I think it is certainly preferable to communicate them to you rather than keep them to me, isn't it ?

1. The main remark concerns the 'attrformat' option recently added by Moosh. This option was added because when Jan Wagner implement the possibility to fetch additional user attributes in the 1.3 branch, he choose to store them in pure LDAP format. This format is NOT compatible with data format used by other Auth containers (actually the 'DB' one ...) and it will probably pose problems when user will try to retrieve these data from the Auth getAuthData() interface. Later, after discussions with Arnaud Limbourg, Moosh has added the 'attrformat' option allowing user to set these data format to 'AUTH' or 'LDAP'. At that time it was decided to set the default value to 'LDAP' to not break the compatibility. But Moosh didn't really figure out that the 'additional attributes' feature was only implemented in Auth 1.3, not flagged as stable yet. Thus, couldn't we reconsider the default value of 'attrformat' option and set it to 'AUTH' before releasing the stable version ? It would probably be far easier for Auth library user, wouldn't it ?

2. The 'attributes' option of the LDAP container expects an array. On the other side, the equivalent option for the DB container ('db_fields') expects a comma separated list in string format. Wouldn't it be better if both options, in LDAP and DB share, a similar format ? That just mean a simple explode() statement to add in the current code.

3. This remark is really minor. I wonder if 'attributes' is really the most appropriate name for this option as we know that there is also an other option called 'userattr' in this container . Considering the existence of this other 'attr' option, maybe another name for 'attributes', like 'otherattr', would clarify the option purpose in a greater degree, wouldn't it ? For your information, that's what has already been done for the 'scope', option which as been converted in 'userscope' and 'groupscope' in the Auth LDAP 1.3 version.

I hope that this message doesn't upset too much the release planning.

Best regards,

Hugues Peeters

PS. I would like to test the attached code again juste before sending it to you. Sadly the computer hosting our LDAP server has just crashed. As time is running, I indulge myself to send it nevertheless (it worked yesterday, it should today ... ). But, please, could you test it again before commit it into Pear CVS.

Test script:
---------------
http://www.claroline.net/dlarea/misc/Pear.Auth.LDAP.phps

[2006-03-01 05:51 UTC] aashley at php dot net

- Rename checkGroup() method in _checkGroup() to follow the convention of other private class methods.

Not doing as function may be useful to others.

- Set a default value for $this->options['url'] rather than test later in the code if it is set.

Not doing as this is specifically done to handle the case where PHP is not compiled against openldap (which is the only one to support the url style connect options). URL takes precedence over host:port so if URL has a default host:port will never be used and in some cases it should be.

I'm not going to do the code refactoring at this late stage in the release. perhaps later.

I've merged the 1.2 compatibility options.

attrformat default has been changed to AUTH to make it more consistent with the rest of auth.

attributes options now accepts a comma limited string as well as an array the same as db_fields on the database backends.

[2006-03-01 13:48 UTC] jw at php dot net

- Rename checkGroup() method in _checkGroup() to follow the convention of other private class methods.

This would be consistent as none has access to the Container anyway, except the Auth class itself which doesn't need the function. Must have been a typo back then ;-)

- Set a default value for $this->options['url'] rather than test later in the code if it is set.

Nicely explained aashley. It is even documented in the code.

- Limit the _deconnect() method calls by refactoring a bit the code.

What's the problem with it? It gets called only once in any case I can see.

1. The main remark concerns the 'attrformat' option recently added by Moosh. This option was added because when Jan Wagner implement the ...

Hehe, if I remember right, this was just a quick hack and none ever got back with me that LDAP format is a problem.

2. The 'attributes' option of the LDAP container expects an array. On the other side, the equivalent option for the DB container ('db_fields') expects a comma separated list in string format. Wouldn't it be better if both options, in LDAP and DB share, a similar format? That just mean a simple explode() statement to add in the current code.

Hmmh, LDAP is certainly not SQL in any case, and there IS a difference between attributes and columns I guess, but as it doesn't hurt ...

3. This remark is really minor. I wonder if 'attributes' is really the most appropriate name for this option as we know that there is also an ...

As you have to read the docs anyway to use the Container and its name is neither way intuitive it should not matter. I see a problem with documentation though. I started documenting the class in source back in 2003. Now that more and more features get added it would be time to move it into a separate README file.

[2006-03-02 11:17 UTC] hugues dot peeters at claroline dot net

>> Set a default value for $this->options['url'] rather than test later in the code if it is set.

> Not doing as this is specifically done to handle the case where PHP is not compiled against openldap (which is the only one to support the url style connect options). URL takes precedence over host:port so if URL has a default host:port will never be used and in some cases it should be.

It depends how the _connect() function is written.

function _connect()
{
if (isset($this->options['url']) &&
$this->options['url'] != '') {
...

In the Auth::LDAP version I've provided, the line below was changed to:

if ( $this->options['url'] != '') {

In that case, precedence problem shouldn't occur, shouldn't it?

From my point of view, it is mostly a matter of consistency. 'url' is the only option not set to a default value in the LDAP container. For example, 'group' and 'groupdn' options are set to a default empty string, even if they are not needed.

function fetchData($username, $password)
{
...
if( $this->options['group'] != '' ) {

>> Limit the _deconnect() method calls by refactoring a bit the code.

> What's the problem with it? It gets called only once in any case I can see.

Is it really the job of the checkGroup() method to close the LDAP connection ? From my point of view, the LDAP connection closing should ideally be managed in the same location where it has been open.

function fetchData($username, $password)
{
$this->_connect();
...
$this->_disconnect();
}

I'm not familiar with Pear community habits yet. I don't know if these topics are usually discussed in the bug tracker, and I hope that this few remarks won't be take as an offence. This is only a 0.2 cent contribution of a grateful PEAR user who has to dig in the code for the first time to fix a problem and has take the occasion to refactor it a bit. But if the Auth::LDAP works, it's ok for me. Thanks for all of your work.

Regards

Hugues
hugues.peeters@claroline.net

[2006-03-02 23:21 UTC] aashley at php dot net

oh you mean setting url to a default value of ''. Sorry I misunderstood your comment. That is already done and yes as such probably could simplify the check in _connect().

to the disconnect() yes I agree with you, but I didnt want to merge your refactoring of the LDAP process this late in the release procedure. It's next on the list for after the stable version is out there.

[2006-08-10 03:45 UTC] aashley at php dot net

I think all the bits of this have made it into CVS. if there is stuff you think isnt please open new bugs for it.