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

BREAKING CHANGE: Make JWT Codable and generic on claims #10

Merged
merged 16 commits into from
Nov 19, 2018
Merged

Conversation

Andrew-Lees11
Copy link
Contributor

@Andrew-Lees11 Andrew-Lees11 commented Oct 26, 2018

This pull request makes JWT Codable instead of using [String: Any] dictionaries.
Since this is a breaking change it also includes major refactoring of the project.
The following changes have been made:

  • Remove dependency on BlueCryptor, instead use BlueRSA for both mac and linux.
  • Refactor package.swift to match the layout of our other repos
  • Refactor Base64URL as a data extension so it matches base64 API and make the functions internal.
  • Split Algorithm into sign and verify algorithms with corresponding private and public key requirements
  • Created JWTSigner and JWTVerifier structs as extensible implementations of the sign and verify algorithms.
  • Added the none algorithm for the case of always accepting/no signing of JWTs
  • Made ValidateClaimsResult an extensible struct that only checks time based standard claims
  • Added JWTError struct and made sign and encode functions only throw instead of throwing or returning optional
  • Removed Hash.swift since hashing is not part of the JWT package
  • Made Header a codable struct with all the possible fields of a JWT header
  • Made claims a Codable protocol to represent the claims on a JWT
  • Made JWT a codable struct containing the headers and generic on claims
  • removed encode() and replaced it with signing the JWT with none
  • Made RSAKeyType an internal class
  • Updates Jazzy docs with descriptions and examples of public API
  • Updated tests to reflect new API
  • Update to the Standard README with the new usage examples

Copy link
Contributor

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

I think we should consider replacing the use of optionals here with throws, so that failures to encode or decode a JWT can be debugged / logged.

return base64URLEncodedString
}
return nil
extension Data {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this source file to reflect the new contents?

/// - Parameter jwt: A String with the encoded and signed JWT.
/// - Parameter verifier: The `JWTVerifier` used to verify the JWT.
/// - Returns: An instance of `JWT` if the decoding succeeds.
public init?(jwtString: String, verifier: JWTVerifier = .none ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why optional initializer versus a failable initializer? There's no way to communicate back the reason for initialization failing (decode error etc)

///
/// - Note: Sets header.alg with the name of the signing algorithm.
/// - Note: This function will set header.alg field to the name of the signing algorithm.
///
/// - Parameter using algorithm: The algorithm to sign with.
/// - Returns: A String with the encoded and signed JWT.
/// - Throws: An error thrown during the encoding or signing.
Copy link
Contributor

Choose a reason for hiding this comment

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

This no longer throws

@@ -98,73 +98,18 @@ public struct JWT {
/// - Parameter using algorithm: The algorithm to verify with.
/// - Returns: A Bool indicating whether the verification was successful.
/// - Throws: An error thrown during the verification.
Copy link
Contributor

Choose a reason for hiding this comment

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

This no longer throws

/// - Parameter verifier: The `JWTVerifier` used to verify the JWT.
/// - Returns: An instance of `JWT` if the decoding succeeds.
/// - Throws: `JWTError.invalidJWTString` if the jwtString is not base64urlEncoded sections seperated by either 2 or 3 full stops.
/// - Throws: `JWTError.iailedVerification` if the verifier fails to verify the jwtString.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@@ -20,7 +20,8 @@ import LoggerAPI

import Foundation

class BlueRSA: EncryptionAlgorithm {
class BlueRSA: SignerAlgorithm, VerifierAlgorithm {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does BlueRSA now provide enough Linux support that we could use this code on Linux and get rid of the Linux-specific RSA type?

@@ -0,0 +1,37 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call this file Data+Base64URLEncoded.swift?

Sources/SwiftJWT/JWTError.swift Show resolved Hide resolved
let unsignedJWT = header + "." + claims
guard let unsignedData = unsignedJWT.data(using: .utf8) else {
// replace with custom error
throw NSError(domain: "sign", code: 500, userInfo: [:])
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a JWTError

Package.swift Outdated
@@ -18,16 +18,6 @@

import PackageDescription

var listDependencies: [Package.Dependency] = [
.package(url: "https://github.com/IBM-Swift/HeliumLogger.git", from: "1.7.1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do still want to log errors, but this import should be LoggerAPI instead.

Copy link
Contributor

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

🎉

@djones6 djones6 merged commit f246142 into master Nov 19, 2018
@Andrew-Lees11 Andrew-Lees11 deleted the codableJWT branch November 30, 2018 11: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.

2 participants