Skip to content

Issue #698: Fix roles list keys - #755

Open
ZCute1 wants to merge 2 commits into
openlawlibrary:masterfrom
ZCute1:fix-roles-list-keys
Open

Issue #698: Fix roles list keys#755
ZCute1 wants to merge 2 commits into
openlawlibrary:masterfrom
ZCute1:fix-roles-list-keys

Conversation

@ZCute1

@ZCute1 ZCute1 commented Jul 3, 2026

Copy link
Copy Markdown

Description

Fix taf roles list-keys when called without a role argument so it prints each key with its associated role(s), instead of only showing key IDs.

Also added CLI test coverage for taf roles list-keys --path <repo> with no role positional argument.

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

@CLAassistant

CLAassistant commented Jul 3, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@ZCute1 ZCute1 changed the title Fix roles list keys Issue #698: Fix roles list keys Jul 3, 2026

@renatav renatav left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think is very close to being what we need. My main concern is keeping the public api as intuitive and clean as possible, so it might be a good idea to refactor this a bit before merging.

Comment thread taf/api/roles.py
return f"Role: {role_label}\n" + "\n".join(key_lines)


def _roles_for_keys(auth_repo: AuthenticationRepository) -> Dict[str, set]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It was surprising that a similar utility was not already somewhere in the codebase. Let's move this to tuf/repository/MetadataRepository

Comment thread taf/api/roles.py

def _roles_for_keys(auth_repo: AuthenticationRepository) -> Dict[str, set]:
roles_for_key: Dict[str, set] = defaultdict(set)
for role_name in auth_repo.get_all_roles():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This works, but is not the most efficient solution. That is fine as we do not expect to ever encounter a situation where we have hundreds of roles.

get_all_roles() walks the full delegation tree and in doing so, it already knows each role's parent, but throws that away and returns a flat list. Then, for each of those role names, get_role_keys(role_name) has to call find_delegated_roles_parent(role). Implementing something similar to find_keysid_roles would be more efficient.

However, this is fine as is, unless you want to tackle the harder implementation as a way of getting more familiar with the system.

Comment thread taf/api/roles.py
on_exceptions=TAFError,
reraise=True,
)
def list_keys_of_role(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's actually better to split the two up:kKeep list_keys_of_role(path, role) exactly as it was (role required), and add a new function for the "all roles" case list_keys_by_roles. This preserves the existing function's contract, and the existing tests should pass without needing any modifications. And then the CLI could call one or the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants