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.

Sign up for GitHub

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

Refactors Publicize connection URL detection #17816

Merged
merged 7 commits into from
Jan 28, 2022
Merged

Refactors Publicize connection URL detection #17816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b98e781
Add tests for PublicizeAuthorizationURLComponents
frosty Jan 25, 2022
f2bcba2
Refactor PublicizeAuthorizationURLComponents to check for query items…
frosty Jan 25, 2022
a3d896f
Move Publicize connection URL matching to its own file
frosty Jan 25, 2022
9a440ac
Refactor Publicize URL matcher to handle both query item and string m…
frosty Jan 26, 2022
8bb971c
Move authorize actions to Publicize URL matcher and add tests
frosty Jan 26, 2022
23c73ff
Code cleanup
frosty Jan 27, 2022
0b54622
Merge branch 'trunk' into fix/publicize-connection-refactor
frosty Jan 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 159 additions & 0 deletions WordPress/Classes/ViewRelated/Blog/PublicizeConnectionURLMatcher.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import Foundation

/// Used to detect whether a URL matches a particular Publicize authorization success or failure route.
struct PublicizeConnectionURLMatcher {
enum MatchComponent {
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
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
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
///
static func url(_ url: URL, contains matchComponent: MatchComponent) -> Bool {
if let queryItem = matchComponent.queryItem {
return self.url(url, contains: queryItem)
}

if let matchString = matchComponent.matchString {
switch matchComponent {
case .declinePath:
return url.path.contains(matchString)
guarani marked this conversation as resolved.
Show resolved Hide resolved
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 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

if let value = queryItem.value {
result = result && (urlItem.value == value)
}

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 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
}

// 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
}
}
89 changes: 2 additions & 87 deletions WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import WebKit
import CoreMedia

@objc
protocol SharingAuthorizationDelegate: NSObjectProtocol {
Expand All @@ -14,43 +15,8 @@ protocol SharingAuthorizationDelegate: NSObjectProtocol {

@objc
class SharingAuthorizationWebViewController: WPWebViewController {
/// Classify actions taken by the web API
///
private enum AuthorizeAction: Int {
case none
case unknown
case request
case verify
case deny
}

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
Expand Down Expand Up @@ -152,57 +118,6 @@ class SharingAuthorizationWebViewController: WPWebViewController {
private func displayLoadError(error: NSError) {
delegate?.authorize(self.publicizer, didFailWithError: error)
}

// MARK: - URL Interpretation

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) {
return .deny
}

if !url.absoluteString.hasPrefix(AuthorizeURLComponents.authorizationPrefix.rawValue) {
return .none
}

if AuthorizeURLComponents.requestActionParameter.containedIn(url) {
return .request
}

// Check the rest of the various decline ranges
if AuthorizeURLComponents.denyActionParameter.containedIn(url) {
return .deny
}

// LinkedIn
if AuthorizeURLComponents.userRefused.containedIn(url) {
return .deny
}

// Facebook and Google+
if AuthorizeURLComponents.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) {
return .verify
}

// Facebook
if AuthorizeURLComponents.state.containedIn(url) && AuthorizeURLComponents.code.containedIn(url) {
return .verify
}

// Facebook failure
if AuthorizeURLComponents.state.containedIn(url) && AuthorizeURLComponents.error.containedIn(url) {
return .unknown
}

return .unknown
}
}

// MARK: - WKNavigationDelegate
Expand All @@ -218,7 +133,7 @@ extension SharingAuthorizationWebViewController {
return
}

let action = authorizeAction(from: url)
let action = PublicizeConnectionURLMatcher.authorizeAction(for: url)

switch action {
case .none:
Expand Down
10 changes: 10 additions & 0 deletions WordPress/WordPress.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -313,6 +315,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 */; };
Expand Down Expand Up @@ -4886,6 +4889,7 @@
1751E5901CE0E552000CA08D /* KeyValueDatabase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = KeyValueDatabase.swift; sourceTree = "<group>"; };
1751E5921CE23801000CA08D /* NSAttributedString+StyledHTML.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "NSAttributedString+StyledHTML.swift"; sourceTree = "<group>"; };
17523380246C4F9200870B4A /* HomepageSettingsViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HomepageSettingsViewController.swift; sourceTree = "<group>"; };
175507B227A062980038ED28 /* PublicizeConnectionURLMatcher.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PublicizeConnectionURLMatcher.swift; sourceTree = "<group>"; };
175721152754D31F00DE38BC /* AppIcon.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AppIcon.swift; sourceTree = "<group>"; };
1759F16F1FE017BF0003EC81 /* Queue.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Queue.swift; sourceTree = "<group>"; };
1759F1711FE017F20003EC81 /* QueueTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = QueueTests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -4947,6 +4951,7 @@
178DDD2F266D7576006C68C4 /* BloggingRemindersFlowCompletionViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BloggingRemindersFlowCompletionViewController.swift; sourceTree = "<group>"; };
178DDD56266E4165006C68C4 /* CalendarDayToggleButton.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CalendarDayToggleButton.swift; sourceTree = "<group>"; };
1790A4521E28F0ED00AE54C2 /* UINavigationController+Helpers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "UINavigationController+Helpers.swift"; sourceTree = "<group>"; };
179501CC27A01D4100882787 /* PublicizeAuthorizationURLComponentsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PublicizeAuthorizationURLComponentsTests.swift; sourceTree = "<group>"; };
1797373620EBAA4100377B4E /* RouteMatcherTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RouteMatcherTests.swift; sourceTree = "<group>"; };
179A70EF2729834B006DAC0A /* Binding+OnChange.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Binding+OnChange.swift"; sourceTree = "<group>"; };
17A09B98238FE13B0022AE0D /* FeatureFlagOverrideStore.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FeatureFlagOverrideStore.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -11092,6 +11097,7 @@
24B1AE3024FEC79900B9F334 /* RemoteFeatureFlagTests.swift */,
F551E7F623FC9A5C00751212 /* Collection+RotateTests.swift */,
FAE8EE9B273AD0A800A65307 /* QuickStartSettingsTests.swift */,
179501CC27A01D4100882787 /* PublicizeAuthorizationURLComponentsTests.swift */,
);
name = Utility;
sourceTree = "<group>";
Expand Down Expand Up @@ -14069,6 +14075,7 @@
E63BBC941C5168BE00598BE8 /* SharingAuthorizationHelper.h */,
E63BBC951C5168BE00598BE8 /* SharingAuthorizationHelper.m */,
F16601C323E9E783007950AE /* SharingAuthorizationWebViewController.swift */,
175507B227A062980038ED28 /* PublicizeConnectionURLMatcher.swift */,
E663D18F1C65383E0017F109 /* SharingAccountViewController.swift */,
E6431DE11C4E892900FD8D90 /* SharingDetailViewController.h */,
E6431DE21C4E892900FD8D90 /* SharingDetailViewController.m */,
Expand Down Expand Up @@ -17461,6 +17468,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 */,
Expand Down Expand Up @@ -19255,6 +19263,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 */,
Expand Down Expand Up @@ -20177,6 +20186,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 */,
Expand Down
Loading