-
Notifications
You must be signed in to change notification settings - Fork 270
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
Prevents delegate role name as top-level role name #1690
Prevents delegate role name as top-level role name #1690
Conversation
This commit adds the validation in the ``metadata.Delegations`` to prevent that one of the delegate role names given is a top-level role name. A ``ValueError`` will be raised if one of the roles names in the list given to as delegated contains the role name as one of the top-level roles. Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
Pull Request Test Coverage Report for Build 1516182630
💛 - Coveralls |
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.
Thank you @kairoaraujo for your pr and effort!
There are a couple of things to fix, but it's a great start.
Please note that at the current stage mirrors
won't be implemented in the new implementation. For more reference read the second point from issue #1317.
tests/test_metadata_serialization.py
Outdated
@@ -360,6 +360,43 @@ def test_delegation_serialization(self, test_case_data: str): | |||
delegation = Delegations.from_dict(copy.deepcopy(case_dict)) | |||
self.assertDictEqual(case_dict, delegation.to_dict()) | |||
|
|||
valid_delegations: utils.DataSet = { |
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.
Can you please use the dataset defined at line 321
invalid_delegations: utils.DataSet = { |
and the test function defined in 336
def test_invalid_delegation_serialization(self, test_case_data: str): |
instead of defining a
valid_delegations
and a new test function?
We aim to group all valid tests related to a specific class in one dataset and test function.
Also, the name valid_delegations
is not correct.
Those are actually cases of invalid_delegations
as when you call Delegations.from_dict(copy.deepcopy(case_dict))
it causes ValueError
as you catch it inside your the test function.
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 have a concern about how to make sure that the ValueError
is the correct unit we want.
I mean, we test the ValueError
raised, but we don't check if the message comes from the part we want to cover.
Unless we don't want to go deep in this level, it is ok.
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 think this is a more general question that probably is worth adressing.
I think for sure we want to have all invalid tests for Delegations
in one single place, but I understand what you mean.
Maybe we can check the error message and type inside the with?
What do you think @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.
Yes, indeed. If agreed in checking the type and error message, we can address it in another issue.
- Reuse the dataset and the existing tests - Fix the keyids in the tests datasets to be aligned - Fix the ``ValueError`` message aligned to the existent messages Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
The issue discussion mentions also preventing delegated roles from being empty strings. Do you mind adding this check as part of this PR? Sorry if it's been done already somewhere and I've missed that. |
- Add the check for empty strings in the Delegate Role name - Remove the comprehensive lists to make the code more readable - Remove the test for empty file name from ``test_updater_with_simulator`` Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
tuf/api/metadata.py
Outdated
for role in set(roles): | ||
if not role or role in TOP_LEVEL_ROLE_NAMES: | ||
raise ValueError( | ||
"Delegated roles cannot be empty or use top-level role names" | ||
) |
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.
You still can write the check as a one-line like this:
for role in set(roles): | |
if not role or role in TOP_LEVEL_ROLE_NAMES: | |
raise ValueError( | |
"Delegated roles cannot be empty or use top-level role names" | |
) | |
if any(not role or role in TOP_LEVEL_ROLE_NAMES for role in set(roles)): | |
raise ValueError( | |
"Delegated roles cannot be empty or use top-level role names" | |
) |
and I personally prefer it.
@sechkova what do you think?
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.
Yes, I had a small talk with @jku, and we came out about making it more readable. I'm open about any option 🙂
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 for adding the check, I agree that separate lines are more readable.
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.
set() does not seem necessary?
@sechkova can you approve running the CI workflows as GitHub says that: |
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 don't know if @MVrachev has any more comments, looks ok to me.
Thanks for asking. |
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.
LGTM, but there does seem to be an unnecessary set construction in there
tuf/api/metadata.py
Outdated
for role in set(roles): | ||
if not role or role in TOP_LEVEL_ROLE_NAMES: | ||
raise ValueError( | ||
"Delegated roles cannot be empty or use top-level role names" | ||
) |
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.
set() does not seem necessary?
The set() is not required in the OrderedDict. Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
Thanks! |
This commit adds the validation in the
metadata.Delegations
to prevent that one of the delegate role names given is a top-level
role name.
A
ValueError
will be raised if one of the roles names in thethe list given to as delegated contains the role name as one of the
top-level roles.
The mirrors role is not covered in this PR.
Signed-off-by: Kairo de Araujo kdearaujo@vmware.com
Please fill in the fields below to submit a pull request. The more information
that is provided, the better.
Fixes #1558
Description of the changes being introduced by the pull request:
Please verify and check that the pull request fulfills the following
requirements: