ruckus / quickbooks-ruby

Quickbooks Online REST API V3 - Ruby
MIT License
374 stars 302 forks source link

Implement OAuth 2.0 #389

Closed Pro777 closed 5 years ago

Pro777 commented 7 years ago

Per this page: https://developer.intuit.com/docs/0100_quickbooks_online/0100_essentials/000500_authentication_and_authorization/connect_from_within_your_app

As of July 17, 2017, new or existing developers with no previous apps must implement OAuth 2.0. Click here for OAuth 1.0a documentation, available as a reference for existing applications.

CapellaBlue commented 7 years ago

I'm currently connecting my rails app to QB, and this gem is perfect-- I've been following the OAuth 2 discussion. Do you think I can use the OAuth 2 branch in development? We wont be this feature to production until end of year.

fabrouy commented 7 years ago

@CapellaBlue Of course! We appreciate you to give this branch a try and let us know how it goes.

ghost commented 7 years ago

I've verified that the following features work in my project with the new branch (389-oauth2) using Ruby 2.4:

What other tests need to be performed before we can merge 389-oauth2 into master?

fabrouy commented 7 years ago

IMO:

@ruckus What do you think?

ruckus commented 6 years ago

Has anyone had a chance to deploy this branch into production? Hows it holding up?

gregawoods commented 6 years ago

We soft launched a new app a little over a month ago and have had zero issues so far. I say soft launch because we are waiting on our client to start adding more data to the app, but in general, I feel pretty good about it.

justinstander commented 6 years ago

Hi. This been released? OAuth 1.0 is officially dead. We got cut off and need to switch to Oauth 2.0.

justinstander commented 6 years ago

I do see above there are branches to use. I'd sleep better at night if it was in production ;)

alienxp03 commented 6 years ago

Tried 389-oauth2 branch, but I'm getting error page 500 after clicking on the Connect to Quickbooks button. Seems like could be from their site, but no error message was displayed that could help me debug the issue.

gregawoods commented 6 years ago

We've actually been running into some trouble the last few days with this. Not sure if it's something in our code or the OAuth2 implementation. When we try to refresh our token we get a an error: OAuth2::Error: invalid_grant: {"error":"invalid_grant"}

QuickBooks docs say that the refresh token is supposed to be good for 100 days. We are seeing refresh errors way before that point.

The docs also say this...

When you request a fresh access token, always use the refresh token returned in the most recent token_endpoint response. Your previous refresh tokens expire 24 hours after you receive a new one.

But what I'm seeing so far is the refresh token isn't changing. The new refresh token we get back is identical to the one we used originally.

This is what my refresh code looks like.

class QuickbooksToken < ApplicationRecord
  validates :token, :refresh_token, :company_id, :expires_at,
    presence: true

  def access_token
    OAuth2::AccessToken.new(QuickbooksHelper.oauth_client, token, refresh_token: refresh_token)
  end

  def refresh!
    t2 = access_token.refresh!
    update!(
      token: t2.token,
      refresh_token: t2.refresh_token,
      expires_at: Time.zone.at(t2.expires_at)
    )
  end
end
gregawoods commented 6 years ago

Seems like other users are complaining about this as well. Solution might be to aggressively refresh your token every so many minutes.

https://help.developer.intuit.com/s/question/0D50f000051WMAfCAO/why-is-my-refreshtoken-invalid-after-24-hours

https://help.developer.intuit.com/s/question/0D50f000051WZUGCA4/refresh-token-is-expiring-each-day-instead-of-lasting-100-days

ruckus commented 6 years ago

But what I'm seeing so far is the refresh token isn't changing. The new refresh token we get back is identical to the one we used originally.

I spoke to Intuit last week regarding OAuth2 and they said the Refresh Tokens might or might not change at a value level but should always be treated as changing - so when do you refresh always store the latest value irrespective of it actually changing or not.

Also, yes, the refresh interval is 1 hour. Doing a fresh OAuth2 connection the response payload is:

{
    "access_token": " ...snipped ... ",
    "x_refresh_token_expires_in": 8726400,
    "refresh_token": "...snipped...",
    "token_type": "bearer",
    "expires_in": 3600
}

So that last expires_in is the expiry at 1 hour.

The expiration of Refresh Token itself is x_refresh_token_expires_in which is 100 days. Intuit told me they are considering lifting this, which I hope they do because if NOT then it means the whole enchilada is invalid after 100 days and customers would have to re-authenticate whole hog. Which is crazy.

