[geeklog-devel] Re: A&A AEPasswordGenerator.class.php

[Vincent Furia]

On Tue, 22 Jun 2004 13:15:21 -0500, Tony Bibbs <tony at tonybibbs.com> wrote:
> 1) This is more of a personal thing.  I hate appreviate IF..THEN so
> stuff like this:
>
> $len = ($gConf['randompasswordlength'] >= 4) ?
> $gConf['randompasswordlength'] : 4;
>
> Should be replace with the traditional IF..THEN.  This is more of a
> readability thing really.  This sort of stuff will get ironed out more
> when we get formal coding standards agreed on.  We should do that in the
> next week or so.
>

No problem.  I'll add that in to the GL2 coding style guide (in progress). :)

> 2) Most of your config variables have some sort of Auth_Enterprise
> equivalent in AEServerConfig.php.  Not a huge deal but we should either
> replace what I have or modify your code to use mine.
>

I realize that some of my variables will replace AEServerConfig.php
variables, I just wanted to hash out the code first before committing
to changing other files.

> 3) Your code assumes pspell and crack are in the path.  We should have
> config variables for them.  In fact we may want to include a user
> executable path:
> $gConf['user_executables'] = '/usr/bin/';
> $gConf['path_pspell'] = $gConf['user_executables'] . 'pspell';
> $gConf['path_crack'] = $gConf['user_executables'] . 'crack';
>
> We should also add an option for the pspell dictionary as we don't want
> to assume english.
>

I agree we need a variable for language, don't know what I was
thinking there.  For pspell and crack I planned to use the built in
PHP calls if they're available, and otherwise just skip those checks.
Those portions of the code haven't been tested because I can't get
PHP5 to work with either right now... *sigh*

Since we would use the built in functions, PHP will take care of
finding the relevant executables and libraries (or they would need to
be specified in php.ini).

> 4) Use of die() in Auth_Enterprise should not be allowed.  Instead we
> should throw an exception and let the calling application figure out
> what to do more gracefully.  You can either use one of the ones I have
> in AEExceptions.php or add a new one for that case to that file.
>

No problem, I should probably put in some "throws" in other places as well.

> Finally, in general I like the concept of my code in that they can
> configure if they require lowercase, require upper case, require special
> chars.  To that end, we should have regular expressions for all those
> permutations.  Also the minimum and maximum password length should
> probably be configurable.
>

Minimum and maximum password length is included in the first regex.
The "{7,20}" indicates repetition between 7 and 20 times (in this case
7-20 instances of the set of characters described).

> The regex's are nice.  It's probably a better way to go.  Good work. If
> you have questions let me know.  When we get this all ironed out, do you
> want to take a stab at adding these updates?
>

Sure, I like stabbing things.

> FWIW, this is a great example of how the OO-nature of Auth_Enterprise
> has.  SHould someone not like our implementation fo AEPasswordGenerator,
> they can write their own, drop it in their filesystem and start using it.
>

Couldn't agree more.

As for concerns about the regex being difficult to understand I think
providing a set of generic rules as $gConf vars and explaining what
they do with a note indicating to comment out the rule to not enforce
it, etc should be enough.  A decent (but not crazy) set of default
rules enabled will allow most users to simply ignore that section.
Maybe a link to a regex tutorial wouldn't be a bad idea either...

The reason I like the regex better than the flags has less to do with
performance (though better performance is always nice) and more to do
with flexibility.  Some companies (mine included) have extremely
complex password policies that are difficult to describe
(computationly) without regexes.  Along the same vein we might want to
consider keeping old password hashes so people can enforce a don't use
previous X passwords rule...

Anyway, I'll get to work on the changes you recommended above and have
a improved version available soon.

-Vinny