ngclient updater: API functions argument validation

[MVrachev]

Description of issue or feature request:
In the tuf/ngclient/updater.py module we are providing a couple API functions with arguments: Updater.__init__, get_one_valid_targetinfo, updated_targets and download_targets.
It seems to me it's a good idea to decide which arguments in those functions require validation and if there should be any what it should be.

Current behavior:
No validation is done for:

• repository_dir in Updater.__init__()
• target_path in Updater.get_targetinfo()
• maybe targetinfo in Updater.download_target()
• maybe target_base_url in Updater.download_target()

Expected behavior:
Decide if we want to add validation and if there is any what it should be:

• repository_dir in Updater.__init__()
• target_path in Updater.get_targetinfo()
• targetinfo in Updater.download_target()
• target_base_url in Updater.download_target()

[MVrachev]

I will document some of my thoughts:

• repository_dir in Updater.__init__(): I think we should check if the repository_dir exists during the initialization and fail early.
  Otherwise when calling Updater._load_local_metadata("root") an exception will be thrown.

• target_path in Updater.get_one_valid_targetinfo(): We should validate that target_path is a non-empty string.
  Otherwise, if target_path is not a string a KeyError will be thrown when trying to access information in targets:
  

  python-tuf/tuf/ngclient/updater.py

  Line 417 in 82b0967

  target = role_metadata.targets.get(target_filepath)

  
  Additionally, we should validate that `target_path points to a file and not a directory. As only files could be targets.

• targets in Updater.updated_targets(): Not sure if validation is needed here. Traversing the list in for the cycle is already checking that targets is at least a container that could be iterated.

• destination_directory in Updater.updated_targets(): I think we should validate that the directory exists. Otherwise FileNotFoundError when trying to open it

  python-tuf/tuf/ngclient/updater.py

  Line 203 in 82b0967

  target_filepath = os.path.join(destination_directory, target.path)

  which could be a little more ambiguous.

• targetinfo in Updater.download_target(): Not sure if we need validation here. targetinfo is expected to be received from get_one_valid_targetinfo and there will be validation.

• destination_directory in Updater.download_target(): I think we should validate that the directory exists. Otherwise FileNotFoundError when trying to open it during persisting the target.

• target_base_url in Updater.download_target(): Not sure about that one as it already has some validation when calling _ensure_trailing_slash. This ensures that at least the target_base_url is a string otherwise AttributeError will be raised because endswith() won't be an attribute to it:

  python-tuf/tuf/ngclient/updater.py

  Line 464 in 82b0967

  return url if url.endswith("/") else f"{url}/"

[jku]

let's leave this on back burner until #1580 is resolved: arguments (and how they are used) is likely to change slightly

__init__() could be fixed already but I actually think the current FileNotFoundError is fine.

[MVrachev]

I have updated the issue description addressing the new changes merged in #1604.

• After Ngclient api polish #1604, I no longer see a point in validating targetinfo and filepath arguments in find_cached_target.
  If filepath is an empty string the os will complain it can't open the file. The same is if if targetinfo.path has an empty string assigned for path.

• If targetInfo has to be validated as if it has an empty path or 0 length that will be noticed
  when calling with self._fetcher.download_file(full_url, targetinfo.length) and either the file won't be found for download or max_legth in download_file will be 0 and the download will fail.

• I am not sure about target_base_url as well. Again if it's a nonvalid URL that should be noticed when downloading the file right?

• I am wondering should we do something to validate target_path in Updater.get_targetinfo() at all.
  It's used to retrieve check a target and return the target information.
  Given that we are adding a sanity check that TARGETPATHs should be a non-empty string, then the responsibility of validating TARGETPATHs is given to Metadata API.

[MVrachev]

What do you think @jku? Am I correct in my assumptions?

Then, the only one left is repository_dir in Updater.__init__().
We can validate that repository_dir does exist and throw an error otherwise. This will help the users to know during initialization that they have provided a non-existent directory to correct themselves.

[jku]

WRT validating TargetFile arguments, I guess the potential check would be "is this really a TargetFile instance?" but considering that A) we support static type checking (well almost anyway: #1633) and B) there is never any need to modify the targetfile in application code C) it will fail pretty gracefully anyway, I don't think that check is needed...

We can validate that repository_dir does exist and throw an error otherwise

I think this currently ends up complaining that repository_dir/root.json does not exist -- which seems like an accurate description of the problem: the updater isn't interested in just the directory, it needs a directory with a root.json.

So potentially we don't need to do anything here: I'm fine with that

[MVrachev]

@jku I assume you agree with my thoughts about the rest of the attributes?

I agree with your thoughts about TargetFile arguments and repository_dir.
Then, we don't want to change anything and we can close this issue, correct?

[jku]

Yep, I think we agree here, let's close