On Thu, 2009-10-29 at 15:34 +0100, Matthias Mohr wrote:
> Hi Andrew,
>
> as promised a while ago, I'll reinvestigate my changes and split them
> in several smaller parts.
>
> Here's one which adds a new configuration parameter
> "$c->usernames_case_insensitive"
I prefer more plain-english sounding names, so in this case it would
seem better to be:
$c->case_insensitive_usernames
> If set to true, all username handling will be treated case insensitive;
> they'll still be stored in the case which the user enters (or the external
> authorisation hook provides), but the comparison or search or other access
> will not distinguish between cases.
>
> It's tested here and works with LDAP authorisation - but I'm not absolutely
> sure if there are other places which I missed (e.g. in drivers_squid_pam)!?
Yeah, quite possibly. Given that calls an external script it may be
difficult to arrange for case insensitivity without changing that
external script.
> Though one issue exists:
> Currently it's not possible to access the configuration array in libawl,
> so there's one place where the case insensitive comparison is hardcoded
> (I added a @TODO at that place)...
Sure it's possible. The function you were modifying even declared
"global $c;" already :-)
> if ( isset($username) && $username != "" ) {
> - $where = "WHERE active AND usr.username = ". qpg($username );
> + // MM-20091029: case insensitive comparison of usernames
> + // @TODO: respect DAViCal configuration setting...
> + $where = 'WHERE active AND LOWER(usr.username) = LOWER(' . qpg($username) . ')';
> }
> else if ( isset($email_address) && $email_address != "" ) {
> - $where = "WHERE active AND usr.email = ". qpg($email_address );
> + // MM-20091029: case insensitive comparison of email addresses
> + $where = 'WHERE active AND LOWER(usr.email) = LOWER(' . qpg($email_address) . ')';
> }
Since this is doing forgotten password lookups I'd already changed it
some weeks ago to always be lower case:
http://repo.or.cz/w/awl.git?a=commitdiff;h=ac1d854256e18fb458e5a5bbd93e72a985da5be3 > - $this->Order .= $field;
> + // MM-20091029: Hack: case insensitive sorting for string fields:
> + // @TODO: find a better more genereic way to detect string fields
> + if ( strcasecmp($field, 'username') == 0 ||
> + strcasecmp($field, 'fullname') == 0 ||
> + strcasecmp($field, 'email') == 0 ) {
> + $this->Order .= 'LOWER(' . $field . ')';
> + } else {
> + $this->Order .= $field;
> + }
When I create my database with LC_COLLATE=en_NZ.UTF8 I see case
insensitivity in list sorts already, and I think that is probably a
better way to go for this one.
If you still want this functionality then send me a patch providing an
additional field property & method where the application can define the
field as case-insensitive. Depending on the username/fullname/email
fieldname is a truly ugly hack, and completely unacceptable in library
code like this :-)
> @@ -470,6 +471,6 @@
> }
> return false;
> }
>
> }
> -
> +?>
Please don't add back all the ?> that I have carefully removed from the
ends of all of these files...
While it is still there on some files it's only because I haven't edited
that file since I started my war on these. The direction should be
removal rather than adding :-)
> foreach($ldap_users_tmp as $key => $ldap_user){
> - $ldap_users_info[$ldap_user[$mapping["username"]]] = $ldap_user;
> + // MM-20090703: if usernames shall be case insensitive...
> + $my_username = $ldap_user[ $mapping['username'] ];
> + if ( isset($c->usernames_case_insensitive) && $c->usernames_case_insensitive ) {
> + $my_username = strtolower( $my_username );
> + }
> + $ldap_users_info[ $my_username ] = $ldap_user;
A better variable name might be '$mapped_username'. Also please remove
the comment which doesn't say anything more than the name of the
configuration item already does.
Given the number of times this code fragment occurs in the patch, it
might be better to wrap it in a small function so that the change
becomes:
$ldap_users_info[ some_function($ldap_user[$mapping['username']]) ] = $ldap_user;
Or something better thought-out which is more reusable throughout
drivers_ldap.php.
> - $qry = new PgQuery( "SELECT username, user_no, updated FROM usr ");
> +
> + // MM-20091029: always sort case insensitive...
> + $sql = 'SELECT username, user_no, updated FROM usr ORDER BY LOWER(username)';
I'm curious why the database collation doesn't do this already. I'd
rather leave sort order out of the patch for the time being.
> $usr_in = substr($usr_in,1);
> $c->messages[] = sprintf(i18n('- deactivating users : %s'),join(', ',$users_to_deactivate));
> - $qry = new PgQuery( "UPDATE usr SET active = FALSE WHERE lower(username) IN ($usr_in)");
> + $sql = 'UPDATE usr SET active = FALSE WHERE ';
> + if ( isset($c->usernames_case_insensitive) && $c->usernames_case_insensitive ) {
> + // MM-20091029: if usernames shall be case insensitive...
> + $sql .= 'LOWER(username)';
> + } else {
> + $sql .= 'username';
> + }
> + $qry = new PgQuery( $sql );
That looks suspiciously like it is not going to apply cleanly... Does
the existing code really say lower(username) at that point?
> $ldap_timestamp = "$Y"."$m"."$d"."$H"."$M"."$S";
> $valid[$mapping["updated"]] = "$Y-$m-$d $H:$M:$S";
>
> $db_timestamp = substr(strtr($db_users_info[$username]['updated'], array(':' => '',' '=>'','-'=>'')),0,14);
> if ( $ldap_timestamp > $db_timestamp ) {
> - sync_user_from_LDAP($usr, $mapping, $valid );
> + sync_user_from_LDAP($user, $mapping, $valid );
I feel that subtle variable name differentiation like that is setting us
up for errors in the future.
> }
> else {
> unset($users_to_update[$key]);
> - $users_nothing_done[] = $username;
> + // MM-20091029: always use the unchanged username here:
> + $users_nothing_done[] = $my_username;
That comment doesn't seem to match the code. If the comment is needed
then it would be valuable to have the *reason* behind what the code
does, rather than the *statement* of what the code does, which should be
(more or less) readable from the code itself after all.
Cheers,
Andrew.
------------------------------------------------------------------------
andrew (AT) morphoss (DOT) com +64(272)DEBIAN
You will be awarded some great honor.
------------------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <
http://lists.morphoss.com/pipermail/davical-dev/attachments/20091030/e09d0e7f/attachment.pgp>