openstack/nova
Code Issues Proposed changes

fix os-volume_attachments policy checks

Browse Source 
The os-volume_attachments APIs have their own policy settings defined,
yet were also checking the policy settings defined for the os-volumes
APIs. This should never have been the case, but especially not now
that the os-volumes APIs are deprecated and don't even work anymore
with newer microversions. This change removes the os-volumes policy
checks for os-volume_attachment API requests. The code will continue
to make os-volumes policy checks for os-volumes APIs, and
os-volume_attachment policy checks for os-volume_attachment APIs.

Removed the _items method, which was only being called from one place,
to resolve comments that policy checks should always happen immediately
upon entering the API methods.

Change-Id: I35aaedf5c4c49cb568fa06c2974f9a35aa2ffcc5
Closes-Bug: #1635358
UpgradeImpact
This commit is contained in:
Matthew Edmonds 2016-10-20 14:26:35 -04:00
parent b672f35de6
commit 4aa55f3edf
3 changed files with 31 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
nova/api/openstack/compute/volumes.py
View File

@ -266,14 +266,27 @@ class VolumeAttachmentController(wsgi.Controller):
"""Returns the list of volume attachments for a given instance."""
context = req.environ['nova.context']
context.can(va_policies.POLICY_ROOT % 'index')
return self._items(req, server_id,
entity_maker=_translate_attachment_summary_view)
instance = common.get_instance(self.compute_api, context, server_id)
bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
context, instance.uuid)
limited_list = common.limited(bdms, req)
results = []
for bdm in limited_list:
if bdm.volume_id:
va = _translate_attachment_summary_view(bdm.volume_id,
bdm.instance_uuid,
bdm.device_name)
results.append(va)
return {'volumeAttachments': results}
@extensions.expected_errors(404)
def show(self, req, server_id, id):
"""Return data about the given volume attachment."""
context = req.environ['nova.context']
context.can(vol_policies.BASE_POLICY_NAME)
context.can(va_policies.POLICY_ROOT % 'show')
volume_id = id
@ -308,7 +321,6 @@ class VolumeAttachmentController(wsgi.Controller):
def create(self, req, server_id, body):
"""Attach a volume to an instance."""
context = req.environ['nova.context']
context.can(vol_policies.BASE_POLICY_NAME)
context.can(va_policies.POLICY_ROOT % 'create')
volume_id = body['volumeAttachment']['volumeId']
@ -362,7 +374,6 @@ class VolumeAttachmentController(wsgi.Controller):
@validation.schema(volumes_schema.update_volume_attachment)
def update(self, req, server_id, id, body):
context = req.environ['nova.context']
context.can(vol_policies.BASE_POLICY_NAME)
context.can(va_policies.POLICY_ROOT % 'update')
old_volume_id = id
@ -420,7 +431,6 @@ class VolumeAttachmentController(wsgi.Controller):
def delete(self, req, server_id, id):
"""Detach a volume from an instance."""
context = req.environ['nova.context']
context.can(vol_policies.BASE_POLICY_NAME)
context.can(va_policies.POLICY_ROOT % 'delete')
volume_id = id
@ -474,26 +484,6 @@ class VolumeAttachmentController(wsgi.Controller):
msg = _("volume_id not found: %s") % volume_id
raise exc.HTTPNotFound(explanation=msg)
def _items(self, req, server_id, entity_maker):
"""Returns a list of attachments, transformed through entity_maker."""
context = req.environ['nova.context']
context.can(vol_policies.BASE_POLICY_NAME)
instance = common.get_instance(self.compute_api, context, server_id)
bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
context, instance.uuid)
limited_list = common.limited(bdms, req)
results = []
for bdm in limited_list:
if bdm.volume_id:
results.append(entity_maker(bdm.volume_id,
bdm.instance_uuid,
bdm.device_name))
return {'volumeAttachments': results}
def _translate_snapshot_detail_view(context, vol):
"""Maps keys for snapshots details view."""

42
nova/tests/unit/api/openstack/compute/test_volumes.py
View File

@ -886,57 +886,29 @@ class TestVolumeAttachPolicyEnforcementV21(test.NoDBTestCase):
self.controller.index, self.req, FAKE_UUID)
def test_show_volume_attach_policy_failed(self):
rule_name = "os_compute_api:os-volumes"
rules = {"os_compute_api:os-volumes-attachments:show": "@",
rule_name: "project:non_fake"}
self._common_policy_check(rules, rule_name, self.controller.show,
self.req, FAKE_UUID, FAKE_UUID_A)
rule_name = "os_compute_api:os-volumes-attachments:show"
rules = {"os_compute_api:os-volumes": "@",
rule_name: "project:non_fake"}
rules = {rule_name: "project:non_fake"}
self._common_policy_check(rules, rule_name, self.controller.show,
self.req, FAKE_UUID, FAKE_UUID_A)
def test_create_volume_attach_policy_failed(self):
rule_name = "os_compute_api:os-volumes"
rules = {"os_compute_api:os-volumes-attachments:create": "@",
rule_name: "project:non_fake"}
body = {'volumeAttachment': {'volumeId': FAKE_UUID_A,
'device': '/dev/fake'}}
self._common_policy_check(rules, rule_name, self.controller.create,
self.req, FAKE_UUID, body=body)
rule_name = "os_compute_api:os-volumes-attachments:create"
rules = {"os_compute_api:os-volumes": "@",
rule_name: "project:non_fake"}
rules = {rule_name: "project:non_fake"}
body = {'volumeAttachment': {'volumeId': FAKE_UUID_A,
'device': '/dev/fake'}}
self._common_policy_check(rules, rule_name, self.controller.create,
self.req, FAKE_UUID, body=body)
def test_update_volume_attach_policy_failed(self):
rule_name = "os_compute_api:os-volumes"
rules = {"os_compute_api:os-volumes-attachments:update": "@",
rule_name: "project:non_fake"}
rule_name = "os_compute_api:os-volumes-attachments:update"
rules = {rule_name: "project:non_fake"}
body = {'volumeAttachment': {'volumeId': FAKE_UUID_B}}
self._common_policy_check(rules, rule_name, self.controller.update,
self.req, FAKE_UUID, FAKE_UUID_A, body=body)
rule_name = "os_compute_api:os-volumes-attachments:update"
rules = {"os_compute_api:os-volumes": "@",
rule_name: "project:non_fake"}
self._common_policy_check(rules, rule_name, self.controller.update,
self.req, FAKE_UUID, FAKE_UUID_A, body=body)
def test_delete_volume_attach_policy_failed(self):
rule_name = "os_compute_api:os-volumes"
rules = {"os_compute_api:os-volumes-attachments:delete": "@",
rule_name: "project:non_fake"}
self._common_policy_check(rules, rule_name, self.controller.delete,
self.req, FAKE_UUID, FAKE_UUID_A)
rule_name = "os_compute_api:os-volumes-attachments:delete"
rules = {"os_compute_api:os-volumes": "@",
rule_name: "project:non_fake"}
rules = {rule_name: "project:non_fake"}
self._common_policy_check(rules, rule_name, self.controller.delete,
self.req, FAKE_UUID, FAKE_UUID_A)

8
releasenotes/notes/bug-volume-attach-policy-1635358-671ce4d4ee8c211b.yaml Normal file
View File

@ -0,0 +1,8 @@
---
upgrade:
- |
The os-volume_attachments APIs no longer check
``os_compute_api:os-volumes`` policy. They do still check
``os_compute_api:os-volumes-attachments`` policy rules. Deployers
who have customized policy should confirm that their settings for
os-volume_attachments policy checks are sufficient.