Fix snapshot_filename inconsistency usage

First in the generate_timestamp_metadata both "snapshot_filename"
and the constant SNAPSHOT_FILENAME are used which is redundant
and possibly confusing. There should be only one input
for the snapshot file name.

Second, when calling the generate_timestamp_metadata there are
cases when "snapshot_filename" is in reality "snapshot_file_path".
That's what led to the need for the addition of SNAPSHOT_FILENAME
when populating the "meta" field from the TIMESTAMP_SCHEMA.
For the same reason, it seems logical to me to rename snapshot_filename
to snapshot_file_path and explicitly take the snapshot file name
from it.

Third, in the _generate_and_write_metadata function the argument
"filenames" is by default None, but at the same time without check
it's considered that filenames is a dictionary which has a key
"snapshot". This is could be okay if the default "filenames" value
was not None, but in the current situation it's easy to call
"_generate_and_write_metadata" with rolename = timestamp
and forget to populate the filenames dictionary.

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

Author: MVrachev

tests/test_repository_lib.py

@@ -606,7 +606,7 @@ def _setup_generate_timestamp_metadata_test(self):
                                       repo_lib.METADATA_STAGED_DIRECTORY_NAME)
     targets_directory = os.path.join(repository_directory, repo_lib.TARGETS_DIRECTORY_NAME)
 
-    snapshot_filename = os.path.join(metadata_directory,
+    snapshot_file_path = os.path.join(metadata_directory,
                                      repo_lib.SNAPSHOT_FILENAME)
 
     # Set valid generate_timestamp_metadata() arguments.
@@ -622,15 +622,15 @@ def _setup_generate_timestamp_metadata_test(self):
     repository_junk = repo_tool.load_repository(repository_directory,
         repository_name)
 
-    return snapshot_filename, version, expiration_date, storage_backend, \
+    return snapshot_file_path, version, expiration_date, storage_backend, \
         repository_name
 
 
   def test_generate_timestamp_metadata(self):
-    snapshot_filename, version, expiration_date, storage_backend, \
+    snapshot_file_path, version, expiration_date, storage_backend, \
       repository_name = self._setup_generate_timestamp_metadata_test()
 
-    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_filename,
+    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_file_path,
         version, expiration_date, storage_backend, repository_name)
     self.assertTrue(tuf.formats.TIMESTAMP_SCHEMA.matches(timestamp_metadata))
 
@@ -640,19 +640,19 @@ def test_generate_timestamp_metadata(self):
         repo_lib.generate_timestamp_metadata, 3, version, expiration_date,
         storage_backend, repository_name)
     self.assertRaises(securesystemslib.exceptions.FormatError,
-        repo_lib.generate_timestamp_metadata, snapshot_filename, '3',
+        repo_lib.generate_timestamp_metadata, snapshot_file_path, '3',
         expiration_date, storage_backend, repository_name)
     self.assertRaises(securesystemslib.exceptions.FormatError,
-        repo_lib.generate_timestamp_metadata, snapshot_filename, version, '3',
+        repo_lib.generate_timestamp_metadata, snapshot_file_path, version, '3',
         storage_backend, repository_name)
 
 
 
   def test_generate_timestamp_metadata_without_length(self):
-    snapshot_filename, version, expiration_date, storage_backend, \
+    snapshot_file_path, version, expiration_date, storage_backend, \
       repository_name = self._setup_generate_timestamp_metadata_test()
 
-    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_filename,
+    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_file_path,
         version, expiration_date, storage_backend, repository_name,
         use_length=False)
     self.assertTrue(tuf.formats.TIMESTAMP_SCHEMA.matches(timestamp_metadata))
@@ -666,10 +666,10 @@ def test_generate_timestamp_metadata_without_length(self):
 
 
   def test_generate_timestamp_metadata_without_hashes(self):
-    snapshot_filename, version, expiration_date, storage_backend, \
+    snapshot_file_path, version, expiration_date, storage_backend, \
       repository_name = self._setup_generate_timestamp_metadata_test()
 
-    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_filename,
+    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_file_path,
         version, expiration_date, storage_backend, repository_name,
         use_hashes=False)
     self.assertTrue(tuf.formats.TIMESTAMP_SCHEMA.matches(timestamp_metadata))
@@ -683,10 +683,10 @@ def test_generate_timestamp_metadata_without_hashes(self):
 
 
   def test_generate_timestamp_metadata_without_length_and_hashes(self):
-    snapshot_filename, version, expiration_date, storage_backend, \
+    snapshot_file_path, version, expiration_date, storage_backend, \
       repository_name = self._setup_generate_timestamp_metadata_test()
 
