Shopify / omniauth-shopify-oauth2

Shopify OAuth2 Strategy for OmniAuth 1.0
http://shopify.github.io/omniauth-shopify-oauth2
91 stars 69 forks source link

valid_site? fails on 2.0.0 #72

Open joelturnbull opened 5 years ago

joelturnbull commented 5 years ago

2.0.0 broke the shopify omniauth feature in my app and I had to downgrade back to 1.2.1. As I've seen in some other threads, the valid_site? method is returning false because of the change to how the shop value is accessed through the session. It seems like a lot of people are using this gem in conjunction with shopify_app, which I'm not. I'm using Devise and the devise initializer instead of the omniauth initializer to provide the keys and set the scopes we want to request.

When I inspect the values incoming to omniauth/strategies/shopify.rb#valid_site? I find my shop value in strategy.session['omniauth.params']['shop'] and nothing in strategy.session['shopify.omniauth_params']. I'm not sure what is setting this session value or how to tweak it in my code to provide it in the way the that 2.0.0 change expects. Any insight? Thanks

tylerball commented 5 years ago

Hi Joe,

A bit of background: there was a lot of confusion in our codebase around this, shopify_app's docs were updated to recommend using session for storing the shop, but the example app and this library was not. I've tried to make this more consistent everywhere.

All we do in shopify_app is set session['shopify.omniauth_params'] to contain the shop given and pick it up in the default setup method here. I would double check that you aren't overriding that setup method in an initializer anywhere as that's what we used to do in the shopify_app generator, but if you're seeing the shop populated in that session var this package should be picking it up properly.

uurcank commented 5 years ago

I think 2.0 released assuming everybody is using the shopify_app gem. This currently does not work for a regular omniauth request.

Reverting back to 1.2.1 fixes the problem

sumitbirla commented 5 years ago

Is there any update or workaround on this issue? I had to revert to 1.2.1 as well, however even that seems to have stopped working in Safari today.

uurcank commented 5 years ago

Safari issue might be related to this

43 Users signing up gets redircted to /auth/failure?message=csrf_detected&strategy=shopify

troex commented 5 years ago

I'm using sinatra and ran into similar issue when upgraded, just added

session['shopify.omniauth_params'] = { shop: params['shop'] }

to the route which receives install redirect

clupprich commented 5 years ago

Looking at source code of OmniAuth::Strategy, the session['omniauth.params'] is set in the request phase only after the setup phase has run. That means that we can't use session['omniauth.params'] to extract the shop param we gonna need.

Instead, we need to read the shop param from the Rack env, for example:

option :setup, proc { |env|
  request = Rack::Request.new(env)
  site = if request.params['shop']
    "https://#{request.params['shop']}"
  else
    ''
  end

  env['omniauth.strategy'].options[:client_options][:site] = site
}

That's basically what the shopify_app does, too.

I'll create a PR in the next couple of days.

Edit: Just realized that this is basically the code in v1.2.1

eni9889 commented 4 years ago

To anyone else having this issue I had to update my OmniAuth config to include this:

  provider :shopify,
           ENV['SHOPIFY_API_KEY'],
           ENV['SHOPIFY_API_SECRET'],
           scope: ...,
           setup:         ->(env) {
             request                                                  = Rack::Request.new(env)
             site                                                     = if request.params['shop']
                                                                          "https://#{request.params['shop']}"
                                                                        else
                                                                          ''
                                                                        end
             env['omniauth.strategy'].options[:client_options][:site] = site
           }