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

Safer argument parsing #220

Closed
wants to merge 3 commits into from
Closed

Conversation

bobjflong
Copy link
Contributor

An issue I've seen crop up is that if a global key is set (which most of your docs use), and then client API keys are used (eg. if using OAuth), then unexpected results can happen. This includes loading customer data from the parent global key.

> require 'stripe'

> Stripe.api_key = "sk_test_foo"
=> "sk_test_foo"

> Stripe::Customer.all
# => loads customers from the global key

> Stripe::Customer.all({}, { api_key: nil })
# => ALSO loads customers from the global key!!

With my changes:

> Stripe::Customer.all({}, { api_key: nil })
TypeError: api_key must be a String

@@ -2,7 +2,8 @@ module Stripe
module APIOperations
module Create
module ClassMethods
def create(params={}, opts={})
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems unnecessary. Tests pass without it.

@russelldavis
Copy link
Contributor

@bobjflong Thanks for the pull request! I like the extra safety this provides. I left a few comments about changing the approach a bit; other than that I'll be happy to merge this.

@bobjflong
Copy link
Contributor Author

thanks @russelldavis made a bunch of changes based on your feedback

@@ -119,21 +119,35 @@ def self.flatten_params_array(value, calculated_key)
def self.parse_opts(opts)
case opts
when NilClass
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about just getting rid of this when NilClass clause altogether and letting the else clause handle it? (In which case you could also merge raise_bad_api_key into check_bad_api_key, since that would be the only place it's used.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, fixed

@russelldavis
Copy link
Contributor

Github says there are conflicts that need to be resolved -- can you rebase against master and resolve those?

@bobjflong
Copy link
Contributor Author

Huh weird. Will do tomorrow

@bobjflong
Copy link
Contributor Author

Ugh seems like my local files were super out of date. Going to replay my changes in a new pr.

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.

None yet

2 participants