New type of ephemeral keys #1131

acavailhez-stripe · 2019-02-07T00:33:54Z

Summary

Allowing for ephemeral keys that are not customer

Motivation

Stripe Issuing will need those

Testing

Existing ephemeral keys test apply

fred-stripe

Overall a nice refactor to add the new type of ephemeral keys!. I'll defer the rest of review to someone who understands the SDK internals better than I do.

fred-stripe · 2019-02-11T17:35:03Z

Stripe/STPEphemeralKeyManager.m

@@ -64,18 +68,44 @@ - (void)handleWillForegroundNotification {
    // eager refreshses to once per hour.


Minor typo:
// eager refreshes to once per hour.

fred-stripe · 2019-02-11T17:38:26Z

Stripe/STPEphemeralKeyManager.m

+            // the ephemeral key could not be decoded
+            else {
+                [self.createKeyPromise fail:[NSError stp_ephemeralKeyDecodingError]];
+                NSAssert(NO, @"Could not parse the ephemeral key response. Make sure your backend is sending the unmodified JSON of the ephemeral key to your app. For more info, see https://stripe.com/docs/mobile/ios/standard#prepare-your-api");


It might be handy to log the type of key here (Issuing vs Customer), not sure if that's available in this context. My thought is the user will contact us when they see that error, and it'd be nice to know which key provider is causing the issue.

fred-stripe · 2019-02-11T17:42:58Z

Stripe/PublicHeaders/STPCustomerContext.h

@@ -35,7 +35,7 @@ NS_ASSUME_NONNULL_BEGIN
 @param keyProvider   The key provider the customer context will use.
 @return the newly-instantiated customer context.
 */
- (instancetype)initWithKeyProvider:(id<STPEphemeralKeyProvider>)keyProvider;
+- (instancetype)initWithKeyProvider:(id<STPCustomerEphemeralKeyProvider>)keyProvider;


Renaming this class means:

We're also going to have to update the Examples, the stripe.com docs, and the generated docs

We'll need coordinate a release between all three.

Users will have to update their existing code when they upgrade, even if they're not using Issuing.

I don't think any of this is a blocker, just thinking through the user-facing consequences here.

@jack-stripe Just to make sure: is it in line with what you expected?

I believe I safely deprecated STPEphemeralKeyProvider by changing it to subclass STPCustomerEphemeralKeyProvider - this would trigger a deprecation warning in existing integrations but not a compiler error. Yes, this will mean a docs update though.

csabol-stripe · 2019-02-11T17:27:55Z

Stripe/STPEphemeralKey.m

 }

- (BOOL)isEqualToResourceKey:(STPEphemeralKey *)other {
-    return [self.stripeID isEqualToString:other.stripeID] 
-        && [self.secret isEqualToString:other.secret]


why were these removed?

@jack-stripe ?

I renamed it to isEqualToEphemeralKey ("Resource Key" was an early and terrible name for ephemeral keys)

ah sorry, I mean why was it changed so it only compares the stripeIDs?

ping on this @jack-stripe ? (not going to block patch because id equivalence makes sense as sufficient to me but would be useful to know the reasoning

Oh, sorry, missed your comment @csabol-stripe!
I honestly don't remember why I changed that. I don't see a world in which stripeID is the same but secret is different. I'm happy with either implementation.

csabol-stripe · 2019-02-11T17:29:59Z

Stripe/STPEphemeralKeyManager.m

-                self.customerKey = key;
-            }
+        [self getOrCreateKey:^(__unused STPEphemeralKey * _Nullable ephemeralKey, __unused NSError * _Nullable error) {
+


It looks like self.ephemeralKey gets set in the actual to getOrCreateKey. Could we add some comments here explaining why this is called and why we do nothing with the response here?

csabol-stripe · 2019-02-11T17:31:33Z

Stripe/STPEphemeralKeyManager.m

+                [self.createKeyPromise fail:error];
+            }
+            // the ephemeral key could not be decoded
+            else {


style-nit: I prefer to have the else on the same line as the closing brace, } else {

csabol-stripe · 2019-02-11T17:32:06Z

Stripe/STPEphemeralKeyManager.m

+        id<STPCustomerEphemeralKeyProvider> provider = self.keyProvider;
+        [provider createCustomerKeyWithAPIVersion:self.apiVersion completion:jsonCompletion];
+    }
+    else if ([self.keyProvider conformsToProtocol:@protocol(STPIssuingCardEphemeralKeyProvider)]) {


csabol-stripe · 2019-02-11T17:34:52Z

Stripe/STPEphemeralKeyManager.m

        }];
    }
 }

