Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metadata API validation: validate root roles dictionary content #1516

Closed
jku opened this issue Aug 9, 2021 · 7 comments · Fixed by #1630
Closed

Metadata API validation: validate root roles dictionary content #1516

jku opened this issue Aug 9, 2021 · 7 comments · Fixed by #1630
Assignees
Labels
backlog Issues to address with priority for current development goals good first issue Bite-sized items for first time contributors
Milestone

Comments

@jku
Copy link
Member

jku commented Aug 9, 2021

Spec says this about root roles:

A role for each of "root", "snapshot", "timestamp", and "targets" MUST be specified in the key list. The role of "mirror" is OPTIONAL.

The reference to "key list" seems like a mistake (maybe referring to an earlier design?) but the intent seems clear: the roles dictionary is allowed to contain either the roles for ["root", "snapshot", "timestamp", "targets"] or the roles for ["root", "snapshot", "timestamp", "targets", "mirror"] (and I suppose we are not going to implement mirror for now)). Other roles are not valid and not having the first four roles is not valid.

It makes sense to validate this in Root construction/deserialization.

Aside/thought: maybe it also makes sense to validate in serialization but I think documenting that roles dict is not intended to be modified goes a long way.

@jku jku changed the title Metadata API validation: validate root roles keys Metadata API validation: validate root roles dictionary content Aug 9, 2021
@jku
Copy link
Member Author

jku commented Aug 10, 2021

These seem to be the steps:

  • validate roles dictionary keys in Root.__init__(): keys should be ["root", "snapshot", "timestamp", "targets"]
  • update failing tests (at leasts test_metadata_serialization valid_roots dataset makes assumptions about this)
  • add a test for wrong roles

Additionally:

  • roles should be annotated as Mapping instead of Dict (this prevents nothing but informs users the dict should not be modified)

@jku jku added the backlog Issues to address with priority for current development goals label Aug 18, 2021
@joshuagl joshuagl added the good first issue Bite-sized items for first time contributors label Aug 18, 2021
@jku
Copy link
Member Author

jku commented Aug 25, 2021

I guess this is a good time to think if the Root constructor signature makes sense: roles could be just a Tuple[Role, Role, Role, Role] or the individual roles could be named arguments themselves -- there's no need for roles to be a dict since we know what the keys are.

This would make Constructing Root from scratch a little easier, and from_dict() could still do the validation of keys -- but I'm not sure if this is still worth it: I'm fine with just doing what the previous comment says. For reference this is how weird the construction of an empty Root looks like now:

roles = {
    "root": Role([], 1),
    "targets": Role([], 1),
    "snapshot": Role([], 1),
    "timestamp": Role([], 1),
}
root = Root(1, "1.0.19", expiry_date, {}, roles, True)

@MVrachev
Copy link
Collaborator

MVrachev commented Oct 20, 2021

I guess this is a good time to think if the Root constructor signature makes sense: roles could be just a Tuple[Role, Role, Role, Role] or the individual roles could be named arguments themselves -- there's no need for roles to be a dict since we know what the keys are.

Maybe then we should add additional validation that roles contain exactly four roles representing the top-level metadata roles?
It's said in the spec that:
A role for each of "root", "snapshot", "timestamp", and "targets" MUST be specified in the roles object.
The only drawback I see is that we will have always to provide 4 roles even during testing.

Also about what you said above:

update failing tests (at leasts test_metadata_serialization valid_roots dataset makes assumptions about this)

If we leave it as it is and don't check that root contains exactly 4 roles with the correct naming, then I don't see what do we have to update in that dataset.

@jku
Copy link
Member Author

jku commented Oct 20, 2021

Maybe then we should add additional validation that roles contain exactly four roles representing the top-level metadata roles?

Are you now talking about my off the cuff idea of changing the constructor or the current situation? Whenever we take a dictionary as input we should validate that the four correct keys are there -- A Root object without exactly four correct roles is invalid and shouldn't never exist IMO. So as a minimum we need to do it on from_dict() but if the constructor takes a dict as it does now and if we store the dict, then it makes sense to do the validation in the constructor instead.

Now, I would like to make the constructors easier to use for the repository use case but there is no concrete proposal so I don't think you should worry about that too much in this issues. Anyway, possibly we can avoid big API changes (like the one I proposed above) and e.g. just accept None as roles in constructor : then it's easier to create a new Root (and the constructor would generate the default roles in my example).

@MVrachev
Copy link
Collaborator

Are you now talking about my off the cuff idea of changing the constructor or the current situation?

I wanted to raise discussion for all of the topics you mentioned.

Whenever we take a dictionary as input we should validate that the four correct keys are there -- A Root object without exactly four correct roles is invalid and shouldn't never exist IMO.

Agree. Validation for this should be added as well and this will require changing the valid_roots dataset in test_metadata_serialization.py.

Now, I would like to make the constructors easier to use for the repository use case but there is no concrete proposal so I don't think you should worry about that too much in this issues. Anyway, possibly we can avoid big API changes (like the one I proposed above)

I agree with this, but I am not sure about this part:

and e.g. just accept None as roles in constructor : then it's easier to create a new Root (and the constructor would generate the default roles in my example).

you don't suggest doing that API change in this issue right?

@jku
Copy link
Member Author

jku commented Oct 21, 2021

you don't suggest doing that API change in this issue right?

I don't: only reason to discuss these here is just in case they would make the validation obsolete... That doesn't seem likely so let's just forget it for now.

@MVrachev
Copy link
Collaborator

Okay, then I can fix this issue because it's connected with another one I am already involved in #1356.

@MVrachev MVrachev self-assigned this Oct 21, 2021
@MVrachev MVrachev added this to the Sprint 10 milestone Oct 21, 2021
@jku jku closed this as completed in #1630 Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals good first issue Bite-sized items for first time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants