WordPress-Android icon indicating copy to clipboard operation
WordPress-Android copied to clipboard

[Jetpack Social] Implement workaround for publicize WebView Google sign-in

Open RenanLukas opened this issue 2 years ago β€’ 7 comments

Do not merge

Fixes #18999

Screen Shot 2023-08-20 at 11 43 49 AM

This is happening because of the user-agent in the WebView used by publicize flow.

The default user-agent in Android versions >= 10 contains the string wv.

Note An easy way to verify the default user-agent used by WebViews in API 17+ is by using WebSettings.getDefaultUserAgent(context)

Example version < Android 10:

Mozilla/5.0 (Linux; Android 4.4; Nexus 5 Build/_BuildID_) 
AppleWebKit/537.36 (KHTML, like Gecko) 
Version/4.0 Chrome/30.0.0.0 Mobile Safari/537.36

Example version >= Android 10:

Mozilla/5.0 (Linux; Android 5.1.1; Nexus 5 Build/LMY48B; wv)
AppleWebKit/537.36 (KHTML, like Gecko) 
Version/4.0 Chrome/43.0.2357.65 Mobile Safari/537.36

Since 2017 Google have started to block Google sign-in in all embedded browsers (such as web views). This is the reason we get the error Error 403: disallowed_useragent.

The way they probably identify a web view is by checking the user-agent, because if we manually remove the wv part, the Google sign-in works. I've tried using our own implementation of the user-agent:

        val userAgent: String by lazy {
            if (TextUtils.isEmpty(defaultUserAgent)) {
                WordPress.USER_AGENT_APPNAME + "/" + PackageUtils.getVersionName(context)
            } else {
                (defaultUserAgent + " " + WordPress.USER_AGENT_APPNAME + "/" + PackageUtils.getVersionName(context))
            }
        }

The problem is that since it also uses the default WebView user-agent, the final string also contains wv and we get the same error.

Workaround 1

If we add anything to the user-agent, Google sign-in works. For example, I've managed to perform a Google sign-in with the following code:

        mWebView.getSettings().setUserAgentString("Anything that doesn't contain the webview constant");

The problem with this approach is that we get a security alert email from Google ("New sign-in to your account"), which sounds like a deal breaker to me.

Workaround 2

Like mentioned above, if we manually remove wv from the WebView user-agent string, the Google sign-in works:

        mWebView.getSettings().getUserAgentString().replace("; wv)", ")")

We could even use our own implementation of the WebView user-agent if we replace the string:

        mWebView.getSettings().setUserAgentString(WordPress.getUserAgent().replace("; wv)", ")"));

We also don't get a security email from Google, but IMHO this solution is weak because we're not in control of the default WebView user-agent, which means if this string ever changes (e.g. wv becomes something else) Google sign-in will stop working in this flow.

Workaround 3

This is the workaround I've implemented in this PR, because it seemed better than relying on string replacement.

        mWebView.getSettings().setUserAgentString(System.getProperty("http.agent"));

It doesn't use the default user-agent of a WebView, but instead it uses the default user-agent for HTTP requests. The reason I believe this is not as weak as Workaround 2 is because even though we're not in control of the default HTTP user-agent implementation it is something completely separate from WebViews. This is the agent we'd be using in HTTP requests, so I don't see a reason why it would ever know something about WebViews. Besides, it seems to be a completely valid user-agent because unlike Workaround 1 I was not only able to log in but also didn't get a security alert email.

What the proper solution looks like

It's worth saying that these workarounds might have security implications. Google's recommendation is to replace the WebView with Chrome Custom Tabs. The reason I don't believe this will be necessarily a straightforward migration is because we have a considerable amount of custom logic in our publicize WebView, for example:

            mWebView.setWebViewClient(new PublicizeWebViewClient());
            mWebView.setWebChromeClient(new PublicizeWebChromeClient());
            mWebView.getSettings().setCacheMode(WebSettings.LOAD_NO_CACHE);
            mWebView.getSettings().setJavaScriptEnabled(true);
            mWebView.getSettings().setDomStorageEnabled(true);

