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

refactor: source Stripe API keys from ENV vars #5771

Merged

Conversation

willopez
Copy link
Member

@willopez willopez commented Oct 30, 2019

Resolves #5767
Impact: minor
Type: refactor

Issue

The Stripe API key needs to be sourced from environment variables for security considerations.

Solution

Source the secret Stripe API key from environment variables. Some settings were removed as they are no longer needed.
Removed:

  • publishable_key: only used by client to create Stripe tokens. details
  • client_id - used by client

Why

  • In order to move towards PCI compliance
  • Secrets should be stored in a secured vault, moving to ENV vars facilitates using a secrets provider
  • Remove dependency on Packages collection as it's not needed and we are working on removing it

@willopez willopez requested review from aldeed and mikemurray October 30, 2019 22:48
@willopez
Copy link
Member Author

Note: integration tests pass locally, they fail in Circle CI due to a known issue with in-memory Mongo, that should hopefully be fixed soon.

@willopez
Copy link
Member Author

willopez commented Nov 4, 2019

@mikemurray @aldeed I have verified that I amble to checkout on this branch, so we can move forward and merge.

Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

Seem to work, but I had one comment. If I don't have a STRIPE_API_KEY in the .env file the app fails to start with an error form envalid.

Not sure what we want to do here, but if you don't have a stripe key, or don't intend to use stripe, the app shouldn't fail to start. Also might need some docs or put an example in .env.example. Of course in the future, this plugin would be split out of the API into its own package so whatever we do now is probably temporary.

@willopez
Copy link
Member Author

willopez commented Nov 6, 2019

@mikemurray good point, I have added a placeholder key to the example.env that will prevent the app from crashing.

Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

One suggestion @willopez

@willopez
Copy link
Member Author

willopez commented Nov 6, 2019

@mikemurray @aldeed Removed unnecessary code and re-tested with storefront and can confirm checkout with Stripe works.

Signed-off-by: Will Lopez <[email protected]>
@willopez willopez force-pushed the refactor-5767-stripe-api-keys-from-env-vars branch from f7b5413 to 9c9fb8f Compare November 6, 2019 23:51
@willopez willopez requested a review from aldeed November 6, 2019 23:58
Signed-off-by: Will Lopez <[email protected]>
Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

👍

@mikemurray mikemurray merged commit 6732f07 into release-3.0.0 Nov 7, 2019
@mikemurray mikemurray deleted the refactor-5767-stripe-api-keys-from-env-vars branch November 7, 2019 19:30
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.

3 participants