Merge "rest: string → UUID conversion for resource.id to be unique per user"

gnocchi/indexer/alembic/versions/397987e38570_no_more_slash_and_reencode.py

@@ -13,7 +13,7 @@
 #    under the License.
 #
 
-"""no-more-slash
+"""Remove slashes from original resource IDs, recompute their id with creator
 
 Revision ID: 397987e38570
 Revises: aba5a217ca9b
@@ -49,7 +49,8 @@ resource_table = sa.Table(
               sqlalchemy_utils.types.uuid.UUIDType(),
               nullable=False),
     sa.Column('original_resource_id', sa.String(255)),
-    sa.Column('type', sa.String(255))
+    sa.Column('type', sa.String(255)),
+    sa.Column('creator', sa.String(255))
 )
 
 resourcehistory_table = sa.Table(
@@ -100,15 +101,28 @@ def upgrade():
                       nullable=False),
         )
 
-    for resource in connection.execute(resource_table.select().where(
+    for resource in connection.execute(resource_table.select()):
-            resource_table.c.original_resource_id.like('%/%'))):
+
+        if resource_table.c.original_resource_id is None:
+            # statsd resource has no original_resource_id and is NULL
+            continue
+
+        try:
+            orig_as_uuid = uuid.UUID(
+                str(resource_table.c.original_resource_id))
+        except ValueError:
+            pass
+        else:
+            if orig_as_uuid == resource_table.c.id:
+                continue
+
         new_original_resource_id = resource.original_resource_id.replace(
             '/', '_')
         if six.PY2:
             new_original_resource_id = new_original_resource_id.encode('utf-8')
         new_id = sa.literal(uuidtype.process_bind_param(
-            str(uuid.uuid5(utils.RESOURCE_ID_NAMESPACE,
+            str(utils.ResourceUUID(
-                           new_original_resource_id)),
+                new_original_resource_id, resource.creator)),
             connection.dialect))
 
         # resource table

gnocchi/rest/__init__.py

@@ -14,6 +14,7 @@
 # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations
 # under the License.
+import functools
 import itertools
 import uuid
 
@@ -849,8 +850,10 @@ class ResourceController(rest.RestController):
 
     def __init__(self, resource_type, id):
         self._resource_type = resource_type
+        creator = pecan.request.auth_helper.get_current_user(
+            pecan.request.headers)
         try:
-            self.id = utils.ResourceUUID(id)
+            self.id = utils.ResourceUUID(id, creator)
         except ValueError:
             abort(404, indexer.NoSuchResource(id))
         self.metric = NamedMetricController(str(self.id), self._resource_type)
@@ -930,8 +933,15 @@ def schema_for(resource_type):
     return ResourceSchema(resource_type.schema)
 
 
-def ResourceID(value):
+def ResourceUUID(value, creator):
-    return (six.text_type(value), utils.ResourceUUID(value))
+    try:
+        return utils.ResourceUUID(value, creator)
+    except ValueError as e:
+        raise voluptuous.Invalid(e)
+
+
+def ResourceID(value, creator):
+    return (six.text_type(value), ResourceUUID(value, creator))
 
 
 class ResourcesController(rest.RestController):
@@ -947,7 +957,9 @@ class ResourcesController(rest.RestController):
         # NOTE(sileht): we need to copy the dict because when change it
         # and we don't want that next patch call have the "id"
         schema = dict(schema_for(self._resource_type))
-        schema["id"] = ResourceID
+        creator = pecan.request.auth_helper.get_current_user(
+            pecan.request.headers)
+        schema["id"] = functools.partial(ResourceID, creator=creator)
 
         body = deserialize_and_validate(schema)
         body["original_resource_id"], body["id"] = body["id"]
@@ -957,8 +969,6 @@ class ResourcesController(rest.RestController):
         }
         target.update(body)
         enforce("create resource", target)
-        creator = pecan.request.auth_helper.get_current_user(
-            pecan.request.headers)
         rid = body['id']
         del body['id']
         try:
@@ -1004,8 +1014,7 @@ class ResourcesController(rest.RestController):
     def delete(self, **kwargs):
         # NOTE(sileht): Don't allow empty filter, this is going to delete
         # the entire database.
-        attr_filter = deserialize_and_validate(
+        attr_filter = deserialize_and_validate(ResourceSearchSchema)
-            SearchResourceTypeController.ResourceSearchSchema)
 
         # the voluptuous checks everything, but it is better to
         # have this here.
