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

Introduced Policies to override Timestamp checking #241

Merged

Conversation

daniyal-khalil
Copy link
Contributor

Pull Request to Introduce Policies for VerifyJWT

Description

This merge requests introduces a new interface in the JWT.ts module which JWTVerifyPolicies. The policies at the moment are only restricted to timestamps, but can be further expanded upon to serve better. The JWTVerifyPolicies are made a part of VerifyJWTOptions defined as policies. These policies are used to be able to override a certain checks that are made in the VerifyJWT command. It is not a breaking change but an extension to the current implementation.

Policies

The checks for these policies will not me made only and only if they are explicitly stated to be

  • nbf (Not Before)
  • iat (Issued At)
  • exp (Expires At)

Tests

Further tests were added to the JWT.test.ts to check whether the implemented policies were properly allowing the presentation to verify in case of a failure and the policy was made to be overridden. The following cases are covered:

  • nbf is in the future and policy for nbf is false
  • nbf is in the future and iat is in the past with nbf policy false
  • expired JWT with exp policy false
  • expired JWT without skew time but exp policy false
  • iat is in the future with iat policy false

@daniyal-khalil daniyal-khalil marked this pull request as ready for review August 10, 2022 13:48
@daniyal-khalil daniyal-khalil marked this pull request as draft August 10, 2022 13:57
@daniyal-khalil daniyal-khalil marked this pull request as ready for review August 10, 2022 13:58
Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

This looks good and is a good first pass at #239.
Please take a moment to comment on that too if you have opinions about it. IMO, using a now parameter for the policy to represent the timestamp at which the JWT should be valid is also a good way to override some timestamp checks, with a more precise intention, and these 2 methods don't exclude each other.. one can specify a now and also disable a nbf check in the same policy.

Regarding this PR, there are some linter errors which should be easy to fix. Please run yarn lint --fix and commit the changes.

@daniyal-khalil
Copy link
Contributor Author

@mirceanis
Thank you for such a fast response, I have added the now policy which looks at all the timestamp checks, if it is set to false, the verification will not look at the timestamps at all. Similarly, as mentioned above, now can be true, while nbf can still be false. Let me know if I misunderstood anything regarding this, Thank you so much!

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Kudos for the thorough testing work!

Please read my comment about the now param. Yeah, there was a bit of a misunderstanding.

src/JWT.ts Show resolved Hide resolved
Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for contributing this!

@mirceanis mirceanis merged commit 2934f4c into decentralized-identity:master Aug 12, 2022
uport-automation-bot pushed a commit that referenced this pull request Aug 12, 2022
# [6.4.0](6.3.0...6.4.0) (2022-08-12)

### Features

* add JWT verification policies to override timestamp checking ([#241](#241)) ([2934f4c](2934f4c))
@uport-automation-bot
Copy link
Collaborator

🎉 This PR is included in version 6.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants