Merge "Streamline and simplify package filtering api"

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)**

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]

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,
+    query = session.query(pkg)
-    # 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
 
-    if context.is_admin:
+    if catalog:
-        if not include_disabled:
+        # Only show packages one can deploy, i.e. own + public
-            # NOTE(efedorova): is needed for SA 0.7.9, but could be done
+        query = query.filter(or_(
-            # simpler in SA 0.8. See http://goo.gl/9stlKu for a details
+            pkg.owner_id == context.tenant, pkg.is_public)
-            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)
     else:
-        if not include_disabled:
+        # Show packages one can edit.
-            query = session.query(pkg).filter(
+        if not context.is_admin:
-                or_((pkg.is_public & pkg.enabled),
+            query = query.filter(pkg.owner_id == context.tenant)
-                    ((pkg.owner_id == context.tenant) & pkg.enabled))
+        # No else here admin can edit everything.
-            )
+
-        else:
+    if not filters.get('include_disabled', '').lower() == 'true':
-            query = session.query(pkg).filter(
+        query = query.filter(pkg.enabled)
-                or_((pkg.is_public & pkg.enabled),
+
-                    pkg.owner_id == context.tenant)
+    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)
 

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(

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))