`HTTPClient`: added support for sending `X-Nonce` by NachoSoto · Pull Request #2214 · RevenueCat/purchases-ios

NachoSoto · 2023-01-13T23:58:22Z

tonidero

It's looking good!

codecov · 2023-02-02T19:23:30Z

Codecov Report

Merging #2214 (bc52a3b) into main (cf3090a) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head bc52a3b differs from pull request most recent head 5ab771e. Consider uploading reports for the commit 5ab771e to get more accurate results

@@            Coverage Diff             @@
##             main    #2214      +/-   ##
==========================================
- Coverage   85.96%   85.89%   -0.08%     
==========================================
  Files         184      183       -1     
  Lines       12121    12152      +31     
==========================================
+ Hits        10420    10438      +18     
- Misses       1701     1714      +13

Impacted Files	Coverage Δ
Sources/Networking/HTTPClient/HTTPClient.swift	`94.21% <100.00%> (-3.99%)`	⬇️
Sources/Networking/HTTPClient/HTTPRequest.swift	`100.00% <100.00%> (ø)`
...s/Logging/Strings/ManageSubscriptionsStrings.swift	`75.00% <0.00%> (-6.25%)`	⬇️
Sources/Support/ManageSubscriptionsHelper.swift	`68.04% <0.00%> (-2.07%)`	⬇️
Sources/Logging/Strings/NetworkStrings.swift	`98.36% <0.00%> (-1.64%)`	⬇️
...ources/Purchasing/IntroEligibilityCalculator.swift	`100.00% <0.00%> (ø)`
...chasing/StoreKitAbstractions/SK2StoreProduct.swift	`96.72% <0.00%> (ø)`
Sources/Misc/Box.swift
...hasing/StoreKit2/StoreKit2StorefrontListener.swift	`100.00% <0.00%> (+13.63%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

NachoSoto · 2023-02-06T19:31:02Z

Okay finished this. The new implementation uses the term "nonce" everywhere, and I added a test that ensures that the format matches the python tests, using the same string used there.

tonidero · 2023-02-07T09:22:17Z

Interesting, I didn't know you could unpack a UUID like this. But it looks good 👍

NachoSoto · 2023-02-07T15:51:32Z

@tonidero I've updated this implementation with a proper nonce generation algorithm now that the backend will accept 12 bytes (see ecfa437b7f557a57fdf4377b3af27dbf124908cf)

Love the name 🤣

I want all features to be named like that

NachoSoto · 2023-02-07T15:52:21Z

@tonidero @aboedo thoughts on this? assert only compiles on DEBUG builds so it can help us catch issues in tests. Sending anything other than 12 bytes would fail so might as well catch it early.

Hmm when we deploy the library, we deploy the release version I think right? so this shouldn't affect developers. Also, seems like we've already done that in a few places in the codebase, so I'm ok with it 👍

The pre-built frameworks are release, yeah. But also for people integrating with CocoaPods / SPM, their production builds are release by default.

tonidero · 2023-02-07T18:00:21Z

Love the name 🤣

tonidero · 2023-02-07T18:12:16Z

Hmm when we deploy the library, we deploy the release version I think right? so this shouldn't affect developers. Also, seems like we've already done that in a few places in the codebase, so I'm ok with it 👍

Fixes [CSDK-631].

aboedo · 2023-02-08T18:44:04Z

+        if self.path.authenticated {
+            result += authHeaders
+        }
+
+        if let nonce = self.nonce {
+            result += HTTPClient.nonceHeader(with: nonce)
+        }


is there any situation where the nonce should be nil but the path.authenticated = true?
If not, we should try to consolidate the conditions and probably log if they're not simultaneously met

The /health endpoint is one example.

aboedo · 2023-02-08T18:45:22Z

+    /// Creates an `HTTPRequest` with a `nonce`.
+    @available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.2, *)
+    static func createIntegrityEnforcedRequestRequest(method: Method, path: Path) -> Self {
+        return .init(method: method, path: path, nonce: Data.randomNonce())
+    }


technically we're verifying integrity and authenticity and the same time, right? maybe we could have the name reflect that?
also, typo here: RequestRequest

technically we're verifying integrity and authenticity and the same time, right? maybe we could have the name reflect that?

How about HTTPRequest.createWithResponseVerification?

also, typo here: RequestRequest
Thanks 🤦🏻 I noticed that in the next PR, I'll fix it.

@vegaro

**This is an automatic release.** ### Dependency Updates * Bump fastlane-plugin-revenuecat_internal from `738f255` to `9255366` (#2264) via dependabot[bot] (@dependabot[bot]) * Update `Gemfile.lock` (#2254) via Cesar de la Vega (@vegaro) ### Other Changes * `HTTPClient`: added support for sending `X-Nonce` (#2214) via NachoSoto (@NachoSoto) * `Configuration`: added (`internal` for now) API to load public key (#2215) via NachoSoto (@NachoSoto) * Replaced `Any` uses for workaround with `Box` (#2250) via NachoSoto (@NachoSoto) * `HTTPClientTests`: fixed failing test with missing assertions (#2262) via NachoSoto (@NachoSoto) * `HTTPClientTests`: refactored tests to use `waitUntil` (#2257) via NachoSoto (@NachoSoto) * PurchaseTester: Add Receipt Inspector UI (#2249) via Andy Boedo (@aboedo) * Adds dependabot (#2259) via Cesar de la Vega (@vegaro) * `StoreKit1WrapperTests`: avoid using `Bool.random` to fix flaky code coverage (#2258) via NachoSoto (@NachoSoto) * `IntroEligibilityCalculator`: changed logic to handle products with no subscription group (#2247) via NachoSoto (@NachoSoto)

…2267) ⚠️ 🎉 This also changes integration tests to use `EntitlementVerificationLevel.enforced` so that integration tests fail if signatures are invalid. #### Depends on: - https://github.com/RevenueCat/khepri/pull/5191 - https://github.com/RevenueCat/khepri/pull/5204 - #2214 - #2215 - #2216 - #2272 _Marking this as `feat`ure because it contains a new error in `PurchasesDiagnostics.Error`_

NachoSoto added the refactor label Jan 13, 2023

NachoSoto requested a review from a team January 13, 2023 23:58

NachoSoto force-pushed the http-client-request-uuid branch from 912ba5d to faa2587 Compare January 14, 2023 00:02

aboedo reviewed Jan 17, 2023

View reviewed changes

Comment thread Sources/Networking/HTTPClient/HTTPClient.swift Outdated

Comment thread Sources/Networking/HTTPClient/HTTPClient.swift

NachoSoto force-pushed the http-client-request-uuid branch from fbc2779 to d668f83 Compare January 17, 2023 22:02

NachoSoto added the blocked label Jan 25, 2023

tonidero reviewed Jan 30, 2023

View reviewed changes

Comment thread Sources/Networking/HTTPClient/HTTPClient.swift Outdated

tonidero reviewed Jan 30, 2023

View reviewed changes

Comment thread Sources/Networking/HTTPClient/HTTPRequest.swift Outdated

NachoSoto force-pushed the http-client-request-uuid branch from d668f83 to bc52a3b Compare February 2, 2023 19:13

NachoSoto force-pushed the http-client-request-uuid branch from bc52a3b to 1f5a4aa Compare February 6, 2023 19:05

NachoSoto requested review from aboedo and tonidero and removed request for aboedo February 6, 2023 19:29

NachoSoto removed the blocked label Feb 6, 2023

NachoSoto force-pushed the http-client-request-uuid branch from d17bc8d to e5e2819 Compare February 6, 2023 19:32

NachoSoto changed the title ~~HTTPClient: added support for sending request UUIDs~~ HTTPClient: added support for sending X-Nonce Feb 6, 2023

NachoSoto force-pushed the http-client-request-uuid branch 2 times, most recently from 8a4075a to c95b581 Compare February 6, 2023 23:22

tonidero approved these changes Feb 7, 2023

View reviewed changes

NachoSoto requested a review from tonidero February 7, 2023 15:50

NachoSoto commented Feb 7, 2023

View reviewed changes

tonidero approved these changes Feb 7, 2023

View reviewed changes

NachoSoto mentioned this pull request Feb 7, 2023

PurchasesDiagnostics: added step to verify signature verification #2267

Merged

NachoSoto force-pushed the http-client-request-uuid branch from dfe3bd9 to 113cadd Compare February 7, 2023 21:10

NachoSoto mentioned this pull request Feb 7, 2023

[WIP] HTTPClient: added signature validation #2268

Merged

NachoSoto added 2 commits February 8, 2023 08:54

HTTPClient: added support for sending request UUIDs

004934e

Fixes [CSDK-631].

Added missing import to fix SPM

901d7f0

NachoSoto added 7 commits February 8, 2023 08:54

Updated signature header name

29be145

Replaced nonce with random UUID represented as Data

58eb815

Use SecRandomCopyBytes instead

08d10fa

Fixed tests

434cd7b

Replaced nonce generation with ChaChaPoly.Nonce

89c8e92

Added assertion message

4fd3d14

Renamed method

5ab771e

NachoSoto force-pushed the http-client-request-uuid branch from 113cadd to 5ab771e Compare February 8, 2023 16:54

NachoSoto enabled auto-merge (squash) February 8, 2023 16:55

NachoSoto merged commit 88a7841 into main Feb 8, 2023

RCGitBot mentioned this pull request Feb 8, 2023

[AUTOMATIC] Release/4.17.5 #2270

Merged

aboedo reviewed Feb 8, 2023

View reviewed changes

NachoSoto deleted the http-client-request-uuid branch February 8, 2023 18:50

NachoSoto mentioned this pull request Feb 8, 2023

HTTPClient: added signature validation and introduced ErrorCode.signatureVerificationFailed #2272

Merged

Conversation

NachoSoto commented Jan 13, 2023 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NachoSoto commented Feb 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NachoSoto commented Jan 13, 2023 •

edited by atlassian Bot

Loading

codecov Bot commented Feb 2, 2023 •

edited

Loading