[rbac] Pull up policy checks on share/snapshot APIs

RBAC enforcement in manila happens in stages:
1) Does user have access to the API
2) Does user have access to the resource
3) Is user permitted to perform the API action on the resource

If (1) fails, user gets a HTTP 403, if (2) fails,
they get a HTTP 404; if (3) fails, they get a HTTP 403.

More often than not, (2) prevents "existence" detection
of resources that don't belong to the user; except in
case of "public" resources (e.g.: shares can be "public").

In some share API methods, policy checks for (1) are
happening after a bunch of processing. This leads to
some inconsistency.

Fix these occurrences to ensure a consistent user
experience.

Change-Id: I5b1f1ce517efed000f17b1e0901e183a1913ba9f
Related-Bug: #2004230
Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>
(cherry picked from commit 190876809f)
(cherry picked from commit 318140e250)
(cherry picked from commit 8edaa3254c)
(cherry picked from commit f878e15bca)

manila/api/v1/share_snapshots.py

@@ -26,6 +26,7 @@ from manila.api.views import share_snapshots as snapshot_views
 from manila import db
 from manila import exception
 from manila.i18n import _
+from manila import policy
 from manila import share
 
 LOG = log.getLogger(__name__)
@@ -63,6 +64,7 @@ class ShareSnapshotMixin(object):
         context = req.environ['manila.context']
 
         LOG.info("Delete snapshot with id: %s", id, context=context)
+        policy.check_policy(context, 'share', 'delete_snapshot')
 
         try:
             snapshot = self.share_api.get_snapshot(context, id)
@@ -145,6 +147,7 @@ class ShareSnapshotMixin(object):
     def update(self, req, id, body):
         """Update a snapshot."""
         context = req.environ['manila.context']
+        policy.check_policy(context, 'share', 'snapshot_update')
 
         if not body or 'snapshot' not in body:
             raise exc.HTTPUnprocessableEntity()

manila/api/v1/shares.py

@@ -52,6 +52,7 @@ class ShareMixin(object):
     def _delete(self, *args, **kwargs):
         return self.share_api.delete(*args, **kwargs)
 
+    @wsgi.Controller.authorize('get')
     def show(self, req, id):
         """Return data about the given share."""
         context = req.environ['manila.context']
@@ -63,6 +64,7 @@ class ShareMixin(object):
 
         return self._view_builder.detail(req, share)
 
+    @wsgi.Controller.authorize
     def delete(self, req, id):
         """Delete a share."""
         context = req.environ['manila.context']
@@ -97,6 +99,7 @@ class ShareMixin(object):
 
         return webob.Response(status_int=http_client.ACCEPTED)
 
+    @wsgi.Controller.authorize("get_all")
     def index(self, req):
         """Returns a summary list of shares."""
         req.GET.pop('export_location_id', None)
@@ -107,6 +110,7 @@ class ShareMixin(object):
         req.GET.pop('with_count', None)
         return self._get_shares(req, is_detail=False)
 
+    @wsgi.Controller.authorize("get_all")
     def detail(self, req):
         """Returns a detailed list of shares."""
         req.GET.pop('export_location_id', None)
@@ -518,6 +522,7 @@ class ShareMixin(object):
 
         return self._access_view_builder.list_view(req, access_rules)
 
+    @wsgi.Controller.authorize("extend")
     def _extend(self, req, id, body):
         """Extend size of a share."""
         context = req.environ['manila.context']
@@ -533,6 +538,7 @@ class ShareMixin(object):
 
         return webob.Response(status_int=http_client.ACCEPTED)
 
+    @wsgi.Controller.authorize("shrink")
     def _shrink(self, req, id, body):
         """Shrink size of a share."""
         context = req.environ['manila.context']

manila/api/v2/shares.py

