Re: [Mantisbt-dev] LDAP Authentication

[Julian F. <ju...@be...>]

Ok, Robert, comments for you.  I was going to just apply it but I 
figured there were enough little things that I might as well throw it 
back at you one more time. (again as for details, I can't review whether 
it works since I don't know LDAP, excuse me if I sound dumb).

Robert Foster wrote:

> Hi All,
> Here's a patch for the enhanced LDAP Authentication code & relevant file
> changes.  I've taken the liberty to update the login.php file as well.
> I want to look at changing the Password in an LDAP Directory next.  

Do most LDAP services allow you to change your password or do you 
usually have a central way of changing it?  I have no idea...

>
> (Will have to include a note on the page warning the user that other
> services will be affected as well.  Alternatively, create a custom
> schema for Mantis, and have a userpassword-like attribute to
> authenticate off.
>
> <<...>>
>
> *Robert Foster*
> *Mountain Visions P/L*
> Australia.
> rf...@mo...
> _http://mountainvisions.com.au/_
>
> Supporter & Contributor - Computing in Silence
>
>
> ------------------------------------------------------------------------
>
> diff -urN mantisbt/config_inc.php mantisbt_dev/config_inc.php
> --- mantisbt/config_inc.php	Thu Nov 21 00:17:12 2002
> +++ mantisbt_dev/config_inc.php	Wed Nov 27 08:37:33 2002
> @@ -138,11 +138,14 @@
>  	# look in README.LDAP for details
>
>  	# --- using openldap -------------
> -	#$g_ldap_server       = "ldap2.mountainvisions.com.au";
> -	#$g_ldap_root_dn      = "dc=mountainvisions,dc=com,dc=au";
> -	#$g_ldap_organisation = "(organizationname=*Mountain Visions)"; # 
> optional
> -	#$g_ldap_auth_type = "CRYPT"; # optional
> -	#$g_use_ldap_email    = ON; # Should we send to the LDAP email 
> address or what MySql tells us
> +	$g_ldap_server         = "ldaps://ldap.mountainvisions.com.au/";
> +	$g_ldap_port           = "636";
> +	$g_ldap_root_dn        = "dc=example, dc=com";
> +	$g_ldap_organisation   = ""; # optional
> +	#$g_ldap_auth_type      = "CRYPT"; # NOT USED
> +	$g_use_ldap_email      = ON; # Should we send to the LDAP email 
> address or what MySql tells us
> +	$g_ldap_bind_dn        = "cn=Manager, dc=example, dc=com";
> +	$g_ldap_bind_passwd    = "secret";
>
>  	############################
>  	### Mantis Misc Settings ###
> @@ -150,7 +153,7 @@
>
>  	# --- login method ----------------
>  	# CRYPT or PLAIN or MD5 or LDAP or BASIC_AUTH
> -	$g_login_method = CRYPT;
> +	$g_login_method = LDAP;
>
>  	# --- limit reporters -------------
>  	# Set to 1 if you wish to limit reporters to only viewing bugs that 
> they report.

Heh, if you can make your patch not include local files it makes 
reviewing that much quicker, and that much more likely that I'll get to 
it sooner.  Not a big deal but, if you can make avoid it, it helps.

>
> diff -urN mantisbt/constant_inc.php mantisbt_dev/constant_inc.php
> --- mantisbt/constant_inc.php	Tue Nov 26 09:35:26 2002
> +++ mantisbt_dev/constant_inc.php	Wed Nov 27 08:33:16 2002
> @@ -205,6 +205,12 @@
>  	define( 'ERROR_CUSTOM_FIELD_CAPTION_NOT_UNIQUE',1301 );
>  	define( 'ERROR_CUSTOM_FIELD_IN_USE',			1302 );
>
> +	# ERROR_LDAP_*
> +	define( 'ERROR_LDAP_AUTH_FAILED',				1400 );
> +	define( 'ERROR_LDAP_SERVER_CONNECT_FAILED',		1401 );
> +	define( 'ERROR_LDAP_UPDATE_FAILED',				1402 );
> +	define( 'ERROR_LDAP_USER_NOT_FOUND',			1402 );
> +	

Great, perfect! ;)

>
>  	# Status Legend Position
>  	define( 'STATUS_LEGEND_POSITION_TOP',		1);
>  	define( 'STATUS_LEGEND_POSITION_BOTTOM',	2);
> diff -urN mantisbt/core/authentication_api.php 
> mantisbt_dev/core/authentication_api.php
> --- mantisbt/core/authentication_api.php	Sun Sep 22 15:24:52 2002
> +++ mantisbt_dev/core/authentication_api.php	Tue Nov 26 22:49:23 2002
> @@ -157,7 +157,8 @@
>  		$t_login_method = config_get( 'login_method' );
>
>  		if ( LDAP == $t_login_method ) {
> -			return ldap_uid_pass( $p_username, $p_test_password );
> + 			$t_username = user_get_field( $p_user_id, 'username' );
> +			return ldap_authenticate( $t_username, $p_test_password );
>  		}
>
>  		$t_password = user_get_field( $p_user_id, 'password' );

thanks.  another case of me cleaning up this code without being able to 
test it.

>
> diff -urN mantisbt/core/ldap_api.php mantisbt_dev/core/ldap_api.php
> --- mantisbt/core/ldap_api.php	Tue Aug 27 20:08:08 2002
> +++ mantisbt_dev/core/ldap_api.php	Wed Nov 27 08:55:19 2002
> @@ -15,88 +15,140 @@
>
>  	# Some simple LDAP stuff that makes the work go 'round
>  	# Leigh Morresi
> +	# Enhancements for authenticating users through a 'closed' Directory by
> +	# Robert Foster  with help & code by
> +	# Christian Mayer
>  

I don't know if it's an officially stated policy for this project, but I 
think keeping names out of source code is a good idea (I've been 
removing them as I come across them).  I will always include the name of 
the person who submitted a patch in the CVS commit message so it can be 
easily found by looking at the log for the file.  We should also make 
sure everyone gets added to the CREDITS file.

If you submit a log message with your patch (you can see the format I 
use when possible in mine - stolen from the subversion project) it helps 
review the changes.  We're not as strict as subversion but I do think 
that, on changes that aren't simple and global (like "rename foo() to 
bar() in every file), it's useful to list what changes you made in each 
file and then afterwards a few sentences on why.  Anyway, if you submit 
a log message with your patch, you could remove these names from the 
file and list them all in the log message.

