[PATCH RESEND] hgweb: add group authorization

[Markus Zapke-Gründemann]

Wagner Bruna schrieb:
> Em 07-02-2013 15:36, Markus Zapke-Gründemann escreveu:
>> # HG changeset patch
>> # User Markus Zapke-Gründemann <markus at keimlink.de>
>> # Date 1360231888 -3600
>> # Node ID d2dbfdee987a51efb6f4ad69e3b116aa22553326
>> # Parent  2fefd1170bf269e26bb304553009f38e0117c342
>> hgweb: add group authorization.
> 
> (sorry for taking so long to reply, I was kind of internet-impaired these days)
> 
> Very useful feature IMHO. I have a few suggestions below:
> 
>> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
>> --- a/mercurial/help/config.txt
>> +++ b/mercurial/help/config.txt
>> @@ -1286,8 +1286,12 @@ The full set of options is:
>>      push is not allowed. If the special value ``*``, any remote user can
>>      push, including unauthenticated users. Otherwise, the remote user
>>      must have been authenticated, and the authenticated user name must
>> -    be present in this list. The contents of the allow_push list are
>> -    examined after the deny_push list.
>> +    be present in this list. It is also possible to use groups in this
>> +    list. A group name is prefixed by an ``@``. Groups can either be
>> +    groups defined in the ``groups_section`` or Unix groups. If a group
>> +    from the ``groups_section`` has the same name as an Unix group it
>> +    is used instead. The contents of the allow_push list are examined
>> +    after the deny_push list.
>>  
>>  ``allow_read``
>>      If the user has not already been denied repository access due to
>> @@ -1297,8 +1301,12 @@ The full set of options is:
>>      denied for the user. If the list is empty or not set, then access
>>      is permitted to all users by default. Setting allow_read to the
>>      special value ``*`` is equivalent to it not being set (i.e. access
>> -    is permitted to all users). The contents of the allow_read list are
>> -    examined after the deny_read list.
>> +    is permitted to all users). It is also possible to use groups in
>> +    this list. A group name is prefixed by an ``@``. Groups can either
>> +    be groups defined in the ``groups_section`` or Unix groups. If a
>> +    group from the ``groups_section`` has the same name as an Unix group
>> +    it is used instead. The contents of the allow_read list are examined
>> +    after the deny_read list.
>>  
>>  ``allowzip``
>>      (DEPRECATED) Whether to allow .zip downloading of repository
>> @@ -1366,8 +1374,13 @@ The full set of options is:
>>      Whether to deny pushing to the repository. If empty or not set,
>>      push is not denied. If the special value ``*``, all remote users are
>>      denied push. Otherwise, unauthenticated users are all denied, and
>> -    any authenticated user name present in this list is also denied. The
>> -    contents of the deny_push list are examined before the allow_push list.
>> +    any authenticated user name present in this list is also denied. It
>> +    is also possible to use groups in this list. A group name is
>> +    prefixed by an ``@``. Groups can either be groups defined in the
>> +    ``groups_section`` or Unix groups. If a group from the
>> +    ``groups_section`` has the same name as an Unix group it is used
>> +    instead. The contents of the deny_push list are examined before the
>> +    allow_push list.
>>  
>>  ``deny_read``
>>      Whether to deny reading/viewing of the repository. If this list is
>> @@ -1380,9 +1393,12 @@ The full set of options is:
>>      deny_read and allow_read are empty or not set, then access is
>>      permitted to all users by default. If the repository is being
>>      served via hgwebdir, denied users will not be able to see it in
>> -    the list of repositories. The contents of the deny_read list have
>> -    priority over (are examined before) the contents of the allow_read
>> -    list.
>> +    the list of repositories. It is also possible to use groups in this
>> +    list. A group name is prefixed by an ``@``. Groups can either be
>> +    groups defined in the ``groups_section`` or Unix groups. If a group
>> +    from the ``groups_section`` has the same name as an Unix group it is
>> +    used instead. The contents of the deny_read list have priority over
>> +    (are examined before) the contents of the allow_read list.
>>  
>>  ``descend``
>>      hgwebdir indexes will not descend into subdirectories. Only repositories
>> @@ -1400,6 +1416,30 @@ The full set of options is:
>>  ``errorlog``
>>      Where to output the error log. Default is stderr.
>>  
>> +``groups_section``
>> +    Name of hgrc section used to define groups for authorization.
>> +    Default is ``web.groups``. Use the section to define the groups used
>> +    by authorization.
>> +
>> +    Example::
>> +
>> +        [web]
>> +        allow_read = @devs
>> +
>> +        [web.groups]
> 
> Minor nit: the dot could cause some confusion with the section.key notation,
> so I'd call it simply "webgroups" (or even "usergroups").
I've used the same notation used by the acl extension. It uses acl.allow and
acl.deny for example. IMO this is a good design. One can see that this is a
subsection of the web section.

>> +        devs = alice, bob, clara, david
>> +
>> +    Groups can contain other groups::
>> +
>> +        [web]
>> +        allow_read = @devs, @testers
>> +        allow_push = @devs
>> +
>> +        [web.groups]
>> +        devs = alice, bob, clara, david
>> +        ci = hudson
>> +        testers = @ci, lisa, mario
> 
> What about a different notation, similar to the [auth] section, supporting
> per-group attributes:
> 
> 
> [webgroups]
> 
> devs.type = hgconfig
> devs.members = alice, bob, clara, david
> 
> # this would pull members from the same named unix group...
> anotherteam.type = unix
> # ...or perhaps explicitly:
> # anotherteam.group = anotherteamunixgroup
> 
> # the '@' marker would be fully optional then:
> @ci.members = hudson
> # @ci.type = hgconfig would be the default here
I don't see the advantage of the explicit declaration in your solution. The
current implementation is simpler and therefore easier to understand. If it
would be possible to "overwrite" Unix groups with groups created in hgrc this
could cause some confusion.

The @ marker is again taken from the acl extension which is using it also for
groups.

> Together with a bit more flexibility in the _ismember function below (that
> could be implemented in a follow-up patch), additional group providers could
> then be easily added through extensions (ldap comes to mind).
The integration of LDAP or many other services can be easily done on Unix-based
systems using PAM. I think it's easier to rely on PAM then implementing
everything again in hgweb.


Regards

Markus