[MBL-736] Upgrade PerimeterX #1839

msadoon · 2023-07-26T21:32:34Z

📲 What

Dependency is 2 major versions back (1.16.3 -> 3.0.4) so probably should be upgraded.

🤔 Why

We were having login issues related to PerimeterX hopefully this upgrade solves those. We no longer manually launch the PX captcha sheet like in 1.16.3 (see goToPerimeterXCaptcha in AppDelegate below). 3.0.4 should take care of it automatically.

This seemed like the issue causing our login button to not work.

🛠 How

Used the simpler PerimeterX upgrade documentation here and the work done on this older, closed pr.

Was able to reliably to show the captcha sheet on any REST call with these headers: (Solution Architect at PX provided this to us)

request.allHTTPHeaderFields = request.allHTTPHeaderFields?.withAllValuesFrom(["x-px-authorization": "4", "x-ks-block": "1"])

However, this isn't necessary to do on xauth or any other REST call, because PerimeterX has two modes monitor mode and enforce mode.

We can enforce PerimeterX to fully validate each request from the Cloudflare worker.

Suggestion to @kickstarter/infrastructure was to add xauth/access_token to px_enforced_routes to ensure that route shows captcha (at least on mobile).

Either way - this boils down to an framework upgrade plain and simple. No extra customization required, let the Captcha sheet show if the SDK decides to.

There's some good cleanup happening as well, with the new SDK instructions which make this implementation much simpler:
Removed:
PerimeterXBlockResponseType
PerimeterXManagerType
PXManagerDelegate
KsAPINotification

PerimeterXClientTests was also removed, reason being we use PerimeterXClient class to wrap the actual SDK, there's no protocol in between to mock. We don't want to be testing the actual SDK.

✅ Acceptance criteria

PerimeterX is updated and correctly configured to send the headers on each REST request.

⏰ TODO

Go through solution with HUMAN Architect and try to identify if there is a way to reliably show the captcha sheet on login.

msadoon · 2023-07-26T21:36:13Z

KsApi/GraphAPI.swift

@@ -578,182 +578,6 @@ public enum GraphAPI {
    }


This file gets re-gened every time KsAPI is rebuilt, so can't avoid it being a change.

we might consider putting this in our .gitignore to avoid confusion. We intentionally update this file as needed, when we support a new mutation for example. is there a reason we need to update this file on every build?

Hey, yea it can get a little hairy if the intention is to leave out updating the graph schema.
The web folks are pretty good at honouring the API contract, which they have checks for.

The app will rarely fail a build because the schema doesn't match our side.

The two scenarios are:

Breaking change - we have to update our app anyway.

Non breaking change - we can assume the backward compatibility checks web folks have done are enough.

Problem with putting it in .gitignore is we have to take it out every time we look for a GQL change.

Also we kind of want to know if the schema changes immediately. We can choose not to commit it - but then we'd be out of sync with the latest API's.

My thinking is - keep it the way it is - investigate the schema change, if it happens. If it's breaking especially. Otherwise trust the web folks to support our mobile side.

codecov · 2023-07-26T22:15:35Z

Codecov Report

Merging #1839 (c7235d9) into main (0e82d13) will decrease coverage by 0.02%.
The diff coverage is 9.52%.

@@            Coverage Diff             @@
##             main    #1839      +/-   ##
==========================================
- Coverage   84.54%   84.52%   -0.02%     
==========================================
  Files        1274     1271       -3     
  Lines      115279   115144     -135     
  Branches    30698    30657      -41     
==========================================
- Hits        97458    97323     -135     
- Misses      16752    16755       +3     
+ Partials     1069     1066       -3

Files Changed	Coverage Δ
Kickstarter-iOS/AppDelegateViewModel.swift	`93.00% <ø> (+0.05%)`	⬆️
Kickstarter-iOS/AppDelegateViewModelTests.swift	`98.72% <ø> (-0.01%)`	⬇️
KsApi/Service.swift	`8.92% <0.00%> (-0.47%)`	⬇️
KsApi/ServiceTypeTests.swift	`98.57% <ø> (-0.04%)`	⬇️
KsApi/extensions/NSURLSession.swift	`0.00% <0.00%> (ø)`
KsApi/px/PerimeterXClient.swift	`13.79% <5.88%> (-70.83%)`	⬇️
KsApi/ServiceType.swift	`75.65% <100.00%> (-0.42%)`	⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

KsApi/ServiceType.swift

msadoon · 2023-08-01T16:01:25Z

Kickstarter-iOS/AppDelegate.swift

@@ -219,6 +219,12 @@ internal final class AppDelegate: UIResponder, UIApplicationDelegate {
          .messageBannerViewController?.showBanner(with: success ? .success : .error, message: message)
      }

+    self.viewModel.outputs.configurePerimeterX
+      .observeValues {
+        AppEnvironment.current.apiService.perimeterXClient


This is now where we call start on PerimeterX, not in Service. Thinking is more along the lines of their official instructions (Step 5). Although the SDK instructions explicitly say to "You should only call this function once" and appDidFinishLaunchingWithOptions is called on every relaunch.
Some ambiguity there.

The instructions also say "It's essential to call this function as early as possible in your application and before any URL request to your server" so this should be fine. I feel like they meant to say "only call this function once per app launch" 🤷🏻‍♂️

Ah ok makes sense

ifosli

I'm confused about what's going on here, so I don't want to approve and have my approval mean something. But in general, looks good! I left one comment on a section that feels incomplete to me, but feel free to ignore if I'm wrong.

KsApi/px/PerimeterXClient.swift

…int statements for PerimeterX captcha state. Also removed the fatalError.

scottkicks

Followed the implementation steps in the docs and this all makes sense to me. App builds and tests pass. Did some general QA as well and everything seems fine 👍

ifosli

I took another pass at this and I think the handleError being renamed to handleResponse makes some of the comments unclear. I also don't know if the boolean means the same thing now as it did previously, and since I'm not sure how easy that's to test/catch, I wanted to bring it up so you could look into it, if you haven't already.

ifosli · 2023-08-03T16:59:57Z

KsApi/ErrorHandler.swift

   Given a URL Response and data, this method will allow for custom error handling logic.

-   - parameter response: An `HTTPURLResponse` object with response data from a request.
-   - parameter data: Data` associated with the request
+   - parameter data: `Data` associated with the request
+   - parameter response: An `URLResponse` object with response data from a request.

   - returns: A boolean indicating whether or not the error was handled.


What does handleResponse actually do now? And what does the boolean mean? Since this is mostly done for us by PerimeterX, I think it'd be useful to have a more up-to-date comment here. Specifically, the returns refers to "the error" when nothing in the function or parameter names indicates what that error is.

Well this is meant to be more of a wrapper around any error handling (ie. ErrorHandler) for a URLRequest. We use PerimeterX now but in the future if we changed to another captcha sheet provider or any URLRequest with error handling - this is still a useful method of dependency injection.
Comment is left generic on purpose because it might not always be PerimeterX we handle errors for.

ifosli · 2023-08-03T17:00:48Z

KsApi/extensions/NSURLSession.swift

@@ -35,7 +35,8 @@ internal extension URLSession {
        guard let response = response as? HTTPURLResponse else { fatalError() }

        /// `error` is `nil` or `handleError` returns `false`.


Another outdated comment referring to handleError - please update. Also, in this comment, can you expand on why we want to process the response when the SDK couldn't handle the response? I would've expected that to be a failure state and the true to be a continue state, but I'll admit I don't think the documentation is very clear here. It's also very possible that true indicates that the error is fully handled and we don't need to do anything else. Is this something you've been able to test (where the error is non-nil)?

Ok I'm going to delete the inline comment because I don' think we need it.
In fact - we generally don't write comments in line unless its' /// TODO: or /// FIXME:
This was just something left in from the previous implementation that doesn't provide much clarity.

KsApi/px/PerimeterXClient.swift

updated perimeterX integration for version 3

4da2ca9

msadoon added the WIP label Jul 26, 2023

msadoon added this to the release-5.10.0 milestone Jul 26, 2023

msadoon self-assigned this Jul 26, 2023

msadoon mentioned this pull request Jul 26, 2023

[MBL-736] Upgrade PerimeterX #1822

Closed

2 tasks

msadoon commented Jul 26, 2023

View reviewed changes

msadoon added 2 commits August 1, 2023 10:53

Added final configuration piece to start PerimeterX

eb7af07

formatting

e284ff2

msadoon commented Aug 1, 2023

View reviewed changes

KsApi/ServiceType.swift Show resolved Hide resolved

msadoon commented Aug 1, 2023

View reviewed changes

msadoon added needs review and removed WIP labels Aug 1, 2023

msadoon marked this pull request as ready for review August 1, 2023 16:09

msadoon requested review from ifosli and scottkicks August 1, 2023 16:10

msadoon added 2 commits August 2, 2023 12:08

Merge branch 'main' into mbl-736/perimeterx-upgrade

56c8f7e

Merge branch 'main' into mbl-736/perimeterx-upgrade

ceee9b5

ifosli reviewed Aug 2, 2023

View reviewed changes

KsApi/px/PerimeterXClient.swift Outdated Show resolved Hide resolved

Opted to update the print statement to be inline with the previous pr…

35d153a

…int statements for PerimeterX captcha state. Also removed the fatalError.

scottkicks approved these changes Aug 3, 2023

View reviewed changes

ifosli reviewed Aug 3, 2023

View reviewed changes

removed inline comment

c7235d9

msadoon merged commit 9a1dc93 into main Aug 8, 2023
2 checks passed

msadoon deleted the mbl-736/perimeterx-upgrade branch August 8, 2023 17:38

msadoon mentioned this pull request Aug 14, 2023

[MBL-900] Use of AI Tab #1842

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MBL-736] Upgrade PerimeterX #1839

[MBL-736] Upgrade PerimeterX #1839

msadoon commented Jul 26, 2023 •

edited

Loading

msadoon Jul 26, 2023

scottkicks Aug 3, 2023

msadoon Aug 8, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

msadoon Aug 1, 2023 •

edited

Loading

scottkicks Aug 3, 2023

msadoon Aug 8, 2023

ifosli left a comment

scottkicks left a comment

ifosli left a comment

ifosli Aug 3, 2023

msadoon Aug 8, 2023

ifosli Aug 3, 2023

msadoon Aug 8, 2023

msadoon Aug 8, 2023 •

edited

Loading

		@@ -35,7 +35,8 @@ internal extension URLSession {
		guard let response = response as? HTTPURLResponse else { fatalError() }

		/// `error` is `nil` or `handleError` returns `false`.

[MBL-736] Upgrade PerimeterX #1839

[MBL-736] Upgrade PerimeterX #1839

Conversation

msadoon commented Jul 26, 2023 • edited Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

⏰ TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msadoon Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2023 • edited Loading

Codecov Report

msadoon Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifosli left a comment

Choose a reason for hiding this comment

scottkicks left a comment

Choose a reason for hiding this comment

ifosli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msadoon Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

msadoon commented Jul 26, 2023 •

edited

Loading

msadoon Aug 8, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

msadoon Aug 1, 2023 •

edited

Loading

msadoon Aug 8, 2023 •

edited

Loading