Merge "add policy entries for /v3/regions"

4 files changed, 90 insertions, 17 deletions

diff --git a/etc/policy.json b/etc/policy.json
index 14bb89f99..fbb83a9dc 100644
--- a/etc/policy.json
+++ b/etc/policy.json
@@ -7,6 +7,12 @@
 
     "default": "rule:admin_required",
 
+    "identity:get_region": "",
+    "identity:list_regions": "",
+    "identity:create_region": "rule:admin_required",
+    "identity:update_region": "rule:admin_required",
+    "identity:delete_region": "rule:admin_required",
+
     "identity:get_service": "rule:admin_required",
     "identity:list_services": "rule:admin_required",
     "identity:create_service": "rule:admin_required",
diff --git a/etc/policy.v3cloudsample.json b/etc/policy.v3cloudsample.json
index 995665d2c..a78fcc12a 100644
--- a/etc/policy.v3cloudsample.json
+++ b/etc/policy.v3cloudsample.json
@@ -17,6 +17,12 @@
 
     "default": "rule:admin_required",
 
+    "identity:get_region": "",
+    "identity:list_regions": "",
+    "identity:create_region": "rule:admin_or_cloud_admin",
+    "identity:update_region": "rule:admin_or_cloud_admin",
+    "identity:delete_region": "rule:admin_or_cloud_admin",
+
     "identity:get_service": "rule:admin_or_cloud_admin",
     "identity:list_services": "rule:admin_or_cloud_admin",
     "identity:create_service": "rule:cloud_admin",
diff --git a/keystone/catalog/controllers.py b/keystone/catalog/controllers.py
index f289932b1..b9780f552 100644
--- a/keystone/catalog/controllers.py
+++ b/keystone/catalog/controllers.py
@@ -19,6 +19,7 @@ import six
 
 from keystone.common import controller
 from keystone.common import dependency
+from keystone.common import wsgi
 from keystone import exception
 
 
@@ -141,21 +142,34 @@ class RegionV3(controller.V3Controller):
     collection_name = 'regions'
     member_name = 'region'
 
-    @controller.protected()
     def create_region_with_id(self, context, region_id, region):
-        """Specialized route target for PUT /regions/{region_id}."""
-        ref = self._normalize_dict(region)
-        ref['id'] = region_id
-        ref = self.catalog_api.create_region(ref)
-        return RegionV3.wrap_member(context, ref)
+        """Create a region with a user-specified ID.
+
+        This method is unprotected because it depends on ``self.create_region``
+        to enforce policy.
+        """
+        if 'id' in region and region_id != region['id']:
+            raise exception.ValidationError(
+                _('Conflicting region IDs specified: '
+                  '"%(url_id)s" != "%(ref_id)s"') % {
+                      'url_id': region_id,
+                      'ref_id': region['id']})
+        region['id'] = region_id
+        return self.create_region(context, region)
 
     @controller.protected()
     def create_region(self, context, region):
-        ref = self._assign_unique_id(self._normalize_dict(region))
+        ref = self._normalize_dict(region)
+
+        if 'id' not in ref:
+            ref = self._assign_unique_id(ref)
 
         ref = self.catalog_api.create_region(ref)
-        return RegionV3.wrap_member(context, ref)
+        return wsgi.render_response(
+            RegionV3.wrap_member(context, ref),
+            status=(201, 'Created'))
 
+    @controller.protected()
     def list_regions(self, context):
         refs = self.catalog_api.list_regions()
         return RegionV3.wrap_collection(context, refs)
diff --git a/keystone/tests/test_v3_catalog.py b/keystone/tests/test_v3_catalog.py
index f36a3da28..a1cbea231 100644
--- a/keystone/tests/test_v3_catalog.py
+++ b/keystone/tests/test_v3_catalog.py
@@ -12,6 +12,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import uuid
+
 from keystone.tests import test_v3
 
 
@@ -21,26 +23,71 @@ class CatalogTestCase(test_v3.RestfulTestCase):
     # region crud tests
 
     def test_create_region_with_id(self):
-        """Call ``PUT /regions/{region_id}``."""
-        ref = dict(description="my region")
-        region_id = 'myregion'
+        """Call ``PUT /regions/{region_id}`` w/o an ID in the request body."""
+        ref = self.new_region_ref()
+        region_id = ref.pop('id')
         r = self.put(
-            '/regions/myregion',
-            body={'region': ref}, expected_status=200)
+            '/regions/%s' % region_id,
+            body={'region': ref},
+            expected_status=201)
         self.assertValidRegionResponse(r, ref)
         # Double-check that the region ID was kept as-is and not
-        # populated with a UUID, like is the case with POST /regions
-        entity = r.result.get("region")
-        self.assertEqual(region_id, entity['id'])
+        # populated with a UUID, as is the case with POST /v3/regions
+        self.assertEqual(region_id, r.json['region']['id'])
+
+    def test_create_region_with_matching_ids(self):
+        """Call ``PUT /regions/{region_id}`` with an ID in the request body."""
+        ref = self.new_region_ref()
+        region_id = ref['id']
+        r = self.put(
+            '/regions/%s' % region_id,
+            body={'region': ref},
+            expected_status=201)
+        self.assertValidRegionResponse(r, ref)
+        # Double-check that the region ID was kept as-is and not
+        # populated with a UUID, as is the case with POST /v3/regions
+        self.assertEqual(region_id, r.json['region']['id'])
 
     def test_create_region(self):
-        """Call ``POST /regions``."""
+        """Call ``POST /regions`` with an ID in the request body."""
+        # the ref will have an ID defined on it
         ref = self.new_region_ref()
         r = self.post(
             '/regions',
             body={'region': ref})
         self.assertValidRegionResponse(r, ref)
 
+        # we should be able to get the region, having defined the ID ourselves
+        r = self.get(
+            '/regions/%(region_id)s' % {
+                'region_id': ref['id']})
+        self.assertValidRegionResponse(r, ref)
+
+    def test_create_region_without_id(self):
+        """Call ``POST /regions`` without an ID in the request body."""
+        ref = self.new_region_ref()
+
+        # instead of defining the ID ourselves...
+        del ref['id']
+
+        # let the service define the ID
+        r = self.post(
+            '/regions',
+            body={'region': ref},
+            expected_status=201)
+        self.assertValidRegionResponse(r, ref)
+
+    def test_create_region_with_conflicting_ids(self):
+        """Call ``PUT /regions/{region_id}`` with conflicting region IDs."""
+        # the region ref is created with an ID
+        ref = self.new_region_ref()
+
+        # but instead of using that ID, make up a new, conflicting one
+        self.put(
+            '/regions/%s' % uuid.uuid4().hex,
+            body={'region': ref},
+            expected_status=400)
+
     def test_list_regions(self):
         """Call ``GET /regions``."""
         r = self.get('/regions')