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

Add granularity for DID Token Errors #50

Conversation

justinnout
Copy link
Contributor

@justinnout justinnout commented Jan 5, 2023

📦 Pull Request

The DIDTokenError is too vague so we split this up into DIDTokenExpired, DIDTokenMalformed, and DIDTokenInvalid.

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.)

🗜 Versioning

(Check one!)

  • Patch: Bug Fix?
  • Minor: New Feature?
  • Major: Breaking Change?

✅ Fixed Issues

  • [List any fixed issues here like: Fixes #XXXX]

🚨 Test instructions

Tests pass with make test.

⚠️ Update CHANGELOG.md

  • I have updated the Upcoming Changes section of CHANGELOG.md with context related to this Pull Request.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #67980: Python Admin SDK - split DIDTokenError into 2 error types.


Returns:
None.
"""
proof, claim = cls.decode(did_token)

if claim['ext'] is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding this to the validate method!

@ayv8er ayv8er linked an issue Jan 5, 2023 that may be closed by this pull request
Comment on lines 145 to 147
message='Given DID token cannot be used at this time. Please '
'check the "ext" field and regenerate a new token with a suitable '
'value.',

Choose a reason for hiding this comment

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

I think the first line could go away. The "at this time" makes me think of a temporal issue vs malformed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - removed.

Copy link

@dgerrellsMagic dgerrellsMagic left a comment

Choose a reason for hiding this comment

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

LGTM

@justinnout justinnout merged commit f26323f into master Jan 6, 2023
@justinnout justinnout deleted the justinherrera-sc-67980-python-admin-sdk-split-didtokenerror-into branch January 6, 2023 00:04
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.

Untreated exception when checking expiration
3 participants