- (void)getCustomerKey:(STPEphemeralKeyCompletionBlock)completion {
+- (void)createKey {


style-nit: (this is a new one I'm trying to enforce so a lot of existing code doesn't follow it). Internal methods should start with an underscore to differentiate from public methods - (void)_createKey

csabol-stripe · 2019-02-11T17:37:30Z

Tests/Tests/STPEphemeralKeyManagerTest.m

@@ -32,12 +31,16 @@ - (void)setUp {
    self.apiVersion = @"2015-03-03";
 }

+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated"


what are we using here that's deprecated?

@jack-stripe ?

My mistake, I added this accidentally in my initial testing, feel free to remove these ignores

csabol-stripe · 2019-02-11T17:37:40Z

Tests/Tests/STPEphemeralKeyManagerTest.m

 - (id)mockKeyProviderWithKeyResponse:(NSDictionary *)keyResponse {
    XCTestExpectation *exp = [self expectationWithDescription:@"createCustomerKey"];
    id mockKeyProvider = OCMProtocolMock(@protocol(STPEphemeralKeyProvider));
    OCMStub([mockKeyProvider createCustomerKeyWithAPIVersion:[OCMArg isEqual:self.apiVersion]
                                                  completion:[OCMArg any]])
    .andDo(^(NSInvocation *invocation) {
+        [invocation retainArguments];


why is this needed?

There is a bug using OCMock that this avoids: erikdoe/ocmock#147

👍 a comment referencing this would be helpfule

Let me add this

csabol-stripe · 2019-02-11T17:39:30Z

Stripe/PublicHeaders/STPEphemeralKeyProvider.h

+ It provides a way for Stripe utility classes to request a new ephemeral key from
+ your backend, which it will use to retrieve and update Stripe API objects.
+ */
+@protocol STPIssuingCardEphemeralKeyProvider <NSObject>


I'm not sure these need to be two separate protocols. The documentation says that the API client should conform to both of these, which means checking for keyProvider protocol conformance will always succeed for both.

It's not totally clear to me how or why we intend to differentiate between creating customer and card ephemeral keys? Is the intention for the integrator to create two instances of STPEphemeralKeyManager and call the right one as needed?

@jack-stripe ?

That's the way I imagined it, yes. You might imagine one API that calls POST https://myapi.com/users/:id/ephemeral_keys and one that calls POST https://myapi.com/cards/:id/ephemeral_keys. Then you might have one API client client-side that calls the first, and another that calls the second.

The other alternative, as you suggest, is to make a single API - POST https://myapi.com/ephemeral_keys (with user or card parameters) and a single API client to talk to both. The issue is that now the method signature needs to change - we need to tell the app what they should be making an ephemeral key for exactly. So the protocol method would have to be something generic like createKey:forAssociatedStripeObject: which just feels like turning a thing to figure out at compile-time into a thing to figure out at runtime.

You raise a good point, though, that this breaks if someone's API client conforms to both protocols. Sadly, I think we'll need to add a property to STPEphemeralKeyManager to indicate what kind of key it is intended to make (not the worst thing, as it's a private class). Thoughts @csabol-stripe ?

hmm, what about adding a new method to STPEphemeralKeyManager so we have getCustomerKey: and getCardKey: (or similar)? That way we only have one key manager but the requests are still "typed" at compile time.

Similarly, what are your thoughts about keeping STPEphemeralKeyProvider and just adding createIssuingCardKeyWithAPIVersion:completion: to the existing protocol (and marking as optional if we think that would be useful?)

I think I'd prefer to keep a property on STPEphemeralKeyManager, because I think it'd be a programmer error to call getCustomerKey: and then later getCardKey: on the same instance. Nice thing is, assuming that property is set in the initializer, we could assert that the given keyProvider conforms to the necessary protocol.

I don't feel as strongly about whether or not to split STPEphemeralKeyProvider into 2 protocols, but realistically users are going to be implementing only one of createCustomerKeyWithAPIVersion:completion or createIssuingCardKeyWithAPIVersion:completion`, so both would have to be optional. In that case, I think it's a little nicer to just have two protocols so that you can enforce at compile-time (instead of runtime) that the necessary methods are implemented.

Thoughts @csabol-stripe? Can also discuss in slack on monday : )

closing the loop: @jack-stripe and I met to discuss this today and we are happy moving forward with the patch as is :)

acavailhez-stripe · 2019-02-20T17:10:36Z

r? @csabol-stripe

csabol-stripe · 2019-02-21T22:38:09Z

Stripe/PublicHeaders/STPEphemeralKeyProvider.h

+ It provides a way for Stripe utility classes to request a new ephemeral key from
+ your backend, which it will use to retrieve and update Stripe API objects.
+ */
+@protocol STPIssuingCardEphemeralKeyProvider <NSObject>


closing the loop: @jack-stripe and I met to discuss this today and we are happy moving forward with the patch as is :)

csabol-stripe · 2019-02-21T22:43:05Z

Stripe/STPEphemeralKey.m

 }

- (BOOL)isEqualToResourceKey:(STPEphemeralKey *)other {
-    return [self.stripeID isEqualToString:other.stripeID] 
-        && [self.secret isEqualToString:other.secret]


ping on this @jack-stripe ? (not going to block patch because id equivalence makes sense as sufficient to me but would be useful to know the reasoning

acavailhez-stripe · 2019-02-22T16:42:10Z

Thanks everyone

* Extract UI logic into a view model * Implement name collection * Add test for `String.isBlank` * Cleanup

acavailhez-stripe and others added 5 commits February 6, 2019 13:40

Jack's changes allowing Ephemeral keys not linked to a customer

067c02f

Fix tests

1e6ae5b

fix test

c506067

Fix more tests

f502724

Added doc

3f54ab1

fred-stripe reviewed Feb 11, 2019

View reviewed changes

csabol-stripe suggested changes Feb 11, 2019

View reviewed changes

stripe-ci assigned acavailhez-stripe Feb 11, 2019

acavailhez-stripe added 7 commits February 11, 2019 10:51

Protocol in error message

dae2f88

Fix style nits

2e7709c

Private method prefixed with _

8c345e1

added comment

3d6773d

Fix types

fc048d2

Additional comments

38e5a16

Better layout for error

69d2bbb

csabol-stripe approved these changes Feb 21, 2019

View reviewed changes

stripe-ci added the approved label Feb 21, 2019

acavailhez-stripe merged commit 3fe5307 into master Feb 22, 2019

ramont-stripe added a commit that referenced this pull request May 23, 2022

[Link] Collect name Part 2: Modal signup (#1131)

49a9a65

* Extract UI logic into a view model * Implement name collection * Add test for `String.isBlank` * Cleanup

		@@ -64,18 +68,44 @@ - (void)handleWillForegroundNotification {
		// eager refreshses to once per hour.

New type of ephemeral keys #1131

New type of ephemeral keys #1131

Conversation

acavailhez-stripe commented Feb 7, 2019

Summary

Motivation

Testing

fred-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fred-stripe Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acavailhez-stripe commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acavailhez-stripe commented Feb 22, 2019

fred-stripe Feb 11, 2019 •

edited

Loading