refactor a keys test

tuf_conformance/test_keys.py

@@ -8,69 +8,77 @@
 def initial_setup_for_key_threshold(
     client: ClientRunner, repo: RepositorySimulator, init_data: ClientInitData
 ) -> None:
-    # Explicitly set the threshold
+    """This is a helper for tests for key thresholds.
+    It sets a threshold of 3 for snapshot metadata,
+    adds three keys, bumps the repo metadata and
+    refreshes the client"""
+
+    # Set treshold to 3 for snapshot metadata
     repo.root.roles[Snapshot.type].threshold = 3
-    repo.bump_root_by_one()  # v2
 
-    # Add a legitimate key:
+    # Add three keys for snapshot:
     repo.add_key(Snapshot.type)
-    repo.update_timestamp()
-    repo.update_snapshot()
-    repo.bump_root_by_one()  # v3
-
-    assert client.refresh(init_data) == 1
-
-    # Add a legitimate key:
     repo.add_key(Snapshot.type)
     repo.add_key(Snapshot.type)
     repo.update_timestamp()
-    repo.update_snapshot()
-    repo.bump_root_by_one()  # v4
+    repo.update_snapshot()  # v2
+    repo.bump_root_by_one()  # v2
 
     assert len(repo.root.roles[Snapshot.type].keyids) == 4
 
     assert client.refresh(init_data) == 0
-    assert client.version(Snapshot.type) == 3
-    assert client.version(Root.type) == 4
+    assert client.version(Snapshot.type) == 2
+    assert client.version(Root.type) == 2
 
 
-def test_root_has_keys_but_not_snapshot(
+def test_root_meets_threshold_but_snapshot_does_not(
     client: ClientRunner, server: SimulatorServer
 ) -> None:
-    """This test adds keys to the repo root MD to test for cases
-    where are client might calculate the threshold from only the
-    roots keys and not check that the snapshot MD has the same
-    keys"""
+    """In this test, repo.root.roles.snapshot.threshold is 5,
+    and there are 5 keyids in repo.root.roles.snapshot.keyids
+    which meets the threshold. However, repo.snapshot.signatures
+    only has 4 of the 5 keys listed in repo.root.roles.snapshot.keyids,
+    so repo.snapshot.signatures does not meet the threshold. The client
+    should therefore not refresh the snapshot metadata from the repo.
+
+    Specifically, the goal is to bring the repository into a state where:
+
+    1. repo.root.roles.snapshot.keyids has the following keyids:
+      1.a: ccce1a69b4eea9daf315d8a9c43fffd8ee541bfb41fdcb773657192af51d3cfa
+      1.b: 4f7c454c55a81918fa1fc1f513d510f36c3efa796711d9f5883602f91a5b9da7
+      1.c: 146e0dc5038d3b8772f5d4bd6626fe1436f656494b501628a2b54d6aa6a6edfe
+      1.d: 3c80449ebd33c67ff8d9befce23c534b50d97ed805e52da8c746e6a1daefab8e
+    2: repo.root.roles.snapshot.threshold is 4
+    3: repo.snapshot.signatures has the following keyids:
+      3.a: ccce1a69b4eea9daf315d8a9c43fffd8ee541bfb41fdcb773657192af51d3cfa
+      3.b: 4f7c454c55a81918fa1fc1f513d510f36c3efa796711d9f5883602f91a5b9da7
+      3.c: 146e0dc5038d3b8772f5d4bd6626fe1436f656494b501628a2b54d6aa6a6edfe
+
+    The test ensures that clients do not update snapshot in this scenario.
+    """
     init_data, repo = server.new_test(client.test_name)
 
     assert client.init_client(init_data) == 0
     assert client.refresh(init_data) == 0
 
     initial_setup_for_key_threshold(client, repo, init_data)
 
-    # Increase the threshold
-    repo.root.roles[Snapshot.type].threshold = 5
-    repo.bump_root_by_one()  # v5
+    # Set repo.root.roles.snapshot.threshold to 4 and remove
+    # a signer from snapshot metadata. The snapshot metadata
+    # then only has 3 signers which is below the threshold.
+    repo.root.roles[Snapshot.type].threshold = 4
+    repo.signers[Snapshot.type].popitem()
+    repo.bump_root_by_one()  # v3
+    repo.update_snapshot()  # v3
 
-    # Refresh should fail: root updates but there's no new snapshot:
-    # The existing snapshot does not meet threshold anymore.
+    # The snapshot does not meet the threshold anymore,
+    # because there are 3 snapshot signers keys, and the
+    # threshold is 4.
     # NOTE: we don't actually expect clients to delete the
     # file from trusted_roles() at this point
     assert client.refresh(init_data) == 1
-    assert client.version(Root.type) == 5
-
-    # Add two keyids only to root and expect the client
-    # to fail updating
-    signer = repo.new_signer()
-    repo.root.roles[Snapshot.type].keyids.append(signer.public_key.keyid)
-
-    # Sanity check
-    assert len(repo.root.roles[Snapshot.type].keyids) == 5
-
-    # Updating should fail. Root should bump, but not snapshot
-    assert client.refresh(init_data) == 1
-    assert client.version(Root.type) == 5
-    assert client.version(Snapshot.type) == 3
+    assert client.version(Root.type) == 3
+    assert client.version(Snapshot.type) == 2
 
 
 def test_wrong_hashing_algorithm(client: ClientRunner, server: SimulatorServer) -> None:
@@ -94,8 +102,8 @@ def test_wrong_hashing_algorithm(client: ClientRunner, server: SimulatorServer)
     repo.root.keys[valid_key].unrecognized_fields = dict()
     alg_key = "keyid_hash_algorithms"
     repo.root.keys[valid_key].unrecognized_fields[alg_key] = ["md5"]
-    repo.bump_root_by_one()  # v5
-    repo.update_snapshot()  # v4
+    repo.bump_root_by_one()  # v4
+    repo.update_snapshot()  # v3
 
     # All metadata should update; even though "keyid_hash_algorithms"
     # is wrong, it is not a part of the TUF spec. This is the tests