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

Support Strong Customer Authentication using Stripe with PaymentIntent #182

Merged
merged 12 commits into from
Sep 10, 2019

Conversation

mraerino
Copy link
Member

@mraerino mraerino commented Sep 6, 2019

Fixes #179

- Summary

New payments flow

  • Client: Create a payment method and pass that to POST /orders/:order_id/payments
  • Server: Create a PaymentIntent with the payment method from the client - report back if the payment needs more actions (including the client secret for the payment intent)
  • Client: Completes additional action and calls POST /payments/:payment_id/confirm
  • Server: Responds with the result of checking if the transaction is completed, updates payment status on order

API docs for updated/new endpoints

POST /orders/:order_id/payments (updated)

Payload:

{
  "provider": "stripe",
  "stripe_payment_method_id": "<the id>"
}

Response: Transaction object
(state if 3D secure is pending)

{
  "id": "...", // transaction id
  "order_id": "...",
  "invoice_number": "...",
  "processor_id": "...",
  "user_id": "...",
  "amount": "...",
  "currency": "...",
  "status": "pending",
  "type": "...",
  "created_at": "...",
  "provider_metadata": {
    "payment_intent_secret": "<client secret>"
  }
}

POST /payments/:payment_id/confirm (new)

(payment id from previous response)

Payload: none

Response:

{
  "id": "...",
  "order_id": "...",
  "invoice_number": "...",
  "processor_id": "...",
  "user_id": "...",
  "amount": "...",
  "currency": "...",
  "status": "paid",
  "type": "...",
  "created_at": "..."
}

- Test plan

TBD

- Description for the changelog

Support Strong Customer Authentication using Stripe with PaymentIntent

- A picture of a cute animal (not mandatory but encouraged)

Enable via payment.stripe.use_payment_intents
api/payments.go Outdated Show resolved Hide resolved
api/payments.go Outdated Show resolved Hide resolved
@mraerino mraerino requested a review from rybit September 9, 2019 10:24
api/payments.go Outdated Show resolved Hide resolved

if intent.Status == stripe.PaymentIntentStatusRequiresAction {
return intent.ID, payments.NewPaymentPendingError(map[string]interface{}{
"payment_intent_secret": intent.ClientSecret,
Copy link
Member

Choose a reason for hiding this comment

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

So I'd cut this. I think that anything _secret makes me nervous. Like is it going to leak to the customer? logs? is that ok?

If you cut this then I think that you can also simplify the PaymentPendingError b/c this is the only place I saw the metadata field used.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually required behaviour for the new Stripe flow to work.
This "secret" is supposed to be passed to the client. It is not being saved to the database with the transaction because I disabled that field using sql:"-"

I'm using the metadata field so the payment provider impl stays generic.

See:

Copy link
Member

@rybit rybit left a comment

Choose a reason for hiding this comment

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

small things but generally I like it.

@mraerino mraerino merged commit 23b821f into master Sep 10, 2019
@mraerino mraerino deleted the feature/stripe-2step branch September 10, 2019 08:49
@mraerino mraerino mentioned this pull request Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Strong Customer Authentication using stripe
2 participants