Some APIs return None when missing permissions instead of raising 403

[Felipe Monteiro]

Steps to reproduce:
------------------

1) Update Cinder's policy "volume_extension:volume_image_metadata"
   to "rule:admin_api" in policy.json
2) Call the os-unset_image_metadata or os-set_image_metadata API endpoints with a non-admin user.
3) None is returned.

Reason for bug:
---------------

For os-unset_image_metadata it is apparent from reading code [0]
that None is returned instead of a 403 being raised.

This leads to strange behavior in, say, Tempest, where it expects a dictionary to be returned, but instead receives None from the API.

Most APIs in Cinder only use fatal=False in context.authorize() for extending a response body (i.e. adding additional information to it). But for these examples it affects the entire response body being returned or not.

Places affected:
----------------
os-unset_image_metadata [0]
os-set_image_metadata [1]

Used this to look for others but didn't find any: http://codesearch.openstack.org/?q=if%20context.authorize&i=nope&files=&repos=cinder

Example stacktrace:
-------------------

Traceback (most recent call last):
  File "/opt/stack/patrole/patrole_tempest_plugin/rbac_rule_validation.py", line 169, in wrapper
    LOG.error(msg)
  File "/usr/local/lib/python2.7/dist-packages/oslo_utils/excutils.py", line 220, in __exit__
    self.force_reraise()
  File "/usr/local/lib/python2.7/dist-packages/oslo_utils/excutils.py", line 196, in force_reraise
    six.reraise(self.type_, self.value, self.tb)
  File "/opt/stack/patrole/patrole_tempest_plugin/rbac_rule_validation.py", line 142, in wrapper
    test_func(*args, **kwargs)
  File "/opt/stack/patrole/patrole_tempest_plugin/tests/api/volume/test_volume_metadata_rbac.py", line 161, in test_update_volume_image_metadata
    self.volume['id'], image_id=self.image_id)
  File "tempest/lib/services/volume/v2/volumes_client.py", line 321, in update_volume_image_metadata
    body = json.loads(body)
  File "/usr/local/lib/python2.7/dist-packages/oslo_serialization/jsonutils.py", line 260, in loads
    return json.loads(encodeutils.safe_decode(s, encoding), **kwargs)
  File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 382, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded

[0] https://github.com/openstack/cinder/blob/0cf910d4345c000e8c306b1cb2b2dd291975cf71/cinder/api/contrib/volume_image_metadata.py#L131
[1] https://github.com/openstack/cinder/blob/0cf910d4345c000e8c306b1cb2b2dd291975cf71/cinder/api/contrib/volume_image_metadata.py#L88

[Jay Bryant (jsbryant)]

This was discussed in today's IRC meeting and there was agreement that this is a bug. We should be returning a 403. So, Felipe is going to work on a fix.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/527838

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/527838
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=b5f6c2864f5ca829854af5c12f37a3d49ccc9d5f
Submitter: Zuul
Branch: master

commit b5f6c2864f5ca829854af5c12f37a3d49ccc9d5f
Author: Felipe Monteiro <email address hidden>
Date: Thu Dec 14 02:24:29 2017 +0000

    Fix volume image metadata endpoints returning None

    This commit fixes the following volume image metadata
    endpoints returning None following policy enforcement
    failure:

      * ``os-set_image_metadata``
      * ``os-unset_image_metadata``

    The endpoints will now correctly raise a 403 Forbidden
    instead.

    The kwarg `fatal=False` was dropped from
    `context.authorize` for these APIs because the kwarg
    is only useful when adding additional information to
    the response body (if the user is authorized).

    This commit:

      * makes the fix for the two endpoints above
      * adds unit tests for validating the new, correct
        behavior (as a side note, policy overriding
        in tests can be more easily accomplished via
        adoption of something like [0])

    Also note that since the default policy rule
    for these endpoints is "admin_or_owner" Tempest
    doesn't validate this behavior by default.

    [0] https://github.com/openstack/nova/blob/e599b13e4940fb9654f0e0c0f43077a6979eaabe/nova/tests/unit/policy_fixture.py#L30

    Change-Id: Icc286d529609165e5f14cb506342660d7bc2ae9f
    Closes-Bug: #1737609

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/cinder 12.0.0.0b3 development milestone.