Hi Andrew,
sorry that I didn't answer to your message.
Unfortunately I'm currently very busy with my job and also have currently
several "private projects" which need my attention.
This gives me currently nearly no spare time for looking at DAViCal.
You're right the patch was incomplete / inproper split from other changes - I
need to have a look at this.
And I'll also consider your other remoarks...
But probably not before 0.9.8 - that surely means that I have to rewrite the
patch - but this gives me some more time ;-)
Maybe I'll find some time during the Christmas holidays...
with regards,
Matthias
Andrew McMillan wrote:
> 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.
> ------------------------------------------------------------------------
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Davical-dev mailing list
> Davical-dev at lists.morphoss.com
> http://lists.morphoss.com/listinfo/davical-dev
sorry that I didn't answer to your message.
Unfortunately I'm currently very busy with my job and also have currently
several "private projects" which need my attention.
This gives me currently nearly no spare time for looking at DAViCal.
You're right the patch was incomplete / inproper split from other changes - I
need to have a look at this.
And I'll also consider your other remoarks...
But probably not before 0.9.8 - that surely means that I have to rewrite the
patch - but this gives me some more time ;-)
Maybe I'll find some time during the Christmas holidays...
with regards,
Matthias
Andrew McMillan wrote:
> 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.
> ------------------------------------------------------------------------
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Davical-dev mailing list
> Davical-dev at lists.morphoss.com
> http://lists.morphoss.com/listinfo/davical-dev