@@ -219,6 +219,7 @@ class ShareController(shares.ShareMixin,
 
     @wsgi.Controller.api_version('2.7')
     @wsgi.action('reset_status')
+    @wsgi.Controller.authorize('reset_status')
     def share_reset_status(self, req, id, body):
         return self._reset_status(req, id, body)
 
@@ -464,6 +465,7 @@ class ShareController(shares.ShareMixin,
         return self._revert(req, id, body)
 
     @wsgi.Controller.api_version("2.0")
+    @wsgi.Controller.authorize("get_all")
     def index(self, req):
         """Returns a summary list of shares."""
         if req.api_version_request < api_version.APIVersionRequest("2.35"):
@@ -481,6 +483,7 @@ class ShareController(shares.ShareMixin,
         return self._get_shares(req, is_detail=False)
 
     @wsgi.Controller.api_version("2.0")
+    @wsgi.Controller.authorize("get_all")
     def detail(self, req):
         """Returns a detailed list of shares."""
         if req.api_version_request < api_version.APIVersionRequest("2.35"):

manila/share/api.py

@@ -1837,8 +1837,6 @@ class API(base.Base):
 
     def _get_all(self, context, search_opts=None, sort_key='created_at',
                  sort_dir='desc', show_count=False):
-        policy.check_policy(context, 'share', 'get_all')
-
         if search_opts is None:
             search_opts = {}
 
@@ -2224,9 +2222,9 @@ class API(base.Base):
 
     def extend(self, context, share, new_size, force=False):
         if force:
-            policy.check_policy(context, 'share', 'force_extend')
+            policy.check_policy(context, 'share', 'force_extend', share)
         else:
-            policy.check_policy(context, 'share', 'extend')
+            policy.check_policy(context, 'share', 'extend', share)
 
         if share['status'] != constants.STATUS_AVAILABLE:
             msg_params = {
@@ -2333,8 +2331,6 @@ class API(base.Base):
                  resource=share)
 
     def shrink(self, context, share, new_size):
-        policy.check_policy(context, 'share', 'shrink')
-
         status = six.text_type(share['status']).lower()
         valid_statuses = (constants.STATUS_AVAILABLE,
                           constants.STATUS_SHRINKING_POSSIBLE_DATA_LOSS_ERROR)

manila/tests/api/v2/test_shares.py

@@ -106,6 +106,7 @@ class ShareAPITest(test.TestCase):
         }
 
         CONF.set_default("default_share_type", None)
+        self.mock_object(policy, 'check_policy')
 
     def _process_expected_share_detailed_response(self, shr_dict, req_version):
         """Sets version based parameters on share dictionary."""
@@ -1692,6 +1693,10 @@ class ShareAPITest(test.TestCase):
                 {'display_name~': search_opts['display_name~'],
                  'display_description~': search_opts['display_description~']})
 
+        policy.check_policy.assert_called_once_with(
+            req.environ['manila.context'],
+            'share',
+            'get_all')
         if use_admin_context:
             search_opts_expected.update({'fake_key': 'fake_value'})
             search_opts_expected['host'] = search_opts['host']
@@ -1736,6 +1741,10 @@ class ShareAPITest(test.TestCase):
 
         search_opts_expected = {}
 
+        policy.check_policy.assert_called_once_with(
+            req.environ['manila.context'],
+            'share',
+            'get_all')
         if use_admin_context:
             search_opts_expected.update({'fake_key': 'fake_value'})
             search_opts_expected['host'] = search_opts['host']
@@ -1771,6 +1780,10 @@ class ShareAPITest(test.TestCase):
                 }
             ]
         }
+        policy.check_policy.assert_called_once_with(
+            req.environ['manila.context'],
+            'share',
+            'get_all')
         self.assertEqual(expected, res_dict)
 
     @ddt.data({'use_admin_context': False, 'version': '2.4'},
@@ -1857,10 +1870,14 @@ class ShareAPITest(test.TestCase):
                 search_opts['export_location_id'])
             search_opts_expected['export_location_path'] = (
                 search_opts['export_location_path'])
-
         if use_admin_context:
             search_opts_expected.update({'fake_key': 'fake_value'})
             search_opts_expected['host'] = search_opts['host']
+
+        policy.check_policy.assert_called_once_with(
+            req.environ['manila.context'],
+            'share',
+            'get_all')
         mock_get_all.assert_called_once_with(
             req.environ['manila.context'],
             sort_key=search_opts['sort_key'],
@@ -1928,7 +1945,13 @@ class ShareAPITest(test.TestCase):
     def _list_detail_test_common(self, req, expected):
         self.mock_object(share_api.API, 'get_all',
                          stubs.stub_share_get_all_by_project)
+
         res_dict = self.controller.detail(req)
+
+        policy.check_policy.assert_called_once_with(
+            req.environ['manila.context'],
+            'share',
+            'get_all')
         self.assertDictListMatch(expected['shares'], res_dict['shares'])
         self.assertEqual(res_dict['shares'][0]['volume_type'],
                          res_dict['shares'][0]['share_type'])
@@ -1987,7 +2010,13 @@ class ShareAPITest(test.TestCase):
         req = fakes.HTTPRequest.blank(
             '/v2/fake/shares/detail', environ=env,
             version=share_replicas.MIN_SUPPORTED_API_VERSION)
+
         res_dict = self.controller.detail(req)
+
+        policy.check_policy.assert_called_once_with(
+            req.environ['manila.context'],
+            'share',
+            'get_all')
         expected = {
             'shares': [
                 {
@@ -2072,6 +2101,7 @@ class ShareActionsTest(test.TestCase):
         super(ShareActionsTest, self).setUp()
         self.controller = shares.ShareController()
         self.mock_object(share_api.API, 'get', stubs.stub_share_get)
+        self.mock_object(policy, 'check_policy')
 
     @ddt.unpack
     @ddt.data(

manila/tests/share/test_api.py

@@ -240,9 +240,9 @@ class ShareAPITestCase(test.TestCase):
         self.mock_object(db_api, 'share_get_all_by_project',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
         ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
+
         shares = self.api.get_all(ctx)
-        share_api.policy.check_policy.assert_called_once_with(
+
-            ctx, 'share', 'get_all')
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_1', filters={}, is_public=False
@@ -253,9 +253,9 @@ class ShareAPITestCase(test.TestCase):
         ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
         self.mock_object(db_api, 'share_get_all',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES))
+
         shares = self.api.get_all(ctx, {'all_tenants': 1})
-        share_api.policy.check_policy.assert_called_once_with(
+
-            ctx, 'share', 'get_all')
         db_api.share_get_all.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at', filters={})
         self.assertEqual(_FAKE_LIST_OF_ALL_SHARES, shares)
@@ -264,9 +264,9 @@ class ShareAPITestCase(test.TestCase):
         ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
         self.mock_object(db_api, 'share_get_all',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES))
+
         shares = self.api.get_all(ctx, {'all_tenants': ''})
-        share_api.policy.check_policy.assert_called_once_with(
+
-            ctx, 'share', 'get_all')
         db_api.share_get_all.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at', filters={})
         self.assertEqual(_FAKE_LIST_OF_ALL_SHARES, shares)
@@ -275,9 +275,9 @@ class ShareAPITestCase(test.TestCase):
         ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
         self.mock_object(db_api, 'share_get_all_by_project',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
+
         shares = self.api.get_all(ctx, {'all_tenants': 'false'})
-        share_api.policy.check_policy.assert_called_once_with(
+
-            ctx, 'share', 'get_all')
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_1', filters={}, is_public=False
@@ -311,10 +311,8 @@ class ShareAPITestCase(test.TestCase):
             exception.NotAuthorized,
             self.api.get_all, ctx, filters)
 
-        share_api.policy.check_policy.assert_has_calls([
+        share_api.policy.check_policy.assert_called_once_with(
-            mock.call(ctx, 'share', 'get_all'),
+            ctx, 'share', policy)
-            mock.call(ctx, 'share', policy),
-        ])
 
     def test_get_all_admin_filter_by_share_server_and_all_tenants(self):
         # NOTE(vponomaryov): if share_server_id provided, 'all_tenants' opt
@@ -326,10 +324,8 @@ class ShareAPITestCase(test.TestCase):
         self.mock_object(db_api, 'share_get_all_by_project')
         shares = self.api.get_all(
             ctx, {'share_server_id': 'fake_server_3', 'all_tenants': 1})
-        share_api.policy.check_policy.assert_has_calls([
+        share_api.policy.check_policy.assert_called_once_with(
-            mock.call(ctx, 'share', 'get_all'),
+            ctx, 'share', 'list_by_share_server_id')
-            mock.call(ctx, 'share', 'list_by_share_server_id'),
-        ])
         db_api.share_get_all_by_share_server.assert_called_once_with(
             ctx, 'fake_server_3', sort_dir='desc', sort_key='created_at',
             filters={},
@@ -344,10 +340,9 @@ class ShareAPITestCase(test.TestCase):
             db_api, 'share_get_all_by_project',
             mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[1::2]))
         expected_filters = {'display_name': 'bar'}
+
         shares = self.api.get_all(ctx, {'display_name': 'bar'})
-        share_api.policy.check_policy.assert_has_calls([
+
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_2', filters=expected_filters, is_public=False
@@ -377,9 +372,6 @@ class ShareAPITestCase(test.TestCase):
         expected_filters = copy.copy(search_opts)
 
         shares = self.api.get_all(ctx, search_opts)
-        share_api.policy.check_policy.assert_has_calls([
-            mock.call(ctx, 'share', 'get_all'),
-        ])
 
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
@@ -394,10 +386,9 @@ class ShareAPITestCase(test.TestCase):
         ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=True)
         self.mock_object(db_api, 'share_get_all_by_project',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
+
         shares = self.api.get_all(ctx, {'export_location_' + type: 'test'})
-        share_api.policy.check_policy.assert_has_calls([
+
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_2',
@@ -409,10 +400,9 @@ class ShareAPITestCase(test.TestCase):
         ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=True)
         self.mock_object(db_api, 'share_get_all',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[:1]))
+
         shares = self.api.get_all(ctx, {'name': 'foo', 'all_tenants': 1})
-        share_api.policy.check_policy.assert_has_calls([
+
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at', filters={})
         self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[:1], shares)
@@ -425,9 +415,7 @@ class ShareAPITestCase(test.TestCase):
             mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0::2]))
 
         shares = self.api.get_all(ctx, {'status': constants.STATUS_AVAILABLE})
-        share_api.policy.check_policy.assert_has_calls([
+
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_2', filters=expected_filter, is_public=False
@@ -442,9 +430,6 @@ class ShareAPITestCase(test.TestCase):
         expected_filter = {'status': constants.STATUS_ERROR}
         shares = self.api.get_all(
             ctx, {'status': constants.STATUS_ERROR, 'all_tenants': 1})
-        share_api.policy.check_policy.assert_has_calls([
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             filters=expected_filter)
@@ -455,10 +440,9 @@ class ShareAPITestCase(test.TestCase):
         ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=False)
         self.mock_object(db_api, 'share_get_all_by_project',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
+
         shares = self.api.get_all(ctx, {'all_tenants': 1})
-        share_api.policy.check_policy.assert_has_calls([
+
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_2', filters={}, is_public=False
@@ -472,11 +456,10 @@ class ShareAPITestCase(test.TestCase):
             mock.Mock(side_effect=[
                 _FAKE_LIST_OF_ALL_SHARES[1::2],
                 _FAKE_LIST_OF_ALL_SHARES[2::4]]))
+
         shares = self.api.get_all(
             ctx, {'name': 'bar', 'status': constants.STATUS_ERROR})
-        share_api.policy.check_policy.assert_has_calls([
+
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         expected_filter_1 = {'status': constants.STATUS_ERROR}
         expected_filter_2 = {'status': constants.STATUS_AVAILABLE}
 
@@ -491,11 +474,8 @@ class ShareAPITestCase(test.TestCase):
         # one item expected, two filtered
         shares = self.api.get_all(
             ctx, {'name': 'foo1', 'status': constants.STATUS_AVAILABLE})
+
         self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[2::4], shares)
-        share_api.policy.check_policy.assert_has_calls([
-            mock.call(ctx, 'share', 'get_all'),
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all_by_project.assert_has_calls([
             mock.call(
                 ctx, sort_dir='desc', sort_key='created_at',
@@ -514,9 +494,6 @@ class ShareAPITestCase(test.TestCase):
         self.mock_object(db_api, 'share_get_all_by_project', mock.Mock(
             return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
         shares = self.api.get_all(ctx, {'is_public': is_public})
-        share_api.policy.check_policy.assert_has_calls([
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_2', filters={}, is_public=True
@@ -530,9 +507,6 @@ class ShareAPITestCase(test.TestCase):
         self.mock_object(db_api, 'share_get_all_by_project', mock.Mock(
             return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
         shares = self.api.get_all(ctx, {'is_public': is_public})
-        share_api.policy.check_policy.assert_has_calls([
-            mock.call(ctx, 'share', 'get_all'),
-        ])
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_2', filters={}, is_public=False
@@ -545,17 +519,14 @@ class ShareAPITestCase(test.TestCase):
                                      is_admin=False)
         self.assertRaises(ValueError, self.api.get_all,
                           ctx, {'is_public': is_public})
-        share_api.policy.check_policy.assert_has_calls([
-            mock.call(ctx, 'share', 'get_all'),
-        ])
 
     def test_get_all_with_sorting_valid(self):
         self.mock_object(db_api, 'share_get_all_by_project',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
         ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=False)
+
         shares = self.api.get_all(ctx, sort_key='status', sort_dir='asc')
-        share_api.policy.check_policy.assert_called_once_with(
+
-            ctx, 'share', 'get_all')
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='asc', sort_key='status',
             project_id='fake_pid_1', filters={}, is_public=False
@@ -572,8 +543,6 @@ class ShareAPITestCase(test.TestCase):
             ctx,
             sort_key=1,
         )
-        share_api.policy.check_policy.assert_called_once_with(
-            ctx, 'share', 'get_all')
 
     def test_get_all_sort_dir_invalid(self):
         self.mock_object(db_api, 'share_get_all_by_project',
@@ -585,23 +554,18 @@ class ShareAPITestCase(test.TestCase):
             ctx,
             sort_dir=1,
         )
-        share_api.policy.check_policy.assert_called_once_with(
-            ctx, 'share', 'get_all')
 
     def _get_all_filter_metadata_or_extra_specs_valid(self, key):
         self.mock_object(db_api, 'share_get_all_by_project',
                          mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
         ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=False)
         search_opts = {key: {'foo1': 'bar1', 'foo2': 'bar2'}}
+
         shares = self.api.get_all(ctx, search_opts=search_opts.copy())
+
         if key == 'extra_specs':
-            share_api.policy.check_policy.assert_has_calls([
-                mock.call(ctx, 'share', 'get_all'),
-                mock.call(ctx, 'share_types_extra_spec', 'index'),
-            ])
-        else:
             share_api.policy.check_policy.assert_called_once_with(
-                ctx, 'share', 'get_all')
+                ctx, 'share_types_extra_spec', 'index')
         db_api.share_get_all_by_project.assert_called_once_with(
             ctx, sort_dir='desc', sort_key='created_at',
             project_id='fake_pid_1', filters=search_opts, is_public=False)
@@ -621,13 +585,8 @@ class ShareAPITestCase(test.TestCase):
         self.assertRaises(exception.InvalidInput, self.api.get_all, ctx,
                           search_opts=search_opts)
         if key == 'extra_specs':
-            share_api.policy.check_policy.assert_has_calls([
-                mock.call(ctx, 'share', 'get_all'),
-                mock.call(ctx, 'share_types_extra_spec', 'index'),
-            ])
-        else:
             share_api.policy.check_policy.assert_called_once_with(
-                ctx, 'share', 'get_all')
+                ctx, 'share_types_extra_spec', 'index')
 
     def test_get_all_filter_by_invalid_metadata(self):
         self._get_all_filter_metadata_or_extra_specs_invalid(key='metadata')