Workaround for SafariVC reporting failure while redirecting to iDEAL #937

danj-stripe · 2018-05-17T00:05:56Z

Summary

Ignores reported errors from SafariVC unless we know the error was for a
stripe.com domain.

Adds missing error handling to the example app for errors from STPRedirectContext

Motivation

Reported in #917 and to Stripe support, more details internally in IOS-729.

SafariVC is basically a black box: give it a URL and ask it to load. It'll
report via the delegate method when the initial load finishes, and whether
it was successful or not.

Prior to this commit, iDEAL (and others?) would fail and close SafariVC.
However, it hadn't actually failed, and just leaving the SafariVC open
would allow the user to finish the redirect workflow.

It's strange to me that SafariVC behaves this way. It's also a little
strange how the redirects are being handled by girogate.de. This works
around the interaction between the two, while still attempting to report
legitimate errors, so the host application can tell the user and allow them
to retry.

Testing

This is very much dependent on the specific behavior of the 3rd party and SafariVC, and
only shows up with livemode transactions.

I was able to isolate this behavior in a test application that just used SafariVC, hitting
a sinatra web-app that mimicked the redirect behavior I observed from girogate. That allowed
me to eliminate any Stripe SDK-specific code, and experiment to verify hypotheses.

Then, applied this fix, and made sure that the "SafariVC opens and then closes before the
page loads" bug wasn't happening any more.

Automated tests are passing.

Ignores reported errors from SafariVC unless we know the error was for a `stripe.com` domain. SafariVC is basically a black box: give it a URL and ask it to load. It'll report via the delegate method when the initial load finishes, and whether it was successful or not. Prior to this commit, iDEAL (and others?) would fail and close SafariVC. However, it hadn't actually failed, and just leaving the SafariVC open would allow the user to finish the redirect workflow. It's strange to me that SafariVC behaves this way. It's also a little strange how the redirects are being handled by girogate.de. This works around the interaction between the two, while still attempting to report legitimate errors, so the host application can tell the user and allow them to retry.

… example code The example code was ignoring the error parameter, instead of reporting on it.

teich-stripe · 2018-05-17T00:13:13Z

Stripe/STPRedirectContext.m

