Skip to content

Commit

Permalink
API CHANGE: ValueError in add/remove key in Root
Browse files Browse the repository at this point in the history
This is an API change to the exceptions thrown in Root.add_key()
and Root.remove_key().
The reason for that change is that in my opinion the correct exceptions
in these cases should be "ValueError" instead of "KeyError" as
the problems are in the given values - role doesn't exist or
key is not used by a particular role.

Additionally, document the thrown exceptions in "Root.add_key" and
add a test which invokes that exception.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
  • Loading branch information
MVrachev committed Sep 20, 2021
1 parent 6e05cdd commit 85fb461
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 3 deletions.
8 changes: 7 additions & 1 deletion tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,10 @@ def test_root_add_key_and_remove_key(self):
# Add the same key to targets role as well
root.signed.add_key('targets', key_metadata)

# Add the same key to a nonexistent role.
with self.assertRaises(ValueError):
root.signed.add_key("nosuchrole", key_metadata)

# Remove the key from root role (targets role still uses it)
root.signed.remove_key('root', keyid)
self.assertNotIn(keyid, root.signed.roles['root'].keyids)
Expand All @@ -476,8 +480,10 @@ def test_root_add_key_and_remove_key(self):
self.assertNotIn(keyid, root.signed.roles['targets'].keyids)
self.assertNotIn(keyid, root.signed.keys)

with self.assertRaises(KeyError):
with self.assertRaises(ValueError):
root.signed.remove_key('root', 'nosuchkey')
with self.assertRaises(ValueError):
root.signed.remove_key('nosuchrole', keyid)

def test_is_target_in_pathpattern(self):
supported_use_cases = [
Expand Down
14 changes: 12 additions & 2 deletions tuf/api/metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -760,16 +760,26 @@ def to_dict(self) -> Dict[str, Any]:

# Update key for a role.
def add_key(self, role: str, key: Key) -> None:
"""Adds new signing key for delegated role 'role'."""
"""Adds new signing key for delegated role 'role'.
Raises:
ValueError: If 'role' doesn't exist.
"""
if role not in self.roles:
raise ValueError(f"Role {role} doesn't exist")
self.roles[role].keyids.add(key.keyid)
self.keys[key.keyid] = key

def remove_key(self, role: str, keyid: str) -> None:
"""Removes key from 'role' and updates the key store.
Raises:
KeyError: If 'role' does not include the key
ValueError: If 'role' does not include the key.
"""
if role not in self.roles:
raise ValueError(f"Role {role} doesn't exist")
if keyid not in self.roles[role].keyids:
raise ValueError(f"Key with id {keyid} is not used by {role}")
self.roles[role].keyids.remove(keyid)
for keyinfo in self.roles.values():
if keyid in keyinfo.keyids:
Expand Down

0 comments on commit 85fb461

Please sign in to comment.