Better error messages when missing cryptography package #231
Conversation
|
Whoops, looks like i based this on an older branch. I will update this. |
6 similar comments
| @@ -303,6 +303,12 @@ def test_invalid_crypto_alg(self, jws, payload): | |||
| with pytest.raises(NotImplementedError): | |||
| jws.encode(payload, 'secret', algorithm='HS1024') | |||
|
|
|||
| @pytest.mark.skipif(has_crypto, reason='Tests better errors if crypography not installed') | |||
There was a problem hiding this comment.
I think this would be better if it just said "Scenario requires that cryptography not be installed"
| raise NotImplementedError('Algorithm not supported') | ||
| if not has_crypto and algorithm in get_crypto_algorithms(): | ||
| raise NotImplementedError('"cryptography" package must be ' | ||
| 'installed to use this algorithm') |
There was a problem hiding this comment.
How about something like Algorithm %s could not be found. Do you have cryptography installed?
|
|
||
| return default_algorithms | ||
|
|
||
|
|
||
| def get_crypto_algorithms(): |
There was a problem hiding this comment.
Instead of the get_crypto_algorithms() approach, I think it might be simpler to just use a set to define the cryptography-supplied algorithms.
Something like:
requires_cryptography = set([
'RS256', 'RS384', 'RS512', 'ES256', 'ES384', 'ES521',
'PS256', 'PS384', 'PS512'
])
Then instead of if not has_crypto and algorithm in get_crypto_algorithms(), you could simply check if algorithm in requires_cryptography.
There was a problem hiding this comment.
My thought with having it be a method was so we could define what algorithms required crypto to be installed in a single place, instead of having to remember to update a set as well as the function which actually saves the crypto algorithms as available. If you think this would be better, I would be more then happy to make the change :)
There was a problem hiding this comment.
Yea, I think that it requires a little extra work to update but it is a little simpler in the actual implementation.
| @@ -131,5 +131,6 @@ def main(): | |||
| else: | |||
| p.print_help() | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Looks like you might have added an extra space here?
| @@ -171,6 +199,7 @@ def sign(self, msg, key): | |||
| def verify(self, msg, key, sig): | |||
| return constant_time_compare(sig, self.sign(msg, key)) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
This (and the other extra space) was to fix flake8 errors reported by the tests. It seemed to be failing to pass without the changes (but maybe that was just a local configuration on my box)
There was a problem hiding this comment.
er. Extra newline, not extra space.
There was a problem hiding this comment.
It's possible our flake8 in CI just started caring about these. If so, carry on. As long as flake8 is happy, I'm happy.
|
Thx for the feedback. I'll make any changes tomorrow and get them pushed to you. Cheers. |
1 similar comment
|
One last thing... do you think you could add a line to the CHANGELOG about this? |
|
Sure. Just a sec |
|
Thanks for contributing @vimalloc! 👍 |
|
@vimalloc @mark-adams thanks! 🎉 |
|
@mark-adams there's nothing else pressing, should we release v1.4.3 with this? |
In reference to #230. If trying to use a cryptography algorithm without the cryptography package installed, you will get this error:
instead of: