From e76c0657e4b5febc18709ee6c592a928a25e1fcd Mon Sep 17 00:00:00 2001 From: Kirill Zaitsev Date: Mon, 27 Apr 2015 17:32:13 +0300 Subject: [PATCH] Streamline and simplify package filtering api Introduce catalogue parameter to allow distinguishing between catalogue of applications (packages that one can deploy) and managable-apps (packages that one can edit). Streamline include_disabled filtering. Allow filtering out your own packages for admin requests. Change-Id: Ica031058d963246ff4d76e52cfc8b8b44d207c9a Partial-Bug: #1448226 Relates-Bug: #1448135 --- .../specification/murano-repository.rst | 11 +- murano/api/v1/catalog.py | 6 +- murano/db/catalog/api.py | 62 +++---- murano/tests/unit/api/v1/test_catalog.py | 171 ++++++++++++++++++ murano/tests/unit/db/test_catalog.py | 160 ++++++++++++++++ 5 files changed, 368 insertions(+), 42 deletions(-) diff --git a/doc/source/specification/murano-repository.rst b/doc/source/specification/murano-repository.rst index cb12e3a65..9e95fe0db 100644 --- a/doc/source/specification/murano-repository.rst +++ b/doc/source/specification/murano-repository.rst @@ -37,12 +37,12 @@ Methods for application package management - ``class_definition``: list of class names used by a package - ``is_public``: determines whether the package is shared for other tenants - ``enabled``: determines whether the package is browsed in the Application Catalog -- ``owner_id``: id of a tenant which user not an owned the package +- ``owner_id``: id of a tenant that owns the package List packages ------------- -`/v1/catalog/packages?{marker}{limit}{order_by}{type}{category}{fqn}{owned}{class_name} [GET]` +`/v1/catalog/packages?{marker}{limit}{order_by}{type}{category}{fqn}{owned}{catalog}{class_name} [GET]` This is the compound request to list and search through application catalog. If there are no search parameters all packages that is_public, enabled and belong to the user's tenant will be listed. @@ -54,6 +54,9 @@ For an admin role all packages are available. +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ | Attribute | Type | Description | +======================+=============+==============================================================================================================================+ +| ``catalog`` | bool | If false (default) - search packages, that current user can edit (own for non-admin, all for admin) | +| | | If true - search packages, that current user can deploy (i.e. his own + public) | ++----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ | ``marker`` | string | A package identifier marker may be specified. When present only packages which occur after the identifier ID will be listed | +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ | ``limit`` | string | When present the maximum number of results returned will not exceed the specified value. | @@ -68,13 +71,13 @@ For an admin role all packages are available. +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ | ``fqn`` | string | Allows to point a fully qualified package name for a search | +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ -| ``owned`` | bool | Search only from packages owned by user tenant | +| ``owned`` | bool | Search only from packages owned by current tenant | +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ | ``include_disabled`` | bool | Include disabled packages in a the result | +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ | ``search`` | string | Gives opportunity to search specified data by all the package parameters | +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ -| ``class_name`` | string |Search only for packages, that use specified class | +| ``class_name`` | string | Search only for packages, that use specified class | +----------------------+-------------+------------------------------------------------------------------------------------------------------------------------------+ **Response 200 (application/json)** diff --git a/murano/api/v1/catalog.py b/murano/api/v1/catalog.py index ffe5e5113..5c7b39022 100644 --- a/murano/api/v1/catalog.py +++ b/murano/api/v1/catalog.py @@ -178,13 +178,17 @@ class Controller(object): policy.check("get_package", req.context) filters = _get_filters(req.GET.items()) + limit = _validate_limit(filters.get('limit')) if limit is None: limit = CONF.packages_opts.limit_param_default limit = min(CONF.packages_opts.api_limit_max, limit) result = {} - packages = db_api.package_search(filters, req.context, limit) + + catalog = req.GET.pop('catalog', '').lower() == 'true' + packages = db_api.package_search( + filters, req.context, limit, catalog=catalog) if len(packages) == limit: result['next_marker'] = packages[-1].id result['packages'] = [package.to_dict() for package in packages] diff --git a/murano/db/catalog/api.py b/murano/db/catalog/api.py index 100f81b3c..37488251d 100644 --- a/murano/db/catalog/api.py +++ b/murano/db/catalog/api.py @@ -255,8 +255,11 @@ def package_update(pkg_id_or_name, changes, context): return pkg -def package_search(filters, context, limit=None): +def package_search(filters, context, limit=None, catalog=False): """Search packages with different filters + Catalog param controls the base query creation. Catalog queries + only search packages a user can deploy. Non-catalog queries searches + packages a user can edit. * Admin is allowed to browse all the packages * Regular user is allowed to browse all packages belongs to user tenant and all other packages marked is_public. @@ -266,45 +269,28 @@ def package_search(filters, context, limit=None): request and then to use the ID of the last package from the response as the marker parameter in a subsequent limited request. """ + session = db_session.get_session() pkg = models.Package - # If the packages search specifies the inclusion of disabled packages, - # we handle this differently for admins vs non-admins: - # For admins: *don't* require pkg.enabled == True (no changes to query) - # For non-admins: add an OR-condition to filter for packages that are owned - # by the tenant in the current context - # Otherwise: in all other cases, we return only enabled packages - if filters.get('include_disabled', '').lower() == 'true': - include_disabled = True - else: - include_disabled = False + query = session.query(pkg) - if context.is_admin: - if not include_disabled: - # NOTE(efedorova): is needed for SA 0.7.9, but could be done - # simpler in SA 0.8. See http://goo.gl/9stlKu for a details - query = session.query(pkg).filter(pkg.__table__.c.enabled) - else: - query = session.query(pkg) - elif filters.get('owned', '').lower() == 'true': - if not include_disabled: - query = session.query(pkg).filter( - (pkg.owner_id == context.tenant) & pkg.enabled - ) - else: - query = session.query(pkg).filter(pkg.owner_id == context.tenant) + if catalog: + # Only show packages one can deploy, i.e. own + public + query = query.filter(or_( + pkg.owner_id == context.tenant, pkg.is_public) + ) else: - if not include_disabled: - query = session.query(pkg).filter( - or_((pkg.is_public & pkg.enabled), - ((pkg.owner_id == context.tenant) & pkg.enabled)) - ) - else: - query = session.query(pkg).filter( - or_((pkg.is_public & pkg.enabled), - pkg.owner_id == context.tenant) - ) + # Show packages one can edit. + if not context.is_admin: + query = query.filter(pkg.owner_id == context.tenant) + # No else here admin can edit everything. + + if not filters.get('include_disabled', '').lower() == 'true': + query = query.filter(pkg.enabled) + + if filters.get('owned', '').lower() == 'true': + query = query.filter(pkg.owner_id == context.tenant) if 'type' in filters.keys(): query = query.filter(pkg.type == filters['type'].title()) @@ -347,12 +333,14 @@ def package_search(filters, context, limit=None): query = query.filter(or_(*conditions)) sort_keys = [SEARCH_MAPPING[sort_key] for sort_key in - filters.get('order_by', []) or ['name']] + filters.get('order_by', ['name'])] + marker = filters.get('marker') - # TODO(btully): sort_dir is always None - not getting passed as a filter? sort_dir = filters.get('sort_dir') + if marker is not None: # set marker to real object instead of its id marker = _package_get(marker, session) + query = utils.paginate_query( query, pkg, limit, sort_keys, marker, sort_dir) diff --git a/murano/tests/unit/api/v1/test_catalog.py b/murano/tests/unit/api/v1/test_catalog.py index a7dd0c8d1..d6dac8be0 100644 --- a/murano/tests/unit/api/v1/test_catalog.py +++ b/murano/tests/unit/api/v1/test_catalog.py @@ -18,6 +18,7 @@ import cStringIO import imghdr import json import os +import uuid import mock from oslo.utils import timeutils @@ -35,6 +36,176 @@ class TestCatalogApi(test_base.ControllerTest, test_base.MuranoApiTestCase): def setUp(self): super(TestCatalogApi, self).setUp() self.controller = catalog.Controller() + _, self.test_package = self._test_package() + + def _add_pkg(self, tenant_name, public=False, classes=None, **kwargs): + package_to_upload = self.test_package.copy() + package_to_upload['is_public'] = public + package_to_upload['fully_qualified_name'] = str(uuid.uuid4()) + if classes: + package_to_upload['class_definitions'] = classes + else: + package_to_upload['class_definitions'] = [] + package_to_upload.update(kwargs) + return db_catalog_api.package_upload( + package_to_upload, tenant_name) + + def test_packages_filtering_admin(self): + self.is_admin = True + self._set_policy_rules( + {'get_package': ''} + ) + for i in range(7): + self.expect_policy_check('get_package') + + pkg = self._add_pkg('test_tenant', type='Library') + self._add_pkg('test_tenant') + self._add_pkg('test_tenant2', public=True, type='Library') + self._add_pkg('test_tenant3') + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'False', + 'owned': 'False'})) + self.assertEqual(len(result['packages']), 4) + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'False', + 'owned': 'True'})) + self.assertEqual(len(result['packages']), 2) + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True', + 'owned': 'False'})) + self.assertEqual(len(result['packages']), 3) + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True', + 'owned': 'True'})) + self.assertEqual(len(result['packages']), 2) + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={ + 'owned': 'True', + 'fqn': pkg.fully_qualified_name})) + self.assertEqual(len(result['packages']), 1) + self.assertEqual(result['packages'][0]['fully_qualified_name'], + pkg.fully_qualified_name) + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={ + 'owned': 'True', + 'type': 'Library'})) + self.assertEqual(len(result['packages']), 1) + self.assertEqual(result['packages'][0]['fully_qualified_name'], + pkg.fully_qualified_name) + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={ + 'type': 'Library'})) + self.assertEqual(len(result['packages']), 2) + + def test_packages_filtering_non_admin(self): + self.is_admin = False + self._set_policy_rules( + {'get_package': ''} + ) + for i in range(7): + self.expect_policy_check('get_package') + + pkg = self._add_pkg('test_tenant', type='Library') + self._add_pkg('test_tenant') + self._add_pkg('test_tenant2', public=True, type='Library') + self._add_pkg('test_tenant3') + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'False', + 'owned': 'False'})) + self.assertEqual(len(result['packages']), 2) + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'False', + 'owned': 'True'})) + self.assertEqual(len(result['packages']), 2) + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True', + 'owned': 'False'})) + self.assertEqual(len(result['packages']), 3) + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True', + 'owned': 'True'})) + self.assertEqual(len(result['packages']), 2) + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={ + 'owned': 'True', + 'fqn': pkg.fully_qualified_name})) + self.assertEqual(len(result['packages']), 1) + self.assertEqual(result['packages'][0]['fully_qualified_name'], + pkg.fully_qualified_name) + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={ + 'owned': 'True', + 'type': 'Library'})) + self.assertEqual(len(result['packages']), 1) + self.assertEqual(result['packages'][0]['fully_qualified_name'], + pkg.fully_qualified_name) + + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={ + 'type': 'Library'})) + self.assertEqual(len(result['packages']), 1) + + def test_packages(self): + self._set_policy_rules( + {'get_package': ''} + ) + for i in range(9): + self.expect_policy_check('get_package') + + result = self.controller.search(self._get('/v1/catalog/packages/')) + self.assertEqual(len(result['packages']), 0) + + self._add_pkg('test_tenant') + self._add_pkg('test_tenant') + self._add_pkg('other_tenant') + self._add_pkg('other_tenant') + + # non-admin should only see 2 pkgs he can edit. + self.is_admin = False + result = self.controller.search(self._get('/v1/catalog/packages/')) + self.assertEqual(len(result['packages']), 2) + # can only deploy his + public + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True'})) + self.assertEqual(len(result['packages']), 2) + + # admin can edit anything + self.is_admin = True + result = self.controller.search(self._get('/v1/catalog/packages/')) + self.assertEqual(len(result['packages']), 4) + # admin can only deploy his + public + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True'})) + self.assertEqual(len(result['packages']), 2) + + self._add_pkg('test_tenant', public=True) + self._add_pkg('other_tenant', public=True) + + # non-admin can't edit other_tenants public pkgs + self.is_admin = False + result = self.controller.search(self._get('/v1/catalog/packages/')) + self.assertEqual(len(result['packages']), 3) + # can deploy mine + other public + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True'})) + self.assertEqual(len(result['packages']), 4) + + # admin can edit anything + self.is_admin = True + result = self.controller.search(self._get('/v1/catalog/packages/')) + self.assertEqual(len(result['packages']), 6) + # can deploy mine + public + result = self.controller.search(self._get( + '/v1/catalog/packages/', params={'catalog': 'True'})) + self.assertEqual(len(result['packages']), 4) def _test_package(self): package_dir = os.path.abspath( diff --git a/murano/tests/unit/db/test_catalog.py b/murano/tests/unit/db/test_catalog.py index da8e3dba1..434ba2d76 100644 --- a/murano/tests/unit/db/test_catalog.py +++ b/murano/tests/unit/db/test_catalog.py @@ -30,6 +30,9 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase): self.context = utils.dummy_context(tenant_id=self.tenant_id) self.context_2 = utils.dummy_context(tenant_id=self.tenant_id_2) + self.context_admin = utils.dummy_context(tenant_id=self.tenant_id) + self.context_admin.is_admin = True + def _create_categories(self): api.category_add('cat1') api.category_add('cat2') @@ -58,6 +61,163 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase): 'value': value } + def test_package_search_search(self): + pkg1 = api.package_upload( + self._stub_package( + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + pkg2 = api.package_upload( + self._stub_package( + tags=[], + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + + res = api.package_search( + {'search': 'tag1'}, self.context) + self.assertEqual(len(res), 1) + res = api.package_search( + {'search': pkg1.fully_qualified_name}, self.context) + self.assertEqual(len(res), 1) + res = api.package_search( + {'search': pkg2.fully_qualified_name}, self.context) + self.assertEqual(len(res), 1) + res = api.package_search( + {'search': 'not_a_valid_uuid'}, self.context) + self.assertEqual(len(res), 0) + + res = api.package_search( + {'search': 'some text'}, self.context) + self.assertEqual(len(res), 2) + + def test_package_search_tags(self): + api.package_upload( + self._stub_package( + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + api.package_upload( + self._stub_package( + tags=[], + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + + res = api.package_search( + {'tag': ['tag1']}, self.context) + self.assertEqual(len(res), 1) + res = api.package_search( + {'tag': ['tag2']}, self.context) + self.assertEqual(len(res), 1) + res = api.package_search( + {'tag': ['tag3']}, self.context) + self.assertEqual(len(res), 0) + + def test_package_search_type(self): + api.package_upload( + self._stub_package( + type="Application", + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + api.package_upload( + self._stub_package( + type="Library", + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + + res = api.package_search( + {'type': 'Library'}, self.context) + self.assertEqual(len(res), 1) + res = api.package_search( + {'type': 'Application'}, self.context) + self.assertEqual(len(res), 1) + + def test_package_search_disabled(self): + api.package_upload( + self._stub_package( + is_public=True, + enabled=True, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + api.package_upload( + self._stub_package( + is_public=True, + enabled=False, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + + res = api.package_search( + {'include_disabled': 'false'}, self.context) + self.assertEqual(len(res), 1) + res = api.package_search( + {'include_disabled': 'true'}, self.context) + self.assertEqual(len(res), 2) + + def test_package_search_owned(self): + api.package_upload( + self._stub_package( + is_public=True, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + api.package_upload( + self._stub_package( + is_public=True, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id_2) + + res = api.package_search({'owned': 'true'}, self.context_admin) + self.assertEqual(len(res), 1) + res = api.package_search({'owned': 'false'}, self.context_admin) + self.assertEqual(len(res), 2) + + def test_package_search_no_filters_catalog(self): + res = api.package_search({}, self.context, catalog=True) + self.assertEqual(len(res), 0) + + api.package_upload( + self._stub_package( + is_public=True, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + api.package_upload( + self._stub_package( + is_public=False, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + + api.package_upload( + self._stub_package( + is_public=True, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id_2) + api.package_upload( + self._stub_package( + is_public=False, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id_2) + + # catalog=True should show public + mine + res = api.package_search({}, self.context, catalog=True) + self.assertEqual(len(res), 3) + + res = api.package_search({}, self.context_admin, catalog=True) + self.assertEqual(len(res), 3) + + def test_package_search_no_filters(self): + res = api.package_search({}, self.context) + self.assertEqual(len(res), 0) + + api.package_upload( + self._stub_package( + is_public=True, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + api.package_upload( + self._stub_package( + is_public=False, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id) + + api.package_upload( + self._stub_package( + is_public=True, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id_2) + api.package_upload( + self._stub_package( + is_public=False, + fully_qualified_name=str(uuid.uuid4())), self.tenant_id_2) + + # I can only edit mine pkgs + res = api.package_search({}, self.context) + self.assertEqual(len(res), 2) + for pkg in res: + self.assertEqual(pkg.owner_id, self.tenant_id) + + # Admin can see everything + res = api.package_search({}, self.context_admin) + self.assertEqual(len(res), 4) + def test_list_empty_categories(self): res = api.category_get_names() self.assertEqual(0, len(res))