-
Notifications
You must be signed in to change notification settings - Fork 130
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
Fix Client for sig_version 4 #159
Conversation
# Raises if params are not a dict that can be converted | ||
# to json. | ||
params = self.canon_json(params) | ||
body = params |
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.
This is a bit hacky, but since sig v3 is deprecated, we can remove this in a later PR.
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.
Yeah, I thought we had already removed it here. We did remove it from other code bases. We should get on that ASAP. (I misread splitting out this if statement as adding it back before.)
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've got a diff in progress to do so. All red :)
body = self.canon_json(params) | ||
params = {} | ||
else: | ||
body = '' |
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.
Is the distinction between an empty string for body (here) vs. None (as at line 229) important?
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.
Line 229 is for canonicalization versions where the body must never be concatenated into the canonicalized representation of the request. I like setting body to a non-string value in those cases because future bugs that tried to use the body wrong would raise, making them easier to catch early.
Perhaps body should be a required parameter to canonicalize() -- or default to None -- for that reason?
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 wanted to make canonicalize backwards compatible, so I didn't make it required, but I will default to 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.
Is the distinction between an empty string for body (here) vs. None (as at line 229) important?
For sig v4, the body must be an empty string for GET and a json string for other request methods. For other sig versions, it's ignored.
|
||
self.assertIn('Content-type', response.headers) | ||
self.assertEqual(response.headers['Content-type'], 'application/json') | ||
self.assertIn('Authorization', response.headers) |
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.
Why are you explicitly testing for the existence of an authorization header here? or alternately, why not test for the authn header in the GET tests above? both kinds of requests have the header IIRC?
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.
To be honest, I copied this entire test class from TestJsonRequests
, which is all sig v3, and added some cases from TestRequests
to ensure all these cases work for sig v4. I'll add the header assertions to all the cases.
Fix logic for canonicalizing and signing requests using sig_version 4 by distinguishing between params and body in a backwards-compatible manner.
Add tests.