Improve client authentication method specs and assert request_parameters for client_secret_post

Author: ThisIsMissEm

spec/lib/oauth/client_authentication/client_secret_basic_spec.rb

@@ -5,21 +5,21 @@
 RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretBasic do
   describe 'matches_request?' do
     it "matches if the request has basic authorization" do
-      request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password'))
+      request = mock_request authorization: ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password')
 
       expect(described_class.matches_request?(request)).to be true
     end
 
     it "doesn't match if the request has bearer authorization" do
-      request = mock_request({}, "Bearer foobar")
+      request = mock_request authorization: "Bearer foobar"
 
       expect(described_class.matches_request?(request)).to_not be true
     end
   end
 
   describe 'authenticate' do
     it "returns credentials using the values from the authorization header" do
-      request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('client_id', 'client_secret'))
+      request = mock_request authorization: ActionController::HttpAuthentication::Basic.encode_credentials('client_id', 'client_secret')
 
       credentials = described_class.authenticate(request)
 
@@ -29,7 +29,7 @@
     end
 
     it "returns nil if the client_secret is missing from the authorization header" do
-      request = mock_request({}, ActionController::HttpAuthentication::Basic.encode_credentials('client_id', ''))
+      request = mock_request authorization: ActionController::HttpAuthentication::Basic.encode_credentials('client_id', '')
 
       credentials = described_class.authenticate(request)
 

spec/lib/oauth/client_authentication/client_secret_post_spec.rb

@@ -5,29 +5,38 @@
 RSpec.describe Doorkeeper::OAuth::ClientAuthentication::ClientSecretPost do
   describe 'matches_request?' do
     it "matches if the request doesn't have authorization" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: '1234',
         client_secret: '5678'
-      })
+      }
 
       expect(described_class.matches_request?(request)).to be true
     end
 
     it "doesn't match if the request is missing client_secret" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: '1234'
-      })
+      }
+
+      expect(described_class.matches_request?(request)).to_not be true
+    end
+
+    it "doesn't match if the parameters are in the query parameters" do
+      request = mock_request query_parameters: {
+        client_id: '1234',
+        client_secret: '5678'
+      }
 
       expect(described_class.matches_request?(request)).to_not be true
     end
   end
 
   describe 'authenticate' do
     it "returns credentials using the values from the request parameters" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: 'client_id',
         client_secret: 'client_secret'
-      })
+      }
 
       credentials = described_class.authenticate(request)
 

spec/lib/oauth/client_authentication/none_spec.rb

@@ -5,36 +5,36 @@
 RSpec.describe Doorkeeper::OAuth::ClientAuthentication::None do
   describe 'matches_request?' do
     it "matches if the request doesn't have authorization or client_secret" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: '1234'
-      })
+      }
 
       expect(described_class.matches_request?(request)).to be true
     end
 
     it "doesn't match if the request has client_secret" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: '1234',
         client_secret: "5678"
-      })
+      }
 
       expect(described_class.matches_request?(request)).to_not be true
     end
 
     it "doesn't match if the request has authorization" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: '1234'
-      }, ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password'))
+      }, authorization: ActionController::HttpAuthentication::Basic.encode_credentials('username', 'password')
 
       expect(described_class.matches_request?(request)).to_not be true
     end
   end
 
   describe 'authenticate' do
     it "returns credentials using the values from the request parameters, without a secret" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: 'client_id'
-      })
+      }
 
       credentials = described_class.authenticate(request)
 
@@ -44,10 +44,10 @@
     end
 
     it "ignores the client_secret if set" do
-      request = mock_request({
+      request = mock_request request_parameters: {
         client_id: 'client_id',
         client_secret: 'client_secret'
-      })
+      }
 
       credentials = described_class.authenticate(request)
 

spec/support/helpers/request_mock_helper.rb

@@ -2,7 +2,7 @@ module RequestMockHelper
   # I'm not sure if there's a better way to get a mock rack request for
   # testing. Here we don't need a full request spec, but we do need enough to
   # check that the logic of these classes works.
-  def mock_request(params, credentials = nil)
+  def mock_request(request_parameters: {}, query_parameters: {}, authorization: nil)
     request = ActionDispatch::Request.new({
       "REQUEST_METHOD"=>"POST",
       "SERVER_NAME"=>"example.org",
@@ -12,12 +12,12 @@ def mock_request(params, credentials = nil)
       "HTTP_HOST"=> "example.org",
       "ORIGINAL_FULLPATH" => "/test",
       "action_dispatch.remote_ip" => "127.0.0.1",
-      "action_dispatch.request.query_parameters" => {},
-      "action_dispatch.request.request_parameters" => params
+      "action_dispatch.request.query_parameters" => query_parameters,
+      "action_dispatch.request.request_parameters" => request_parameters
     })
 
-    unless credentials.nil?
-      request.env["HTTP_AUTHORIZATION"] = credentials
+    unless authorization.nil?
+      request.env["HTTP_AUTHORIZATION"] = authorization
     end
 
     request