Merge "[RBAC] Enforce check for share updates"

manila/api/openstack/wsgi.py

@@ -1237,8 +1237,17 @@ class AdminActionsMixin(object):
         return update
 
     @Controller.authorize('reset_status')
-    def _reset_status(self, req, id, body, status_attr='status'):
+    def _reset_status(self, req, id, body, status_attr='status',
-        """Reset the status_attr specified on the resource."""
+                      resource=None):
+        """Reset the status_attr specified on the resource.
+
+        :param req: API request object
+        :param id: ID of the resource
+        :param body: API request body
+        :param status_attr: Attribute on the resource denoting the status
+                            to be reset
+        :param resource: Resource model or dict if we need to avoid fetching it
+        """
         context = req.environ['manila.context']
         body_attr = self.body_attributes[status_attr]
         update = self.validate_update(
@@ -1248,9 +1257,17 @@ class AdminActionsMixin(object):
         LOG.debug(msg, {'resource': self.resource_name, 'id': id,
                         'update': update})
         try:
-            self._update(context, id, update)
+            resource = resource or self._get(context, id)
         except exception.NotFound as e:
             raise webob.exc.HTTPNotFound(e.message)
+        try:
+            policy.check_policy(context,
+                                self.resource_name,
+                                "reset_status",
+                                target_obj=resource)
+        except exception.NotAuthorized as e:
+            raise webob.exc.HTTPForbidden(e.message)
+        self._update(context, id, update)
         return webob.Response(status_int=http_client.ACCEPTED)
 
     @Controller.authorize('force_delete')
@@ -1261,6 +1278,10 @@ class AdminActionsMixin(object):
             resource = self._get(context, id)
         except exception.NotFound as e:
             raise webob.exc.HTTPNotFound(e.message)
+        policy.check_policy(context,
+                            self.resource_name,
+                            "force_delete",
+                            target_obj=resource)
         self._delete(context, resource, force=True)
         return webob.Response(status_int=http_client.ACCEPTED)
 

manila/api/v2/share_networks.py

@@ -59,6 +59,9 @@ class ShareNetworkController(wsgi.Controller, wsgi.AdminActionsMixin):
         'status': set(constants.SHARE_NETWORK_STATUSES)
     }
 
+    def _get(self, *args, **kwargs):
+        return db_api.share_network_get(*args, **kwargs)
+
     def show(self, req, id):
         """Return data about the requested network info."""
         context = req.environ['manila.context']

manila/api/v2/share_servers.py

@@ -47,6 +47,9 @@ class ShareServerController(share_servers.ShareServerController,
         'task_state': set(constants.SERVER_TASK_STATE_STATUSES),
     }
 
+    def _get(self, *args, **kwargs):
+        return db_api.share_server_get(*args, **kwargs)
+
     def _update(self, context, id, update):
         db_api.share_server_update(context, id, update)
 

manila/api/v2/share_snapshot_instances.py

