Take order into account for certain cases

After we have dropped OrderedDict in e3b267e
we are relying on python3.7+ default behavior to preserve the insertion
order, but there is one caveat.
When comparing dictionaries the order is still irrelevant compared to
OrderedDict. For example:
>>> OrderedDict([(1,1), (2,2)]) == OrderedDict([(2,2), (1,1)])
False
>>> dict([(1,1), (2,2)]) == dict([(2,2), (1,1)])
True

There are two special attributes, defined in the specification, where
the order makes a difference when comparing two objects:
- Metadata.signatures
- Targets.delegations.roles.
We want to make sure that the order in those two cases makes a
difference when comparing two objects and that's why those changes
are required inside two __eq__ implementations.

Signed-off-by: Martin Vrachev <[email protected]>

Author: MVrachev

tests/test_metadata_eq_.py

@@ -10,9 +10,12 @@
 import os
 import sys
 import unittest
-from typing import Any, ClassVar, Dict
+from typing import Any, ClassVar, Dict, List
+
+from securesystemslib.signer import SSlibSigner
 
 from tests import utils
+from tests.repository_simulator import RepositorySimulator
 from tuf.api.metadata import (
     TOP_LEVEL_ROLE_NAMES,
     DelegatedRole,
@@ -60,6 +63,35 @@ def test_metadata_eq_(self, test_case_data: Any) -> None:
         setattr(md_2, self.case_name, test_case_data)
         self.assertNotEqual(md, md_2)
 
+    def test_md_eq_signatures_reversed_order(self) -> None:
+        # Test comparing objects with same signatures but different order.
+
+        # Remove all signatures and create new ones.
+        md = Metadata.from_bytes(self.metadata["snapshot"])
+        md.signatures = {}
+        md_2 = copy.deepcopy(md)
+        signatures: List[SSlibSigner] = []
+        # Assign the new signatures to the md object in the straight order.
+        for i in ["0", "1", "2"]:
+            _, signer = RepositorySimulator.create_key()
+            signature = signer.sign(md.to_bytes())
+            signatures.append(signature)
+            md.signatures[i] = signature
+
+        # Assign the new signatures in a reverse order for the md_2 object.
+        for j in [2, 1, 0]:
+            md_2.signatures[str(j)] = signatures[j]
+
+        # Assert that both objects are not the same because of signatures order.
+        self.assertNotEqual(md, md_2)
+
+        # but if we fix the signatures order they will be equal
+        md_2.signatures = {}
+        for j in [0, 1, 2]:
+            md_2.signatures[str(j)] = signatures[j]
+
+        self.assertEqual(md, md_2)
+
     def test_md_eq_special_signatures_tests(self) -> None:
         # Test that metadata objects with different signatures are not equal.
         md = Metadata.from_bytes(self.metadata["snapshot"])
@@ -284,6 +316,64 @@ def test_delegations_eq_(self, test_case_data: Any) -> None:
         setattr(delegations_2, self.case_name, test_case_data)
         self.assertNotEqual(delegations, delegations_2)
 
+    def test_delegations_eq_roles_reversed_order(self) -> None:
+        # Test comparing objects with same delegated roles but different order.
+        role_one_dict = {
+            "keyids": ["keyid1"],
+            "name": "a",
+            "terminating": False,
+            "paths": ["fn1"],
+            "threshold": 1,
+        }
+        role_two_dict = {
+            "keyids": ["keyid2"],
+            "name": "b",
+            "terminating": True,
+            "paths": ["fn2"],
+            "threshold": 4,
+        }
+        role_one = DelegatedRole.from_dict(role_one_dict)
+        role_two = DelegatedRole.from_dict(role_two_dict)
+
+        delegations_dict = {
+            "keys": {
+                "keyid2": {
+                    "keytype": "ed25519",
+                    "scheme": "ed25519",
+                    "keyval": {"public": "bar"},
+                }
+            },
+            "roles": [],
+        }
+        delegations = Delegations.from_dict(delegations_dict)
+        delegations.roles["a"] = role_one
+        delegations.roles["b"] = role_two
+
+        # Create a second delegations obj with reversed roles order
+        delegations_dict_2 = {
+            "keys": {
+                "keyid2": {
+                    "keytype": "ed25519",
+                    "scheme": "ed25519",
+                    "keyval": {"public": "bar"},
+                }
+            },
+            "roles": [],
+        }
+        delegations_2 = Delegations.from_dict(delegations_dict_2)
+        delegations.roles["b"] = copy.deepcopy(role_two)
+        delegations.roles["a"] = copy.deepcopy(role_one)
+
+        # Both objects are not the equal because of delegated roles order.
+        self.assertNotEqual(delegations, delegations_2)
+
+        # but if we fix the delegated roles order they will be equal
+        delegations_2.roles = {}
+        delegations_2.roles["a"] = copy.deepcopy(role_one)
+        delegations_2.roles["b"] = copy.deepcopy(role_two)
+
+        self.assertEqual(delegations, delegations_2)
+
     # Common attributes between TargetFile and MetaFile doesn't need testing.
     targetfile_attributes_modifications: utils.DataSet = {
         "path": "",

tuf/api/metadata.py

@@ -130,10 +130,14 @@ def __eq__(self, other: Any) -> bool:
         if self.signatures.keys() != other.signatures.keys():
             return False
 
-        for sig_info in self.signatures.values():
-            sig = sig_info.signature
-            keyid = sig_info.keyid
-            if sig != other.signatures[keyid].signature:
+        # Iterate over self.signatures and other.signatures at the same time
+        # as the signatures in the file format are ordered.
+        keyids = zip(self.signatures.keys(), other.signatures.keys())
+        for self_keyid, other_keyid in keyids:
+            self_sig = self.signatures[self_keyid].signature
+            other_sig = other.signatures[other_keyid].signature
+
+            if (self_keyid != other_keyid) or (self_sig != other_sig):
                 return False
 
         # Finally, check that the signed portions are the same
@@ -1347,9 +1351,23 @@ def __eq__(self, other: Any) -> bool:
         if not isinstance(other, Delegations):
             return False
 
+        # Assert that roles keys are the same before traversing them.
+        if self.roles.keys() != other.roles.keys():
+            return False
+
+        # Iterate over self.roles and other.roles at the same time
+        # as the order of the roles dictionaries is important.
+        role_names = zip(self.roles.keys(), other.roles.keys())
+        for self_role_name, other_role_name in role_names:
+            self_role = self.roles[self_role_name]
+            other_role = other.roles[other_role_name]
+
+            if (self_role_name != other_role_name) or (self_role != other_role):
+                return False
+
+        # Compare the rest of the fields besides "roles"
         return (
             self.keys == other.keys
-            and self.roles == other.roles
             and self.unrecognized_fields == other.unrecognized_fields
         )