Fixed multiple issues with configuration of Legacy API

[kirtithorat]

I would like to submit the following changes/ fixes in this pull request. Please review and merge the pull request as soon as possible. If you have any questions and concerns about these changes then let me know by replying on this thread.

1. Error: no implicit conversion of OpenSSL::X509::Certificate into String

   The bigcommerce-api-ruby gem already takes care of creating appropriate OpenSSL::X509::Certificate and OpenSSL::PKey::RSA instances for keys ssl_client_cert and ssl_client_key respectively in lib/bigcommerce/connection.rb. It should not be provided again while configuring Legacy API client as it results in above mentioned error.

   Fixes:
   Updated README.md to reflect the correct usage:

     require 'bigcommerce'
   
     api = Bigcommerce::Api.new({
       :store_url => "https://store.mybigcommerce.com",
       :username  => "username",
       :api_key   => "api_key",
       :ssl_client_cert  =>  "/path/to/cert.pem",                ## <<-- Only path should be specified
       :ssl_client_key   =>  { path: "/path/to/key.pem", passphrase: "passphrase, if any" },  ## <<-- path and passphrase should be specified
       :ssl_ca_file      =>  "/path/to/ca_certificate.pem",
       :verify_ssl       =>  OpenSSL::SSL::VERIFY_PEER
   })
   

   Also, updated ssl_client_key= method in lib/bigcommerce/connection.rb so that it accepts an options hash with path and passphrase.

2. Errors: undefined methods client_cert, ssl_client_key, ssl_ca_file and verify_ssl
   Fixes:
   In lib/bigcommerce/connection.rb, methods ssl_ca_file=, ssl_client_key= and ssl_client_cert= try to set attributes on @configuration which is a Hash. This results in undefined methods error in all the three cases. Also, the key name for client certificate is used as ssl_client_cert throughout the gem code but in lib/bigcommerce/connection.rb it is used as @configuration.client_cert , fixed the code to use @connection[:ssl_client_cert] instead . Modified code to set the correct key-value pair in @configuration Hash is as below:

   def ssl_ca_file=(path)
     @configuration[:ssl_ca_file] = path   ## <<-- Used @configuration[:ssl_ca_file]  instead of @configuration.ssl_ca_file
   end
   
   def ssl_client_key=(options)
     if options[:passphrase].nil?
       @configuration[:ssl_client_key] = OpenSSL::PKey::RSA.new(File.read(options[:path]))  ## <<-- Used @configuration[:ssl_client_key]  instead of @configuration.ssl_client_key
     else
       @configuration[:ssl_client_key] = OpenSSL::PKey::RSA.new(File.read(options[:path]), options[:passphrase])  ## <<-- Used @configuration[:ssl_client_key]  instead of @configuration.ssl_client_key
     end
   end
   
   def ssl_client_cert=(path)
     @configuration[:ssl_client_cert] = OpenSSL::X509::Certificate.new(File.read(path)) ## <<-- Used @configuration[:ssl_client_cert]  instead of @configuration.client_cert
   end
   
   

   Also, while configuring Legacy API with SSL, expected method name is verify_ssl= . Corrected the method name from verify_peer= to verify_ssl= in lib/bigcommerce/connection.rb

3. Error:RestClient uses :user as a valid key not :username
   Fixes:
   Because of this issue the connection was not getting established as :user key was not found. Updated lib/bigcommerce/connection.rb to set :user key instead of :username

    if @configuration[:ssl_client_key] && @configuration[:ssl_client_cert] && @configuration[:ssl_ca_file]
       rest_client = RestClient::Resource.new(
           "#{@configuration[:store_url]}/api/v2#{path}.json",
           :user => @configuration[:username],                        ## <<-- Used :user key  instead of :username
           :password => @configuration[:api_key],
           :ssl_client_cert => @configuration[:ssl_client_cert],
           :ssl_client_key => @configuration[:ssl_client_key],
           :ssl_ca_file => @configuration[:ssl_ca_file],
           :verify_ssl => @configuration[:verify_ssl]
       )
     end
   

4. **Error:**Removed default enabled SSL for Legacy API

   Because of this issue, when we try to configure and connect via Legacy API(without SSL fields), we get failure due to SSL.
   Fixes:
   RestClient by default enables SSL(when :verify_ssl key is not supplied), which was causing Legacy API to default to the same with or without SSL specifications in its API configuration. Updated lib/bigcommerce/connection.rb so that Legacy API does not default to SSL enabled form.

      resource_options = {
         :user => @configuration[:username],
         :password => @configuration[:api_key],
         :verify_ssl => OpenSSL::SSL::VERIFY_NONE,    ## <<--  Added this so that SSL is disabled when SSL fields are not supplied while configuring Legacy API
         :headers => headers
     }
   

[coveralls]

Coverage remained the same at 61.76% when pulling 394b481 on kirtithorat:master into 095245a on bigcommerce:master.

[coveralls]

Coverage remained the same at 61.76% when pulling 394b481 on kirtithorat:master into 095245a on bigcommerce:master.

[coveralls]

Coverage remained the same at 61.76% when pulling 394b481 on kirtithorat:master into 095245a on bigcommerce:master.

[pedelman]

Thanks for the PR, can you explain a bit more about your use case? Are you using a self-signed certificate? I do agree the readme needs to be updated, but I am able to hit the api using legacy credentials without any changes.

[gregory]

ping @kirtithorat

[kirtithorat]

@pedelman Thank you for your response. I am using a self-signed certificate.

I tried two use cases, one connecting Legacy API without SSL enabled and other with SSL enabled.

In the first case, Legacy API without SSL, I am expecting to connect to BigCommerce without SSL being verified. Configuration looks like:

    api = Bigcommerce::Api.new({:store_url => Rails.application.secrets.bc_store_url,
                            :username  => Rails.application.secrets.bc_username,
                            :api_key   => Rails.application.secrets.bc_api_key})

I started getting the error RestClient::SSLCertificateNotVerified - SSL_connect returned=1 errno=0 state=SSLv3 read server certificate B: certificate verify failed which made me question two things, first Why SSL is getting verified when I didn't specify it and second certificate are probably out of date on my system. Important point to note here is: If my certificate was not out of date then I would have never found that SSL was getting verified each time I tried to connect via Legacy API(irrespective of specification in the api configuration). Only after making the change suggested in Point 4 (Removed default enabled SSL for Legacy API), I started getting a response for my api call.

In the second case, Legacy API with enabled SSL, I got the error no implicit conversion of OpenSSL::X509::Certificate into String mentioned in Point 1 in my Pull request. Once I corrected that by fixing the configuration, I started getting the next errors as described in point 2 and 3 due to incorrectly configured methods in lib/bigcommerce/connection.rb

Please let me know if you have any other questions regarding the PR.

[pedelman]

@kirtithorat

1. Can you open this PR into the 0.x branch, we will be maintaining that version separately. We support this in master currently (See: Faraday SSL as an example), sorry for any confusion.
2. I think its a good idea to keep verification by default, since it is a more secure default configuration. If you need to explicitly disable verification for your specific use case, then we should make a change to add support to turn off verification.

[kirtithorat]

@pedelman

1. Not a problem. I have opened a new PR Fixed multiple issues with configuration of Legacy API on 0.x branch #96 into 0.x branch for these changes. Please have a look at that.
2. I completely agree that having SSL enabled makes your web application more secure.
   But reading about the Legacy API configuration in the README gives the gem users an incorrect idea about the actual code implementation. It makes us believe that SSL is only enabled when all the 4 SSL fields :ssl_client_cert, :ssl_client_key, :ssl_ca_file and :verify_ssl are specified in the API configuration.
   Also, we use Legacy API without enabling SSL only in development environment not in production (This is more to do with the common certificate related error which sometimes get really tedious to resolve in development environment) . I would say that providing support for the same would immensely help.

[pedelman]

Im going to close this as we have a good discussion in #96