[PATCH RESEND] hgweb: add group authorization

[Wagner Bruna]

Em 27-02-2013 13:59, Markus Zapke-Gründemann escreveu:
> 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.

Fair enough.

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

It's mainly for extensibility: some of those "attributes" would be useful for
other group providers (they could specify an LDAP query, for instance). I
agree it's not particularly useful for plain config and Unix groups.

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

But your implementation already allows that, since a group defined in
web.options overrides a same-named Unix group.

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

Possibly (that's the setup I have at work, fortunately), but hgweb works on
non-Unix platforms too.

Regards,
Wagner

> Regards
> 
> Markus