From 35fc3ed37b600570b5a294cf9621c1400bd48bf1 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 25 May 2012 16:50:17 +0100 Subject: [PATCH 01/88] [xs] Helpful SOLR hint. --- doc/solr-setup.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/doc/solr-setup.rst b/doc/solr-setup.rst index 395da67e0a9..c7c2f53ee2d 100644 --- a/doc/solr-setup.rst +++ b/doc/solr-setup.rst @@ -192,6 +192,18 @@ Some problems that can be found during the install: ${dataDir} +* When running Solr it says `Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps JAVA_HOME does not point to the JDK.` + + See the note above about JAVA_HOME. Alternatively you may not have installed the JDK. Check by seeing if javac is installed:: + + which javac + + If it isn't do:: + + sudo apt-get install openjdk-6-jdk + + and restart SOLR. + Handling changes in the CKAN schema ----------------------------------- From 04ca4191673010e86ac0d77991cb7967fe6c58ae Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 15 Jun 2012 13:45:46 +0100 Subject: [PATCH 02/88] Fix minor bug that caused create_users to not commit changes. --- ckan/lib/create_test_data.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 53743e6c2cc..1b8d3b601a2 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -537,6 +537,7 @@ def _create_user_without_commit(cls, name='', **user_dict): user = model.User(name=unicode(name), **user_dict) model.Session.add(user) cls.user_refs.append(user_ref) + return user @classmethod def create_user(cls, name='', **kwargs): From 6780e91351915bf0a020c93cae67d597c406c348 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 15 Jun 2012 13:46:04 +0100 Subject: [PATCH 03/88] Improve logging in useful places. --- ckan/controllers/package.py | 2 ++ ckan/lib/authenticator.py | 6 ++++++ ckan/lib/base.py | 3 +++ ckan/lib/search/__init__.py | 13 +++++++------ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 9403a0271e3..a7df2cf1849 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -592,6 +592,8 @@ def _save_new(self, context, package_type=None): def _save_edit(self, name_or_id, context): from ckan.lib.search import SearchIndexError + log.debug('Package save request name: %s POST: %r', + name_or_id, request.POST) try: data_dict = clean_dict(unflatten( tuplize_dict(parse_params(request.POST)))) diff --git a/ckan/lib/authenticator.py b/ckan/lib/authenticator.py index b56711a3427..6f061caad91 100644 --- a/ckan/lib/authenticator.py +++ b/ckan/lib/authenticator.py @@ -1,8 +1,12 @@ +import logging + from zope.interface import implements from repoze.who.interfaces import IAuthenticator from ckan.model import User, Session +log = logging.getLogger(__name__) + class OpenIDAuthenticator(object): implements(IAuthenticator) @@ -25,8 +29,10 @@ def authenticate(self, environ, identity): return None user = User.by_name(identity.get('login')) if user is None: + log.debug('Login failed - username %r not found', identity.get('login')) return None if user.validate_password(identity.get('password')): return user.name + log.debug('Login as %r failed - password not valid', identity.get('login')) return None diff --git a/ckan/lib/base.py b/ckan/lib/base.py index f2cc447fdea..52604d48e7f 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -29,6 +29,8 @@ from ckan.lib.helpers import json import ckan.model as model +log = logging.getLogger(__name__) + PAGINATE_ITEMS_PER_PAGE = 50 APIKEY_HEADER_NAME_KEY = 'apikey_header_name' @@ -139,6 +141,7 @@ def render_template(): response.headers["Cache-Control"] = "private" # Prevent any further rendering from being cached. request.environ['__no_cache__'] = True + log.debug('Template cache-control: %s' % response.headers["Cache-Control"]) # Render Time :) try: diff --git a/ckan/lib/search/__init__.py b/ckan/lib/search/__init__.py index abc78b798fd..b68e7850c47 100644 --- a/ckan/lib/search/__init__.py +++ b/ckan/lib/search/__init__.py @@ -128,11 +128,11 @@ def rebuild(package_id=None,only_missing=False,force=False,refresh=False): If a dataset id is provided, only this dataset will be reindexed. When reindexing all datasets, if only_missing is True, only the datasets not already indexed will be processed. If force equals - True, if an execption is found, the exception will be logged, but + True, if an exception is found, the exception will be logged, but the process will carry on. ''' from ckan import model - log.debug("Rebuilding search index...") + log.info("Rebuilding search index...") package_index = index_for(model.Package) @@ -140,21 +140,22 @@ def rebuild(package_id=None,only_missing=False,force=False,refresh=False): pkg_dict = get_action('package_show')( {'model': model, 'ignore_auth': True, 'validate': False}, {'id': package_id}) + log.info('Indexing just package %r...', pkg_dict['name']) package_index.remove_dict(pkg_dict) package_index.insert_dict(pkg_dict) else: package_ids = [r[0] for r in model.Session.query(model.Package.id).filter(model.Package.state == 'active').all()] if only_missing: - log.debug('Indexing only missing packages...') + log.info('Indexing only missing packages...') package_query = query_for(model.Package) indexed_pkg_ids = set(package_query.get_all_entity_ids(max_results=len(package_ids))) package_ids = set(package_ids) - indexed_pkg_ids # Packages not indexed if len(package_ids) == 0: - log.debug('All datasets are already indexed') + log.info('All datasets are already indexed') return else: - log.debug('Rebuilding the whole index...') + log.info('Rebuilding the whole index...') # When refreshing, the index is not previously cleared if not refresh: package_index.clear() @@ -176,7 +177,7 @@ def rebuild(package_id=None,only_missing=False,force=False,refresh=False): raise model.Session.commit() - log.debug('Finished rebuilding search index.') + log.info('Finished rebuilding search index.') def check(): from ckan import model From 75ba31027650b1ff3af3c8dc16cc11663f515350 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 21 Jun 2012 11:22:36 +0100 Subject: [PATCH 04/88] Comment required, else Toby would just delete the method out of hand. --- ckan/lib/create_test_data.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 1b8d3b601a2..7b4a7815359 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -511,6 +511,7 @@ def create(cls, auth_profile="", package_type=None): model.repo.commit_and_remove() + # method used in DGU and all good tests elsewhere @classmethod def create_users(cls, user_dicts): needs_commit = False From 98888d2b5a46945a96b1d4719718c836a6a15e60 Mon Sep 17 00:00:00 2001 From: David Read Date: Mon, 22 Jul 2013 17:43:45 +0100 Subject: [PATCH 05/88] [#1038] Correct copyright sign for UTF8 file and cut/paste error. --- ckan/config/environment.py | 2 +- ckan/lib/plugins.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ckan/config/environment.py b/ckan/config/environment.py index 7a2dd27682d..fd8b46fa98d 100644 --- a/ckan/config/environment.py +++ b/ckan/config/environment.py @@ -237,7 +237,7 @@ def template_loaded(template): ''' This code is based on Genshi code - Copyright © 2006-2012 Edgewall Software + Copyright © 2006-2012 Edgewall Software All rights reserved. Redistribution and use in source and binary forms, with or diff --git a/ckan/lib/plugins.py b/ckan/lib/plugins.py index f60b928f434..1bf87ac29cb 100644 --- a/ckan/lib/plugins.py +++ b/ckan/lib/plugins.py @@ -104,7 +104,7 @@ def register_group_plugins(map): """ Register the various IGroupForm instances. - This method will setup the mappings between package types and the + This method will setup the mappings between group types and the registered IGroupForm instances. If it's called more than once an exception will be raised. """ @@ -149,7 +149,7 @@ def register_group_plugins(map): if group_type in _group_plugins: raise ValueError, "An existing IGroupForm is "\ - "already associated with the package type "\ + "already associated with the group type "\ "'%s'" % group_type _group_plugins[group_type] = plugin From 21cd0ac33023d13127a1e66094334de9d53b8b24 Mon Sep 17 00:00:00 2001 From: David Read Date: Mon, 22 Jul 2013 17:51:18 +0100 Subject: [PATCH 06/88] [#1038] Fix to allow sqlite testing - for until the tests are overhaulled. --- ckan/config/environment.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ckan/config/environment.py b/ckan/config/environment.py index fd8b46fa98d..49348789369 100644 --- a/ckan/config/environment.py +++ b/ckan/config/environment.py @@ -353,6 +353,8 @@ def genshi_lookup_attr(cls, obj, key): # Here we create the site user if they are not already in the database try: logic.get_action('get_site_user')({'ignore_auth': True}, None) - except sqlalchemy.exc.ProgrammingError: - # The database is not initialised. This is a bit dirty. + except (sqlalchemy.exc.ProgrammingError, sqlalchemy.exc.OperationalError): + # (ProgrammingError for Postgres, OperationalError for SQLite) + # The database is not initialised. This is a bit dirty. This occurs + # when running tests. pass From 96f0a3e775e14207cd1eec3b82f0e05a1f5a48ed Mon Sep 17 00:00:00 2001 From: David Read Date: Mon, 22 Jul 2013 17:52:48 +0100 Subject: [PATCH 07/88] [#1038] Test fixtures for organization hierarchy. --- ckan/lib/cli.py | 4 +- ckan/lib/create_test_data.py | 81 +++++++++++++++++++++++++++++++++--- 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/ckan/lib/cli.py b/ckan/lib/cli.py index bf4654e2e09..0acae3550bb 100644 --- a/ckan/lib/cli.py +++ b/ckan/lib/cli.py @@ -1273,7 +1273,7 @@ class CreateTestDataCommand(CkanCommand): translations of terms create-test-data vocabs - annakerenina, warandpeace, and some test vocabularies - + create-test-data hierarchy - hierarchy of groups ''' summary = __doc__.split('\n')[0] usage = __doc__ @@ -1309,6 +1309,8 @@ def command(self): CreateTestData.create_translations_test_data() elif cmd == 'vocabs': CreateTestData.create_vocabs_test_data() + elif cmd == 'hierarchy': + CreateTestData.create_group_hierarchy_test_data() else: print 'Command %s not recognized' % cmd raise NotImplementedError diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 3edf865c7c7..f464907d745 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -38,6 +38,12 @@ def create_family_test_data(cls, extra_users=[]): relationships=family_relationships, extra_user_names=extra_users) + @classmethod + def create_group_hierarchy_test_data(cls, extra_users=[]): + cls.create_groups(group_hierarchy_groups) + cls.create_arbitrary(group_hierarchy_datasets, + extra_user_names=group_hierarchy_users) + @classmethod def create_test_user(cls): tester = model.User.by_name(u'tester') @@ -315,10 +321,9 @@ def pkg(pkg_name): @classmethod def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): '''A more featured interface for creating groups. - All group fields can be filled, packages added and they can - have an admin user.''' + All group fields can be filled, packages added, can have + an admin user and be a member of other groups.''' rev = model.repo.new_revision() - # same name as user we create below rev.author = cls.author if admin_user_name: admin_users = [model.User.by_name(admin_user_name)] @@ -327,7 +332,7 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): assert isinstance(group_dicts, (list, tuple)) group_attributes = set(('name', 'title', 'description', 'parent_id')) for group_dict in group_dicts: - if model.Group.by_name(group_dict['name']): + if model.Group.by_name(unicode(group_dict['name'])): log.warning('Cannot create group "%s" as it already exists.' % \ (group_dict['name'])) continue @@ -346,7 +351,35 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): member = model.Member(group=group, table_id=pkg.id, table_name='package') model.Session.add(member) model.Session.add(group) - model.setup_default_user_roles(group, admin_users) + admins = [model.User.by_name(user_name) \ + for user_name in group_dict.get('admins', [])] \ + + admin_users + for admin in admins: + member = model.Member(group=group, table_id=user.id, + table_name='user', capacity='admin') + model.Session.add(member) + editors = [model.User.by_name(user_name) \ + for user_name in group_dict.get('editors', [])] + for editor in editors: + member = model.Member(group=group, table_id=user.id, + table_name='user', capacity='editor') + model.Session.add(member) + # Need to commit the current Group for two reasons: + # 1. It might have a parent, and the Member will need the Group.id + # value allocated on commit. + # 2. The next Group created may have this Group as a parent so + # creation of the Member needs to refer to this one. + model.Session.commit() + rev = model.repo.new_revision() + rev.author = cls.author + # add it to a parent's group + 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, + table_name='group', capacity='parent') + model.Session.add(member) + #model.setup_default_user_roles(group, admin_users) cls.group_names.add(group_dict['name']) model.repo.commit_and_remove() @@ -825,6 +858,44 @@ def make_some_vocab_tags(cls): } ] +group_hierarchy_groups = [ + {'name': 'department-of-health', + 'title': 'Department of Health', + 'contact-email': 'contact@doh.gov.uk'}, + {'name': 'food-standards-agency', + 'title': 'Food Standards Agency', + 'contact-email': 'contact@fsa.gov.uk', + 'parent': 'department-of-health'}, + {'name': 'national-health-service', + 'title': 'National Health Service', + 'contact-email': 'contact@nhs.gov.uk', + 'parent': 'department-of-health'}, + {'name': 'nhs-wirral-ccg', + 'title': 'NHS Wirral CCG', + 'contact-email': 'contact@wirral.nhs.gov.uk', + 'parent': 'national-health-service'}, + {'name': 'nhs-southwark-ccg', + 'title': 'NHS Southwark CCG', + 'contact-email': 'contact@southwark.nhs.gov.uk', + 'parent': 'national-health-service'}, + {'name': 'cabinet-office', + 'title': 'Cabinet Office', + 'contact-email': 'contact@cabinet-office.gov.uk'}, + ] + +group_hierarchy_datasets = [ + {'name': 'doh-spend', 'title': 'Department of Health Spend Data', + 'groups': ['department-of-health']}, + {'name': 'nhs-spend', 'title': 'NHS Spend Data', + 'groups': ['national-health-service']}, + {'name': 'wirral-spend', 'title': 'Wirral Spend Data', + 'groups': ['nhs-wirral-ccg']}, + {'name': 'southwark-spend', 'title': 'Southwark Spend Data', + 'groups': ['nhs-southwark-ccg']}, + ] + +group_hierarchy_users = ['nhsadmin', 'nhseditor', 'wirraladmin', 'wirraleditor'] + # Some test terms and translations. terms = ('A Novel By Tolstoy', 'Index of the novel', From fde50fa47123c714e8098e8f7dbf88085da67ddc Mon Sep 17 00:00:00 2001 From: David Read Date: Mon, 22 Jul 2013 17:54:56 +0100 Subject: [PATCH 08/88] [#1038] Model methods for organization hierarchy. --- ckan/model/group.py | 111 ++++++++++++++++++++++++++------ ckan/tests/models/test_group.py | 64 +++++++++++++++++- 2 files changed, 153 insertions(+), 22 deletions(-) diff --git a/ckan/model/group.py b/ckan/model/group.py index c16424affee..2cf748bb078 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -87,6 +87,17 @@ def related_packages(self): return meta.Session.query(_package.Package).filter_by( id=self.table_id).all() + def __unicode__(self): + # refer to objects by name, not ID, to help debugging + if self.table_name == 'package': + table_info = 'package=%s' % meta.Session.query(_package.Package).get(self.table_id).name + elif self.table_name == 'group': + table_info = 'group=%s' % meta.Session.query(Group).get(self.table_id).name + else: + table_info = 'table_name=%s table_id=%s' % (self.table_name, self.table_id) + return u'' % \ + (self.group.name if self.group else repr(self.group), + table_info, self.capacity, self.state) class Group(vdm.sqlalchemy.RevisionedObjectMixin, vdm.sqlalchemy.StatefulObjectMixin, @@ -145,17 +156,55 @@ def set_approval_status(self, status): pass def get_children_groups(self, type='group'): - # Returns a list of dicts where each dict contains "id", "name", - # and "title" When querying with a CTE specifying a model in the - # query parameter causes problems as it returns only the first - # level deep apparently not recursing any deeper than that. If - # we simplify and request only specific fields then if returns - # the full depth of the hierarchy. - results = meta.Session.query("id", "name", "title").\ - from_statement(HIERARCHY_CTE).params(id=self.id, type=type).all() - return [{"id":idf, "name": name, "title": title} - for idf, name, title in results] + '''Returns the groups one level underneath this group in the hierarchy. + Groups come in a list of dicts, each keyed by "id", "name" and "title". + ''' + # The original intention of this method was to provide the full depth of + # the tree, but the CTE was incorrect. This new query does what that old CTE + # actually did, but is now far simpler. + results = meta.Session.query(Group.id, Group.name, Group.title).\ + filter_by(type=type).\ + join(Member, Member.table_id == Group.id).\ + filter_by(group=self).\ + filter_by(table_name='group').\ + filter_by(state='active').\ + all() + + return [{'id':id_, 'name': name, 'title': title} + for id_, name, title in results] + + def get_children_group_hierarchy(self, type='group'): + '''Returns the groups in all levels underneath this group in the hierarchy. + + :rtype: a list of tuples, each one a Group and the ID its their parent + group. + + e.g. >>> dept-health.get_children_group_hierarchy() + [(, u'8a163ba7-5146-4325-90c8-fe53b25e28d0'), + (, u'06e6dbf5-d801-40a1-9dc0-6785340b2ab4'), + (, u'd2e25b41-720c-4ba7-bc8f-bb34b185b3dd')] + ''' + results = meta.Session.query(Group, 'parent_id').\ + from_statement(HIERARCHY_DOWNWARDS_CTE).params(id=self.id, type=type).all() + return results + def get_parent_group_hierarchy(self, type='group'): + '''Returns this group's parent, parent's parent, parent's parent's parent + etc.. Sorted with the top level parent first.''' + return meta.Session.query(Group).\ + from_statement(HIERARCHY_UPWARDS_CTE).params(id=self.id, type=type).all() + + @classmethod + def get_top_level_groups(cls, type='group'): + '''Returns a list of the groups (of the specified type) which have + no parent groups.''' + return meta.Session.query(cls).\ + outerjoin(Member, Member.table_id == Group.id and \ + Member.table_name == 'group' and \ + Member.state == 'active').\ + filter(Member.id==None).\ + filter(Group.type==type).\ + order_by(Group.title).all() def packages(self, with_private=False, limit=None, return_query=False, context=None): @@ -248,6 +297,8 @@ def add_package_by_name(self, package_name): def get_groups(self, group_type=None, capacity=None): """ Get all groups that this group is within """ import ckan.model as model + # DR: Why is this cached? Surely the members can change in the + # lifetime of this Group? if '_groups' not in self.__dict__: self._groups = meta.Session.query(model.Group).\ join(model.Member, model.Member.group_id == model.Group.id and @@ -314,15 +365,33 @@ def __repr__(self): MemberRevision.related_packages = lambda self: [self.continuity.package] -HIERARCHY_CTE = """WITH RECURSIVE subtree(id) AS ( - SELECT M.* FROM public.member AS M - WHERE M.table_name = 'group' AND M.state = 'active' - UNION - SELECT M.* FROM public.member M, subtree SG - WHERE M.table_id = SG.group_id AND M.table_name = 'group' - AND M.state = 'active') - SELECT G.* FROM subtree AS ST - INNER JOIN public.group G ON G.id = ST.table_id - WHERE group_id = :id AND G.type = :type and table_name='group' - and G.state='active'""" +HIERARCHY_DOWNWARDS_CTE = """WITH RECURSIVE child AS +( + -- non-recursive term + SELECT * FROM member + WHERE group_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' + 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 + WHERE G.type = :type AND G.state='active';""" + +HIERARCHY_UPWARDS_CTE = """WITH RECURSIVE parenttree(id) AS ( + -- non-recursive term + SELECT M.* FROM public.member AS M + WHERE table_id = :id AND M.table_name = 'group' AND M.state = 'active' + UNION + -- recursive term + SELECT M.* FROM public.member M + JOIN parenttree as PG ON PG.group_id = M.table_id + WHERE M.table_name = 'group' AND M.state = 'active' + ) + +SELECT G.* FROM parenttree AS PT + INNER JOIN public.group G ON G.id = PT.group_id + WHERE G.type = :type AND G.state='active';""" diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py index 88d1e63d9be..000c5c41450 100644 --- a/ckan/tests/models/test_group.py +++ b/ckan/tests/models/test_group.py @@ -1,4 +1,4 @@ -from nose.tools import assert_equal +from ckan.tests import assert_equal, assert_not_in, assert_in import ckan.model as model from ckan.tests import * @@ -92,6 +92,68 @@ def _search_results(self, query, is_org=False): results = model.Group.search_by_name_or_title(query,is_org=is_org) return set([group.name for group in results]) +name_set_from_dicts = lambda groups: set([group['name'] for group in groups]) +name_set_from_group_tuple = lambda tuples: set([t[0].name for t in tuples]) +name_set_from_groups = lambda groups: set([group.name for group in groups]) +names_from_groups = lambda groups: [group.name for group in groups] + +class TestHierarchy: + @classmethod + def setup_class(self): + CreateTestData.create_group_hierarchy_test_data() + + def test_get_children_groups(self): + res = model.Group.by_name(u'department-of-health').\ + get_children_groups() + # check groups + assert_equal(name_set_from_dicts(res), + set(('national-health-service', + 'food-standards-agency'))) + # check each group is expressed as a small dict + assert_equal(set(res[0].keys()), set(('id', 'name', 'title'))) + assert_in(res[0]['name'], ('national-health-service', 'food-standards-agency')) + assert_in(res[0]['title'], ('National Health Service', 'Food Standards Agency')) + + def test_get_children_group_hierarchy__from_top(self): + assert_equal(name_set_from_group_tuple(model.Group.by_name(u'department-of-health').\ + get_children_group_hierarchy()), + set(('national-health-service', 'food-standards-agency', + 'nhs-wirral-ccg', 'nhs-southwark-ccg'))) + # i.e. not cabinet-office + + def test_get_children_group_hierarchy__from_tier_two(self): + assert_equal(name_set_from_group_tuple(model.Group.by_name(u'national-health-service').\ + get_children_group_hierarchy()), + set(('nhs-wirral-ccg', + 'nhs-southwark-ccg'))) + # i.e. not department-of-health or food-standards-agency + + def test_get_children_group_hierarchy__from_bottom_tier(self): + assert_equal(name_set_from_group_tuple(model.Group.by_name(u'nhs-wirral-ccg').\ + get_children_group_hierarchy()), + set()) + + def test_get_parent_groups_up_hierarchy__from_top(self): + assert_equal(names_from_groups(model.Group.by_name(u'department-of-health').\ + get_parent_group_hierarchy()), + []) + + def test_get_parent_groups_up_hierarchy__from_tier_two(self): + assert_equal(names_from_groups(model.Group.by_name(u'national-health-service').\ + get_parent_group_hierarchy()), + ['department-of-health']) + + def test_get_parent_groups_up_hierarchy__from_tier_three(self): + assert_equal(names_from_groups(model.Group.by_name(u'nhs-wirral-ccg').\ + get_parent_group_hierarchy()), + ['department-of-health', + 'national-health-service']) + + def test_get_top_level_groups(self): + assert_equal(names_from_groups(model.Group.by_name(u'nhs-wirral-ccg').\ + get_top_level_groups()), + ['cabinet-office', 'department-of-health']) + class TestGroupRevisions: @classmethod def setup_class(self): From 2c0bbafc509fc340dbdaa50c73e5ca712dc6b57a Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 23 Jul 2013 17:21:08 +0100 Subject: [PATCH 09/88] [#1038] Test data is now organizations rather than groups. --- ckan/lib/create_test_data.py | 34 +++++++++++++++++++++++++-------- ckan/tests/models/test_group.py | 18 +++++++++-------- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index f464907d745..8b4d44707e1 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -231,8 +231,12 @@ def create_arbitrary(cls, package_dicts, relationships=[], # session has not yet been committed at this point. # Fetch from the new_groups dict instead. group = new_groups[group_name] - member = model.Member(group=group, table_id=pkg.id, table_name='package') + capacity = 'organization' if group.is_organization \ + else 'public' + member = model.Member(group=group, table_id=pkg.id, table_name='package', capacity=capacity) model.Session.add(member) + if group.is_organization: + pkg.owner_org = group.id elif attr == 'license': pkg.license_id = val elif attr == 'license_id': @@ -330,7 +334,8 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): else: admin_users = [] assert isinstance(group_dicts, (list, tuple)) - group_attributes = set(('name', 'title', 'description', 'parent_id')) + group_attributes = set(('name', 'title', 'description', 'parent_id', + 'type', 'is_organization')) for group_dict in group_dicts: if model.Group.by_name(unicode(group_dict['name'])): log.warning('Cannot create group "%s" as it already exists.' % \ @@ -861,26 +866,39 @@ def make_some_vocab_tags(cls): group_hierarchy_groups = [ {'name': 'department-of-health', 'title': 'Department of Health', - 'contact-email': 'contact@doh.gov.uk'}, + 'contact-email': 'contact@doh.gov.uk', + 'type': 'organization', + 'is_organization': True + }, {'name': 'food-standards-agency', 'title': 'Food Standards Agency', 'contact-email': 'contact@fsa.gov.uk', - 'parent': 'department-of-health'}, + 'parent': 'department-of-health', + 'type': 'organization', + 'is_organization': True}, {'name': 'national-health-service', 'title': 'National Health Service', 'contact-email': 'contact@nhs.gov.uk', - 'parent': 'department-of-health'}, + 'parent': 'department-of-health', + 'type': 'organization', + 'is_organization': True}, {'name': 'nhs-wirral-ccg', 'title': 'NHS Wirral CCG', 'contact-email': 'contact@wirral.nhs.gov.uk', - 'parent': 'national-health-service'}, + 'parent': 'national-health-service', + 'type': 'organization', + 'is_organization': True}, {'name': 'nhs-southwark-ccg', 'title': 'NHS Southwark CCG', 'contact-email': 'contact@southwark.nhs.gov.uk', - 'parent': 'national-health-service'}, + 'parent': 'national-health-service', + 'type': 'organization', + 'is_organization': True}, {'name': 'cabinet-office', 'title': 'Cabinet Office', - 'contact-email': 'contact@cabinet-office.gov.uk'}, + 'contact-email': 'contact@cabinet-office.gov.uk', + 'type': 'organization', + 'is_organization': True}, ] group_hierarchy_datasets = [ diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py index 000c5c41450..13cee1907dc 100644 --- a/ckan/tests/models/test_group.py +++ b/ckan/tests/models/test_group.py @@ -97,6 +97,8 @@ def _search_results(self, query, is_org=False): name_set_from_groups = lambda groups: set([group.name for group in groups]) names_from_groups = lambda groups: [group.name for group in groups] +group_type = 'organization' + class TestHierarchy: @classmethod def setup_class(self): @@ -104,7 +106,7 @@ def setup_class(self): def test_get_children_groups(self): res = model.Group.by_name(u'department-of-health').\ - get_children_groups() + get_children_groups(type=group_type) # check groups assert_equal(name_set_from_dicts(res), set(('national-health-service', @@ -116,42 +118,42 @@ def test_get_children_groups(self): def test_get_children_group_hierarchy__from_top(self): assert_equal(name_set_from_group_tuple(model.Group.by_name(u'department-of-health').\ - get_children_group_hierarchy()), + get_children_group_hierarchy(type=group_type)), set(('national-health-service', 'food-standards-agency', 'nhs-wirral-ccg', 'nhs-southwark-ccg'))) # i.e. not cabinet-office def test_get_children_group_hierarchy__from_tier_two(self): assert_equal(name_set_from_group_tuple(model.Group.by_name(u'national-health-service').\ - get_children_group_hierarchy()), + get_children_group_hierarchy(type=group_type)), set(('nhs-wirral-ccg', 'nhs-southwark-ccg'))) # i.e. not department-of-health or food-standards-agency def test_get_children_group_hierarchy__from_bottom_tier(self): assert_equal(name_set_from_group_tuple(model.Group.by_name(u'nhs-wirral-ccg').\ - get_children_group_hierarchy()), + get_children_group_hierarchy(type=group_type)), set()) def test_get_parent_groups_up_hierarchy__from_top(self): assert_equal(names_from_groups(model.Group.by_name(u'department-of-health').\ - get_parent_group_hierarchy()), + get_parent_group_hierarchy(type=group_type)), []) def test_get_parent_groups_up_hierarchy__from_tier_two(self): assert_equal(names_from_groups(model.Group.by_name(u'national-health-service').\ - get_parent_group_hierarchy()), + get_parent_group_hierarchy(type=group_type)), ['department-of-health']) def test_get_parent_groups_up_hierarchy__from_tier_three(self): assert_equal(names_from_groups(model.Group.by_name(u'nhs-wirral-ccg').\ - get_parent_group_hierarchy()), + get_parent_group_hierarchy(type=group_type)), ['department-of-health', 'national-health-service']) def test_get_top_level_groups(self): assert_equal(names_from_groups(model.Group.by_name(u'nhs-wirral-ccg').\ - get_top_level_groups()), + get_top_level_groups(type=group_type)), ['cabinet-office', 'department-of-health']) class TestGroupRevisions: From 1229ff24d506a57f56ca4d030ab9228ff1d647c9 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 23 Jul 2013 17:22:27 +0100 Subject: [PATCH 10/88] [#1038] Docs for Member and its capacities is most handy. Explained the sort ordering for some of the group functions. --- ckan/model/group.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/ckan/model/group.py b/ckan/model/group.py index 2cf748bb078..a7de2495233 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -54,6 +54,19 @@ class Member(vdm.sqlalchemy.RevisionedObjectMixin, vdm.sqlalchemy.StatefulObjectMixin, domain_object.DomainObject): + '''A Member object represents any other object being a 'member' of a + particular Group. + + Meanings: + * Package - the Group is a collection of Packages + - capacity is 'public', 'private' + or 'organization' if the Group is an Organization + (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) + in a hierarchy. + ''' def __init__(self, group=None, table_id=None, group_id=None, table_name=None, capacity='public', state='active'): self.group = group @@ -175,6 +188,7 @@ def get_children_groups(self, type='group'): def get_children_group_hierarchy(self, type='group'): '''Returns the groups in all levels underneath this group in the hierarchy. + The ordering is such that children always come after their parent. :rtype: a list of tuples, each one a Group and the ID its their parent group. @@ -197,7 +211,8 @@ def get_parent_group_hierarchy(self, type='group'): @classmethod def get_top_level_groups(cls, type='group'): '''Returns a list of the groups (of the specified type) which have - no parent groups.''' + 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 \ From 68bfd7aefc272bcd94cff226ce97067ec44a91a4 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 30 Jul 2013 16:18:07 +0100 Subject: [PATCH 11/88] [#1038] Permission cascading code with tests. --- ckan/lib/create_test_data.py | 24 ++-- ckan/logic/auth/update.py | 2 +- ckan/logic/validators.py | 3 +- ckan/new_authz.py | 34 ++++- ckan/tests/logic/test_auth.py | 235 ++++++++++++++++++++++++++++++++-- 5 files changed, 275 insertions(+), 23 deletions(-) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 8b4d44707e1..80f32a1038d 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -40,9 +40,9 @@ def create_family_test_data(cls, extra_users=[]): @classmethod def create_group_hierarchy_test_data(cls, extra_users=[]): + cls.create_users(group_hierarchy_users) cls.create_groups(group_hierarchy_groups) - cls.create_arbitrary(group_hierarchy_datasets, - extra_user_names=group_hierarchy_users) + cls.create_arbitrary(group_hierarchy_datasets) @classmethod def create_test_user(cls): @@ -347,7 +347,7 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): for key in group_dict: if key in group_attributes: setattr(group, key, group_dict[key]) - else: + elif key not in ('admins', 'editors', 'parent'): group.extras[key] = group_dict[key] assert isinstance(pkg_names, (list, tuple)) for pkg_name in pkg_names: @@ -360,13 +360,13 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): for user_name in group_dict.get('admins', [])] \ + admin_users for admin in admins: - member = model.Member(group=group, table_id=user.id, + member = model.Member(group=group, table_id=admin.id, table_name='user', capacity='admin') model.Session.add(member) editors = [model.User.by_name(user_name) \ for user_name in group_dict.get('editors', [])] for editor in editors: - member = model.Member(group=group, table_id=user.id, + member = model.Member(group=group, table_id=editor.id, table_name='user', capacity='editor') model.Session.add(member) # Need to commit the current Group for two reasons: @@ -881,13 +881,17 @@ def make_some_vocab_tags(cls): 'contact-email': 'contact@nhs.gov.uk', 'parent': 'department-of-health', 'type': 'organization', - 'is_organization': True}, + 'is_organization': True, + 'editors': ['nhseditor'], + 'admins': ['nhsadmin']}, {'name': 'nhs-wirral-ccg', 'title': 'NHS Wirral CCG', 'contact-email': 'contact@wirral.nhs.gov.uk', 'parent': 'national-health-service', 'type': 'organization', - 'is_organization': True}, + 'is_organization': True, + 'editors': ['wirraleditor'], + 'admins': ['wirraladmin']}, {'name': 'nhs-southwark-ccg', 'title': 'NHS Southwark CCG', 'contact-email': 'contact@southwark.nhs.gov.uk', @@ -912,7 +916,11 @@ def make_some_vocab_tags(cls): 'groups': ['nhs-southwark-ccg']}, ] -group_hierarchy_users = ['nhsadmin', 'nhseditor', 'wirraladmin', 'wirraleditor'] +group_hierarchy_users = [{'name': 'nhsadmin', 'password': 'pass'}, + {'name': 'nhseditor', 'password': 'pass'}, + {'name': 'wirraladmin', 'password': 'pass'}, + {'name': 'wirraleditor', 'password': 'pass'}, + ] # Some test terms and translations. terms = ('A Novel By Tolstoy', diff --git a/ckan/logic/auth/update.py b/ckan/logic/auth/update.py index ac907f487d0..7376f27afb9 100644 --- a/ckan/logic/auth/update.py +++ b/ckan/logic/auth/update.py @@ -17,7 +17,7 @@ def package_update(context, data_dict): if package.owner_org: # if there is an owner org then we must have update_dataset - # premission for that organization + # permission for that organization check1 = new_authz.has_user_permission_for_group_or_org( package.owner_org, user, 'update_dataset' ) diff --git a/ckan/logic/validators.py b/ckan/logic/validators.py index f9de157795c..0fb3887798e 100644 --- a/ckan/logic/validators.py +++ b/ckan/logic/validators.py @@ -36,7 +36,8 @@ def owner_org_validator(key, data, errors, context): group_id = group.id user = context['user'] user = model.User.get(user) - if not(user.sysadmin or user.is_in_group(group_id)): + if not(user.sysadmin or new_authz.has_user_permission_for_group_or_org( + group_id, user.name, 'create_dataset')): raise Invalid(_('You cannot add a dataset to this organization')) data[key] = group_id diff --git a/ckan/new_authz.py b/ckan/new_authz.py index 1d391c50900..3296e795cf8 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -128,10 +128,14 @@ 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 permission for the group ''' + ''' Check if the user has the given permissions for the group, + allowing for sysadmin rights and permission cascading down groups. ''' if not group_id: return False - group_id = model.Group.get(group_id).id + group = model.Group.get(group_id) + if not group: + return False + group_id = group.id # Sys admins can do anything if is_sysadmin(user_name): @@ -140,12 +144,29 @@ def has_user_permission_for_group_or_org(group_id, user_name, permission): user_id = get_user_id_for_username(user_name, allow_none=True) if not user_id: return False + if _has_user_permission_for_groups(user_id, permission, [group_id]): + return True + # Handle when permissions cascade. Check the user's roles on groups higher + # in the group hierarchy for permission. + for capacity in check_config_permission('roles_that_cascade_to_sub_groups'): + parent_groups = group.get_parent_group_hierarchy(type=group.type) + group_ids = [group.id for group in parent_groups] + if _has_user_permission_for_groups(user_id, permission, group_ids, + capacity=capacity): + return True + return False + +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''' # get any roles the user has for the group q = model.Session.query(model.Member) \ - .filter(model.Member.group_id == group_id) \ + .filter(model.Member.group_id.in_(group_ids)) \ .filter(model.Member.table_name == 'user') \ .filter(model.Member.state == 'active') \ .filter(model.Member.table_id == user_id) + if capacity: + q = q.filter(model.Member.capacity == capacity) # see if any role has the required permission # admin permission allows anything for the group for row in q.all(): @@ -281,18 +302,21 @@ def _get_auth_function(action): 'user_delete_groups': True, 'user_delete_organizations': True, 'create_user_via_api': False, + 'roles_that_cascade_to_sub_groups': ['admin'], } CONFIG_PERMISSIONS = {} def check_config_permission(permission): - ''' Returns the permission True/False based on config ''' + ''' Returns the permission configuration, usually True/False ''' # set up perms if not already done if not CONFIG_PERMISSIONS: for perm in CONFIG_PERMISSIONS_DEFAULTS: key = 'ckan.auth.' + perm default = CONFIG_PERMISSIONS_DEFAULTS[perm] - CONFIG_PERMISSIONS[perm] = asbool(config.get(key, default)) + CONFIG_PERMISSIONS[perm] = config.get(key, default) + if isinstance(perm, bool): + CONFIG_PERMISSIONS[perm] = asbool(CONFIG_PERMISSIONS[perm]) if permission in CONFIG_PERMISSIONS: return CONFIG_PERMISSIONS[permission] return False diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index 3ea9f2db39f..fcac3c1707b 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -1,7 +1,10 @@ +import paste.fixture + import ckan.tests as tests from ckan.logic import get_action import ckan.model as model import ckan.new_authz as new_authz +from ckan.lib.create_test_data import CreateTestData, group_hierarchy_groups import json INITIAL_TEST_CONFIG_PERMISSIONS = { @@ -13,6 +16,7 @@ 'user_delete_organizations': False, 'create_user_via_api': False, 'create_unowned_dataset': False, + 'roles_that_cascade_to_sub_groups': ['admin'], } @@ -33,19 +37,26 @@ def teardown_class(cls): new_authz.CONFIG_PERMISSIONS.update(cls.old_perm) model.repo.rebuild_db() - def _call_api(self, action, data, user, status=None): + @classmethod + def _call_api(cls, action, data, user, status=None): params = '%s=1' % json.dumps(data) - return self.app.post('/api/action/%s' % action, - params=params, - extra_environ={'Authorization': self.apikeys[user]}, - status=status) + res = cls.app.post('/api/action/%s' % action, + params=params, + extra_environ={'Authorization': cls.apikeys[user]}, + status=[200, 403, 409]) + if res.status != (status or 200): + error = json.loads(res.body)['error'] + raise AssertionError('Status was %s but should be %s. Error: %s' % + (res.status, status, error)) + return res - def create_user(self, name): + @classmethod + def create_user(cls, name): user = {'name': name, 'password': 'pass', 'email': 'moo@moo.com'} - res = self._call_api('user_create', user, 'sysadmin', 200) - self.apikeys[name] = str(json.loads(res.body)['result']['apikey']) + res = cls._call_api('user_create', user, 'sysadmin', 200) + cls.apikeys[name] = str(json.loads(res.body)['result']['apikey']) class TestAuthOrgs(TestAuth): @@ -190,6 +201,214 @@ def test_11_delete_org(self): self._call_api('organization_delete', org, 'org_editor', 403) self._call_api('organization_delete', org, 'org_admin', 403) +ORG_HIERARCHY_PERMISSIONS = { + 'roles_that_cascade_to_sub_groups': ['admin'], + } + +class TestAuthOrgHierarchy(TestAuth): + # Tests are in the same vein as TestAuthOrgs, testing the cases where the + # group hierarchy provices extra permissions through cascading + + @classmethod + def setup_class(cls): + TestAuth.setup_class() + CreateTestData.create_group_hierarchy_test_data() + for user in model.Session.query(model.User): + cls.apikeys[user.name] = str(user.apikey) + new_authz.CONFIG_PERMISSIONS.update(ORG_HIERARCHY_PERMISSIONS) + CreateTestData.create_arbitrary( + package_dicts= [{'name': 'adataset', + 'groups': ['national-health-service']}], + extra_user_names=['john']) + + def _reset_adatasets_owner_org(self): + rev = model.repo.new_revision() + get_action('package_owner_org_update')( + {'model': model, 'ignore_auth': True}, + {'id': 'adataset', + 'organization_id': 'national-health-service'}) + + def _undelete_package_if_needed(self, package_name): + pkg = model.Package.by_name(package_name) + if pkg and pkg.state == 'deleted': + rev = model.repo.new_revision() + pkg.state = 'active' + model.repo.commit_and_remove() + + def test_05_add_users_to_org_1(self): + member = {'username': 'john', 'role': 'admin', + 'id': 'department-of-health'} + self._call_api('organization_member_create', member, 'nhsadmin', 403) + def test_05_add_users_to_org_2(self): + member = {'username': 'john', 'role': 'editor', + 'id': 'department-of-health'} + self._call_api('organization_member_create', member, 'nhsadmin', 403) + def test_05_add_users_to_org_3(self): + member = {'username': 'john', 'role': 'admin', + 'id': 'national-health-service'} + self._call_api('organization_member_create', member, 'nhsadmin', 200) + def test_05_add_users_to_org_4(self): + member = {'username': 'john', 'role': 'editor', + 'id': 'national-health-service'} + self._call_api('organization_member_create', member, 'nhsadmin', 200) + def test_05_add_users_to_org_5(self): + member = {'username': 'john', 'role': 'admin', + 'id': 'nhs-wirral-ccg'} + self._call_api('organization_member_create', member, 'nhsadmin', 200) + def test_05_add_users_to_org_6(self): + member = {'username': 'john', 'role': 'editor', + 'id': 'nhs-wirral-ccg'} + self._call_api('organization_member_create', member, 'nhsadmin', 200) + def test_05_add_users_to_org_7(self): + member = {'username': 'john', 'role': 'editor', + 'id': 'national-health-service'} + self._call_api('organization_member_create', member, 'nhseditor', 403) + + def test_07_add_datasets_1(self): + dataset = {'name': 't1', 'owner_org': 'department-of-health'} + self._call_api('package_create', dataset, 'nhsadmin', 403) + + def test_07_add_datasets_2(self): + dataset = {'name': 't2', 'owner_org': 'national-health-service'} + self._call_api('package_create', dataset, 'nhsadmin', 200) + + def test_07_add_datasets_3(self): + dataset = {'name': 't3', 'owner_org': 'nhs-wirral-ccg'} + self._call_api('package_create', dataset, 'nhsadmin', 200) + + def test_07_add_datasets_4(self): + dataset = {'name': 't4', 'owner_org': 'department-of-health'} + self._call_api('package_create', dataset, 'nhseditor', 403) + + def test_07_add_datasets_5(self): + dataset = {'name': 't5', 'owner_org': 'national-health-service'} + self._call_api('package_create', dataset, 'nhseditor', 200) + + def test_07_add_datasets_6(self): + dataset = {'name': 't6', 'owner_org': 'nhs-wirral-ccg'} + self._call_api('package_create', dataset, 'nhseditor', 403) + + def test_08_update_datasets_1(self): + dataset = {'name': 'adataset', 'owner_org': 'department-of-health'} + self._call_api('package_update', dataset, 'nhsadmin', 409) + + def test_08_update_datasets_2(self): + dataset = {'name': 'adataset', 'owner_org': 'national-health-service'} + self._call_api('package_update', dataset, 'nhsadmin', 200) + + def test_08_update_datasets_3(self): + dataset = {'name': 'adataset', 'owner_org': 'nhs-wirral-ccg'} + try: + self._call_api('package_update', dataset, 'nhsadmin', 200) + finally: + self._reset_adatasets_owner_org() + + def test_08_update_datasets_4(self): + dataset = {'name': 'adataset', 'owner_org': 'department-of-health'} + self._call_api('package_update', dataset, 'nhseditor', 409) + + def test_08_update_datasets_5(self): + dataset = {'name': 'adataset', 'owner_org': 'national-health-service'} + try: + self._call_api('package_update', dataset, 'nhseditor', 200) + finally: + self._reset_adatasets_owner_org() + + def test_08_update_datasets_6(self): + dataset = {'name': 'adataset', 'owner_org': 'nhs-wirral-ccg'} + self._call_api('package_update', dataset, 'nhseditor', 409) + + def test_09_delete_datasets_1(self): + dataset = {'id': 'doh-spend'} + try: + self._call_api('package_delete', dataset, 'nhsadmin', 403) + finally: + self._undelete_package_if_needed(dataset['id']) + + def test_09_delete_datasets_2(self): + dataset = {'id': 'nhs-spend'} + try: + self._call_api('package_delete', dataset, 'nhsadmin', 200) + finally: + self._undelete_package_if_needed(dataset['id']) + + def test_09_delete_datasets_3(self): + dataset = {'id': 'wirral-spend'} + try: + self._call_api('package_delete', dataset, 'nhsadmin', 200) + finally: + self._undelete_package_if_needed(dataset['id']) + + def test_09_delete_datasets_4(self): + dataset = {'id': 'nhs-spend'} + try: + self._call_api('package_delete', dataset, 'nhseditor', 200) + finally: + self._undelete_package_if_needed(dataset['id']) + + def test_09_delete_datasets_5(self): + dataset = {'id': 'wirral-spend'} + try: + self._call_api('package_delete', dataset, 'nhseditor', 403) + finally: + self._undelete_package_if_needed(dataset['id']) + + def _flesh_out_organization(self, org): + # When calling organization_update, unless you include the list of + # editor and admin users and parent groups, it will remove them. So + # get the current list + existing_org = get_action('organization_show')( + {'model': model, 'ignore_auth': True}, {'id': org['id']}) + org.update(existing_org) + + def test_10_edit_org_1(self): + org = {'id': 'department-of-health', 'title': 'test'} + self._flesh_out_organization(org) + self._call_api('organization_update', org, 'nhsadmin', 403) + + def test_10_edit_org_2(self): + org = {'id': 'national-health-service', 'title': 'test'} + self._flesh_out_organization(org) + import pprint; pprint.pprint(org) + print model.Session.query(model.Member).filter_by(state='deleted').all() + self._call_api('organization_update', org, 'nhsadmin', 200) + print model.Session.query(model.Member).filter_by(state='deleted').all() + + def test_10_edit_org_3(self): + org = {'id': 'nhs-wirral-ccg', 'title': 'test'} + self._flesh_out_organization(org) + self._call_api('organization_update', org, 'nhsadmin', 200) + + def test_10_edit_org_4(self): + org = {'id': 'department-of-health', 'title': 'test'} + self._flesh_out_organization(org) + self._call_api('organization_update', org, 'nhseditor', 403) + + def test_10_edit_org_5(self): + org = {'id': 'national-health-service', 'title': 'test'} + self._flesh_out_organization(org) + self._call_api('organization_update', org, 'nhseditor', 403) + + def test_10_edit_org_6(self): + org = {'id': 'nhs-wirral-ccg', 'title': 'test'} + self._flesh_out_organization(org) + self._call_api('organization_update', org, 'nhseditor', 403) + + def test_11_delete_org_1(self): + org = {'id': 'department-of-health'} + self._call_api('organization_delete', org, 'nhsadmin', 403) + self._call_api('organization_delete', org, 'nhseditor', 403) + + def test_11_delete_org_2(self): + org = {'id': 'national-health-service'} + self._call_api('organization_delete', org, 'nhsadmin', 403) + self._call_api('organization_delete', org, 'nhseditor', 403) + + def test_11_delete_org_3(self): + org = {'id': 'nhs-wirral-ccg'} + self._call_api('organization_delete', org, 'nhsadmin', 403) + self._call_api('organization_delete', org, 'nhseditor', 403) + class TestAuthGroups(TestAuth): From 4db2d24908fc382a7548fbbcc7dc5133dcdd29ad Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 30 Jul 2013 16:44:59 +0100 Subject: [PATCH 12/88] [#1038] Fix permission checking for organizations. Corrected bad test. --- ckan/logic/auth/create.py | 2 +- ckan/tests/logic/test_auth.py | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/ckan/logic/auth/create.py b/ckan/logic/auth/create.py index bf9c3d17ea3..299308a9ae5 100644 --- a/ckan/logic/auth/create.py +++ b/ckan/logic/auth/create.py @@ -23,7 +23,7 @@ def package_create(context, data_dict=None): # If an organization is given are we able to add a dataset to it? data_dict = data_dict or {} - org_id = data_dict.get('organization_id') + org_id = data_dict.get('owner_org') if org_id and not new_authz.has_user_permission_for_group_or_org( org_id, user, 'create_dataset'): return {'success': False, 'msg': _('User %s not authorized to add dataset to this organization') % user} diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index fcac3c1707b..2dc6b5e9a5a 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -60,6 +60,8 @@ def create_user(cls, name): class TestAuthOrgs(TestAuth): + # NB: These tests are dependent on each other, so don't run them + # separately. def test_01_create_users(self): # actual roles assigned later @@ -90,6 +92,7 @@ def test_02_create_orgs(self): def test_03_create_dataset_no_org(self): + # no owner_org supplied dataset = {'name': 'admin_create_no_org'} self._call_api('package_create', dataset, 'sysadmin', 409) @@ -106,7 +109,7 @@ def test_04_create_dataset_with_org(self): 'owner_org': 'org_no_user'} self._call_api('package_create', dataset, 'sysadmin', 200) - dataset = {'name': 'user_create_with_org', + dataset = {'name': 'user_create_with_no_org', 'owner_org': 'org_with_user'} self._call_api('package_create', dataset, 'no_org', 403) @@ -138,7 +141,7 @@ def _add_datasets(self, user): #not able to add dataset to org admin does not belong to. dataset = {'name': user + '_dataset_bad', 'owner_org': 'org_no_user'} - self._call_api('package_create', dataset, user, 409) + self._call_api('package_create', dataset, user, 403) #admin not able to make dataset not owned by a org dataset = {'name': user + '_dataset_bad'} @@ -146,7 +149,7 @@ def _add_datasets(self, user): #not able to add org to not existant org dataset = {'name': user + '_dataset_bad', 'owner_org': 'org_not_exist'} - self._call_api('package_create', dataset, user, 409) + self._call_api('package_create', dataset, user, 403) def test_07_add_datasets(self): self._add_datasets('org_admin') @@ -317,7 +320,7 @@ def test_08_update_datasets_5(self): def test_08_update_datasets_6(self): dataset = {'name': 'adataset', 'owner_org': 'nhs-wirral-ccg'} self._call_api('package_update', dataset, 'nhseditor', 409) - + def test_09_delete_datasets_1(self): dataset = {'id': 'doh-spend'} try: From 6d97199ca314f5130f5318d23fc6a2f04e6d7ab5 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 30 Jul 2013 16:45:52 +0100 Subject: [PATCH 13/88] [#1038] Fix unreliable ordering of upward CTE. --- ckan/model/group.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ckan/model/group.py b/ckan/model/group.py index a7de2495233..f21bafb3cb8 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -396,17 +396,17 @@ def __repr__(self): INNER JOIN public.group G ON G.id = child.table_id WHERE G.type = :type AND G.state='active';""" -HIERARCHY_UPWARDS_CTE = """WITH RECURSIVE parenttree(id) AS ( +HIERARCHY_UPWARDS_CTE = """WITH RECURSIVE parenttree(depth) AS ( -- non-recursive term - SELECT M.* FROM public.member AS M + SELECT 0, M.* FROM public.member AS M WHERE table_id = :id AND M.table_name = 'group' AND M.state = 'active' UNION -- recursive term - SELECT M.* FROM public.member M - JOIN parenttree as PG ON PG.group_id = M.table_id - WHERE M.table_name = 'group' AND M.state = 'active' + SELECT PG.depth + 1, M.* FROM parenttree PG, public.member M + WHERE PG.group_id = M.table_id AND M.table_name = 'group' AND M.state = 'active' ) -SELECT G.* FROM parenttree AS PT +SELECT G.*, PT.depth FROM parenttree AS PT INNER JOIN public.group G ON G.id = PT.group_id - WHERE G.type = :type AND G.state='active';""" + WHERE G.type = :type AND G.state='active' + ORDER BY PT.depth DESC;""" From 7ce1555ca3dc65adbb882a2373d4f99ce65fbf8c Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 6 Sep 2013 16:19:27 +0100 Subject: [PATCH 14/88] [#1038] Fix up authz config error and deleted groups * Fix - CONFIG_PERMISSIONS were not read properly following my prev changes * Fix deleted child groups being visible. * Fix up a couple of tests --- ckan/logic/auth/create.py | 7 +++++++ ckan/model/group.py | 1 + ckan/new_authz.py | 2 +- ckan/tests/functional/test_group.py | 18 +++++++++++++----- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/ckan/logic/auth/create.py b/ckan/logic/auth/create.py index ae98e257914..6061e4eb69b 100644 --- a/ckan/logic/auth/create.py +++ b/ckan/logic/auth/create.py @@ -114,6 +114,13 @@ def user_create(context, data_dict=None): def _check_group_auth(context, data_dict): + '''Has this user got update permission for all of the given groups? + If there is a package in the context then ignore that package's groups. + :returns: False if not allowed to update one (or more) of the given groups. + True otherwise. i.e. True is the default. A blank data_dict + mentions no groups, so it returns True. + + ''' # FIXME This code is shared amoung other logic.auth files and should be # somewhere better if not data_dict: diff --git a/ckan/model/group.py b/ckan/model/group.py index f21bafb3cb8..a25cebf9927 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -177,6 +177,7 @@ def get_children_groups(self, type='group'): # actually did, but is now far simpler. 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).\ filter_by(table_name='group').\ diff --git a/ckan/new_authz.py b/ckan/new_authz.py index a8f13063f60..370b19fb581 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -354,7 +354,7 @@ def check_config_permission(permission): key = 'ckan.auth.' + perm default = CONFIG_PERMISSIONS_DEFAULTS[perm] CONFIG_PERMISSIONS[perm] = config.get(key, default) - if isinstance(perm, bool): + if isinstance(default, bool): CONFIG_PERMISSIONS[perm] = asbool(CONFIG_PERMISSIONS[perm]) if permission in CONFIG_PERMISSIONS: return CONFIG_PERMISSIONS[permission] diff --git a/ckan/tests/functional/test_group.py b/ckan/tests/functional/test_group.py index b07bbe7ea6f..4f4fddc3e05 100644 --- a/ckan/tests/functional/test_group.py +++ b/ckan/tests/functional/test_group.py @@ -24,6 +24,10 @@ def setup_class(self): model.Session.remove() CreateTestData.create() + # reduce extraneous logging + from ckan.lib import activity_streams_session_extension + activity_streams_session_extension.logger.level = 100 + @classmethod def teardown_class(self): model.repo.rebuild_db() @@ -102,17 +106,21 @@ def test_children(self): def test_sorting(self): model.repo.rebuild_db() + testsysadmin = model.User(name=u'testsysadmin') + testsysadmin.sysadmin = True + model.Session.add(testsysadmin) + pkg1 = model.Package(name="pkg1") pkg2 = model.Package(name="pkg2") model.Session.add(pkg1) model.Session.add(pkg2) CreateTestData.create_groups([{'name': "alpha", 'packages': []}, - {'name': "beta", - 'packages': ["pkg1", "pkg2"]}, - {'name': "delta", - 'packages': ["pkg1"]}, - {'name': "gamma", 'packages': []}], + {'name': "beta", + 'packages': ["pkg1", "pkg2"]}, + {'name': "delta", + 'packages': ["pkg1"]}, + {'name': "gamma", 'packages': []}], admin_user_name='testsysadmin') context = {'model': model, 'session': model.Session, From f7f5049b0ab0a7220210722461bf56768d33df89 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 6 Sep 2013 16:42:30 +0000 Subject: [PATCH 15/88] [#1038] PEP8 fixes only. --- ckan/lib/create_test_data.py | 47 +++++++++++++++----------- ckan/logic/validators.py | 5 +-- ckan/model/group.py | 51 ++++++++++++++++------------- ckan/new_authz.py | 14 +++++--- ckan/tests/functional/test_group.py | 14 ++++---- ckan/tests/test_coding_standards.py | 1 - 6 files changed, 75 insertions(+), 57 deletions(-) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 80f32a1038d..2ca59a42fcc 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -221,19 +221,23 @@ def create_arbitrary(cls, package_dicts, relationships=[], group = model.Group.by_name(unicode(group_name)) if not group: if not group_name in new_groups: - group = model.Group(name=unicode(group_name)) + group = model.Group(name= + unicode(group_name)) model.Session.add(group) new_group_names.add(group_name) new_groups[group_name] = group else: - # If adding multiple packages with the same group name, - # model.Group.by_name will not find the group as the - # session has not yet been committed at this point. - # Fetch from the new_groups dict instead. + # If adding multiple packages with the same + # group name, model.Group.by_name will not + # find the group as the session has not yet + # been committed at this point. Fetch from + # the new_groups dict instead. group = new_groups[group_name] - capacity = 'organization' if group.is_organization \ + capacity = 'organization' if group.is_organization\ else 'public' - member = model.Member(group=group, table_id=pkg.id, table_name='package', capacity=capacity) + member = model.Member(group=group, table_id=pkg.id, + table_name='package', + capacity=capacity) model.Session.add(member) if group.is_organization: pkg.owner_org = group.id @@ -338,8 +342,8 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): 'type', 'is_organization')) for group_dict in group_dicts: if model.Group.by_name(unicode(group_dict['name'])): - log.warning('Cannot create group "%s" as it already exists.' % \ - (group_dict['name'])) + log.warning('Cannot create group "%s" as it already exists.' % + group_dict['name']) continue pkg_names = group_dict.pop('packages', []) group = model.Group(name=unicode(group_dict['name'])) @@ -353,18 +357,19 @@ def create_groups(cls, group_dicts, admin_user_name=None, auth_profile=""): for pkg_name in pkg_names: pkg = model.Package.by_name(unicode(pkg_name)) assert pkg, pkg_name - member = model.Member(group=group, table_id=pkg.id, table_name='package') + member = model.Member(group=group, table_id=pkg.id, + table_name='package') model.Session.add(member) model.Session.add(group) - admins = [model.User.by_name(user_name) \ - for user_name in group_dict.get('admins', [])] \ - + admin_users + admins = [model.User.by_name(user_name) + for user_name in group_dict.get('admins', [])] + \ + admin_users for admin in admins: member = model.Member(group=group, table_id=admin.id, table_name='user', capacity='admin') model.Session.add(member) - editors = [model.User.by_name(user_name) \ - for user_name in group_dict.get('editors', [])] + editors = [model.User.by_name(user_name) + for user_name in group_dict.get('editors', [])] for editor in editors: member = model.Member(group=group, table_id=editor.id, table_name='user', capacity='editor') @@ -400,7 +405,8 @@ def create(cls, auth_profile="", package_type=None): * Associated tags, etc etc ''' if auth_profile == "publisher": - organization_group = model.Group(name=u"organization_group", type="organization") + organization_group = model.Group(name=u"organization_group", + type="organization") cls.pkg_names = [u'annakarenina', u'warandpeace'] pkg1 = model.Package(name=cls.pkg_names[0], type=package_type) @@ -525,7 +531,6 @@ def create(cls, auth_profile="", package_type=None): model.repo.commit_and_remove() - # method used in DGU and all good tests elsewhere @classmethod def create_users(cls, user_dicts): @@ -539,9 +544,11 @@ def create_users(cls, user_dicts): @classmethod def _create_user_without_commit(cls, name='', **user_dict): - if model.User.by_name(name) or (user_dict.get('open_id') and model.User.by_openid(user_dict.get('openid'))): - log.warning('Cannot create user "%s" as it already exists.' % \ - (name or user_dict['name'])) + if model.User.by_name(name) or \ + (user_dict.get('open_id') and + model.User.by_openid(user_dict.get('openid'))): + log.warning('Cannot create user "%s" as it already exists.' % + name or user_dict['name']) return # User objects are not revisioned so no need to create a revision user_ref = name or user_dict['openid'] diff --git a/ckan/logic/validators.py b/ckan/logic/validators.py index 0fb3887798e..429fcb5a774 100644 --- a/ckan/logic/validators.py +++ b/ckan/logic/validators.py @@ -36,8 +36,9 @@ def owner_org_validator(key, data, errors, context): group_id = group.id user = context['user'] user = model.User.get(user) - if not(user.sysadmin or new_authz.has_user_permission_for_group_or_org( - group_id, user.name, 'create_dataset')): + if not(user.sysadmin or + new_authz.has_user_permission_for_group_or_org( + group_id, user.name, 'create_dataset')): raise Invalid(_('You cannot add a dataset to this organization')) data[key] = group_id diff --git a/ckan/model/group.py b/ckan/model/group.py index a25cebf9927..ee637dde82f 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -103,15 +103,19 @@ def related_packages(self): def __unicode__(self): # refer to objects by name, not ID, to help debugging if self.table_name == 'package': - table_info = 'package=%s' % meta.Session.query(_package.Package).get(self.table_id).name + table_info = 'package=%s' % meta.Session.query(_package.Package).\ + get(self.table_id).name elif self.table_name == 'group': - table_info = 'group=%s' % meta.Session.query(Group).get(self.table_id).name + table_info = 'group=%s' % meta.Session.query(Group).\ + get(self.table_id).name else: - table_info = 'table_name=%s table_id=%s' % (self.table_name, self.table_id) + table_info = 'table_name=%s table_id=%s' % (self.table_name, + self.table_id) return u'' % \ (self.group.name if self.group else repr(self.group), table_info, self.capacity, self.state) + class Group(vdm.sqlalchemy.RevisionedObjectMixin, vdm.sqlalchemy.StatefulObjectMixin, domain_object.DomainObject): @@ -172,10 +176,10 @@ def get_children_groups(self, type='group'): '''Returns the groups one level underneath this group in the hierarchy. Groups come in a list of dicts, each keyed by "id", "name" and "title". ''' - # The original intention of this method was to provide the full depth of - # the tree, but the CTE was incorrect. This new query does what that old CTE - # actually did, but is now far simpler. - results = meta.Session.query(Group.id, Group.name, Group.title).\ + # The original intention of this method was to provide the full depth + # of the tree, but the CTE was incorrect. This new query does what that + # old CTE actually did, but is now far simpler. + 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).\ @@ -184,12 +188,13 @@ def get_children_groups(self, type='group'): filter_by(state='active').\ all() - return [{'id':id_, 'name': name, 'title': title} + return [{'id': id_, 'name': name, 'title': title} for id_, name, title in results] def get_children_group_hierarchy(self, type='group'): - '''Returns the groups in all levels underneath this group in the hierarchy. - The ordering is such that children always come after their parent. + '''Returns the groups in all levels underneath this group in the + hierarchy. The ordering is such that children always come after their + parent. :rtype: a list of tuples, each one a Group and the ID its their parent group. @@ -200,14 +205,16 @@ def get_children_group_hierarchy(self, type='group'): (, u'd2e25b41-720c-4ba7-bc8f-bb34b185b3dd')] ''' results = meta.Session.query(Group, 'parent_id').\ - from_statement(HIERARCHY_DOWNWARDS_CTE).params(id=self.id, type=type).all() + from_statement(HIERARCHY_DOWNWARDS_CTE).\ + params(id=self.id, type=type).all() return results def get_parent_group_hierarchy(self, type='group'): - '''Returns this group's parent, parent's parent, parent's parent's parent - etc.. Sorted with the top level parent first.''' + '''Returns this group's parent, parent's parent, parent's parent's + parent etc.. Sorted with the top level parent first.''' return meta.Session.query(Group).\ - from_statement(HIERARCHY_UPWARDS_CTE).params(id=self.id, type=type).all() + from_statement(HIERARCHY_UPWARDS_CTE).\ + params(id=self.id, type=type).all() @classmethod def get_top_level_groups(cls, type='group'): @@ -215,12 +222,12 @@ 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').\ - filter(Member.id==None).\ - filter(Group.type==type).\ - order_by(Group.title).all() + outerjoin(Member, Member.table_id == Group.id and + Member.table_name == 'group' and + Member.state == 'active').\ + filter(Member.id == None).\ + filter(Group.type == type).\ + order_by(Group.title).all() def packages(self, with_private=False, limit=None, return_query=False, context=None): @@ -381,7 +388,6 @@ def __repr__(self): MemberRevision.related_packages = lambda self: [self.continuity.package] - HIERARCHY_DOWNWARDS_CTE = """WITH RECURSIVE child AS ( -- non-recursive term @@ -404,7 +410,8 @@ def __repr__(self): 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' AND M.state = 'active' + WHERE PG.group_id = M.table_id AND M.table_name = 'group' + AND M.state = 'active' ) SELECT G.*, PT.depth FROM parenttree AS PT diff --git a/ckan/new_authz.py b/ckan/new_authz.py index 370b19fb581..2f612c82b41 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -213,7 +213,9 @@ 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. ''' + allowing for sysadmin rights and permission cascading down groups. + + ''' if not group_id: return False group = model.Group.get(group_id) @@ -234,15 +236,19 @@ def has_user_permission_for_group_or_org(group_id, user_name, permission): # in the group hierarchy for permission. for capacity in check_config_permission('roles_that_cascade_to_sub_groups'): parent_groups = group.get_parent_group_hierarchy(type=group.type) - group_ids = [group.id for group in parent_groups] + group_ids = [group_.id for group_ in parent_groups] if _has_user_permission_for_groups(user_id, permission, group_ids, capacity=capacity): return True return False -def _has_user_permission_for_groups(user_id, permission, group_ids, capacity=None): + +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. Can also be filtered by a particular capacity. + + ''' # get any roles the user has for the group q = model.Session.query(model.Member) \ .filter(model.Member.group_id.in_(group_ids)) \ diff --git a/ckan/tests/functional/test_group.py b/ckan/tests/functional/test_group.py index 4f4fddc3e05..5575bd6e4ba 100644 --- a/ckan/tests/functional/test_group.py +++ b/ckan/tests/functional/test_group.py @@ -2,7 +2,6 @@ from nose.tools import assert_equal -import ckan.tests.test_plugins as test_plugins import ckan.model as model import ckan.lib.search as search @@ -15,7 +14,6 @@ from ckan.tests import is_search_supported - class TestGroup(FunctionalTestCase): @classmethod @@ -59,14 +57,14 @@ def test_atom_feed_page_negative(self): assert '"page" parameter must be a positive integer' in res, res def test_children(self): - if model.engine_is_sqlite() : + if model.engine_is_sqlite(): from nose import SkipTest raise SkipTest("Can't use CTE for sqlite") group_name = 'deletetest' CreateTestData.create_groups([{'name': group_name, 'packages': []}, - {'name': "parent_group", + {'name': "parent_group", 'packages': []}], admin_user_name='testsysadmin') @@ -155,7 +153,6 @@ def test_sorting(self): assert results[0]['name'] == u'beta', results[0]['name'] assert results[1]['name'] == u'delta', results[1]['name'] - def test_mainmenu(self): # the home page does a package search so have to skip this test if # search is not supported @@ -205,14 +202,16 @@ def test_read_and_authorized_to_edit(self): title = u'Dave\'s books' pkgname = u'warandpeace' offset = url_for(controller='group', action='read', id=name) - res = self.app.get(offset, extra_environ={'REMOTE_USER': 'testsysadmin'}) + res = self.app.get(offset, + extra_environ={'REMOTE_USER': 'testsysadmin'}) assert title in res, res assert 'edit' in res assert name in res def test_new_page(self): offset = url_for(controller='group', action='new') - res = self.app.get(offset, extra_environ={'REMOTE_USER': 'testsysadmin'}) + res = self.app.get(offset, + extra_environ={'REMOTE_USER': 'testsysadmin'}) assert 'Add A Group' in res, res @@ -265,7 +264,6 @@ def setup_class(self): model.Session.add(model.Package(name=self.packagename)) model.repo.commit_and_remove() - @classmethod def teardown_class(self): model.Session.remove() diff --git a/ckan/tests/test_coding_standards.py b/ckan/tests/test_coding_standards.py index f0b7469c3f4..4481342acd5 100644 --- a/ckan/tests/test_coding_standards.py +++ b/ckan/tests/test_coding_standards.py @@ -751,7 +751,6 @@ class TestPep8(object): 'ckan/tests/functional/test_cors.py', 'ckan/tests/functional/test_error.py', 'ckan/tests/functional/test_follow.py', - 'ckan/tests/functional/test_group.py', 'ckan/tests/functional/test_home.py', 'ckan/tests/functional/test_package.py', 'ckan/tests/functional/test_package_relationships.py', From af6c039c778a78f471cb6112175aae46dfda6729 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 12 Sep 2013 13:54:42 +0100 Subject: [PATCH 16/88] [#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, From 34b3fb7d882afbf024c2929dbba80e5385f2d7f6 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 12 Sep 2013 15:05:46 +0000 Subject: [PATCH 17/88] [noticket] Fix paster commands that fail due to The synchronous_search plugin is enabled automatically in ckan/plugins/core.py so a few paster commands failed trying to load it again. Looks like this was broken since 2371c05c 2013-04-05. Affected: create-test-data, dataset delete, dataset purge. --- ckan/lib/cli.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/ckan/lib/cli.py b/ckan/lib/cli.py index 0acae3550bb..a7e89d27991 100644 --- a/ckan/lib/cli.py +++ b/ckan/lib/cli.py @@ -844,7 +844,6 @@ def delete(self, dataset_ref): dataset = self._get_dataset(dataset_ref) old_state = dataset.state - plugins.load('synchronous_search') rev = model.repo.new_revision() dataset.delete() model.repo.commit_and_remove() @@ -857,7 +856,6 @@ def purge(self, dataset_ref): dataset = self._get_dataset(dataset_ref) name = dataset.name - plugins.load('synchronous_search') rev = model.repo.new_revision() dataset.purge() model.repo.commit_and_remove() @@ -1284,7 +1282,6 @@ def command(self): self._load_config() self._setup_app() from ckan import plugins - plugins.load('synchronous_search') # so packages get indexed from create_test_data import CreateTestData if self.args: From 527f3e9fba7831290be6f8ac8d67b78504a051b6 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 12 Sep 2013 15:08:25 +0000 Subject: [PATCH 18/88] [#1038] Fix display of tree hierarchy, broken in reversal af6c039. --- ckan/model/group.py | 2 +- ckan/tests/models/test_group.py | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/ckan/model/group.py b/ckan/model/group.py index b0c1c22fba5..fc3c5b17b30 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -404,7 +404,7 @@ def __repr__(self): 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 +SELECT G.*, child.table_id as parent_id FROM child INNER JOIN public.group G ON G.id = child.group_id WHERE G.type = :type AND G.state='active';""" diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py index 13cee1907dc..3c6973b8b40 100644 --- a/ckan/tests/models/test_group.py +++ b/ckan/tests/models/test_group.py @@ -116,6 +116,15 @@ def test_get_children_groups(self): assert_in(res[0]['name'], ('national-health-service', 'food-standards-agency')) assert_in(res[0]['title'], ('National Health Service', 'Food Standards Agency')) + def test_get_children_group_hierarchy__from_top_2(self): + groups = model.Group.by_name(u'department-of-health').\ + get_children_group_hierarchy(type=group_type) + # the first group must be NHS or Food Standards Agency - i.e. on the + # first level down + nhs = groups[0] + assert_in(nhs[0].name, ('national-health-service', 'food-standards-agency')) + assert_equal(model.Group.get(nhs[1]).name, 'department-of-health') + def test_get_children_group_hierarchy__from_top(self): assert_equal(name_set_from_group_tuple(model.Group.by_name(u'department-of-health').\ get_children_group_hierarchy(type=group_type)), From 8babc4f90f6f053c1c509476d183aa61f383ba8f Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 17 Sep 2013 10:32:19 +0000 Subject: [PATCH 19/88] [#1038] Setting of parent group in the form is now done via group['groups'] rather than custom 'parent' Removed explicit creation of Members in the create/update logic functions - these were a hangover. Instead, the form just needs to ensure the parent group gets added to the group['groups'] and it will get set in the model_save as normal. Group.get_group no longer used anywhere (and the caching looked ropey anyway) now that the previous bit is removed. Added schema validator to ensure that no loops form in the hierarchy. Downwards CTE needed ordering as found it also couldn't be relied upon to be in order it recursed, probably due to the join. Removed duplicate 'groups' key in the group schema. --- ckan/controllers/organization.py | 6 ++-- ckan/lib/dictization/model_save.py | 6 +++- ckan/logic/action/create.py | 10 +----- ckan/logic/action/update.py | 18 ---------- ckan/logic/schema.py | 8 ++--- ckan/logic/validators.py | 19 ++++++++++ ckan/model/group.py | 56 +++++++++++++----------------- ckan/tests/models/test_group.py | 9 +++++ 8 files changed, 65 insertions(+), 67 deletions(-) diff --git a/ckan/controllers/organization.py b/ckan/controllers/organization.py index b44d95e057f..fe2be826a8a 100644 --- a/ckan/controllers/organization.py +++ b/ckan/controllers/organization.py @@ -26,8 +26,10 @@ def _db_to_form_schema(self, group_type=None): into a format suitable for the form (optional)''' pass - def _setup_template_variables(self, context, data_dict, group_type=None): - pass + # This is commented so that the Group controller method runs instead, + # allowing a group plugins to setup template variables. + #def _setup_template_variables(self, context, data_dict, group_type=None): + # pass def _new_template(self, group_type): return 'organization/new.html' diff --git a/ckan/lib/dictization/model_save.py b/ckan/lib/dictization/model_save.py index 4122172b837..88bc106bac9 100644 --- a/ckan/lib/dictization/model_save.py +++ b/ckan/lib/dictization/model_save.py @@ -342,7 +342,11 @@ def group_member_save(context, group_dict, member_table_name): entities = {} Member = model.Member - ModelClass = getattr(model, member_table_name[:-1].capitalize()) + classname = member_table_name[:-1].capitalize() + if classname == 'Organization': + # Organizations use the model.Group class + classname = 'Group' + ModelClass = getattr(model, classname) for entity_dict in entity_list: name_or_id = entity_dict.get('id') or entity_dict.get('name') diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index fcb55ceae64..a3a062ce44a 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -15,6 +15,7 @@ import ckan.lib.dictization.model_dictize as model_dictize import ckan.lib.dictization.model_save as model_save import ckan.lib.navl.dictization_functions +import ckan.lib.navl.validators as validators from ckan.common import _ @@ -472,7 +473,6 @@ def _group_or_org_create(context, data_dict, is_org=False): model = context['model'] user = context['user'] session = context['session'] - parent = context.get('parent', None) data_dict['is_organization'] = is_org @@ -512,14 +512,6 @@ def _group_or_org_create(context, data_dict, is_org=False): group = model_save.group_dict_save(data, context) - if parent: - parent_group = model.Group.get( parent ) - if parent_group: - member = model.Member(group=parent_group, table_id=group.id, table_name='group') - session.add(member) - log.debug('Group %s is made child of group %s', - group.name, parent_group.name) - if user: admins = [model.User.by_name(user.decode('utf8'))] else: diff --git a/ckan/logic/action/update.py b/ckan/logic/action/update.py index c841a9bb25c..fcc8f9b5546 100644 --- a/ckan/logic/action/update.py +++ b/ckan/logic/action/update.py @@ -408,7 +408,6 @@ def _group_or_org_update(context, data_dict, is_org=False): user = context['user'] session = context['session'] id = _get_or_bust(data_dict, 'id') - parent = context.get('parent', None) group = model.Group.get(id) context["group"] = group @@ -464,23 +463,6 @@ def _group_or_org_update(context, data_dict, is_org=False): context['prevent_packages_update'] = True group = model_save.group_dict_save(data, context) - if parent: - parent_group = model.Group.get( parent ) - if parent_group and not parent_group in group.get_groups(group.type): - # Delete all of this groups memberships - current = session.query(model.Member).\ - filter(model.Member.table_id == group.id).\ - filter(model.Member.table_name == "group").all() - if current: - log.debug('Parents of group %s deleted: %r', group.name, - [membership.group.name for membership in current]) - for c in current: - session.delete(c) - member = model.Member(group=parent_group, table_id=group.id, table_name='group') - session.add(member) - log.debug('Group %s is made child of group %s', - group.name, parent_group.name) - if is_org: plugin_type = plugins.IOrganizationController else: diff --git a/ckan/logic/schema.py b/ckan/logic/schema.py index f8fe8df7fbd..66d2934c86e 100644 --- a/ckan/logic/schema.py +++ b/ckan/logic/schema.py @@ -50,6 +50,7 @@ url_validator, datasets_with_no_organization_cannot_be_private, list_of_strings, + no_loops_in_hierarchy, ) from ckan.logic.converters import (convert_user_name_or_id_to_id, convert_package_name_or_id_to_id, @@ -274,11 +275,6 @@ def default_group_schema(): "title":[ignore_missing, unicode], "name":[ignore_missing, unicode], "__extras": [ignore] - }, - 'groups': { - "name": [not_empty, unicode], - "capacity": [ignore_missing], - "__extras": [ignore] }, 'users': { "name": [not_empty, unicode], @@ -286,7 +282,7 @@ def default_group_schema(): "__extras": [ignore] }, 'groups': { - "name": [not_empty, unicode], + "name": [not_empty, no_loops_in_hierarchy, unicode], "capacity": [ignore_missing], "__extras": [ignore] } diff --git a/ckan/logic/validators.py b/ckan/logic/validators.py index 429fcb5a774..f9889297e3b 100644 --- a/ckan/logic/validators.py +++ b/ckan/logic/validators.py @@ -649,3 +649,22 @@ def list_of_strings(key, data, errors, context): if not isinstance(x, basestring): raise Invalid('%s: %s' % (_('Not a string'), x)) +def no_loops_in_hierarchy(key, data, errors, context): + '''Checks that the parent groups specified in the data would not cause + a loop in the group hierarchy, and therefore cause the recursion up/down + the hierarchy to get into an infinite loop. + ''' + if not 'id' in data: + # Must be a new group - has no children, so no chance of loops + return + group = context['model'].Group.get(data['id']) + allowable_parents = group.\ + groups_allowed_to_be_its_parent(type=group.type) + for parent in data['groups']: + parent_name = parent['name'] + # a blank name signifies top level, which is always allowed + if parent_name and context['model'].Group.get(parent_name) \ + not in allowable_parents: + raise Invalid(_('This parent would create a loop in the ' + 'hierarchy')) + diff --git a/ckan/model/group.py b/ckan/model/group.py index fc3c5b17b30..3c1b3369cac 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -193,11 +193,11 @@ def get_children_groups(self, type='group'): for id_, name, title in results] def get_children_group_hierarchy(self, type='group'): - '''Returns the groups in all levels underneath this group in the + '''Returns the groups in all levels underneath this group in the hierarchy. The ordering is such that children always come after their parent. - :rtype: a list of tuples, each one a Group and the ID its their parent + :rtype: a list of tuples, each one a Group and the ID of its parent group. e.g. >>> dept-health.get_children_group_hierarchy() @@ -211,7 +211,7 @@ def get_children_group_hierarchy(self, type='group'): return results def get_parent_group_hierarchy(self, type='group'): - '''Returns this group's parent, parent's parent, parent's parent's + '''Returns this group's parent, parent's parent, parent's parent's parent etc.. Sorted with the top level parent first.''' return meta.Session.query(Group).\ from_statement(HIERARCHY_UPWARDS_CTE).\ @@ -231,6 +231,22 @@ def get_top_level_groups(cls, type='group'): filter(Group.type == type).\ order_by(Group.title).all() + def groups_allowed_to_be_its_parent(self, type='group'): + '''Returns a list of the groups (of the specified type) which are + allowed to be this group's parent. It excludes ones which would + create a loop in the hierarchy, causing the recursive CTE to + be in an infinite loop. + + :returns: A list of group objects ordered by group title + + ''' + all_groups = self.all(group_type=type) + excluded_groups = set(group.name for group, id_ in + self.get_children_group_hierarchy(type=type)) + excluded_groups.add(self.name) + return [group for group in all_groups + if group.name not in excluded_groups] + def packages(self, with_private=False, limit=None, return_query=False, context=None): '''Return this group's active and pending packages. @@ -319,28 +335,6 @@ def add_package_by_name(self, package_name): table_name='package') meta.Session.add(member) - def get_groups(self, group_type=None, capacity=None): - """ 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? - if '_groups' not in self.__dict__: - self._groups = meta.Session.query(model.Group).\ - join(model.Member, model.Member.group_id == model.Group.id and - model.Member.table_name == 'group').\ - filter(model.Member.state == 'active').\ - filter(model.Member.table_id == self.id).all() - - groups = self._groups - if group_type: - groups = [g for g in groups if g.type == group_type] - if capacity: - groups = [g for g in groups if g.capacity == capacity] - return groups - @property def all_related_revisions(self): '''Returns chronological list of all object revisions related to @@ -368,7 +362,6 @@ def all_related_revisions(self): def __repr__(self): return '' % self.name - meta.mapper(Group, group_table, extension=[vdm.sqlalchemy.Revisioner(group_revision_table), ], ) @@ -393,20 +386,21 @@ def __repr__(self): MemberRevision.related_packages = lambda self: [self.continuity.package] -HIERARCHY_DOWNWARDS_CTE = """WITH RECURSIVE child AS +HIERARCHY_DOWNWARDS_CTE = """WITH RECURSIVE child(depth) AS ( -- non-recursive term - SELECT * FROM member + SELECT 0, * FROM member WHERE table_id = :id AND table_name = 'group' AND state = 'active' UNION ALL -- recursive term - SELECT m.* FROM member AS m, child AS c + SELECT c.depth + 1, m.* FROM member AS m, child AS c WHERE m.table_id = c.group_id AND m.table_name = 'group' AND m.state = 'active' ) -SELECT G.*, child.table_id as parent_id FROM child +SELECT G.*, child.depth, child.table_id as parent_id FROM child INNER JOIN public.group G ON G.id = child.group_id - WHERE G.type = :type AND G.state='active';""" + WHERE G.type = :type AND G.state='active' + ORDER BY child.depth ASC;""" HIERARCHY_UPWARDS_CTE = """WITH RECURSIVE parenttree(depth) AS ( -- non-recursive term diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py index 3c6973b8b40..8e12a0fe9b2 100644 --- a/ckan/tests/models/test_group.py +++ b/ckan/tests/models/test_group.py @@ -165,6 +165,15 @@ def test_get_top_level_groups(self): get_top_level_groups(type=group_type)), ['cabinet-office', 'department-of-health']) + def test_groups_allowed_to_be_its_parent(self): + groups = model.Group.by_name(u'national-health-service').\ + groups_allowed_to_be_its_parent(type=group_type) + names = names_from_groups(groups) + assert_in('department-of-health', names) + assert_in('cabinet-office', names) + assert_not_in('natonal-health-service', names) + assert_not_in('nhs-wirral-ccg', names) + class TestGroupRevisions: @classmethod def setup_class(self): From 407cf91a69b2bb8ae9e6763048aa6241b2001e8e Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 17 Sep 2013 14:09:59 +0000 Subject: [PATCH 20/88] [#1038] Fix group deletion mid-hierarchy. --- ckan/logic/action/delete.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ckan/logic/action/delete.py b/ckan/logic/action/delete.py index 933e7624b43..8803768db22 100644 --- a/ckan/logic/action/delete.py +++ b/ckan/logic/action/delete.py @@ -1,5 +1,7 @@ '''API functions for deleting data from CKAN.''' +from sqlalchemy import or_ + import ckan.logic import ckan.logic.action import ckan.plugins as plugins @@ -252,6 +254,15 @@ def _group_or_org_delete(context, data_dict, is_org=False): rev = model.repo.new_revision() rev.author = user rev.message = _(u'REST API: Delete %s') % revisioned_details + + # The group's Member objects are deleted + # (including hierarchy connections to parent and children groups) + for member in model.Session.query(model.Member).\ + filter(or_(model.Member.table_id == id, + model.Member.group_id == id)).\ + filter(model.Member.state == 'active').all(): + member.delete() + group.delete() if is_org: From df890fc12c9ce68959898337e48d89fdbc4b5b68 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 17 Sep 2013 15:16:57 +0000 Subject: [PATCH 21/88] [#1038] Config option fixed and documented. And tidy typos etc. --- ckan/new_authz.py | 4 ++++ ckan/tests/logic/test_auth.py | 8 ++++---- ckan/tests/logic/test_member.py | 2 +- doc/configuration.rst | 16 ++++++++++++++++ 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/ckan/new_authz.py b/ckan/new_authz.py index 495f3806b17..d2a355234c6 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -367,6 +367,10 @@ def check_config_permission(permission): CONFIG_PERMISSIONS[perm] = config.get(key, default) if isinstance(default, bool): CONFIG_PERMISSIONS[perm] = asbool(CONFIG_PERMISSIONS[perm]) + elif isinstance(default, list): + CONFIG_PERMISSIONS[perm] = \ + CONFIG_PERMISSIONS[perm].split(' ') \ + if CONFIG_PERMISSIONS[perm] else [] if permission in CONFIG_PERMISSIONS: return CONFIG_PERMISSIONS[permission] return False diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index 2dc6b5e9a5a..0fbad3af803 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -210,7 +210,7 @@ def test_11_delete_org(self): class TestAuthOrgHierarchy(TestAuth): # Tests are in the same vein as TestAuthOrgs, testing the cases where the - # group hierarchy provices extra permissions through cascading + # group hierarchy provides extra permissions through cascading @classmethod def setup_class(cls): @@ -224,7 +224,7 @@ def setup_class(cls): 'groups': ['national-health-service']}], extra_user_names=['john']) - def _reset_adatasets_owner_org(self): + def _reset_a_datasets_owner_org(self): rev = model.repo.new_revision() get_action('package_owner_org_update')( {'model': model, 'ignore_auth': True}, @@ -304,7 +304,7 @@ def test_08_update_datasets_3(self): try: self._call_api('package_update', dataset, 'nhsadmin', 200) finally: - self._reset_adatasets_owner_org() + self._reset_a_datasets_owner_org() def test_08_update_datasets_4(self): dataset = {'name': 'adataset', 'owner_org': 'department-of-health'} @@ -315,7 +315,7 @@ def test_08_update_datasets_5(self): try: self._call_api('package_update', dataset, 'nhseditor', 200) finally: - self._reset_adatasets_owner_org() + self._reset_a_datasets_owner_org() def test_08_update_datasets_6(self): dataset = {'name': 'adataset', 'owner_org': 'nhs-wirral-ccg'} diff --git a/ckan/tests/logic/test_member.py b/ckan/tests/logic/test_member.py index f3d0ec448a0..fa55696624b 100644 --- a/ckan/tests/logic/test_member.py +++ b/ckan/tests/logic/test_member.py @@ -171,7 +171,7 @@ def _member_create_in_group(self, 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 + '''Makes the given 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) diff --git a/doc/configuration.rst b/doc/configuration.rst index 284dbbfde76..61eb0754573 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -355,6 +355,22 @@ Default value: ``False`` Allow new user accounts to be created via the API. +.. _ckan.auth.roles_that_cascade_to_sub_groups: + +ckan.auth.roles_that_cascade_to_sub_groups +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Example:: + + ckan.auth.roles_that_cascade_to_sub_groups = admin editor + +Default value: ``admin`` + + +Makes role permissions apply to all the groups down the hierarchy from the groups that the role is applied to. + +e.g. a particular user has the 'admin' role for group 'Department of Health'. If you set the value of this option to 'admin' then the user will automatically have the same admin permissions for the child groups of 'Department of Health' such as 'Cancer Research' (and its children too and so on). + .. end_config-authorization From e023b44abe2d1fa502ba8f7cd746d9a87e0500ab Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 17 Sep 2013 15:18:52 +0000 Subject: [PATCH 22/88] [#1038] Fix an apparently botched merge. --- doc/solr-setup.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/solr-setup.rst b/doc/solr-setup.rst index 6d5bb785559..8b5b90ae89b 100644 --- a/doc/solr-setup.rst +++ b/doc/solr-setup.rst @@ -212,17 +212,17 @@ Some problems that can be found during the install: ${dataDir} -* When running Solr it says `Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps JAVA_HOME does not point to the JDK.` +* When running Solr it says ``Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps JAVA_HOME does not point to the JDK.`` - See the note above about JAVA_HOME. Alternatively you may not have installed the JDK. Check by seeing if javac is installed:: + See the note above about JAVA_HOME. Alternatively you may not have installed the JDK. Check by seeing if javac is installed:: which javac - If it isn't do:: + If it isn't do:: sudo apt-get install openjdk-6-jdk - and restart SOLR. + and restart SOLR. Handling changes in the CKAN schema ----------------------------------- From 4290821c16217fedf652efb9187f43e29a28b269 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 17 Sep 2013 15:20:26 +0000 Subject: [PATCH 23/88] [#1038] Fix an apparently botched merge (cont). --- doc/solr-setup.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/solr-setup.rst b/doc/solr-setup.rst index 8b5b90ae89b..bc1961dd375 100644 --- a/doc/solr-setup.rst +++ b/doc/solr-setup.rst @@ -212,7 +212,7 @@ Some problems that can be found during the install: ${dataDir} -* When running Solr it says ``Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps JAVA_HOME does not point to the JDK.`` +* When running Solr it says ``Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps ``JAVA_HOME`` does not point to the JDK.`` See the note above about JAVA_HOME. Alternatively you may not have installed the JDK. Check by seeing if javac is installed:: @@ -222,7 +222,7 @@ Some problems that can be found during the install: sudo apt-get install openjdk-6-jdk - and restart SOLR. + and restart Solr. Handling changes in the CKAN schema ----------------------------------- From 24548c0a401f3dd4d4f16a087278065173e60a68 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 17 Sep 2013 15:22:07 +0000 Subject: [PATCH 24/88] [#1038] Fix an apparently botched merge (cont). --- doc/solr-setup.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/solr-setup.rst b/doc/solr-setup.rst index bc1961dd375..a628ad2fe3b 100644 --- a/doc/solr-setup.rst +++ b/doc/solr-setup.rst @@ -212,9 +212,9 @@ Some problems that can be found during the install: ${dataDir} -* When running Solr it says ``Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps ``JAVA_HOME`` does not point to the JDK.`` +* When running Solr it says ``Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps JAVA_HOME does not point to the JDK.`` - See the note above about JAVA_HOME. Alternatively you may not have installed the JDK. Check by seeing if javac is installed:: + See the note above about ``JAVA_HOME``. Alternatively you may not have installed the JDK. Check by seeing if javac is installed:: which javac From fafd9f9f32a4795bca8c2eed61001b70fdcf770a Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 17 Sep 2013 16:19:29 +0000 Subject: [PATCH 25/88] [#1038] Fix reading config. --- ckan/config/deployment.ini_tmpl | 1 + ckan/new_authz.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/ckan/config/deployment.ini_tmpl b/ckan/config/deployment.ini_tmpl index 59e0a2e789c..a19862a4f73 100644 --- a/ckan/config/deployment.ini_tmpl +++ b/ckan/config/deployment.ini_tmpl @@ -66,6 +66,7 @@ ckan.auth.user_create_organizations = true ckan.auth.user_delete_groups = true ckan.auth.user_delete_organizations = true ckan.auth.create_user_via_api = false +ckan.auth.roles_that_cascade_to_sub_groups = admin ## Search Settings diff --git a/ckan/new_authz.py b/ckan/new_authz.py index 4dc15197643..7593cc1f1f3 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -387,7 +387,7 @@ def check_config_permission(permission): CONFIG_PERMISSIONS[perm] = config.get(key, default) if isinstance(default, bool): CONFIG_PERMISSIONS[perm] = asbool(CONFIG_PERMISSIONS[perm]) - elif isinstance(default, list): + elif isinstance(default, list) and key in config: CONFIG_PERMISSIONS[perm] = \ CONFIG_PERMISSIONS[perm].split(' ') \ if CONFIG_PERMISSIONS[perm] else [] From b7136b8beccf3817294ce6951209e26b389a4293 Mon Sep 17 00:00:00 2001 From: David Read Date: Mon, 23 Sep 2013 12:06:54 +0100 Subject: [PATCH 26/88] [#1038] Fix top level groups including deleted ones. --- ckan/model/group.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ckan/model/group.py b/ckan/model/group.py index 3c1b3369cac..2c69c11a539 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -229,6 +229,7 @@ def get_top_level_groups(cls, type='group'): Member.state == 'active')).\ filter(Member.id == None).\ filter(Group.type == type).\ + filter(Group.state == 'active').\ order_by(Group.title).all() def groups_allowed_to_be_its_parent(self, type='group'): From 1be9681cf67d5d0990aa22673d32b5d7abaa24f7 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 24 Sep 2013 15:17:08 +0100 Subject: [PATCH 27/88] [noticket] Fix displaying About page for organizations. --- ckan/controllers/group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index ccef46bfe78..3381bc69abd 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -824,7 +824,7 @@ def _get_group_dict(self, id): 'user': c.user or c.author, 'for_view': True} try: - return get_action('group_show')(context, {'id': id}) + return self._action('group_show')(context, {'id': id}) except NotFound: abort(404, _('Group not found')) except NotAuthorized: From ed78fc9ed5448685413597e3871f30e69e9bc751 Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 24 Sep 2013 15:18:29 +0100 Subject: [PATCH 28/88] [#1038] Improve performance of viewing hierarchy, avoiding groups as SQLAlchemy objects. --- ckan/model/group.py | 20 +++++++++++--------- ckan/tests/models/test_group.py | 6 +++--- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/ckan/model/group.py b/ckan/model/group.py index 2c69c11a539..6bf544bd635 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -197,15 +197,16 @@ def get_children_group_hierarchy(self, type='group'): hierarchy. The ordering is such that children always come after their parent. - :rtype: a list of tuples, each one a Group and the ID of its parent - group. + :rtype: a list of tuples, each one a Group ID, name and title and then + the ID of its parent group. - e.g. >>> dept-health.get_children_group_hierarchy() - [(, u'8a163ba7-5146-4325-90c8-fe53b25e28d0'), - (, u'06e6dbf5-d801-40a1-9dc0-6785340b2ab4'), - (, u'd2e25b41-720c-4ba7-bc8f-bb34b185b3dd')] + e.g. + >>> dept-health.get_children_group_hierarchy() + [(u'8ac0...', u'national-health-service', u'National Health Service', u'e041...'), + (u'b468...', u'nhs-wirral-ccg', u'NHS Wirral CCG', u'8ac0...')] ''' - results = meta.Session.query(Group, 'parent_id').\ + results = meta.Session.query(Group.id, Group.name, Group.title, + 'parent_id').\ from_statement(HIERARCHY_DOWNWARDS_CTE).\ params(id=self.id, type=type).all() return results @@ -242,7 +243,8 @@ def groups_allowed_to_be_its_parent(self, type='group'): ''' all_groups = self.all(group_type=type) - excluded_groups = set(group.name for group, id_ in + excluded_groups = set(group_name + for group_id, group_name, group_title, parent in self.get_children_group_hierarchy(type=type)) excluded_groups.add(self.name) return [group for group in all_groups @@ -398,7 +400,7 @@ def __repr__(self): WHERE m.table_id = c.group_id AND m.table_name = 'group' AND m.state = 'active' ) -SELECT G.*, child.depth, child.table_id as parent_id FROM child +SELECT G.id, G.name, G.title, child.depth, child.table_id as parent_id FROM child INNER JOIN public.group G ON G.id = child.group_id WHERE G.type = :type AND G.state='active' ORDER BY child.depth ASC;""" diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py index 8e12a0fe9b2..71567481953 100644 --- a/ckan/tests/models/test_group.py +++ b/ckan/tests/models/test_group.py @@ -93,7 +93,7 @@ def _search_results(self, query, is_org=False): return set([group.name for group in results]) name_set_from_dicts = lambda groups: set([group['name'] for group in groups]) -name_set_from_group_tuple = lambda tuples: set([t[0].name for t in tuples]) +name_set_from_group_tuple = lambda tuples: set([t[1] for t in tuples]) name_set_from_groups = lambda groups: set([group.name for group in groups]) names_from_groups = lambda groups: [group.name for group in groups] @@ -122,8 +122,8 @@ def test_get_children_group_hierarchy__from_top_2(self): # the first group must be NHS or Food Standards Agency - i.e. on the # first level down nhs = groups[0] - assert_in(nhs[0].name, ('national-health-service', 'food-standards-agency')) - assert_equal(model.Group.get(nhs[1]).name, 'department-of-health') + assert_in(nhs[1], ('national-health-service', 'food-standards-agency')) + assert_equal(model.Group.get(nhs[3]).name, 'department-of-health') def test_get_children_group_hierarchy__from_top(self): assert_equal(name_set_from_group_tuple(model.Group.by_name(u'department-of-health').\ From 218f73bdbdb06ef9a192dd07816e87ac6486dfcb Mon Sep 17 00:00:00 2001 From: David Read Date: Tue, 24 Sep 2013 19:59:01 +0000 Subject: [PATCH 29/88] [#1038] get_parent_groups added * get_parent_groups is used by DGU and no doubt others that use hierarchy. * get_children_groups changed to return objects as that is more convenient for DGU. Conflicts: ckan/tests/models/test_group.py --- ckan/model/group.py | 24 ++++++++++++++++++------ ckan/tests/models/test_group.py | 27 +++++++++++++++++++++------ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/ckan/model/group.py b/ckan/model/group.py index 6bf544bd635..df5e8124ed9 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -175,12 +175,12 @@ def set_approval_status(self, status): def get_children_groups(self, type='group'): '''Returns the groups one level underneath this group in the hierarchy. - Groups come in a list of dicts, each keyed by "id", "name" and "title". ''' # The original intention of this method was to provide the full depth # of the tree, but the CTE was incorrect. This new query does what that - # old CTE actually did, but is now far simpler. - results = meta.Session.query(Group.id, Group.name, Group.title).\ + # old CTE actually did, but is now far simpler, and returns Group objects + # instead of a dict. + return meta.Session.query(Group).\ filter_by(type=type).\ filter_by(state='active').\ join(Member, Member.group_id == Group.id).\ @@ -189,9 +189,6 @@ def get_children_groups(self, type='group'): filter_by(state='active').\ all() - return [{'id': id_, 'name': name, 'title': title} - for id_, name, title in results] - def get_children_group_hierarchy(self, type='group'): '''Returns the groups in all levels underneath this group in the hierarchy. The ordering is such that children always come after their @@ -211,6 +208,21 @@ def get_children_group_hierarchy(self, type='group'): params(id=self.id, type=type).all() return results + def get_parent_groups(self, type='group'): + '''Returns this group's parent groups. + Returns a list. Will have max 1 value for organizations. + + ''' + return meta.Session.query(Group).\ + join(Member, + and_(Member.table_id == Group.id, + Member.table_name == 'group', + Member.state == 'active')).\ + filter(Member.group_id == self.id).\ + filter(Group.type == type).\ + filter(Group.state == 'active').\ + all() + def get_parent_group_hierarchy(self, type='group'): '''Returns this group's parent, parent's parent, parent's parent's parent etc.. Sorted with the top level parent first.''' diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py index 71567481953..68c41864007 100644 --- a/ckan/tests/models/test_group.py +++ b/ckan/tests/models/test_group.py @@ -1,4 +1,4 @@ -from ckan.tests import assert_equal, assert_not_in, assert_in +from nose.tools import assert_equal, assert_in, assert_not_in import ckan.model as model from ckan.tests import * @@ -108,13 +108,13 @@ def test_get_children_groups(self): res = model.Group.by_name(u'department-of-health').\ get_children_groups(type=group_type) # check groups - assert_equal(name_set_from_dicts(res), + assert_equal(name_set_from_groups(res), set(('national-health-service', 'food-standards-agency'))) - # check each group is expressed as a small dict - assert_equal(set(res[0].keys()), set(('id', 'name', 'title'))) - assert_in(res[0]['name'], ('national-health-service', 'food-standards-agency')) - assert_in(res[0]['title'], ('National Health Service', 'Food Standards Agency')) + # check each group is a Group + assert isinstance(res[0], model.Group) + assert_in(res[0].name, ('national-health-service', 'food-standards-agency')) + assert_in(res[0].title, ('National Health Service', 'Food Standards Agency')) def test_get_children_group_hierarchy__from_top_2(self): groups = model.Group.by_name(u'department-of-health').\ @@ -144,6 +144,21 @@ def test_get_children_group_hierarchy__from_bottom_tier(self): get_children_group_hierarchy(type=group_type)), set()) + def test_get_parents__top(self): + assert_equal(names_from_groups(model.Group.by_name(u'department-of-health').\ + get_parent_groups(type=group_type)), + []) + + def test_get_parents__tier_two(self): + assert_equal(names_from_groups(model.Group.by_name(u'national-health-service').\ + get_parent_groups(type=group_type)), + ['department-of-health']) + + def test_get_parents__tier_three(self): + assert_equal(names_from_groups(model.Group.by_name(u'nhs-wirral-ccg').\ + get_parent_groups(type=group_type)), + ['national-health-service']) + def test_get_parent_groups_up_hierarchy__from_top(self): assert_equal(names_from_groups(model.Group.by_name(u'department-of-health').\ get_parent_group_hierarchy(type=group_type)), From 660fc8194233a2b746c239a7b731218d66cf53ec Mon Sep 17 00:00:00 2001 From: kindly Date: Sun, 29 Sep 2013 17:56:01 +0100 Subject: [PATCH 30/88] basic group add remove done with out any styling --- ckan/config/routing.py | 3 ++ ckan/controllers/package.py | 59 +++++++++++++++++++++++ ckan/logic/action/create.py | 3 +- ckan/logic/action/delete.py | 3 +- ckan/logic/action/get.py | 3 +- ckan/logic/auth/create.py | 21 ++++++++ ckan/new_authz.py | 4 +- ckan/templates/package/group_list.html | 32 ++++++++++++ ckan/templates/package/read_base.html | 1 + ckan/templates/snippets/package_list.html | 2 +- 10 files changed, 123 insertions(+), 8 deletions(-) create mode 100644 ckan/templates/package/group_list.html diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 0fbb02bb886..8bb15623082 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -230,6 +230,7 @@ def make_map(): 'history_ajax', 'follow', 'activity', + 'groups', 'unfollow', 'delete', 'api_data', @@ -239,6 +240,8 @@ def make_map(): m.connect('dataset_activity', '/dataset/activity/{id}', action='activity', ckan_icon='time') m.connect('/dataset/activity/{id}/{offset}', action='activity') + m.connect('dataset_groups', '/dataset/groups/{id}', + action='groups', ckan_icon='group') m.connect('/dataset/{id}.{format}', action='read') m.connect('dataset_read', '/dataset/{id}', action='read', ckan_icon='sitemap') diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index d1852cd95b7..7feab9c2f95 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -1246,6 +1246,65 @@ def followers(self, id=None): return render('package/followers.html') + def groups(self, id): + context = {'model': model, 'session': model.Session, + 'user': c.user or c.author, 'for_view': True, + 'auth_user_obj': c.userobj} + data_dict = {'id': id} + try: + c.pkg_dict = get_action('package_show')(context, data_dict) + except NotFound: + abort(404, _('Dataset not found')) + except NotAuthorized: + abort(401, _('Unauthorized to read dataset %s') % id) + + if request.method == 'POST': + new_group = request.POST.get('group_added') + if new_group: + data_dict = {"id": new_group, + "object": id, + "object_type": 'package', + "capacity": 'public'} + try: + get_action('member_create')(context, data_dict) + except NotFound: + abort(404, _('Group not found')) + + removed_group = request.POST.get('group_removed') + if removed_group: + data_dict = {"id": removed_group, + "object": id, + "object_type": 'package'} + + try: + get_action('member_delete')(context, data_dict) + except NotFound: + abort(404, _('Group not found')) + redirect(h.url_for(controller='package', + action='groups', id=id)) + + + + context['is_member'] = True + users_groups = get_action('group_list_authz')(context, data_dict) + + pkg_group_ids = set(group['id'] for group + in c.pkg_dict.get('groups', [])) + user_group_ids = set(group['id'] for group + in users_groups) + + c.group_dropdown = [[group['id'], group['display_name']] + for group in users_groups if + group['id'] not in pkg_group_ids] + + for group in c.pkg_dict.get('groups', []): + ## this could be pushed back to package dictize + group['display_name'] = group.get('titile') or group.get('name') + group['user_member'] = (group['id'] in user_group_ids) + + + return render('package/group_list.html') + def activity(self, id): '''Render this package's public activity stream page.''' diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index baac5a2154c..d868aac62b4 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -446,8 +446,7 @@ def member_create(context, data_dict=None): if not obj: raise NotFound('%s was not found.' % obj_type.title()) - # User must be able to update the group to add a member to it - _check_access('group_update', context, data_dict) + _check_access('member_create', context, data_dict) # Look up existing, in case it exists member = model.Session.query(model.Member).\ diff --git a/ckan/logic/action/delete.py b/ckan/logic/action/delete.py index 60ea1a5bed8..1d3b5fe879c 100644 --- a/ckan/logic/action/delete.py +++ b/ckan/logic/action/delete.py @@ -203,8 +203,7 @@ def member_delete(context, data_dict=None): if not obj: raise NotFound('%s was not found.' % obj_type.title()) - # User must be able to update the group to remove a member from it - _check_access('group_update', context, data_dict) + _check_access('member_create', context, data_dict) member = model.Session.query(model.Member).\ filter(model.Member.table_name == obj_type).\ diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 4f4b2fb57ee..89f2ad97c55 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -331,6 +331,7 @@ def translated_capacity(capacity): def _group_or_org_list(context, data_dict, is_org=False): model = context['model'] + user = context['user'] api = context.get('api_version') groups = data_dict.get('groups') ref_group_by = 'id' if api == 2 else 'name' @@ -460,7 +461,7 @@ def group_list_authz(context, data_dict): _check_access('group_list_authz',context, data_dict) sysadmin = new_authz.is_sysadmin(user) - roles = ckan.new_authz.get_roles_with_permission('edit_group') + roles = ckan.new_authz.get_roles_with_permission('manage_group') if not roles: return [] user_id = new_authz.get_user_id_for_username(user, allow_none=True) diff --git a/ckan/logic/auth/create.py b/ckan/logic/auth/create.py index a18485c2c57..c6dd89527d8 100644 --- a/ckan/logic/auth/create.py +++ b/ckan/logic/auth/create.py @@ -1,5 +1,6 @@ import ckan.logic as logic import ckan.new_authz as new_authz +import ckan.logic.auth as logic_auth from ckan.common import _ @@ -197,3 +198,23 @@ def organization_member_create(context, data_dict): def group_member_create(context, data_dict): return _group_or_org_member_create(context, data_dict) + +def member_create(context, data_dict): + group = logic_auth.get_group_object(context, data_dict) + user = context['user'] + + # User must be able to update the group to add a member to it + permission = 'update' + # However if the user is member of group then they can add/remove datasets + if not group.is_organization and data_dict.get('object_type') == 'package': + permission = 'manage_group' + + authorized = new_authz.has_user_permission_for_group_or_org(group.id, + user, + permission) + if not authorized: + return {'success': False, + 'msg': _('User %s not authorized to edit group %s') % + (str(user), group.id)} + else: + return {'success': True} diff --git a/ckan/new_authz.py b/ckan/new_authz.py index abeb8f9b57a..af224ee67b7 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -183,8 +183,8 @@ def is_authorized(action, context, data_dict=None): # these are the permissions that roles have ROLE_PERMISSIONS = OrderedDict([ ('admin', ['admin']), - ('editor', ['read', 'delete_dataset', 'create_dataset', 'update_dataset']), - ('member', ['read']), + ('editor', ['read', 'delete_dataset', 'create_dataset', 'update_dataset', 'manage_group']), + ('member', ['read', 'manage_group']), ]) diff --git a/ckan/templates/package/group_list.html b/ckan/templates/package/group_list.html new file mode 100644 index 00000000000..63440bfacb7 --- /dev/null +++ b/ckan/templates/package/group_list.html @@ -0,0 +1,32 @@ +{% import 'macros/form.html' as form %} +{% extends "package/read_base.html" %} + +{% block primary_content_inner %} +

