Skip to content
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

Quote/Escape all URL placeholders #532

Merged
merged 9 commits into from Oct 11, 2019
Merged

Quote/Escape all URL placeholders #532

merged 9 commits into from Oct 11, 2019

Conversation

@llamasoft
Copy link
Contributor

llamasoft commented Oct 10, 2019

This MR fixes issue #528.

A new utility function, utils.format_url, has been added which can be used like a string's .format method but it applies urllib.parse.quote to all string-like placeholders. All api_path values that used .format have been updated to use the new function instead. For Python 2/3 compatibility it uses six library which is now included in the requirements file. All requirements files have been updated as tox fails when the requirements contain library version differences.

It should be noted that urllib.parse.quote doesn't escape forward slashes. I was extremely happy to find out that Vault handles all forward slashes correctly, even in cases where it might be confusing to a human. This means that no special handling is required for any placeholders regardless of the number of URL segments they should represent.

I also fixed a few minor formatting inconsistencies in api/secrets_engines/pki.pyand removed an unused placeholder indelete_root`.

Lastly, there is a potentially breaking change in api/auth_methods/aws.py function place_role_tags_in_blacklist where the misspelled argument "mount_pont" has been corrected.

@llamasoft llamasoft requested a review from hvac/hvac-maintainers as a code owner Oct 10, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 10, 2019

Codecov Report

Merging #532 into develop will increase coverage by <.01%.
The diff coverage is 75.16%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #532      +/-   ##
===========================================
+ Coverage    82.84%   82.85%   +<.01%     
===========================================
  Files           54       54              
  Lines         2949     2968      +19     
===========================================
+ Hits          2443     2459      +16     
- Misses         506      509       +3
Impacted Files Coverage Δ
hvac/api/secrets_engines/kv_v2.py 100% <100%> (ø) ⬆️
hvac/api/auth_methods/ldap.py 100% <100%> (ø) ⬆️
hvac/api/system_backend/audit.py 95.23% <100%> (+0.23%) ⬆️
hvac/utils.py 77.66% <100%> (+2.13%) ⬆️
hvac/api/auth_methods/okta.py 100% <100%> (ø) ⬆️
hvac/api/secrets_engines/pki.py 100% <100%> (ø) ⬆️
hvac/api/system_backend/lease.py 86.2% <100%> (+0.49%) ⬆️
hvac/api/system_backend/auth.py 87.17% <100%> (ø) ⬆️
hvac/api/secrets_engines/transit.py 93.23% <100%> (ø) ⬆️
hvac/api/secrets_engines/aws.py 98.07% <100%> (ø) ⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7485853...547b6a2. Read the comment docs.

Copy link
Collaborator

jeffwecan left a comment

This looks like a great addition. Also good callout on the breaking change! We can make sure to call that out in the changelog notes on the off chance it would break someone's existing usage. 👍

requirements.in Show resolved Hide resolved
hvac/utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

jeffwecan left a comment

💯

@jeffwecan jeffwecan merged commit 500ce54 into hvac:develop Oct 11, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.