357668 - merging Sun and Mozilla libldap: bring new srcs unique to Sun branch

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, defect)

Description

[Anton Bobrov]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

this is to add new controls and extended operations and their related 
api and data structs etc to Mozilla branch. all new stuff apart from :

- changing LDAP_VERSION to default to LDAPv3

- exposing some more ber functions thru libldap export 
  [ something i forgot to do during liblber merge ]

- redoing some macros [ compatibility preserved ]


Reproducible: Always

Comment 1

[Anton Bobrov]

Attachment: the first cut of the patch

Comment 2

[Anton Bobrov]

Attachment: new file authzidctrl.c

Comment 3

[Anton Bobrov]

Attachment: new file geteffectiverightsctrl.c

Comment 4

[Anton Bobrov]

Attachment: new file pwpctrl.c

Comment 5

[Anton Bobrov]

Attachment: new file uactrl.c

Comment 6

[Anton Bobrov]

Attachment: new file whoami.c

Comment 7

[Anton Bobrov]

Mark, Rich, please review.

Comment 8

[Anton Bobrov]

Mark, Rich, i know you guys are busy but i need to move on so 
i would appreciate if you could find some time to review this.
 

Comment 9

[Mark Smith [:mcs]]

Comment on attachment 243185 [details] [diff] [review]
the first cut of the patch

Changing LDAP_VERSION to default to LDAPv3 seems like a good idea in theory but
it may confuse people (OpenLDAP still defaults to LDAPv2).

Is there any documentation for the user status control?

The parameter lists for ldap_whoami_s() and ldap_parse_whoami() do
not match the functions defined in OpenLDAP.  I really do not want to introduce new functions that are incompatible with the OpenLDAP equivalent without a good reason.  And OpenLDAP defines differently named functions for password policy.

My understanding is that Sun and Fedora/Red Hat have a different
implementation of get effective rights.  I am not sure it makes sense to introduce APIs for this control.

Comment 10

[Mark Smith [:mcs]]

Comment on attachment 243186 [details]
new file authzidctrl.c

Contributors are typically listed using an email address.  I'd also prefer that the exact format of the Mozilla trilicense template be followed (I think that means adding "Portions created by the Initial Developer are" in front of your copyright notices).

Comment 11

[Mark Smith [:mcs]]

Comment on attachment 243187 [details]
new file geteffectiverightsctrl.c

>/* ldap_create_proxyauth_control
>
>   Create a "version 1" proxied authorization control.
>
>   Parameters are  
>
>   ld              LDAP pointer to the desired connection 
>
>   dn		   The dn used in the proxy auth
>
>   ctl_iscritical  Indicates whether the control is critical of not. If
>                   this field is non-zero, the operation will only be car-
>                   ried out if the control is recognized by the server
>                   and/or client
>
>   ctrlp           the address of a place to put the constructed control
>*/

The above comment block needs to be updated.


>	if ( LBER_ERROR == ber_printf( ber, 
>									"{s", 
>									authzid ) ) 
>        {
>            LDAP_SET_LDERRNO( ld, LDAP_ENCODING_ERROR, NULL, NULL );
>            ber_free( ber, 1 );
>            return( LDAP_ENCODING_ERROR );
>        }	
>
>
>	if ( LBER_ERROR == ber_printf( ber, 
>									"{v}}", 
>									attrlist ) )

Is there any reason you cannot combine these two ber_printf() calls?

Comment 12

[Mark Smith [:mcs]]

Comment on attachment 243188 [details]
new file pwpctrl.c

>    pp->pp_warning = 0;
>    pp->pp_warning_info = -1;
>    pp->pp_error = -1;

