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.
> * Rules preceded with a "~" are inverted, so that, for
> instance, "~anonymous" will match all authenticated users.
>
> As part of the change, the code to test whether a rule applies to
> the current user has been factored out into a new method in
> authz.c called authz_line_applies_to_user().
>
> The authz_validate_rule() method has also been updated to check
> for "double negatives" -- inversion used repeatedly within a
> single rule -- which I have explicitly denied to help assure
> a modicum of sanity.
>
> My e-mail client isn't very good at converting newlines and
> such with plain text attachments, so I'm including the patch
> text inline for review, and attaching the actual patch file as
> a binary file (gzipped) to avoid posting messages with LF line
> endings interspersed with CRLF line endings :-)
>
> Send all comments, and (hopefully) enjoy,
>
> Jonathan Gilbert
>
> [[[
> 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").
> ]]]
>
> Index: subversion/libsvn_repos/authz.c
> ===================================================================
> --- subversion/libsvn_repos/authz.c (revision 21101)
> +++ subversion/libsvn_repos/authz.c (working copy)
> @@ -161,6 +161,70 @@
> }
>
>
> +/* Determines whether an authz rule applies to the current
> + * user, given the name part of the rule's name-value pair
> + * and the authz_lookup_baton object 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);
>
Just "return !recursion_result;" would be fine here, and more readable,
at least for me.
> + }
> +
> + /* 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?
> + /* 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 +234,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;
> @@ -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.
> @@ -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.
> +
> /* 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 +529,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
--
-- 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 09:33:28 2006