@@ -1130,16 +1139,16 @@ class QueryStringSearchAttrFilter(object):
         return cls._parsed_query2dict(parsed_query)
 
 
-def _ResourceSearchSchema(v):
+def ResourceSearchSchema(v):
-    """Helper method to indirect the recursivity of the search schema"""
+    return _ResourceSearchSchema()(v)
-    return SearchResourceTypeController.ResourceSearchSchema(v)
 
 
-class SearchResourceTypeController(rest.RestController):
+def _ResourceSearchSchema():
-    def __init__(self, resource_type):
+    user = pecan.request.auth_helper.get_current_user(
-        self._resource_type = resource_type
+        pecan.request.headers)
+    _ResourceUUID = functools.partial(ResourceUUID, creator=user)
 
-    ResourceSearchSchema = voluptuous.Schema(
+    return voluptuous.Schema(
         voluptuous.All(
             voluptuous.Length(min=0, max=1),
             {
@@ -1156,31 +1165,36 @@ class SearchResourceTypeController(rest.RestController):
                     voluptuous.Length(min=1, max=1),
                     voluptuous.Any(
                         {"id": voluptuous.Any(
-                            utils.ResourceUUID, [utils.ResourceUUID]),
+                            [_ResourceUUID], _ResourceUUID),
                          voluptuous.Extra: voluptuous.Extra})),
                 voluptuous.Any(
                     u"and", u"∨",
                     u"or", u"∧",
                     u"not",
                 ): voluptuous.All(
-                    [_ResourceSearchSchema], voluptuous.Length(min=1)
+                    [ResourceSearchSchema], voluptuous.Length(min=1)
                 )
             }
         )
     )
 
-    @classmethod
+
-    def parse_and_validate_qs_filter(cls, query):
+class SearchResourceTypeController(rest.RestController):
+    def __init__(self, resource_type):
+        self._resource_type = resource_type
+
+    @staticmethod
+    def parse_and_validate_qs_filter(query):
         try:
             attr_filter = QueryStringSearchAttrFilter.parse(query)
         except InvalidQueryStringSearchAttrFilter as e:
             raise abort(400, e)
