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.

Sign up for GitHub

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

Jump to bottom

Added OAuth support. #70

Merged
merged 2 commits into from
Oct 17, 2018
Merged

Added OAuth support. #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f18def
Added OAuth support.
blakewest Mar 31, 2017
15d8c78
Added an OAuth class for refreshing/creating.
blakewest Mar 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/hubspot-ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
require 'hubspot/deal_properties'
require 'hubspot/owner'
require 'hubspot/engagement'
require 'hubspot/oauth'

module Hubspot
def self.configure(config={})
Expand Down
21 changes: 19 additions & 2 deletions lib/hubspot/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
require 'logger'
require 'hubspot/connection'

module Hubspot
class Config

CONFIG_KEYS = [:hapikey, :base_url, :portal_id, :logger]
CONFIG_KEYS = [
:hapikey, :base_url, :portal_id, :logger, :access_token, :client_id,
:client_secret, :redirect_uri
]
DEFAULT_LOGGER = Logger.new('/dev/null')

class << self
Expand All @@ -14,7 +18,18 @@ def configure(config)
@hapikey = config["hapikey"]
@base_url = config["base_url"] || "https://api.hubapi.com"
@portal_id = config["portal_id"]
@logger = config['logger'] || DEFAULT_LOGGER
@logger = config["logger"] || DEFAULT_LOGGER
@access_token = config["access_token"]
@client_id = config["client_id"] if config["client_id"].present?
@client_secret = config["client_secret"] if config["client_secret"].present?
@redirect_uri = config["redirect_uri"] if config["redirect_uri"].present?

unless access_token.present? ^ hapikey.present?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a binary operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulodiniz yeah that's ruby's XOR operator. Meaning, "Exactly one of these must be present" (not both, and not none).

Copy link

Choose a reason for hiding this comment

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

I don't think you want to #present? as it's a rails specific method? Also, I think the logical OR (||) is more appropriate. This should do it:

unless access_token || hapikey

Copy link

Choose a reason for hiding this comment

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

Sorry I jumped the gun on this a bit before looking through the code more, my bad. I didn't realize that activesupport was included. The bitwise comparison still smells funny though and at the least the docs and/or error message should be made a bit clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmac yeah I'm happy to switch that to not use the XOR operator. But I think logically, the semantics of XOR is correct. If you have both access_token and hapikey, then calls will not work as expected. You must have exactly one of them, but not both, and not neither. So I could switch it to maybe...

if (access_token && hapikey) || !(access_token || hapikey)
 fail "You must provide..." 
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine in it's current version and is succinct without being obtuse. The ^ operator works on binary values and booleans are a form of binary value:

irb(main):001:0> true ^ true
=> false
irb(main):002:0> true ^ false
=> true
irb(main):003:0> false ^ false
=> false

Maybe update this to be @access_token and @hapikey

Hubspot::ConfigurationError.new("You must provide either an access_token or an hapikey")
end
if access_token.present?
Hubspot::Connection.headers("Authorization" => "Bearer #{access_token}")
end
self
end

Expand All @@ -23,6 +38,8 @@ def reset!
@base_url = "https://api.hubapi.com"
@portal_id = nil
@logger = DEFAULT_LOGGER
@access_token = nil
Hubspot::Connection.headers({})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also reset @client_id, @client_secret, and @redirect_uri?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbisnett I would say no, because those things (client_id, client_secret, and redirect_uri) never change over time. Whereas the access_token must be constantly refreshed. So resetting those other vars means they'd just have to get set back to the same value again, which seems unnecessary.

end

def ensure!(*params)
Expand Down
6 changes: 5 additions & 1 deletion lib/hubspot/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ def log_request_and_response(uri, response, body=nil)
end

def generate_url(path, params={}, options={})
Hubspot::Config.ensure! :hapikey
if Hubspot::Config.access_token.present?
options[:hapikey] = false
else
Hubspot::Config.ensure! :hapikey
end
path = path.clone
params = params.clone
base_url = options[:base_url] || Hubspot::Config.base_url
Expand Down
46 changes: 46 additions & 0 deletions lib/hubspot/oauth.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
require 'httparty'
module Hubspot
class OAuth < Connection
include HTTParty
DEFAULT_OAUTH_HEADERS = {"Content-Type" => "application/x-www-form-urlencoded;charset=utf-8"}
class << self
def refresh(params)
params.stringify_keys!
no_parse = params.delete("no_parse") { false }
body = {
client_id: params["client_id"] || Hubspot::Config.client_id,
grant_type: "refresh_token",
client_secret: params["client_secret"] || Hubspot::Config.client_secret,
redirect_uri: params["redirect_uri"] || Hubspot::Config.redirect_uri,
refresh_token: params["refresh_token"]
}
response = post(oauth_url, body: body, headers: DEFAULT_OAUTH_HEADERS)
log_request_and_response oauth_url, response, body
raise(Hubspot::RequestError.new(response)) unless response.success?

no_parse ? response : response.parsed_response
end

def create(params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the implementations of create() and refresh() the same here? If so, can these be moved to a single function so there aren't two separate implementations?

Copy link
Contributor Author

@blakewest blakewest Feb 4, 2018 •
edited

Choose a reason for hiding this comment

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

@cbisnett they aren't quite the same. Notice the grant_type param is different ("refresh_token" vs. "authorization_code"), and also the last param is different. It's refresh_token in the "refresh" method, but it's code in the "create" method.
But yeah they're pretty similar. It looks like they're the same, except for the body params. If you wanted to refactor out the common components, that would probably be good. You could maybe do something like...

def make_oauth_call(body)
  params.stringify_keys!
  no_parse = params.delete("no_parse") { false }
  response = post(oauth_url, body: body, headers: DEFAULT_OAUTH_HEADERS)
  log_request_and_response oauth_url, response, body
  raise(Hubspot::RequestError.new(response)) unless response.success?
  no_parse ? response : response.parsed_response
end

def refresh(params)
  body = {
    client_id: params["client_id"] || Hubspot::Config.client_id,
    grant_type: "refresh_token",
    client_secret: params["client_secret"] || Hubspot::Config.client_secret,
    redirect_uri: params["redirect_uri"] || Hubspot::Config.redirect_uri,
    refresh_token: params["refresh_token"]
  }
  make_oauth_call(body)
end

def create(params)
  body = {
    client_id: params["client_id"] || Hubspot::Config.client_id,
    grant_type: "authorization_code",
    client_secret: params["client_secret"] || Hubspot::Config.client_secret,
    redirect_uri: params["redirect_uri"] || Hubspot::Config.redirect_uri,
    code: params["code"]
  }
  make_oauth_call(body)
end

params.stringify_keys!
no_parse = params.delete("no_parse") { false }
body = {
client_id: params["client_id"] || Hubspot::Config.client_id,
grant_type: "authorization_code",
client_secret: params["client_secret"] || Hubspot::Config.client_secret,
redirect_uri: params["redirect_uri"] || Hubspot::Config.redirect_uri,
code: params["code"]
}
response = post(oauth_url, body: body, headers: DEFAULT_OAUTH_HEADERS)
log_request_and_response oauth_url, response, body
raise(Hubspot::RequestError.new(response)) unless response.success?

no_parse ? response : response.parsed_response
end

def oauth_url
oauth_url = Hubspot::Config.base_url + "/oauth/v1/token"
end
end
end
end