Added OAuth support. #70

blakewest · 2017-03-31T00:05:49Z

This commit lets the user pass an "access_token" to Hubspot.configure. At that point,
if an access token is passed, we set default headers on the Connection class, and this supports OAuth.
I added a check in the get_url method which will not set an API key if an access token is present. This is required. Otherwise, Hubspot will fail the request, thinking that the api key was the intended auth method, but that the key was blank.
I added an OAuth class to do simple creating/refreshing of the OAuth access tokens. For convenience here, I also added several of the params (like client id, and client secret) to the Hubspot.config method.

This has no tests yet. I've tested locally, but mainly looking to get your (@adimichele) thoughts to see if this direction seems acceptable, and if it's worth adding tests to this PR. If so, what level of tests do you think would work?

This change is

- This commit lets the user pass an "access_token" to Hubspot.configure. At that point, if an access token is passed, we set default headers on the Connection class, and this supports OAuth. - I added a check in the get_url method which will not set an API key if an access token is present. This is required. Otherwise, Hubspot will fail the request, thinking that the api key was the intended auth method, but that the key was blank.

- This class is responsbile for grabbing and refreshing OAuth access tokens. - It just returns the results to you for the caller to actually do something with them. - It inherits from the Connection class because it needs to re-use many of the methods for consistency, yet actually has fairly different needs. The headers are form-urlencoded, not json. So using the existing post_json methods would have required some re-jiggering to work correctly here. Also, these dont' require an API key in the same way as the others, so we would have to always pass in "hapikey": false, and this would have required more changes to the post_json method. Lastly, of course you aren't actually posting json here! So using that method would be extra weird.

paulodiniz · 2017-04-03T19:12:11Z

Thanks for contributing ❤️

I've been away from this project so much I feel a little bit lost, so sorry for any silly questions.

My main concern here is that this PR has 0 specs, is the code too hard to test?

paulodiniz · 2017-04-03T19:11:12Z

lib/hubspot/config.rb

+        @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?


Is this a binary operator?

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

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

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.

@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

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

blakewest · 2017-04-03T21:57:12Z

@paulodiniz yeah, you're right, it doesn't have any specs. As I mentioned in my opening PR comment, I wanted to get your early thoughts before writing specs. Like, just make sure that the direction looked good. If you thought the code was generally fine, then I would write some specs.

Also, another thing I realized as I got more into this is that anyone who's using this gem in production would really need some kind of getter/setter methods for the OAuth key. Because the key needs to be periodically refreshed, every single request from it needs to ensure it has the latest OAuth access token.
This means either that the user of the gem needs to have some wrapper component that spins up a new class for every request, and hands the latest key to this gem, or that the gem itself has a way to get the latest key. Personally, I think the latter is superior. I was thinking of allowing the user to pass in a block that each request would then use to get the latest. Something like...

Hubspot.configure(
  access_token_getter: -> { REDIS.get("HUBSPOT_ACCESS_TOKEN") }
)

# And then inside the connection class. Something like...

class Connection
  def get
    ensure_latest_access_token!
    # All the normal get method stuff
  end

  def ensure_latest_access_token!
    Hubspot::Config.access_token = Hubspot::Config.access_token_getter
  end
end

Sound good?

dan987 · 2017-04-27T20:01:03Z

This is not a feature that we use.... does @paulodiniz or anyone have opinions on @blakewest comments?... sorry for the nudge, i think it would be useful for many to get a version of this feature into production.

blakewest · 2017-05-01T02:26:02Z

@tmac @patrickdavey and any feedback on the getter/setter idea mentioned above? I believe it would be pretty easy to implement. If you're cool with it, I can put it in, and try to write some specs, to see if we can get this merged. But I just want to make sure that approach seems mergeable to you before I do the rest of the work.

patrickdavey · 2017-05-01T02:39:34Z

I am no longer using Hubspot @blakewest , but the approach looks nice to me. I haven't looked at the code, but, will this have backwardly incompatible changes?

blakewest · 2017-05-01T04:54:47Z

@patrickdavey No, I don't believe it would.

b4syth · 2018-01-23T19:35:22Z

Any progress on this? It looks like a good approach and OAuth is required for timelines.

cbisnett · 2018-01-23T20:07:39Z

I’ve been working my way through the outstanding pull requests but I haven’t got to this one yet. I’m out of town this week but I’ll take a look when I get back next week to see what the code is like and if we can get some tests in place to get this merged.

bpirtle-nova · 2018-01-26T20:47:15Z

+1 We just implemented this and it is working well so far as a drop-in addition for a Hubspot Oauth2 integration. Had to use the version on HintHealth for now though since this isn't approved yet.

cbisnett

Other than a few simple changes, this looks like a good first implementation and it looks like several people are already using the changes from the hinthealth fork.

While it would be nice to have a few tests to verify basic functionality like the ability to get a token and refresh it once it expires, I don't think we need to hold off on merging this just because it doesn't have tests or documentation. Let's get this merged and then we can add some simple documentation and can then refactor for a more robust integration with some of the work for v1.0.0.

@blakewest if you are busy or no longer working on this, let me know and I'll make the changes and merge it. Thanks again for contributing.

cbisnett · 2018-02-01T15:29:30Z

lib/hubspot/config.rb

+        @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?


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

cbisnett · 2018-02-01T17:42:32Z

lib/hubspot/config.rb

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


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

@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.

cbisnett · 2018-02-01T19:29:18Z

lib/hubspot/oauth.rb

+        no_parse ? response : response.parsed_response
+      end
+
+      def create(params)


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?

@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

blakewest · 2018-02-04T17:14:27Z

@cbisnett hey, yeah I'm kinda busy at the moment. If you can finish those changes, that would be great. I'd love to see this get merged! I still think my suggestion from earlier is valid. Could be a V2 addition. But for really making this useful, it would be nice.

cbisnett · 2018-10-17T20:15:47Z

I updated the interface a little to reduce the duplicated code and make the required parameters for creating and refreshing tokens more obvious.

blakewest mentioned this pull request Mar 31, 2017

No OAuth? #39

Closed

dan987 requested review from paulodiniz and adimichele April 1, 2017 01:17

paulodiniz reviewed Apr 3, 2017

View reviewed changes

cbisnett requested changes Feb 1, 2018

View reviewed changes

cbisnett merged commit 15d8c78 into HubspotCommunity:master Oct 17, 2018

cbisnett mentioned this pull request Oct 18, 2018

Add support for access_token and oauth_token #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OAuth support. #70

Added OAuth support. #70

blakewest commented Mar 31, 2017 •

edited

Loading

paulodiniz commented Apr 3, 2017

paulodiniz Apr 3, 2017

blakewest Apr 3, 2017

tmac Apr 24, 2017

tmac Apr 27, 2017

blakewest May 1, 2017

cbisnett Feb 1, 2018

blakewest commented Apr 3, 2017 •

edited

Loading

dan987 commented Apr 27, 2017 •

edited

Loading

blakewest commented May 1, 2017

patrickdavey commented May 1, 2017

blakewest commented May 1, 2017

b4syth commented Jan 23, 2018

cbisnett commented Jan 23, 2018

bpirtle-nova commented Jan 26, 2018 •

edited

Loading

cbisnett left a comment

cbisnett Feb 1, 2018

cbisnett Feb 1, 2018

blakewest Feb 4, 2018

cbisnett Feb 1, 2018

blakewest Feb 4, 2018 •

edited

Loading

blakewest commented Feb 4, 2018

cbisnett commented Oct 17, 2018

Added OAuth support. #70

Added OAuth support. #70

Conversation

blakewest commented Mar 31, 2017 • edited Loading

paulodiniz commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakewest commented Apr 3, 2017 • edited Loading

dan987 commented Apr 27, 2017 • edited Loading

blakewest commented May 1, 2017

patrickdavey commented May 1, 2017

blakewest commented May 1, 2017

b4syth commented Jan 23, 2018

cbisnett commented Jan 23, 2018

bpirtle-nova commented Jan 26, 2018 • edited Loading

cbisnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakewest Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

blakewest commented Feb 4, 2018

cbisnett commented Oct 17, 2018

blakewest commented Mar 31, 2017 •

edited

Loading

blakewest commented Apr 3, 2017 •

edited

Loading

dan987 commented Apr 27, 2017 •

edited

Loading

bpirtle-nova commented Jan 26, 2018 •

edited

Loading

blakewest Feb 4, 2018 •

edited

Loading