-        return voluptuous.Schema(cls.ResourceSearchSchema,
+        return voluptuous.Schema(ResourceSearchSchema,
                                  required=True)(attr_filter)
 
     def _search(self, **kwargs):
         if pecan.request.body:
-            attr_filter = deserialize_and_validate(self.ResourceSearchSchema)
+            attr_filter = deserialize_and_validate(ResourceSearchSchema)
         elif kwargs.get("filter"):
             attr_filter = self.parse_and_validate_qs_filter(kwargs["filter"])
         else:
@@ -1329,13 +1343,16 @@ class SearchMetricController(rest.RestController):
 
 
 class ResourcesMetricsMeasuresBatchController(rest.RestController):
-    MeasuresBatchSchema = voluptuous.Schema(
-        {ResourceID: {six.text_type: MeasuresListSchema}}
-    )
-
     @pecan.expose('json')
     def post(self, create_metrics=False):
-        body = deserialize_and_validate(self.MeasuresBatchSchema)
+        creator = pecan.request.auth_helper.get_current_user(
+            pecan.request.headers)
+        MeasuresBatchSchema = voluptuous.Schema(
+            {functools.partial(ResourceID, creator=creator):
+             {six.text_type: MeasuresListSchema}}
+        )
+
+        body = deserialize_and_validate(MeasuresBatchSchema)
 
         known_metrics = []
         unknown_metrics = []
@@ -1350,8 +1367,6 @@ class ResourcesMetricsMeasuresBatchController(rest.RestController):
 
             known_names = [m.name for m in metrics]
             if strutils.bool_from_string(create_metrics):
-                creator = pecan.request.auth_helper.get_current_user(
-                    pecan.request.headers)
                 already_exists_names = []
                 for name in names:
                     if name not in known_names:

gnocchi/tests/gabbi/gabbits/base.yaml

@@ -134,13 +134,13 @@ tests:
     project_id: f3d41b77-0cc1-4f0b-b94a-1d5be9c0e3ea
   status: 201
   response_headers:
-    location: $SCHEME://$NETLOC/v1/resource/generic/8d835270-2834-5e55-a693-fd0cf91cba3d
+    location: $SCHEME://$NETLOC/v1/resource/generic/2d869568-70d4-5ed6-9891-7d7a3bbf572d
   response_json_paths:
     type: generic
     started_at: "2014-01-03T02:02:02+00:00"
     project_id: f3d41b77-0cc1-4f0b-b94a-1d5be9c0e3ea
     created_by_project_id: 99d13f22-3618-4288-82b8-6512ded77e4f
-    id: 8d835270-2834-5e55-a693-fd0cf91cba3d
+    id: 2d869568-70d4-5ed6-9891-7d7a3bbf572d
     original_resource_id: 1.2.3.4
 
 - name: get status denied

gnocchi/tests/gabbi/gabbits/batch-measures.yaml

@@ -244,7 +244,7 @@ tests:
       response_json_paths:
           $.description.cause: "Unknown resources"
           $.description.detail:
-            - resource_id: "301dbf9a-4fce-52b6-9010-4484c469dcec"
+            - resource_id: "6b8e287d-c01a-538c-979b-a819ee49de5d"
               original_resource_id: "foobar"
 
     - name: push measurements to named metrics and resource with create_metrics with wrong measure objects

gnocchi/tests/gabbi/gabbits/resource-type.yaml

@@ -6,6 +6,11 @@
 fixtures:
     - ConfigFixture
 
+defaults:
+  request_headers:
+      x-user-id: 0fbb231484614b1a80131fc22f6afc9c
+      x-project-id: f3d41b770cc14f0bb94a1d5be9c0e3ea
+
 tests:
 
     - name: list resource type
@@ -222,8 +227,6 @@ tests:
     - name: post invalid resource
       POST: /v1/resource/my_custom_resource
       request_headers:
-          x-user-id: 0fbb2314-8461-4b1a-8013-1fc22f6afc9c
-          x-project-id: f3d41b77-0cc1-4f0b-b94a-1d5be9c0e3ea
           content-type: application/json
       data:
           id: d11edfca-4393-4fda-b94d-b05a3a1b3747
@@ -239,8 +242,6 @@ tests:
     - name: post invalid resource uuid
       POST: $LAST_URL
       request_headers:
-          x-user-id: 0fbb2314-8461-4b1a-8013-1fc22f6afc9c
-          x-project-id: f3d41b77-0cc1-4f0b-b94a-1d5be9c0e3ea
           content-type: application/json
       data:
           id: d11edfca-4393-4fda-b94d-b05a3a1b3747
@@ -258,8 +259,6 @@ tests:
     - name: post custom resource
       POST: $LAST_URL
       request_headers:
-          x-user-id: 0fbb2314-8461-4b1a-8013-1fc22f6afc9c
-          x-project-id: f3d41b77-0cc1-4f0b-b94a-1d5be9c0e3ea
           content-type: application/json
       data:
           id: d11edfca-4393-4fda-b94d-b05a3a1b3747
@@ -276,8 +275,6 @@ tests:
     - name: patch custom resource
       PATCH: /v1/resource/my_custom_resource/d11edfca-4393-4fda-b94d-b05a3a1b3747
       request_headers:
-          x-user-id: 0fbb2314-8461-4b1a-8013-1fc22f6afc9c
-          x-project-id: f3d41b77-0cc1-4f0b-b94a-1d5be9c0e3ea
           content-type: application/json
       data:
           name: foo
@@ -303,8 +300,6 @@ tests:
     - name: post resource with default
       POST: /v1/resource/my_custom_resource
       request_headers:
-          x-user-id: 0fbb2314-8461-4b1a-8013-1fc22f6afc9c
-          x-project-id: f3d41b77-0cc1-4f0b-b94a-1d5be9c0e3ea
           content-type: application/json
       data:
           id: c4110aec-6e5c-43fa-b8c5-ffdfbca3ce59

gnocchi/tests/gabbi/gabbits/transformedids.yaml

@@ -66,8 +66,26 @@ tests:
           project_id: f3d41b770cc14f0bb94a1d5be9c0e3ea
       status: 400
       response_strings:
-        - "Invalid input: not a valid value for dictionary value @ data["
+        - "'/' is not supported in resource id"
-        - "'id']  "
+
+
+    - name: post new resource non uuid again different user
+      POST: /v1/resource/generic
+      request_headers:
+        x-user-id: 0fbb231484614b1a80131fc22f6afc9b
+        x-project-id: f3d41b770cc14f0bb94a1d5be9c0e3ea
+      data:
+          id: generic zero
+          metrics:
+              cpu.util:
+                  archive_policy_name: medium
+      status: 201
+      response_json_paths:
+          created_by_user_id: 0fbb231484614b1a80131fc22f6afc9b
+          created_by_project_id: f3d41b770cc14f0bb94a1d5be9c0e3ea
+      response_headers:
+          # is a UUID
+          location: /v1/resource/generic/[a-f0-9-]{36}/
 
     - name: post new resource non uuid
       POST: /v1/resource/generic
@@ -151,7 +169,7 @@ tests:
                   archive_policy_name: medium
       status: 400
       response_strings:
-          - not a valid value for
+        - transformable resource id >255 max allowed characters for dictionary value
 
     - name: post long non uuid resource id
       POST: $LAST_URL

gnocchi/utils.py

@@ -39,23 +39,24 @@ LOG = log.getLogger(__name__)
 RESOURCE_ID_NAMESPACE = uuid.UUID('0a7a15ff-aa13-4ac2-897c-9bdf30ce175b')
 
 
-def ResourceUUID(value):
+def ResourceUUID(value, creator):
     if isinstance(value, uuid.UUID):
         return value
     if '/' in value:
         raise ValueError("'/' is not supported in resource id")
     try:
-        try:
+        return uuid.UUID(value)
-            return uuid.UUID(value)
+    except ValueError:
-        except ValueError:
+        if len(value) <= 255:
-            if len(value) <= 255:
+            # value/creator must be str (unicode) in Python 3 and str (bytes)
-                if six.PY2:
+            # in Python 2. It's not logical, I know.
-                    value = value.encode('utf-8')
+            if six.PY2:
-                return uuid.uuid5(RESOURCE_ID_NAMESPACE, value)
+                value = value.encode('utf-8')
-            raise ValueError(
+                creator = creator.encode('utf-8')
-                'transformable resource id >255 max allowed characters')
+            return uuid.uuid5(RESOURCE_ID_NAMESPACE,
-    except Exception as e:
+                              value + "\x00" + creator)
-        raise ValueError(e)
+        raise ValueError(
+            'transformable resource id >255 max allowed characters')
 
 
 def UUID(value):

releasenotes/notes/uuid5-change-8a8c467d2b2d4c85.yaml

@@ -0,0 +1,12 @@
+---
+issues:
+  - >-
+    The conversion mechanism provided by the API to convert non-UUID resource
+    id to UUID is now also based on the user creating/accessing the resource.
+    This makes sure that the conversion generates a unique UUID for the user
+    and that several users can use the same string as `original_resource_id`.
+upgrade:
+  - >-
+    Since `original_resource_id` is now unique per creator, that means users
+    cannot refer to resource by using the `original_resource_id` if the
+    resource was not created by them.

run-upgrade-tests.sh

@@ -111,12 +111,16 @@ RESOURCE_IDS=(
     "5a301761-cccc-46e2-8900-8b4f6fe6675a"
 )
 # NOTE(sileht): / are now _
-[ "$have_resource_type_post" ] && RESOURCE_ID_EXT="5a301761_dddd_46e2_8900_8b4f6fe6675a"
+# NOTE(jdanjou): and we reencode for admin:admin, but we cannot authenticate as
+# admin:admin in basic since ":" is forbidden in any username, so let's use the direct
+# computed ID
+[ "$have_resource_type_post" ] && RESOURCE_ID_EXT="517920a9-2e50-58b8-88e8-25fd7aae1d8f"
+
 dump_data $GNOCCHI_DATA/new
 
 # NOTE(sileht): change the output of the old gnocchi to compare with the new without '/'
 $GSED -i -e "s,5a301761/dddd/46e2/8900/8b4f6fe6675a,5a301761_dddd_46e2_8900_8b4f6fe6675a,g" \
-      -e "s,19235bb9-35ca-5f55-b7db-165cfb033c86,fe1bdabf-d94c-5b3a-af1e-06bdff53f228,g" $GNOCCHI_DATA/old/resources.list
+      -e "s,19235bb9-35ca-5f55-b7db-165cfb033c86,517920a9-2e50-58b8-88e8-25fd7aae1d8f,g" $GNOCCHI_DATA/old/resources.list
 
 echo "* Checking output difference between Gnocchi $old_version and $new_version"
 diff -uNr $GNOCCHI_DATA/old $GNOCCHI_DATA/new