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

Key.to_dict() now always returns JSON encodeable keys and values. #139

Closed
wants to merge 1 commit into from

Conversation

zejn
Copy link
Collaborator

@zejn zejn commented Apr 14, 2019

If this is merged, then a major version should be bumped as this change is somewhat backwards incompatible.

Fixes #137 and #127.

@codecov
Copy link

codecov bot commented Apr 14, 2019

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   96.54%   96.54%           
=======================================
  Files          14       14           
  Lines        1071     1071           
=======================================
  Hits         1034     1034           
  Misses         37       37
Impacted Files Coverage Δ
jose/backends/ecdsa_backend.py 98.66% <ø> (ø) ⬆️
jose/backends/cryptography_backend.py 98.11% <ø> (ø) ⬆️
jose/backends/rsa_backend.py 96.5% <ø> (ø) ⬆️
jose/backends/pycrypto_backend.py 95.76% <ø> (ø) ⬆️
jose/jwk.py 95.12% <ø> (ø) ⬆️

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 1c3c1a6...f8e2c92. Read the comment docs.

Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, just needs a few nitpicks fixed.

@@ -43,3 +43,7 @@ def test_to_dict(self):

assert 'k' in as_dict
assert as_dict['k'] == encoded

# as_dict should be serializable to JSON
import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nitpick here: import json at the top of the module.

@@ -370,6 +370,10 @@ def assert_parameters(self, as_dict, private):
assert 'dq' not in as_dict
assert 'qi' not in as_dict

# as_dict should be serializable to JSON
import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nitpick here: import json at the top of the module.

@@ -194,6 +194,10 @@ def assert_parameters(self, as_dict, private):
# Private parameters should be absent
assert 'd' not in as_dict

# as_dict should be serializable to JSON
import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: import json at the top of the module.

@@ -138,5 +138,5 @@ def to_dict(self):
return {
'alg': self._algorithm,
'kty': 'oct',
'k': base64url_encode(self.prepared_key),
'k': base64url_encode(self.prepared_key).decode('ascii'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I know that the encoding names get normalized anyway, but I'd prefer to have one style. ASCII is capitalized everywhere else, so I'd like it capitalized here as well.

@blag blag mentioned this pull request Dec 17, 2019
@blag blag closed this in #165 Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jwk construct with hs256 returns 'k' as binary
2 participants