It depends on your usage patterns but it might possible to just iterate through your active tokens every X minutes using a cron/scheduler and just refresh them all. This would only work if you are using QBO in such a batched way, e.g. your app hits the QBO API every X hours to do something and there is no real-time access. If there was real-time access and your customers are doing stuff in your app and interacting with QBO randomly then this would fail spectacularly because your clients could be in the middle of a QBO operation / transaction when your scheduled refresh cycle kicks in and wipes out their access token under the feet.

So barring that kind of usage pattern the only alternative I can see is a way to wrap all QBO operations in its own auth exception handling which transparently rescues from the auth error, refreshes the token, and then carries on with the original request.

Some pseudo-code piggy-backing off your code above (no idea if this works)!:

class QuickbooksToken < ApplicationRecord
  validates :token, :refresh_token, :company_id, :expires_at,
    presence: true

  def access_token
    OAuth2::AccessToken.new(QuickbooksHelper.oauth_client, token, refresh_token: refresh_token)
  end

  def refresh!
    t2 = access_token.refresh!
    update!(
      token: t2.token,
      refresh_token: t2.refresh_token,
      expires_at: Time.zone.at(t2.expires_at)
    )
  end

  def perform_request(&block)
    begin
      yield block(access_token)
    rescue OAuth2::Error > ex
       # to prevent an infinite loop here keep a counter and bail out after N times...

       # check if its an invalid_grant first, but assume it is for now
       refresh!

       retry
    end
  end

end

token = QuickbooksToken.first
token.perform_request do |access_token|
  service = Quickbooks::Service::Customer.new
  service.access_token = access_token
  service.query
end
gregawoods commented 6 years ago

Hi @ruckus - thanks for the insight.

What you recommend with auto-refreshing is remarkably similar to what we are currently doing. All calls to QuickBooks are wrapped in a with_refreshing block:

def with_refreshing
  yield
rescue Quickbooks::AuthorizationFailure
  quickbooks_token.refresh!
  service.oauth = quickbooks_token.access_token
  yield
end

This had been working great until this week. I think what's happening is we have a period of inactivity in the app long enough for the refresh token to expire.

I guess I'm still stumped as to why we are running into this problem well before 100 days.

[...] when your scheduled refresh cycle kicks in and wipes out their access token under the feet.

Yeah, this is what worries me about using the cron job workaround. My other thought was to implement some kind of locking mechanism where by the token could only be accessed by one thread at a time.

justinstander commented 6 years ago

Also... be careful your business is "acceptable" to QB. Otherwise you'll get cut off and authentication will fail for ever and ever :)

On Fri, Mar 16, 2018 at 10:17 AM, Greg Woods notifications@github.com wrote:

Hi @ruckus https://github.com/ruckus - thanks for the insight.

What you recommend with auto-refreshing is remarkably similar to what we are currently doing. All calls to QuickBooks are wrapped in a with_refreshing block:

def with_refreshing yieldrescue Quickbooks::AuthorizationFailure quickbooks_token.refresh! service.oauth = quickbooks_token.access_token yieldend

This had been working great until this week. I think what's happening is we have a period of inactivity in the app long enough for the refresh token to expire.

I guess I'm still stumped as to why we are running into this problem well before 100 days.

[...] when your scheduled refresh cycle kicks in and wipes out their access token under the feet.

Yeah, this is what worries me about using the cron job workaround. My other thought was to implement some kind of locking mechanism where by the token could only be accessed by one thread at a time.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ruckus/quickbooks-ruby/issues/389#issuecomment-373726901, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDf4zMdrwSxei1ubVveF2HiXTjB7R4tks5te8mLgaJpZM4OeYnE .

fabrouy commented 6 years ago

We have been running this branch in production with no issues so far 👍

NaumanZahid commented 6 years ago

@ruckus Thanks for working on this. I am also building my app using this branch and so far it worked good.

phstc commented 6 years ago

The perform_request solution is great, thanks for sharing 🍻

But using it as is (with a block) is complicated for me since sometimes I have more than one QB call, and if I retry the whole thing, it would end up retrying more than I would like it to.

I ended up creating a wrapper class for "fail-safe" QB calls.

# instead of this
service = Quickbooks::Service::Item.new(access_token: access_token, company_id: realm_id)

# I use this
service = QuickbookServiceWrapper.new(qb, Quickbooks::Service::Item)

The full implementation:

class QuickbooksServiceWrapper
  attr_reader :qb, :target_class, :target

  def initialize(qb, target_class)
    @qb = qb
    @target_class = target_class
    init_target
  end

  def method_missing(method_name, *arguments, &block)
    perform_request do
      target.send(method_name, *arguments, &block)
    end
  end

  def respond_to?(method_name, include_private = false)
    target.respond_to?(method_name, include_private)
  end

  private

  def init_target
    @target = target_class.new(access_token: qb.access_token, company_id: qb.realm_id)
  end

  def perform_request(&block)
    # see https://github.com/ruckus/quickbooks-ruby/issues/389#issuecomment-373724559
    attempts = 0
    begin
      yield
    rescue Quickbooks::AuthorizationFailure => ex
      raise if attempts.positive?

      attempts += 1

      qb.refresh!

      # reset target with new keys
      init_target

      retry
    end
  end
end

For the refresh_token update in case of inactive users, we have a "cron" that renews tokens about expire (7 days before their expiration) every day.

Besides updating with the new tokens, we also update their respective expiration at.

auth_keys.expires_at = Time.zone.at(access_token.expires_at)
auth_keys.refresh_token_expires_at = Time.current + access_token.params['x_refresh_token_expires_in']

[...] when your scheduled refresh cycle kicks in and wipes out their access token under the feet.

Yeah, this is what worries me about using the cron job workaround. My other thought was to implement some kind of locking mechanism where the token could only be accessed by one thread at a time.

I tested having an access token in memory on a process, then refreshing the access token on another process, the initial one continued to work just fine. So it seems that updating an access token does not auto-invalidate existing ones. The cron job seems to be working fine so far.

plainlystated commented 5 years ago

Any idea when this will get merged into master? I see that there's gem available quickbooks-ruby-oauth2 (0.6.1.3). Is that a packaged version of the branch in question?

EDIT - nope, looks like it an unofficial fork of this project.

ruckus commented 5 years ago

Hi @plainlystated good question. I had not seen that fork before. Interesting.

Basically I've been letting the community run this branch in production and was hoping that any bugs would be reported back. Before merging into master. In this thread there is feedback with people successfully running it in production.

I don't see any major showstoppers being reported. But, maybe there are some and no one has reported it.

joshbeckman commented 5 years ago

Can confirm: using branch 389-oauth2 in production for last 6 months to both push and pull invoices with no issues at all. 👍

ruckus commented 5 years ago

Nice @andjosh ! Are your authenticated users/clients/orgs blended across oauth1 and oauth2 or are they all oauth2?

joshbeckman commented 5 years ago

We use only oauth2, and largely for only one authenticated user token that is refreshed repeatedly. So, this is not a test of the combined auth flows or a highly multi-tenant implementation.

Good question!

gregawoods commented 5 years ago

I'll go ahead and chime in, we've been running on the 389-oauth2 branch for a good 8 months now and have not experienced any issues.

Ours is also a single-token app, so I can't vouch for multiple tokens or blended oauth1/2.

senzpo commented 5 years ago

Hi all, is it necessary to use these strict dependencies:

+      oauth (= 0.4.7)
+      oauth2 (= 1.4.0)
+      roxml (= 4.0.0)

it looks outdated and difficult to includes with other gems

orangethunder commented 5 years ago

@senzpo, we just did QA on my fork (from master, not the the oauth2 branch). The QA was successful for us using oauth 0.5.4, and consisted of authenticating and posting some invoices. Will see how it holds up in production!

ruckus commented 5 years ago

@senzpo @orangethunder my memory was fuzzy but I vaguely recall having to use those versions to make the file attachments work by hooking into the oauth/http library for multi-part uploads.

If you have the wherewithal it would be wonderful and much appreciated to test that.

  1. upgrade oauth dependencies to latest (or whatever is needed)
  2. perform an attachment upload

thank you again!

senzpo commented 5 years ago

@orangethunder @ruckus Hi! I resolved my issue with this PR: https://github.com/ruckus/quickbooks-ruby/pull/459, thanks to @ruckus. It will be perfect to slowly merge the oauth2 branch to the main branch :) But I realized that isn't easy. Thanks a lot!

c-moyer commented 5 years ago

@ruckus Hey, thanks for all the work on this branch, I have been following it for a while now. As of today I received an email from Intuit that noted an EOL for OAuth 1.0.

`On December 17th, 2019, Intuit will discontinue all support for OAuth 1.0 and OpenID 2.0. After December 17th, 2019, applications will no longer be allowed to make API calls using OAuth 1.0 and OpenID 2.0.`

Are there any near future plans to merge this branch with Master? Again, I appreciate all your work here!

Thanks!

shepmaster commented 5 years ago

@phstc's wrapper class is a good idea as it prevents retrying things that don't need to be retried, but be aware that some quickbooks-ruby classes (such as Quickbooks::Service::Invoice) define their own send method, which conflicts. I chose to use the __send__ method instead. Here's my updated version:

# frozen_string_literal: true

# Any arbitrary QBO API call may fail because the access token has
# been invalidated. We catch that, refresh the token, then retry the
# call.
class QuickbooksIntegrationService
  class RetryAuthFailures
    attr_reader :qbo_auth, :target_class

    def initialize(qbo_auth:, target_class:)
      @qbo_auth = qbo_auth
      @target_class = target_class
    end

    def method_missing(method_name, *arguments, &block)
      if target.respond_to?(method_name)
        perform_request do
          target.__send__(method_name, *arguments, &block)
        end
      else
        super
      end
    end

    def respond_to_missing?(method_name, include_private = false)
      target.respond_to?(method_name, include_private) || super
    end

    private

    def target
      @target ||= target_class.new(
        access_token: qbo_auth.oauth2_access_token,
        company_id: qbo_auth.oauth2_realm_id
      )
    end

    def perform_request
      yield
    rescue Quickbooks::AuthorizationFailure
      # Update target with new keys
      qbo_auth.oauth2_renew_token!

      # Try again
      @target = nil
      yield
    end
  end
end
shepmaster commented 5 years ago

The QBO documentation states:

As a best practice, always store the latest refresh token received from the API response and use that to make subsequent calls to obtain a new pair of tokens.

What is the intended method to get back the potentially-updated refresh token from an arbitrary quickbooks-ruby API call?

ruckus commented 5 years ago

@shepmaster that comment is not about an arbitrary API response. No auth tokens of any kind are embedded in an arbitrary response.

What that line is a reference to always storing the latest tokens from the Refresh Token API call and not assuming it wont change. That is, always store the values you receive back during the refresh operation and make no assumptions it will remain static for any length of time.

shepmaster commented 5 years ago

No auth tokens of any kind are embedded in an arbitrary response.

Hmm. I think the original documentation is ambiguous at best, but I'll trust that you have far more experience than I do! However, I am storing both the access and refresh tokens in response to the refresh call, so that sounds like I'm on the right track!

shepmaster commented 5 years ago

We are currently using quickbooks-ruby 0.6.7, but the 389-oauth2 branch still reports 0.6.1. Is there a possibility to get the bug fixes ported to that branch?

drewish commented 5 years ago

Would it be possible to open up a PR for https://github.com/ruckus/quickbooks-ruby/tree/389-oauth2 against master? We're getting ready to try to do the upgrade to OAuth 2 and it took me a while to realize that the code hadn't already been merged. Having an open PR would make that easier to discover the state.

ruckus commented 5 years ago

@drewish I should probably just merge to master now. If anyone needs this version for the next couple of months they could specify the current version number.

I'm also about to do the upgrade and migrate current OAuth1 tokens ... I'm scared

barconati commented 5 years ago

Great news! I'm about to release a new QuickBooks app and have been using this branch in development. I'll be good to switch to master before launching the app. Thanks for all your work on this @ruckus

drewish commented 5 years ago

@ruckus yeah maybe just create and oauth_1 branch or tag from master right before you merge it so folks have an easy way to run the old stuff if need be. The branch could be good if you were going to accept back ports of bug fixes… but probably not required since there’s a hard cut over required in 3 months.

ruckus commented 5 years ago

Has anyone done any token migration from O1 to O2?

https://developer.intuit.com/app/developer/qbo/docs/develop/authentication-and-authorization/migration#migration-api

If so, how did you do it? Using the QBO gem:

https://github.com/intuit/oauth-rubyclient

? Or just manually using the REST API?

shepmaster commented 5 years ago

