DO NOT MERGE: draft: improv: Check reference prop types for all customs

Previously the requirement that properties ending in `_ref(s)` were
instances of an appropriate type capable of containing a STIX
`identifier` (the current interpretation of section 3.1 of the spec) was
only enforced for custom observables. This change refactors the property
checks for custom objects to enforce this and the STIX 2.1 property name
requirement (also from section 3.1) in a common helper, therefore
extending the enforcement of both requirements to all custom object
types created by downstream projects.

DRAFT NOTES: There are some TODOs in here which need addressing!

stix2/registration.py

@@ -3,10 +3,83 @@
 from . import registry
 from .base import _DomainObject, _Observable
 from .exceptions import DuplicateRegistrationError
-from .properties import _validate_type
-from .utils import PREFIX_21_REGEX, get_class_hierarchy_names
+from .properties import (
+    ListProperty, ObjectReferenceProperty, ReferenceProperty, _validate_type,
+)
+from .utils import PREFIX_21_REGEX
 from .version import DEFAULT_VERSION
 
+# Properties ending in "_ref/s" need to be instances of specific types to meet
+# our interpretation of section 3.1 of the spec
+_VERSION_REF_TYPES_MAP = {
+    # TODO: The 2.0 code seems to use `ReferenceProperty`s - is that okay?
+    "2.0": (ObjectReferenceProperty, ReferenceProperty),
+    "2.1": (ReferenceProperty,),
+}
+# Any unknown versions are presumed to be newer than STIX 2.1 so we'll define
+# this to enforce the 2.1 compatible type in such situations
+_UNKNOWN_VERSION_REF_TYPES = _VERSION_REF_TYPES_MAP["2.1"]
+
+
+def _validate_ref_props(props_map, version):
+    """
+    Validate that reference properties contain an expected type.
+
+    Args:
+        props_map (mapping): A mapping of STIX object properties to be checked.
+        version (str): Which STIX2 version the properties must confirm to.
+
+    Raises:
+        ValueError: If the properties do not conform.
+    """
+    try:
+        ref_prop_types = _VERSION_REF_TYPES_MAP[version]
+    except KeyError:
+        ref_prop_types = _UNKNOWN_VERSION_REF_TYPES
+
+    for prop_name, prop_obj in props_map.items():
+        tail = prop_name.rsplit("_", 1)[-1]
+        if tail == "ref" and not isinstance(prop_obj, ref_prop_types):
+            raise ValueError(
+                f"{prop_name!r} is named like a reference property but is not "
+                f"a subclass of any of {ref_prop_types!r}.",
+            )
+        elif tail == "refs" and not all((
+            isinstance(prop_obj, ListProperty),
+            isinstance(getattr(prop_obj, "contained", None), ref_prop_types),
+        )):
+            raise ValueError(
+                f"{prop_name!r} is named like a reference list property but is not"
+                f"a 'ListProperty' containing a subclass of any of {ref_prop_types!r}.",
+            )
+
+
+def _validate_props(props_map, version):
+    """
+    Validate that a map of properties is conformant for this STIX `version`.
+
+    Args:
+        props_map (mapping): A mapping of STIX object properties to be checked.
+        version (str): Which STIX2 version the properties must confirm to.
+
+    Raises:
+        ValueError: If the properties do not conform.
+    """
+    # Confirm conformance with STIX 2.1 requirements for property names
+    #
+    # TODO: Should this be 2.1 or anything unknown assuming 2.1+, like we do
+    # for identifier properties above? `version != "2.0"`?
+    if version == "2.1":
+        for prop_name, prop_value in props_map.items():
+            # TODO: We could probably avoid doing a regex here just for the
+            # first char, but actually there are other constraints in 3.1 which
+            # we don't check which a regex would be suitable for
+            # (e.g. don't use hyphens in prop names, length min/max)
+            if not re.match(PREFIX_21_REGEX, prop_name):
+                raise ValueError("Property name '%s' must begin with an alpha character." % prop_name)
+    # Confirm conformance to section 3.1 regarding identifier properties
+    _validate_ref_props(props_map, version)
+
 
 def _register_object(new_type, version=DEFAULT_VERSION):
     """Register a custom STIX Object type.
@@ -29,15 +102,10 @@ def _register_object(new_type, version=DEFAULT_VERSION):
             new_type.__name__,
         )
 
-    properties = new_type._properties
-
     if not version:
         version = DEFAULT_VERSION
 
-    if version == "2.1":
-        for prop_name, prop in properties.items():
-            if not re.match(PREFIX_21_REGEX, prop_name):
-                raise ValueError("Property name '%s' must begin with an alpha character" % prop_name)
+    _validate_props(new_type._properties, version)
 
     OBJ_MAP = registry.STIX2_OBJ_MAPS[version]['objects']
     if new_type._type in OBJ_MAP.keys():
@@ -54,19 +122,12 @@ def _register_marking(new_marking, version=DEFAULT_VERSION):
             None, use latest version.
 
     """
-
-    mark_type = new_marking._type
-    properties = new_marking._properties
-
     if not version:
         version = DEFAULT_VERSION
 
+    mark_type = new_marking._type
     _validate_type(mark_type, version)
-
-    if version == "2.1":
-        for prop_name, prop_value in properties.items():
-            if not re.match(PREFIX_21_REGEX, prop_name):
-                raise ValueError("Property name '%s' must begin with an alpha character." % prop_name)
+    _validate_props(new_marking._properties, version)
 
     OBJ_MAP_MARKING = registry.STIX2_OBJ_MAPS[version]['markings']
     if mark_type in OBJ_MAP_MARKING.keys():
