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