Fix type annotation for `decode()`/`encode()`'s parameter key #605

michael-k · 2021-01-12T13:18:22Z

See #602 for details.

The issue suggests that the parameter key should not accept str. I've not included this for backwards compatibility and because I'm not sure if that's really necessary.

Closes #602

jpadilla · 2021-01-17T00:07:46Z

@jdufresne since you worked in some of the typing stuff recently, thoughts on this change?

jdufresne

Are there existing tests that cover type bytes? If not, they should be added.

jwt/api_jws.py

CHANGELOG.rst

jwt/algorithms.py

WolfgangFellger · 2021-01-19T11:22:29Z

Please note that there are even more types that some of the algorithms accept for the key argument.

michael-k · 2021-01-20T14:25:41Z

Please note that there are even more types that some of the algorithms accept for the key argument.

I have no idea how to properly use them in the type annotations. Everything I've tried so far resulted some error by mypy.

This should work but would probably result in Union[Any, …] for every setup that doesn't use cryptography:

if MYPY:
    from cryptography.hazmat.primitives.asymmetric.ec import EllipticCurvePrivateKey

def prepare(key: Union[str, bytes, "EllipticCurvePrivateKey"]):
    pass

WolfgangFellger · 2021-01-20T14:55:31Z

Personally I would be okay with just reverting to Any.

terencehonles · 2021-02-02T23:05:09Z

Please note that there are even more types that some of the algorithms accept for the key argument.

I have no idea how to properly use them in the type annotations. Everything I've tried so far resulted some error by mypy.

This should work but would probably result in Union[Any, …] for every setup that doesn't use cryptography:
if MYPY:
    from cryptography.hazmat.primitives.asymmetric.ec import EllipticCurvePrivateKey

def prepare(key: Union[str, bytes, "EllipticCurvePrivateKey"]):
    pass

I'm just checking to see if this relates to a change I was going to make with decode not accepting bytes, but the quoted Python should be expressed as the following:

# so KEY_TYPES is not needed at runtime and doesn't break because it's in the TYPE_CHECKING block.
# The constant could be define outside of the block and this future import would not be needed
from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    try:
        from cryptography.hazmat.primitives.asymmetric.ec import EllipticCurvePrivateKey
        KEY_TYPES = Union[str, bytes, EllipticCurvePrivateKey]
    except ImportError:
        KEY_TYPES = Union[str, bytes]

def prepare(key: KEY_TYPES): ...

If you need this to be dynamic you could do the following:

_KEY_TYPES = [str, bytes]

# import 1
_KEY_TYPES.append(...)
# import 2
_KEY_TYPES.append(...)

KEY_TYPES = Union[tuple(_KEY_TYPES)]

This works because obj[one, two] is just passing (one, two) to obj.__getitem__ and as long as the type is a tuple not a list it will be the expected union.

terencehonles · 2021-02-02T23:20:23Z

It looks like this change is adjacent, but not the change I was looking to make. jwt should also be bytes | str for the decode functions and there may be other places it's been made too specific and only accepts str.

michael-k · 2021-02-03T09:35:12Z

Your first example leads to Cannot assign multiple types to name "KeyType" without an explicit "Type[...]" annotation.

KEY_TYPES = Union[tuple(_KEY_TYPES)] → Invalid type alias: expression is not a valid type.

Also I don't think that something like _KEY_TYPES.append(…) could work at all as mypy doesn't import and/or run the code afaik. The same should be true for try-except.

terencehonles · 2021-02-04T18:19:49Z

Hmm good point. I hadn't completely thought about that and I was going off what I did (a while ago) without testing mypy. At this point I've changed the original type declaration and am playing with getting mypy running so I mainly came here for #605 (comment) but if you're not able to import or type against a concrete type I'm assuming you'd need to type with a protocol, but that only comes in Python 3.8.