-    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_filename,
+    timestamp_metadata = repo_lib.generate_timestamp_metadata(snapshot_file_path,
         version, expiration_date, storage_backend, repository_name,
         use_hashes=False, use_length=False)
     self.assertTrue(tuf.formats.TIMESTAMP_SCHEMA.matches(timestamp_metadata))

tuf/repository_lib.py

@@ -137,8 +137,11 @@ def _generate_and_write_metadata(rolename, metadata_filename,
         SNAPSHOT_EXPIRES_WARN_SECONDS)
 
   elif rolename == 'timestamp':
-    snapshot_filename = filenames['snapshot']
-    metadata = generate_timestamp_metadata(snapshot_filename, roleinfo['version'],
+    # If filenames don't have "snapshot_filename" key, defaults to "snapshot.json"
+    snapshot_file_path = (filenames and filenames['snapshot']) \
+        or SNAPSHOT_FILENAME
+
+    metadata = generate_timestamp_metadata(snapshot_file_path, roleinfo['version'],
         roleinfo['expires'], storage_backend, repository_name,
         use_length=use_timestamp_length, use_hashes=use_timestamp_hashes)
 
@@ -156,9 +159,10 @@ def _generate_and_write_metadata(rolename, metadata_filename,
     # Don't hash-prefix consistent target files if they are handled out of band
     consistent_targets = consistent_snapshot and not use_existing_fileinfo
 
-    metadata = generate_targets_metadata(targets_directory, roleinfo['paths'],
-        roleinfo['version'], roleinfo['expires'], roleinfo['delegations'],
-        consistent_targets, use_existing_fileinfo, storage_backend)
+    metadata = generate_targets_metadata(targets_directory,
+        roleinfo['paths'], roleinfo['version'], roleinfo['expires'],
+        roleinfo['delegations'], consistent_targets, use_existing_fileinfo,
+        storage_backend)
 
   # Before writing 'rolename' to disk, automatically increment its version
   # number (if 'increment_version_number' is True) so that the caller does not
@@ -1523,16 +1527,17 @@ def generate_snapshot_metadata(metadata_directory, version, expiration_date,
 
 
 
-def generate_timestamp_metadata(snapshot_filename, version, expiration_date,
+
+def generate_timestamp_metadata(snapshot_file_path, version, expiration_date,
     storage_backend, repository_name, use_length=True, use_hashes=True):
   """
   <Purpose>
     Generate the timestamp metadata object.  The 'snapshot.json' file must
     exist.
 
   <Arguments>
-    snapshot_filename:
-      The required filename of the snapshot metadata file.  The timestamp role
+    snapshot_file_path:
+      Path to the required snapshot metadata file.  The timestamp role
       needs to the calculate the file size and hash of this file.
 
     version:
@@ -1578,7 +1583,7 @@ def generate_timestamp_metadata(snapshot_filename, version, expiration_date,
   # This check ensures arguments have the appropriate number of objects and
   # object types, and that all dict keys are properly named.
   # Raise 'securesystemslib.exceptions.FormatError' if the check fails.
-  securesystemslib.formats.PATH_SCHEMA.check_match(snapshot_filename)
+  securesystemslib.formats.PATH_SCHEMA.check_match(snapshot_file_path)
   tuf.formats.METADATAVERSION_SCHEMA.check_match(version)
   securesystemslib.formats.ISO8601_DATETIME_SCHEMA.check_match(expiration_date)
   securesystemslib.formats.NAME_SCHEMA.check_match(repository_name)
@@ -1587,15 +1592,16 @@ def generate_timestamp_metadata(snapshot_filename, version, expiration_date,
 
   snapshot_fileinfo = {}
 
-  length, hashes = securesystemslib.util.get_file_details(snapshot_filename,
+  length, hashes = securesystemslib.util.get_file_details(snapshot_file_path,
       tuf.settings.FILE_HASH_ALGORITHMS, storage_backend)
 
   length = (use_length and length) or None
   hashes = (use_hashes and hashes) or None
 
+  _, snapshot_filename = os.path.split(snapshot_file_path)
   # Retrieve the versioninfo of the Snapshot metadata file.
   snapshot_version = get_metadata_versioninfo('snapshot', repository_name)
-  snapshot_fileinfo[SNAPSHOT_FILENAME] = \
+  snapshot_fileinfo[snapshot_filename] = \
       tuf.formats.make_metadata_fileinfo(snapshot_version['version'],
           length, hashes)