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.

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

build_access_token raises un-rescued Faraday error that bubble to apps using shopify-app #79

Open
ewalk153 opened this issue Mar 11, 2019 · 0 comments

Comments

@ewalk153
Copy link
Member

ewalk153 commented Mar 11, 2019

We expect that the underlying omniauth gem will reliably raise a connection errors with a fail when we call build_access_token:

https://github.com/omniauth/omniauth-oauth2/blob/10e1a42c7a49ad4488fe9085f814681e8848fbf6/lib/omniauth/strategies/oauth2.rb#L66-L83

      def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity
        error = request.params["error_reason"] || request.params["error"]
        if error
          fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"]))
        elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state"))
          fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
        else
          self.access_token = build_access_token
          self.access_token = access_token.refresh! if access_token.expired?
          super
        end
      rescue ::OAuth2::Error, CallbackError => e
        fail!(:invalid_credentials, e)
      rescue ::Timeout::Error, ::Errno::ETIMEDOUT => e
        fail!(:timeout, e)
      rescue ::SocketError => e
        fail!(:failed_to_connect, e)
      end

The underlying connection can close, which normally raises a Faraday ConnectionFailed error.

vendor/ruby-2.5.3/lib/ruby/2.5.0/net/protocol.rb:189:in `rbuf_fill': end of file reached (Faraday::ConnectionFailed)
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/protocol.rb:157:in `readuntil'
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/protocol.rb:167:in `readline'
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/http/response.rb:40:in `read_status_line'
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/http/response.rb:29:in `read_new'
[snip]

Recently, we saw some internal app fail to authenticate in a new fashion which stress a new failure mode to these request.

Options that I I see:

  1. We rescue in the omniauth-shopify-oauth2 as the oauth code path in shopify-app is merely asking for a token and shouldn't care about why it failed
  2. We push this concern up to the superclass omniauth-oauth2 to make sure Faraday errors are rescued
  3. We let it bubble up to shopify-app and rescue it there

In any event, this should not reach Shopify apps without being first handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant