Re: [PATCH] Add anonymous user and inverted authz rule matching.

At 08:46 PM 19/08/2006 +0200, you wrote:
>Jonathan Gilbert wrote:
>> 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. :)

All right :-)

>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.

I agree :-) I have added it to the validation in the authz_validate_rule()
function, and I have rewritten the function's doc string, since it now does
a lot more than check groups. The validation doesn't need to be added to
authz_line_applies_to_user(), because it will already parse "~*" correctly
to mean "match nobody".

>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. :)

Okay :-) I kept the capitalized parameters in authz_validate_rule()'s doc
string, and I added capitalized parameter names the comment before
authz_line_applies_to_user().

>> 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.

I was more thinking about ways to get at authz rules without using the
regular loading mechanism, but of course any way around it is a bug of some
sort. The validation is pretty cheap, anyway -- it just directly checks the
second character of the string, no function calls involved. :-)

Here is the updated patch (again, inline with CRLF newlines + attached
gzipped with native newlines):

[[[
Add pure anonymous rules & inverted rules to the authz system.

* subversion/libsvn_repos/authz.c
  (authz_line_applies_to_user): New method factored out of
   authz_parse_line() and extended to support pure anonymous
   rules & inverted rules.
  (authz_validate_rule): Reject doubly-inverted rules ("double
   negatives") and "~*", which never matches.
]]]

Index: subversion/libsvn_repos/authz.c
===================================================================
--- subversion/libsvn_repos/authz.c (revision 21128)
+++ subversion/libsvn_repos/authz.c (working copy)
@@ -161,6 +161,71 @@
 }

+/* 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. */
+
+ /* 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;
+
+ /* 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))
+ return (b->user == NULL);
+
+ /* Check for a wildcard rule. */
+ if (strcmp(rule_match_string, "*") == 0)
+ return TRUE;
+
+ /* If we get here, then the rule is:
+ * - Not an inversion rule.
+ * - Not a pure-anonymous rule.
+ * - Not a wildcard rule.
+ *
+ * All that's left over is regular old-fashioned
+ * user or group specifications.
+ */
+
+ /* If the session is anonymous, then a user/group
+ * rule definitely won't match.
+ */
+ if (b->user == NULL)
+ return FALSE;
+
+ /* Process the rule depending on whether it is
+ * a user or group rule.
+ */
+ if (rule_match_string[0] == '@')
+ return authz_group_contains_user(
+ b->config, &rule_match_string[1], b->user, pool);
+ else
+ return (strcmp(b->user, rule_match_string) == 0);
+}
+
+
 /* Callback to parse one line of an authz file and update the
  * authz_baton accordingly.
  */
@@ -170,26 +235,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. */
+ if (!authz_line_applies_to_user(name, b, pool))
+ return TRUE;

- /* Group rule and user not in group. Stop. */
- if (*name == '@')
- {
- if (!authz_group_contains_user(b->config, &name[1],
- b->user, pool))
- return TRUE;
- }
-
- /* User rule for wrong user. Stop. */
- else if (strcmp(name, b->user) != 0)
- return TRUE;
- }
-
   /* Set the access grants for the rule. */
   if (strchr(value, 'r'))
     b->allow |= svn_authz_read;
@@ -433,10 +482,15 @@
 }

-/* Callback to check whether GROUP is a group name, and if so, whether
- the group definition exists. Return TRUE if the rule has no
+/* Callback to perform some simple sanity checks on an authz rule.
+ If RULE_MATCH_STRING is a group name (or an inverted group name),
+ check whether the group 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. */
-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)
@@ -444,10 +498,33 @@
   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_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.");
+ return FALSE;
+ }
+
   /* If the rule applies to a group, check its existence. */
- if (*group == '@')
+ if (*rule_match_string == '@')
     {
- svn_config_get(b->config, &val, "groups", &group[1], NULL);
+ const char *group = &rule_match_string[1];
+
+ svn_config_get(b->config, &val, "groups", group, NULL);
       /* Having a non-existent group in the ACL configuration might be
          the sign of a typo. Refuse to perform authz on uncertain
          rules. */
@@ -468,7 +545,8 @@
       if (*val != 'r' && *val != 'w' && ! svn_ctype_isspace(*val))
         b->err = svn_error_createf(SVN_ERR_AUTHZ_INVALID_CONFIG, NULL,
                                    "The character '%c' in rule '%s' is not "
- "allowed in authz rules", *val, group);
+ "allowed in authz rules", *val,
+ rule_match_string);

       ++val;
     }

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org