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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NT-1774 ] Managing denied requests #1374

Merged
merged 8 commits into from
Feb 22, 2021
Merged

Conversation

singhhari
Copy link
Contributor

馃摬 What

Re-factoring our implementation of the Perimeter X SDK with some extensions to the initial functionality. We're now enforcing a CAPTCHA screen on endpoints that are deemed suspicious.

馃 Why

We're bolstering our security against bot attacks with Perimeter Xs SDK. This should add another layer of protection and mitigate future attacks.

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #1374 (1845646) into master (9f35eba) will decrease coverage by 13.02%.
The diff coverage is 54.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1374       +/-   ##
===========================================
- Coverage   85.87%   72.85%   -13.03%     
===========================================
  Files        1100      597      -503     
  Lines       95970    40243    -55727     
===========================================
- Hits        82418    29318    -53100     
+ Misses      13552    10925     -2627     
Impacted Files Coverage 螖
Kickstarter-iOS/Library/WebViewController.swift 12.50% <0.00%> (-2.66%) 猬囷笍
KsApi/Service+RequestHelpers.swift 0.00% <0.00%> (酶)
KsApi/extensions/NSURLSession.swift 0.00% <0.00%> (酶)
Library/TestHelpers/MockPerimeterXClient.swift 25.00% <25.00%> (酶)
KsApi/MockService.swift 17.20% <33.33%> (+0.08%) 猬嗭笍
KsApi/Service.swift 12.82% <75.00%> (+0.75%) 猬嗭笍
...kstarter-iOS/ViewModels/AppDelegateViewModel.swift 96.38% <100.00%> (+<0.01%) 猬嗭笍
...ter-iOS/ViewModels/AppDelegateViewModelTests.swift 99.65% <100.00%> (+<0.01%) 猬嗭笍
KsApi/ServiceType.swift 82.05% <100.00%> (酶)
KsApi/ServiceTypeTests.swift 100.00% <100.00%> (酶)
... and 508 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 9f35eba...1845646. Read the comment docs.

func headers() -> [String: String]
}

public protocol PerimeterXErrorHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about creating another file for this and removing the Perimeter X prefix? I don't think this protocol is specific to the perimeter x, but open to go either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Moved this into a separate file and removed the prefix in the latest commit.

Verifies a URLs response and handles any errors with the Perimeter X SDK.

- parameter blockResponse: An `HTTPURLResponse` object with response data from a request.
- parameter data: `Data` associated with the request.
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra comment space

.expires: NSDate(timeIntervalSinceNow: 3_600)
])

public static func startPerimeterX(with pxManagerDelegate: PXManagerDelegate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document the functions that are no in the PerimeterXClientType?

}

public func headers() -> [String: String] {
return (PXManager.sharedInstance().httpHeaders() as! [String: String])
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to avoid !. What do you think about PXManager.sharedInstance().httpHeaders() ?? [:]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch. This came right off of the documentation and I didn't notice the force casting.

@@ -9,6 +9,8 @@ internal class WebViewController: UIViewController {
internal let webView = WKWebView(frame: .zero, configuration: WKWebViewConfiguration())
internal var bottomAnchorConstraint: NSLayoutConstraint?

var webKitCookieStore: WKHTTPCookieStore?
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable can be private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment above the variable to explain what it's doing as well.

if let newCookie = PerimeterXClient.cookie {
self.webKitCookieStore?.setCookie(newCookie, completionHandler: {
print("Perimeter X mobile VID cookie set.")
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Too much indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Called the formatter to fix this.

@jgsamudio
Copy link
Contributor

There seems to be a larger reduction in test coverage. What do you think about adding tests to the comments by codecov?

@singhhari singhhari merged commit a7b8d48 into master Feb 22, 2021
@singhhari singhhari deleted the NT-1774-managing-denied-requests branch February 22, 2021 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants