... And from the depths of time (and a ping on IRC), I revive and review it! :-)
Index: subversion/libsvn_repos/authz.c
===================================================================
+/* Determines whether an authz rule applies to the current
+ * user, given the name part of the rule's name-value pair
+ * in RULE_MATCH_STRING and the authz_lookup_baton object
+ * B with the username in question.
+ */
+static svn_boolean_t
+authz_line_applies_to_user(const char *rule_match_string,
+ struct authz_lookup_baton *b,
+ apr_pool_t *pool)
+{
+ /* Work out whether this ACL line applies to the user. */
Comment not needed.
+ /* Is this rule an inversion? */
+ if (rule_match_string[0] == '~')
+ {
+ svn_boolean_t recursion_result;
+
+ /* Double inversion? This rule is malformed, so it
+ * definitely does not apply!
+ */
+ if (rule_match_string[1] == '~')
+ return FALSE;
Opening an authz file always implies validation, no need to recheck here.
+ /* Recurse, and return the opposite. */
+ recursion_result = authz_line_applies_to_user(
+ &rule_match_string[1], b, pool);
+
+ return (recursion_result == FALSE);
+ }
+
+ /* 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.
That said, I don't have any suggestion for a better name. How about
having only '$', or using '$anonymous' for the anonymous user?
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.
+ return (b->user == NULL);
@@ -200,34 +268,10 @@
{
struct authz_lookup_baton *b = baton;
- /* Work out whether this ACL line applies to the user. */
- if (strcmp(name, "*") != 0)
- {
- /* Non-anon rule, anon user. Stop. */
- if (!b->user)
- return TRUE;
+ /* Always pass if the rule doesn't apply to this user. */
/* Stop if the rule doesn't apply. */. Pass to me implies success and
continuation.
+ if (!authz_line_applies_to_user(name, b, pool))
+ return TRUE;
@@ -484,10 +528,16 @@
}
-/* 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),
No need for the parenthesized bit. "If RULE_MATCH_STRING refers to a
group name or alias, check that the group or alias definition exists."
+ check whether the group definition exists. If RULE_MATCH_STRING
+ is an alias, verify that the alias definition exists. If
+ RULE_MATCH_STRING is using inversion, verify that it isn't doing it
+ more than once, and that it isn't "~*", a rule which can never have
+ any effect. Make sure that VALUE part of the rule specifies only
+ allowed rule flag characters ('r' and 'w'). Return TRUE if the rule
+ has no errors. Use BATON for context and error reporting. */
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
matches.
* - Check that VALUE specifies only allowed rule flag characters ('r' and 'w').
*
* Return TRUE if the rule has no errors, and use BATON for context
and error reporting.
*/
(yeah, I'm being persnickety, so what :) )
@@ -495,10 +545,34 @@
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_createf(SVN_ERR_AUTHZ_INVALID_CONFIG, NULL,
+ "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."
+ rule_match_string);
+ 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."
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.
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
your patch.
Thanks!
- Dave
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 8 05:11:30 2007