Enrique,

Also note that you/we could have avoided this issue if we made sure that each feature branch
only has changes relating to one feature.  I think one of the reasons why I missed this change
is because the branch was riddled with many different features.  I was focusing on the SASL
feature specifically and missed this password policy change.

Overall I think we all need to adhere to a policy where if we need a review to take place on a
feature branch that all must collaborate on, then we must be strict with ourselves by keeping
changes in that branch specific to the purpose of that branch. 

So if we have a SASL branch we only do SASL changes.  If ancilliary changes are needed to accomplish this task then we can make those changes in another branch or in the trunk keeping the view of changes in the SASL branch specific.  Thinking about the config and password policy changes specifically.

Alex

On 6/3/07, Alex Karasulu <akarasulu@apache.org> wrote:
Comments in-line ...

On 6/3/07, Emmanuel Lecharny < elecharny@apache.org> wrote:
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".

This should not be the default behavior and furthermore any change that effects default
behavior out of the box must be discussed beforehand on this list.  Just think about the
issues we had with the small fix to the BasicAttributes(true) change a little while ago.

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.

Yes this is a very serious issue.  As I said above we cannot change the default behavior of
the server without discussing the impact of such changes on this mailing list.

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.

Yes this is probably due to inadequate testing or a presumption that the configuration
works in one environment without testing other environments.

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.

I have kept quiet while dealing with merge issues in the trunk.  I know the changes
introduced were complex and involved.  Nothing ever goes smoothly on big change
merges however they could have been smoother than it was with this particular merge.

There were several gross presumptions that were made which cost a number of us
several hours while trying to decipher what is happening while having tests fail with
these JVM issues with the crypto introductions.   These problems could have easily
been avoided by following some simple protocol.

I said nothing because I did not want to upset you Enrique but you have to make
some effort to consider the fallout of these kinds of changes.  You need to lookout
for your fellow committers by:

(1) thoroughly testing your changes
(2) not presuming everyone has the same environment and testing different stock JVMs
(3) reporting and discussing each behavior altering change on this ML before merging it

The trunk is a serious place where we have exposure.  This is why we work on feature
branches where we need to shield others from inconsistent states during the course of
introducing a feature.  Once you decide to merge though you need to make sure all your
i's are dotted and your t's are crossed.

I know I missed these changes while reviewing your branch however I did not think
you would take it upon yourself to enable these features by default in the server.  Many people
depend on certain behaviors and before we change them we need to discuss the changes
and give users a heads up. 

Please consider others and have some doubt your ability to make final decision to
introduce such default functionality.  This doubt should not make you stop but should
make you ask some questions or open up some conversations.  To give you credit
I already see you doing this and I commend you on it (internal vs. external user convo). 

Please keep it up and try to understand our point of view for avoiding these kinds of changes
in the future. 

Enrique, do you see where we are coming from with our concerns? 

Alex