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.

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

Add support for Cvc and Number on Issuing Card #2003

Merged
merged 1 commit into from
Apr 17, 2020
Merged

Conversation

remi-stripe
Copy link
Contributor

Mirrors part of stripe/stripe-java#1010

r? @cjavilla-stripe
cc @stripe/api-libraries

Copy link
Contributor

@cjavilla-stripe cjavilla-stripe left a comment

Choose a reason for hiding this comment

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

One question, but not blocking.

/// be omitted unless you explicitly expand it in the request.
/// </summary>
[JsonProperty("cvc")]
public string Cvc { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

in the api ref this is expandable, but I'm not sure what it would expand to? do we need to make this expandable 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.

No we don't. Expandable here means "includable" which means the field is not present unless you expand it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize that some scalar fields could be expanded.

@remi-stripe remi-stripe merged commit 4b2cae5 into master Apr 17, 2020
@remi-stripe remi-stripe deleted the remi-issuing-card branch April 17, 2020 00:41
@remi-stripe
Copy link
Contributor Author

@clement911 This feature is brand new and those fields are the first ones we implemented. We use the Expand feature but we call them "includable properties". In the future we will move a lot of sub-lists to that approach so that you only render the costly sub-resources if you really need them.

@clement911
Copy link
Contributor

I see, thanks for the details.
For CVC and card number, I see how it makes sense given that these should be confidential (although a Last4Digits field would be quite useful to identify the card securely).

I do have a bit of feedback (hopefully you're not too tired yet of me doing this on a regular basis)

One thing that worries me a bit with these includable fields is the way this will be handled with respect to events/webhooks. Will these fields be included in events?
If not it could lead to a situation whereby, upon receiving the event via our webhook, we need to call back into Stripe to expand/include the field. This would be very inefficient for both sides.

I could see this becoming a problem if this technique becomes generalized for many fields but I'm sure it's something you've considered.
For sub-lists, I guess it could suffer from the same problem, although if the sub-lists object has its own event, then we could listen for these.... But it's not always the case...
As an example, we can consider application_fee.refunds. If refunds is not included in the application_fee.refunded event it would be a problem given than there is no application_fee.refund.created. This might not be the best example but hopefully makes the point.

Not to get too far down this rabbit hole, but I think it would be useful if, when registering a webhook endpoint, we could specify not only the events we want to listen to, but also the fields that we would like to have expanded.

@remi-stripe
Copy link
Contributor Author

For CVC and card number, I see how it makes sense given that these should be confidential (although a Last4Digits field would be quite useful to identify the card securely).

The card has last4 already and it's always returned: https://stripe.com/docs/api/issuing/cards/object#issuing_card_object-last4

I do have a bit of feedback (hopefully you're not too tired yet of me doing this on a regular basis)

For sure though I think we might be better off keeping one unique github issue for all high level feedback and not the PR itself.

One thing that worries me a bit with these includable fields is the way this will be handled with respect to events/webhooks. Will these fields be included in events?

No they won't be in events. It would be really dangerous if we sent the full card number and CVC in events.

If not it could lead to a situation whereby, upon receiving the event via our webhook, we need to call back into Stripe to expand/include the field. This would be very inefficient for both sides.

I disagree that it would be "very inefficient". There's no reason to need the card number and CVC in the event. This is a sensitive operation that you do on purpose when you need to reveal the card number to your customer usually because they explicitly request it.

I could see this becoming a problem if this technique becomes generalized for many fields but I'm sure it's something you've considered.

Yes we have and put a lot of thought into it and have been discussing this feature for a long time internally.

For sub-lists, I guess it could suffer from the same problem, although if the sub-lists object has its own event, then we could listen for these.... But it's not always the case... As an example, we can consider application_fee.refunds. If refunds is not included in the application_fee.refunded event it would be a problem given than there is no application_fee.refund.created. This might not be the best example but hopefully makes the point.

We're aware of this and we definitely wouldn't remove something that prevents any integration from knowing about data being created.

Not to get too far down this rabbit hole, but I think it would be useful if, when registering a webhook endpoint, we could specify not only the events we want to listen to, but also the fields that we would like to have expanded.

We could as needed in the future. But we won't at first. For now expandable fields apply to 2 brand new properties for a really specific integration path for Issuing integrations! We'll definitely keep an eye on this and introduce any feature needed for integrations to continue using our product properly!

@clement911
Copy link
Contributor

The card has last4 already and it's always returned

Sweet!

For sure though I think we might be better off keeping one unique github issue for all high level feedback and not the PR itself.

OK. Do you want to create that issue and I'll post feedback there instead?

No they won't be in events. It would be really dangerous if we sent the full card number and CVC in events.

Oh sure, for CVC and card number it totally makes sense but I was thinking of includable/expandable fields in general, including ones that are includable/expandable for performance reasons and not security reasons (as you said there will be more of these in the future).

@remi-stripe
Copy link
Contributor Author

Do you want to create that issue and I'll post feedback there instead?

Let's wait for the next one you find and then we can use it as a canonical issue to report that kind of problems/questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants