Skip to content

Ensure credentials set in Twitter.configure are of a valid type. #338

Merged
merged 2 commits into from Dec 30, 2012

2 participants

@DanKnox
DanKnox commented Dec 30, 2012

This pull request adds basic credential validation when the credentials are set using the Twitter.configure block. If the credentials are not a string or symbol, a Twitter::Error::ConfigurationError is raised with a helpful message.

Whether or not this is worthy of being merged in is debatable, however it would have saved @saiko-chriskun $10 today. He had opened issue #336 and posted to the mailing list. Unfortunately, no one had noticed the syntax error in his code snippet before he had opened a bounty on Bountify.

Twitter.configure do |config|
  config.consumer_key = ENV['TWITTER_KEY'],   # Whoops a comma 
  config.consumer_secret = ENV['TWITTER_SECRET']
end

After this block was evaluated, the config.consumer_secret was set correctly, but the config.consumer_key received the result of that assignment along with the intended ENV['TWITTER_KEY'] packaged up as a nice neat array.

 => #<Twitter::Client:0x007fbe6bb8f188 @consumer_key=["twitter_key","twitter_secret"], @consumer_secret="twitter_secret", # snipped

If my PR is accepted, typos like this will be greeted with a helpful configuration error.

@DanKnox DanKnox Ensure credentials set in Twitter.configure are of a valid type.
This commit adds basic credential validation after the credentials
are set using the Twitter.configure block. If the credentials are
not a string or symbol, a Twitter::Error::ConfigurationError is
raised with a helpful message.
6349fa7
@sferik sferik commented on an outdated diff Dec 30, 2012
lib/twitter/configurable.rb
@@ -61,5 +67,20 @@ def options
Hash[Twitter::Configurable.keys.map{|key| [key, instance_variable_get(:"@#{key}")]}]
end
+ # Ensures that all credentials set during configuration are of a
+ # valid type. Valid types are String and Symbol.
+ #
+ # @raise [Twitter::Error::ConfigurationError] Error is raised when
+ # supplied twitter credentials are not a String or Symbol.
+ def validate_credential_type!
+ credentials.each do |credential, value|
+ next if value.nil?
+
+ unless value.is_a?(String) || value.is_a?(Symbol)
+ raise(Error::ConfigurationError, "Invalid #{credential} specified: #{value} must be a string.")
@sferik
Owner
sferik added a note Dec 30, 2012

The condition allows symbols but the error says it "must be a string". I think these should be made to match, one way or the other. There's a bigger issue, which is that the identiy_map configuration accepts boolean or a class as its value and middleware accepts a Faraday::Builder object. This "validation" would prevent their configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sferik
Owner
sferik commented Dec 30, 2012

Philosophically, I have no problem with this pull request but you need to fix the implementation to allow identity_map and middleware configuration. You could simply check whether the value is a hash, but that could come around to bite us if future configuration options accept a hash. I guess this is why some people like type-checked languages?

Anyway, thanks for the effort. Let me know if you come up with something that works.

@DanKnox
DanKnox commented Dec 30, 2012

I just updated the error message to state that symbols are indeed allowed.

As for the identity map and middleware values, I believe they will be unaffected by this change. The #validate_credential_type! method only checks the hash returned by #credentials instead of all of the available keys in the Configurable module. I figured the credentials hash was unlikely to ever contain anything other than string or symbol values.

If I missed something here I can correct the code accordingly.

Thanks for considering the pull request.

@DanKnox
DanKnox commented Dec 30, 2012

One thing I didn't consider however was that checking only the credentials hash still leaves room for the same mistake if the other options are specified.

On the other hand, someone that is specifying custom middleware options is probably unlikely to make that same mistake.

Let me know if you would prefer the validation to check the identity map and middleware options and confirm that they are of the correct type as well.

@sferik
Owner
sferik commented Dec 30, 2012

Okay, this look good now. Thanks for the pull request.

@sferik sferik merged commit be08002 into sferik:master Dec 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.