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

Publicize: Fix Facebook auth success detection #17803

Merged
merged 4 commits into from
Jan 21, 2022
Merged

Publicize: Fix Facebook auth success detection #17803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ca4d58
Sharing Authorization: Move auth url components to their own enum
frosty Jan 21, 2022
b0f8c79
Publicize: Switch authorize components to use enum cases
frosty Jan 21, 2022
62629ac
Publicize authorize: Add new Facebook cases
frosty Jan 21, 2022
1cfe665
Updated release notes
frosty Jan 21, 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
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* [*] Block editor: Highlight text: fix applying formatting for non-selected text [https://github.com/wordpress-mobile/gutenberg-mobile/pull/4471]
* [**] Block editor: Fix Android handling of Hebrew and Indonesian translations [https://github.com/wordpress-mobile/gutenberg-mobile/pull/4397]
* [***] Self-hosted sites: Fixed a crash when saving media and no Internet connection was available. [#17759]
* [*] Publicize: Fixed an issue where a successful login was not automatically detected when connecting a Facebook account to Publicize. [#17803]

19.0
-----
Expand Down
65 changes: 42 additions & 23 deletions WordPress/Classes/ViewRelated/Blog/SharingAuthorizationWebViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,31 @@ class SharingAuthorizationWebViewController: WPWebViewController {
}

private static let loginURL = "https://wordpress.com/wp-login.php"
private static let authorizationPrefix = "https://public-api.wordpress.com/connect/"
private static let requestActionParameter = "action=request"
private static let verifyActionParameter = "action=verify"
private static let denyActionParameter = "action=deny"

// 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.
private static let declinePath = "/decline"
private static let userRefused = "oauth_problem=user_refused"
private static let authorizationDenied = "denied="
private static let accessDenied = "error=access_denied"

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
///
Expand Down Expand Up @@ -145,43 +156,51 @@ class SharingAuthorizationWebViewController: WPWebViewController {
// MARK: - URL Interpretation

private func authorizeAction(from url: URL) -> AuthorizeAction {
let requested = url.absoluteString

// Path oauth declines are handled by a redirect to a path.com URL, so check this first.
if requested.range(of: SharingAuthorizationWebViewController.declinePath) != nil {
if AuthorizeURLComponents.declinePath.containedIn(url) {
return .deny
}

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

if requested.range(of: SharingAuthorizationWebViewController.requestActionParameter) != nil {
if AuthorizeURLComponents.requestActionParameter.containedIn(url) {
return .request
}

// Check the rest of the various decline ranges
if requested.range(of: SharingAuthorizationWebViewController.denyActionParameter) != nil {
if AuthorizeURLComponents.denyActionParameter.containedIn(url) {
return .deny
}

// LinkedIn
if requested.range(of: SharingAuthorizationWebViewController.userRefused) != nil {
if AuthorizeURLComponents.userRefused.containedIn(url) {
return .deny
}

// Facebook and Google+
if requested.range(of: SharingAuthorizationWebViewController.accessDenied) != nil {
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 requested.range(of: SharingAuthorizationWebViewController.verifyActionParameter) != nil {
if AuthorizeURLComponents.verifyActionParameter.containedIn(url) {
return .verify
}

// Facebook
if AuthorizeURLComponents.state.containedIn(url) && AuthorizeURLComponents.code.containedIn(url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a blocker, just a possible improvement.

The URL format I see is:

https://public-api.wordpress.com/connect/?code=VERY_LONG_RANDOM_STRING&state=VERY_LONG_RANDOM_STRING#_=_

I'm not sure if failure cases can also include these random strings (tokens). If they can, it seems possible (though somewhat unlikely) for theses tokens to sometimes contain the characters "code" and "state", causing a false positive here.

Perhaps looking for query params with the names "code" and "state" would work more reliably:

        if let queryItems = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems {
            let containsState = queryItems.contains(where: { queryItem in
                queryItem.name == AuthorizeURLComponents.state.rawValue
            })
            let containsCode = queryItems.contains(where: { queryItem in
                queryItem.name == AuthorizeURLComponents.code.rawValue
            })
            if containsState && containsCode {
                return .verify
            }
        }

return .verify
}

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

return .unknown
}
}
Expand Down