@@ -83,49 +144,10 @@ def _register_observable(new_observable, version=DEFAULT_VERSION):
             None, use latest version.
 
     """
-    properties = new_observable._properties
-
     if not version:
         version = DEFAULT_VERSION
 
-    if version == "2.0":
-        # If using STIX2.0, check properties ending in "_ref/s" are ObjectReferenceProperties
-        for prop_name, prop in properties.items():
-            if prop_name.endswith('_ref') and ('ObjectReferenceProperty' not in get_class_hierarchy_names(prop)):
-                raise ValueError(
-                    "'%s' is named like an object reference property but "
-                    "is not an ObjectReferenceProperty." % prop_name,
-                )
-            elif (
-                prop_name.endswith('_refs') and (
-                    'ListProperty' not in get_class_hierarchy_names(prop) or
-                    'ObjectReferenceProperty' not in get_class_hierarchy_names(prop.contained)
-                )
-            ):
-                raise ValueError(
-                    "'%s' is named like an object reference list property but "
-                    "is not a ListProperty containing ObjectReferenceProperty." % prop_name,
-                )
-    else:
-        # If using STIX2.1 (or newer...), check properties ending in "_ref/s" are ReferenceProperties
-        for prop_name, prop in properties.items():
-            if not re.match(PREFIX_21_REGEX, prop_name):
-                raise ValueError("Property name '%s' must begin with an alpha character." % prop_name)
-            elif prop_name.endswith('_ref') and ('ReferenceProperty' not in get_class_hierarchy_names(prop)):
-                raise ValueError(
-                    "'%s' is named like a reference property but "
-                    "is not a ReferenceProperty." % prop_name,
-                )
-            elif (
-                prop_name.endswith('_refs') and (
-                    'ListProperty' not in get_class_hierarchy_names(prop) or
-                    'ReferenceProperty' not in get_class_hierarchy_names(prop.contained)
-                )
-            ):
-                raise ValueError(
-                    "'%s' is named like a reference list property but "
-                    "is not a ListProperty containing ReferenceProperty." % prop_name,
-                )
+    _validate_props(new_observable._properties, version)
 
     OBJ_MAP_OBSERVABLE = registry.STIX2_OBJ_MAPS[version]['observables']
     if new_observable._type in OBJ_MAP_OBSERVABLE.keys():
@@ -149,29 +171,24 @@ def _register_observable_extension(
     obs_class = observable if isinstance(observable, type) else \
         type(observable)
     ext_type = new_extension._type
-    properties = new_extension._properties
 
     if not issubclass(obs_class, _Observable):
         raise ValueError("'observable' must be a valid Observable class!")
 
     _validate_type(ext_type, version)
-
-    if not new_extension._properties:
-        raise ValueError(
-            "Invalid extension: must define at least one property: " +
-            ext_type,
-        )
-
     if version == "2.1":
         if not ext_type.endswith('-ext'):
             raise ValueError(
                 "Invalid extension type name '%s': must end with '-ext'." %
                 ext_type,
             )
 
-        for prop_name, prop_value in properties.items():
-            if not re.match(PREFIX_21_REGEX, prop_name):
-                raise ValueError("Property name '%s' must begin with an alpha character." % prop_name)
+    if not new_extension._properties:
+        raise ValueError(
+            "Invalid extension: must define at least one property: " +
+            ext_type,
+        )
+    _validate_props(new_extension._properties, version)
 
     try:
         observable_type = observable._type

stix2/test/v20/test_custom.py

@@ -561,7 +561,7 @@ def test_custom_observable_object_invalid_ref_property():
         )
         class NewObs():
             pass
-    assert "is named like an object reference property but is not an ObjectReferenceProperty" in str(excinfo.value)
+    assert "is named like a reference property but is not" in str(excinfo.value)
 
 
 def test_custom_observable_object_invalid_refs_property():
@@ -573,7 +573,7 @@ def test_custom_observable_object_invalid_refs_property():
         )
         class NewObs():
             pass
-    assert "is named like an object reference list property but is not a ListProperty containing ObjectReferenceProperty" in str(excinfo.value)
+    assert "is named like a reference list property but is not" in str(excinfo.value)
 
 
 def test_custom_observable_object_invalid_refs_list_property():
@@ -585,7 +585,7 @@ def test_custom_observable_object_invalid_refs_list_property():
         )
         class NewObs():
             pass
-    assert "is named like an object reference list property but is not a ListProperty containing ObjectReferenceProperty" in str(excinfo.value)
+    assert "is named like a reference list property but is not" in str(excinfo.value)
 
 
 def test_custom_observable_object_invalid_valid_refs():

stix2/test/v21/test_custom.py

@@ -709,7 +709,7 @@ def test_custom_observable_object_invalid_ref_property():
         )
         class NewObs():
             pass
-    assert "is named like a reference property but is not a ReferenceProperty" in str(excinfo.value)
+    assert "is named like a reference property but is not" in str(excinfo.value)
 
 
 def test_custom_observable_object_invalid_refs_property():
@@ -721,7 +721,7 @@ def test_custom_observable_object_invalid_refs_property():
         )
         class NewObs():
             pass
-    assert "is named like a reference list property but is not a ListProperty containing ReferenceProperty" in str(excinfo.value)
+    assert "is named like a reference list property but is not" in str(excinfo.value)
 
 
 def test_custom_observable_object_invalid_refs_list_property():
@@ -733,7 +733,7 @@ def test_custom_observable_object_invalid_refs_list_property():
         )
         class NewObs():
             pass
-    assert "is named like a reference list property but is not a ListProperty containing ReferenceProperty" in str(excinfo.value)
+    assert "is named like a reference list property but is not" in str(excinfo.value)
 
 
 def test_custom_no_properties_raises_exception():