Merge pull request #116 from Shopify/remove-scope-check

Stop validating that returned scope matches requested scope

lib/omniauth/strategies/shopify.rb

@@ -73,13 +73,6 @@ def valid_signature?
         validate_signature(new_secret) || (old_secret && validate_signature(old_secret))
       end
 
-      def valid_scope?(token)
-        params = options.authorize_params.merge(options_for("authorize"))
-        return false unless token && params[:scope] && token['scope']
-        expected_scope = normalized_scopes(params[:scope]).sort
-        (expected_scope == token['scope'].split(SCOPE_DELIMITER).sort)
-      end
-
       def normalized_scopes(scopes)
         scope_list = scopes.to_s.split(SCOPE_DELIMITER).map(&:strip).reject(&:empty?).uniq
         ignore_scopes = scope_list.map { |scope| scope =~ /\A(unauthenticated_)?write_(.*)\z/ && "#{$1}read_#{$2}" }.compact
@@ -128,9 +121,6 @@ def callback_phase
         return fail!(:invalid_signature, CallbackError.new(:invalid_signature, "Signature does not match, it may have been tampered with.")) unless valid_signature?
 
         token = build_access_token
-        unless valid_scope?(token)
-          return fail!(:invalid_scope, CallbackError.new(:invalid_scope, "Scope does not match, it may have been tampered with."))
-        end
         unless valid_permissions?(token)
           return fail!(:invalid_permissions, CallbackError.new(:invalid_permissions, "Requested API access mode does not match."))
         end

omniauth-shopify-oauth2.gemspec

@@ -25,5 +25,6 @@ Gem::Specification.new do |s|
   s.add_development_dependency 'minitest', '~> 5.6'
   s.add_development_dependency 'rspec', '~> 3.9.0'
   s.add_development_dependency 'fakeweb', '~> 1.3'
+  s.add_development_dependency 'rack-session', '~> 2.0'
   s.add_development_dependency 'rake'
 end

test/integration_test.rb

@@ -206,29 +206,27 @@ def test_callback_with_invalid_state_fails
     assert_equal '/auth/failure?message=csrf_detected&strategy=shopify', response.location
   end
 
-  def test_callback_with_mismatching_scope_fails
+  def test_callback_with_mismatching_scope_succeeds
     access_token = SecureRandom.hex(16)
     code = SecureRandom.hex(16)
     expect_access_token_request(access_token, 'some_invalid_scope', nil)
 
     response = callback(sign_with_new_secret(shop: 'snowdevil.myshopify.com', code: code, state: opts["rack.session"]["omniauth.state"]))
 
-    assert_equal 302, response.status
-    assert_equal '/auth/failure?message=invalid_scope&strategy=shopify', response.location
+    assert_callback_success(response, access_token, code)
   end
 
-  def test_callback_with_no_scope_fails
+  def test_callback_with_no_scope_succeeds
     access_token = SecureRandom.hex(16)
     code = SecureRandom.hex(16)
     expect_access_token_request(access_token, nil)
 
     response = callback(sign_with_new_secret(shop: 'snowdevil.myshopify.com', code: code, state: opts["rack.session"]["omniauth.state"]))
 
-    assert_equal 302, response.status
-    assert_equal '/auth/failure?message=invalid_scope&strategy=shopify', response.location
+    assert_callback_success(response, access_token, code)
   end
 
-  def test_callback_with_missing_access_scope_fails
+  def test_callback_with_missing_access_scope_succeeds
     build_app scope: 'first_scope,second_scope'
 
     access_token = SecureRandom.hex(16)
@@ -237,11 +235,10 @@ def test_callback_with_missing_access_scope_fails
 
     response = callback(sign_with_new_secret(shop: 'snowdevil.myshopify.com', code: code, state: opts["rack.session"]["omniauth.state"]))
 
-    assert_equal 302, response.status
-    assert_equal '/auth/failure?message=invalid_scope&strategy=shopify', response.location
+    assert_callback_success(response, access_token, code)
   end
 
-  def test_callback_with_extra_access_scope_fails
+  def test_callback_with_extra_access_scope_succeeds
     build_app scope: 'first_scope,second_scope'
 
     access_token = SecureRandom.hex(16)
@@ -250,8 +247,7 @@ def test_callback_with_extra_access_scope_fails
 
     response = callback(sign_with_new_secret(shop: 'snowdevil.myshopify.com', code: code, state: opts["rack.session"]["omniauth.state"]))
 
-    assert_equal 302, response.status
-    assert_equal '/auth/failure?message=invalid_scope&strategy=shopify', response.location
+    assert_callback_success(response, access_token, code)
   end
 
   def test_callback_with_scopes_out_of_order_works
@@ -375,7 +371,7 @@ def test_callback_when_creds_are_invalid
 
     FakeWeb.register_uri(
       :post,
-      "https://snowdevil.myshopify.com/admin/oauth/access_token",
+      %r{snowdevil.myshopify.com/admin/oauth/access_token},
       status: [ "401", "Invalid token" ],
       body: "Token is invalid or has already been requested"
     )
@@ -415,7 +411,7 @@ def sign_with_new_secret(params)
   end
 
   def expect_access_token_request(access_token, scope, associated_user=nil, session=nil)
-    FakeWeb.register_uri(:post, "https://snowdevil.myshopify.com/admin/oauth/access_token",
+    FakeWeb.register_uri(:post, %r{snowdevil.myshopify.com/admin/oauth/access_token},
                          body: JSON.dump(
                            access_token: access_token,
                            scope: scope,
@@ -426,10 +422,11 @@ def expect_access_token_request(access_token, scope, associated_user=nil, sessio
   end
 
   def assert_callback_success(response, access_token, code)
+    credentials = ::Base64.decode64(FakeWeb.last_request['authorization'].split(" ", 2)[1] || "")
+    assert_equal "123:#{@secret}", credentials
+
     token_request_params = Rack::Utils.parse_query(FakeWeb.last_request.body)
-    assert_equal token_request_params['client_id'], '123'
-    assert_equal token_request_params['client_secret'], @secret
-    assert_equal token_request_params['code'], code
+    assert_equal code, token_request_params['code']
 
     assert_equal 'snowdevil.myshopify.com', @omniauth_result.uid
     assert_equal access_token, @omniauth_result.credentials.token

test/test_helper.rb

@@ -3,6 +3,7 @@
 require 'omniauth-shopify-oauth2'
 
 require 'minitest/autorun'
+require 'rack/session'
 require 'fakeweb'
 require 'json'
 require 'active_support/core_ext/hash'