Skip to content

Metadata API: Make Role.keyids ordered #1754

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Metadata API: Make Role.keyids ordered #1754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tests/test_metadata_serialization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ def test_invalid_role_serialization(self, test_case_data: str) -> None:
valid_roles: utils.DataSet = {
"all": '{"keyids": ["keyid"], "threshold": 3}',
"many keyids": '{"keyids": ["a", "b", "c", "d", "e"], "threshold": 1}',
"ordered keyids": '{"keyids": ["c", "b", "a"], "threshold": 1}',
"empty keyids": '{"keyids": [], "threshold": 1}',
"unrecognized field": '{"keyids": ["keyid"], "threshold": 3, "foo": "bar"}',
}
Expand Down Expand Up @@ -295,6 +296,8 @@ def test_snapshot_serialization(self, test_case_data: str) -> None:
"unrecognized field": '{"keyids": ["keyid"], "name": "a", "terminating": true, "paths": ["fn1"], "threshold": 3, "foo": "bar"}',
"many keyids": '{"keyids": ["keyid1", "keyid2"], "name": "a", "paths": ["fn1", "fn2"], \
"terminating": false, "threshold": 1}',
"ordered keyids": '{"keyids": ["keyid2", "keyid1"], "name": "a", "paths": ["fn1", "fn2"], \
"terminating": false, "threshold": 1}',
}

@utils.run_sub_tests_with_dataset(valid_delegated_roles)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_trusted_metadata_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ def test_update_root_new_root_fail_threshold_verification(self) -> None:
# new_root data with threshold which cannot be verified.
root = Metadata.from_bytes(self.metadata[Root.type])
# remove root role keyids representing root signatures
root.signed.roles[Root.type].keyids = set()
root.signed.roles[Root.type].keyids.clear()
with self.assertRaises(exceptions.UnsignedMetadataError):
self.trusted_set.update_root(root.to_bytes())

Expand Down
18 changes: 8 additions & 10 deletions tuf/api/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,15 +676,11 @@ def __init__(
threshold: int,
unrecognized_fields: Optional[Mapping[str, Any]] = None,
):
keyids_set = set(keyids)
if len(keyids_set) != len(keyids):
raise ValueError(
f"keyids should be a list of unique strings,"
f" instead got {keyids}"
)
if len(set(keyids)) != len(keyids):
raise ValueError(f"Nonunique keyids: {keyids}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, feel free to ignore

Suggested change
raise ValueError(f"Nonunique keyids: {keyids}")
raise ValueError(f"Non-unique keyids: {keyids}")

if threshold < 1:
raise ValueError("threshold should be at least 1!")
self.keyids = keyids_set
self.keyids = keyids
self.threshold = threshold
self.unrecognized_fields: Mapping[str, Any] = unrecognized_fields or {}

Expand All @@ -699,7 +695,7 @@ def from_dict(cls, role_dict: Dict[str, Any]) -> "Role":
def to_dict(self) -> Dict[str, Any]:
"""Returns the dictionary representation of self."""
return {
"keyids": sorted(self.keyids),
"keyids": self.keyids,
"threshold": self.threshold,
**self.unrecognized_fields,
}
Expand Down Expand Up @@ -789,7 +785,8 @@ def add_key(self, role: str, key: Key) -> None:
"""
if role not in self.roles:
raise ValueError(f"Role {role} doesn't exist")
self.roles[role].keyids.add(key.keyid)
if key.keyid not in self.roles[role].keyids:
self.roles[role].keyids.append(key.keyid)
self.keys[key.keyid] = key

def remove_key(self, role: str, keyid: str) -> None:
Expand Down Expand Up @@ -1474,7 +1471,8 @@ def add_key(self, role: str, key: Key) -> None:
"""
if self.delegations is None or role not in self.delegations.roles:
raise ValueError(f"Delegated role {role} doesn't exist")
self.delegations.roles[role].keyids.add(key.keyid)
if key.keyid not in self.delegations.roles[role].keyids:
self.delegations.roles[role].keyids.append(key.keyid)
self.delegations.keys[key.keyid] = key

def remove_key(self, role: str, keyid: str) -> None:
Expand Down