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

Improve Claim API #25

Merged
merged 6 commits into from
Feb 16, 2017
Merged

Improve Claim API #25

merged 6 commits into from
Feb 16, 2017

Conversation

siemensikkema
Copy link
Member

Time based claims (Expiration- and NotBefore-) are now able to evaluate the comparison at time of verification and API more closely matches that of signature verification.

This will allow us to pass in claims into our jwt-provider's LoginMiddleware at init and verify claims as well as signatures. A PR that adds that functionality is next on my list.

- rename JWTError.verificationFailed to .signatureVerificationFailed
- make verifyClaims throw on failure
- use throwable tests where applicable
- use XCTAssertThrowsError to ensure we fail when no errors are thrown
  where they are expected
- update Claim tests to reflect their new throwing nature
- rename SecondsWithLeewayClaim to TimeBasedClaim
- hide protocols that should be internal
- use more explicit initializer labels
@siemensikkema siemensikkema self-assigned this Feb 15, 2017
@siemensikkema siemensikkema added the enhancement New feature or request label Feb 15, 2017
@siemensikkema siemensikkema added this to the 0.9 milestone Feb 15, 2017
@codecov-io
Copy link

codecov-io commented Feb 15, 2017

Codecov Report

Merging #25 into master will increase coverage by 0.55%.
The diff coverage is 79.11%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   81.24%   81.79%   +0.55%     
==========================================
  Files          31       30       -1     
  Lines         693      714      +21     
==========================================
+ Hits          563      584      +21     
  Misses        130      130
Impacted Files Coverage Δ
Sources/JWT/SignatureVerifiable.swift 75% <ø> (ø)
Sources/JWT/Signers/HMACSigner.swift 60% <ø> (ø)
Sources/JWT/Signers/RSASigner.swift 89.33% <ø> (ø)
Sources/JWT/Signers/ECDSASigner.swift 81.81% <ø> (ø)
Sources/JWT/Claims/EqualityClaim.swift 60% <ø> (ø)
Sources/JWT/JWTError.swift 0% <ø> (ø)
Sources/JWT/Claims/NotBeforeClaim.swift 100% <100%> (ø)
Sources/JWT/Claims/Claim.swift 100% <100%> (ø)
Sources/JWT/Claims/ExpirationTimeClaim.swift 100% <100%> (ø)
Sources/JWT/ClaimsVerifiable.swift 100% <100%> (ø)
... and 12 more

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 239c4df...88d5f00. Read the comment docs.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

Code looks good :)


public init(_ value: Seconds, leeway: Seconds = 0) {
self.value = value
public init(createTimestamp: @escaping () -> Seconds, leeway: Seconds) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the default value of 0 for leeway should stay. And the createTimestamp would be nicer for the caller with @autoclosure. (I won't copy this to nbf but obviously I mean for both places.)

Copy link
Member Author

Choose a reason for hiding this comment

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

good call! This functionality used to come from the protocol but got lost when I decided to hide that.

- add "convenience" initializers for TimeBasedClaims
- update AudienceClaim interface to match other Claims
- use default value for leeway
- allow initializing IssuedAtClaim using Date (with default value)
@siemensikkema
Copy link
Member Author

@vzsg through the TimeBasedClaim protocol there are now several ways to initialize ExpirationTimeClaim/NotBeforeClaim:

screen shot 2017-02-15 at 16 11 14

I discovered the playgrounds were outdated (can Travis check for that? 🤔) and that there were some inconsistencies with initializing IssuedAtClaim and AudienceClaim so I fixed that.

@tanner0101
Copy link
Member

Looks like @vzsg comments were resolved. Going to merge this :)

@tanner0101 tanner0101 merged commit 582acf8 into master Feb 16, 2017
@siemensikkema siemensikkema deleted the improve-claim-api branch June 15, 2017 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants