From b98e781e0102d46a3bfb4c3bffaadc7eaf93ea3e Mon Sep 17 00:00:00 2001 From: James Frost Date: Tue, 25 Jan 2022 12:16:51 +0000 Subject: [PATCH 1/6] Add tests for PublicizeAuthorizationURLComponents --- ...haringAuthorizationWebViewController.swift | 69 ++++++++++--------- WordPress/WordPress.xcodeproj/project.pbxproj | 4 ++ ...icizeAuthorizationURLComponentsTests.swift | 60 ++++++++++++++++ 3 files changed, 99 insertions(+), 34 deletions(-) create mode 100644 WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift diff --git a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift index 8a86963d52be..1fe38d1c465d 100644 --- a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift +++ b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift @@ -1,4 +1,30 @@ import WebKit +import CoreMedia + +enum PublicizeAuthorizationURLComponents: String { + case verifyActionParameter = "action=verify" + case denyActionParameter = "action=deny" + case requestActionParameter = "action=request" + + case declinePath = "/decline" + case authorizationPrefix = "https://public-api.wordpress.com/connect/" + case accessDenied = "error=access_denied" + + case state = "state" + case code = "code" + case error = "error" + + // Special handling for the inconsistent way that services respond to a user's choice to decline + // oauth authorization. + // Right now we have no clear way to know if Tumblr fails. This is something we should try + // fixing moving forward. + // Path does not set the action param or call the callback. It forwards to its own URL ending in /decline. + case userRefused = "oauth_problem=user_refused" + + func containedIn(_ url: URL) -> Bool { + return url.absoluteString.contains(rawValue) + } +} @objc protocol SharingAuthorizationDelegate: NSObjectProtocol { @@ -26,31 +52,6 @@ class SharingAuthorizationWebViewController: WPWebViewController { private static let loginURL = "https://wordpress.com/wp-login.php" - private enum AuthorizeURLComponents: String { - case verifyActionParameter = "action=verify" - case denyActionParameter = "action=deny" - case requestActionParameter = "action=request" - - case declinePath = "/decline" - case authorizationPrefix = "https://public-api.wordpress.com/connect/" - case accessDenied = "error=access_denied" - - case state = "state" - case code = "code" - case error = "error" - - // Special handling for the inconsistent way that services respond to a user's choice to decline - // oauth authorization. - // Right now we have no clear way to know if Tumblr fails. This is something we should try - // fixing moving forward. - // Path does not set the action param or call the callback. It forwards to its own URL ending in /decline. - case userRefused = "oauth_problem=user_refused" - - func containedIn(_ url: URL) -> Bool { - url.absoluteString.contains(rawValue) - } - } - /// Verification loading -- dismiss on completion /// private var loadingVerify: Bool = false @@ -157,47 +158,47 @@ class SharingAuthorizationWebViewController: WPWebViewController { private func authorizeAction(from url: URL) -> AuthorizeAction { // Path oauth declines are handled by a redirect to a path.com URL, so check this first. - if AuthorizeURLComponents.declinePath.containedIn(url) { + if PublicizeAuthorizationURLComponents.declinePath.containedIn(url) { return .deny } - if !url.absoluteString.hasPrefix(AuthorizeURLComponents.authorizationPrefix.rawValue) { + if !url.absoluteString.hasPrefix(PublicizeAuthorizationURLComponents.authorizationPrefix.rawValue) { return .none } - if AuthorizeURLComponents.requestActionParameter.containedIn(url) { + if PublicizeAuthorizationURLComponents.requestActionParameter.containedIn(url) { return .request } // Check the rest of the various decline ranges - if AuthorizeURLComponents.denyActionParameter.containedIn(url) { + if PublicizeAuthorizationURLComponents.denyActionParameter.containedIn(url) { return .deny } // LinkedIn - if AuthorizeURLComponents.userRefused.containedIn(url) { + if PublicizeAuthorizationURLComponents.userRefused.containedIn(url) { return .deny } // Facebook and Google+ - if AuthorizeURLComponents.accessDenied.containedIn(url) { + if PublicizeAuthorizationURLComponents.accessDenied.containedIn(url) { return .deny } // If we've made it this far and verifyRange is found then we're *probably* // verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) // where verification is declined and we get a false positive. - if AuthorizeURLComponents.verifyActionParameter.containedIn(url) { + if PublicizeAuthorizationURLComponents.verifyActionParameter.containedIn(url) { return .verify } // Facebook - if AuthorizeURLComponents.state.containedIn(url) && AuthorizeURLComponents.code.containedIn(url) { + if PublicizeAuthorizationURLComponents.state.containedIn(url) && PublicizeAuthorizationURLComponents.code.containedIn(url) { return .verify } // Facebook failure - if AuthorizeURLComponents.state.containedIn(url) && AuthorizeURLComponents.error.containedIn(url) { + if PublicizeAuthorizationURLComponents.state.containedIn(url) && PublicizeAuthorizationURLComponents.error.containedIn(url) { return .unknown } diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index 0af5bc44f838..ebf096bec952 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -313,6 +313,7 @@ 178DDD31266D7576006C68C4 /* BloggingRemindersFlowCompletionViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 178DDD2F266D7576006C68C4 /* BloggingRemindersFlowCompletionViewController.swift */; }; 178DDD57266E4165006C68C4 /* CalendarDayToggleButton.swift in Sources */ = {isa = PBXBuildFile; fileRef = 178DDD56266E4165006C68C4 /* CalendarDayToggleButton.swift */; }; 1790A4531E28F0ED00AE54C2 /* UINavigationController+Helpers.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1790A4521E28F0ED00AE54C2 /* UINavigationController+Helpers.swift */; }; + 179501CD27A01D4100882787 /* PublicizeAuthorizationURLComponentsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 179501CC27A01D4100882787 /* PublicizeAuthorizationURLComponentsTests.swift */; }; 1797373720EBAA4100377B4E /* RouteMatcherTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1797373620EBAA4100377B4E /* RouteMatcherTests.swift */; }; 179A70F02729834B006DAC0A /* Binding+OnChange.swift in Sources */ = {isa = PBXBuildFile; fileRef = 179A70EF2729834B006DAC0A /* Binding+OnChange.swift */; }; 179A70F12729834B006DAC0A /* Binding+OnChange.swift in Sources */ = {isa = PBXBuildFile; fileRef = 179A70EF2729834B006DAC0A /* Binding+OnChange.swift */; }; @@ -4939,6 +4940,7 @@ 178DDD2F266D7576006C68C4 /* BloggingRemindersFlowCompletionViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BloggingRemindersFlowCompletionViewController.swift; sourceTree = ""; }; 178DDD56266E4165006C68C4 /* CalendarDayToggleButton.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CalendarDayToggleButton.swift; sourceTree = ""; }; 1790A4521E28F0ED00AE54C2 /* UINavigationController+Helpers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "UINavigationController+Helpers.swift"; sourceTree = ""; }; + 179501CC27A01D4100882787 /* PublicizeAuthorizationURLComponentsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PublicizeAuthorizationURLComponentsTests.swift; sourceTree = ""; }; 1797373620EBAA4100377B4E /* RouteMatcherTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RouteMatcherTests.swift; sourceTree = ""; }; 179A70EF2729834B006DAC0A /* Binding+OnChange.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Binding+OnChange.swift"; sourceTree = ""; }; 17A09B98238FE13B0022AE0D /* FeatureFlagOverrideStore.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FeatureFlagOverrideStore.swift; sourceTree = ""; }; @@ -11078,6 +11080,7 @@ 24B1AE3024FEC79900B9F334 /* RemoteFeatureFlagTests.swift */, F551E7F623FC9A5C00751212 /* Collection+RotateTests.swift */, FAE8EE9B273AD0A800A65307 /* QuickStartSettingsTests.swift */, + 179501CC27A01D4100882787 /* PublicizeAuthorizationURLComponentsTests.swift */, ); name = Utility; sourceTree = ""; @@ -19236,6 +19239,7 @@ 246D0A0325E97D5D0028B83F /* Blog+ObjcTests.m in Sources */, 9A9D34FF2360A4E200BC95A3 /* StatsPeriodAsyncOperationTests.swift in Sources */, B5EFB1C91B333C5A007608A3 /* NotificationSettingsServiceTests.swift in Sources */, + 179501CD27A01D4100882787 /* PublicizeAuthorizationURLComponentsTests.swift in Sources */, 4089C51422371EE30031CE78 /* TodayStatsTests.swift in Sources */, 7E53AB0A20FE83A9005796FE /* MockContentCoordinator.swift in Sources */, BE1071FF1BC75FFA00906AFF /* WPStyleGuide+BlogTests.swift in Sources */, diff --git a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift new file mode 100644 index 000000000000..f227af19f6aa --- /dev/null +++ b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift @@ -0,0 +1,60 @@ +import XCTest +@testable import WordPress + +class PublicizeAuthorizationURLComponentsTests: XCTestCase { + override func setUp() { + } + + override func tearDown() { + } + + func testURLContainingAuthorizationPrefix() { + let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.authorizationPrefix.containedIn(url)) + } + + func testURLContainingVerifyActionParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.verifyActionParameter.containedIn(url)) + } + + func testURLContainingDenyActionParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?action=deny")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.denyActionParameter.containedIn(url)) + } + + func testURLContainingRequestActionParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?action=request")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.requestActionParameter.containedIn(url)) + } + + func testURLContainingDeclinePath() { + let url = URL(string: "https://public-api.wordpress.com/connect/decline")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.declinePath.containedIn(url)) + } + + func testURLContainingAccessDeniedErrorParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?error=access_denied")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.accessDenied.containedIn(url)) + } + + func testURLContainingStateParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.state.containedIn(url)) + } + + func testURLContainingCodeParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.code.containedIn(url)) + } + + func testURLContainingErrorParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&error=1234567")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.error.containedIn(url)) + } + + func testURLContainingUserRefusedParameter() { + let url = URL(string: "https://public-api.wordpress.com/connect/?oauth_problem=user_refused")! + XCTAssertTrue(PublicizeAuthorizationURLComponents.userRefused.containedIn(url)) + } +} From f2bcba2eda192d48fc67fe9f638b7273a1e09147 Mon Sep 17 00:00:00 2001 From: James Frost Date: Tue, 25 Jan 2022 16:53:00 +0000 Subject: [PATCH 2/6] Refactor PublicizeAuthorizationURLComponents to check for query items or contained substrings --- ...haringAuthorizationWebViewController.swift | 123 ++++++++++++++---- ...icizeAuthorizationURLComponentsTests.swift | 26 ++-- 2 files changed, 114 insertions(+), 35 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift index 1fe38d1c465d..3c8b35ffa17e 100644 --- a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift +++ b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift @@ -1,28 +1,104 @@ import WebKit import CoreMedia -enum PublicizeAuthorizationURLComponents: String { - case verifyActionParameter = "action=verify" - case denyActionParameter = "action=deny" - case requestActionParameter = "action=request" - - case declinePath = "/decline" - case authorizationPrefix = "https://public-api.wordpress.com/connect/" - case accessDenied = "error=access_denied" - - case state = "state" - case code = "code" - case error = "error" +/// Used to detect whether a URL matches a particular Publicize authorization success or failure route. +enum PublicizeAuthorizationURLComponents { + case verifyActionItem + case denyActionItem + case requestActionItem + case stateItem + case codeItem + case errorItem + + case authorizationPrefix + case declinePath + case accessDenied // Special handling for the inconsistent way that services respond to a user's choice to decline // oauth authorization. // Right now we have no clear way to know if Tumblr fails. This is something we should try // fixing moving forward. // Path does not set the action param or call the callback. It forwards to its own URL ending in /decline. - case userRefused = "oauth_problem=user_refused" + case userRefused + + // In most cases, we attempt to find a matching URL by checking for a specific URL component + private var queryItem: URLQueryItem? { + switch self { + case .verifyActionItem: + return URLQueryItem(name: "action", value: "verify") + case .denyActionItem: + return URLQueryItem(name: "action", value: "deny") + case .requestActionItem: + return URLQueryItem(name: "action", value: "request") + case .accessDenied: + return URLQueryItem(name: "error", value: "access_denied") + case .stateItem: + return URLQueryItem(name: "state", value: nil) + case .codeItem: + return URLQueryItem(name: "code", value: nil) + case .errorItem: + return URLQueryItem(name: "error", value: nil) + case .userRefused: + return URLQueryItem(name: "oauth_problem", value: "user_refused") + default: + return nil + } + } + // In a handful of cases, we're just looking for a substring or prefix in the URL + private var matchString: String? { + switch self { + case .declinePath: + return "/decline" + case .authorizationPrefix: + return "https://public-api.wordpress.com/connect/" + default: + return nil + } + } + + /// @return True if the url matches the current authorization component + /// func containedIn(_ url: URL) -> Bool { - return url.absoluteString.contains(rawValue) + if let _ = queryItem { + return queryItemContainedIn(url) + } + + return stringContainedIn(url) + } + + // Checks to see if the current QueryItem is present in the specified URL + private func queryItemContainedIn(_ url: URL) -> Bool { + guard let queryItems = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems, + let queryItem = queryItem else { + return false + } + + return queryItems.contains(where: { urlItem in + var result = urlItem.name == queryItem.name + + if let value = queryItem.value { + result = result && (urlItem.value == value) + } + + return result + }) + } + + // Checks to see if the current matchString is present in the specified URL + private func stringContainedIn(_ url: URL) -> Bool { + guard let matchString = matchString else { + return false + } + + switch self { + case .declinePath: + return url.path.contains(matchString) + case .authorizationPrefix: + return url.absoluteString.hasPrefix(matchString) + default: + return url.absoluteString.contains(matchString) + } } } @@ -42,7 +118,7 @@ protocol SharingAuthorizationDelegate: NSObjectProtocol { class SharingAuthorizationWebViewController: WPWebViewController { /// Classify actions taken by the web API /// - private enum AuthorizeAction: Int { + enum AuthorizeAction: Int { case none case unknown case request @@ -156,22 +232,25 @@ class SharingAuthorizationWebViewController: WPWebViewController { // MARK: - URL Interpretation - private func authorizeAction(from url: URL) -> AuthorizeAction { + func authorizeAction(from url: URL) -> AuthorizeAction { // Path oauth declines are handled by a redirect to a path.com URL, so check this first. if PublicizeAuthorizationURLComponents.declinePath.containedIn(url) { return .deny } - if !url.absoluteString.hasPrefix(PublicizeAuthorizationURLComponents.authorizationPrefix.rawValue) { + if !url.absoluteString.hasPrefix("https://public-api.wordpress.com/connect/") { return .none } +// if !url.absoluteString.hasPrefix(PublicizeAuthorizationURLComponents.authorizationPrefix.rawValue) { +// return .none +// } - if PublicizeAuthorizationURLComponents.requestActionParameter.containedIn(url) { + if PublicizeAuthorizationURLComponents.requestActionItem.containedIn(url) { return .request } // Check the rest of the various decline ranges - if PublicizeAuthorizationURLComponents.denyActionParameter.containedIn(url) { + if PublicizeAuthorizationURLComponents.denyActionItem.containedIn(url) { return .deny } @@ -188,17 +267,17 @@ class SharingAuthorizationWebViewController: WPWebViewController { // If we've made it this far and verifyRange is found then we're *probably* // verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) // where verification is declined and we get a false positive. - if PublicizeAuthorizationURLComponents.verifyActionParameter.containedIn(url) { + if PublicizeAuthorizationURLComponents.verifyActionItem.containedIn(url) { return .verify } // Facebook - if PublicizeAuthorizationURLComponents.state.containedIn(url) && PublicizeAuthorizationURLComponents.code.containedIn(url) { + if PublicizeAuthorizationURLComponents.stateItem.containedIn(url) && PublicizeAuthorizationURLComponents.codeItem.containedIn(url) { return .verify } // Facebook failure - if PublicizeAuthorizationURLComponents.state.containedIn(url) && PublicizeAuthorizationURLComponents.error.containedIn(url) { + if PublicizeAuthorizationURLComponents.stateItem.containedIn(url) && PublicizeAuthorizationURLComponents.errorItem.containedIn(url) { return .unknown } diff --git a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift index f227af19f6aa..325b5597e5ac 100644 --- a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift +++ b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift @@ -8,24 +8,24 @@ class PublicizeAuthorizationURLComponentsTests: XCTestCase { override func tearDown() { } - func testURLContainingAuthorizationPrefix() { + func testURLContainingAuthorizationItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! XCTAssertTrue(PublicizeAuthorizationURLComponents.authorizationPrefix.containedIn(url)) } - func testURLContainingVerifyActionParameter() { + func testURLContainingVerifyActionItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.verifyActionParameter.containedIn(url)) + XCTAssertTrue(PublicizeAuthorizationURLComponents.verifyActionItem.containedIn(url)) } func testURLContainingDenyActionParameter() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=deny")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.denyActionParameter.containedIn(url)) + XCTAssertTrue(PublicizeAuthorizationURLComponents.denyActionItem.containedIn(url)) } - func testURLContainingRequestActionParameter() { + func testURLContainingRequestActionItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=request")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.requestActionParameter.containedIn(url)) + XCTAssertTrue(PublicizeAuthorizationURLComponents.requestActionItem.containedIn(url)) } func testURLContainingDeclinePath() { @@ -33,24 +33,24 @@ class PublicizeAuthorizationURLComponentsTests: XCTestCase { XCTAssertTrue(PublicizeAuthorizationURLComponents.declinePath.containedIn(url)) } - func testURLContainingAccessDeniedErrorParameter() { + func testURLContainingAccessDeniedErrorItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?error=access_denied")! XCTAssertTrue(PublicizeAuthorizationURLComponents.accessDenied.containedIn(url)) } - func testURLContainingStateParameter() { + func testURLContainingStateItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.state.containedIn(url)) + XCTAssertTrue(PublicizeAuthorizationURLComponents.stateItem.containedIn(url)) } - func testURLContainingCodeParameter() { + func testURLContainingCodeItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.code.containedIn(url)) + XCTAssertTrue(PublicizeAuthorizationURLComponents.codeItem.containedIn(url)) } - func testURLContainingErrorParameter() { + func testURLContainingErrorItemItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&error=1234567")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.error.containedIn(url)) + XCTAssertTrue(PublicizeAuthorizationURLComponents.errorItem.containedIn(url)) } func testURLContainingUserRefusedParameter() { From a3d896f32546fadb46fa9e3eb1c7601a09eb3544 Mon Sep 17 00:00:00 2001 From: James Frost Date: Tue, 25 Jan 2022 17:04:49 +0000 Subject: [PATCH 3/6] Move Publicize connection URL matching to its own file --- .../Blog/PublicizeConnectionURLMatcher.swift | 102 +++++++++++++++ ...haringAuthorizationWebViewController.swift | 117 ++---------------- WordPress/WordPress.xcodeproj/project.pbxproj | 6 + ...icizeAuthorizationURLComponentsTests.swift | 22 ++-- 4 files changed, 127 insertions(+), 120 deletions(-) create mode 100644 WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift diff --git a/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift new file mode 100644 index 000000000000..9c31a049f456 --- /dev/null +++ b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift @@ -0,0 +1,102 @@ +import Foundation + +/// Used to detect whether a URL matches a particular Publicize authorization success or failure route. +enum PublicizeConnectionURLMatcher { + case verifyActionItem + case denyActionItem + case requestActionItem + case stateItem + case codeItem + case errorItem + + case authorizationPrefix + case declinePath + case accessDenied + + // Special handling for the inconsistent way that services respond to a user's choice to decline + // oauth authorization. + // Right now we have no clear way to know if Tumblr fails. This is something we should try + // fixing moving forward. + // Path does not set the action param or call the callback. It forwards to its own URL ending in /decline. + case userRefused + + // In most cases, we attempt to find a matching URL by checking for a specific URL component + private var queryItem: URLQueryItem? { + switch self { + case .verifyActionItem: + return URLQueryItem(name: "action", value: "verify") + case .denyActionItem: + return URLQueryItem(name: "action", value: "deny") + case .requestActionItem: + return URLQueryItem(name: "action", value: "request") + case .accessDenied: + return URLQueryItem(name: "error", value: "access_denied") + case .stateItem: + return URLQueryItem(name: "state", value: nil) + case .codeItem: + return URLQueryItem(name: "code", value: nil) + case .errorItem: + return URLQueryItem(name: "error", value: nil) + case .userRefused: + return URLQueryItem(name: "oauth_problem", value: "user_refused") + default: + return nil + } + } + + // In a handful of cases, we're just looking for a substring or prefix in the URL + private var matchString: String? { + switch self { + case .declinePath: + return "/decline" + case .authorizationPrefix: + return "https://public-api.wordpress.com/connect/" + default: + return nil + } + } + + /// @return True if the url matches the current authorization component + /// + func containedIn(_ url: URL) -> Bool { + if let _ = queryItem { + return queryItemContainedIn(url) + } + + return stringContainedIn(url) + } + + // Checks to see if the current QueryItem is present in the specified URL + private func queryItemContainedIn(_ url: URL) -> Bool { + guard let queryItems = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems, + let queryItem = queryItem else { + return false + } + + return queryItems.contains(where: { urlItem in + var result = urlItem.name == queryItem.name + + if let value = queryItem.value { + result = result && (urlItem.value == value) + } + + return result + }) + } + + // Checks to see if the current matchString is present in the specified URL + private func stringContainedIn(_ url: URL) -> Bool { + guard let matchString = matchString else { + return false + } + + switch self { + case .declinePath: + return url.path.contains(matchString) + case .authorizationPrefix: + return url.absoluteString.hasPrefix(matchString) + default: + return url.absoluteString.contains(matchString) + } + } +} diff --git a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift index 3c8b35ffa17e..030215981355 100644 --- a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift +++ b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift @@ -1,107 +1,6 @@ import WebKit import CoreMedia -/// Used to detect whether a URL matches a particular Publicize authorization success or failure route. -enum PublicizeAuthorizationURLComponents { - case verifyActionItem - case denyActionItem - case requestActionItem - case stateItem - case codeItem - case errorItem - - case authorizationPrefix - case declinePath - case accessDenied - - // Special handling for the inconsistent way that services respond to a user's choice to decline - // oauth authorization. - // Right now we have no clear way to know if Tumblr fails. This is something we should try - // fixing moving forward. - // Path does not set the action param or call the callback. It forwards to its own URL ending in /decline. - case userRefused - - // In most cases, we attempt to find a matching URL by checking for a specific URL component - private var queryItem: URLQueryItem? { - switch self { - case .verifyActionItem: - return URLQueryItem(name: "action", value: "verify") - case .denyActionItem: - return URLQueryItem(name: "action", value: "deny") - case .requestActionItem: - return URLQueryItem(name: "action", value: "request") - case .accessDenied: - return URLQueryItem(name: "error", value: "access_denied") - case .stateItem: - return URLQueryItem(name: "state", value: nil) - case .codeItem: - return URLQueryItem(name: "code", value: nil) - case .errorItem: - return URLQueryItem(name: "error", value: nil) - case .userRefused: - return URLQueryItem(name: "oauth_problem", value: "user_refused") - default: - return nil - } - } - - // In a handful of cases, we're just looking for a substring or prefix in the URL - private var matchString: String? { - switch self { - case .declinePath: - return "/decline" - case .authorizationPrefix: - return "https://public-api.wordpress.com/connect/" - default: - return nil - } - } - - /// @return True if the url matches the current authorization component - /// - func containedIn(_ url: URL) -> Bool { - if let _ = queryItem { - return queryItemContainedIn(url) - } - - return stringContainedIn(url) - } - - // Checks to see if the current QueryItem is present in the specified URL - private func queryItemContainedIn(_ url: URL) -> Bool { - guard let queryItems = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems, - let queryItem = queryItem else { - return false - } - - return queryItems.contains(where: { urlItem in - var result = urlItem.name == queryItem.name - - if let value = queryItem.value { - result = result && (urlItem.value == value) - } - - return result - }) - } - - // Checks to see if the current matchString is present in the specified URL - private func stringContainedIn(_ url: URL) -> Bool { - guard let matchString = matchString else { - return false - } - - switch self { - case .declinePath: - return url.path.contains(matchString) - case .authorizationPrefix: - return url.absoluteString.hasPrefix(matchString) - default: - return url.absoluteString.contains(matchString) - } - } -} - @objc protocol SharingAuthorizationDelegate: NSObjectProtocol { @objc @@ -234,7 +133,7 @@ class SharingAuthorizationWebViewController: WPWebViewController { func authorizeAction(from url: URL) -> AuthorizeAction { // Path oauth declines are handled by a redirect to a path.com URL, so check this first. - if PublicizeAuthorizationURLComponents.declinePath.containedIn(url) { + if PublicizeConnectionURLMatcher.declinePath.containedIn(url) { return .deny } @@ -245,39 +144,39 @@ class SharingAuthorizationWebViewController: WPWebViewController { // return .none // } - if PublicizeAuthorizationURLComponents.requestActionItem.containedIn(url) { + if PublicizeConnectionURLMatcher.requestActionItem.containedIn(url) { return .request } // Check the rest of the various decline ranges - if PublicizeAuthorizationURLComponents.denyActionItem.containedIn(url) { + if PublicizeConnectionURLMatcher.denyActionItem.containedIn(url) { return .deny } // LinkedIn - if PublicizeAuthorizationURLComponents.userRefused.containedIn(url) { + if PublicizeConnectionURLMatcher.userRefused.containedIn(url) { return .deny } // Facebook and Google+ - if PublicizeAuthorizationURLComponents.accessDenied.containedIn(url) { + if PublicizeConnectionURLMatcher.accessDenied.containedIn(url) { return .deny } // If we've made it this far and verifyRange is found then we're *probably* // verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) // where verification is declined and we get a false positive. - if PublicizeAuthorizationURLComponents.verifyActionItem.containedIn(url) { + if PublicizeConnectionURLMatcher.verifyActionItem.containedIn(url) { return .verify } // Facebook - if PublicizeAuthorizationURLComponents.stateItem.containedIn(url) && PublicizeAuthorizationURLComponents.codeItem.containedIn(url) { + if PublicizeConnectionURLMatcher.stateItem.containedIn(url) && PublicizeConnectionURLMatcher.codeItem.containedIn(url) { return .verify } // Facebook failure - if PublicizeAuthorizationURLComponents.stateItem.containedIn(url) && PublicizeAuthorizationURLComponents.errorItem.containedIn(url) { + if PublicizeConnectionURLMatcher.stateItem.containedIn(url) && PublicizeConnectionURLMatcher.errorItem.containedIn(url) { return .unknown } diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index ebf096bec952..16c0e00772be 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -244,6 +244,8 @@ 1752D4FA238D702E002B79E7 /* KeyValueDatabase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1751E5901CE0E552000CA08D /* KeyValueDatabase.swift */; }; 1752D4FB238D702F002B79E7 /* KeyValueDatabase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1751E5901CE0E552000CA08D /* KeyValueDatabase.swift */; }; 1752D4FC238D703A002B79E7 /* KeyValueDatabase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1751E5901CE0E552000CA08D /* KeyValueDatabase.swift */; }; + 175507B327A062980038ED28 /* PublicizeConnectionURLMatcher.swift in Sources */ = {isa = PBXBuildFile; fileRef = 175507B227A062980038ED28 /* PublicizeConnectionURLMatcher.swift */; }; + 175507B427A062980038ED28 /* PublicizeConnectionURLMatcher.swift in Sources */ = {isa = PBXBuildFile; fileRef = 175507B227A062980038ED28 /* PublicizeConnectionURLMatcher.swift */; }; 175721162754D31F00DE38BC /* AppIcon.swift in Sources */ = {isa = PBXBuildFile; fileRef = 175721152754D31F00DE38BC /* AppIcon.swift */; }; 175721172754D31F00DE38BC /* AppIcon.swift in Sources */ = {isa = PBXBuildFile; fileRef = 175721152754D31F00DE38BC /* AppIcon.swift */; }; 1759F1701FE017BF0003EC81 /* Queue.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1759F16F1FE017BF0003EC81 /* Queue.swift */; }; @@ -4879,6 +4881,7 @@ 1751E5901CE0E552000CA08D /* KeyValueDatabase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = KeyValueDatabase.swift; sourceTree = ""; }; 1751E5921CE23801000CA08D /* NSAttributedString+StyledHTML.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "NSAttributedString+StyledHTML.swift"; sourceTree = ""; }; 17523380246C4F9200870B4A /* HomepageSettingsViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HomepageSettingsViewController.swift; sourceTree = ""; }; + 175507B227A062980038ED28 /* PublicizeConnectionURLMatcher.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PublicizeConnectionURLMatcher.swift; sourceTree = ""; }; 175721152754D31F00DE38BC /* AppIcon.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AppIcon.swift; sourceTree = ""; }; 1759F16F1FE017BF0003EC81 /* Queue.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Queue.swift; sourceTree = ""; }; 1759F1711FE017F20003EC81 /* QueueTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = QueueTests.swift; sourceTree = ""; }; @@ -14056,6 +14059,7 @@ E63BBC941C5168BE00598BE8 /* SharingAuthorizationHelper.h */, E63BBC951C5168BE00598BE8 /* SharingAuthorizationHelper.m */, F16601C323E9E783007950AE /* SharingAuthorizationWebViewController.swift */, + 175507B227A062980038ED28 /* PublicizeConnectionURLMatcher.swift */, E663D18F1C65383E0017F109 /* SharingAccountViewController.swift */, E6431DE11C4E892900FD8D90 /* SharingDetailViewController.h */, E6431DE21C4E892900FD8D90 /* SharingDetailViewController.m */, @@ -17447,6 +17451,7 @@ E64384831C628FCC0052ADB5 /* WPStyleGuide+Sharing.swift in Sources */, F504D2B025D60C5900A2764C /* StoryPoster.swift in Sources */, 981C82B62193A7B900A06E84 /* Double+Stats.swift in Sources */, + 175507B327A062980038ED28 /* PublicizeConnectionURLMatcher.swift in Sources */, 177074851FB209F100951A4A /* CircularProgressView.swift in Sources */, 98458CB821A39D350025D232 /* StatsNoDataRow.swift in Sources */, 3234BB172530DFCA0068DA40 /* ReaderTableCardCell.swift in Sources */, @@ -20161,6 +20166,7 @@ FABB23C22602FC2C00C8785C /* PrepublishingHeaderView.swift in Sources */, 3FB1929126C6C56E000F5AA3 /* TimeSelectionButton.swift in Sources */, FABB23C32602FC2C00C8785C /* StatsWidgetsStore.swift in Sources */, + 175507B427A062980038ED28 /* PublicizeConnectionURLMatcher.swift in Sources */, FABB23C42602FC2C00C8785C /* MenuItemsVisualOrderingView.m in Sources */, FABB23C52602FC2C00C8785C /* JetpackScanViewController.swift in Sources */, FABB23C62602FC2C00C8785C /* ReaderSubscribingNotificationAction.swift in Sources */, diff --git a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift index 325b5597e5ac..92968d933c96 100644 --- a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift +++ b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift @@ -1,7 +1,7 @@ import XCTest @testable import WordPress -class PublicizeAuthorizationURLComponentsTests: XCTestCase { +class PublicizeConnectionURLMatcherTests: XCTestCase { override func setUp() { } @@ -10,51 +10,51 @@ class PublicizeAuthorizationURLComponentsTests: XCTestCase { func testURLContainingAuthorizationItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.authorizationPrefix.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.authorizationPrefix.containedIn(url)) } func testURLContainingVerifyActionItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.verifyActionItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.verifyActionItem.containedIn(url)) } func testURLContainingDenyActionParameter() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=deny")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.denyActionItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.denyActionItem.containedIn(url)) } func testURLContainingRequestActionItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=request")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.requestActionItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.requestActionItem.containedIn(url)) } func testURLContainingDeclinePath() { let url = URL(string: "https://public-api.wordpress.com/connect/decline")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.declinePath.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.declinePath.containedIn(url)) } func testURLContainingAccessDeniedErrorItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?error=access_denied")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.accessDenied.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.accessDenied.containedIn(url)) } func testURLContainingStateItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.stateItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.stateItem.containedIn(url)) } func testURLContainingCodeItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.codeItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.codeItem.containedIn(url)) } func testURLContainingErrorItemItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&error=1234567")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.errorItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.errorItem.containedIn(url)) } func testURLContainingUserRefusedParameter() { let url = URL(string: "https://public-api.wordpress.com/connect/?oauth_problem=user_refused")! - XCTAssertTrue(PublicizeAuthorizationURLComponents.userRefused.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.userRefused.containedIn(url)) } } From 9a440ac5c46126c5ace36930e85b161ea9df456d Mon Sep 17 00:00:00 2001 From: James Frost Date: Wed, 26 Jan 2022 10:54:11 +0000 Subject: [PATCH 4/6] Refactor Publicize URL matcher to handle both query item and string matching --- .../Blog/PublicizeConnectionURLMatcher.swift | 140 +++++++++--------- ...haringAuthorizationWebViewController.swift | 21 ++- ...icizeAuthorizationURLComponentsTests.swift | 20 +-- 3 files changed, 87 insertions(+), 94 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift index 9c31a049f456..687d0f45f79a 100644 --- a/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift +++ b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift @@ -1,77 +1,89 @@ import Foundation /// Used to detect whether a URL matches a particular Publicize authorization success or failure route. -enum PublicizeConnectionURLMatcher { - case verifyActionItem - case denyActionItem - case requestActionItem - case stateItem - case codeItem - case errorItem +struct PublicizeConnectionURLMatcher { + enum MatchComponent { + case verifyActionItem + case denyActionItem + case requestActionItem + case stateItem + case codeItem + case errorItem - case authorizationPrefix - case declinePath - case accessDenied + case authorizationPrefix + case declinePath + case accessDenied - // Special handling for the inconsistent way that services respond to a user's choice to decline - // oauth authorization. - // Right now we have no clear way to know if Tumblr fails. This is something we should try - // fixing moving forward. - // Path does not set the action param or call the callback. It forwards to its own URL ending in /decline. - case userRefused + // Special handling for the inconsistent way that services respond to a user's choice to decline + // oauth authorization. + // Right now we have no clear way to know if Tumblr fails. This is something we should try + // fixing moving forward. + // Path does not set the action param or call the callback. It forwards to its own URL ending in /decline. + case userRefused - // In most cases, we attempt to find a matching URL by checking for a specific URL component - private var queryItem: URLQueryItem? { - switch self { - case .verifyActionItem: - return URLQueryItem(name: "action", value: "verify") - case .denyActionItem: - return URLQueryItem(name: "action", value: "deny") - case .requestActionItem: - return URLQueryItem(name: "action", value: "request") - case .accessDenied: - return URLQueryItem(name: "error", value: "access_denied") - case .stateItem: - return URLQueryItem(name: "state", value: nil) - case .codeItem: - return URLQueryItem(name: "code", value: nil) - case .errorItem: - return URLQueryItem(name: "error", value: nil) - case .userRefused: - return URLQueryItem(name: "oauth_problem", value: "user_refused") - default: - return nil + // In most cases, we attempt to find a matching URL by checking for a specific URL component + fileprivate var queryItem: URLQueryItem? { + switch self { + case .verifyActionItem: + return URLQueryItem(name: "action", value: "verify") + case .denyActionItem: + return URLQueryItem(name: "action", value: "deny") + case .requestActionItem: + return URLQueryItem(name: "action", value: "request") + case .accessDenied: + return URLQueryItem(name: "error", value: "access_denied") + case .stateItem: + return URLQueryItem(name: "state", value: nil) + case .codeItem: + return URLQueryItem(name: "code", value: nil) + case .errorItem: + return URLQueryItem(name: "error", value: nil) + case .userRefused: + return URLQueryItem(name: "oauth_problem", value: "user_refused") + default: + return nil + } } - } - // In a handful of cases, we're just looking for a substring or prefix in the URL - private var matchString: String? { - switch self { - case .declinePath: - return "/decline" - case .authorizationPrefix: - return "https://public-api.wordpress.com/connect/" - default: - return nil + // In a handful of cases, we're just looking for a substring or prefix in the URL + fileprivate var matchString: String? { + switch self { + case .declinePath: + return "/decline" + case .authorizationPrefix: + return "https://public-api.wordpress.com/connect/" + default: + return nil + } } } /// @return True if the url matches the current authorization component /// - func containedIn(_ url: URL) -> Bool { - if let _ = queryItem { - return queryItemContainedIn(url) + static func url(_ url: URL, contains matchComponent: MatchComponent) -> Bool { + if let queryItem = matchComponent.queryItem { + return self.url(url, contains: queryItem) } - return stringContainedIn(url) + if let matchString = matchComponent.matchString { + switch matchComponent { + case .declinePath: + return url.path.contains(matchString) + case .authorizationPrefix: + return url.absoluteString.hasPrefix(matchString) + default: + return url.absoluteString.contains(matchString) + } + } + + return false } // Checks to see if the current QueryItem is present in the specified URL - private func queryItemContainedIn(_ url: URL) -> Bool { - guard let queryItems = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems, - let queryItem = queryItem else { - return false - } + private static func url(_ url: URL, contains queryItem: URLQueryItem) -> Bool { + guard let queryItems = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems else { + return false + } return queryItems.contains(where: { urlItem in var result = urlItem.name == queryItem.name @@ -83,20 +95,4 @@ enum PublicizeConnectionURLMatcher { return result }) } - - // Checks to see if the current matchString is present in the specified URL - private func stringContainedIn(_ url: URL) -> Bool { - guard let matchString = matchString else { - return false - } - - switch self { - case .declinePath: - return url.path.contains(matchString) - case .authorizationPrefix: - return url.absoluteString.hasPrefix(matchString) - default: - return url.absoluteString.contains(matchString) - } - } } diff --git a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift index 030215981355..ef3f4ae19e0f 100644 --- a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift +++ b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift @@ -133,50 +133,47 @@ class SharingAuthorizationWebViewController: WPWebViewController { func authorizeAction(from url: URL) -> AuthorizeAction { // Path oauth declines are handled by a redirect to a path.com URL, so check this first. - if PublicizeConnectionURLMatcher.declinePath.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .declinePath) { return .deny } - if !url.absoluteString.hasPrefix("https://public-api.wordpress.com/connect/") { + if !PublicizeConnectionURLMatcher.url(url, contains: .authorizationPrefix) { return .none } -// if !url.absoluteString.hasPrefix(PublicizeAuthorizationURLComponents.authorizationPrefix.rawValue) { -// return .none -// } - if PublicizeConnectionURLMatcher.requestActionItem.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .requestActionItem) { return .request } // Check the rest of the various decline ranges - if PublicizeConnectionURLMatcher.denyActionItem.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .denyActionItem) { return .deny } // LinkedIn - if PublicizeConnectionURLMatcher.userRefused.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .userRefused) { return .deny } // Facebook and Google+ - if PublicizeConnectionURLMatcher.accessDenied.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .accessDenied) { return .deny } // If we've made it this far and verifyRange is found then we're *probably* // verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) // where verification is declined and we get a false positive. - if PublicizeConnectionURLMatcher.verifyActionItem.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .verifyActionItem) { return .verify } // Facebook - if PublicizeConnectionURLMatcher.stateItem.containedIn(url) && PublicizeConnectionURLMatcher.codeItem.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .stateItem) && PublicizeConnectionURLMatcher.url(url, contains: .codeItem) { return .verify } // Facebook failure - if PublicizeConnectionURLMatcher.stateItem.containedIn(url) && PublicizeConnectionURLMatcher.errorItem.containedIn(url) { + if PublicizeConnectionURLMatcher.url(url, contains: .stateItem) && PublicizeConnectionURLMatcher.url(url, contains: .errorItem) { return .unknown } diff --git a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift index 92968d933c96..ed48d560faf8 100644 --- a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift +++ b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift @@ -10,51 +10,51 @@ class PublicizeConnectionURLMatcherTests: XCTestCase { func testURLContainingAuthorizationItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! - XCTAssertTrue(PublicizeConnectionURLMatcher.authorizationPrefix.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .authorizationPrefix)) } func testURLContainingVerifyActionItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! - XCTAssertTrue(PublicizeConnectionURLMatcher.verifyActionItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .verifyActionItem)) } func testURLContainingDenyActionParameter() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=deny")! - XCTAssertTrue(PublicizeConnectionURLMatcher.denyActionItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .denyActionItem)) } func testURLContainingRequestActionItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=request")! - XCTAssertTrue(PublicizeConnectionURLMatcher.requestActionItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .requestActionItem)) } func testURLContainingDeclinePath() { let url = URL(string: "https://public-api.wordpress.com/connect/decline")! - XCTAssertTrue(PublicizeConnectionURLMatcher.declinePath.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .declinePath)) } func testURLContainingAccessDeniedErrorItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?error=access_denied")! - XCTAssertTrue(PublicizeConnectionURLMatcher.accessDenied.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .accessDenied)) } func testURLContainingStateItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! - XCTAssertTrue(PublicizeConnectionURLMatcher.stateItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .stateItem)) } func testURLContainingCodeItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&code=1234567")! - XCTAssertTrue(PublicizeConnectionURLMatcher.codeItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .codeItem)) } func testURLContainingErrorItemItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&error=1234567")! - XCTAssertTrue(PublicizeConnectionURLMatcher.errorItem.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .errorItem)) } func testURLContainingUserRefusedParameter() { let url = URL(string: "https://public-api.wordpress.com/connect/?oauth_problem=user_refused")! - XCTAssertTrue(PublicizeConnectionURLMatcher.userRefused.containedIn(url)) + XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .userRefused)) } } From 8bb971ca2d150191e2c2dce51b52c2b694f0a9f1 Mon Sep 17 00:00:00 2001 From: James Frost Date: Wed, 26 Jan 2022 11:32:45 +0000 Subject: [PATCH 5/6] Move authorize actions to Publicize URL matcher and add tests --- .../Blog/PublicizeConnectionURLMatcher.swift | 61 ++++++++++++++++++ ...haringAuthorizationWebViewController.swift | 63 +------------------ ...icizeAuthorizationURLComponentsTests.swift | 50 +++++++++++++++ 3 files changed, 112 insertions(+), 62 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift index 687d0f45f79a..780446d96361 100644 --- a/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift +++ b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift @@ -95,4 +95,65 @@ struct PublicizeConnectionURLMatcher { return result }) } + + // MARK: - Authorization Actions + + /// Classify actions taken by the web API + /// + enum AuthorizeAction: Int { + case none + case unknown + case request + case verify + case deny + } + + static func authorizeAction(for matchURL: URL) -> AuthorizeAction { + // Path oauth declines are handled by a redirect to a path.com URL, so check this first. + if url(matchURL, contains: .declinePath) { + return .deny + } + + if !url(matchURL, contains: .authorizationPrefix) { + return .none + } + + if url(matchURL, contains: .requestActionItem) { + return .request + } + + // Check the rest of the various decline ranges + if url(matchURL, contains: .denyActionItem) { + return .deny + } + + // LinkedIn + if url(matchURL, contains: .userRefused) { + return .deny + } + + // Facebook and Google+ + if url(matchURL, contains: .accessDenied) { + return .deny + } + + // If we've made it this far and verifyRange is found then we're *probably* + // verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) + // where verification is declined and we get a false positive. + if url(matchURL, contains: .verifyActionItem) { + return .verify + } + + // Facebook + if url(matchURL, contains: .stateItem) && url(matchURL, contains: .codeItem) { + return .verify + } + + // Facebook failure + if url(matchURL, contains: .stateItem) && url(matchURL, contains: .errorItem) { + return .unknown + } + + return .unknown + } } diff --git a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift index ef3f4ae19e0f..cf8d2ddd801b 100644 --- a/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift +++ b/WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift @@ -15,16 +15,6 @@ protocol SharingAuthorizationDelegate: NSObjectProtocol { @objc class SharingAuthorizationWebViewController: WPWebViewController { - /// Classify actions taken by the web API - /// - enum AuthorizeAction: Int { - case none - case unknown - case request - case verify - case deny - } - private static let loginURL = "https://wordpress.com/wp-login.php" /// Verification loading -- dismiss on completion @@ -128,57 +118,6 @@ class SharingAuthorizationWebViewController: WPWebViewController { private func displayLoadError(error: NSError) { delegate?.authorize(self.publicizer, didFailWithError: error) } - - // MARK: - URL Interpretation - - func authorizeAction(from url: URL) -> AuthorizeAction { - // Path oauth declines are handled by a redirect to a path.com URL, so check this first. - if PublicizeConnectionURLMatcher.url(url, contains: .declinePath) { - return .deny - } - - if !PublicizeConnectionURLMatcher.url(url, contains: .authorizationPrefix) { - return .none - } - - if PublicizeConnectionURLMatcher.url(url, contains: .requestActionItem) { - return .request - } - - // Check the rest of the various decline ranges - if PublicizeConnectionURLMatcher.url(url, contains: .denyActionItem) { - return .deny - } - - // LinkedIn - if PublicizeConnectionURLMatcher.url(url, contains: .userRefused) { - return .deny - } - - // Facebook and Google+ - if PublicizeConnectionURLMatcher.url(url, contains: .accessDenied) { - return .deny - } - - // If we've made it this far and verifyRange is found then we're *probably* - // verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) - // where verification is declined and we get a false positive. - if PublicizeConnectionURLMatcher.url(url, contains: .verifyActionItem) { - return .verify - } - - // Facebook - if PublicizeConnectionURLMatcher.url(url, contains: .stateItem) && PublicizeConnectionURLMatcher.url(url, contains: .codeItem) { - return .verify - } - - // Facebook failure - if PublicizeConnectionURLMatcher.url(url, contains: .stateItem) && PublicizeConnectionURLMatcher.url(url, contains: .errorItem) { - return .unknown - } - - return .unknown - } } // MARK: - WKNavigationDelegate @@ -194,7 +133,7 @@ extension SharingAuthorizationWebViewController { return } - let action = authorizeAction(from: url) + let action = PublicizeConnectionURLMatcher.authorizeAction(for: url) switch action { case .none: diff --git a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift index ed48d560faf8..7f2cafaea1d5 100644 --- a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift +++ b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift @@ -57,4 +57,54 @@ class PublicizeConnectionURLMatcherTests: XCTestCase { let url = URL(string: "https://public-api.wordpress.com/connect/?oauth_problem=user_refused")! XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .userRefused)) } + + // MARK: - Authorize Actions + + func testAuthorizeActionForDeclineURL() { + let url = URL(string: "https://public-api.wordpress.com/decline?state=test")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: url), .deny) + } + + func testAuthorizeActionWhenNoAuthorizationPrefix() { + let url = URL(string: "https://public-api.wordpress.com/example")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: url), .none) + } + + func testAuthorizeActionForVerifyAction() { + let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: url), .verify) + } + + func testAuthorizeActionForDenyAction() { + let url = URL(string: "https://public-api.wordpress.com/connect/?action=deny")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: url), .deny) + } + + func testAuthorizeActionForRequestAction() { + let url = URL(string: "https://public-api.wordpress.com/connect/?action=request")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: url), .request) + } + + func testAuthorizeActionWhenUserRefused() { + let url = URL(string: "https://public-api.wordpress.com/connect/?oauth_problem=user_refused")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: url), .deny) + } + + func testAuthorizeActionWhenAccessDenied() { + let url = URL(string: "https://public-api.wordpress.com/connect/?error=access_denied")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: url), .deny) + } + + func testAuthorizeActionForFacebook() { + let successURL = URL(string: "https://public-api.wordpress.com/connect/?state=1234&code=abcd")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: successURL), .verify) + + // Ensure code must be present + let unknownURL = URL(string: "https://public-api.wordpress.com/connect/?state=1234")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: unknownURL), .unknown) + + // Error case + let errorURL = URL(string: "https://public-api.wordpress.com/connect/?state=1234&error=abcd")! + XCTAssertEqual(PublicizeConnectionURLMatcher.authorizeAction(for: errorURL), .unknown) + } } From 23c73ff5f4d54df4ed3f3965a9bc545b30870aec Mon Sep 17 00:00:00 2001 From: James Frost Date: Thu, 27 Jan 2022 21:50:04 +0000 Subject: [PATCH 6/6] Code cleanup --- .../ViewRelated/Blog/PublicizeConnectionURLMatcher.swift | 6 +++--- .../PublicizeAuthorizationURLComponentsTests.swift | 8 +------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift index 780446d96361..98c7673b05c8 100644 --- a/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift +++ b/WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift @@ -51,7 +51,7 @@ struct PublicizeConnectionURLMatcher { case .declinePath: return "/decline" case .authorizationPrefix: - return "https://public-api.wordpress.com/connect/" + return "https://public-api.wordpress.com/connect" default: return nil } @@ -137,8 +137,8 @@ struct PublicizeConnectionURLMatcher { return .deny } - // If we've made it this far and verifyRange is found then we're *probably* - // verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) + // If we've made it this far and the `action=verify` query param is present then we're + // *probably* verifying the oauth request. There are edge cases ( :cough: tumblr :cough: ) // where verification is declined and we get a false positive. if url(matchURL, contains: .verifyActionItem) { return .verify diff --git a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift index 7f2cafaea1d5..5f8c07a6dd66 100644 --- a/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift +++ b/WordPress/WordPressTest/PublicizeAuthorizationURLComponentsTests.swift @@ -2,12 +2,6 @@ import XCTest @testable import WordPress class PublicizeConnectionURLMatcherTests: XCTestCase { - override func setUp() { - } - - override func tearDown() { - } - func testURLContainingAuthorizationItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?action=verify")! XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .authorizationPrefix)) @@ -48,7 +42,7 @@ class PublicizeConnectionURLMatcherTests: XCTestCase { XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .codeItem)) } - func testURLContainingErrorItemItem() { + func testURLContainingErrorItem() { let url = URL(string: "https://public-api.wordpress.com/connect/?state=abcdef&error=1234567")! XCTAssertTrue(PublicizeConnectionURLMatcher.url(url, contains: .errorItem)) }