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

Set iat timestamp by default instead of nbf #43

Merged
merged 6 commits into from
Aug 13, 2019

Conversation

mi-xu
Copy link
Contributor

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

This PR reverts the changes to createJWT introduced in #41 and slightly alters the behavior of how some of the arguments are used:

  • createJWT no longer attempts sets the nbf attribute to the current timestamp by default
  • createJWT once again sets the iat attribute to the current timestamp by default
  • createJWT omits the iat attribute if it is passed in as undefined in the payload
  • createJWT only calculates the exp value given the expiresIn option if nbf is present in the payload

nbf no longer defaults to the current timestamp is because it's an optional attribute for JWTs, and therefore does not justify causing a breaking change in this library. did-jwt-vc will provide higher level functions for creating Verifiable Credentials and Presentations which are responsible for defaulting the value of nbf since it is a necessary attribute for verification in those contexts.

Another slight difference in behavior here is that the expiresIn option will be ignored if the payload does not specify nbf. The previous behavior would only function appropriately in the scenario that nbf was not passed in as part of the payload since it would always calculate exp from the automatically generated nbf even if a value was assigned explicitly.

@mi-xu mi-xu requested a review from mirceanis August 12, 2019 18:02
@codecov
Copy link

codecov bot commented Aug 12, 2019

Codecov Report

Merging #43 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #43   +/-   ##
========================================
  Coverage    96.65%   96.65%           
========================================
  Files            7        7           
  Lines          209      209           
  Branches        51       50    -1     
========================================
  Hits           202      202           
  Misses           7        7
Impacted Files Coverage Δ
src/JWT.ts 96.47% <100%> (ø) ⬆️

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 edf9c14...44bf4f7. 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 great.

There is a breaking change that should be reflected in a version bump. The expiresIn parameter used to be applied to iat and even when that was missing(undefined) it was used to compute the exp field based on the system clock.

@mi-xu mi-xu force-pushed the feature/167819821/iat-create branch from 3a336c1 to 44bf4f7 Compare August 13, 2019 18:50
@mi-xu mi-xu merged commit 7e131bf into develop Aug 13, 2019
@mi-xu mi-xu deleted the feature/167819821/iat-create branch August 13, 2019 19:43
mi-xu added a commit that referenced this pull request Aug 15, 2019
* 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.
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
localredhead pushed a commit that referenced this pull request Sep 6, 2019
* 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.
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