-
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
Explicit encode role names #1759
Explicit encode role names #1759
Conversation
Pull Request Test Coverage Report for Build 1707644438
💛 - 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.
The actual change is correct IMO, but I have some questions on the tests...
We might need some target downloads tests using delegations but your case does not seem to need target downloads: maybe just add the tests into test_updater_delegation_graphs.py -- I've not confirmed but I would expect you can just add new test cases without having to modify the test code in any way?
Also could maybe add some explanation to the commit message: that 99% of this is already happening in the real use case as requests silently encodes everything that is not valid in a URL: we're just making this explicit. The only "breaking" change here for Requests-using-code is that "/" in a rolename will now be encoded
Speaking of "/" or "\" in a rolename: this is a case I'd like to see a test for... which leads to: we should have some tests for when we really use Requests (and not only simulator). These tests should maybe be in test_updater_ng.py
as that already runs a webserver but I don't think they absolutely need to be added in this PR.
tests/test_updater_fetch_target.py
Outdated
encoded_path="projectA%2Ffile_a.txt", | ||
rolename="A", | ||
delegated_role=DelegatedRole( | ||
"A", [], 1, False, ["projectA/**"], None |
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.
"projectA/**"
: the double asterisk format (as it's used in many places) is not supported in python-tuf so this looks misleading.
02d7415
to
6cb8328
Compare
@jku, I'm still not happy with tests for it. 😔 |
6016a8d
to
9c33cd3
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 looks good. I'm approving this but would not mind a better docstring and/or name for the test if you can improve them: it now tests that rolenames are correctly encoded in both filenames and URLs.
I will improve it. Please wait before merging it. |
This commit explicitly encodes role names. Mostly this encoding is already happening in ``requests`` for what is not a URL. The "/" in a role name will now be encoded. Also, a slight change in the RepositorySimulator will align with the tests. This commit partially covers issue theupdateframework#1634 Signed-off-by: Kairo de Araujo <[email protected]>
9c33cd3
to
aa6d28f
Compare
This commit explicitly encodes role names.
Also, a slight change in the RepositorySimulator will align
with the tests.
This commit partially covers issue #1634
Signed-off-by: Kairo de Araujo [email protected]