ng client improve TrustedMetadataSet testing

[MVrachev]

Fixes #1465

Description of the changes being introduced by the pull request:
The current situation with the TrustedMetadataSet testing is that
we don't have a minimal amount of unit tests testing the different
branches in the various API functionality in the class.

This commit proposes simple unit tests covering almost all of the
branches in the API functions and increasing the unit test coverage
(as reported from the "coverage" tool) from 74 % to 97 %.

The code could be complicated at places because the different
branches in the update_* functions depend on other metadata classes
as well.

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

History of modifications made on the pr:

1. An initial version of the pr:
   1.1 Move the shared code between tests into the "setupClass" function.
   1.2 Add longer test functions with multiple test cases inside them
2. Split longer test functions into multiple smaller ones
   2.1 Added a couple of setup helper functions calling trusted_metadata_set.update_*
   aiming to move the common code between tests into separate functions.
3. Addressing multiple suggestions and optimizations by @jku.
4. Experimenting with Jussi suggestion that instead of modifying internal trusted_metadata_set state
   we should modify the input data we initially feed for trusted_metadata_set.
   4.1 Add modify_snapshot, modify_timestamp helper functions splitting common code that
   now can be reused. They end up with quite complicated functions because I wanted to reuse them
   in different scenarios.
5. By suggestion of @jku, replacing modify_snapshot and modify_timestamp with a
   higher order function modify_metadata which accepts a modification function as a parameter
   then calls it. This allows the actual modification function to be defined inside the test itself and
   be a lot simpler and still reuse a lot of the code responsible to serialization, signing and other
   operations inside the modify_metadata function.

Please verify and check that the pull request fulfills the following
requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[MVrachev]

Ready for review after #1408 has been merged.

[sechkova]

LGTM with small comments below.

[MVrachev]

Rebased on top of develop branch to get the latest changes related to hashes and length validation.

Also, I added two new commits:

• the first one TrustedMetadataSet testing: start using setUp uses setUp to create a trusted metadata set instance per test
• the second one splits the big test functions into smaller ones separated by a test case. This required creating a couple of extra small helpful functions.

Can you have another look @sechkova and tell me do you think it's better that way using many smaller functions?

[jku]

Sooo many comments, sorry about that... but it is a big patch and again the first of it's kind.

I like the smaller function, much easier to read.

many tests finish by modifying some trusted_set metadata internals: that should never be needed, or am I missing something?

There's a lot of hash calculating that I think isn't needed: MetaFile hashes are optional, you can just remove the hashes if they are there.

Many tests follow this pattern:

• update TrustedMetadataSet using valid metadata
• modify TrustedMetadataSet internal metadata
• do the test

In some cases that's what you have to do... but in a lot of cases I think it might be beneficial if we could avoid modifying internal state and instead could do:

• modify metadata before feeding it to TrustedMetadataSet
• Update TrustedMetadataSet with modified metadata (triggering the test)

It's possible that this change is not worth the trouble or should be in another PR... but let's consider at least

[MVrachev]

I addressed your comments @jku.

[jku]

I'm seeing several smaller issues in the code that I've commented on and discussion was marked resolved. I'm not sure how to interpret those. I guess I'll unresolve. Please try to comment in the discussions if you don't think fixes are needed.

Do you have an opinion on the pattern I mentioned (modifying TrustedMetadataSet internals during the test): are there solutions to avoid that, what would they look like, do we want to try them or leave for later?

[jku]

wait, what is this line trying to test for?

How does it succeed, is this a TrustedMetadataSet bug?

[jku]

I think this is just an unneeded call...

The call succeeds (even though delegated target role1 has already been loaded) against what's said in the TrustedMetadataSet docs "Metadata is not updatable if any metadata depending on it has been loaded". We ended up with a version that does not explicitly check that I think because the meta info in snapshot should ensure that the targets metadata is consistent even if a delegating metadata gets "reloaded". That said, there is now a possibility of a repository doing weird things like publishing a new "targets.json" with same version but modified delegations... maybe it's not something we need to care about, but at least the TrustedMetadataSet docs should be updated (not in this PR)

[jku]

actually the quote from the docs is for top-level metadata so it is technically correct... Might make sense to mention that delegating targets consistency comes from snapshots meta information.

[MVrachev]

That said, there is now a possibility of a repository doing weird things like publishing a new "targets.json" with same version but modified delegations... maybe it's not something we need to care about, but at least the TrustedMetadataSet docs should be updated (not in this PR)

I guess we don't care about that because if the file is signed from the keys trusted by root.json then everything is fine right?

We haven't documented which roles are used for verification when calling all other update_* functions.
Why do we want to add this specifically here?

PS: I will remove the redundant two updates of update_targets and update_delegated_targets.

[MVrachev]

I'm seeing several smaller issues in the code that I've commented on and discussion was marked resolved. I'm not sure how to interpret those. I guess I'll unresolve. Please try to comment in the discussions if you don't think fixes are needed.

I am sorry that I closed discussions that weren't addressed.
I rebased a couple of times locally and I was distracted.

Do you have an opinion on the pattern I mentioned (modifying TrustedMetadataSet internals during the test): are there solutions to avoid that, what would they look like, do we want to try them or leave for later?

I have to experiment with this and will do that on Monday.
My only concern is that it could be more complicated as most of the time a test depends on the state of other metadata classes inside the set.

So, for example I want to check if new_snapshot.version != trusted timestamp.meta["snapshot"].version will fail.
I will have to do the following operations:

To do it the way you propose I will have to:

1. instantiate a timestamp object from bytes
2. change the timestamp_obj.meta["snapshot"].version
3. convert timestamp_obj to bytes again
4. call update_timestamp
5. perform the test

Compared to now where I do:

1. call update_timestamp
2. change the internal timestamp.signed.meta["snapshot.json"].version
3. perform the test.

PS: I have submitted #1490 which I hope will simplify the code a little.

[jku]

To do it the way you propose I will have to:

1. instantiate a timestamp object from bytes
2. change the timestamp_obj.meta["snapshot"].version
3. convert timestamp_obj to bytes again
4. call update_timestamp
5. perform the test

You make it sound like this is a lot more complex but I'm not sure it is

For example, the current test:

    def test_update_snapshot_cannot_verify_snapshot_with_threshold(self):
        self._root_update_finished_and_update_timestamp()
        # remove signature for snapshot from root data
        self.trusted_set.root.signed.roles["snapshot"].keyids = []
        with self.assertRaises(exceptions.UnsignedMetadataError):
            self.trusted_set.update_snapshot(self.metadata["snapshot"])
        self.trusted_set.root.signed.roles["snapshot"].threshold = 1

could be something like

    def test_update_snapshot_cannot_verify_snapshot_with_threshold(self):
        self._root_update_finished_and_update_timestamp()

        md = Metadata.from_bytes(self.metadata["snapshot"])
        md.signatures.clear()

        with self.assertRaises(exceptions.UnsignedMetadataError):
            self.trusted_set.update_snapshot(JSONSerializer().serialize(md))

That does not look complex in comparison.

Sure, in some cases you need to sign the resulting metadata as well: I'm sure there could be a helper function that makes the whole edit process reasonable for individual tests.

I'm really not saying we need to do this now though -- this PR is big enough, feel free to leave it as is to move things along.

I am saying that as general rule, tests are better when they do not modify internal state: we will likely keep refactoring the implementation and then slowly we will have less trust on the tests (as they rely on the internals that have now changed)

[MVrachev]

@jku I decide to experiment with updating the tests so that we don't change the internal state of the trusted metadata set.

I was wondering do I want to call self._root_updated_and_update_timestamp(self._modify_timestamp_meta(version=3))
directly or instead do it like I did in the tests

modified_timestamp = self._modify_timestamp_meta(version=3)
self._root_updated_and_update_timestamp(modified_timestamp)

What do you think of the way I implemented it?
Does the new helper function look understandable?

[MVrachev]

As suggested by @jku, in my last commit I removed the modify_timestamp and modify_snapshot functions and instead defined modify_metadata(rolename: str, modification_func: Callable[["Signed"], None]) function which does the common thinks
like:

• instantiating a metadata object
• calling the modification function
• signing the modified object
• serializing back to bytes

[jku]

I think that's a pretty good solution with the constraints you have... It's not immediately obvious how modify_metadata() works but reader can figure it out and it makes test code very readable.

The one suggestion I still have is about hashes and length in timestamp: I understand you don't want to modify the test json file but you could just set hashes and length to None in setUpClass() once. That would simplify code in 9 different tests.

I'll approve this as is, you decide.

[jku]

Approved, see comment above.

[MVrachev]

@jku I applied your suggestion in my latest commit and additionally noticed there are more places where I can simplify the code or add annotations.