Custom Tabs is not exactly an equivalent or new implementation of WebView, but instead it's a completely different thing: while WebView is basically a View that displays a web page (an embedded browser), Custom Tabs allows the app to actually launch the user's preferred browser directly in the app's flow instead of opening the browser app. Since we're actually launching the user's browser instead of having our own embedded browser, there are many things we don't have control over, such as whether JavaScript is enabled or not (seen in the code above).

TL;DR migrating to Custom Tabs seems to be the way to go, but that means we'll have a significant amount of changes in the publicize flow code.

To test: 1 - Install JP and log in; 2 - Select a site; 3 - Tap on "My Site"; 4 - Tap on "Menu"; 5 - Select "Social"; 6 - Select a social network without any connected accounts that supports Google sign-in (e.g. Tumblr); 7 - Tap "Connect"; 8 - Try to log in using Google sign-in: it should work as expected without error 403.

Regression Notes

  1. Potential unintended areas of impact See description above.

  2. What I did to test those areas of impact (or what existing automated tests I relied on) Manual testing.

  3. What automated tests I added (or what prevented me from doing so) --

PR submission checklist:

  • [x] I have completed the Regression Notes.
  • [x] I have considered adding accessibility improvements for my changes.
  • [x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • [ ] Portrait and landscape orientations.
  • [ ] Light and dark modes.
  • [ ] Fonts: Larger, smaller and bold text.
  • [ ] High contrast.
  • [ ] Talkback.
  • [ ] Languages with large words or with letters/accents not frequently used in English.
  • [ ] Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • [ ] Large and small screen sizes. (Tablet and smaller phones)
  • [ ] Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

RenanLukas avatar Aug 20 '23 17:08 RenanLukas

Considering the above, I wanted to ask if we should go with this workaround (in case it's working as expected) or if we should actually take time to implement the proper solution. I'm asking this because I suppose this is a critical bug already affecting people in production.

cc @tiagomar @develric @thomashorta

RenanLukas avatar Aug 20 '23 17:08 RenanLukas

JetpackπŸ“² You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr19009-7edac84
Commit7edac8460bc517c06de550c4f4be058ccbca2a96
Direct Downloadjetpack-prototype-build-pr19009-7edac84.apk
Note: Google Login is not supported on these builds.

wpmobilebot avatar Aug 20 '23 17:08 wpmobilebot

WordPressπŸ“² You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr19009-7edac84
Commit7edac8460bc517c06de550c4f4be058ccbca2a96
Direct Downloadwordpress-prototype-build-pr19009-7edac84.apk
Note: Google Login is not supported on these builds.

wpmobilebot avatar Aug 20 '23 17:08 wpmobilebot

Thanks for the review, @thomashorta

I had a quick question though: do you think it makes sense to add that to other WebViews in the app as well?

That's a good question. I thought about it, but IMHO it would only make sense to have this change in the WebViews where we need a Google sign-in. I couldn't remember any other besides this one in publicize flow, but in case there are others might make sense to update them as well πŸ‘ WDYT?

RenanLukas avatar Aug 21 '23 15:08 RenanLukas

IMHO it would only make sense to have this change in the WebViews where we need a Google sign-in. I couldn't remember any other besides this one in publicize flow, but in case there are others might make sense to update them as well πŸ‘

Yeah, I think that makes sense, though it might be hard to identify what those other flows are. Maybe we can get this merged and keep this fix in the back of our minds in case we see similar issues/reports in other WebView flows.

thomashorta avatar Aug 21 '23 18:08 thomashorta

Warnings
:warning: PR is not assigned to a milestone.

Generated by :no_entry_sign: dangerJS

Since this PR implements an unofficial workaround, we're not merging it for now until we measure the impact of this issue. As shared above, it seems like the ideal solution would be the Custom Tabs migration.

Context: p1693405565033609/1693405176.286989-slack-C01CW1VMLAF

RenanLukas avatar Aug 30 '23 15:08 RenanLukas