1 This fix will be included in future 2015.1.3 (kilo) and 11.0.2 |
|
2 (liberty) releases. |
|
3 |
|
4 From c5c731c7153d6d46c27260474d2811d504dfac5c Mon Sep 17 00:00:00 2001 |
|
5 From: Erno Kuvaja <[email protected]> |
|
6 Date: Tue, 19 Jan 2016 13:37:05 +0000 |
|
7 Subject: [PATCH] Prevent user to remove last location of the image |
|
8 |
|
9 If the last location of the image is removed, image transitions back to queued. |
|
10 This allows user to upload new data into the existing image record. By |
|
11 preventing removal of the last location we prevent the image transition back to |
|
12 queued. |
|
13 |
|
14 This change also prevents doing the same operation via replacing the locations |
|
15 with empty list. |
|
16 |
|
17 SecurityImpact |
|
18 DocImpact |
|
19 APIImpact |
|
20 |
|
21 Conflicts: |
|
22 glance/tests/unit/v2/test_images_resource.py |
|
23 |
|
24 Conflicts: |
|
25 glance/api/v2/images.py |
|
26 |
|
27 Change-Id: Ieb03aaba887492819f9c58aa67f7acfcea81720e |
|
28 Closes-Bug: #1525915 |
|
29 (cherry picked from commit e9e45baa9aaf58e69964419b6b4fb2048d115a0c) |
|
30 --- |
|
31 glance/api/v2/images.py | 19 +++- |
|
32 glance/tests/functional/v2/test_images.py | 14 --- |
|
33 glance/tests/unit/v2/test_images_resource.py | 122 ++++----------------- |
|
34 ...oving-last-image-location-d5ee3e00efe14f34.yaml | 10 ++ |
|
35 4 files changed, 44 insertions(+), 121 deletions(-) |
|
36 create mode 100644 releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml |
|
37 |
|
38 diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py |
|
39 index 22c8e70..90435ac 100644 |
|
40 --- a/glance/api/v2/images.py |
|
41 +++ b/glance/api/v2/images.py |
|
42 @@ -168,7 +168,10 @@ class ImagesController(object): |
|
43 path = change['path'] |
|
44 path_root = path[0] |
|
45 value = change['value'] |
|
46 - if path_root == 'locations': |
|
47 + if path_root == 'locations' and value == []: |
|
48 + msg = _("Cannot set locations to empty list.") |
|
49 + raise webob.exc.HTTPForbidden(message=msg) |
|
50 + elif path_root == 'locations' and value != []: |
|
51 self._do_replace_locations(image, value) |
|
52 else: |
|
53 if hasattr(image, path_root): |
|
54 @@ -201,7 +204,10 @@ class ImagesController(object): |
|
55 path = change['path'] |
|
56 path_root = path[0] |
|
57 if path_root == 'locations': |
|
58 - self._do_remove_locations(image, path[1]) |
|
59 + try: |
|
60 + self._do_remove_locations(image, path[1]) |
|
61 + except exception.Forbidden as e: |
|
62 + raise webob.exc.HTTPForbidden(e.msg) |
|
63 else: |
|
64 if hasattr(image, path_root): |
|
65 msg = _("Property %s may not be removed.") |
|
66 @@ -285,6 +291,11 @@ class ImagesController(object): |
|
67 explanation=utils.exception_to_str(ve)) |
|
68 |
|
69 def _do_remove_locations(self, image, path_pos): |
|
70 + if len(image.locations) == 1: |
|
71 + LOG.debug("User forbidden to remove last location of image %s", |
|
72 + image.image_id) |
|
73 + msg = _("Cannot remove last location in the image.") |
|
74 + raise exception.Forbidden(message=msg) |
|
75 pos = self._get_locations_op_pos(path_pos, |
|
76 len(image.locations), False) |
|
77 if pos is None: |
|
78 @@ -294,11 +305,11 @@ class ImagesController(object): |
|
79 # NOTE(zhiyan): this actually deletes the location |
|
80 # from the backend store. |
|
81 image.locations.pop(pos) |
|
82 + # TODO(jokke): Fix this, we should catch what store throws and |
|
83 + # provide definitely something else than IternalServerError to user. |
|
84 except Exception as e: |
|
85 raise webob.exc.HTTPInternalServerError( |
|
86 explanation=utils.exception_to_str(e)) |
|
87 - if len(image.locations) == 0 and image.status == 'active': |
|
88 - image.status = 'queued' |
|
89 |
|
90 |
|
91 class RequestDeserializer(wsgi.JSONRequestDeserializer): |
|
92 diff --git a/glance/tests/functional/v2/test_images.py b/glance/tests/functional/v2/test_images.py |
|
93 index 35d3ab2..c27c27f 100644 |
|
94 --- a/glance/tests/functional/v2/test_images.py |
|
95 +++ b/glance/tests/functional/v2/test_images.py |
|
96 @@ -489,20 +489,6 @@ class TestImages(functional.FunctionalTest): |
|
97 response = requests.patch(path, headers=headers, data=data) |
|
98 self.assertEqual(200, response.status_code, response.text) |
|
99 |
|
100 - # Remove all locations of the image then the image size shouldn't be |
|
101 - # able to access |
|
102 - path = self._url('/v2/images/%s' % image2_id) |
|
103 - media_type = 'application/openstack-images-v2.1-json-patch' |
|
104 - headers = self._headers({'content-type': media_type}) |
|
105 - doc = [{'op': 'replace', 'path': '/locations', 'value': []}] |
|
106 - data = jsonutils.dumps(doc) |
|
107 - response = requests.patch(path, headers=headers, data=data) |
|
108 - self.assertEqual(200, response.status_code, response.text) |
|
109 - image = jsonutils.loads(response.text) |
|
110 - self.assertIsNone(image['size']) |
|
111 - self.assertIsNone(image['virtual_size']) |
|
112 - self.assertEqual('queued', image['status']) |
|
113 - |
|
114 # Deletion should work. Deleting image-1 |
|
115 path = self._url('/v2/images/%s' % image_id) |
|
116 response = requests.delete(path, headers=self._headers()) |
|
117 diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py |
|
118 index be4e60a..9cc1f25 100644 |
|
119 --- a/glance/tests/unit/v2/test_images_resource.py |
|
120 +++ b/glance/tests/unit/v2/test_images_resource.py |
|
121 @@ -1323,26 +1323,6 @@ class TestImagesController(base.IsolatedUnitTest): |
|
122 self.assertRaises(webob.exc.HTTPConflict, self.controller.update, |
|
123 another_request, created_image.image_id, changes) |
|
124 |
|
125 - def test_update_replace_locations(self): |
|
126 - self.stubs.Set(store, 'get_size_from_backend', |
|
127 - unit_test_utils.fake_get_size_from_backend) |
|
128 - request = unit_test_utils.get_fake_request() |
|
129 - changes = [{'op': 'replace', 'path': ['locations'], 'value': []}] |
|
130 - output = self.controller.update(request, UUID1, changes) |
|
131 - self.assertEqual(UUID1, output.image_id) |
|
132 - self.assertEqual(0, len(output.locations)) |
|
133 - self.assertEqual('queued', output.status) |
|
134 - self.assertIsNone(output.size) |
|
135 - |
|
136 - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} |
|
137 - changes = [{'op': 'replace', 'path': ['locations'], |
|
138 - 'value': [new_location]}] |
|
139 - output = self.controller.update(request, UUID1, changes) |
|
140 - self.assertEqual(UUID1, output.image_id) |
|
141 - self.assertEqual(1, len(output.locations)) |
|
142 - self.assertEqual(new_location, output.locations[0]) |
|
143 - self.assertEqual('active', output.status) |
|
144 - |
|
145 def test_update_replace_locations_non_empty(self): |
|
146 new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} |
|
147 request = unit_test_utils.get_fake_request() |
|
148 @@ -1354,35 +1334,9 @@ class TestImagesController(base.IsolatedUnitTest): |
|
149 def test_update_replace_locations_invalid(self): |
|
150 request = unit_test_utils.get_fake_request() |
|
151 changes = [{'op': 'replace', 'path': ['locations'], 'value': []}] |
|
152 - output = self.controller.update(request, UUID1, changes) |
|
153 - self.assertEqual(UUID1, output.image_id) |
|
154 - self.assertEqual(0, len(output.locations)) |
|
155 - self.assertEqual('queued', output.status) |
|
156 - |
|
157 - request = unit_test_utils.get_fake_request() |
|
158 - changes = [{'op': 'replace', 'path': ['locations'], |
|
159 - 'value': [{'url': 'unknow://foo', 'metadata': {}}]}] |
|
160 - self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, |
|
161 + self.assertRaises(webob.exc.HTTPForbidden, self.controller.update, |
|
162 request, UUID1, changes) |
|
163 |
|
164 - def test_update_replace_locations_status_exception(self): |
|
165 - self.stubs.Set(store, 'get_size_from_backend', |
|
166 - unit_test_utils.fake_get_size_from_backend) |
|
167 - request = unit_test_utils.get_fake_request() |
|
168 - changes = [{'op': 'replace', 'path': ['locations'], 'value': []}] |
|
169 - output = self.controller.update(request, UUID2, changes) |
|
170 - self.assertEqual(UUID2, output.image_id) |
|
171 - self.assertEqual(0, len(output.locations)) |
|
172 - self.assertEqual('queued', output.status) |
|
173 - |
|
174 - self.db.image_update(None, UUID2, {'disk_format': None}) |
|
175 - |
|
176 - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} |
|
177 - changes = [{'op': 'replace', 'path': ['locations'], |
|
178 - 'value': [new_location]}] |
|
179 - self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, |
|
180 - request, UUID2, changes) |
|
181 - |
|
182 def test_update_add_property(self): |
|
183 request = unit_test_utils.get_fake_request() |
|
184 |
|
185 @@ -1506,24 +1460,6 @@ class TestImagesController(base.IsolatedUnitTest): |
|
186 self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, |
|
187 request, UUID1, changes) |
|
188 |
|
189 - def test_update_add_locations_status_exception(self): |
|
190 - self.stubs.Set(store, 'get_size_from_backend', |
|
191 - unit_test_utils.fake_get_size_from_backend) |
|
192 - request = unit_test_utils.get_fake_request() |
|
193 - changes = [{'op': 'replace', 'path': ['locations'], 'value': []}] |
|
194 - output = self.controller.update(request, UUID2, changes) |
|
195 - self.assertEqual(UUID2, output.image_id) |
|
196 - self.assertEqual(0, len(output.locations)) |
|
197 - self.assertEqual('queued', output.status) |
|
198 - |
|
199 - self.db.image_update(None, UUID2, {'disk_format': None}) |
|
200 - |
|
201 - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} |
|
202 - changes = [{'op': 'add', 'path': ['locations', '-'], |
|
203 - 'value': new_location}] |
|
204 - self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, |
|
205 - request, UUID2, changes) |
|
206 - |
|
207 def test_update_add_duplicate_locations(self): |
|
208 new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} |
|
209 request = unit_test_utils.get_fake_request() |
|
210 @@ -1537,23 +1473,6 @@ class TestImagesController(base.IsolatedUnitTest): |
|
211 self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, |
|
212 request, UUID1, changes) |
|
213 |
|
214 - def test_update_replace_duplicate_locations(self): |
|
215 - self.stubs.Set(store, 'get_size_from_backend', |
|
216 - unit_test_utils.fake_get_size_from_backend) |
|
217 - request = unit_test_utils.get_fake_request() |
|
218 - changes = [{'op': 'replace', 'path': ['locations'], 'value': []}] |
|
219 - output = self.controller.update(request, UUID1, changes) |
|
220 - self.assertEqual(UUID1, output.image_id) |
|
221 - self.assertEqual(0, len(output.locations)) |
|
222 - self.assertEqual('queued', output.status) |
|
223 - |
|
224 - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} |
|
225 - changes = [{'op': 'replace', 'path': ['locations'], |
|
226 - 'value': [new_location, new_location]}] |
|
227 - |
|
228 - self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, |
|
229 - request, UUID1, changes) |
|
230 - |
|
231 def test_update_add_too_many_locations(self): |
|
232 self.config(image_location_quota=1) |
|
233 request = unit_test_utils.get_fake_request() |
|
234 @@ -1654,9 +1573,12 @@ class TestImagesController(base.IsolatedUnitTest): |
|
235 {'op': 'add', 'path': ['locations', '-'], |
|
236 'value': {'url': '%s/fake_location_1' % BASE_URI, |
|
237 'metadata': {}}}, |
|
238 + {'op': 'add', 'path': ['locations', '-'], |
|
239 + 'value': {'url': '%s/fake_location_2' % BASE_URI, |
|
240 + 'metadata': {}}}, |
|
241 ] |
|
242 self.controller.update(request, UUID1, changes) |
|
243 - self.config(image_location_quota=1) |
|
244 + self.config(image_location_quota=2) |
|
245 |
|
246 # We must remove two properties to avoid being |
|
247 # over the limit of 1 property |
|
248 @@ -1669,8 +1591,8 @@ class TestImagesController(base.IsolatedUnitTest): |
|
249 ] |
|
250 output = self.controller.update(request, UUID1, changes) |
|
251 self.assertEqual(UUID1, output.image_id) |
|
252 - self.assertEqual(1, len(output.locations)) |
|
253 - self.assertIn('fake_location_3', output.locations[0]['url']) |
|
254 + self.assertEqual(2, len(output.locations)) |
|
255 + self.assertIn('fake_location_3', output.locations[1]['url']) |
|
256 self.assertNotEqual(output.created_at, output.updated_at) |
|
257 |
|
258 def test_update_remove_base_property(self): |
|
259 @@ -1711,24 +1633,23 @@ class TestImagesController(base.IsolatedUnitTest): |
|
260 unit_test_utils.fake_get_size_from_backend) |
|
261 |
|
262 request = unit_test_utils.get_fake_request() |
|
263 - changes = [{'op': 'remove', 'path': ['locations', '0']}] |
|
264 - output = self.controller.update(request, UUID1, changes) |
|
265 - self.assertEqual(output.image_id, UUID1) |
|
266 - self.assertEqual(0, len(output.locations)) |
|
267 - self.assertEqual('queued', output.status) |
|
268 - self.assertIsNone(output.size) |
|
269 - |
|
270 new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} |
|
271 changes = [{'op': 'add', 'path': ['locations', '-'], |
|
272 'value': new_location}] |
|
273 + self.controller.update(request, UUID1, changes) |
|
274 + changes = [{'op': 'remove', 'path': ['locations', '0']}] |
|
275 output = self.controller.update(request, UUID1, changes) |
|
276 self.assertEqual(UUID1, output.image_id) |
|
277 self.assertEqual(1, len(output.locations)) |
|
278 - self.assertEqual(new_location, output.locations[0]) |
|
279 self.assertEqual('active', output.status) |
|
280 |
|
281 def test_update_remove_location_invalid_pos(self): |
|
282 request = unit_test_utils.get_fake_request() |
|
283 + changes = [ |
|
284 + {'op': 'add', 'path': ['locations', '-'], |
|
285 + 'value': {'url': '%s/fake_location' % BASE_URI, |
|
286 + 'metadata': {}}}] |
|
287 + self.controller.update(request, UUID1, changes) |
|
288 changes = [{'op': 'remove', 'path': ['locations', None]}] |
|
289 self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, |
|
290 request, UUID1, changes) |
|
291 @@ -1750,6 +1671,11 @@ class TestImagesController(base.IsolatedUnitTest): |
|
292 fake_delete_image_location_from_backend) |
|
293 |
|
294 request = unit_test_utils.get_fake_request() |
|
295 + changes = [ |
|
296 + {'op': 'add', 'path': ['locations', '-'], |
|
297 + 'value': {'url': '%s/fake_location' % BASE_URI, |
|
298 + 'metadata': {}}}] |
|
299 + self.controller.update(request, UUID1, changes) |
|
300 changes = [{'op': 'remove', 'path': ['locations', '0']}] |
|
301 self.assertRaises(webob.exc.HTTPInternalServerError, |
|
302 self.controller.update, request, UUID1, changes) |
|
303 @@ -2036,16 +1962,6 @@ class TestImagesControllerPolicies(base.IsolatedUnitTest): |
|
304 self.assertRaises(webob.exc.HTTPForbidden, self.controller.update, |
|
305 request, UUID1, changes) |
|
306 |
|
307 - self.stubs.Set(self.store_utils, 'delete_image_location_from_backend', |
|
308 - fake_delete_image_location_from_backend) |
|
309 - |
|
310 - changes = [{'op': 'replace', 'path': ['locations'], 'value': []}] |
|
311 - self.controller.update(request, UUID1, changes) |
|
312 - changes = [{'op': 'replace', 'path': ['locations'], |
|
313 - 'value': [new_location]}] |
|
314 - self.assertRaises(webob.exc.HTTPForbidden, self.controller.update, |
|
315 - request, UUID1, changes) |
|
316 - |
|
317 def test_update_delete_image_location_unauthorized(self): |
|
318 rules = {"delete_image_location": False} |
|
319 self.policy.set_rules(rules) |
|
320 diff --git a/releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml b/releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml |
|
321 new file mode 100644 |
|
322 index 0000000..344e6e5 |
|
323 --- /dev/null |
|
324 +++ b/releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml |
|
325 @@ -0,0 +1,10 @@ |
|
326 +--- |
|
327 +security: |
|
328 + - Fixing bug 1525915; image might be transitioning |
|
329 + from active to queued by regular user by removing |
|
330 + last location of image (or replacing locations |
|
331 + with empty list). This allows user to re-upload |
|
332 + data to the image breaking Glance's promise of |
|
333 + image data immutability. From now on, last |
|
334 + location cannot be removed and locations cannot |
|
335 + be replaced with empty list. |
|
336 -- |
|
337 1.9.1 |
|
338 |
|