@@ -81,6 +81,9 @@ class ShareSnapshotInstancesController(wsgi.Controller,
     def _update(self, *args, **kwargs):
         db.share_snapshot_instance_update(*args, **kwargs)
 
+    def _get(self, *args, **kwargs):
+        db.share_snapshot_instance_get(*args, **kwargs)
+
 
 def create_resource():
     return wsgi.Resource(ShareSnapshotInstancesController())

manila/api/v2/shares.py

@@ -233,12 +233,12 @@ class ShareController(wsgi.Controller,
         try:
             share = self.share_api.get(context, id)
         except exception.NotFound:
-            raise exception.ShareNotFound(share_id=id)
+            raise exc.HTTPNotFound("Share %s not found" % id)
         if share.get('is_soft_deleted'):
             msg = _("status cannot be reset for share '%s' "
                     "since it has been soft deleted.") % id
             raise exc.HTTPForbidden(explanation=msg)
-        return self._reset_status(req, id, body)
+        return self._reset_status(req, id, body, resource=share)
 
     @wsgi.Controller.api_version('2.7')
     @wsgi.action('reset_status')
@@ -248,12 +248,12 @@ class ShareController(wsgi.Controller,
         try:
             share = self.share_api.get(context, id)
         except exception.NotFound:
-            raise exception.ShareNotFound(share_id=id)
+            raise exc.HTTPNotFound("Share %s not found" % id)
         if share.get('is_soft_deleted'):
             msg = _("status cannot be reset for share '%s' "
                     "since it has been soft deleted.") % id
             raise exc.HTTPForbidden(explanation=msg)
-        return self._reset_status(req, id, body)
+        return self._reset_status(req, id, body, resource=share)
 
     @wsgi.Controller.api_version('2.0', '2.6')
     @wsgi.action('os-force_delete')
@@ -455,7 +455,8 @@ class ShareController(wsgi.Controller,
             msg = _("task state cannot be reset for share '%s' "
                     "since it has been soft deleted.") % id
             raise exc.HTTPForbidden(explanation=msg)
-        return self._reset_status(req, id, body, status_attr='task_state')
+        return self._reset_status(req, id, body, status_attr='task_state',
+                                  resource=share)
 
     @wsgi.Controller.api_version('2.0', '2.6')
     @wsgi.action('os-allow_access')

manila/tests/api/v2/test_share_servers.py

@@ -54,14 +54,22 @@ class ShareServerControllerTest(test.TestCase):
         body = {'reset_status': {'status': status}}
 
         context = req.environ['manila.context']
+        self.mock_object(self.controller, '_get', mock.Mock(
+            return_value={'share_server': 'object'}))
         mock_update = self.mock_object(db_api, 'share_server_update')
 
         result = self.controller.share_server_reset_status(
             req, 'fake_server_id', body)
 
         self.assertEqual(202, result.status_int)
-        policy.check_policy.assert_called_once_with(
+        policy.check_policy.assert_has_calls([
-            context, self.resource_name, 'reset_status')
+            mock.call(context, self.resource_name, 'reset_status'),
+            mock.call(context,
+                      self.resource_name,
+                      'reset_status',
+                      target_obj={'share_server': 'object'}),
+        ])
+
         mock_update.assert_called_once_with(
             context, 'fake_server_id', {'status': status})
 
@@ -881,16 +889,18 @@ class ShareServerControllerTest(test.TestCase):
         update = {'task_state': constants.TASK_STATE_MIGRATION_ERROR}
         body = {'reset_task_state': update}
 
-        self.mock_object(db_api, 'share_server_update',
+        self.mock_object(db_api, 'share_server_get',
                          mock.Mock(side_effect=exception.ShareServerNotFound(
                                    share_server_id='fake_server_id')))
+        self.mock_object(db_api, 'share_server_update')
 
         self.assertRaises(webob.exc.HTTPNotFound,
                           self.controller.share_server_reset_task_state,
                           req, server['id'], body)
 
-        db_api.share_server_update.assert_called_once_with(utils.IsAMatcher(
+        db_api.share_server_get.assert_called_once_with(utils.IsAMatcher(
-            ctx_api.RequestContext), server['id'], update)
+            ctx_api.RequestContext), server['id'])
+        db_api.share_server_update.assert_not_called()
 
     def test_share_server_migration_complete(self):
         server = db_utils.create_share_server(

manila/tests/api/v2/test_shares.py

@@ -1138,15 +1138,42 @@ class ShareAPITest(test.TestCase):
         update = {'task_state': constants.TASK_STATE_MIGRATION_ERROR}
         body = {'reset_task_state': update}
 
-        self.mock_object(db, 'share_update',
+        self.mock_object(share_api.API, 'get',
-                         mock.Mock(side_effect=exception.NotFound()))
+                         mock.Mock(side_effect=exception.NotFound))
+        self.mock_object(db, 'share_update')
 
-        self.assertRaises(webob.exc.HTTPNotFound,
+        self.assertRaises(exception.NotFound,
                           self.controller.reset_task_state, req, share['id'],
                           body)
 
-        db.share_update.assert_called_once_with(utils.IsAMatcher(
+        share_api.API.get.assert_called_once_with(utils.IsAMatcher(
-            context.RequestContext), share['id'], update)
+            context.RequestContext), share['id'])
+        db.share_update.assert_not_called()
+
+    def test_reset_task_state_share_other_project_public_share(self):
+        share = db_utils.create_share(is_public=True)
+        req = fakes.HTTPRequest.blank(
+            '/v2/fake/shares/%s/action' % share['id'],
+            use_admin_context=True, version=LATEST_MICROVERSION)
+        req.method = 'POST'
+        req.headers['content-type'] = 'application/json'
+        req.api_version_request.experimental = True
+        update = {'task_state': constants.TASK_STATE_MIGRATION_ERROR}
+        body = {'reset_task_state': update}
+
+        # NOTE(gouthamr): we're testing a scenario where someone has access
+        # to the RBAC rule share:reset_task_state, but doesn't own the share.
+        # Ideally we'd override the default policy, but it's a shared
+        # resource and we'll bleed into other tests, so we'll mock the
+        # policy check to return False instead
+        rbac_checks = [None, None, exception.NotAuthorized]
+        with mock.patch.object(policy, 'check_policy',
+                               side_effect=rbac_checks):
+            self.mock_object(share_api.API, 'get',
+                             mock.Mock(return_value=share))
+            self.assertRaises(webob.exc.HTTPForbidden,
+                              self.controller.reset_task_state,
+                              req, share['id'], body)
 
     def test_reset_task_state_share_has_been_soft_deleted(self):
         share = self.share_in_recycle_bin
@@ -2717,19 +2744,18 @@ class ShareAdminActionsAPITest(test.TestCase):
         req.headers['X-Openstack-Manila-Api-Version'] = version
         req.body = jsonutils.dumps(body).encode("utf-8")
         req.environ['manila.context'] = ctxt
-        self.mock_object(share_api.API, 'get', mock.Mock(return_value=model))
 
-        resp = req.get_response(fakes.app())
+        resp = req.get_response(fakes.app(), catch_exc_info=True)
 
         # validate response code and model status
         self.assertEqual(valid_code, resp.status_int)
 
-        if valid_code == 404:
+        if valid_code == 404 and db_access_method is not None:
             self.assertRaises(exception.NotFound,
                               db_access_method,
                               ctxt,
                               model['id'])
-        else:
+        elif db_access_method:
             actual_model = db_access_method(ctxt, model['id'])
             self.assertEqual(valid_status, actual_model['status'])
 
@@ -2739,6 +2765,7 @@ class ShareAdminActionsAPITest(test.TestCase):
                                                      valid_status, version):
         share, req = self._setup_share_data(version=version)
         ctxt = self._get_context(role)
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
 
         self._reset_status(ctxt, share, req, db.share_get, valid_code,
                            valid_status, version=version)
@@ -2747,6 +2774,7 @@ class ShareAdminActionsAPITest(test.TestCase):
     def test_share_invalid_reset_status_body(self, body):
         share, req = self._setup_share_data(version='2.6')
         ctxt = self.admin_context
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
 
         self._reset_status(ctxt, share, req, db.share_get, 400,
                            constants.STATUS_AVAILABLE, body, version='2.6')
@@ -2758,7 +2786,23 @@ class ShareAdminActionsAPITest(test.TestCase):
             '/v2/fake/shares/%s/action' % fake_share['id'], version=version)
 
         self._reset_status(self.admin_context, fake_share, req,
-                           db.share_snapshot_get, 404, version=version)
+                           db.share_get, 404, version=version)
+
+    @ddt.data('2.6', '2.7')
+    def test_reset_status_other_project_public_share(self, version):
+        # NOTE(gouthamr): we're testing a scenario where someone has access
+        # to the RBAC rule share:reset_status, but doesn't own the share.
+        # Ideally we'd override the default policy, but it's a shared
+        # resource and we'll bleed into other tests, so we'll mock the
+        # policy check to return False instead
+        share, req = self._setup_share_data(version=version)
+        share['is_public'] = True
+        rbac_checks = [None, exception.NotAuthorized]
+        with mock.patch.object(policy, 'authorize', side_effect=rbac_checks):
+            self.mock_object(share_api.API, 'get',
+                             mock.Mock(return_value=share))
+            self._reset_status(
+                self.member_context, share, req, None, 403, version=version)
 
     def _force_delete(self, ctxt, model, req, db_access_method, valid_code,
                       check_model_in_db=False, version='2.7'):

releasenotes/notes/bug-1955627-add-check-to-reset-status-baa126a7145a45bb.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Role based access control is enforced on the POST /shares/{share_id}/action
+    API to reset status, task state, replica state and similar fields. This
+    prevents the situation where deployments allow some users access to
+    these APIs, but they don't belong to projects where the resources exist.
+    See `bug 1955627 <https://launchpad.net/bugs/1955627>`_ for more context.