>
>   	# --------------------
> -	# Find someone email address based on their login name
> +	# Connect and Bind to the LDAP Directory.
> +	function ldap_connect_bind( $binddn = "", $password = "" )	{
> +		$t_ldap_server = config_get( 'ldap_server' );
> +		$t_ldap_port = config_get( 'ldap_port' );
> +
> +		$t_ldap_bind_passwd    = config_get( 'ldap_bind_passwd', '' );
> +		$t_ldap_bind_dn        = config_get( 'ldap_bind_dn', '' );
> +
> +		$status = -1; // This should probably be registered as a global 
> variable?

A global?  We don't like globals... why does it need to be global?

>
> +
> +		$ds = @ldap_connect ( $t_ldap_server, $t_ldap_port );

Hmm... no one's ever used that @-syntax in here before... my error 
handler doesn't handle it right now (as I recall).. perhaps I better go 
look at that.

>
> +		if ( $ds > 0 ) {
> +			# If no Bind DN and Password is set, attempt to login as the 
> configured
> +			# Bind DN.
> +			if ( strlen( $binddn ) == 0 && strlen( $password ) == 0 ) {

I'd use the new is_blank() function for these checks I think.

>
> +				$binddn = $t_ldap_bind_dn;
> +				$password = $t_ldap_bind_passwd;

You could do the config_get() calls here directly since you never need 
the values anywhere else.  Cuts a few unnecessary lines from the function.

>
> +			}
> +			if ( ( strlen( $binddn ) > 0 ) && ( strlen( $password ) > 0 ) ) {
> +				$br = @ldap_bind( $ds, $binddn, $password );
> +			} else {
> +				# Either the Bind DN or the Password are empty, attempt an 
> anonymous bind.
> +				$br = @ldap_bind( $ds );
> +			}
> +			if ( ! $br ) {
> +				$status = ERROR_LDAP_AUTH_FAILED;
> +			}		
> +		} else {
> +			$status = ERROR_LDAP_SERVER_CONNECT_FAILED;
> +		}

Do you need to call ldap_unbind() if $ds <= 0 (ie if we didn't connect 
successfully?)  It seems like you could do "if ( $ds <= 0 ) { 
trigger_error( ERROR_LDAP_SERVER_CONNECT_FAILED, WARNING ); return 0; }" 
above this code and then pull everything else out of the if block.  then 
you can put the code below into the "if ( ! $br ) {...}" above.  This 
means you don't need $status at all and you end up with shallower if-blocks.

