Untreated exception when checking expiration

[ccrvlh]

Question

I was not able to replicate this, but I've seen an exception a couple of times related to the comparison to the expiration.

  File "myapp/magic_link.py", line 34, in generate_token
    metadata = magic.validate_magic_token(did_token)
  File "myapp/magic.py", line 38, in validate_magic_token
    magic.Token.validate(token)
  File "myapp/.venv/lib/python3.10/site-packages/magic_admin/resources/token.py", line 154, in validate
    if current_time_in_s > claim['ext']:
TypeError: '>' not supported between instances of 'int' and 'NoneType'

From what I understand, this token is being decoded wiith the decode() method, returning a tuple (proof and claim) in the validate() method.

If the claim["ext"] is required, would it make sense to raise a DIDTokenError when the claim is empty? since the field is required, but is None after decoding.

If the claim is not required and could allow for empty/None values, then it could probably make sense to change the access method to a get('ext', 0) in a way that the comparison is still valid and would always return DIDTokenError with an "expired".

In parallel, would it make sense to separate types of DIDTokenError? For example DIDTokenExpired vs DIDTokenMalformed?

Environment

Software	Version(s)
magic-admin-python	0.0.5
python	3.10.6
Operating System	macOS / ubuntu22

Would be happy to contribute to this.

[ayv8er]

Hello @lowercase00,

After validating the did token via validate method, you can pass it to the decode method.
The decode method will resolve to a tuple containing proof and claim.

Could I ask what you mean by "If the claim["ext"] is required?"
Does the returned claim contain None for the ext key?

[ccrvlh]

Yes, so looking at the exception, the claim["ext"] is None, which doesn't allow for the comparison as it is now.
What I mean by required is: should claim["ext"] never be None? If that's the case, then the token is invalid and this code wouldn't have been reached. if the claim["ext"] can be None for any reason, that it could make sense to change the approach to compare it with current_time_in_s.

[ayv8er]

If the did token (from a client-side Magic SDK login method) is passed to your backend, then validated, and passed to the decode, the "ext" should be the expiration of that token. It would be odd to have it None.
If that token was invalid, I am also surprised it was validated.

Being that this is not reproducible on your end, I will bring up your suggestion to throw an error if that claim is empty.

[ayv8er]

@lowercase00 Can you verify how the did token was generated? Was it via one of our Magic client-side SDK login methods?

[ccrvlh]

Client side SDK (not sure whether it was Android or iOS). Seems our team had experienced it before in the past, this is the description:

It seems to be when you request a magic link code, then go back and request a new one either without inputting in the first or potentially just too soon after completing the first. Basically, it seems to happen the second time using magic link for the same user rather than the first (in any given "session")

The result is that the backend received an empty (None) ext and the lib doesn't handle this scenario.

[ayv8er]

@lowercase00, thank you for the suggestion on splitting up DIDTokenError, it will be split up into 3.

DIDTokenMalformed: raised when DID token cannot be decoded or if required fields are missing
DIDTokenExpired: raised when current time > DID Token expiration time
DIDTokenInvalid: raised when DID token can be decoded but does not pass validation (missing values, signature mismatch, etc.)

Also, the validate method will throw an error when Claim["ext"] is None