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

Trusted Entitlements: add support for signing POST body #2753

Merged
merged 7 commits into from
Jul 13, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Jul 6, 2023

Changes:

  • HTTPClient has a new X-Post-Params-Hash request header
  • Replaced HTTPRequest.Method.post's Encodable value with a more specific HTTPRequestBody (like HTTPResponseBody)
  • HTTPRequestBody allows overriding contentForSignature (it defaults to []) which then Signing uses for the hash
  • Fixed LogInOperation not passing the VerificationResult to the result CustomerInfo. This is captured in unit and integration tests now.
  • Add support for signing POST requests in Path.logIn and Path.postReceiptData

Depends on https://github.com/RevenueCat/khepri/pull/6246

@NachoSoto NachoSoto requested a review from a team July 6, 2023 00:02
Comment on lines 241 to 266
var asData: Data {
let nonce: Data = self.nonce ?? .init()
let path: Data = self.path.unescapedPath.asData
let postParameterHash: Data = self.requestBody?.postParameterHash.asData ?? .init()
let requestDate: Data = String(self.requestDate).asData
let etag: Data = (self.etag ?? "").asData
let message: Data = self.message ?? .init()

return (
(self.nonce ?? .init()) +
self.path.unescapedPath.asData +
String(self.requestDate).asData +
(self.etag ?? "").asData +
(self.message ?? .init())
nonce +
path +
postParameterHash +
requestDate +
etag +
message
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separating this sped up compilation from 4 seconds to less than 100ms (not sure exactly how long, but that was the threshold I set for the warning).

@NachoSoto NachoSoto force-pushed the nacho/signature-post-params branch 4 times, most recently from e6d030e to d2eb42b Compare July 7, 2023 20:06
Comment on lines +95 to +103
func testLogInWithValidSignature() async throws {
let info = try await Purchases.shared.logIn(UUID().uuidString).customerInfo
expect(info.entitlements.verification) == .verified
}

func testLogInWithInvalidSignature() async throws {
self.invalidSignature = true

let info = try await Purchases.shared.logIn(UUID().uuidString).customerInfo
expect(info.entitlements.verification) == .failed
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These exposed the fact that LogInOperation wasn't forwarding the verification result.

@@ -715,13 +715,17 @@ class BackendPostReceiptDataTests: BaseBackendPostReceiptDataTests {
expect(completionCalled.value).toEventually(equal(2))
}

func testGetsEntitlementsWithVerifiedResponse() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff here is horribly hard to read for no reason.
I simply moved 2 tests into a separate class, which is this:

class BackendPostReceiptWithSignatureVerificationTests: BaseBackendPostReceiptDataTests {

    override var verificationMode: Configuration.EntitlementVerificationMode { .informational }

    func testGetsEntitlementsWithVerifiedResponse() {
        self.httpClient.mock(
            requestPath: .postReceiptData,
            response: .init(statusCode: .success,
                            response: Self.validCustomerResponse,
                            verificationResult: .verified)
        )

        let result = waitUntilValue { completed in
            self.backend.post(receiptData: Self.receiptData,
                              productData: nil,
                              transactionData: .init(
                                 appUserID: Self.userID,
                                 presentedOfferingID: nil,
                                 unsyncedAttributes: nil,
                                 storefront: nil,
                                 source: .init(isRestore: false, initiationSource: .purchase)
                              ),
                              observerMode: false,
                              completion: { result in
                completed(result)
            })
        }

        expect(result).to(beSuccess())

        if #available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.2, *) {
            expect(result?.value?.entitlements.verification) == .verified
        }
    }

    func testGetsEntitlementsWithFailedVerification() {
        self.httpClient.mock(
            requestPath: .postReceiptData,
            response: .init(statusCode: .success,
                            response: Self.validCustomerResponse,
                            verificationResult: .failed)
        )

        let result = waitUntilValue { completed in
            self.backend.post(receiptData: Self.receiptData2,
                              productData: nil,
                              transactionData: .init(
                                 appUserID: Self.userID,
                                 presentedOfferingID: nil,
                                 unsyncedAttributes: nil,
                                 storefront: nil,
                                 source: .init(isRestore: false, initiationSource: .purchase)
                              ),
                              observerMode: false,
                              completion: { result in
                completed(result)
            })
        }

        expect(result).to(beSuccess())

        if #available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.2, *) {
            expect(result?.value?.entitlements.verification) == .failed
        }
    }

}

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the heads up!

expect(headers?.keys).toNot(contain(HTTPClient.RequestHeader.postParameters.rawValue))
}

