Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New type of ephemeral keys #1131

Merged
merged 12 commits into from
Feb 22, 2019
Merged

New type of ephemeral keys #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
067c02f
Jack's changes allowing Ephemeral keys not linked to a customer
acavailhez-stripe Feb 6, 2019
1e6ae5b
Fix tests
acavailhez-stripe Feb 6, 2019
c506067
fix test
jack-stripe Feb 7, 2019
f502724
Fix more tests
acavailhez-stripe Feb 8, 2019
3f54ab1
Added doc
acavailhez-stripe Feb 9, 2019
dae2f88
Protocol in error message
acavailhez-stripe Feb 11, 2019
2e7709c
Fix style nits
acavailhez-stripe Feb 11, 2019
8c345e1
Private method prefixed with _
acavailhez-stripe Feb 11, 2019
3d6773d
added comment
acavailhez-stripe Feb 12, 2019
fc048d2
Fix types
acavailhez-stripe Feb 12, 2019
38e5a16
Additional comments
acavailhez-stripe Feb 15, 2019
69d2bbb
Better layout for error
acavailhez-stripe Feb 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Stripe/PublicHeaders/STPCustomerContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

NS_ASSUME_NONNULL_BEGIN

@protocol STPEphemeralKeyProvider;
@protocol STPCustomerEphemeralKeyProvider;
@class STPEphemeralKey, STPEphemeralKeyManager;

/**
Expand All @@ -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;
Copy link

@fred-stripe fred-stripe Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming this class means:

  1. We're also going to have to update the Examples, the stripe.com docs, and the generated docs
  2. We'll need coordinate a release between all three.
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
`STPCustomerContext` will cache its customer object for up to 60 seconds.
Expand Down
48 changes: 42 additions & 6 deletions Stripe/PublicHeaders/STPEphemeralKeyProvider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,20 @@ NS_ASSUME_NONNULL_BEGIN

/**
You should make your application's API client conform to this interface.
It provides a way for `STPCustomerContext` to request a new ephemeral key from
your backend, which it will use to retrieve and update a Stripe customer.
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 STPEphemeralKeyProvider <NSObject>

@protocol STPCustomerEphemeralKeyProvider <NSObject>
/**
Creates a new ephemeral key for retrieving and updating a Stripe customer.
On your backend, you should create a new ephemeral key for the Stripe customer
associated with your user, and return the raw JSON response from the Stripe API.
For an example Ruby implementation of this API, refer to our example backend:
https://github.com/stripe/example-ios-backend/blob/v14.0.0/web.rb

Back in your iOS app, once you have a response from this API, call the provided
completion block with the JSON response, or an error if one occurred.

@param apiVersion The Stripe API version to use when creating a key.
You should pass this parameter to your backend, and use it to set the API version
in your key creation request. Passing this version parameter ensures that the
Expand All @@ -40,7 +39,44 @@ NS_ASSUME_NONNULL_BEGIN
or `completion(nil, error)` if an error is returned.
*/
- (void)createCustomerKeyWithAPIVersion:(NSString *)apiVersion completion:(STPJSONResponseCompletionBlock)completion;
@end

/**
You should make your application's API client conform to this interface.
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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-stripe ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
Creates a new ephemeral key for retrieving and updating a Stripe Issuing Card.
On your backend, you should create a new ephemeral key for your logged-in user's
primary Issuing Card, and return the raw JSON response from the Stripe API.
For an example Ruby implementation of this API, refer to our example backend:
https://github.com/stripe/example-ios-backend/blob/v14.0.0/web.rb

Back in your iOS app, once you have a response from this API, call the provided
completion block with the JSON response, or an error if one occurred.

@param apiVersion The Stripe API version to use when creating a key.
You should pass this parameter to your backend, and use it to set the API version
in your key creation request. Passing this version parameter ensures that the
Stripe SDK can always parse the ephemeral key response from your server.
@param completion Call this callback when you're done fetching a new ephemeral
key from your backend. For example, `completion(json, nil)` (if your call succeeds)
or `completion(nil, error)` if an error is returned.
*/
- (void)createIssuingCardKeyWithAPIVersion:(NSString *)apiVersion completion:(STPJSONResponseCompletionBlock)completion;
@end

/**
You should make your application's API client conform to this interface.
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.
@deprecated use `STPCustomerEphemeralKeyProvider` or `STPIssuingCardEphemeralKeyProvider`
depending on the type of key that will be fetched.
*/
__attribute__ ((deprecated("STPEphemeralKeyProvider has been renamed to STPCustomerEphemeralKeyProvider", "STPCustomerEphemeralKeyProvider")))
@protocol STPEphemeralKeyProvider <STPCustomerEphemeralKeyProvider>
@end

NS_ASSUME_NONNULL_END
4 changes: 4 additions & 0 deletions Stripe/STPAPIClient+Private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ NS_ASSUME_NONNULL_BEGIN

@end

@interface STPAPIClient (EphemeralKeys)
+ (instancetype)apiClientWithEphemeralKey:(STPEphemeralKey *)key;
@end

@interface STPAPIClient (Customers)

/**
Expand Down
15 changes: 8 additions & 7 deletions Stripe/STPCustomerContext.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ @interface STPCustomerContext ()

@implementation STPCustomerContext

- (instancetype)initWithKeyProvider:(nonnull id<STPEphemeralKeyProvider>)keyProvider {
- (instancetype)initWithKeyProvider:(nonnull id<STPCustomerEphemeralKeyProvider>)keyProvider {
STPEphemeralKeyManager *keyManager = [[STPEphemeralKeyManager alloc] initWithKeyProvider:keyProvider
apiVersion:[STPAPIClient apiVersion]];

apiVersion:[STPAPIClient apiVersion] performsEagerFetching:YES];
return [self initWithKeyManager:keyManager];
}

Expand Down Expand Up @@ -75,7 +76,7 @@ - (void)retrieveCustomer:(STPCustomerCompletionBlock)completion {
}
return;
}
[self.keyManager getCustomerKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
[self.keyManager getOrCreateKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
if (retrieveKeyError) {
if (completion) {
stpDispatchToMainThreadIfNecessary(^{
Expand All @@ -99,7 +100,7 @@ - (void)retrieveCustomer:(STPCustomerCompletionBlock)completion {
}

- (void)attachSourceToCustomer:(id<STPSourceProtocol>)source completion:(STPErrorBlock)completion {
[self.keyManager getCustomerKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
[self.keyManager getOrCreateKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
if (retrieveKeyError) {
if (completion) {
stpDispatchToMainThreadIfNecessary(^{
Expand All @@ -123,7 +124,7 @@ - (void)attachSourceToCustomer:(id<STPSourceProtocol>)source completion:(STPErro
}

- (void)selectDefaultCustomerSource:(id<STPSourceProtocol>)source completion:(STPErrorBlock)completion {
[self.keyManager getCustomerKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
[self.keyManager getOrCreateKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
if (retrieveKeyError) {
if (completion) {
stpDispatchToMainThreadIfNecessary(^{
Expand All @@ -149,7 +150,7 @@ - (void)selectDefaultCustomerSource:(id<STPSourceProtocol>)source completion:(ST
}

- (void)updateCustomerWithShippingAddress:(STPAddress *)shipping completion:(STPErrorBlock)completion {
[self.keyManager getCustomerKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
[self.keyManager getOrCreateKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
if (retrieveKeyError) {
if (completion) {
stpDispatchToMainThreadIfNecessary(^{
Expand Down Expand Up @@ -178,7 +179,7 @@ - (void)updateCustomerWithShippingAddress:(STPAddress *)shipping completion:(STP
}

- (void)detachSourceFromCustomer:(id<STPSourceProtocol>)source completion:(STPErrorBlock)completion {
[self.keyManager getCustomerKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
[self.keyManager getOrCreateKey:^(STPEphemeralKey *ephemeralKey, NSError *retrieveKeyError) {
if (retrieveKeyError) {
if (completion) {
stpDispatchToMainThreadIfNecessary(^{
Expand Down
3 changes: 2 additions & 1 deletion Stripe/STPEphemeralKey.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ NS_ASSUME_NONNULL_BEGIN
@property (nonatomic, readonly) BOOL livemode;
@property (nonatomic, readonly) NSString *secret;
@property (nonatomic, readonly) NSDate *expires;
@property (nonatomic, readonly) NSString *customerID;
@property (nonatomic, readonly, nullable) NSString *customerID;
@property (nonatomic, readonly, nullable) NSString *issuingCardID;

/**
You cannot directly instantiate an `STPEphemeralKey`. You should instead use
Expand Down
19 changes: 11 additions & 8 deletions Stripe/STPEphemeralKey.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ @interface STPEphemeralKey ()
@property (nonatomic, readwrite) BOOL livemode;
@property (nonatomic, readwrite) NSString *secret;
@property (nonatomic, readwrite) NSDate *expires;
@property (nonatomic, readwrite) NSString *customerID;
@property (nonatomic, readwrite, nullable) NSString *customerID;
@property (nonatomic, readwrite, nullable) NSString *issuingCardID;
@property (nonatomic, readwrite, nonnull, copy) NSDictionary *allResponseFields;

@end
Expand All @@ -40,19 +41,24 @@ + (instancetype)decodedObjectFromAPIResponse:(NSDictionary *)response {
}

NSString *customerID;
NSString *issuingCardID;
for (id obj in associatedObjects) {
if ([obj isKindOfClass:[NSDictionary class]]) {
NSString *type = [obj stp_stringForKey:@"type"];
if ([type isEqualToString:@"customer"]) {
customerID = [obj stp_stringForKey:@"id"];
}
if ([type isEqualToString:@"issuing.card"]) {
issuingCardID = [obj stp_stringForKey:@"id"];
}
}
}
if (!customerID) {
if (!customerID && !issuingCardID) {
return nil;
}
STPEphemeralKey *key = [self new];
key.customerID = customerID;
key.issuingCardID = issuingCardID;
key.stripeID = stripeId;
key.livemode = [dict stp_boolForKey:@"livemode" or:YES];
key.created = created;
Expand All @@ -73,14 +79,11 @@ - (BOOL)isEqual:(id)object {
if (!object || ![object isKindOfClass:self.class]) {
return NO;
}
return [self isEqualToResourceKey:object];
return [self isEqualToEphemeralKey:object];
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why were these removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-stripe ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&& [self.expires isEqual:other.expires]
&& [self.customerID isEqual:other.customerID];
- (BOOL)isEqualToEphemeralKey:(STPEphemeralKey *)other {
return [self.stripeID isEqualToString:other.stripeID];
}

@end
20 changes: 14 additions & 6 deletions Stripe/STPEphemeralKeyManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,37 @@ typedef void (^STPEphemeralKeyCompletionBlock)(STPEphemeralKey * __nullable ephe

/**
If the current ephemeral key expires in less than this time interval, a call
to `getCustomerKey` will request a new key from the manager's key provider.
to `getOrCreateKey` will request a new key from the manager's key provider.
The maximum allowed value is one hour – higher values will be clamped.
*/
@property (nonatomic, assign) NSTimeInterval expirationInterval;

