From c995faee4d3d65f10cc128c3283fd8b88e09a404 Mon Sep 17 00:00:00 2001 From: Martin Vrachev Date: Tue, 1 Dec 2020 15:46:45 +0200 Subject: [PATCH] New API: Make sure we are not changing dict arg In the Signed.from_dict function we are passing signed - a dictionary. Dictionaries are passed by reference and thus we want to make sure we are not changing the dictionary passed as a function argument. I had to give default function arguments for all class arguments assigned in __init__ for Root, Snapshot, Targets, and Timestamp classes. I chose "None" as the easiest solution as a default argument, but we definitely want to add proper validation which will ensure we are not creating empty or partially populated objects. I didn't want to create a discussion for sensible defaults and argument validation here. There is already existing issue for that: https://github.com/theupdateframework/tuf/issues/1140 Signed-off-by: Martin Vrachev --- tuf/api/metadata.py | 84 +++++++++++++++++++++++++++------------------ 1 file changed, 51 insertions(+), 33 deletions(-) diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py index 77e66bcc52..9cde07e08f 100644 --- a/tuf/api/metadata.py +++ b/tuf/api/metadata.py @@ -279,8 +279,6 @@ def __init__( self.expires = expires # TODO: Should we separate data validation from constructor? - if version < 0: - raise ValueError(f'version must be < 0, got {version}') self.version = version @@ -289,22 +287,24 @@ def __init__( def from_dict(cls, signed_dict: JsonDict) -> 'Signed': """Creates Signed object from its JSON/dict representation. """ + # Create empty object with default or parametrized constructor with + # default arguments. + obj = cls() + obj._type = signed_dict['_type'] + obj.version = signed_dict['version'] + obj.spec_version = signed_dict['spec_version'] # Convert 'expires' TUF metadata string to a datetime object, which is # what the constructor expects and what we store. The inverse operation # is implemented in 'to_dict'. - signed_dict['expires'] = tuf.formats.expiry_string_to_datetime( + obj.expires = tuf.formats.expiry_string_to_datetime( signed_dict['expires']) - # NOTE: We write the converted 'expires' back into 'signed_dict' above - # so that we can pass it to the constructor as '**signed_dict' below, - # along with other fields that belong to Signed subclasses. - # Any 'from_dict'(-like) conversions of fields that correspond to a - # subclass should be performed in the 'from_dict' method of that - # subclass and also be written back into 'signed_dict' before calling - # super().from_dict. + # NOTE: Any 'from_dict'(-like) conversions of fields that correspond + # to a subclass should be performed in the 'from_dict' method of that + # subclass and assigned to the below returned object. # NOTE: cls might be a subclass of Signed, if 'from_dict' was called on # that subclass (see e.g. Metadata.from_dict). - return cls(**signed_dict) + return obj # Serialization. @@ -376,9 +376,9 @@ class Root(Signed): # default max-args value for pylint is 5 # pylint: disable=too-many-arguments def __init__( - self, _type: str, version: int, spec_version: str, - expires: datetime, consistent_snapshot: bool, - keys: JsonDict, roles: JsonDict) -> None: + self, _type: str=None, version: int=None, spec_version: str=None, + expires: datetime=None, consistent_snapshot: bool=None, + keys: JsonDict=None, roles: JsonDict=None) -> None: super().__init__(_type, version, spec_version, expires) # TODO: Add classes for keys and roles self.consistent_snapshot = consistent_snapshot @@ -386,6 +386,18 @@ def __init__( self.roles = roles + @classmethod + def from_dict(cls, signed_dict: JsonDict) -> 'Root': + """Creates Root object from its JSON/dict representation. """ + + # Get a parent object with its attributes already assigned. + obj = super().from_dict(signed_dict) + obj.consistent_snapshot = signed_dict['consistent_snapshot'] + obj.keys = signed_dict['keys'] + obj.roles = signed_dict['roles'] + return obj + + # Serialization. def to_dict(self) -> JsonDict: """Returns the JSON-serializable dictionary representation of self. """ @@ -476,8 +488,9 @@ class Timestamp(Signed): """ def __init__( - self, _type: str, version: int, spec_version: str, - expires: datetime, meta: Dict[str, MetadataInfo]) -> None: + self, _type: str=None, version: int=None, spec_version: str=None, + expires: datetime=None, meta: Dict[str, MetadataInfo]=None + ) -> None: super().__init__(_type, version, spec_version, expires) self.meta = meta @@ -498,12 +511,13 @@ def to_dict(self) -> JsonDict: def from_dict(cls, signed_dict: JsonDict) -> 'Timestamp': """Creates Timestamp object from its JSON/dict representation. """ - # signed_dict is passed by reference, make sure we are not changing it. - changed_signed = signed_dict.copy() - changed_signed['meta']['snapshot.json'] = MetadataInfo( + # Get a parent object with its attributes already assigned. + obj = super().from_dict(signed_dict) + obj.meta = {} + obj.meta['snapshot.json'] = MetadataInfo( **signed_dict['meta']['snapshot.json']) - return super().from_dict(changed_signed) + return obj # Modification. @@ -529,8 +543,9 @@ class Snapshot(Signed): """ def __init__( - self, _type: str, version: int, spec_version: str, - expires: datetime, meta: Dict[str, MetadataInfo]) -> None: + self, _type: str=None, version: int=None, spec_version: str=None, + expires: datetime=None, meta: Dict[str, MetadataInfo]=None + ) -> None: super().__init__(_type, version, spec_version, expires) self.meta = meta @@ -539,13 +554,14 @@ def __init__( def from_dict(cls, signed_dict: JsonDict) -> 'Snapshot': """Creates Snapshot object from its JSON/dict representation. """ - # signed_dict is passed by reference, make sure we are not changing it. - changed_signed = signed_dict.copy() + # Get a parent object with its attributes already assigned. + obj = super().from_dict(signed_dict) + obj.meta = {} for meta_path in signed_dict['meta'].keys(): - changed_signed['meta'][meta_path] = MetadataInfo( + obj.meta[meta_path] = MetadataInfo( **signed_dict['meta'][meta_path]) - return super().from_dict(changed_signed) + return obj # Serialization. @@ -674,9 +690,9 @@ class Targets(Signed): # default max-args value for pylint is 5 # pylint: disable=too-many-arguments def __init__( - self, _type: str, version: int, spec_version: str, - expires: datetime, targets: Dict[str, TargetInfo], - delegations: JsonDict) -> None: + self, _type: str=None, version: int=None, spec_version: str=None, + expires: datetime=None, targets: Dict[str, TargetInfo]=None, + delegations: JsonDict=None) -> None: super().__init__(_type, version, spec_version, expires) self.targets = targets @@ -688,13 +704,15 @@ def __init__( def from_dict(cls, signed_dict: JsonDict) -> 'Targets': """Creates Targets object from its JSON/dict representation. """ - # signed_dict is passed by reference, make sure we are not changing it. - changed_signed = signed_dict.copy() + # Get a parent object with its attributes already assigned. + obj = super().from_dict(signed_dict) + obj.targets = {} for target_path in signed_dict['targets'].keys(): - changed_signed['targets'][target_path] = TargetInfo( + obj.targets[target_path] = TargetInfo( **signed_dict['targets'][target_path]) - return super().from_dict(changed_signed) + obj.delegations = signed_dict['delegations'] + return obj # Serialization.