Add support for Google SignIn without SDK by mokagio · Pull Request #20128 · wordpress-mobile/WordPress-iOS

mokagio · 2023-02-13T03:03:21Z

Relies on the changes in WordPress Authenticator 5.6.0 to run Google SignIn without the Google SDK, plus the fixes for issues found after integrating, see wordpress-mobile/WordPressAuthenticator-iOS#764 and wordpress-mobile/WordPressAuthenticator-iOS#762. See also wordpress-mobile/WordPressAuthenticator-iOS#741

The behavior is off by default for App Store build, on otherwise. That is, App Store builds will still use the SDK, but all other builds won't. This should give us time to test the new implementation internally and see if we run against hard edges.

Testing

Given the "on internally" configuration, one can verify that Google SignIn doesn't use the SDK by adding a breakpoint in GoogleAuthenticator.swift (a file from the WordPressAuthenticator pod) line 190 (or wherever you prefer in that method or other methods in the Google authentication chain) and attempting to login with Google:

2023-03-21.13-24-29.2023-03-21.13_25_36.mp4

Repeat by either changing the hardcoded value or by enabling the flag from the in-app debug view and restarting the app (from Xcode, given we're interested in breakpoints) and observe the breakpoint being hit.

Regression Notes

Potential unintended areas of impact – App authentication
What I did to test those areas of impact (or what existing automated tests I relied on) – The logic is thoroughly tested at the WordPress Authenticator level
What automated tests I added (or what prevented me from doing so) – I started working on some at the library level but run into issues because of MFA.

Next steps

I need to create a new account via Google to see if it works. I don't see why because the only thing we do on our side is getting a token and email pair to pass to the WordPress API.
It would be good to add some logging to monitor the behavior in production

I have completed the Regression Notes.
I have considered adding unit tests for my changes. N.A.
I have considered adding accessibility improvements for my changes. N.A.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

wpmobilebot · 2023-03-17T05:00:08Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr20128-3fee26b
	Version	22.1
	Bundle ID	org.wordpress.alpha
	Commit	`3fee26b`
	App Center Build	WPiOS - One-Offs #5394

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-03-17T05:00:56Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr20128-3fee26b
	Version	22.1
	Bundle ID	com.jetpack.alpha
	Commit	`3fee26b`
	App Center Build	jetpack-installable-builds #4422

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

mokagio · 2023-03-21T03:21:03Z

+        let googleLogingWithoutSDK: Bool = {
+            switch BuildConfiguration.current {
+            case .appStore:
+                // Rely on the remote flag in production
+                return RemoteFeatureFlagStore().value(for: FeatureFlag.sdkLessGoogleSignIn)
+            case _:
+                // Do whatever is hardcoded or overridden on device in non-production builds
+                return FeatureFlag.sdkLessGoogleSignIn.enabled
+            }
+        }()


@hassaanelgarem @momo-ozawa you've been working on feature flags right?

Is this a reasonable approach? I was surprised by having to use a dedicated store to read remote values instead of accessing them directly from FeatureFlag.<case>.enabled so I'm not sure whether I'm using it right.

I considered moving this logic into the enabled implementation, but realized it would have created an infinite loop in case the value was not found remotely 😅

I was surprised by having to use a dedicated store to read remote values instead of accessing them directly from FeatureFlag..enabled

Yes, using remote feature flags is really unintuitive at the moment, but coincidently, this is something I'm working on fixing during HACK week (we're having a late HACK week within our team cause we were on release rotation during the original week). Ref: pbArwn-5Uc-p2#comment-7452

Is this a reasonable approach?

Yes, this looks reasonable. However, you could also ditch the whole switch and only depend on RemoteFeatureFlagStore.value(for:). Currently, value(for:) returns the overridden value if it exists. If not, it returns the remote value if it exists. If not, it returns the default (hardcoded) value. If you've added the switch just to facilitate testing, you can consider removing it. If it serves a different purpose, then this looks good 👍

FYI: There's a known issue where if you have a remote flag that is remotely set to true, you can't override it to return false. This is also something I'm working on fixing now.

Thank you for the feedback @hassaanelgarem 🙇‍♂️

However, you could also ditch the whole switch and only depend on RemoteFeatureFlagStore.value(for:). Currently, value(for:) returns the overridden value if it exists. If not, it returns the remote value if it exists. If not, it returns the default (hardcoded) value.

🤔

Interesting. Maybe there's a way to move those build-time configuration at the feature toggle level then? I'd like to force it to true for testing within Automattic, but start as false in production.

I'll look into it and update if necessary.

I'll look into it and update if necessary.

For the record, I've rebased and adopted the new syntax vai e91ca3c

mokagio · 2023-03-29T01:24:04Z

-        switch service {
-        case .google(let user):
-            fullName = user.profile?.name ?? String()
-            email = user.profile?.email ?? String()
-        case .apple(let user):
-            fullName = user.fullName
-            email = user.email
-        }
+        fullName = service.user.fullName
+        email = service.user.email


At the WordPressAuthenticator level, both cases now have the same associated type (because we no longer rely on Google's GIDGoogleUser. user is a computed var on SocialService that hides away that switch and unwrapping logic. See wordpress-mobile/WordPressAuthenticator-iOS#764

mokagio · 2023-03-30T11:45:26Z

I need to create a new account via Google to see if it works. I don't see why because the only thing we do on our side is getting a token and email pair to pass to the WordPress API.

Of course, it didn't work out of the box 🙃 But the issue was not with the APIs but with the port. See wordpress-mobile/WordPressAuthenticator-iOS#764

It would be good to add some logging to monitor the behavior in production

I since discovered that we already have extensive events tracked. There is little additional value in adding more to track whether we are using the Google SDK compared to the amount of code we'd need to write.

peril-wordpress-mobile · 2023-03-30T11:48:46Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

mokagio · 2023-03-30T11:50:31Z

-  pod 'WordPressAuthenticator', '~> 5.6-beta'
-  # pod 'WordPressAuthenticator', git: 'https://github.com/wordpress-mobile/WordPressAuthenticator-iOS.git', branch: 'trunk'
+  # pod 'WordPressAuthenticator', '~> 5.6-beta'
+  pod 'WordPressAuthenticator', git: 'https://github.com/wordpress-mobile/WordPressAuthenticator-iOS.git', branch: 'mokagio/social-service-without-google'


I'll point back to trunk once the PR for this branch (wordpress-mobile/WordPressAuthenticator-iOS#764) is merged and before merging this.

mokagio · 2023-04-03T01:03:27Z

Just a note to say that I'll start the 22.1 code freeze soon but I'll leave this PR in 22.1 aiming to ship it with a followup beta.

The reason I want to do this is to avoid delaying the testing of the Google SignIn without SDK in the real world for other two weeks.

crazytonyli · 2023-04-03T08:05:38Z

+            case .appStore:
+                // Rely on the remote flag in production
+                return RemoteFeatureFlag.sdkLessGoogleSignIn.enabled(using: remoteFeaturesStore)
+            case _:


I guess it makes sense that the _ syntax would compile here. But I think default is more suitable for "all other cases"?

But look at how homogeneous it looks with only case. 😄

I never dug this up, but I don't see any mention of case _ in the Swift book. Also taking into account the more relatively recent @unknown default syntax, it makes sense to prefer default.

I believe the usage of _ here falls into the Wildcard Pattern

While `case _` works, `default` is the recommended syntax. See conversation at #20128 (comment)

mokagio · 2023-04-05T04:14:35Z

Ouch! I should have updated the target branch 🤦‍♂️ This was supposed to land on release/22.1 not trunk!

The damage is done now. While I'd love to get real world feedback ASAP, I don't think it's worth jumping through hoops to cherry pick these changes into the release branch. 😞

There were conflicts on `Podfile` and `Podfile.lock` because `release/22.1` and `trunk` both changed the Gutenberg version. As usual, I resolved them with `git checkout --theirs`, keeping the value from `trunk` under the assumption that the latest version of Gutenberg, 1.93.0-alpha2, is what `trunk` needs to work and that if the 1.92.1 hotfix hasn't already been integrated in the latest release, it will be soon and the Gutenberg team will followup with a new version update on `trunk`. There was also a conflict on the `RELEASE-NOTES.txt` that GitHub picked up with its more refined conflicts engine and that auto-merge didn't address properly but that I manually fixed. That was due to `release/22.1` adding new release note entries – which will not make it into the App Store release notes – in the 22.1 section and `trunk` having #20128 editing that section, too. #20128 was meant to land on `release/22.1` but I forgot to update the base branch after starting the code freeze 🤦‍♂️😭 ``` <<<<<<< release/22.1 -- Incoming Change * [**] Add a "Personalize Home Tab" button to the bottom of the Home tab that allows changing cards visibility. [#20369] * [*] [Reader] Fix an issue that was causing the app to crash when tapping the More or Share buttons in Reader Detail screen. [#20490] * [*] Block editor: Avoid empty Gallery block error [WordPress/gutenberg#49557] ======= * [**] [internal] Refactored Google SignIn implementation to not use the Google SDK [#20128] >>>>>>> trunk -- Current Change ```

There were conflicts on `Podfile` and `Podfile.lock` because `release/22.1` and `trunk` both changed the Gutenberg version. As usual, I resolved them with `git checkout --theirs`, keeping the value from `trunk` under the assumption that the latest version of Gutenberg, 1.93.0-alpha2, is what `trunk` needs to work and that if the 1.92.1 hotfix hasn't already been integrated in the latest release, it will be soon and the Gutenberg team will followup with a new version update on `trunk`. There was also a conflict on the `RELEASE-NOTES.txt` that GitHub picked up with its more refined conflicts engine and that auto-merge didn't address properly but that I manually fixed. That was due to `release/22.1` adding new release note entries – which will not make it into the App Store release notes – in the 22.1 section and `trunk` having #20128 editing that section, too. #20128 was meant to land on `release/22.1` but I forgot to update the base branch after starting the code freeze 🤦‍♂️😭 Also, the entry about the personalize home tab had been removed on `trunk` via ec8a377. The release notes editor was notified about it, #20483 (review) ``` <<<<<<< release/22.1 -- Incoming Change * [**] Add a "Personalize Home Tab" button to the bottom of the Home tab that allows changing cards visibility. [#20369] * [*] [Reader] Fix an issue that was causing the app to crash when tapping the More or Share buttons in Reader Detail screen. [#20490] * [*] Block editor: Avoid empty Gallery block error [WordPress/gutenberg#49557] ======= * [**] [internal] Refactored Google SignIn implementation to not use the Google SDK [#20128] >>>>>>> trunk -- Current Change ```

mokagio mentioned this pull request Feb 13, 2023

Google SignIn without SDK – What's left to do wordpress-mobile/WordPressAuthenticator-iOS#741

Closed

10 tasks

mokagio mentioned this pull request Feb 22, 2023

SDK-less Google SignIn – Part 5 – Connect the pieces wordpress-mobile/WordPressAuthenticator-iOS#743

Merged

1 task

mokagio force-pushed the mokagio/sdk-less-google branch from 73ee4d5 to f5f311e Compare March 17, 2023 04:16

mokagio mentioned this pull request Mar 20, 2023

Implement VideoPress metadata fetching #20237

Merged

4 tasks

mokagio force-pushed the mokagio/sdk-less-google branch from e706ae7 to 98a6b7f Compare March 21, 2023 02:05

mokagio changed the title ~~WIP – Google SignIn without SDK~~ Google SignIn without SDK Mar 21, 2023

mokagio commented Mar 21, 2023

View reviewed changes

mokagio force-pushed the mokagio/sdk-less-google branch from 596bffa to fc71c9a Compare March 22, 2023 04:57

hassaanelgarem mentioned this pull request Mar 22, 2023

Remote Feature Flags: Separate local and remote feature flags #20381

Merged

4 tasks

mokagio mentioned this pull request Mar 24, 2023

Google sign in without SDK – New accounts lack email in WPAccount wordpress-mobile/WordPressAuthenticator-iOS#759

Closed

mokagio force-pushed the mokagio/sdk-less-google branch 2 times, most recently from 8c0ba7e to 940e399 Compare March 29, 2023 00:24

mokagio commented Mar 29, 2023

View reviewed changes

mokagio force-pushed the mokagio/sdk-less-google branch from 940e399 to 0644db7 Compare March 30, 2023 11:48

mokagio marked this pull request as ready for review March 30, 2023 11:48

mokagio requested a review from crazytonyli March 30, 2023 11:48

mokagio modified the milestones: 22.0 ❄️, 22.1 Mar 30, 2023

mokagio commented Mar 30, 2023

View reviewed changes

This was referenced Mar 30, 2023

Replace GIDGoogleUser with SocialService.User in case .google wordpress-mobile/WordPressAuthenticator-iOS#764

Merged

Bump version to 6.0.0 wordpress-mobile/WordPressAuthenticator-iOS#766

Merged

mokagio force-pushed the mokagio/sdk-less-google branch from c4c6bfb to 144b74a Compare April 3, 2023 06:33

crazytonyli approved these changes Apr 3, 2023

View reviewed changes

Add feature flag for SDK-less Google SignIn

72bd4f2

mokagio added 6 commits April 3, 2023 20:49

Use feature flag to drive SDK-less Google SignIn

d50df96

Use remote flag for SDK-less Google sign in in production

f918891

Add entry for SDK-less Google SignIn in RELEASE-NOTES.txt

f467ce3

Switch WordPressAuthenticator to branch with new user bugfix

f689c49

Point WordPressAuthenticator to stable version 6.0.0

bbf56c2

Use default instead of case _ in googleLogingWithoutSDK

3fee26b

mokagio force-pushed the mokagio/sdk-less-google branch from 144b74a to 3fee26b Compare April 3, 2023 10:52

mokagio enabled auto-merge April 3, 2023 10:53

mokagio changed the title ~~Google SignIn without SDK~~ Add support for Google SignIn without SDK Apr 3, 2023

mokagio added a commit that referenced this pull request Apr 3, 2023

Replace all case _: with default: – There was only one

06e2d26

While `case _` works, `default` is the recommended syntax. See conversation at #20128 (comment)

mokagio mentioned this pull request Apr 3, 2023

Replace all case _: with default: #20465

Merged

4 tasks

mokagio merged commit 4085c5b into trunk Apr 3, 2023

mokagio deleted the mokagio/sdk-less-google branch April 3, 2023 11:29

mokagio modified the milestones: 22.1 ❄️, 22.2 Apr 5, 2023

mokagio mentioned this pull request Apr 13, 2023

Merge beta 22.1.0.2 #20527

Merged

3 tasks

Uh oh!

Conversation

mokagio commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Regression Notes

Next steps

Uh oh!

wpmobilebot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Mar 30, 2023

Uh oh!

peril-wordpress-mobile Bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Apr 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mokagio commented Feb 13, 2023 •

edited

Loading

wpmobilebot commented Mar 17, 2023 •

edited

Loading

wpmobilebot commented Mar 17, 2023 •

edited

Loading

mokagio Mar 21, 2023 •

edited

Loading

peril-wordpress-mobile Bot commented Mar 30, 2023 •

edited

Loading