Getting to 100% coverage for crypt module. #290
Conversation
dhermes
force-pushed
the
100-percent-coverage-crypt
branch
from
472d71d to
8c8074d
Compare
|
I intend to get to this but won't until the weekend. |
|
No worries just wanted to make sure it was still on your radar. |
|
Just the one comment on the second of the seven commits. |
| @@ -95,7 +97,108 @@ def make_signed_jwt(signer, payload): | |||
| return b'.'.join(segments) | |||
|
|
|||
|
|
|||
| def verify_signed_jwt_with_certs(jwt, certs, audience): | |||
| def _verify_signature(message, signature, certs): | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Also defining the fallback pkcs12_key_as_pem as a top-level method so that we may test it.
Moved check into protected function _verify_signature.
Previous variables in verify_signed_jwt_with_certs did not illustrate the actual use of each part.
Moved check into protected function _check_audience.
Moved check into protected function _verify_time_range.
This already exists in test_jwt but these tests just make sure the correct exceptions occur on error and make sure that in the success case all the correct verify/check methods are called. Doesn't actually call these methods, just uses mocks instead.
dhermes
force-pushed
the
100-percent-coverage-crypt
branch
from
8c8074d to
ef4d070
Compare
| Args: | ||
| message: string or bytes, The message to verify. | ||
| signature: string or bytes, The signature on the message. | ||
| cert_list: list, certificates in PEM format. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
I would never request a change to a public function signature in a refactoring pull request. :-) At least not accidentally. Private functions are totally fair game. |
dhermes
force-pushed
the
100-percent-coverage-crypt
branch
from
ef4d070 to
3f9b592
Compare
|
@nathanielmanistaatgoogle pushed the rename. It seems you don't really care about the |
|
It's fine to punt on AppIdentityError to a later time when you address what you "dislike [about] the way exceptions are used in this library". :-) The commit log message of 3f9b592 could use correction. |
|
Correction from list to iterable or something else? |
|
Yes, just "iterable" in the commit log message. |
dhermes
force-pushed
the
100-percent-coverage-crypt
branch
from
3f9b592 to
dcefaad
Compare
|
OK cool. Updated the code and the commit message. I can wait on Travis to merge if it LGTY. |
|
Looks good; merge at will and thanks for the discussion. |
Getting to 100% coverage for crypt module.
I also re-factored the module a bit since the code was opaque and there was too much going on in one method.