directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Emmanuel Lecharny <elecha...@apache.org>
Subject Server does not allow anymore simple password
Date Sun, 03 Jun 2007 15:26:29 GMT
Hi,

since the last addition of the PasswordPolicyService interceptor in 
trunk, the server does not accept anymore passwords which are supposly 
violating a policy : "Password violates policy:  insufficient character 
mix".

I was very surprised when I first got this message, and I had to dig 
into the code to find the rationals.

What I found in the code didn't pleased me, and I think this code should 
not have quit a branch to be injected to the trunk.

This was directly related to last Enrique merge.

Here are some samples of what I have found :

    void check( String username, String password ) throws NamingException
    {
        int passwordLength = 6;
        int categoryCount = 2;
        int tokenSize = 3;

        if ( !isValid( username, password, passwordLength,
categoryCount, tokenSize ) )
        {
            String explanation = buildErrorMessage( username, password,
passwordLength, categoryCount, tokenSize );
            log.error( explanation );

            throw new NamingException( explanation );
        }
    }

Such constants deep in the code is a sure way to get stuck when using 
the server. I'm pretty sure that it was just intended for testing 
purpose, but even then, it should not have been written this way.

            else if ( attr instanceof byte[] )
            {
                String string = StringTools.utf8ToString( ( byte[] ) attr
);

                StringBuffer sb = new StringBuffer();
                sb.append( "'" + string + "' ( " );
                sb.append( StringTools.dumpBytes( ( byte[] ) attr
).trim() );
                log.debug( "Adding Attribute id : 'userPassword',
Values : [ " + sb.toString() + " ) ]" );

                userPassword = string;
            }

This is another example of what should not be done : there is no reason 
for debug specific code to be found in the normal flow. Moreoever, I 
have told Enrique the day before to not using such a pattern.

Now, beside those pieces of code, there is something much more important 
: it changes the way the server works, and nobody has been warned about 
it. Not only it has been committed as is, but the configuration has been 
changed so that this code is now active.

I have checked the commit logs, and I found another explaination for a 
problem I encoutered lately, but as I didn't had time to dig it, I just 
commented some lines in the server.xml file :

    <!-- limits searches by non-admin users to a max time of
15000          -->
    <!-- milliseconds and has a default value of
10000                      -->
    <property name="maxTimeLimit" value="0" />

    <!-- limits searches to max size of 1000 entries: default value is
100  -->
    <property name="maxSizeLimit" value="100000" />

Those two values was simply not accepted because the related members 
have been removed from the StartupConfiguration class. I can't imagine 
that it has been unnoticed before committing the code, or I guess that 
*no* integration tests has been done, or that the server has never been 
launch, because launching the server with such a configuration leads to 
a direct crash in one second.

This is also a direct result of the merge.

This is simply not the way we should work. I have spent time to 
understand what was happening, and time to write this mail with all the 
necessary checks to be sure I was not misunderstanding what has been 
done. I would have liked to spend this time on something else more usefull.

As a result, I have reverted the server.xml to its original form, and 
reinjected the missing methods in the StartupConfiguration class.

Emmanuel

Mime
View raw message