-
Notifications
You must be signed in to change notification settings - Fork 272
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
New API: Add DelegationRole and Delegations classes #1370
Conversation
2f56b8e
to
f426415
Compare
elif metadata == "targets" and dict1["signed"].get("delegations"): | ||
for keyid in dict1["signed"]["delegations"]["keys"].keys(): | ||
dict1["signed"]["delegations"]["keys"][keyid]["d"] = "c" | ||
new_roles = [] | ||
for role in dict1["signed"]["delegations"]["roles"]: | ||
role["e"] = "g" | ||
new_roles.append(role) | ||
dict1["signed"]["delegations"]["roles"] = new_roles | ||
dict1["signed"]["delegations"]["foo"] = "bar" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is ugly, but until @jku proposes his test where he automatically validates support for unrecognized fields on each level of the dictionary, we would have to stick to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason I haven't done it is that while there seemingly was a consensus that "unrecognised keys should be allowed everywhere in the file format" ... it's not really true. There's a lot of places where that would be weird and unsafe (what if someone inserts random data into "keys" or "roles") and we should not allow that. The real rule is probably "unreconised keys should be allowed at every object root level": so you should be allowed to insert a key into Signed, Key or Role ... but not into any dictionary where we cannot know the recognised key names before hand (like "keys") or where the allowed keys are strictly defined (like "meta").
Poisoning everything except a hard-coded list of dictionaries (instead of only poisoning specific dicts) may still be the better testing solution but it's not trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's an implementation in #1382 now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take another look after #1367 fixes are done and this is rebased: currently it looks buggy
f426415
to
0b96636
Compare
Rebased on top of the latest develop branch and addressed all comments made by @jku. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks.
I left one typo comment and have a naming question but it's basically ✔️ for me.
Naming: Would DelegatedRole be better than DelegationRole -- maybe it conveys a little bit more information (in that this clearly is the role that is delegated, not the one that is delegating)? I don't feel strongly about it though, just want to make sure we try to find the best names...
Yes, I agree |
ae2d644
to
8519016
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
9815917
to
198025b
Compare
In the top level metadata classes, there are complex attributes such as "meta" in Targets and Snapshot, "key" and "roles" in Root etc. We want to represent those complex attributes with a class to allow easier verification and support for metadata with unrecognized fields. For more context read ADR 0004 and ADR 0008 in the docs/adr folder. DelegatedRole shares a couple of fields with the Role class and that's why it inherits it. I decided to use a separate Delegations class because I thought it will make it easier to read, verify and add additional helper functions. Also, I tried to make sure that I test each level of the delegations representation for support of storing unrecognized fields. Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
Stop using Mapping where we actually mean Dict: Mapping means "we only need a read-only dict" and most of the time this is not really the case. Signed-off-by: Martin Vrachev <[email protected]>
198025b
to
b2cde9b
Compare
Rebased on top of the latest master branch. |
Addresses #1139
Description of the changes being introduced by the pull request:
In the top-level metadata classes, there are complex attributes such as
"meta" in Targets and Snapshot, "key" and "roles" in Root etc.
We want to represent those complex attributes with a class to allow
easier verification and support for metadata with unrecognized fields.
For more context read ADR 0004 and ADR 0008 in the docs/adr folder.
DelegationRole shares a couple of fields with the Role class and that's
why it inherits it.
I decided to use a separate Delegations class because I thought it will
make it easier to read, verify and add additional helper functions.
Also, I tried to make sure that I test each level of the delegations
representation for support of storing unrecognized fields.
Please verify and check that the pull request fulfills the following
requirements: