At 09:33 AM 19/08/2006 +0200, Brane wrote:
>Jonathan Gilbert wrote:
>> The following patch adds two new types of rule to Subversion's
>> authz system:
>>
>> * Rules with the text "$" or "anonymous" match *only* the
>> anonymous user, and not authenticated users.
>
>I'm curious: Why "$"? I've often seen authz systems where "anonymous"
>was special, but none where "$" was special.
I think the reason I chose it because it is what 'sh' shows at the end of
the path for non-'su' logins :-) I wasn't really too worried about exactly
what character it was, I just wanted something that I could use in several
dozen authz rules without having to type "anonymous" over and over. At the
same time, I didn't want to deprive people of an easy-to-read moniker.
>> + return (recursion_result == FALSE);
>
>Just "return !recursion_result;" would be fine here, and more readable,
>at least for me.
I found this way more readable, but of course it's a trivial issue. I'll
change it :-)
>> + }
>> +
>> + /* Check for a pure-anonymous rule. */
>> + if ((strcmp(rule_match_string, "$") == 0)
>> + || (strcmp(rule_match_string, "anonymous") == 0))
>> + return (b->user == NULL);
>> +
>> + /* Check for a wildcard rule. */
>> + if (strcmp(rule_match_string, "*") == 0)
>> + return TRUE;
>>
>
>Hm, you disallow "~~foo", which could actually mean something, but you
>_will_ allow "~*", which is nonsense?
Well, "~~foo" is just equivalent to "foo", so why would anyone use it? If I
don't disallow "~~foo", some people might type "~~~~~~~~~~~~~~~~~~foo" just
to be obnoxious. I've seen stranger things in config files! You're probably
right that "~*" should be disallowed, but at least it doesn't have a
trivial equivalent :-)
>> @@ -436,7 +484,7 @@
>> /* Callback to check whether GROUP is a group name, and if so, whether
>> the group definition exists. Return TRUE if the rule has no
>> errors. Use BATON for context and error reporting. */
>> -static svn_boolean_t authz_validate_rule(const char *group,
>> +static svn_boolean_t authz_validate_rule(const char *rule_match_string,
>> const char *value,
>> void *baton,
>> apr_pool_t *pool)
>>
>
>You should fix the docstring if you change parameter names.
Oh, right, missed that one :-) Is it really proper convention to show the
parameter names in a different capitalization, by the way?
Actually, I just located this issue in HACKING, so in my patch, should I
rewrite just this function's doc string in doxygen format, or should I make
changes to it in the old format to keep consistency within the file?
>> @@ -444,9 +492,22 @@
>> const char *val;
>> struct authz_validate_baton *b = baton;
>>
>> + /* Make sure the user isn't using double-negatives. */
>> + if ((rule_match_string[0] == '~') && (rule_match_string[1] == '~'))
>> + {
>> + b->err = svn_error_create(SVN_ERR_AUTHZ_INVALID_CONFIG, NULL,
>> + "An authz rule uses inverted "
>> + "matching more than once; "
>> + "double negatives are not "
>> + "permitted.");
>> + return FALSE;
>> + }
>>
>
>Why do you check twice for double negatives? If you check this here, you
>don't have to do it again in authz_line_applies_to_user; and you could
>check for "~*" here, too.
I have the check in both places because I'm not certain whether or not
there's any way for an authz rule to sneak by the validation stage of
loading. If it is certain that it is not possible, then I'll remove the
validation from the new authz_line_applies_to_user() function and do it
only once, at authz file load time.
Thanks for the detailed review :-)
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Aug 19 18:38:38 2006