Also, is there some reason you're using WARNING and returning 0?  Isn't 
it a fatal error if we can't connect to the LDAP server?

> +		
> +		if ( $status > -1 ) {
> +	 		ldap_unbind( $ds );
> +			$ds = 0;
> +			error_parameters( ldap_connect_bind );

Nice!  hardly anything uses that yet (and it should eventually plug them 
into the error message ala sprintf)... how'd you find it? :)

>
> +			trigger_error( $status, WARNING );
> +		}
> +
> +		return $ds;
> +	}
> +
> + 	# --------------------
> +	# Find someones email address based on their login name
>  	function ldap_email($worker) {
> -	    global $g_ldap_organisation,$g_ldap_server,$g_ldap_root_dn;
> +		$t_ldap_organisation = config_get( 'ldap_organisation' );
> +		$t_ldap_root_dn = config_get( 'ldap_root_dn' );
>
> -	    $search_dn = "(&$g_ldap_organisation(uid=$worker))";
> -	    $ds        = ldap_connect( "$g_ldap_server" );
> +	    $search_filter = "(&$t_ldap_organisation(uid=$worker))";
> +		$search_attrs  = array( "uid", "email", "dn" );
> +	    $ds            = ldap_connect_bind();
>
> -		if ( $ds ) {
> -			$r    = ldap_bind( $ds );
> -			$sr   = ldap_search( $ds, $g_ldap_root_dn, $search_dn );
> +		if ( $ds > 0 ) {
> +			$sr   = ldap_search( $ds, $t_ldap_root_dn, $search_filter, 
> $search_attrs );
>  			$info = ldap_get_entries( $ds, $sr );
> -			ldap_close( $ds );

ldap_close() isn't necessary?

>
> -			return ($info[0]["mail"][0]);
> -		} else {
> -			echo "
>
>
>         Unable to connect to LDAP server
>
> ";
> -			die;
> +			ldap_free_result( $sr );
> +			ldap_unbind( $ds );
> +			return ( $info[0]["mail"][0] );
>  		}
>  	}
>
>  	# --------------------
>  	# Return true if the $uid has an assigngroup=$group tag
> -	function ldap_has_group($uid,$group) {
> -	    global $g_ldap_organisation,$g_ldap_server,$g_ldap_root_dn;
> -
> -		$search_dn = "(&$g_ldap_organisation(uid=$uid)(assignedgroup=$group))";
> -		$ds        = ldap_connect( "$g_ldap_server" );
> +	function ldap_has_group($uid, $group) {
> +		$t_ldap_organisation = config_get( 'ldap_organisation' );
> +		$t_ldap_root_dn = config_get( 'ldap_root_dn' );
> +
> +		$search_filter = 
> "(&$t_ldap_organisation(uid=$uid)(assignedgroup=$group))";
> +		$search_attrs  = array( "uid", "dn", "assignedgroup" );
> +	    $ds            = ldap_connect_bind();
>
> -		if ( $ds ) {
> -			$r       = ldap_bind( $ds ); # bind to server
> -			$sr      = ldap_search( $ds, $g_ldap_root_dn, $search_dn ); # query
> +		if ( $ds > 0 ) {
> +			$sr      = ldap_search( $ds, $t_ldap_root_dn, $search_filter ); # 
> query

No idea what $search_attrs does but you define it above, should it be 
included in this call like in the previous function?

>
>  			$entries = ldap_count_entries( $ds, $sr );
> -	        ldap_close( $ds ); # clean up
> +			ldap_free_result( $sr );
> +	        ldap_unbind( $ds ); # clean up
>  	        return $entries;
> -		} else {
> -			echo "
>
>
>         Unable to connect to LDAP server
>
> ";
> -			die;
>  		}
>  	}
> +	
>  	# --------------------
> -	# Return true if the $uid has $password (salt soon!)
> -	function ldap_uid_pass($login, $pass) {
> -	global 
> $g_ldap_organisation,$g_ldap_server,$g_ldap_root_dn,$g_ldapauth_type;
> -
> -	$search_dn = "(&$g_ldap_organisation(uid=$login))";
> -	$ds        = ldap_connect( "$g_ldap_server" );
> -
> -	if ( $ds ) {
> -		$r       = ldap_bind( $ds ); # bind to server
> -
> -		if ("CLEAR" == $g_ldapauth_type)
> -		{
> -			$crypted_pass = $pass;
> -		}
> -		elseif ("CRYPT" == $g_ldapauth_type)
> -		{
> -			$sr	= ldap_search( $ds, $g_ldap_root_dn, $search_dn ); # query 
> without password
> -			$entry	= ldap_first_entry($ds, $sr);
> -			if (!($entry)) return false;
> -			$values	= ldap_get_values($ds, $entry,"userpassword");
> -			$salt	= $values[0][0].$values[0][1];
> -			$crypted_pass=crypt($pass,$salt);
> -		}
> -		else
> -		{
> -			die ("wrong LDAP parameter g_ldapauth_type : [$g_ldapauth_type]");
> -		}
> -
> -		$search_dn = 
> "(&$g_ldap_organisation(uid=$login)(userpassword=$crypted_pass))";
> -		$sr      = ldap_search( $ds, $g_ldap_root_dn, $search_dn ); # query 
> with password matching
> -		#---------------------------
> -		$entries = ldap_count_entries( $ds, $sr );
> -		ldap_close( $ds ); # clean up
> -		if ( $entries >= 1 ) {
> -			return true;
> -		} else {
> -			return false;
> +	# Return true if we can authenticate the Login UID with the supplied 
> Password to the Directory.
> +	function ldap_authenticate($login, $password) {
> +		$t_ldap_organisation = config_get( 'ldap_organisation' );
> +		$t_ldap_root_dn = config_get( 'ldap_root_dn' );
> +
> +		$search_filter = "(&$t_ldap_organisation(uid=$login))";
> +		$search_attrs  = array( "uid", "dn" );
> +	    $ds            = ldap_connect_bind();
> +		$status        = -1;
> +		$acct_found    = false;
> +		
> +		if ( $ds > 0 ) {
> +			# Search for the user id.
> +			$sr = ldap_search($ds, $t_ldap_root_dn, $search_filter, 
> $search_attrs);
> +			if ( $info = ldap_get_entries( $ds, $sr ) ) {
> +				$acct_found = false;
> +				# Try to authenticate to each until we get a match!
> +				for ( $i = 0; $i < $info["count"]; $i++ ) {
> +					$dn = $info[$i]["dn"];
> +					$acct_found = @ldap_bind( $ds, $dn, $password );
> +					if ($acct_found) {
> +						$i = $info["count"]; // Don't need to go any further.

can't you just use "break" here to get out of the loop?

>
> +					}
> +				}
> +			}
> +			ldap_free_result( $sr );
> +	 		ldap_unbind( $ds );
> +		}
> +		if (!$acct_found) {
> +			$status = ERROR_LDAP_USER_NOT_FOUND;

can't we just trigger an error inside this if?  why have a second one 
below?  $status doesn't seem to be modified anywhere else...

>
> +		}
> +		if ( $status > -1 ) {
> +			error_parameters( ldap_authenticate );
> +			trigger_error( $status, WARNING );
>  		}

Do you return true anywhere?  seems like it should be here

>
> -	} else {
> -		die ("Unable to connect to LDAP server");
> +
> +		return ( $acct_found );

aren't we always returning false here?  This else only applies if we 
don't even connect right?

>
>  	}
> -}
> +	
> +	# --------------------
> +	# Create a New User Account in the LDAP Directory.
> +#	ldap_create_account() {
> +#	}
> +	
> +	# --------------------
> +	# Update the User Account in the LDAP Directory
> +	
>  	# --------------------
> +	# Change the User Password in the LDAP Directory
> +	
> +	
>  ?>
> diff -urN mantisbt/doc/README.LDAP mantisbt_dev/doc/README.LDAP
> --- mantisbt/doc/README.LDAP	Wed Feb 27 20:53:54 2002
> +++ mantisbt_dev/doc/README.LDAP	Tue Nov 26 21:42:05 2002
> @@ -1,79 +1,81 @@
> --------------------------------------------------------------------------------
>
> +-------------------------------------------------------------------------------
>     Mantis - LDAP capabilities documentation
>     Original by le...@li...
> -   Updated by Adrian Spinei as...@ya... - currently the 
> maintaner of
>
> -   the LDAP capabilities of mantis, please direct all questions to me
>
> --------------------------------------------------------------------------------
>
> -
>
> -Here is my attempt at providing Mantis with LDAP capabilities.
>
> -
>
> -=== Outline
>
> -
>
> -Functionality is provided by using the php-ldap module 
> (/usr/lib/php4/ldap.so)
>
> -An extra login method is defined within core_user_API.php inside of
>
> -function is_password_match ( $f_username, $p_test_password, $p_password )
>
> -
>
> -This has a simple, non encrypted (yet) test of the LDAP directory for 
> that user
>
> -by asking for an entry with uid=username and password=test_password, 
> if this
>
> -exists, it is presumed that the user should be granted access.
>
> -
>
> -== Configuration basics
>
> -
>
> -the LDIF format I use and have tested this with is as follows :
>
> -
>
> -dn: uid=tests, dc=test, dc=com, dc=au
>
> -department: testdep
>
> -organizationname: Testing Organization
>
> -cn: Test Smith
>
> -assignedgroup: users
>
> -givename: Test
>
> -sn: Smith
>
> -mail: te...@te...
>
> -uid: tests
>
> -userPassword: password
>
> -objectclass: testPerson
>
> -
> -Note : the password may be in clear or taken from the /etc/passwd or 
> /etc/shadow file
> -
> if you prefer to keep the password in clear (very insecure !) yop 
> should use the
> -configuration option $g_ldapauth_type = 'CLEAR';
>
> -otherwise $g_ldapauth_type = 'CRYPT'; (and the password should match 
> /etc/passwd)
>
> -There are some specialized software for replicating passwd to LDAP 
> and inversely
>
> -(eg. http://freshmeat.net/projects/cpu/)
>
> -
> -It is also required to set the following configuration items in 
> default/config_inc1.php
>
> -
>
> -
>
> -	#############################
>
> -	### Mantis LDAP Settings  ###
>
> -	#############################
>
> -
>
> -	# --- using openldap -------------
>
> -	$g_ldap_server="127.0.0.1";
>
> -	$g_ldap_root_dn="dc=test,dc=com,dc=au";
>
> -	$g_use_ldap_email=1;    ## Should we send to the LDAP email address 
> or what MySql tells us
>
> -	# $g_ldap_organisation="(organizationname=*Traffic)";     ## optional
>
> -	$g_ldapauth_type = 'CLEAR';
>
> -
>
> -
>
> -Dont forget to change your $g_login_method to $g_login_method = LDAP;
>
> -
>
> -=== Creating new accounts
>
> -
>
> -I guess there is still a bit of problem when you want to create a 
> new user
>
> -to Mantis using LDAP, you must create the LDIF entry to LDAP, and also
>
> -sign up for a new account, if both of these line up correctly, 
> authentication
>
> -will proceed.
>
> -
>
> -=== email issues
>
> -
>
> -Email address is queried from the LDAP database if the authentication 
> is set
>
> -to use LDAP instead of the mySql entry.
>
> -
>
> -
>
> -Hope it works as good for you as it does for me.
>
> -
>
> -		Leigh Morresi
>
> -
>
> --------------------------------------------------------------------------------
>
> -   Mantis - LDAP capabilities documentation 
> le...@li...
>
> +   Updated by Adrian Spinei as...@ya... - currently the 
> maintaner of
> +   the LDAP capabilities of mantis, please direct all questions to me
> +   20 Nov 2002: Updated by Robert Foster
> +                to allow for 'closed' LDAP Directories and/or 
> Anonymous Logins
> +-------------------------------------------------------------------------------
> +
> +Here is my attempt at providing Mantis with LDAP capabilities.
> +
> +=== Outline
> +
> +Functionality is provided by using the php-ldap module 
> (/usr/lib/php4/ldap.so)
> +An extra login method is defined within core_user_API.php inside of
> +function is_password_match ( $f_username, $p_test_password, $p_password )
> +
> +This has a simple, non encrypted (yet) test of the LDAP directory for 
> that user
> +by asking for an entry with uid=username and password=test_password, 
> if this
> +exists, it is presumed that the user should be granted access.
> +
> +== Configuration basics
> +
> +the LDIF format I use and have tested this with is as follows :
> +
> +dn: uid=tests, dc=test, dc=com, dc=au
> +department: testdep
> +organizationname: Testing Organization
> +cn: Test Smith
> +assignedgroup: users
> +givename: Test
> +sn: Smith
> +mail: te...@te...
> +uid: tests
> +userPassword: password
> +objectclass: testPerson
> +
> +Note : the password may be in clear, taken from the /etc/passwd or 
> /etc/shadow file,
> +or simply encrypted and added using current LDAP tools.
> +
> +There are some specialized software for replicating passwd to LDAP 
> and inversely
> +(eg. http://freshmeat.net/projects/cpu/)
> +
> +It is also required to set the following configuration items in 
> config_inc.php
> +
> +
> +	#############################
> +	### Mantis LDAP Settings  ###
> +	#############################
> +
> +	# --- using openldap -------------
> +	$g_ldap_server         = "ldaps://ldap.mountainvisions.com.au/";
> +	$g_ldap_port           = "636";
> +	$g_ldap_root_dn        = "dc=example, dc=com";
> +	#$g_ldap_organisation  = "(organizationname=*Example)"; # optional
> +	#$g_ldap_auth_type 	   = "CRYPT"; # NO LONGER USED
> +	$g_use_ldap_email      = ON; # Should we send to the LDAP email 
> address or what MySql tells us
> +	$t_ldap_bind_dn        = "cn=Manager, dc=example, dc=com";
> +	$t_ldap_bind_passwd    = "secret";
> +
> +Dont forget to change your $g_login_method to $g_login_method = LDAP;
> +
> +=== Creating new accounts
> +
> +I guess there is still a bit of problem when you want to create a 
> new user
> +to Mantis using LDAP, you must create the LDIF entry to LDAP, and also
> +sign up for a new account, if both of these line up correctly, 
> authentication
> +will proceed.
> +
> +=== email issues
> +
> +Email address is queried from the LDAP database if the authentication 
> is set
> +to use LDAP instead of the mySql entry.
> +
> +Hope it works as good for you as it does for me.
> +
> +		Leigh Morresi
> +
> +-------------------------------------------------------------------------------
> +   Mantis - LDAP capabilities documentation 
> le...@li...
>  -------------------------------------------------------------------------------

I assume the problem here is that you are correcting the line endings? 
Is there any chance you could submit one patch that just changes the 
file and one patch that fixes the line endings?  Those of us reading 
your patch have no way of knowing what has changed.

>
> diff -urN mantisbt/lang/strings_english.txt 
> mantisbt_dev/lang/strings_english.txt
> --- mantisbt/lang/strings_english.txt	Tue Nov 26 09:35:39 2002
> +++ mantisbt_dev/lang/strings_english.txt	Wed Nov 27 08:33:06 2002
> @@ -188,6 +188,11 @@
>  	$MANTIS_ERROR[ERROR_CUSTOM_FIELD_CAPTION_NOT_UNIQUE]= 'ERROR: This 
> is a duplicate caption.';
>  	$MANTIS_ERROR[ERROR_CUSTOM_FIELD_IN_USE]= 'ERROR: At least one 
> project still uses this field!';
>
> +	$MANTIS_ERROR[ERROR_LDAP_AUTH_FAILED]= 'ERROR: LDAP Authentication 
> Failed';
> +	$MANTIS_ERROR[ERROR_LDAP_SERVER_CONNECT_FAILED]= 'ERROR: LDAP Server 
> Connection Failed';
> +	$MANTIS_ERROR[ERROR_LDAP_UPDATE_FAILED]= 'ERROR: LDAP Record Update 
> has failed.';
> +	$MANTIS_ERROR[ERROR_LDAP_USER_NOT_FOUND]= 'ERROR: LDAP User Record 
> Not Found.';
> +
>  	# General Strings
>  	$s_go_back = 'Go Back';
> 	$s_proceed = 'Click here to proceed';
> diff -urN mantisbt/login.php mantisbt_dev/login.php
> --- mantisbt/login.php	Wed Sep 18 15:32:50 2002
> +++ mantisbt_dev/login.php	Wed Nov 27 09:05:23 2002
> @@ -4,12 +4,11 @@
>  	# Copyright (C) 2002         Mantis Team   - 
> man...@li...
>  	# This program is distributed under the terms and conditions of the GPL
>  	# See the README and LICENSE files for details
> -?>
> -<?php
> -	# Check login then redirect to main_page.php3 or to login_page.php3
> -?>
> -<?php require_once( 'core.php' ) ?>
> -<?php
> +
> +	# Check login then redirect to main_page.php or to login_page.php
> +
> +	require_once( 'core.php' );
> +
>  	$f_username		= gpc_get_string( 'f_username', '' );
>  	$f_password		= gpc_get_string( 'f_password', '' );
>  	$f_perm_login	= gpc_get_bool( 'f_perm_login' );

Actually, this is a pretty standard pattern that all the files use.  Not 
sure why it was done originally but I've left it because it does allow 
the description and load core line stand out from the rest of the code.

I'm actually propsing a slightly new file header format which I started 
implementing in a few files as a test (if you look at the first few 
files alphabetically you'll see what I mean) but I started thinking it 
might just end up getting out of date.  If we decide to change this, we 
should change all the files at once at some point.

Hope this was helpful and that I didn't totally misunderstand how things 
work.  Once you have time to look at my comments and make whatever fixes 
you want, I'm happy to apply this... like I said I have no way of 
testing if it works so if others who use LDAP can give it a whirl that 
would be fabulous.

Julian

-- 
ju...@be...
Beta4 Productions (http://www.beta4.com)