From cc9f3876c49726d5226f8e33c5b0eae9a3691740 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 9 Sep 2021 22:14:21 +0300
Subject: [PATCH 1/4] tests: Shorten variable names to reasonable length

Otherwise absolutely everything is split on multiple lines.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_updater_ng.py | 142 ++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 85 deletions(-)

diff --git a/tests/test_updater_ng.py b/tests/test_updater_ng.py
index cfdc178d94..8281919724 100644
--- a/tests/test_updater_ng.py
+++ b/tests/test_updater_ng.py
@@ -111,10 +111,10 @@ def setUp(self):
 
         self.metadata_url = f"{url_prefix}/metadata/"
         self.targets_url = f"{url_prefix}/targets/"
-        self.destination_directory = self.make_temp_directory()
+        self.dl_dir = self.make_temp_directory()
         # Creating a repository instance.  The test cases will use this client
         # updater to refresh metadata, fetch target files, etc.
-        self.repository_updater = ngclient.Updater(
+        self.updater = ngclient.Updater(
             self.client_directory, self.metadata_url, self.targets_url
         )
 
@@ -187,50 +187,44 @@ def consistent_snapshot_modifier(root):
         self._modify_repository_root(
             consistent_snapshot_modifier, bump_version=True
         )
-        self.repository_updater = ngclient.Updater(
+        self.updater = ngclient.Updater(
             self.client_directory, self.metadata_url, self.targets_url
         )
 
         # All metadata is in local directory already
-        self.repository_updater.refresh()
+        self.updater.refresh()
         # Make sure that consistent snapshot is enabled
         self.assertTrue(
-            self.repository_updater._trusted_set.root.signed.consistent_snapshot
+            self.updater._trusted_set.root.signed.consistent_snapshot
         )
         # Get targetinfo for "file1.txt" listed in targets
-        targetinfo1 = self.repository_updater.get_one_valid_targetinfo(
-            "file1.txt"
-        )
+        info1 = self.updater.get_one_valid_targetinfo("file1.txt")
         # Get targetinfo for "file3.txt" listed in the delegated role1
-        targetinfo3 = self.repository_updater.get_one_valid_targetinfo(
-            "file3.txt"
-        )
+        info3 = self.updater.get_one_valid_targetinfo("file3.txt")
 
         # Create consistent targets with file path HASH.FILENAME.EXT
-        target1_hash = list(targetinfo1.hashes.values())[0]
-        target3_hash = list(targetinfo3.hashes.values())[0]
+        target1_hash = list(info1.hashes.values())[0]
+        target3_hash = list(info3.hashes.values())[0]
         self._create_consistent_target("file1.txt", target1_hash)
         self._create_consistent_target("file3.txt", target3_hash)
 
-        updated_targets = self.repository_updater.updated_targets(
-            [targetinfo1, targetinfo3], self.destination_directory
+        updated_targets = self.updater.updated_targets(
+            [info1, info3], self.dl_dir
         )
 
-        self.assertListEqual(updated_targets, [targetinfo1, targetinfo3])
-        self.repository_updater.download_target(
-            targetinfo1, self.destination_directory
-        )
-        updated_targets = self.repository_updater.updated_targets(
-            updated_targets, self.destination_directory
+        self.assertListEqual(updated_targets, [info1, info3])
+        self.updater.download_target(info1, self.dl_dir)
+
+        updated_targets = self.updater.updated_targets(
+            updated_targets, self.dl_dir
         )
 
-        self.assertListEqual(updated_targets, [targetinfo3])
+        self.assertListEqual(updated_targets, [info3])
 
-        self.repository_updater.download_target(
-            targetinfo3, self.destination_directory
-        )
-        updated_targets = self.repository_updater.updated_targets(
-            updated_targets, self.destination_directory
+        self.updater.download_target(info3, self.dl_dir)
+
+        updated_targets = self.updater.updated_targets(
+            updated_targets, self.dl_dir
         )
 
         self.assertListEqual(updated_targets, [])
@@ -244,47 +238,42 @@ def test_refresh_and_download(self):
         os.remove(os.path.join(self.client_directory, "1.root.json"))
 
         # top-level metadata is in local directory already
-        self.repository_updater.refresh()
+        self.updater.refresh()
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
 
         # Get targetinfo for 'file1.txt' listed in targets
-        targetinfo1 = self.repository_updater.get_one_valid_targetinfo(
-            "file1.txt"
-        )
+        info1 = self.updater.get_one_valid_targetinfo("file1.txt")
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
+
         # Get targetinfo for 'file3.txt' listed in the delegated role1
-        targetinfo3 = self.repository_updater.get_one_valid_targetinfo(
-            "file3.txt"
-        )
+        info3 = self.updater.get_one_valid_targetinfo("file3.txt")
         expected_files = ["role1", "root", "snapshot", "targets", "timestamp"]
         self._assert_files(expected_files)
 
-        updated_targets = self.repository_updater.updated_targets(
-            [targetinfo1, targetinfo3], self.destination_directory
+        updated_targets = self.updater.updated_targets(
+            [info1, info3], self.dl_dir
         )
 
-        self.assertListEqual(updated_targets, [targetinfo1, targetinfo3])
+        self.assertListEqual(updated_targets, [info1, info3])
 
-        self.repository_updater.download_target(
-            targetinfo1, self.destination_directory
-        )
-        updated_targets = self.repository_updater.updated_targets(
-            updated_targets, self.destination_directory
+        self.updater.download_target(info1, self.dl_dir)
+
+        updated_targets = self.updater.updated_targets(
+            updated_targets, self.dl_dir
         )
 
-        self.assertListEqual(updated_targets, [targetinfo3])
+        self.assertListEqual(updated_targets, [info3])
 
         # Check that duplicates are excluded
-        updated_targets = self.repository_updater.updated_targets(
-            [targetinfo3, targetinfo3], self.destination_directory
+        updated_targets = self.updater.updated_targets(
+            [info3, info3], self.dl_dir
         )
-        self.assertListEqual(updated_targets, [targetinfo3])
+        self.assertListEqual(updated_targets, [info3])
 
-        self.repository_updater.download_target(
-            targetinfo3, self.destination_directory
-        )
-        updated_targets = self.repository_updater.updated_targets(
-            updated_targets, self.destination_directory
+        self.updater.download_target(info3, self.dl_dir)
+
+        updated_targets = self.updater.updated_targets(
+            updated_targets, self.dl_dir
         )
 
         self.assertListEqual(updated_targets, [])
@@ -298,70 +287,53 @@ def test_refresh_with_only_local_root(self):
         os.remove(os.path.join(self.client_directory, "1.root.json"))
         self._assert_files(["root"])
 
-        self.repository_updater.refresh()
+        self.updater.refresh()
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
 
         # Get targetinfo for 'file3.txt' listed in the delegated role1
-        targetinfo3 = self.repository_updater.get_one_valid_targetinfo(
-            "file3.txt"
-        )
+        targetinfo3 = self.updater.get_one_valid_targetinfo("file3.txt")
         expected_files = ["role1", "root", "snapshot", "targets", "timestamp"]
         self._assert_files(expected_files)
 
     def test_both_target_urls_not_set(self):
         # target_base_url = None and Updater._target_base_url = None
-        self.repository_updater = ngclient.Updater(
+        self.updater = ngclient.Updater(
             self.client_directory, self.metadata_url
         )
         with self.assertRaises(ValueError):
-            self.repository_updater.download_target(
-                [], self.destination_directory
-            )
+            self.updater.download_target([], self.dl_dir)
 
     def test_external_targets_url(self):
-        self.repository_updater.refresh()
-        targetinfo = self.repository_updater.get_one_valid_targetinfo(
-            "file1.txt"
-        )
-        self.repository_updater.download_target(
-            targetinfo, self.destination_directory, self.targets_url
-        )
+        self.updater.refresh()
+        info = self.updater.get_one_valid_targetinfo("file1.txt")
+
+        self.updater.download_target(info, self.dl_dir, self.targets_url)
 
     def test_length_hash_mismatch(self):
-        self.repository_updater.refresh()
-        targetinfo = self.repository_updater.get_one_valid_targetinfo(
-            "file1.txt"
-        )
+        self.updater.refresh()
+        targetinfo = self.updater.get_one_valid_targetinfo("file1.txt")
+
         length = targetinfo.length
         with self.assertRaises(exceptions.RepositoryError):
             targetinfo.length = 44
-            self.repository_updater.download_target(
-                targetinfo, self.destination_directory
-            )
+            self.updater.download_target(targetinfo, self.dl_dir)
 
         with self.assertRaises(exceptions.RepositoryError):
             targetinfo.length = length
             targetinfo.hashes = {"sha256": "abcd"}
-            self.repository_updater.download_target(
-                targetinfo, self.destination_directory
-            )
+            self.updater.download_target(targetinfo, self.dl_dir)
 
     def test_updating_root(self):
         # Bump root version, resign and refresh
         self._modify_repository_root(lambda root: None, bump_version=True)
-        self.repository_updater.refresh()
-        self.assertEqual(
-            self.repository_updater._trusted_set.root.signed.version, 2
-        )
+        self.updater.refresh()
+        self.assertEqual(self.updater._trusted_set.root.signed.version, 2)
 
     def test_missing_targetinfo(self):
-        self.repository_updater.refresh()
+        self.updater.refresh()
 
         # Get targetinfo for non-existing file
-        targetinfo = self.repository_updater.get_one_valid_targetinfo(
-            "file33.txt"
-        )
-        self.assertIsNone(targetinfo)
+        self.assertIsNone(self.updater.get_one_valid_targetinfo("file33.txt"))
 
 
 if __name__ == "__main__":

From 9b761b86208918a95b6641a8cad8ec7c267b3e84 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 9 Sep 2021 21:21:57 +0300
Subject: [PATCH 2/4] ngclient: Simplify caching

Remove updated_targets() as it doesn't fit the rest of the API.

In its stead add find_cached_target() which has a similar signature
as download_target(): both accept an optional local filepath as
argument and return full local filepath. In the
find_cached_target() case None is returned if the local file is not the
correct target file.

Updater constructor gets a new optional target_dir argument: This means
client can avoid giving a local filepath as an argument to
find_cached_target()/download_target() -- Updater will then generate a
filename within targets_dir.

A reasonable use pattern (when targets_dir is set in constructor):

    info = updater.get_one_valid_targetinfo("targetname")
    path = updater.find_cached_target(info)
    if path is None:
        path = updater.download_target(info)

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_updater_ng.py             | 118 ++++++++++++------------
 tests/test_updater_with_simulator.py |  42 ++++-----
 tuf/ngclient/updater.py              | 130 +++++++++++++++------------
 3 files changed, 151 insertions(+), 139 deletions(-)

diff --git a/tests/test_updater_ng.py b/tests/test_updater_ng.py
index 8281919724..3e62b6c831 100644
--- a/tests/test_updater_ng.py
+++ b/tests/test_updater_ng.py
@@ -16,7 +16,7 @@
 import tuf.unittest_toolbox as unittest_toolbox
 
 from tests import utils
-from tuf.api.metadata import Metadata
+from tuf.api.metadata import Metadata, TargetFile
 from tuf import exceptions, ngclient
 from securesystemslib.signer import SSlibSigner
 from securesystemslib.interface import import_rsa_privatekey_from_file
@@ -115,7 +115,10 @@ def setUp(self):
         # Creating a repository instance.  The test cases will use this client
         # updater to refresh metadata, fetch target files, etc.
         self.updater = ngclient.Updater(
-            self.client_directory, self.metadata_url, self.targets_url
+            repository_dir=self.client_directory,
+            metadata_base_url=self.metadata_url,
+            target_dir=self.dl_dir,
+            target_base_url=self.targets_url
         )
 
     def tearDown(self):
@@ -187,20 +190,22 @@ def consistent_snapshot_modifier(root):
         self._modify_repository_root(
             consistent_snapshot_modifier, bump_version=True
         )
-        self.updater = ngclient.Updater(
-            self.client_directory, self.metadata_url, self.targets_url
+        updater = ngclient.Updater(
+            self.client_directory, self.metadata_url, self.dl_dir, self.targets_url
         )
 
         # All metadata is in local directory already
-        self.updater.refresh()
+        updater.refresh()
         # Make sure that consistent snapshot is enabled
         self.assertTrue(
-            self.updater._trusted_set.root.signed.consistent_snapshot
+            updater._trusted_set.root.signed.consistent_snapshot
         )
-        # Get targetinfo for "file1.txt" listed in targets
-        info1 = self.updater.get_one_valid_targetinfo("file1.txt")
-        # Get targetinfo for "file3.txt" listed in the delegated role1
-        info3 = self.updater.get_one_valid_targetinfo("file3.txt")
+
+        # Get targetinfos, assert cache does not contain the files
+        info1 = updater.get_one_valid_targetinfo("file1.txt")
+        info3 = updater.get_one_valid_targetinfo("file3.txt")
+        self.assertIsNone(updater.find_cached_target(info1))
+        self.assertIsNone(updater.find_cached_target(info3))
 
         # Create consistent targets with file path HASH.FILENAME.EXT
         target1_hash = list(info1.hashes.values())[0]
@@ -208,26 +213,17 @@ def consistent_snapshot_modifier(root):
         self._create_consistent_target("file1.txt", target1_hash)
         self._create_consistent_target("file3.txt", target3_hash)
 
-        updated_targets = self.updater.updated_targets(
-            [info1, info3], self.dl_dir
-        )
-
-        self.assertListEqual(updated_targets, [info1, info3])
-        self.updater.download_target(info1, self.dl_dir)
+        # Download files, assert that cache has correct files
+        updater.download_target(info1)
+        path = updater.find_cached_target(info1)
+        self.assertEqual(path, os.path.join(self.dl_dir, info1.path))
+        self.assertIsNone(updater.find_cached_target(info3))
 
-        updated_targets = self.updater.updated_targets(
-            updated_targets, self.dl_dir
-        )
-
-        self.assertListEqual(updated_targets, [info3])
-
-        self.updater.download_target(info3, self.dl_dir)
-
-        updated_targets = self.updater.updated_targets(
-            updated_targets, self.dl_dir
-        )
-
-        self.assertListEqual(updated_targets, [])
+        updater.download_target(info3)
+        path = updater.find_cached_target(info1)
+        self.assertEqual(path, os.path.join(self.dl_dir, info1.path))
+        path = updater.find_cached_target(info3)
+        self.assertEqual(path, os.path.join(self.dl_dir, info3.path))
 
     def test_refresh_and_download(self):
         # Test refresh without consistent targets - targets without hash prefixes.
@@ -241,7 +237,7 @@ def test_refresh_and_download(self):
         self.updater.refresh()
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
 
-        # Get targetinfo for 'file1.txt' listed in targets
+        # Get targetinfos, assert that cache does not contain files
         info1 = self.updater.get_one_valid_targetinfo("file1.txt")
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
 
@@ -249,34 +245,22 @@ def test_refresh_and_download(self):
         info3 = self.updater.get_one_valid_targetinfo("file3.txt")
         expected_files = ["role1", "root", "snapshot", "targets", "timestamp"]
         self._assert_files(expected_files)
-
-        updated_targets = self.updater.updated_targets(
-            [info1, info3], self.dl_dir
+        self.assertIsNone(self.updater.find_cached_target(info1))
+        self.assertIsNone(self.updater.find_cached_target(info3))
+
+        # Download files, assert that cache has correct files
+        self.updater.download_target(info1)
+        path = self.updater.find_cached_target(info1)
+        self.assertEqual(path, os.path.join(self.dl_dir, info1.path))
+        self.assertIsNone(
+            self.updater.find_cached_target(info3)
         )
 
-        self.assertListEqual(updated_targets, [info1, info3])
-
-        self.updater.download_target(info1, self.dl_dir)
-
-        updated_targets = self.updater.updated_targets(
-            updated_targets, self.dl_dir
-        )
-
-        self.assertListEqual(updated_targets, [info3])
-
-        # Check that duplicates are excluded
-        updated_targets = self.updater.updated_targets(
-            [info3, info3], self.dl_dir
-        )
-        self.assertListEqual(updated_targets, [info3])
-
-        self.updater.download_target(info3, self.dl_dir)
-
-        updated_targets = self.updater.updated_targets(
-            updated_targets, self.dl_dir
-        )
-
-        self.assertListEqual(updated_targets, [])
+        self.updater.download_target(info3)
+        path = self.updater.find_cached_target(info1)
+        self.assertEqual(path, os.path.join(self.dl_dir, info1.path))
+        path = self.updater.find_cached_target(info3)
+        self.assertEqual(path, os.path.join(self.dl_dir, info3.path))
 
     def test_refresh_with_only_local_root(self):
         os.remove(os.path.join(self.client_directory, "timestamp.json"))
@@ -297,17 +281,25 @@ def test_refresh_with_only_local_root(self):
 
     def test_both_target_urls_not_set(self):
         # target_base_url = None and Updater._target_base_url = None
-        self.updater = ngclient.Updater(
-            self.client_directory, self.metadata_url
-        )
+        updater = ngclient.Updater(self.client_directory, self.metadata_url, self.dl_dir)
+        info = TargetFile(1, {"sha256": ""}, "targetpath")
+        with self.assertRaises(ValueError):
+            updater.download_target(info)
+
+    def test_no_target_dir_no_filepath(self):
+        # filepath = None and Updater.target_dir = None
+        updater = ngclient.Updater(self.client_directory, self.metadata_url)
+        info = TargetFile(1, {"sha256": ""}, "targetpath")
+        with self.assertRaises(ValueError):
+            updater.find_cached_target(info)
         with self.assertRaises(ValueError):
-            self.updater.download_target([], self.dl_dir)
+            updater.download_target(info)
 
     def test_external_targets_url(self):
         self.updater.refresh()
         info = self.updater.get_one_valid_targetinfo("file1.txt")
 
-        self.updater.download_target(info, self.dl_dir, self.targets_url)
+        self.updater.download_target(info, target_base_url=self.targets_url)
 
     def test_length_hash_mismatch(self):
         self.updater.refresh()
@@ -316,12 +308,12 @@ def test_length_hash_mismatch(self):
         length = targetinfo.length
         with self.assertRaises(exceptions.RepositoryError):
             targetinfo.length = 44
-            self.updater.download_target(targetinfo, self.dl_dir)
+            self.updater.download_target(targetinfo)
 
         with self.assertRaises(exceptions.RepositoryError):
             targetinfo.length = length
             targetinfo.hashes = {"sha256": "abcd"}
-            self.updater.download_target(targetinfo, self.dl_dir)
+            self.updater.download_target(targetinfo)
 
     def test_updating_root(self):
         # Bump root version, resign and refresh
diff --git a/tests/test_updater_with_simulator.py b/tests/test_updater_with_simulator.py
index 5ea3fc5f20..e1f10ce67e 100644
--- a/tests/test_updater_with_simulator.py
+++ b/tests/test_updater_with_simulator.py
@@ -54,6 +54,7 @@ def _run_refresh(self) -> Updater:
         updater = Updater(
             self.metadata_dir,
             "https://example.com/metadata/",
+            self.targets_dir,
             "https://example.com/targets/",
             self.sim
         )
@@ -90,9 +91,13 @@ def test_refresh(self):
     @utils.run_sub_tests_with_dataset(targets)
     def test_targets(self, test_case_data: Tuple[str, bytes, str]):
         targetpath, content, encoded_path = test_case_data
-        # target does not exist yet
+        path = os.path.join(self.targets_dir, encoded_path)
+
         updater = self._run_refresh()
+        # target does not exist yet, configuration is what we expect
         self.assertIsNone(updater.get_one_valid_targetinfo(targetpath))
+        self.assertTrue(self.sim.root.consistent_snapshot)
+        self.assertTrue(updater.config.prefix_targets_with_hash)
 
         # Add targets to repository
         self.sim.targets.version += 1
@@ -101,30 +106,25 @@ def test_targets(self, test_case_data: Tuple[str, bytes, str]):
 
         updater = self._run_refresh()
         # target now exists, is not in cache yet
-        file_info = updater.get_one_valid_targetinfo(targetpath)
-        self.assertIsNotNone(file_info)
-        self.assertEqual(
-            updater.updated_targets([file_info], self.targets_dir),
-            [file_info]
-        )
+        info = updater.get_one_valid_targetinfo(targetpath)
+        self.assertIsNotNone(info)
+        # Test without and with explicit local filepath
+        self.assertIsNone(updater.find_cached_target(info))
+        self.assertIsNone(updater.find_cached_target(info, path))
 
-        # Assert consistent_snapshot is True and downloaded targets have prefix.
-        self.assertTrue(self.sim.root.consistent_snapshot)
-        self.assertTrue(updater.config.prefix_targets_with_hash)
         # download target, assert it is in cache and content is correct
-        local_path = updater.download_target(file_info, self.targets_dir)
-        self.assertEqual(
-            updater.updated_targets([file_info], self.targets_dir), []
-        )
-        self.assertTrue(local_path.startswith(self.targets_dir))
-        with open(local_path, "rb") as f:
-            self.assertEqual(f.read(), content)
-
-        # Assert that the targetpath was URL encoded as expected.
-        encoded_absolute_path = os.path.join(self.targets_dir, encoded_path)
-        self.assertEqual(local_path, encoded_absolute_path)
+        self.assertEqual(path, updater.download_target(info))
+        self.assertEqual(path, updater.find_cached_target(info))
+        self.assertEqual(path, updater.find_cached_target(info, path))
 
+        with open(path, "rb") as f:
+            self.assertEqual(f.read(), content)
 
+        # download using explicit filepath as well
+        os.remove(path)
+        self.assertEqual(path, updater.download_target(info, path))
+        self.assertEqual(path, updater.find_cached_target(info))
+        self.assertEqual(path, updater.find_cached_target(info, path))
 
     def test_fishy_rolenames(self):
         roles_to_filenames = {
diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py
index fdbcec4ed3..fee09904e9 100644
--- a/tuf/ngclient/updater.py
+++ b/tuf/ngclient/updater.py
@@ -24,8 +24,8 @@
       * :func:`~tuf.ngclient.updater.Updater.get_one_valid_targetinfo()` is
         used to find information about a specific target. This will load new
         targets metadata as needed (from local cache or remote repository).
-      * :func:`~tuf.ngclient.updater.Updater.updated_targets()` can be used to
-        check if target files are already locally cached.
+      * :func:`~tuf.ngclient.updater.Updater.find_cached_target()` can be used
+        to check if a target file is already locally cached.
       * :func:`~tuf.ngclient.updater.Updater.download_target()` downloads a
         target file and ensures it is verified correct by the metadata.
 
@@ -43,27 +43,30 @@
 
     from tuf.ngclient import Updater
 
-    # Load trusted local root metadata from client metadata cache. Define the
-    # remote repository metadata URL prefix and target URL prefix.
+    # Load trusted local root metadata from client metadata cache. Define
+    # where metadata and targets will be downloaded from.
     updater = Updater(
         repository_dir="~/tufclient/metadata/",
         metadata_base_url="http://localhost:8000/tuf-repo/",
+        target_dir="~/tufclient/downloads/",
         target_base_url="http://localhost:8000/targets/",
     )
 
     # Update top-level metadata from remote
     updater.refresh()
 
-    # Securely download a target:
-    # Update target metadata, then download and verify target
-    targetinfo = updater.get_one_valid_targetinfo("file.txt")
-    updater.download_target(targetinfo, "~/tufclient/downloads/")
+    # Update metadata, then download target if needed
+    info = updater.get_one_valid_targetinfo("file.txt")
+    path = updater.find_cached_target(info)
+    if path is None:
+        path = updater.download_target(info)
+    print(f"Local file {path} contains target {info.path}")
 """
 
 import logging
 import os
 import tempfile
-from typing import List, Optional, Set, Tuple
+from typing import Optional, Set, Tuple
 from urllib import parse
 
 from securesystemslib import util as sslib_util
@@ -84,6 +87,9 @@ class Updater:
         repository_dir: Local metadata directory. Directory must be
             writable and it must contain a trusted root.json file.
         metadata_base_url: Base URL for all remote metadata downloads
+        target_dir: Local targets directory. Directory must be writable. It
+            will be used as the default target download directory by
+            ``find_cached_target()`` and ``download_target()``
         target_base_url: Optional; Default base URL for all remote target
             downloads. Can be individually set in download_target()
         fetcher: Optional; FetcherInterface implementation used to download
@@ -98,12 +104,14 @@ def __init__(
         self,
         repository_dir: str,
         metadata_base_url: str,
+        target_dir: Optional[str] = None,
         target_base_url: Optional[str] = None,
         fetcher: Optional[FetcherInterface] = None,
         config: Optional[UpdaterConfig] = None,
     ):
         self._dir = repository_dir
         self._metadata_base_url = _ensure_trailing_slash(metadata_base_url)
+        self.target_dir = target_dir
         if target_base_url is None:
             self._target_base_url = None
         else:
@@ -145,7 +153,7 @@ def get_one_valid_targetinfo(
         """Returns TargetFile instance with information for 'target_path'.
 
         The return value can be used as an argument to
-        :func:`download_target()` and :func:`updated_targets()`.
+        :func:`download_target()` and :func:`find_cached_target()`.
 
         :func:`refresh()` must be called before calling
         `get_one_valid_targetinfo()`. Subsequent calls to
@@ -173,59 +181,64 @@ def get_one_valid_targetinfo(
         """
         return self._preorder_depth_first_walk(target_path)
 
-    @staticmethod
-    def updated_targets(
-        targets: List[TargetFile], destination_directory: str
-    ) -> List[TargetFile]:
-        """Checks whether local cached target files are up to date
+    def find_cached_target(
+        self,
+        targetinfo: TargetFile,
+        filepath: Optional[str] = None,
+    ) -> Optional[str]:
+        """Checks whether a local file is an up to date target
 
-        After retrieving the target information for the targets that should be
-        updated, updated_targets() can be called to determine which targets
-        have changed compared to locally stored versions.
+        If the ``target_dir`` argument was given to constructor, ``filepath``
+        argument is not required here: in this case a filename will be
+        generated based on the target path like in ``download_target()``
 
-        All the targets that are not up-to-date in destination_directory are
-        returned in a list. The list items can be downloaded with
-        'download_target()'.
-        """
-        # Keep track of TargetFiles and local paths. Return 'updated_targets'
-        # and use 'local_paths' to avoid duplicates.
-        updated_targets: List[TargetFile] = []
-        local_paths: List[str] = []
-
-        for target in targets:
-            # URL encode to get local filename like download_target() does
-            filename = parse.quote(target.path, "")
-            local_path = os.path.join(destination_directory, filename)
+        Args:
+            targetinfo: TargetFile from ``get_one_valid_targetinfo()``.
+            filepath: Local path to file. By default a filename is generated
+                and file is looked for in ``target_dir`` (see note above).
 
-            if local_path in local_paths:
-                continue
+        Raises:
+            ValueError: Incorrect arguments
 
-            try:
-                with open(local_path, "rb") as target_file:
-                    target.verify_length_and_hashes(target_file)
-            # If the file does not exist locally or length and hashes
-            # do not match, append to updated targets.
-            except (OSError, exceptions.LengthOrHashMismatchError):
-                updated_targets.append(target)
-                local_paths.append(local_path)
+        Returns:
+            Local file path if the file is an up to date target file, or None
+            if file is not found or it is not up to date.
+        """
+        if filepath is not None:
+            pass
+        elif self.target_dir is not None:
+            # Use URL encoded target path as name, like download_target()
+            filename = parse.quote(targetinfo.path, "")
+            filepath = os.path.join(self.target_dir, filename)
+        else:
+            raise ValueError("filepath or target_dir must be set")
 
-        return updated_targets
+        try:
+            with open(filepath, "rb") as target_file:
+                targetinfo.verify_length_and_hashes(target_file)
+            return filepath
+        except (OSError, exceptions.LengthOrHashMismatchError):
+            return None
 
     def download_target(
         self,
         targetinfo: TargetFile,
-        destination_directory: str,
+        filepath: Optional[str] = None,
         target_base_url: Optional[str] = None,
     ) -> str:
-        """Downloads the target file specified by 'targetinfo'.
+        """Downloads the target file specified by ``targetinfo``.
+
+        If the ``target_dir`` argument was given to constructor, ``filepath``
+        argument is not required here: in this case a filename will be
+        generated based on the target path.
 
         Args:
-            targetinfo: TargetFile instance received from
-                get_one_valid_targetinfo() or updated_targets().
-            destination_directory: existing local directory to download into.
-                Note that new directories may be created inside
-                destination_directory as required.
-            target_base_url: Optional; Base URL used to form the final target
+            targetinfo: TargetFile with target information from
+                get_one_valid_targetinfo().
+            filepath: Local path to download into. By default the file is
+                downloaded into ``target_dir`` (see note above) with a
+                generated filename. If file already exists, it is overwritten.
+            target_base_url: Base URL used to form the final target
                 download URL. Default is the value provided in Updater()
 
         Raises:
@@ -237,6 +250,15 @@ def download_target(
             Path to downloaded file
         """
 
+        if filepath is not None:
+            pass
+        elif self.target_dir is not None:
+            # Use URL encoded target path as name
+            filename = parse.quote(targetinfo.path, "")
+            filepath = os.path.join(self.target_dir, filename)
+        else:
+            raise ValueError("Either filepath or target_dir must be set")
+
         if target_base_url is None:
             if self._target_base_url is None:
                 raise ValueError(
@@ -266,12 +288,10 @@ def download_target(
                     f"{target_filepath} length or hashes do not match"
                 ) from e
 
-            # Use a URL encoded targetpath as the local filename
-            filename = parse.quote(targetinfo.path, "")
-            local_filepath = os.path.join(destination_directory, filename)
-            sslib_util.persist_temp_file(target_file, local_filepath)
+            sslib_util.persist_temp_file(target_file, filepath)
 
-            return local_filepath
+        logger.info("Downloaded target %s", targetinfo.path)
+        return filepath
 
     def _download_metadata(
         self, rolename: str, length: int, version: Optional[int] = None

From d519a413b0a9e7ada2ecd63658a17bfe423373e9 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Mon, 13 Sep 2021 10:12:48 +0300
Subject: [PATCH 3/4] ngclient: Rename get_one_valid_targetinfo()

This is slightly cosmetic but rename get_one_valid_targetinfo to
get_targetinfo:
* The function name is long without any reason: "one" and "valid" are
  always implicit
* shortening makes code (incl. our examples and tests) easier to read
* We're also already changing updater API (compared to legacy) so this
  alone does not break things -- it's also not a difficult "port".

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_updater_ng.py             | 16 ++++++++--------
 tests/test_updater_with_simulator.py |  6 +++---
 tuf/ngclient/updater.py              | 28 ++++++++++++----------------
 3 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/tests/test_updater_ng.py b/tests/test_updater_ng.py
index 3e62b6c831..69fbeef8cd 100644
--- a/tests/test_updater_ng.py
+++ b/tests/test_updater_ng.py
@@ -202,8 +202,8 @@ def consistent_snapshot_modifier(root):
         )
 
         # Get targetinfos, assert cache does not contain the files
-        info1 = updater.get_one_valid_targetinfo("file1.txt")
-        info3 = updater.get_one_valid_targetinfo("file3.txt")
+        info1 = updater.get_targetinfo("file1.txt")
+        info3 = updater.get_targetinfo("file3.txt")
         self.assertIsNone(updater.find_cached_target(info1))
         self.assertIsNone(updater.find_cached_target(info3))
 
@@ -238,11 +238,11 @@ def test_refresh_and_download(self):
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
 
         # Get targetinfos, assert that cache does not contain files
-        info1 = self.updater.get_one_valid_targetinfo("file1.txt")
+        info1 = self.updater.get_targetinfo("file1.txt")
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
 
         # Get targetinfo for 'file3.txt' listed in the delegated role1
-        info3 = self.updater.get_one_valid_targetinfo("file3.txt")
+        info3 = self.updater.get_targetinfo("file3.txt")
         expected_files = ["role1", "root", "snapshot", "targets", "timestamp"]
         self._assert_files(expected_files)
         self.assertIsNone(self.updater.find_cached_target(info1))
@@ -275,7 +275,7 @@ def test_refresh_with_only_local_root(self):
         self._assert_files(["root", "snapshot", "targets", "timestamp"])
 
         # Get targetinfo for 'file3.txt' listed in the delegated role1
-        targetinfo3 = self.updater.get_one_valid_targetinfo("file3.txt")
+        targetinfo3 = self.updater.get_targetinfo("file3.txt")
         expected_files = ["role1", "root", "snapshot", "targets", "timestamp"]
         self._assert_files(expected_files)
 
@@ -297,13 +297,13 @@ def test_no_target_dir_no_filepath(self):
 
     def test_external_targets_url(self):
         self.updater.refresh()
-        info = self.updater.get_one_valid_targetinfo("file1.txt")
+        info = self.updater.get_targetinfo("file1.txt")
 
         self.updater.download_target(info, target_base_url=self.targets_url)
 
     def test_length_hash_mismatch(self):
         self.updater.refresh()
-        targetinfo = self.updater.get_one_valid_targetinfo("file1.txt")
+        targetinfo = self.updater.get_targetinfo("file1.txt")
 
         length = targetinfo.length
         with self.assertRaises(exceptions.RepositoryError):
@@ -325,7 +325,7 @@ def test_missing_targetinfo(self):
         self.updater.refresh()
 
         # Get targetinfo for non-existing file
-        self.assertIsNone(self.updater.get_one_valid_targetinfo("file33.txt"))
+        self.assertIsNone(self.updater.get_targetinfo("file33.txt"))
 
 
 if __name__ == "__main__":
diff --git a/tests/test_updater_with_simulator.py b/tests/test_updater_with_simulator.py
index e1f10ce67e..8cfb9de72a 100644
--- a/tests/test_updater_with_simulator.py
+++ b/tests/test_updater_with_simulator.py
@@ -95,7 +95,7 @@ def test_targets(self, test_case_data: Tuple[str, bytes, str]):
 
         updater = self._run_refresh()
         # target does not exist yet, configuration is what we expect
-        self.assertIsNone(updater.get_one_valid_targetinfo(targetpath))
+        self.assertIsNone(updater.get_targetinfo(targetpath))
         self.assertTrue(self.sim.root.consistent_snapshot)
         self.assertTrue(updater.config.prefix_targets_with_hash)
 
@@ -106,7 +106,7 @@ def test_targets(self, test_case_data: Tuple[str, bytes, str]):
 
         updater = self._run_refresh()
         # target now exists, is not in cache yet
-        info = updater.get_one_valid_targetinfo(targetpath)
+        info = updater.get_targetinfo(targetpath)
         self.assertIsNotNone(info)
         # Test without and with explicit local filepath
         self.assertIsNone(updater.find_cached_target(info))
@@ -145,7 +145,7 @@ def test_fishy_rolenames(self):
         updater = self._run_refresh()
 
         # trigger updater to fetch the delegated metadata, check filenames
-        updater.get_one_valid_targetinfo("anything")
+        updater.get_targetinfo("anything")
         local_metadata = os.listdir(self.metadata_dir)
         for fname in roles_to_filenames.values():
             self.assertTrue(fname in local_metadata)
diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py
index fee09904e9..5094ae3620 100644
--- a/tuf/ngclient/updater.py
+++ b/tuf/ngclient/updater.py
@@ -21,7 +21,7 @@
     snapshot is consistent so multiple targets can be downloaded without
     fear of repository content changing. For each target:
 
-      * :func:`~tuf.ngclient.updater.Updater.get_one_valid_targetinfo()` is
+      * :func:`~tuf.ngclient.updater.Updater.get_targetinfo()` is
         used to find information about a specific target. This will load new
         targets metadata as needed (from local cache or remote repository).
       * :func:`~tuf.ngclient.updater.Updater.find_cached_target()` can be used
@@ -56,7 +56,7 @@
     updater.refresh()
 
     # Update metadata, then download target if needed
-    info = updater.get_one_valid_targetinfo("file.txt")
+    info = updater.get_targetinfo("file.txt")
     path = updater.find_cached_target(info)
     if path is None:
         path = updater.download_target(info)
@@ -131,7 +131,7 @@ def refresh(self) -> None:
         all the checks required in the TUF client workflow.
 
         The metadata for delegated roles are not refreshed by this method as
-        that happens on demand during get_one_valid_targetinfo().
+        that happens on demand during get_targetinfo().
 
         The refresh() method should be called by the client before any other
         method calls.
@@ -147,19 +147,16 @@ def refresh(self) -> None:
         self._load_snapshot()
         self._load_targets("targets", "root")
 
-    def get_one_valid_targetinfo(
-        self, target_path: str
-    ) -> Optional[TargetFile]:
+    def get_targetinfo(self, target_path: str) -> Optional[TargetFile]:
         """Returns TargetFile instance with information for 'target_path'.
 
         The return value can be used as an argument to
         :func:`download_target()` and :func:`find_cached_target()`.
-
         :func:`refresh()` must be called before calling
-        `get_one_valid_targetinfo()`. Subsequent calls to
-        `get_one_valid_targetinfo()` will use the same consistent repository
+        `get_targetinfo()`. Subsequent calls to
+        `get_targetinfo()` will use the same consistent repository
         state: Changes that happen in the repository between calling
-        :func:`refresh()` and `get_one_valid_targetinfo()` will not be
+        :func:`refresh()` and `get_targetinfo()` will not be
         seen by the updater.
 
         As a side-effect this method downloads all the additional (delegated
@@ -193,7 +190,7 @@ def find_cached_target(
         generated based on the target path like in ``download_target()``
 
         Args:
-            targetinfo: TargetFile from ``get_one_valid_targetinfo()``.
+            targetinfo: TargetFile from ``get_targetinfo()``.
             filepath: Local path to file. By default a filename is generated
                 and file is looked for in ``target_dir`` (see note above).
 
@@ -201,8 +198,8 @@ def find_cached_target(
             ValueError: Incorrect arguments
 
         Returns:
-            Local file path if the file is an up to date target file, or None
-            if file is not found or it is not up to date.
+            Local file path if the file is an up to date target file.
+            None if file is not found or it is not up to date.
         """
         if filepath is not None:
             pass
@@ -233,8 +230,7 @@ def download_target(
         generated based on the target path.
 
         Args:
-            targetinfo: TargetFile with target information from
-                get_one_valid_targetinfo().
+            targetinfo: TargetFile from ``get_targetinfo()``.
             filepath: Local path to download into. By default the file is
                 downloaded into ``target_dir`` (see note above) with a
                 generated filename. If file already exists, it is overwritten.
@@ -247,7 +243,7 @@ def download_target(
             TODO: file write errors
 
         Returns:
-            Path to downloaded file
+            Local path to downloaded file
         """
 
         if filepath is not None:

From 6aaa1ead5974e91ba868a4fd6f483b82540f4bae Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Wed, 27 Oct 2021 10:19:00 +0300
Subject: [PATCH 4/4] ngclient: Refactor target path generation

Also tweak the docstrings: the "caching" target_dir usage is
presented in the module doc example: there should be no need for
additional comments in the methods themselves as long as the argument
docs are readable.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/ngclient/updater.py | 48 ++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py
index 5094ae3620..5857be13d4 100644
--- a/tuf/ngclient/updater.py
+++ b/tuf/ngclient/updater.py
@@ -147,6 +147,14 @@ def refresh(self) -> None:
         self._load_snapshot()
         self._load_targets("targets", "root")
 
+    def _generate_target_file_path(self, targetinfo: TargetFile) -> str:
+        if self.target_dir is None:
+            raise ValueError("target_dir must be set if filepath is not given")
+
+        # Use URL encoded target path as filename
+        filename = parse.quote(targetinfo.path, "")
+        return os.path.join(self.target_dir, filename)
+
     def get_targetinfo(self, target_path: str) -> Optional[TargetFile]:
         """Returns TargetFile instance with information for 'target_path'.
 
@@ -185,14 +193,10 @@ def find_cached_target(
     ) -> Optional[str]:
         """Checks whether a local file is an up to date target
 
-        If the ``target_dir`` argument was given to constructor, ``filepath``
-        argument is not required here: in this case a filename will be
-        generated based on the target path like in ``download_target()``
-
         Args:
             targetinfo: TargetFile from ``get_targetinfo()``.
-            filepath: Local path to file. By default a filename is generated
-                and file is looked for in ``target_dir`` (see note above).
+            filepath: Local path to file. If None, a file path is generated
+            based on ``target_dir`` constructor argument.
 
         Raises:
             ValueError: Incorrect arguments
@@ -201,14 +205,9 @@ def find_cached_target(
             Local file path if the file is an up to date target file.
             None if file is not found or it is not up to date.
         """
-        if filepath is not None:
-            pass
-        elif self.target_dir is not None:
-            # Use URL encoded target path as name, like download_target()
-            filename = parse.quote(targetinfo.path, "")
-            filepath = os.path.join(self.target_dir, filename)
-        else:
-            raise ValueError("filepath or target_dir must be set")
+
+        if filepath is None:
+            filepath = self._generate_target_file_path(targetinfo)
 
         try:
             with open(filepath, "rb") as target_file:
@@ -225,15 +224,12 @@ def download_target(
     ) -> str:
         """Downloads the target file specified by ``targetinfo``.
 
-        If the ``target_dir`` argument was given to constructor, ``filepath``
-        argument is not required here: in this case a filename will be
-        generated based on the target path.
-
         Args:
             targetinfo: TargetFile from ``get_targetinfo()``.
-            filepath: Local path to download into. By default the file is
-                downloaded into ``target_dir`` (see note above) with a
-                generated filename. If file already exists, it is overwritten.
+            filepath: Local path to download into. If None, the file is
+                downloaded into directory defined by ``target_dir`` constructor
+                argument using a generated filename. If file already exists,
+                it is overwritten.
             target_base_url: Base URL used to form the final target
                 download URL. Default is the value provided in Updater()
 
@@ -246,14 +242,8 @@ def download_target(
             Local path to downloaded file
         """
 
-        if filepath is not None:
-            pass
-        elif self.target_dir is not None:
-            # Use URL encoded target path as name
-            filename = parse.quote(targetinfo.path, "")
-            filepath = os.path.join(self.target_dir, filename)
-        else:
-            raise ValueError("Either filepath or target_dir must be set")
+        if filepath is None:
+            filepath = self._generate_target_file_path(targetinfo)
 
         if target_base_url is None:
             if self._target_base_url is None: