[Connect SDK] Support http/https pop-ups #9640

simond-stripe · 2024-11-15T22:59:32Z

Summary

http/https pop-ups are opened in a chrome custom tab and other pop-ups are unsupported.

Motivation

Certain embedded components rely on pop-ups for some functionality (ex. showing the user Stripe's privacy policy)

Testing

Added tests
Modified tests
Manually verified

Screenshots

demo-1731711260.mp4

simond-stripe · 2024-11-15T23:00:06Z

connect-example/src/main/java/com/stripe/android/connect/example/data/SettingsService.kt

@@ -42,7 +42,7 @@ class SettingsService @Inject constructor(@ApplicationContext context: Context)
    fun getAppearanceIdFlow(): Flow<AppearanceInfo.AppearanceId?> {
        return observePref(APPEARANCE_ID_KEY) { prefs, key ->
            val appearanceId = prefs.getString(key, null) ?: return@observePref null
-            AppearanceInfo.AppearanceId.valueOf(appearanceId)
+            AppearanceInfo.AppearanceId.entries.firstOrNull { it.name == appearanceId }


👀 this fixes a crash if any appearance id's change and can't be found anymore (something I ran into myself)

github-actions · 2024-11-15T23:05:36Z

Diffuse output:

OLD: identity-example-release-base.apk (signature: V1, V2)
NEW: identity-example-release-pr.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │     2 MiB │     2 MiB │  0 B │   4.1 MiB │   4.1 MiB │  0 B 
     arsc │     1 MiB │     1 MiB │  0 B │     1 MiB │     1 MiB │  0 B 
 manifest │   2.3 KiB │   2.3 KiB │  0 B │     8 KiB │     8 KiB │  0 B 
      res │ 301.8 KiB │ 301.8 KiB │  0 B │ 455.5 KiB │ 455.5 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.1 KiB │   7.1 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.2 KiB │  90.2 KiB │ +5 B │ 170.3 KiB │ 170.3 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ +5 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19966 │ 19966 │ 0 (+0 -0) 
   types │  6188 │  6188 │ 0 (+0 -0) 
 classes │  4979 │  4979 │ 0 (+0 -0) 
 methods │ 29759 │ 29759 │ 0 (+0 -0) 
  fields │ 17526 │ 17526 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3622 │ 3622 │  0

APK

   compressed    │   uncompressed   │                                           
──────────┬──────┼───────────┬──────┤                                           
 size     │ diff │ size      │ diff │ path                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 28.5 KiB │ +5 B │  62.9 KiB │  0 B │ ∆ META-INF/CERT.SF                        
  1.2 KiB │ -3 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA                       
    271 B │ +2 B │     120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
 25.3 KiB │ +1 B │  62.8 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 55.2 KiB │ +5 B │ 127.1 KiB │  0 B │ (total)

mludowise-stripe

The changes generally lgtm, but I think

We should add some test coverage for the various cases (mailto, http, https, allow-listed, non-allowlisted)
Please add a link to the PR description with the jira ticket for papertrail purposes 😄

mludowise-stripe · 2024-11-15T23:28:14Z

connect/src/main/java/com/stripe/android/connect/webview/StripeConnectWebViewClient.kt

+        view ?: return false // the view shouldn't be null, but if it is then don't handle the request
+        val url = request?.url ?: return false // if the request isn't for a url, then don't handle it
+        return if (url.host in ALLOWLISTED_HOSTS) {
+            logger.warning("(StripeConnectWebViewClient) Received pop-up for allow-listed host: $url")


Is this a warning because we don't expect getting any of these given that messaging between windows on Android isn't supported? If this is the case, we should add a TODO here to add an analytic so we can setup alerts for it.

I was going off of this in the spec: If host is allowlisted (connect.stripe.com or connect-js.stripe.com), print warning (stub out todo to log analytic error) because this isn't supported

Is that still accurate @mludowise-stripe? 👍 on adding an analytic here

mludowise-stripe · 2024-11-15T23:29:19Z

connect/src/main/java/com/stripe/android/connect/webview/StripeConnectWebViewClient.kt

+            logger.debug("(StripeConnectWebViewClient) Received unsupported pop-up request: $url")
+            true // block the request as it's unsupported


Is this getting handled in MXMOBILE-2914? If so, should we add a TODO for it?

We have legitimate use cases for mailto urls, so we will need to handle this. A TODO would help clarify we're planning to do this in a followup PR 😄

Yeah, good call

mludowise-stripe · 2024-11-15T23:32:31Z

connect/src/main/java/com/stripe/android/connect/webview/StripeWebChromeClient.kt

+ * A [WebChromeClient] that provides additional functionality for Stripe Connect Embedded Component WebViews.
+ *
+ * This class is currently empty, but it could be used to add additional functionality in the future
+ * Setting a [WebChromeClient] (even an empty one) is necessary for certain functionality, like
+ * [WebViewClient.shouldOverrideUrlLoading] to work properly.
+ */


mludowise-stripe · 2024-11-15T23:36:13Z

@simond-stripe here are the equivalent test cases on iOS:
https://github.com/stripe/stripe-ios/blob/4308bf5e/StripeConnect/StripeConnectTests/Internal/Webview/ConnectWebViewControllerTests.swift#L59-L146

simond-stripe added 2 commits November 15, 2024 15:03

Fix null ptr in settings (appearance doesn't exist)

94f8e64

Add pop-up supoort

9b43902

simond-stripe requested review from mludowise-stripe and lng-stripe November 15, 2024 22:59

simond-stripe requested a review from a team as a code owner November 15, 2024 22:59

simond-stripe commented Nov 15, 2024

View reviewed changes

simond-stripe added 2 commits November 15, 2024 18:00

Fix lint

eab2df6

Make class internal

8cf612f

mludowise-stripe requested changes Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connect SDK] Support http/https pop-ups #9640

[Connect SDK] Support http/https pop-ups #9640

simond-stripe commented Nov 15, 2024

simond-stripe Nov 15, 2024

github-actions bot commented Nov 15, 2024 •

edited

Loading

mludowise-stripe left a comment

mludowise-stripe Nov 15, 2024

simond-stripe Nov 18, 2024

mludowise-stripe Nov 15, 2024

simond-stripe Nov 18, 2024

mludowise-stripe Nov 15, 2024

mludowise-stripe commented Nov 15, 2024

		logger.debug("(StripeConnectWebViewClient) Received unsupported pop-up request: $url")
		true // block the request as it's unsupported

[Connect SDK] Support http/https pop-ups #9640

Are you sure you want to change the base?

[Connect SDK] Support http/https pop-ups #9640

Conversation

simond-stripe commented Nov 15, 2024

Summary

Motivation

Testing

Screenshots

simond-stripe Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2024 • edited Loading

mludowise-stripe left a comment

Choose a reason for hiding this comment

mludowise-stripe Nov 15, 2024

Choose a reason for hiding this comment

simond-stripe Nov 18, 2024

Choose a reason for hiding this comment

mludowise-stripe Nov 15, 2024

Choose a reason for hiding this comment

simond-stripe Nov 18, 2024

Choose a reason for hiding this comment

mludowise-stripe Nov 15, 2024

Choose a reason for hiding this comment

mludowise-stripe commented Nov 15, 2024

github-actions bot commented Nov 15, 2024 •

edited

Loading