stripe / stripe-android

Stripe Android SDK
https://stripe.com/docs/mobile/android
MIT License
1.29k stars 645 forks source link

EphemeralKeyProvider Called Twice #797

Closed rayliverified closed 5 years ago

rayliverified commented 5 years ago

EphemeralKeyProvider is called twice, once upon attaching the KeyProvider and again when performing any getInstance() method.

Ephemeral Key is obtained here via this code:

CustomerSession.initCustomerSession(EphemeralKeyProvider(App.mInstance, premiumRepositoryImpl, object : EphemeralKeyProvider.ProgressListener {
            override fun onStringResponse(string: String) {
                if (!string.startsWith("Error: ")) {
                    stripeEphemeralKey = string
                } else {

                }
            }
        }))

And then again here:

CustomerSession.getInstance().retrieveCurrentCustomer(object: CustomerSession.CustomerRetrievalListener {
            override fun onCustomerRetrieved(customer: Customer) {
            }
            override fun onError(errorCode: Int, errorMessage: String?) {
            }
        })

Whenever any of the getInstance() methods are called, the ephemeral key is generated again. maxthonsnap20190204115614

This means the first ephemeral key generated is redundant. Generating an ephemeral key takes some time and is slow on high latency networks. Minimizing the key generation calls especially if they are redundant should be considered?

mshafrir-stripe commented 5 years ago

@searchy2 thanks for filing. I've created an internal ticket and will add it to our team's backlog.

rayliverified commented 5 years ago

Thanks for taking a look and adding this to your backlog. Generating the ephm key twice doesn't cause problems as far as I can tell but it makes me somewhat nervous.

mshafrir-stripe commented 5 years ago

@searchy2 thinking about this some more, this is arguably a "feature" instead of a "bug". Making the request for a ephemeral key in advance so that it's ready to be used when needed may save perceived response time for the end user.

rayliverified commented 5 years ago

Correct me if I'm wrong, my understanding is that the ephemeral key is used for initializing a customer session so it needs to be fetched in the initCustomerSession. Then, I'm guessing it's fetched again during an retrieveCurrentCustomer call because of staleness concerns?

For building out a shopping cart flow, it makes sense to initialize the customer and generate the ephemeral key in advance. The problem I see is that the key gets refreshed multiple times during the checkout flow which seems to defeat the purpose of fetching it in advance.

Stripe probably has dozens of payment flows to support so there's probably use cases where the same initCustomerSession generated ephemeral key is used throughout the entire flow. But what exactly is that flow?

My current flow is:

  1. initCustomerSession
  2. retrieveCurrentCustomer
  3. generate source token
  4. addCustomerSource
  5. setCustomerDefaultSource (set just added source as default, would be great to have an option to set newly added source as default to consolidate this step like exists in web)
  6. charge payment in backend

In this flow, the ephemeral key is fetched twice. The PaymentConfiguration has to be initialized twice too. Is the redundancy intentional?

nataliakuznetsova commented 5 years ago

Hello, Stripe. I am experiencing the same issue with version 10.0.3 but in my case EphemeralKeyProvider is called 3 times within SCA 2 flow. Please provide an update. Thank you

mshafrir-stripe commented 5 years ago

@nataliakuznetsova thanks for raising the issue. I'll hopefully investigate it this coming week.

mshafrir-stripe commented 5 years ago

@nataliakuznetsova can you provide me more details around your integration (e.g. which Stripe, PaymentSession, CustomerSession methods are you calling), and at what point do you see EphemeralKeyProvider called?

talhaasim96 commented 5 years ago

@mshafrir-stripe I am experiencing the same issue with version 9.3.6 (also tried latest 10.2.1). In my case, I'm using CustomerSession.initCustomerSession() in onCreate and createEphemeralKey Api hits two times and returns me two customers. I'm not even using getInstance().

mshafrir-stripe commented 5 years ago

@talhaasim96 I just tested CustomerSession.initCustomerSession() with our sample store app, and I only see EphemeralKeyProvider#createEphemeralKey() called once. Can you share some code so that I can repro that method getting called twice?

nataliakuznetsova commented 5 years ago

@mshafrir-stripe I am using paymentSession in 3D Secure flow. I call presentPaymentMethodSelection on the payment session to select a payment methods. First I init CustomerSession with context and emphemeral key provider, then I create and init payment session, and in the end call presentPaymentMethodSelection. As a result I see that the end point is called 3 times. Please let me know if more details are needed

mshafrir-stripe commented 5 years ago

@nataliakuznetsova thanks for the details. To help me debug this issue, can you tell me if you are directly calling any methods on CustomerSession? Would you be able to share the code you're using to init CustomerSession and PaymentSession?

mshafrir-stripe commented 5 years ago

@nataliakuznetsova @talhaasim96 @searchy2

The reason that you're seeing superfluous ephemeral key requests:

  1. Calling CustomerSession#initCustomerSession() makes an initial ephemeral key request by default.
  2. Then when the PaymentSession is initialized, if an ephemeral key is not available, another request is made.

In your case, there is a race condition in which the request for the first key has not returned before we check if an ephemeral key is available in Step 2. The "prefetching" logic in Step 1 is an optimization to have an ephemeral key ready for some point in the near future, but unnecessary in the case where both CustomerSession and PaymentSession are initialized together.

I put up a PR (#1376) that allows you to skip prefetching by setting shouldPrefetchEphemeralKey to false in CustomerSession#initSession().

nataliakuznetsova commented 5 years ago

Hello @mshafrir thanks for your fix. Unfortunately, the end point is still triggered more than once in my case:

As a result the end point is triggered twice now.

mshafrir-stripe commented 5 years ago

@nataliakuznetsova are you are calling PaymentSession#presentPaymentMethodSelection() immediately after initializing the PaymentSession? If that's the case, can you initialize the PaymentSession before you need to present the payment methods screen?

For example, initialize the PaymentSession when the checkout Activity is created, then only call presentPaymentMethodSelection() when the user taps on "Add a payment method"?

nataliakuznetsova commented 5 years ago

@msaffitz-stripe yes, this is how it is done in the check out flow. But besides the check out flow, the app lets user to manage the card via PaymentMethodActivity outside of payment. And in this flow I wouldn't want to initialize the Payment session too early

I am trying now to call presentPaymentMethodSelection when the callback onPaymentSessionDataChanged is invoked. Do you think it is a valid approach @mshafrir-stripe ?

mshafrir-stripe commented 5 years ago

@nataliakuznetsova that makes sense. I put up #1394 to let you customize the prefetch behavior.

nataliakuznetsova commented 5 years ago

@mshafrir-stripe thanks!

mshafrir-stripe commented 5 years ago

@nataliakuznetsova this is now available in 10.3.1

talhaasim96 commented 5 years ago

@mshafrir-stripe

CustomerSession.initCustomerSession(getContext(), new MyEphemeralKeyProvider( new MyEphemeralKeyProvider.ProgressListener() { @Override public void onStringResponse(String string) { Log.e(TAG, string); if (string.startsWith("Error: ")) { ToastUtils.showError(string); } } }));

            PaymentConfiguration.init(Constants.STRIPE_LIVE_KEY);
            paymentSessionConfig = new PaymentSessionConfig.Builder().build();

            paymentSession = new PaymentSession(getActivity());
            paymentSession.init(this, paymentSessionConfig, savedInstanceState);

using above code, my api hits two times. this code is present in onCreate of activity

mshafrir-stripe commented 5 years ago

@talhaasim96 you can use one of the overloaded [CustomerSession#initCustomerSession][https://stripe.dev/stripe-android/com/stripe/android/CustomerSession.html#initCustomerSession-android.content.Context-com.stripe.android.EphemeralKeyProvider-boolean-) methods to specify whether the ephemeral key should be prefetched. In your case, you can disable it.