[#1038] Reverse the meaning of group member of group.

Now member.table_id is the *parent* of member.group_id (not the child). This means the permissions work - a user needs permissions for the member.group_id to change its members, and it makes more sense for a group to choose its parents than its children. This is because a user with rights higher up the tree often gets given automatic rights to groups lower down. If he can also make any other group a child, then he would give himself rights over it.

Added test for the group permissions to check it made sense

Fixed bug in get_top_level_groups - the 'and' was ignored before. The existing test picked this up.

group.get_groups meaning is expanded. Seems to only get used in
_group_or_org_update and treats the result as parents. Needs further
examination when doing the form work on this ticket.

ckan/lib/create_test_data.py

@@ -386,7 +386,7 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""):
             if 'parent' in group_dict:
                 parent = model.Group.by_name(unicode(group_dict['parent']))
                 assert parent, group_dict['parent']
-                member = model.Member(group=parent, table_id=group.id,
+                member = model.Member(group=group, table_id=parent.id,
                                       table_name='group', capacity='parent')
                 model.Session.add(member)
             #model.setup_default_user_roles(group, admin_users)
@@ -527,8 +527,10 @@ def create(cls, auth_profile="", package_type=None):
         roger = model.Group.by_name(u'roger')
         model.setup_default_user_roles(david, [russianfan])
         model.setup_default_user_roles(roger, [russianfan])
-        model.add_user_to_role(visitor, model.Role.ADMIN, roger)
 
+        # in new_authz you can't give a visitor permissions to a
+        # group it seems, so this is a bit meaningless
+        model.add_user_to_role(visitor, model.Role.ADMIN, roger)
         model.repo.commit_and_remove()
 
     # method used in DGU and all good tests elsewhere

ckan/model/group.py

@@ -1,6 +1,6 @@
 import datetime
 
-from sqlalchemy import orm, types, Column, Table, ForeignKey, or_
+from sqlalchemy import orm, types, Column, Table, ForeignKey, or_, and_
 import vdm.sqlalchemy
 
 import meta
@@ -64,8 +64,9 @@ class Member(vdm.sqlalchemy.RevisionedObjectMixin,
                    (see ckan.logic.action.package_owner_org_update)
     * User - the User is granted permissions for the Group
                  - capacity is 'admin', 'editor' or 'member'
-    * Group - the Group (Member.group_id) is a child of the Group (Member.id)
+    * Group - the Group (Member.group_id) is a parent of the Group (Member.id)
               in a hierarchy.
+                 - capacity is 'parent'
     '''
     def __init__(self, group=None, table_id=None, group_id=None,
                  table_name=None, capacity='public', state='active'):
@@ -182,8 +183,8 @@ def get_children_groups(self, type='group'):
         results = meta.Session.query(Group.id, Group.name, Group.title).\
                      filter_by(type=type).\
                      filter_by(state='active').\
-                     join(Member, Member.table_id == Group.id).\
-                     filter_by(group=self).\
+                     join(Member, Member.group_id == Group.id).\
+                     filter_by(table_id=self.id).\
                      filter_by(table_name='group').\
                      filter_by(state='active').\
                      all()
@@ -222,9 +223,10 @@ def get_top_level_groups(cls, type='group'):
         no parent groups. Groups are sorted by title.
         '''
         return meta.Session.query(cls).\
-            outerjoin(Member, Member.table_id == Group.id and
-                      Member.table_name == 'group' and
-                      Member.state == 'active').\
+            outerjoin(Member,
+                      and_(Member.group_id == Group.id,
+                           Member.table_name == 'group',
+                           Member.state == 'active')).\
             filter(Member.id == None).\
             filter(Group.type == type).\
             order_by(Group.title).all()
@@ -318,7 +320,10 @@ def add_package_by_name(self, package_name):
             meta.Session.add(member)
 
     def get_groups(self, group_type=None, capacity=None):
-        """ Get all groups that this group is within """
+        """ Get all groups that this group is within (i.e. this group's child
+        groups)
+
+        """
         import ckan.model as model
         # DR: Why is this cached? Surely the members can change in the
         #     lifetime of this Group?
@@ -392,29 +397,29 @@ def __repr__(self):
 (
     -- non-recursive term
     SELECT * FROM member
-    WHERE group_id = :id AND table_name = 'group' AND state = 'active'
+    WHERE table_id = :id AND table_name = 'group' AND state = 'active'
     UNION ALL
     -- recursive term
     SELECT m.* FROM member AS m, child AS c
-    WHERE m.group_id = c.table_id AND m.table_name = 'group'
+    WHERE m.table_id = c.group_id AND m.table_name = 'group'
           AND m.state = 'active'
 )
 SELECT G.*, child.group_id as parent_id FROM child
-    INNER JOIN public.group G ON G.id = child.table_id
+    INNER JOIN public.group G ON G.id = child.group_id
     WHERE G.type = :type AND G.state='active';"""
 
 HIERARCHY_UPWARDS_CTE = """WITH RECURSIVE parenttree(depth) AS (
     -- non-recursive term
     SELECT 0, M.* FROM public.member AS M
-    WHERE table_id = :id AND M.table_name = 'group' AND M.state = 'active'
+    WHERE group_id = :id AND M.table_name = 'group' AND M.state = 'active'
     UNION
     -- recursive term
     SELECT PG.depth + 1, M.* FROM parenttree PG, public.member M
-    WHERE PG.group_id = M.table_id AND M.table_name = 'group'
+    WHERE PG.table_id = M.group_id AND M.table_name = 'group'
           AND M.state = 'active'
     )
 
 SELECT G.*, PT.depth FROM parenttree AS PT
-    INNER JOIN public.group G ON G.id = PT.group_id
+    INNER JOIN public.group G ON G.id = PT.table_id
     WHERE G.type = :type AND G.state='active'
     ORDER BY PT.depth DESC;"""

ckan/new_authz.py

@@ -103,7 +103,7 @@ def clean_action_name(action_name):
 
 
 def is_sysadmin(username):
-    ''' returns True is username is a sysadmin '''
+    ''' returns True if username is a sysadmin '''
     if not username:
         return False
     # see if we can authorise without touching the database
@@ -212,8 +212,8 @@ def get_roles_with_permission(permission):
 
 
 def has_user_permission_for_group_or_org(group_id, user_name, permission):
-    ''' Check if the user has the given permissions for the group,
-    allowing for sysadmin rights and permission cascading down groups.
+    ''' Check if the user has the given permissions for the group, allowing for
+    sysadmin rights and permission cascading down a group hierarchy.
 
     '''
     if not group_id:
@@ -246,9 +246,11 @@ def has_user_permission_for_group_or_org(group_id, user_name, permission):
 def _has_user_permission_for_groups(user_id, permission, group_ids,
                                     capacity=None):
     ''' Check if the user has the given permissions for the particular
-    group. Can also be filtered by a particular capacity.
-
+    group (ignoring permissions cascading in a group hierarchy).
+    Can also be filtered by a particular capacity.
     '''
+    if not group_ids:
+        return False
     # get any roles the user has for the group
     q = model.Session.query(model.Member) \
         .filter(model.Member.group_id.in_(group_ids)) \
@@ -267,7 +269,10 @@ def _has_user_permission_for_groups(user_id, permission, group_ids,
 
 
 def users_role_for_group_or_org(group_id, user_name):
-    ''' Check if the user role for the group '''
+    ''' Returns the user's role for the group. (Ignores privileges that cascade
+    in a group hierarchy.)
+
+    '''
     if not group_id:
         return None
     group_id = model.Group.get(group_id).id
@@ -288,7 +293,7 @@ def users_role_for_group_or_org(group_id, user_name):
 
 
 def has_user_permission_for_some_org(user_name, permission):
-    ''' Check if the user has the given permission for the group '''
+    ''' Check if the user has the given permission for any organization. '''
     user_id = get_user_id_for_username(user_name, allow_none=True)
     if not user_id:
         return False

ckan/tests/functional/test_group.py

@@ -74,7 +74,7 @@ def test_children(self):
         rev = model.repo.new_revision()
         rev.author = "none"
 
-        member = model.Member(group_id=parent.id, table_id=group.id,
+        member = model.Member(group_id=group.id, table_id=parent.id,
                               table_name='group', capacity='member')
         model.Session.add(member)
         model.repo.commit_and_remove()

ckan/tests/logic/test_member.py

@@ -11,10 +11,18 @@ def setup_class(cls):
         model.repo.new_revision()
         create_test_data.CreateTestData.create()
         cls.user = model.User.get('testsysadmin')
+        cls.tester = model.User.get('tester')
         cls.group = model.Group.get('david')
+        cls.roger = model.Group.get('roger')
         cls.pkgs = [model.Package.by_name('warandpeace'),
                     model.Package.by_name('annakarenina')]
 
+        # 'Tester' becomes an admin for the 'roger' group
+        model.repo.new_revision()
+        model.Member(group=cls.roger, table_id=cls.tester.id,
+                     table_name='user', capacity='admin')
+        model.repo.commit_and_remove()
+
     @classmethod
     def teardown_class(cls):
         model.repo.rebuild_db()
@@ -37,6 +45,20 @@ def test_member_create_raises_if_user_unauthorized_to_update_group(self):
         assert_raises(logic.NotAuthorized,
                       logic.get_action('member_create'), ctx, dd)
 
+    def test_member_create_with_child_group_permission(self):
+        # 'tester' has admin priviledge for roger, so anyone can make it
+        # a child group.
+        self._member_create_group_hierarchy(parent_group=self.group,
+                                            child_group=self.roger,
+                                            user=self.tester)
+
+    def test_member_create_raises_when_only_have_parent_group_permission(self):
+        assert_raises(logic.NotAuthorized,
+                      self._member_create_group_hierarchy,
+                      self.roger,  # parent
+                      self.group,  # child
+                      self.tester)
+
     def test_member_create_accepts_group_name_or_id(self):
         by_name = self._member_create_in_group(self.pkgs[0].id, 'package',
                                                'public', self.group.name)
@@ -139,14 +161,18 @@ def test_member_delete_raises_if_object_type_is_invalid(self):
                       self._member_delete, 'obj_id', 'invalid_obj_type')
 
     def _member_create(self, obj, obj_type, capacity):
+        '''Makes the given object a member of cls.group.'''
         ctx, dd = self._build_context(obj, obj_type, capacity)
         return logic.get_action('member_create')(ctx, dd)
 
     def _member_create_in_group(self, obj, obj_type, capacity, group_id):
+        '''Makes the given object a member of the given group.'''
         ctx, dd = self._build_context(obj, obj_type, capacity, group_id)
         return logic.get_action('member_create')(ctx, dd)
 
     def _member_create_as_user(self, obj, obj_type, capacity, user):
+        '''Makes the griven object a member of cls.group using privileges of
+        the given user.'''
         ctx, dd = self._build_context(obj, obj_type, capacity, user=user)
         return logic.get_action('member_create')(ctx, dd)
 
@@ -162,6 +188,11 @@ def _member_delete_in_group(self, obj, obj_type, group_id):
         ctx, dd = self._build_context(obj, obj_type, group_id=group_id)
         return logic.get_action('member_delete')(ctx, dd)
 
+    def _member_create_group_hierarchy(self, parent_group, child_group, user):
+        ctx, dd = self._build_context(parent_group.name, 'group', 'parent',
+                                      group_id=child_group.name, user=user.id)
+        return logic.get_action('member_create')(ctx, dd)
+
     def _build_context(self, obj, obj_type, capacity='public',
                        group_id=None, user=None):
         ctx = {'model': model,