From e9527abe8616773d274b47a51b0ac94a28a6fa25 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcritten@redhat.com>
Date: Tue, 29 Nov 2016 02:39:26 +0000
Subject: [PATCH] Require a valid image id to enroll a host

I was previously ignoring an invalid or inaccessible image but
I think this should instead be a hard failure. How can a user
request an image create in nova with an image that can't be read?

I also noticed that it was possible for any test looking for a
raised error to fail if that exception wasn't raised so
assert(False) in that case.
---
 novajoin/join.py                       | 10 +++----
 novajoin/tests/unit/api/v1/test_api.py | 36 ++++++++++++++++++++++++++
 novajoin/wsgi.py                       |  2 +-
 3 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/novajoin/join.py b/novajoin/join.py
index 69a6b75..1658337 100644
--- a/novajoin/join.py
+++ b/novajoin/join.py
@@ -143,12 +143,10 @@ class JoinController(Controller):
         image_metadata = {}
         try:
             image = image_service.show(context, image_id)
-        except exception.ImageNotFound:
-            # The image metadata is not a show stopper, proceed
-            # without it.
-            pass
-        except exception.ImageNotAuthorized as e:
-            LOG.error('Failed to get image, proceeding anyway: %s', e)
+        except (exception.ImageNotFound, exception.ImageNotAuthorized) as e:
+            msg = 'Failed to get image: %s' % e
+            LOG.error(msg)
+            raise base.Fault(webob.exc.HTTPBadRequest(explanation=msg))
         else:
             image_metadata = image.get('properties', {})
 
diff --git a/novajoin/tests/unit/api/v1/test_api.py b/novajoin/tests/unit/api/v1/test_api.py
index 71c0a32..4eae293 100644
--- a/novajoin/tests/unit/api/v1/test_api.py
+++ b/novajoin/tests/unit/api/v1/test_api.py
@@ -23,6 +23,8 @@ from novajoin import test
 from novajoin.tests.unit.api import fakes
 from novajoin.tests.unit import fake_constants as fake
 
+import webob.exc
+
 
 class FakeImageService(object):
     def show(self, context, image_id):
@@ -48,6 +50,8 @@ class JoinTest(test.TestCase):
             self.join_controller.create(req, body)
         except Fault as fault:
             assert fault.status_int == 400
+        else:
+            assert(False)
 
     def test_no_instanceid(self):
         body = {"metadata": {"ipa_enroll": "True"},
@@ -64,6 +68,8 @@ class JoinTest(test.TestCase):
             self.join_controller.create(req, body)
         except Fault as fault:
             assert fault.status_int == 400
+        else:
+            assert(False)
 
     def test_no_imageid(self):
         body = {"metadata": {"ipa_enroll": "True"},
@@ -80,6 +86,8 @@ class JoinTest(test.TestCase):
             self.join_controller.create(req, body)
         except Fault as fault:
             assert fault.status_int == 400
+        else:
+            assert(False)
 
     def test_no_hostname(self):
         body = {"metadata": {"ipa_enroll": "True"},
@@ -96,6 +104,8 @@ class JoinTest(test.TestCase):
             self.join_controller.create(req, body)
         except Fault as fault:
             assert fault.status_int == 400
+        else:
+            assert(False)
 
     def test_no_project_id(self):
         body = {"metadata": {"ipa_enroll": "True"},
@@ -112,6 +122,8 @@ class JoinTest(test.TestCase):
             self.join_controller.create(req, body)
         except Fault as fault:
             assert fault.status_int == 400
+        else:
+            assert(False)
 
     @mock.patch('novajoin.join.get_default_image_service')
     def test_request_no_enrollment(self, mock_get_image):
@@ -129,6 +141,28 @@ class JoinTest(test.TestCase):
         res_dict = self.join_controller.create(req, body)
         self.assertEqual(expected, res_dict)
 
+    @mock.patch('novajoin.join.get_default_image_service')
+    def test_request_invalid_image(self, mock_get_image):
+        mock_get_image.side_effect = Fault(webob.exc.HTTPBadRequest())
+        body = {"metadata": {"ipa_enroll": "False"},
+                "instance-id": fake.INSTANCE_ID,
+                "project-id": fake.PROJECT_ID,
+                "image-id": "invalid",
+                "hostname": "test"}
+        req = fakes.HTTPRequest.blank('/v1')
+        req.method = 'POST'
+        req.content_type = "application/json"
+        req.body = jsonutils.dump_as_bytes(body)
+
+        # Not using assertRaises because the exception is wrapped as
+        # a Fault
+        try:
+            self.join_controller.create(req, body)
+        except Fault as fault:
+            assert fault.status_int == 400
+        else:
+            assert(False)
+
     @mock.patch('novajoin.join.get_instance')
     @mock.patch('novajoin.join.get_default_image_service')
     def test_valid_request(self, mock_get_image, mock_get_instance):
@@ -180,3 +214,5 @@ class JoinTest(test.TestCase):
             self.join_controller.create(req, body)
         except Fault as fault:
             assert fault.status_int == 400
+        else:
+            assert(False)
diff --git a/novajoin/wsgi.py b/novajoin/wsgi.py
index 5892c0d..d84ded7 100644
--- a/novajoin/wsgi.py
+++ b/novajoin/wsgi.py
@@ -106,7 +106,7 @@ def main():
 
     keystone_client.register_keystoneauth_opts(CONF)
     CONF(sys.argv[1:], version='1.0.8',
-        default_config_files=config.find_config_files())
+         default_config_files=config.find_config_files())
     log.setup(CONF, 'join')
     launcher = process_launcher()
     server = WSGIService('join')