security fix: fix virtual group bug in ACL evaluation, add a test for it
authorThomas Waldmann <tw AT waldmann-edv DOT de>
Mon, 03 Sep 2012 15:30:35 +0200
changeset 58707b9f39289e16
parent 5869 0e58d9bcd3bd
child 5871 2e90d7b58b42
security fix: fix virtual group bug in ACL evaluation, add a test for it

affected moin releases: all 1.9 releases up to and including 1.9.4

moin releases < 1.9 are NOT affected.

You can find out the moin version by looking at SystemInfo page or at the
output of <<SystemInfo>> macro.

Issue description:

We have code that checks whether a group has special members "All" or "Known"
or "Trusted", but there was a bug that checked whether these are present in
the group NAME (not, as intended, in the group MEMBERS).

a) If you have group MEMBERS like "All" or "Known" or "Trusted", they did not
work until now, but will start working with this changeset.

E.g. SomeGroup:
* JoeDoe
* Trusted

SomeGroup will now (correctly) include JoeDoe and also all trusted users.

It (erroneously) contained only "JoeDoe" and "Trusted" (as a username, not
as a virtual group) before.

b) If you have group NAMES containing "All" or "Known" or "Trusted", they behaved
wrong until now (they erroneously included All/Known/Trusted users even if
you did not list them as members), but will start working correctly with this
changeset.

E.g. AllFriendsGroup:
* JoeDoe

AllFriendsGroup will now (correctly) include only JoeDoe.
It (erroneously) contained all users (including JoeDoe) before.

E.g. MyTrustedFriendsGroup:
* JoeDoe

MyTrustedFriendsGroup will now (correctly) include only JoeDoe.
It (erroneously) contained all trusted users and JoeDoe before.
MoinMoin/security/__init__.py
MoinMoin/security/_tests/test_security.py
     1.1 --- a/MoinMoin/security/__init__.py	Fri Aug 03 17:36:02 2012 +0200
     1.2 +++ b/MoinMoin/security/__init__.py	Mon Sep 03 15:30:35 2012 +0200
     1.3 @@ -320,11 +320,12 @@
     1.4                  handler = getattr(self, "_special_"+entry, None)
     1.5                  allowed = handler(request, name, dowhat, rightsdict)
     1.6              elif entry in groups:
     1.7 -                if name in groups[entry]:
     1.8 +                this_group = groups[entry]
     1.9 +                if name in this_group:
    1.10                      allowed = rightsdict.get(dowhat)
    1.11                  else:
    1.12                      for special in self.special_users:
    1.13 -                        if special in entry:
    1.14 +                        if special in this_group:
    1.15                              handler = getattr(self, "_special_" + special, None)
    1.16                              allowed = handler(request, name, dowhat, rightsdict)
    1.17                              break # order of self.special_users is important
     2.1 --- a/MoinMoin/security/_tests/test_security.py	Fri Aug 03 17:36:02 2012 +0200
     2.2 +++ b/MoinMoin/security/_tests/test_security.py	Mon Sep 03 15:30:35 2012 +0200
     2.3 @@ -16,10 +16,11 @@
     2.4  acliter = security.ACLStringIterator
     2.5  AccessControlList = security.AccessControlList
     2.6  
     2.7 +from MoinMoin.datastruct import ConfigGroups
     2.8  from MoinMoin.PageEditor import PageEditor
     2.9  from MoinMoin.user import User
    2.10  
    2.11 -from MoinMoin._tests import become_trusted, create_page, nuke_page
    2.12 +from MoinMoin._tests import wikiconfig, become_trusted, create_page, nuke_page
    2.13  
    2.14  class TestACLStringIterator(object):
    2.15  
    2.16 @@ -248,6 +249,50 @@
    2.17                  assert not acl.may(self.request, user, right)
    2.18  
    2.19  
    2.20 +class TestGroupACL(object):
    2.21 +
    2.22 +    class Config(wikiconfig.Config):
    2.23 +        def groups(self, request):
    2.24 +            groups = {
    2.25 +                u'PGroup': frozenset([u'Antony', u'Beatrice', ]),
    2.26 +                u'AGroup': frozenset([u'All', ]),
    2.27 +                # note: the next line is a INTENDED misnomer, there is "All" in
    2.28 +                # the group NAME, but not in the group members. This makes
    2.29 +                # sure that a bug that erroneously checked "in groupname" (instead
    2.30 +                # of "in groupmembers") does not reappear.
    2.31 +                u'AllGroup': frozenset([]), # note: intended misnomer
    2.32 +            }
    2.33 +            return ConfigGroups(request, groups)
    2.34 +
    2.35 +    def testApplyACLByGroup(self):
    2.36 +        """ security: applying acl by group name"""
    2.37 +        # This acl string...
    2.38 +        acl_rights = [
    2.39 +            "PGroup,AllGroup:read,write,admin "
    2.40 +            "AGroup:read "
    2.41 +            ]
    2.42 +        acl = security.AccessControlList(self.request.cfg, acl_rights)
    2.43 +
    2.44 +        # Should apply these rights:
    2.45 +        users = (
    2.46 +            # user, rights
    2.47 +            ('Antony', ('read', 'write', 'admin', )),  # in PGroup
    2.48 +            ('Beatrice', ('read', 'write', 'admin', )),  # in PGroup
    2.49 +            ('Charles', ('read', )),  # virtually in AGroup
    2.50 +            )
    2.51 +
    2.52 +        # Check rights
    2.53 +        for user, may in users:
    2.54 +            mayNot = [right for right in self.request.cfg.acl_rights_valid
    2.55 +                      if right not in may]
    2.56 +            # User should have these rights...
    2.57 +            for right in may:
    2.58 +                assert acl.may(self.request, user, right)
    2.59 +            # But NOT these:
    2.60 +            for right in mayNot:
    2.61 +                assert not acl.may(self.request, user, right)
    2.62 +
    2.63 +
    2.64  class TestPageAcls(object):
    2.65      """ security: real-life access control list on pages testing
    2.66      """