I am attempting to do it with oauth-rubyclient but running into opaque 403 errors. My hunch is that it's because I'm using sandbox mode instead of production mode; there were some people on the QBO help site reporting the same problem. I've got a support ticket open attempting to resolve it.

I am using oauth-rubyclient to do the OAuth login and token renewal and it's been working fine in my developer testing.

Update It turns out I was confused by Intuit's system of applications. I have a developer account separate from our production account (a reasonable software practice, IMO). However, my developer account has two applications, one that is OAuth 1.0 only and another one that is OAuth 2.0 only. Our production account/application is OAuth 1.0 and OAuth 2.0, so migrating the tokens works fine there.

Intuit support was pretty unhelpful, it was only after weeks of poking at things that we realized that somehow the two accounts had drastically different abilities.

vanboom commented 5 years ago

Has anyone been able to renew refresh tokens? I am getting an Authentication required error message, even though the access token that I am giving Quickbooks::Service::AccessToken works to process sales receipts...

    qbo = Quickbooks::Service::AccessToken.new
    qbo.access_token = current_access_token
    qbo.realm_id = company_id
    result = qbo.renew
<PlatformResponse xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://platform.intuit.com/api/v1">
  <ErrorMessage>Authentication required</ErrorMessage>
  <ErrorCode>22</ErrorCode>
  <ServerTime>2019-09-17T20:53:38.8615361Z</ServerTime>
</PlatformResponse>

UPDATE: I am not within the 30 day reconnect window, which might be causing this.

serixscorpio commented 5 years ago

@ruckus I tried token migration in sandbox mode using the QBO gem, and it works as expected. I leveraged the example webapp from this repo (added migrate.rb) to test out the migration API .

vanboom commented 5 years ago

@ruckus I just completed migration of two of our apps to OAuth2 using the #389-oauth2 branch. During migration we were able to switch back and forth between OAuth and OAuth2 using this branch. Everything works beautifully with zero issues - thanks everyone for this work.

shepmaster commented 5 years ago

We have deployed the 389-oauth2 branch to production but are still running in OAuth 1 mode. We've gotten some issue reports about invoice synchronization. I see that there's been some changes to invoices in master and there's an open PR (#481) synchronizing the branches; is there some hope for an updated branch soon?

ruckus commented 5 years ago

I have squashed/merged the open PR #481 into master and released version 1.0.0. Thank you to everyone who has helped in this big task. Thank you!

drewish commented 4 years ago

Late to the party but ended up doing a rake task to migrate our connections and didn't want to bother using Intuit's gem. This just uses the OAuth gems more directly:

    migration_endpoint =
      if Quickbooks.sandbox_mode
        'https://developer-sandbox.api.intuit.com/v2/oauth2/tokens/migrate'
      else
        'https://developer.api.intuit.com/v2/oauth2/tokens/migrate'
      end

    # We don't need the exact callback just a valid one for each environment.
    callback_url =
      if Rails.env.production?
        'https://example.com/finish_oauth'
      else
        'https://example.com/finish_oauth'
      end

    body = {
      client_id: $qbo_oauth2_consumer.id,
      client_secret: $qbo_oauth2_consumer.secret,
      redirect_uri: callback_url,
      scope: 'com.intuit.quickbooks.accounting',
    }
    headers = {
      'Content-Type': 'application/json',
      Accept: 'application/json',
    }

    counts = { success: 0, skip: 0, fail: 0 }
    # This is ActiveRecord our model that stores the credentials.
    QBOConnection.connected.find_each do |connection|
      if connection.uses_oauth2?
        puts "connection id: #{connection.id}, skipped, already using OAuth2"
        counts[:skip] += 1
        next
      end

      begin
        # The connection.access_token method is just calling:
        # OAuth::AccessToken.new($qbo_oauth_consumer, secrets[:access_token], secrets[:access_secret])
        response = connection.access_token.post(migration_endpoint, body.to_json, headers)
        new_token = OAuth2::AccessToken.from_hash($qbo_oauth2_consumer, JSON.parse(response.body))
        # The connect! method just persists the new access and refresh tokens.
        connection.connect! new_token['realmId'], new_token
        puts "connection id: #{connection.id}, migrated to OAuth2"
        counts[:success] += 1
      rescue => e
        puts "connection id: #{connection.id}, error: #{e.inspect}"
        counts[:fail] += 1
      end
    end

    puts "Completed #{counts}"

Maybe it'll help someone else with a last minute migration.