{% block page_heading %}{{ _('Groups {dataset} belongs to.').format(dataset=h.dataset_display_name(c.pkg)) }}{% endblock %}

+ + {% if c.group_dropdown %} +
+ + +
+ {% endif %} + +
    + {% for group in c.pkg_dict.groups %} +
  • {{group.display_name}} +
    + {% if group.user_member %} + + {% endif %} +
    +
    • + + {% endfor %} +
+ + +{% endblock %} diff --git a/ckan/templates/package/read_base.html b/ckan/templates/package/read_base.html index 9cf73a5c5a1..0e4b3775f43 100644 --- a/ckan/templates/package/read_base.html +++ b/ckan/templates/package/read_base.html @@ -22,6 +22,7 @@ {% block content_primary_nav %} {{ h.build_nav_icon('dataset_read', _('Dataset'), id=pkg.name) }} + {{ h.build_nav_icon('dataset_groups', _('Groups'), id=pkg.name) }} {{ h.build_nav_icon('dataset_activity', _('Activity Stream'), id=pkg.name) }} {{ h.build_nav_icon('related_list', _('Related'), id=pkg.name) }} {% endblock %} diff --git a/ckan/templates/snippets/package_list.html b/ckan/templates/snippets/package_list.html index 99aed748242..75213d535a3 100644 --- a/ckan/templates/snippets/package_list.html +++ b/ckan/templates/snippets/package_list.html @@ -15,7 +15,7 @@ #} {% if packages %} -