I made a few more style tweaks on the second pass. Also, I saw that
the validation code didn't carry out further validation on an inverted
rule (eg. '~@non_existent_group' didn't get caught). I rejiggered the
authz validation function to account for inversions and keep sanity
checking past the inversion.
I've got to head out, and my trunk hasn't finished building, so I'm
posting the tweaked patch back here. If you have a minute, I'd
appreciate another pair of eyes on the tweaks. If my trunk builds and
passes basic sanity checks, I'll submit the patch when I get back, in
a little over 6hrs.
Also, the new syntax and logic could really do with unit tests. Do you
feel up to writing those? If you do, great! If not, no big deal, I'll
put some together and submit them back to back with your patch.
Thanks!
- Dave
On 3/8/07, Jonathan Gilbert <o2w9gs702@sneakemail.com> wrote:
> 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
> it! :-)
> >
> >+ /* 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
> method.
>
> >+ /* 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.
>
> 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:
>
> [/some/path]
> $anonymous = r
> $authenticated = rw
>
> ..instead of:
>
> [/some/path]
> $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
> >continuation.
>
> Done.
>
> >-/* 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
> >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 :) )
>
> 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."
>
> Done :-)
>
> >+ 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."
>
> Done. I kept the word "Authz", but changed the latter half of the sentence
> as suggested.
>
> >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
> >your patch.
>
> This was the only change made since the patch was last rolled:
>
> @@ -464,6 +464,12 @@
> /* Recurse on that group. */
> SVN_ERR(authz_group_walk(cfg, &group_user[1],
> checked_groups, pool));
> +
> + /* 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[1],
> + 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. :-)
>
> Jonathan Gilbert
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 8 06:56:35 2007