wordpress-mobile / WordPressAuthenticator-iOS

GNU General Public License v2.0
17 stars 11 forks source link

Google sign in without SDK – New accounts lack email in `WPAccount` #759

Closed mokagio closed 1 year ago

mokagio commented 1 year ago

I tested creating a new WordPress.com account by signing in without the Google SDK and WordPress iOS crashed. This is the branch I was working on.

Screenshot 2023-03-24 at 3 52 00 pm

The issue, as far as I could trace it, was that when the app created a LoginEpilogueUserInfo with the WPAccount instance it just loaded from the DB (?!) via WPAccount.lookupDefaultWordPressComAccount(_:), the instance had no email.

mokagio commented 1 year ago

I've been doing more researching into this and I think I got to the root of the issue.

Here's an annotated copy of the code where the crash occurred:

func getUserInfo() {

    guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext) else {
        return
    }

    var userInfo = LoginEpilogueUserInfo(account: account)
    if let socialservice = socialService {
        // socialService gets set by the "legacy" implementation but not by the new one
        showPassword = false
        userInfo.update(with: socialservice)
    } else {
        if let customDisplayName = dataSource?.customDisplayName {
            userInfo.fullName = customDisplayName
        } else {
            // crashed into generateDisplayName(from:) because userInfo.email was ""
            let autoDisplayName = generateDisplayName(from: userInfo.email)

Drilling into the code I realized that the problem is that the SDK-less implementation results in getUserInfo() being called on a SignupEpilogueTableViewController instance that is configured for an email signup, not a user one.

This happens because:

// Set `googleUser` here, `didSignIn(token:, email:)` will do the rest.
loginFields.meta.googleUser = user

That code comes from https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/commit/08aa3a66eabde680d859c6f70e5911f95b538218, where I split the Google SDK part of the code from the rest with the intent of sharing as much implementation as possible.

The reason that assignment did not go in the shared code is that googleUser has type GIDGoogleUser.

Where to go from here

Luckily, there is only one client that reads that property: LoginEpilogueUserInfo update(with:) in WordPress. What that code reads from the GIDGoogleUser are the name and email info. That information is not Google-specific, we can pass it along without GIDGoogleUser.

My plan is:

The second will be a breaking change, but the upgrade effort will be quite limited given WooCommerce doesn't access the associate value and Jetpack/WordPress has only one access.