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

Remove python-jose and ecdsa dependencies #7285

Closed
wants to merge 1 commit into from

Conversation

estyrke
Copy link

@estyrke estyrke commented Jan 31, 2024

Fixes #7244

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (28811ef) 95.88% compared to head (3b33638) 95.88%.

Files Patch % Lines
moto/ec2/utils.py 94.28% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7285   +/-   ##
=======================================
  Coverage   95.88%   95.88%           
=======================================
  Files         843      843           
  Lines       82569    82590   +21     
=======================================
+ Hits        79170    79191   +21     
  Misses       3399     3399           
Flag Coverage Δ
servertests 32.51% <10.52%> (-0.01%) ⬇️
unittests 95.85% <94.73%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

Hi @estyrke, thanks for the PR! Please see my comments

key_material = key_material.encode("ascii")
if isinstance(key_material, str):
key_material = key_material.encode("ascii")
key_material = base64.b64decode(key_material)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we keep this part (or even the entire calculation) inside a try-except? This used to catch a UnicodeDecodeError, and I think we should keep that


import moto.cognitoidp.models
from moto import mock_aws, settings
from moto.cognitoidp.utils import create_id
from moto.core import DEFAULT_ACCOUNT_ID as ACCOUNT_ID

# Taken from jwks-public.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's import the actual file, so we don't have to worry that these values ever go out of sync:

from moto import cognitoidp

    from moto.utilities.utils import load_resource

    private_key = load_resource(cognitoidp.__name__, "resources/jwks-private.json")
    jwk.RSAKey.import_key(private_key)

@@ -29,6 +29,7 @@

RSA_PUBLIC_KEY_RFC4716 = b"""\
---- BEGIN SSH2 PUBLIC KEY ----
cOmmENt: moto@github.com
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a few more tests with example RFC4716-compliant keys here, to verify that they all work?
I'm thinking with comment, without any comment, with commend + x-comment, etc.

I noticed that the spec has a few examples:
https://www.rfc-editor.org/rfc/rfc4716#section-3.6

@bblommers
Copy link
Collaborator

Hi @estyrke! Hope you don't mind - I took the commit with your changes and merged it part of #7356. This change will be part of the upcoming 5.0.2 release, probably later today.

Thanks again for adding this to Moto!

@bblommers bblommers closed this Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unmaintained dependency python-jose
2 participants