~~One thing to call out that without cryptography being declared as typed or on typeshed (I haven't checked if it is) the import will fail in mypy and it will fall back to Any.~~

It looks like it's on typeshed but it might make sense to declare a common base for private keys https://github.com/python/typeshed/blob/master/stubs/cryptography/cryptography/hazmat/primitives/asymmetric/ec.pyi so that way protocols could be used before Python 3.8. It might be possible to type with Protocols keeping everything behind if typing.TYPE_CHECKING and using from __future__ import annotations to make sure that any use of Protocols is by mypy on Python 3.8 or greater.

Anyways, it does sound like it should just be typed as Any.

terencehonles · 2021-02-17T01:01:38Z

It looks like Protocol would be supported pre Python 3.8 via the typing_extensions, but not sure if declaring the protocol would be worth it vs just typing with Any

michael-k · 2021-02-22T09:27:02Z

@jdufresne any suggestions on how to proceed?

terencehonles · 2021-02-24T23:33:02Z

I believe the consensus here would be to use Any. I know you're looking for input from @jdufresne / @jpadilla 's input, but looking at the call chain a call to j*.decode it will call j*.decode_complete which will likely use the key for:

pyjwt/jwt/api_jws.py

Lines 217 to 224 in 587997e

    
           def _verify_signature( 
        
               self, 
        
               signing_input, 
        
               header, 
        
               signature, 
        
               key="", 
        
               algorithms=None, 
        
           ):

which is untyped (Any)

Looking at the API closer it looks like algorithms are puggable here pyjwt.jwt.algorithms which means the API cannot be type safe. A algorithm will alg.prepare_key(self, key: T) -> U and alg.verify(self, msg: ?, key: U, sig: ?) -> bool and there's not a good way to type both of those.

dajiaji · 2021-03-26T23:58:41Z

@michael-k @jpadilla I think this PR should be changed to use Any, and moved forward.

At present, even the simple sample codes in the specification gives mypy warnings.

Closes jpadilla#602

michael-k · 2021-03-29T08:06:47Z

I've changed the type annotation to Any. @jdufresne could you review this PR again?

dajiaji · 2021-03-30T13:22:04Z

jwt/api_jws.py

@@ -76,7 +76,7 @@ def get_algorithms(self):
    def encode(
        self,
        payload: bytes,
-        key: str,
+        key: Any,


Why don't you set a default value for encode as well as decode ?

Suggested change

key: Any,

key: Any = b"",

This PR doesn't set any default value, it just changes the one for decode.

The suggested change would lead to “insecure by default”

The omission of the key does not result in signing with the empty string as the private key, and from the perspective of consistency of API design, my suggestion has some validity. But, anyway, I understood. You don't need to fix it.

dajiaji · 2021-03-30T13:22:49Z

jwt/api_jwt.py

@@ -37,7 +37,7 @@ def _get_default_options() -> Dict[str, Union[bool, List[str]]]:
    def encode(
        self,
        payload: Dict[str, Any],
-        key: str,
+        key: Any,


Suggested change

key: Any,

key: Any = b"",

dajiaji · 2021-03-31T15:27:06Z

@michael-k One last thing: I think you should update https://github.com/jpadilla/pyjwt/blob/master/docs/api.rst.

auvipy

also, please rebase

ps-george · 2022-06-16T16:28:09Z

bump - this is still an issue

michael-k · 2022-06-20T14:33:33Z

I've rebased, but I don't seem to be able to re-open this PR.

lcy0321 · 2022-12-04T16:40:23Z

Hi @michael-k, @auvipy,
I think this is still an issue, will this PR be re-opened?

jdufresne reviewed Jan 17, 2021

View reviewed changes

jwt/api_jws.py Outdated Show resolved Hide resolved

CHANGELOG.rst Outdated Show resolved Hide resolved

michael-k force-pushed the key-type branch 3 times, most recently from 9604b18 to d604ee7 Compare January 18, 2021 16:39

jdufresne reviewed Jan 18, 2021

View reviewed changes

jwt/algorithms.py Outdated Show resolved Hide resolved

WolfgangFellger mentioned this pull request Feb 24, 2021

encode()/decode() key parameter type should not be str #602

Closed

michael-k added 2 commits March 29, 2021 09:55

Fix type annotation for decode()/encode()'s parameter key

cb8732b

Closes jpadilla#602

Let decode/encode accept any key

53117b2

michael-k force-pushed the key-type branch from d604ee7 to 53117b2 Compare March 29, 2021 07:59

dajiaji reviewed Mar 30, 2021

View reviewed changes

auvipy requested changes Apr 8, 2022

View reviewed changes

github-actions bot added the stale Issues without activity for more than 60 days label Jun 8, 2022

github-actions bot closed this Jun 15, 2022

Viicos mentioned this pull request Dec 27, 2022

Add more types #843

Merged

Mark-Hetherington mentioned this pull request Jan 10, 2023

mypy. Key can be cryptography.hazmat.backends.openssl.rsa._RSAPublicKey not only string #848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type annotation for `decode()`/`encode()`'s parameter key #605

Fix type annotation for `decode()`/`encode()`'s parameter key #605

michael-k commented Jan 12, 2021 •

edited

jpadilla commented Jan 17, 2021

jdufresne left a comment

WolfgangFellger commented Jan 19, 2021

michael-k commented Jan 20, 2021 •

edited

WolfgangFellger commented Jan 20, 2021

terencehonles commented Feb 2, 2021 •

edited

terencehonles commented Feb 2, 2021

michael-k commented Feb 3, 2021

terencehonles commented Feb 4, 2021 •

edited

terencehonles commented Feb 17, 2021

michael-k commented Feb 22, 2021

terencehonles commented Feb 24, 2021

dajiaji commented Mar 26, 2021

michael-k commented Mar 29, 2021

dajiaji Mar 30, 2021

michael-k Mar 31, 2021 •

edited

dajiaji Mar 31, 2021

dajiaji Mar 30, 2021

dajiaji commented Mar 31, 2021

auvipy left a comment

ps-george commented Jun 16, 2022

michael-k commented Jun 20, 2022

lcy0321 commented Dec 4, 2022

Fix type annotation for decode()/encode()'s parameter key #605

Fix type annotation for decode()/encode()'s parameter key #605

Conversation

michael-k commented Jan 12, 2021 • edited

jpadilla commented Jan 17, 2021

jdufresne left a comment

Choose a reason for hiding this comment

WolfgangFellger commented Jan 19, 2021

michael-k commented Jan 20, 2021 • edited

WolfgangFellger commented Jan 20, 2021

terencehonles commented Feb 2, 2021 • edited

terencehonles commented Feb 2, 2021

michael-k commented Feb 3, 2021

terencehonles commented Feb 4, 2021 • edited

terencehonles commented Feb 17, 2021

michael-k commented Feb 22, 2021

terencehonles commented Feb 24, 2021

dajiaji commented Mar 26, 2021

michael-k commented Mar 29, 2021

dajiaji Mar 30, 2021

Choose a reason for hiding this comment

michael-k Mar 31, 2021 • edited

Choose a reason for hiding this comment

dajiaji Mar 31, 2021

Choose a reason for hiding this comment

dajiaji Mar 30, 2021

Choose a reason for hiding this comment

dajiaji commented Mar 31, 2021

auvipy left a comment

Choose a reason for hiding this comment

ps-george commented Jun 16, 2022

michael-k commented Jun 20, 2022

lcy0321 commented Dec 4, 2022

Fix type annotation for `decode()`/`encode()`'s parameter key #605

Fix type annotation for `decode()`/`encode()`'s parameter key #605

michael-k commented Jan 12, 2021 •

edited

michael-k commented Jan 20, 2021 •

edited

terencehonles commented Feb 2, 2021 •

edited

terencehonles commented Feb 4, 2021 •

edited

michael-k Mar 31, 2021 •

edited