At 05:11 AM 3/8/2007 +0100, David Anderson wrote:
>... And from the depths of time (and a ping on IRC), I revive and review
>+ /* Work out whether this ACL line applies to the user. */
>Comment not needed.
Gone :-) It was inherited as part of the code factoring from the original
>+ /* Double inversion? This rule is malformed, so it
>+ * definitely does not apply!
>+ if (rule_match_string == '~')
>+ return FALSE;
>Opening an authz file always implies validation, no need to recheck here.
Okay. I felt it was cheap enough to check, but then even though
double-negatives are disallowed by policy, there's nothing inherently
unprocessable about them. I'll change it so that if, somehow,
double-negatives get past the validation, they simply cancel each other
out. No additional code will be needed for this, naturally.
>+ /* Check for a pure-anonymous rule. */
>+ if ((strcmp(rule_match_string, "$") == 0)
>+ || (strcmp(rule_match_string, "anonymous") == 0))
>I don't like reserving 'anonymous' as a special-value keyword. I could
>see people using that as the unprivileged user for repository access.
>And having a rule for 'anonymous' not apply to the authenticated user
>called 'anonymous' would cause much confusion.
Fair enough. And, of course, whatever we name it, if it *could* be a
username, someone eventually will be trying to use that username and
wondering what's going on. :-)
>That said, I don't have any suggestion for a better name. How about
>having only '$', or using '$anonymous' for the anonymous user?
I see nothing wrong with having $anonymous as a keyword in this situation.
>Also, would it make more sense to make this keyword match only all
>authenticated users instead? I know, the inversion rule can make one
>out of the other anyway, I'm just wondering which is the most useful
>to have immediately available.
Well, my need was for matching anonymous users, and "anonymous" is a nice,
well-understood keyword. While people would probably understand
"$authenticated" just as well, it would probably be doing things backwards
from any other program that had a similar kind of authz system.
In any event, I've changed the patch to do what we discussed on IRC -- to
support both $anonymous and $authenticated, rather than having only
$anonymous. I figured it would be best to eliminate '$' on its own as a
synonym for $anonymous; it would be too confusing otherwise. Now an authz
file can specify this:
$anonymous = r
$authenticated = rw
$anonymous = r
~$anonymous = rw
I think this improves maintainability :-)
I also extended the validation to reject entries starting with '$' that
aren't '$anonymous' or '$authenticated'; this leaves the token space open
for future expansion, whatever that may be... :-)
>+ /* Always pass if the rule doesn't apply to this user. */
>/* Stop if the rule doesn't apply. */. Pass to me implies success and
>-/* Callback to check whether NAME is a group or alias name, and if so,
>- whether the group or alias 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 *name,
>+/* Callback to perform some simple sanity checks on an authz rule.
>+ If RULE_MATCH_STRING is a group name (or an inverted group name),
>Maybe the sanity checks could be made into a more readable bulleted list?
>/* Callback to perform some simple sanity checks on an authz rule.
> * - If RULE_MATCH_STRING refers to a group or alias, check that the
>group/alias is defined.
> * - If RULE_MATCH_STRING is using an inversion, check that it isn't
>using it several times, and that the rule isn't '~*', which never
> * - Check that VALUE specifies only allowed rule flag characters ('r' and
> * Return TRUE if the rule has no errors, and use BATON for context
>and error reporting.
>(yeah, I'm being persnickety, so what :) )
Done with only minor differences. :-)
>+ "Inverted matching more than once "
>+ "in match string '%s'; double "
>+ "negatives are not permitted.",
>"Rule '%s' has more than one inversion; double negatives are not permitted."
>+ return FALSE;
>+ /* Make sure that the rule isn't "~*", which won't ever match. */
>+ if (strcmp(rule_match_string, "~*") == 0)
>+ b->err = svn_error_create(SVN_ERR_AUTHZ_INVALID_CONFIG, NULL,
>+ "Authz rules with match string '~*' "
>+ "are disallowed, because they match "
>+ "no users, and thus have no effect.");
>"Rules matching '~*' are not allowed, because they never match anyone."
Done. I kept the word "Authz", but changed the latter half of the sentence
>I'm happy about what the patch actually adds in way of functionality.
>Aside from the rule naming for anonymous, all the rest is really minor
>stuff, so I think we can get this submitted quickly.
That makes me very happy :-D
>Note that there has been a change in the authz code in r22825, after
>you created the patch. Your patch still applies with a little offset
>fuzzing, and looking at the logs for the revision it seems that you
>should be okay, but best make sure you can update and still build with
This was the only change made since the patch was last rolled:
@@ -464,6 +464,12 @@
/* Recurse on that group. */
+ /* Remove group from hash of checked groups, so that we don't
+ incorrectly report an error if we see it again as part of
+ another group. */
+ apr_hash_set(checked_groups, &group_user,
+ APR_HASH_KEY_STRING, NULL);
else if (*group_user == '&')
I think we can agree that that won't interfere with the rule decoder
changes in my patch. :-)
I've attached the updated patch, gzipped as before. I apologize for the
inconvenience of a gzipped patch, but my e-mail client doesn't properly
escape LF newline characters in attachments that look like text, and my
mail server rejects messages that violate the mail RFC by having naked LF
characters in them. :-)
Received on Thu Mar 8 06:03:57 2007
To unsubscribe, e-mail: email@example.com
For additional commands, e-mail: firstname.lastname@example.org