Hi Dan,
> ...
>
> This sounds fairly reasonable to me. One big question I have is how usernames
> are then recorded in the history (e.g. uppercase, lowercase, or as entered
> by the user?).
>
>
Usernames are recorded as provided by the user. I did not want to change
the request_rec->user.
>> [[[
>> Make mod_authz_svn to apply the authz checks against upper/lowercased
>> usernames.
>>
>> * subversion/mod_authz_svn/mod_authz_svn.c
>> (): Include 'apr_lib.h' and 'strings.h'.
>> (struct authz_svn_config_rec): New member 'usernamecase'.
>>
>
> How about "force_username_case", or something similar that indicates what
> we're doing with username case?
>
>
Yes 'force_username_case' makes more sense. Fixed my patch.
>> (authz_svn_cmds): Populate 'authz_svn_config_rec.usernamecase'
>> from configuration directive 'AuthzUsernameCase'.
>>
>
> Ditto for the name of the directive, and even more so for the help (which
> should mention that the case is applied to authz).
>
>
Changed the directive to 'AuthzForceUsernameCase'.
>> (convert_to_uppercase_string,
>> convert_to_lowercase_string,
>> get_username_to_authorize): New functions.
>>
>
> Do we really need both of those conversion functions? Seems like one
> should be sufficient.
>
Yes fixed that too.
>
>> (req_check_access, subreq_bypass): Apply authz check against
>> upper/lowercased usernames.
>>
>
> upper/lower-cased
>
>
Fixed the log message.
> This patch doesn't match our coding conventions, but seems to comply with
> the code in mod_authz_svn...
>
>
Will fix mod_authz_svn itself to match our coding guidelines in
subsequent commit.
> Yeah, these are really redundant; only the apr_toupper()/apr_tolower() calls
> differ. We definitely want a single function here.
>
>
Fixed.
>> if (conf->usernamecase) {
>> + username_to_authorize = apr_pstrdup(r->pool, r->user);
>> + if (strcasecmp(conf->usernamecase, "upper") == 0)
>> + convert_to_uppercase_string(username_to_authorize);
>> + else
>> + convert_to_lowercase_string(username_to_authorize);
>>
>
> Ya know, I'd probably just inline the code here.
>
Fixed.
>
>> + }
>>
>
> If we don't change r->user, I think the history will take the username as
> specified by the end-user. This means that the history will be potentially
> subject to several capitalizations of the same name.
>
>
Yes, you are correct. Can each module change the request_rec->user like
this?
Committed with above fixes in r29875
Thanks for the review.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-12 17:15:26 CET