/**
If this value is YES, the manager will eagerly refresh its key on app foregrounding.
*/
@property (nonatomic, readonly, assign) BOOL performsEagerFetching;

/**
Initializes a new `STPEphemeralKeyManager` with the specified key provider.

@param keyProvider The key provider the manager will use.
@param apiVersion The Stripe API version the manager will use.
@param keyProvider The key provider the manager will use.
@param apiVersion The Stripe API version the manager will use.
@param performsEagerFetching If the manager should eagerly refresh its key on app foregrounding.
@return the newly-initiated `STPEphemeralKeyManager`.
*/
- (instancetype)initWithKeyProvider:(id<STPEphemeralKeyProvider>)keyProvider apiVersion:(NSString *)apiVersion;
- (instancetype)initWithKeyProvider:(id)keyProvider
apiVersion:(NSString *)apiVersion
performsEagerFetching:(BOOL)performsEagerFetching;

/**
If the retriever's stored customer ephemeral key has not expired, it will be
If the retriever's stored ephemeral key has not expired, it will be
returned immediately to the given callback. If the stored key is expiring, a
new key will be requested from the key provider, and returned to the callback.
If the retriever is unable to provide an unexpired key, an error will be returned.

@param completion The callback to be run with the returned key, or an error.
*/
- (void)getCustomerKey:(STPEphemeralKeyCompletionBlock)completion;
- (void)getOrCreateKey:(STPEphemeralKeyCompletionBlock)completion;

@end

Expand Down
Loading