pp->pp_warning should be initialized to LDAP_PP_WARNING_NONE and pp->pp_error to LDAP_PP_ERROR_NONE.  And perhaps we should #define LDAP_PP_WARNING_INFO_NONE -1 and use it to initialize pp->pp_warning_info?
>    
>    tag = ber_peek_tag( ber, &len );
>
>    while ( (tag != LBER_ERROR) && (tag != LBER_END_OF_SEQORSET) ) {
>        if ( tag == ( LBER_CONSTRUCTED | LBER_CLASS_CONTEXT ) ) {

My BER is very rusty.  But I am surprised the [0] after warning does not appear in the line above.  Or is it just left out because it is zero (it will have no effect anyway)?

Comment 13

[Mark Smith [:mcs]]

Comment on attachment 243189 [details]
new file uactrl.c

I'd prefer to use a more descriptive name for this file.  Perhaps userstatusctrl.c?


>    /*
>     * The control value should look like this:
>     *
>	 *	ACCOUNT_USABLE_RESPONSE::= CHOICE {
>	 * 		is_available		[0] INTEGER, ** seconds before expiration **
>	 *		is_not_available	[1] More_info
>	 *	}
>	 *	More_info::= SEQUENCE {
>	 *		inactive				[0] BOOLEAN DEFAULT FALSE,    
>	 *		reset					[1] BOOLEAN DEFAULT FALSE,    
>	 *		expired					[2] BOOLEAN DEFAULT FALSE,    
>	 *		remaining_grace			[3] INTEGER OPTIONAL,
>	 *		seconds_before_unlock	[4] INTEGER OPTIONAL
>	 *	}
>	 */

I think initializing all of the fields inside *us would be a good idea.

Comment 14

[Mark Smith [:mcs]]

Comment on attachment 243191 [details]
new file whoami.c


>	
>	rc = ldap_extended_operation( ld, LDAP_CONTROL_EXT_WHOAMI, 
>								  requestdata, serverctrls, clientctrls, msgidp );

It seems strange to use LDAP_CONTROL in the macro name.  OpenLDAP uses LDAP_EXOP_WHO_AM_I.

>/* ldap_parse_whoami_result */
>int
>LDAP_CALL
>ldap_parse_whoami_result ( 	
>							LDAP           *ld, 
>							int            *msgidp,
>							struct timeval *timeout,
>							char           **authzid
>														)

This seems like a strange function to add.  I'd rather leave it out unless you are really dependent on it.

Comment 15

[Mark Smith [:mcs]]

(In reply to comment #10)
> (From update of attachment 243186 [details] [edit])
> Contributors are typically listed using an email address.  I'd also prefer that
> the exact format of the Mozilla trilicense template be followed (I think that
> means adding "Portions created by the Initial Developer are" in front of your
> copyright notices).

In case this is not obvious:  this comment applies to all of the new source files.

Comment 16

[Anton Bobrov]

(In reply to comment #9)

Mark, first off thanx for finding time to review all this stuff, appreciated.

- re ldapv3. i dont see how it can confuse people. what you mean exactly ?
  i dont think its right to advertise ldapv2 any longer. if OpenLDAP still
  defines v2 it doesnt do them any good and to be honest i think they just
  overlooked it or never bothered to change.

- re user status control. no docs right now. it is a new feature that is
  implemented in upcoming version of Sun DS, i believe Ludo has a draft
  in the works but not ready yet. in any case its implemented on the DS
  side and tested as it is already and i would like to keep that because
  it does not clash with anything.

- re whoami parameters. i guess i dont have many options but fixing them
  to be OpenLDAP compatible. berval is rather unfortunate there, dunno
  why they decided to go with it, perhaps so it can be easily parsed to
  their other functions later.

- re effective rights thing. it got implemented before my time with this
  and therefore is a public interface in Sun version and been like that
  for quite some time. i agree it feels ugly but i need to keep that but
  i can settle for moving it to deprecated if we can come up with better
  solution.

(In reply to comment #10)

- re license. right, i think i just ripped them off some other .c file
  in the tree. will change them as you suggest.

(In reply to comment #11)

- re comment and ber for effective rights api. will fix both.

(In reply to comment #12)

- ok about macros, will do as suggested.

- re 0x00 its left out because it has no effect.

(In reply to comment #13)

- rename to userstatusctrl.c ok by me.

- will do explicit initialization there as well.

(In reply to comment #14)

- re LDAP_CONTROL_EXT. i dunno what i been smoking at the time and how
  i came up with these macro names. i honestly do not remember. we also
  have LDAP_CONTROL_EXT_PASSWD_MODIFY that is already in the main tree.
  should i ditch these macro names in favor of OpenLDAP ones or should
  i keep them and add OpenLDAP names as well ?

- re ldap_parse_whoami_result(). its no problem to ditch it, will do.

Comment 17

[Rich Megginson]

I think the default LDAP_VERSION should be LDAPv3.  It's a far bigger problem for people to try to use the API only to find out that it is speaking v2, then have to figure out how to make it speak v3.

Comment 18

[Mark Smith [:mcs]]

(In reply to comment #16)
> - re ldapv3. i dont see how it can confuse people. what you mean exactly ?
>   i dont think its right to advertise ldapv2 any longer. if OpenLDAP still
>   defines v2 it doesnt do them any good and to be honest i think they just
>   overlooked it or never bothered to change.

OK, since you and Rich both agree I am OK with the change.  It would have been better to do before the 6.0 release though.

> - re effective rights thing. it got implemented before my time with this
>   and therefore is a public interface in Sun version and been like that
>   for quite some time. i agree it feels ugly but i need to keep that but
>   i can settle for moving it to deprecated if we can come up with better
>   solution.

OK.  Rich, do you have any plans for a Fedora/Red Hat compatible GER API?

> - re LDAP_CONTROL_EXT. i dunno what i been smoking at the time and how
>   i came up with these macro names. i honestly do not remember. we also
>   have LDAP_CONTROL_EXT_PASSWD_MODIFY that is already in the main tree.
>   should i ditch these macro names in favor of OpenLDAP ones or should
>   i keep them and add OpenLDAP names as well ?

I think we should just switch to the OpenLDAP style names... unless Rich thinks it will cause problems to remove LDAP_CONTROL_EXT_PASSWD_MODIFY (if so, we can keep it as "deprecated").

Comment 19

[Rich Megginson]

> OK, since you and Rich both agree I am OK with the change.  It would have been
> better to do before the 6.0 release though.

Yeah, but better late than never.

> OK.  Rich, do you have any plans for a Fedora/Red Hat compatible GER API?

No.  The interface is pretty trivial, you just add the control OID with no control value, and the results are returned via pseudo attributes.  So I guess we never saw the need for another API function for this:
http://www.redhat.com/docs/manuals/dir-server/release-notes/ger.html

So, you can go ahead and add the API for this.

> I think we should just switch to the OpenLDAP style names... unless Rich
> thinks it will cause problems to remove LDAP_CONTROL_EXT_PASSWD_MODIFY (if
> so, we can keep it as "deprecated").

Yes, let's switch to OpenLDAP style names.  It should not cause any problems, and if it does, it's easy to fix.

Comment 20

[Anton Bobrov]

ok. there is one more thing in original Mark's review i forgot to address
and that is password policy API. OpenLDAP has different ones doing the
same thing. have a look at their libraries/libldap/ppolicy.c there are
three pub functions there:

ldap_create_passwordpolicy_control()
ldap_parse_passwordpolicy_control()
ldap_passwordpolicy_err2txt()

ldap_create_passwordpolicy_control does not allow for control critical
argument and its set to zero inside the function explicitly. i dont
think its a big deal but still.

ldap_passwordpolicy_err2txt is a very good idea for this stuff i think.

generally i like my implementation better [dont blame me for that :)]
but i reckon it would be easier for everybody in the long run if we
go with OpenLDAP version here and i can probably get away with that
at Sun, as well as whoami stuff, because these interfaces are quite
[ relatively ] new and i dont think i will get too much hassle for
changing them. so i plan to rewrite using OpenLDAP names etc but plz
do tell me if you have any problems with that.

i'm a wee bit busy with some stuff right now but i hope to submit
revamped patch this week so please stay tuned.
  

Comment 21

[Rich Megginson]

(In reply to comment #20)
> ldap_create_passwordpolicy_control does not allow for control critical
> argument and its set to zero inside the function explicitly. i dont
> think its a big deal but still.

Seems like a good thing to be able to set the critical bit.  Do other ldap_create_xxx_control functions allow you to set the critical bit?  In our API and OpenLDAP?  If we go with the OpenLDAP API, perhaps we should have a function ldap_create_passwordpolicy_control_ext() that has the missing critical parameter.

> 
> ldap_passwordpolicy_err2txt is a very good idea for this stuff i think.

Yes.

> 
> generally i like my implementation better [dont blame me for that :)]
> but i reckon it would be easier for everybody in the long run if we
> go with OpenLDAP version here and i can probably get away with that
> at Sun, as well as whoami stuff, because these interfaces are quite
> [ relatively ] new and i dont think i will get too much hassle for
> changing them. so i plan to rewrite using OpenLDAP names etc but plz
> do tell me if you have any problems with that.

I don't have a problem with that.
 
> i'm a wee bit busy with some stuff right now but i hope to submit
> revamped patch this week so please stay tuned.

Comment 22

[Anton Bobrov]

Attachment: new file authzidctrl.c

Comment 23

[Anton Bobrov]

Attachment: new file geteffectiverightsctrl.c

Comment 24

[Anton Bobrov]

Attachment: new file pwpctrl.c

Comment 25

[Anton Bobrov]

Attachment: new file userstatusctrl.c

Comment 26

[Anton Bobrov]

Attachment: new file whoami.c

Comment 27

[Anton Bobrov]

Attachment: patch rev 1

Mark, Rich,

got a bit swamped so it took longer than expected. this is new patch plus
revamped new files to address all comments made. there are two new calls
for password policy create_*_ext and parse_*_ext to stay consistent with
our API and lets face it their calls aint exactly user friendly. making 
standard password policy parse call OpenLDAP compatible required bringing
another new call ldap_find_control. thats it. the rest is the same redone
to address all review comments. please review.

Comment 28

[Rich Megginson]

In authzidctrl.c:
		authzidp = ( (char *)NSLDAPI_MALLOC( 
							( AUTHZIDCtrlp->ldctl_value.bv_len + 1 ) ) );
		if ( authzid == NULL ) {
^^^^ I think this should test authzidp instead ^^^^
			LDAP_SET_LDERRNO( ld, LDAP_NO_MEMORY, NULL, NULL );
			return( LDAP_NO_MEMORY );
		}

Otherwise, looks ok.

Comment 29

[Anton Bobrov]

Attachment: new file authzidctrl.c

thanx for review Rich! fixed authzid/p thing now, good catch.

Comment 30

[Rich Megginson]

Looks good to me.

Comment 31

[Anton Bobrov]

should i go ahead and commit ? Mark, are you reasonably happy with these
changes or is there anything else you would like to see addressed or
otherwise changed ?

 

Comment 32

[Mark Smith [:mcs]]

Comment on attachment 244890 [details] [diff] [review]
patch rev 1

Looks good to me.  Please commit.

Comment 33

[Anton Bobrov]

done, thanx again for reviews and comments!

Checking in mozilla/directory/c-sdk/ldap/include/ldap-extension.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/ldap-extension.h,v  <--  ldap-extension.h
new revision: 5.7; previous revision: 5.6
done
Checking in mozilla/directory/c-sdk/ldap/include/ldaprot.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/ldaprot.h,v  <--  ldaprot.h
new revision: 5.5; previous revision: 5.4
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap.ex;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap.ex,v  <--  libldap.ex
new revision: 5.4; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/Makefile.in;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/Makefile.in,v  <--  Makefile.in
new revision: 5.20; previous revision: 5.19
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/authzidctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/authzidctrl.c,v  <--  authzidctrl.c
new revision: 5.1; previous revision: 1.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/cldap.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/cldap.c,v  <--  cldap.c
new revision: 5.4; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/control.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/control.c,v  <--  control.c
new revision: 5.4; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/geteffectiverightsctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/geteffectiverightsctrl.c,v  <--  geteffectiverightsctrl.c
new revision: 5.1; previous revision: 1.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/open.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/open.c,v  <--  open.c
new revision: 5.8; previous revision: 5.7
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/pwmodext.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/pwmodext.c,v  <--  pwmodext.c
new revision: 5.2; previous revision: 5.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/pwpctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/pwpctrl.c,v  <--  pwpctrl.c
new revision: 5.1; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/userstatusctrl.c,v
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/userstatusctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/userstatusctrl.c,v  <--  userstatusctrl.c
initial revision: 5.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/whoami.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/whoami.c,v  <--  whoami.c
new revision: 5.1; previous revision: 1.2
done

Comment 34

[Anton Bobrov]

Attachment: pwpctrl.c: fix previous commit

sorry folks, checked in wrong revision that by mistake :( it still had unused
and undeclared pp leftover in ldap_parse_passwordpolicy_control_ext() and 
wont build as a result, sorry about that.

Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/pwpctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/pwpctrl.c,v  <--  pwpctrl.c
new revision: 5.2; previous revision: 5.1
done