func testPostRequestWithDisabledSignatureVerificationDoesNotContainPostParametersHeader() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A test WITH the header is in SignatureVerificationHTTPClientTests below.

@NachoSoto NachoSoto changed the title [WIP] Trusted Entitlements: add support for signing POST body Trusted Entitlements: add support for signing POST body Jul 7, 2023
@NachoSoto NachoSoto marked this pull request as ready for review July 7, 2023 20:17
@@ -82,7 +82,10 @@ private extension LogInOperation {
completion: IdentityAPI.LogInResponseHandler) {
let result: Result<(info: CustomerInfo, created: Bool), BackendError> = result
.map { response in
(response.body, created: response.statusCode == .createdSuccess)
(
response.body.copy(with: response.verificationResult),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ this was missing! And we had no unit/integration tests for it. We do now.

@NachoSoto NachoSoto force-pushed the nacho/signature-post-params branch from fc3fdeb to 5f1cde7 Compare July 7, 2023 20:21
Comment on lines +1545 to +1536
let body = try JSONDecoder.default.decode(
AnyEncodableRequestBody.self,
from: requestData
).body
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to refactor these using #2769 because of the change to use HTTPRequestBody

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

approving for now but let's remember to update the signatures of tests with the keys that don't have expiration set

@@ -97,12 +97,21 @@ extension HTTPClient {
return [RequestHeader.nonce.rawValue: data.base64EncodedString()]
}

static func postParametersHeader(for body: HTTPRequestBody) -> RequestHeaders {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: reading this without knowing how we do signature verification, this seems like it'd be confusing and hard to understand why we'd need a specific header for post parameters. maybe something like postParametersHeaderForSigning(with body:? (also changed to "with" since the others use "with" too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -715,13 +715,17 @@ class BackendPostReceiptDataTests: BaseBackendPostReceiptDataTests {
expect(completionCalled.value).toEventually(equal(2))
}

func testGetsEntitlementsWithVerifiedResponse() {
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the heads up!

@NachoSoto NachoSoto force-pushed the nacho/signature-post-params branch from b0941c0 to 208d482 Compare July 7, 2023 22:21
@NachoSoto
Copy link
Contributor Author

approving for now but let's remember to update the signatures of tests with the keys that don't have expiration set

Doing that in a separate PR 👍🏻

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Merging #2753 (99ed6be) into main (1e12136) will decrease coverage by 0.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #2753      +/-   ##
==========================================
- Coverage   86.51%   86.50%   -0.01%     
==========================================
  Files         214      216       +2     
  Lines       15396    15482      +86     
==========================================
+ Hits        13320    13393      +73     
- Misses       2076     2089      +13     
Impacted Files Coverage Δ
...ources/Networking/HTTPClient/HTTPRequestBody.swift 0.00% <0.00%> (ø)
...urces/Networking/HTTPClient/HTTPResponseBody.swift 100.00% <ø> (ø)
...king/Operations/GetIntroEligibilityOperation.swift 100.00% <ø> (ø)
...king/Operations/PostAdServicesTokenOperation.swift 90.90% <ø> (ø)
...king/Operations/PostAttributionDataOperation.swift 88.88% <ø> (ø)
...king/Operations/PostOfferForSigningOperation.swift 95.89% <ø> (ø)
...Operations/PostSubscriberAttributesOperation.swift 90.69% <ø> (ø)
Sources/Networking/HTTPClient/HTTPClient.swift 97.88% <82.35%> (-0.66%) ⬇️
Sources/Security/HTTPRequestBody+Signing.swift 91.89% <91.89%> (ø)
Sources/FoundationExtensions/Data+Extensions.swift 72.22% <100.00%> (+0.79%) ⬆️
... and 5 more

... and 5 files with indirect coverage changes

@NachoSoto NachoSoto enabled auto-merge (squash) July 7, 2023 22:23
@NachoSoto NachoSoto disabled auto-merge July 7, 2023 22:24
@NachoSoto
Copy link
Contributor Author

Verified integration tests are working with the canary 🎉

@NachoSoto
Copy link
Contributor Author

Ship it!

@NachoSoto NachoSoto merged commit f7e36fb into main Jul 13, 2023
2 checks passed
@NachoSoto NachoSoto deleted the nacho/signature-post-params branch July 13, 2023 14:45
NachoSoto added a commit that referenced this pull request Jul 19, 2023
**This is an automatic release.**

### Dependency Updates
* Bump fastlane from 2.213.0 to 2.214.0 (#2824) via dependabot[bot]
(@dependabot[bot])
### Other Changes
* `MainThreadMonitor`: don't crash if there is no test in progress
(#2838) via NachoSoto (@NachoSoto)
* `CI`: fixed Fastlane APITester lanes (#2836) via NachoSoto
(@NachoSoto)
* `Integration Tests`: workaround Swift runtime crash (#2826) via
NachoSoto (@NachoSoto)
* `@EnsureNonEmptyArrayDecodable` (#2831) via NachoSoto (@NachoSoto)
* `iOS 17`: added tests for simulating cancellations (#2597) via
NachoSoto (@NachoSoto)
* `CI`: make all `Codecov` jobs `informational` (#2828) via NachoSoto
(@NachoSoto)
* `MainThreadMonitor`: check deadlocks only ever N seconds (#2820) via
NachoSoto (@NachoSoto)
* New `@NonEmptyStringDecodable` (#2819) via NachoSoto (@NachoSoto)
* `MockDeviceCache`: avoid using real `UserDefaults` (#2814) via
NachoSoto (@NachoSoto)
* `throwAssertion`: fixed Xcode 15 compilation (#2813) via NachoSoto
(@NachoSoto)
* `CustomEntitlementsComputation`: fixed API testers (#2815) via
NachoSoto (@NachoSoto)
* `PackageTypeTests`: fixed iOS 12 (#2807) via NachoSoto (@NachoSoto)
* `Tests`: avoid race-condition in leak detection (#2806) via NachoSoto
(@NachoSoto)
* Revert "`Unit Tests`: removed leak detection" (#2805) via NachoSoto
(@NachoSoto)
* `PackageType: Codable` implementation (#2797) via NachoSoto
(@NachoSoto)
* `SystemInfo.init` no longer `throws` (#2803) via NachoSoto
(@NachoSoto)
* `Trusted Entitlements`: add support for signing `POST` body (#2753)
via NachoSoto (@NachoSoto)
* `Tests`: unified default timeouts (#2801) via NachoSoto (@NachoSoto)
* `Tests`: removed forced-unwrap (#2799) via NachoSoto (@NachoSoto)
* `Tests`: added missing `super.setUp()` (#2804) via NachoSoto
(@NachoSoto)
* Replaced `FatalErrorUtil` with `Nimble` (#2802) via NachoSoto
(@NachoSoto)
* `Tests`: fixed another flaky test (#2795) via NachoSoto (@NachoSoto)
* `TimingUtil`: improved tests by using `Clock` (#2794) via NachoSoto
(@NachoSoto)
* `IgnoreDecodeErrors`: log decoding error (#2778) via NachoSoto
(@NachoSoto)
* `TestLogHandler`: changed all tests to explicitly deinitialize it
(#2784) via NachoSoto (@NachoSoto)
* `LocalReceiptParserStoreKitTests`: fixed flaky test failure (#2785)
via NachoSoto (@NachoSoto)
* `Unit Tests`: removed leak detection (#2792) via NachoSoto
(@NachoSoto)
* `Tests`: fixed another flaky failure with asynchronous check (#2786)
via NachoSoto (@NachoSoto)

---------

Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
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.

3 participants