From af6c039c778a78f471cb6112175aae46dfda6729 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 12 Sep 2013 13:54:42 +0100 Subject: [PATCH] [#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 | 6 ++++-- ckan/model/group.py | 33 +++++++++++++++++------------ ckan/new_authz.py | 19 +++++++++++------ ckan/tests/functional/test_group.py | 2 +- ckan/tests/logic/test_member.py | 31 +++++++++++++++++++++++++++ 5 files changed, 67 insertions(+), 24 deletions(-) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 2ca59a42fcc..713e3b0f9ff 100644 --- a/ckan/lib/create_test_data.py +++ b/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 diff --git a/ckan/model/group.py b/ckan/model/group.py index ee637dde82f..b0c1c22fba5 100644 --- a/ckan/model/group.py +++ b/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;""" diff --git a/ckan/new_authz.py b/ckan/new_authz.py index 2f612c82b41..495f3806b17 100644 --- a/ckan/new_authz.py +++ b/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 diff --git a/ckan/tests/functional/test_group.py b/ckan/tests/functional/test_group.py index 5575bd6e4ba..fd47a6e6699 100644 --- a/ckan/tests/functional/test_group.py +++ b/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() diff --git a/ckan/tests/logic/test_member.py b/ckan/tests/logic/test_member.py index 39f67edd2db..f3d0ec448a0 100644 --- a/ckan/tests/logic/test_member.py +++ b/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,