From 429f16124b7cd69c85e6d764bcf1e47687780286 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Tue, 6 Jul 2021 11:21:15 -0700 Subject: [PATCH] Remove dead 403->404 code The db.ImageRepo.save() operation covers up a Forbidden error with NotFound in the case of trying to update an image that the user does not own. This is actually never hit in reality as the authorization layer would have caught it before, and returned Forbidden. The API is the layer that should be deciding to hide images for which the user cannot see, to avoid things like being able to show an image, but get back a 404 on update. In order to do that, we need the lower layers to report the situation faithfully and let the upper layers decide how to expose that to the user. Specifically, for the policy refactor, we have tests that assert the Forbidden behavior, and after disabling the authorization layer, will break (and rightfully so) because they get NotFound when they hit the lower-layer check. Because it is hidden so deep, even the API can't distinguish between the two. I imagine this check was added long ago to provide the "if you can't see it, return NotFound instead of Forbidden, which would expose the fact that it exists" behavior which is desired. However, the authorization layer means we never get here anymore. This patch is provided without any test changes to prove that it does not actually alter the real behavior. The test_permissions functional test asserts the proper behavior before and after this change, as well as after subsequent refactor patches. Change-Id: I0084350ebb09cc1cb3752b45165e49f166bfdf91 --- glance/db/__init__.py | 18 +++++++----------- glance/db/simple/api.py | 2 +- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/glance/db/__init__.py b/glance/db/__init__.py index 65f35729ce..144af0c20d 100644 --- a/glance/db/__init__.py +++ b/glance/db/__init__.py @@ -182,17 +182,13 @@ class ImageRepo(object): if (image_values['size'] is not None and image_values['size'] > CONF.image_size_cap): raise exception.ImageSizeLimitExceeded - try: - new_values = self.db_api.image_update(self.context, - image.image_id, - image_values, - purge_props=True, - from_state=from_state, - atomic_props=( - IMAGE_ATOMIC_PROPS)) - except (exception.ImageNotFound, exception.Forbidden): - msg = _("No image found with ID %s") % image.image_id - raise exception.ImageNotFound(msg) + new_values = self.db_api.image_update(self.context, + image.image_id, + image_values, + purge_props=True, + from_state=from_state, + atomic_props=( + IMAGE_ATOMIC_PROPS)) self.db_api.image_tag_set_all(self.context, image.image_id, image.tags) image.updated_at = new_values['updated_at'] diff --git a/glance/db/simple/api.py b/glance/db/simple/api.py index f710c20d6c..1a0096ba22 100644 --- a/glance/db/simple/api.py +++ b/glance/db/simple/api.py @@ -804,7 +804,7 @@ def image_update(context, image_id, image_values, purge_props=False, try: image = DATA['images'][image_id] except KeyError: - raise exception.ImageNotFound() + raise exception.ImageNotFound(image_id) location_data = image_values.pop('locations', None) if location_data is not None: