-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Publicize: Fix Facebook auth success detection #17803
Conversation
@guarani I have some test accounts for Facebook and Twitter, so let me know if you need them 🙂 |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested Facebook, LinkedIn, and Twitter and all working great!
I left a minor code comment, but the change looks good to me
} | ||
|
||
if AuthorizeURLComponents.state.containedIn(url) && AuthorizeURLComponents.code.containedIn(url) { |
There was a problem hiding this comment.
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
}
}
Fixes #17794. The URL used to determine success for the Facebook Publicize connection flow has changed. This PR updates the check used to determine a Facebook success so that it matches the new URL (see final commit for those changes), as well as some slight refactoring to improve the existing code in this section.
The new URL should contain
state
andcode
values if it is a Facebook login, andstate
anderror
if it was unsuccessful. All other login types should be unaffected.Please see the original issue for more detail about the problem.
To test
Also ensure that you can still connect some other account types such as Twitter.
Regression Notes
Other account types connecting, as I refactored some of that code slightly.
I manually tested connecting to other account types. I also ensured all tests pass, although I can't see that we have tests for publicize currently.
We could add some to check these various success and failure URLs, but I wanted to merge this soon and it would be a reasonable amount of work to compile a list of all of the various URLs we're checking for.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.