-                        shouldDismissViewController:YES];
-        });
+        if (@available(iOS 11, *)) {
+            stpDispatchToMainThreadIfNecessary(^{


We don't do anything here on iOS < 11?

If there was some way to distinguish between real failures and spurious ones, I'd love to. I wasn't able to find one though.

I had a similar initial reaction mostly by the way it's written. Maybe it needs an else clause with a doc:

stpDispatchToMainThreadIfNecessary(^{ if (didLoadSuccessfully == NO) { if (@available(iOS 11, *) && [self.latestSafariVCUrl.host containsString:@"stripe.com"]) { // Report load errors that exist on `stripe.com` website ... } else { // Do nothing because `latestSafariVCUrl` is not accurate `< iOS 11` } } }

In any case, this looks functional and don't have strong opinion on this one.

joeydong-stripe · 2018-05-17T17:20:58Z

Stripe/STPRedirectContext.m

@@ -25,6 +25,8 @@ @interface STPRedirectContext () <SFSafariViewControllerDelegate, STPURLCallback
 @property (nonatomic, strong) STPSource *source;
 @property (nonatomic, strong, nullable) SFSafariViewController *safariVC;
 @property (nonatomic, assign, readwrite) STPRedirectContextState state;
+/// If we're on iOS 11+ and in the SafariVC flow, this tracks the latest URL loaded/redirected to during the initial load
+@property (nonatomic, strong, readwrite, nullable) NSURL *latestSafariVCUrl;


Maybe latestSafariVCUrl -> latestKnownSafariVCUrl to better emphasize it's an approximation

joeydong-stripe · 2018-05-17T17:28:35Z

Stripe/STPRedirectContext.m

-                        shouldDismissViewController:YES];
-        });
+        if (@available(iOS 11, *)) {
+            stpDispatchToMainThreadIfNecessary(^{


I had a similar initial reaction mostly by the way it's written. Maybe it needs an else clause with a doc:

stpDispatchToMainThreadIfNecessary(^{ if (didLoadSuccessfully == NO) { if (@available(iOS 11, *) && [self.latestSafariVCUrl.host containsString:@"stripe.com"]) { // Report load errors that exist on `stripe.com` website ... } else { // Do nothing because `latestSafariVCUrl` is not accurate `< iOS 11` } } }

In any case, this looks functional and don't have strong opinion on this one.

joeydong-stripe · 2018-05-17T19:03:44Z

Add changelog entry

Ah yes, good catch 😉

@available

…le redirecting I didn't realize that `STPRedirectContext` was as well tested as it is. I ran into test failures when executing on iOS 10 (our CI executes against iOS 11), and realized this needed to be updated. Recap of behavior change: only report errors in initial load on iOS 11+, and only if the latest URL was a `stripe.com` URL. When unit tests were run against iOS 11, this was still true, and the existing test passed. This adds two more tests: for pre-iOS 11 and for iOS 11 with a redirect to non-stripe.com. Both of those cases should not report an error, and should not close the Safari View Controller. We don't have `guard-let` in Obj-C, nor can we check `!@available(...)`, but I have emulated that structure at the beginning of these tests. I think it's worth it, and Joey didn't think it was unreasonable.

@available

…le redirecting (#945) * Add tests for changes from #937: ignoring 3rd party load failures while redirecting I didn't realize that `STPRedirectContext` was as well tested as it is. I ran into test failures when executing on iOS 10 (our CI executes against iOS 11), and realized this needed to be updated. Recap of behavior change: only report errors in initial load on iOS 11+, and only if the latest URL was a `stripe.com` URL. When unit tests were run against iOS 11, this was still true, and the existing test passed. This adds two more tests: for pre-iOS 11 and for iOS 11 with a redirect to non-stripe.com. Both of those cases should not report an error, and should not close the Safari View Controller. We don't have `guard-let` in Obj-C, nor can we check `!@available(...)`, but I have emulated that structure at the beginning of these tests. I think it's worth it, and Joey didn't think it was unreasonable. * Add missing calls to `unsubscribeContext:` in `STPRedirectContextTest` Per documentation at the top of the class: > [If] Your `sut` doesn't call unsubscribe and you explicitly `OCMReject` it firing > [Then] call `[self unsubscribeContext:context]` at the end of your test (use the original > `context` object here and _NOT_ the `sut` or it will not work). I missed that in one of the tests I just added, and while auditing all the tests I found another one that wasn't unsubscribing.

Allow parsing `AMERICAN_EXPRESS` and `DINERS_CLUB` strings as card brands.

danj-stripe added 2 commits May 16, 2018 16:25

Add error handling to STPRedirectContext completion block errors in…

35316d4

… example code The example code was ignoring the error parameter, instead of reporting on it.

teich-stripe reviewed May 17, 2018

View reviewed changes

danj-stripe requested a review from joeydong-stripe May 17, 2018 00:16

joeydong-stripe approved these changes May 17, 2018

View reviewed changes

stripe-ci added the approved label May 17, 2018

stripe-ci assigned danj-stripe May 17, 2018

danj-stripe added 2 commits May 17, 2018 11:44

Rename property and move comment for code review feedback

3d897c3

Add changelog entry

8ad344c

stripe-ci removed the approved label May 17, 2018

danj-stripe assigned joeydong-stripe and unassigned danj-stripe May 17, 2018

joeydong-stripe approved these changes May 17, 2018

View reviewed changes

stripe-ci added the approved label May 17, 2018

stripe-ci assigned danj-stripe and unassigned joeydong-stripe May 17, 2018

danj-stripe merged commit 46df046 into master May 17, 2018

danj-stripe deleted the danj/bugfix/failing-initial-load branch May 17, 2018 20:35

mludowise-stripe pushed a commit that referenced this pull request Apr 2, 2022

Handle card brand string variations (#937)

b52831e

Allow parsing `AMERICAN_EXPRESS` and `DINERS_CLUB` strings as card brands.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for SafariVC reporting failure while redirecting to iDEAL #937

Workaround for SafariVC reporting failure while redirecting to iDEAL #937

danj-stripe commented May 17, 2018

teich-stripe May 17, 2018

danj-stripe May 17, 2018

joeydong-stripe May 17, 2018

joeydong-stripe May 17, 2018

joeydong-stripe May 17, 2018

joeydong-stripe commented May 17, 2018

Workaround for SafariVC reporting failure while redirecting to iDEAL #937

Workaround for SafariVC reporting failure while redirecting to iDEAL #937

Conversation

danj-stripe commented May 17, 2018

Summary

Motivation

Testing

teich-stripe May 17, 2018

Choose a reason for hiding this comment

danj-stripe May 17, 2018

Choose a reason for hiding this comment

joeydong-stripe May 17, 2018

Choose a reason for hiding this comment

joeydong-stripe May 17, 2018

Choose a reason for hiding this comment

joeydong-stripe May 17, 2018

Choose a reason for hiding this comment

joeydong-stripe commented May 17, 2018