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

feat: verifyJWT against nbf if present but use iat if not #45

Merged
merged 5 commits into from
Aug 14, 2019

Conversation

mi-xu
Copy link
Contributor

@mi-xu mi-xu commented Aug 13, 2019

This PR adds support for checking the current time against the nbf field when verifying a JWT. It prioritizes the presence of nbf in order to comply with W3C VC standards, but falls back to iat when it is absent in order to maintain support for legacy format JWTs.

This completes #167881436 on pivotal

Test scenarios have been added to validate the behavior for all combinations of nbf and iat fields being part of the JWT or not.

@mi-xu mi-xu requested a review from mirceanis August 13, 2019 20:57
@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

Merging #45 into develop will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #45      +/-   ##
===========================================
+ Coverage    96.65%   96.71%   +0.06%     
===========================================
  Files            7        7              
  Lines          209      213       +4     
  Branches        51       52       +1     
===========================================
+ Hits           202      206       +4     
  Misses           7        7
Impacted Files Coverage Δ
src/JWT.ts 96.62% <100%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e131bf...5f2214a. Read the comment docs.

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 good.
There's a small correction to be made to the exp comparison, and a couple tests that could be more specific.
Please take a look at the comments.

src/JWT.ts Outdated Show resolved Hide resolved
src/__tests__/JWT-test.ts Outdated Show resolved Hide resolved
src/__tests__/JWT-test.ts Outdated 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.

Excellent work. thanks!

@mi-xu mi-xu merged commit 232ca14 into develop Aug 14, 2019
@mi-xu mi-xu deleted the feature/167881436/verify-nbf branch August 14, 2019 16:54
mi-xu added a commit that referenced this pull request Aug 15, 2019
* feat: verifyJWT against nbf if present but use iat if not

* fix: apply skew in the opposite direction when checking exp

* test: make some test inputs more clear

* test: use hard coded JWTs to test validFrom teimstamp

* style: remove unused constants
mi-xu added a commit that referenced this pull request Aug 26, 2019
* change iat to nbf

* feat: Set `iat` timestamp by default instead of `nbf` (#43)

* refactor: change createJWT to set iat by default instead of nbf

* test: add tests for iat behavior in createJWT

* feat: do not set iat if value in payload is null

* fix: set payload.iat to undefined instead of null to omit from JWT

* fix code style for division

* fix code style

BREAKING CHANGE: expiresIn option passed to createJWT now calculates exp from nbf instead of iat.

* chore: add release automation configuration (#44)

* feat: verifyJWT against nbf if present but use iat if not (#45)

* feat: verifyJWT against nbf if present but use iat if not

* fix: apply skew in the opposite direction when checking exp

* test: make some test inputs more clear

* test: use hard coded JWTs to test validFrom teimstamp

* style: remove unused constants

* feat: upgrade to local did resolver (#48)

* feat: upgrade to local did resolver

* refactor: export resolver so it can be mocked in tests

* wip: refactor tests to remove dependency on uport and nacl did resolvers

* test: fix tests to verify pregenerated JWT

* test: fix hard coded jwts for validFrom timestamp tests

* test: fix  remaining

* test: add tests for normalizeDID

* refactor: upgrade ethr-did-resolver to fix imports

* refactor: rename variables and clean up async syntax for tests
localredhead pushed a commit that referenced this pull request Aug 28, 2019
* change iat to nbf

* feat: Set `iat` timestamp by default instead of `nbf` (#43)

* refactor: change createJWT to set iat by default instead of nbf

* test: add tests for iat behavior in createJWT

* feat: do not set iat if value in payload is null

* fix: set payload.iat to undefined instead of null to omit from JWT

* fix code style for division

* fix code style

BREAKING CHANGE: expiresIn option passed to createJWT now calculates exp from nbf instead of iat.

* chore: add release automation configuration (#44)

* feat: verifyJWT against nbf if present but use iat if not (#45)

* feat: verifyJWT against nbf if present but use iat if not

* fix: apply skew in the opposite direction when checking exp

* test: make some test inputs more clear

* test: use hard coded JWTs to test validFrom teimstamp

* style: remove unused constants

* feat: upgrade to local did resolver (#48)

* feat: upgrade to local did resolver

* refactor: export resolver so it can be mocked in tests

* wip: refactor tests to remove dependency on uport and nacl did resolvers

* test: fix tests to verify pregenerated JWT

* test: fix hard coded jwts for validFrom timestamp tests

* test: fix  remaining

* test: add tests for normalizeDID

* refactor: upgrade ethr-did-resolver to fix imports

* refactor: rename variables and clean up async syntax for tests
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.

2 participants