-
Notifications
You must be signed in to change notification settings - Fork 114
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
Dry up signing roundtrip tests #198
Conversation
Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ Much more readable! Nice improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, a few tiny improvement suggestions if you're motivated.
Tests/JWSRSATests.swift
Outdated
let algKidHeader = "{\"alg\":\"\(algorithm.rawValue)\",\"kid\":\"kid\"}" | ||
let kidAlgHeader = "{\"kid\":\"kid\",\"alg\":\"\(algorithm.rawValue)\"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could make these string literals a little cleaner to read by using extended string delimiters. Your call whether it's worth the trade off for noisier string interpolation.
Something like
let algKidHeader = #"{"alg":"\#(algorithm.rawValue)","kid":"kid"}"#
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 👍 But since we have lots and lots of such strings throughout the project, I tracked #202.
Tests/JWSRSATests.swift
Outdated
@@ -149,7 +128,19 @@ class JWSRSATests: RSACryptoTestCase { | |||
let verifier = Verifier(verifyingAlgorithm: algorithm, publicKey: publicKeyAlice2048!) | |||
|
|||
XCTAssertTrue(secondJWS.isValid(for: verifier!)) | |||
XCTAssertEqual(String(data: jws.header.data(), encoding: .utf8), "{\"alg\":\"\(algorithm.rawValue)\"}") | |||
if withKid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement wants to be a guard. If you move the check from line 144 up above the branch then an early return wouldn't matter, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated ✅
c549b43
@nathan-mohemian Please re-review! (I also ran |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
SonarCloud Quality Gate failed. 0 Bugs |
* Rename asymmetric algorithm * Rename symmetric algorithm * Refactor algorithm enums * Sketch out new encryption flow * Refactor encryption flow * Fix reference to old rsa encrypter * Remove dummy keywrap mode * Fix and extendcompression tests * Add key management mode tests * Adapt rsa tests * Add alg check test * Disable space linter rule for now * Adap AES tests * Add license headers to new files * Make sonarqube happy * Do full Sonarqube analysis in pull requests (#201) * Do sonarqube analysis after test * Log fastlane contents * Remove deletion of coverage report * Update sonar config to point to sonarcloud * Trigger analysis * Exclude tests from duplication calculation * Remove loggign * Dry up signing roundtrip tests (#198) * Dry up tests * Use guard in test helper * Format code * Redo mode creation * REname key encryption mode implementations * Final cleanup * Commit something * Revert "Commit something" This reverts commit 91266d4.
* Restructure JWE encryption flow (#203) * Rename asymmetric algorithm * Rename symmetric algorithm * Refactor algorithm enums * Sketch out new encryption flow * Refactor encryption flow * Fix reference to old rsa encrypter * Remove dummy keywrap mode * Fix and extendcompression tests * Add key management mode tests * Adapt rsa tests * Add alg check test * Disable space linter rule for now * Adap AES tests * Add license headers to new files * Make sonarqube happy * Do full Sonarqube analysis in pull requests (#201) * Do sonarqube analysis after test * Log fastlane contents * Remove deletion of coverage report * Update sonar config to point to sonarcloud * Trigger analysis * Exclude tests from duplication calculation * Remove loggign * Dry up signing roundtrip tests (#198) * Dry up tests * Use guard in test helper * Format code * Redo mode creation * REname key encryption mode implementations * Final cleanup * Commit something * Revert "Commit something" This reverts commit 91266d4. * Restructure JWE decryption flow (#208) * Replicate encryption flow for decryption * Fix tests * Split rsa mode in two types * Replace deprecated decrypter calls in tests * Remove todos * Add documentation * Update alg documentation * Fix typos * Add key types to doc * Change access modifiers back
Closes #189. We had two helper functions that did basically the same. Dried those up.