OAuth 2 support (merge conflicts with master resolved)

[yukideluxe]

Hello!

We are using quickbooks-ruby and we are about to release OAuth v2 support! I took the freedom to merge the latest master with 389-oauth2 and I'm creating this PR just in case it helps you to push the OAuth2 support forward!

Feel free to close it if you want to merge it yourself @ruckus! Thanks for all your work on this <3

[ruckus]

Thanks! I think I just need to review the README and update it for OAuth2 stuff and then I say we can merge in this PR.

Do you have any README notes that you think might be good to include? From what I can remember it should just be:

• New Authentication setup
• Token Refreshing

[pjg]

Token Refreshing

This should probably mention both access_token and refresh_token refresh as those two are distinct. This info is already in the README, but perhaps could be expanded a little bit more (and mention the 100 days expiration time of refresh_token).

And, fwiw, I can confirm that this branch is working nicely with QuickBooks OAuth v2 app.

[yukideluxe]

Thanks for the feedback!

And, fwiw, I can confirm that this branch is working nicely with QuickBooks OAuth v2 app.

@pjg yes! It's already working in our production servers! 😬

This should probably mention both access_token and refresh_token refresh as those two are distinct. This info is already in the README, but perhaps could be expanded a little bit more (and mention the 100 days expiration time of refresh_token).

@pjg There's already a link (that I just updated) to the official documentation where people can read more about how the token expiration works. I'd say that's enough! What do you think? https://github.com/harvesthq/quickbooks-ruby/blame/with-oauth2-support/README.md#L140

@ruckus I went through the documentation and fixed some stuff. I took the chance to put some code example to revoke tokens with the gem!

[pjg]

I would perhaps add one more line with code to the README in the section about refreshing the token:

quickbooks_credentials.update(
  access_token: new_access_token.token,
  refresh_token: new_access_token.refresh_token
)

[pjg]

I'm also not quite sure whether not rescuing the OAuth2::Error in the gem is the proper way to go. In other words, using a library, I would rather expect it to raise its own exceptions, instead of bubbling up exceptions from 3rd party libs (oauth2 in this case).

In the previous version you'd have Quickbooks::AuthorizationFailure raised for invalid credentials errors. I think that rescuing OAuth2::Error and raising Quickbooks::AuthorizationFailure instead would be much better in terms of preserving the compatibility. And in general, would be better behaviour for a lib.

Edit: another argument is that stubbing OAuth2::Error in tests is very cumbersome as this exception expects OAuth2::Response object to be passed into it when raising:

expect(<some service object>).to receive(:new).and_raise OAuth2::Error, 'error'

=> expected OAuth2::Error, got #<NoMethodError: undefined method `error=' for "error":String> with backtrace:
./.bundle/gems/oauth2-1.4.1/lib/oauth2/error.rb:8:in `initialize'

[ruckus]

Good points @pjg .

My memory is rusty and I'm looking into how this would be accomplished by digging into the BaseService methods.

If I recall, oauth1 library would not raise exceptions on its own and we checked the HTTP status code, which allowed us to handle the response handling (and raising of errors) ourselves in the check_response method.

Looking at the OAuth2 gem it appears that it does raise exceptions itself for a non-200 status code. Meaning that our existing check_response method never gets called; thereby implying that we would need to wrap do_http in its own begin/rescue block to catch this.

However, looking a little more at the OAuth2/Client itself:

https://github.com/oauth-xx/oauth2/blob/master/lib/oauth2/client.rb#L119

It appears we might be able to set raise_errors: false in the initial client configuration; thereby not raising the error and letting our current check_response fall through.

Well, to be more clear, we would suggest people set this config value when they create the intial client; as its not up to us internally.

@pjg do you think this would work?

[pjg]

As a developer I value existing conventions/behaviour to be persisted between new releases. As such, I would like to have the raise_errors setting be configurable by this gem's user, but I'd opt for having it be set to false by default.

[ruanltbg]

@ruckus do you have an update on this?

[yukideluxe]

Thanks for merging this @ruckus! I see that you've released a new version of the gem already but shouldn't we update the version file? :)

Thanks again!

[ruckus]

Thanks @yukideluxe -- forgot to push. Doh!