Jonathan Gilbert wrote:
> 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.
>
O.K.
>>> + 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 :-)
>
De gustibus non disputandum est, so just leave it like that. :)
>>> + }
>>> +
>>> + /* 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 :-)
>
Don't misunderstand me, I have nothing against disallowing ~~. I just
think ~* should be disallowed as well, because it truly is nonsense and
will never match anything.
>>> @@ -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?
>
Read HACKING carefully again. We use doxygen only for public symbols;
this is a static function, and parameter capitalization is the right
format. We all love inconsistencies if we're consistent about them. :)
>>> @@ -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.
>
Ah. Well, I'd consider it a bug to have such a validation function and
yet not validate the whole file when loading it. So I'd just go ahead
and keep all validations here.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Aug 19 20:48:13 2006