[PATCH] acl: support negating the list of users/groups with leading "~"

[John Mulligan]

On Mon, Feb 07, 2011 at 04:31:55PM -0600, Matt Mackall wrote:
> On Mon, 2011-02-07 at 17:03 -0500, John Mulligan wrote:
> > # HG changeset patch
> > # User John Mulligan <phlogistonjohn at asynchrono.us>
> > # Date 1297114623 18000
> > # Node ID 864fb160e483d212c3a3c64362766f1c0cbd95ee
> > # Parent  69e69b131458023d21ec40aa48fc5299e43ce69b
> > acl: support negating the list of users/groups with leading "~"
> > 
> > the config lines in the acl extension that accept user or goup lists may
> > be prefixed with a "~" character which stands for "not matching" and
> > applies to all users/groups in the list. This allows the statement "deny access
> > to any user not in the following list" to be easily expressed in the config file.
> > The tilde character is used rather than the "!" in order not to be confused
> > with the hgrc method of disabling other configuration lines.
> 
> (I hate this sort of thing. It's a documentation nightmare. And most
> users will continually file bugs even if the docs are correct and
> precise, because there exists no complete and consistent algorithm that
> matches their intuition of how such things should work.)
> 

I understand, I wasn't sure if I wanted to approach it this way or not.
It certainly did not match my intuition.

> What are the ordering rules?

Currently if there are any deny rules that match a named branch the
users in that rule will be rejected. Then if the allow section is
present only users listed in the rule for a named branch will be
allowed. If the section does not list rule for a branch it appears to
reject all users for that branch.
My change was indented to let me say deny access to all users except the
release engineers on the release branch, but all other branches are
allowed.

> 
> > In addition this adds tests for branch acls.
> > 
> > Example:
> > 
> > [acl.deny.branches]
> > admin = ~ admin1, admin2
> > release = ~release-engineer
> 
> This example is a good example of why this is confusing. What does this
> example mean? Denying to all except admin1 and admin2 or do we accept
> admin1 and deny admin2?
> 
> Why is this better than:
> 
> [acl.accept.branches]
> admin = admin1, admin2
> 
> (which implicitly denies everyone else?)

I thought that was going to work for us, but it denies access to all
named branches that are unlisted. We do not like the idea that we would
have to go into the config file every time we added a named branch to
allow access to it. If there were a way to accomplish this without new
code I would be happy, but I did not see a way to do so.


> 
> The ACL rules are -already- too complex.
> 

I can't disagree with that. After all, I tried a bunch of configs that I
thought might fix our issue before reaching for the code. Plus I hadn't
even looked at the file based part.

I can think of a couple of other approaches that get us what we want.
 1) Have a general option of default deny or default accept. If default
    accept is set the accept rules only apply to the excplicly listed
    branches.
 2) Have a special name/symbol that would apply to all branches that are
    not explictily listed.

I'm leaning more toward the first option, but I'd like to find out what
you might have in mind for improving the situation for all users.


Thanks,
John M.