-
Notifications
You must be signed in to change notification settings - Fork 14
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
Feature/sd jwt issuer verify iat exp #321
Feature/sd jwt issuer verify iat exp #321
Conversation
pyeudiw/jwt/jws_helper.py
Outdated
return signed | ||
except Exception as e: | ||
raise JWSSigningError("Signing error: error in step", e) | ||
return signer.sign_json(keys=[key_from_jwk_dict(signing_key)], headers=[(protected, unprotected)], flatten=True) | ||
return signer.sign_json( | ||
keys=[key_from_jwk_dict(signing_key)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't repeat this twice key_from_jwk_dict(signing_key)
please assign the result to a variable and reuse it
pyeudiw/jwt/jws_helper.py
Outdated
|
||
:param payload: The original payload. | ||
:returns: The payload with added SD-JWT claims. | ||
def validate_jwt_claims(self, payload: dict) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be renamed to something specific for the timestamps used in iat, exp and nbf
validate_jwt_timestamps_claims
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
pyeudiw/sd_jwt/verifier.py
Outdated
else: | ||
raise ValueError( | ||
f"Unsupported serialization format: {self._serialization_format}" | ||
) | ||
|
||
self._holder_public_key_payload = self._sd_jwt_payload.get("cnf", None) | ||
|
||
def validate_claims(self, payload: dict) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, note that the method does not need to be class member: self
is never used and this could be a pure function instead (ivoking the pure function twice also trivially satisfy the review note above).
IMHO I would name the method validate_lifetime_claims(claims: dict)
, but valida_timestamp_claims
is also 100% ok.
pyeudiw/sd_jwt/verifier.py
Outdated
|
||
if 'iat' in payload: | ||
if payload['iat'] > current_time: | ||
raise ValueError("Future issue time, token is invalid.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it matters but this is not a ValueError. This is a validation error. In Python a ValueError is usually used when a passed argument has a valid type, but the method in undefined on that specific value (for example a method could be defined on int
but might only be able to process positive integers).
https://docs.python.org/3/library/exceptions.html#ValueError
A custom exception, such as LifetimeException
or something like that, might be more suitable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LifetimeException that inherits ValidationError
Co-authored-by: Giuseppe De Marco <[email protected]>
…yCodesItBetter/eudi-wallet-it-python into feature/sd-jwt_issuer-verify_iat-exp
Resolve #316