Merge pull request #1488 from toupeira/invalid-client-credentials-bas…

…ic-auth

Fix ROPC client authentication validation when it's optional

CHANGELOG.md

@@ -8,6 +8,9 @@ User-visible changes worth mentioning.
 ## main
 
 - [#PR ID] Add your PR description here.
+- [#1488] Verify client authentication for Resource Owner Password Grant when
+  `config.skip_client_authentication_for_password_grant` is set and the client credentials
+  are sent in a HTTP Basic auth header.
 
 ## 5.5.0
 

lib/doorkeeper/config.rb

@@ -278,6 +278,10 @@ def configure_secrets_for(type, using:, fallback:)
     # Rationale: https://github.com/doorkeeper-gem/doorkeeper/issues/1189
     option :token_reuse_limit,              default: 100
 
+    # Don't require client authentication for password grants. If client credentials
+    # are present they will still be validated, and the grant rejected if the credentials
+    # are invalid.
+    #
     # This is discouraged. Spec says that password grants always require a client.
     #
     # See https://github.com/doorkeeper-gem/doorkeeper/issues/1412#issuecomment-632750422

lib/doorkeeper/oauth/password_access_token_request.rb

@@ -10,12 +10,13 @@ class PasswordAccessTokenRequest < BaseRequest
       validate :resource_owner, error: :invalid_grant
       validate :scopes, error: :invalid_scope
 
-      attr_reader :client, :resource_owner, :parameters, :access_token
+      attr_reader :client, :credentials, :resource_owner, :parameters, :access_token
 
-      def initialize(server, client, resource_owner, parameters = {})
+      def initialize(server, client, credentials, resource_owner, parameters = {})
         @server          = server
         @resource_owner  = resource_owner
         @client          = client
+        @credentials     = credentials
         @parameters      = parameters
         @original_scopes = parameters[:scope]
         @grant_type      = Doorkeeper::OAuth::PASSWORD
@@ -60,7 +61,7 @@ def validate_resource_owner
       #
       def validate_client
         if Doorkeeper.config.skip_client_authentication_for_password_grant
-          !parameters[:client_id] || client.present?
+          client.present? || (!parameters[:client_id] && credentials.blank?)
         else
           client.present?
         end

lib/doorkeeper/request/password.rb

@@ -9,6 +9,7 @@ def request
         @request ||= OAuth::PasswordAccessTokenRequest.new(
           Doorkeeper.config,
           client,
+          credentials,
           resource_owner,
           parameters,
         )

spec/lib/oauth/password_access_token_request_spec.rb

@@ -4,7 +4,7 @@
 
 RSpec.describe Doorkeeper::OAuth::PasswordAccessTokenRequest do
   subject(:request) do
-    described_class.new(server, client, owner)
+    described_class.new(server, client, credentials, owner)
   end
 
   let(:server) do
@@ -19,6 +19,7 @@
     )
   end
   let(:client) { Doorkeeper::OAuth::Client.new(FactoryBot.create(:application)) }
+  let(:credentials) { Doorkeeper::OAuth::Client::Credentials.new("uid", "secret") }
   let(:application) { client.application }
   let(:owner) { FactoryBot.build_stubbed(:resource_owner) }
 
@@ -35,7 +36,7 @@
   end
 
   it "doesn't issue a new token without client authentication" do
-    request = described_class.new(server, nil, owner)
+    request = described_class.new(server, nil, nil, owner)
     expect do
       request.authorize
     end.not_to(change { Doorkeeper::AccessToken.count })
@@ -52,7 +53,7 @@
     end
 
     it "issues a new token for the client without client authentication" do
-      request = described_class.new(server, nil, owner)
+      request = described_class.new(server, nil, nil, owner)
       expect do
         request.authorize
       end.to change { Doorkeeper::AccessToken.count }.by(1)
@@ -62,7 +63,7 @@
   end
 
   it "doesn't issue a new token with an invalid client" do
-    request = described_class.new(server, nil, owner, { client_id: "bad_id" })
+    request = described_class.new(server, nil, credentials, owner, { client_id: "bad_id" })
     expect do
       request.authorize
     end.not_to(change { Doorkeeper::AccessToken.count })
@@ -71,7 +72,7 @@
   end
 
   it "requires the owner" do
-    request = described_class.new(server, client, nil)
+    request = described_class.new(server, client, credentials, nil)
     request.validate
     expect(request.error).to eq(:invalid_grant)
   end
@@ -130,7 +131,7 @@
 
   describe "with scopes" do
     subject(:request) do
-      described_class.new(server, client, owner, scope: "public")
+      described_class.new(server, client, credentials, owner, scope: "public")
     end
 
     context "when scopes_by_grant_type is not configured for grant_type" do
@@ -196,7 +197,7 @@
     end
 
     it "checks scopes" do
-      request = described_class.new(server, client, owner, scope: "public")
+      request = described_class.new(server, client, credentials, owner, scope: "public")
       allow(server).to receive(:scopes).and_return(Doorkeeper::OAuth::Scopes.from_string("public"))
 
       expect do
@@ -207,7 +208,7 @@
     end
 
     it "falls back to the default otherwise" do
-      request = described_class.new(server, client, owner, scope: "private")
+      request = described_class.new(server, client, credentials, owner, scope: "private")
       allow(server).to receive(:scopes).and_return(Doorkeeper::OAuth::Scopes.from_string("private"))
 
       expect do

spec/requests/flows/password_spec.rb

@@ -222,6 +222,38 @@
           expect(token.application_id).to be_nil
           expect(json_response).to include("access_token" => token.token)
         end
+
+        it "doesn't issue a new token with invalid client credentials in query string" do
+          expect do
+            post password_token_endpoint_url(
+              resource_owner: @resource_owner,
+              client_id: "invalid",
+              client_secret: "invalid",
+            )
+          end.not_to(change { Doorkeeper::AccessToken.count })
+
+          expect(response.status).to eq(401)
+          expect(json_response).to match(
+            "error" => "invalid_client",
+            "error_description" => an_instance_of(String),
+          )
+        end
+
+        it "doesn't issue a new token with invalid client credentials in Basic auth" do
+          invalid_client = Doorkeeper::OAuth::Client::Credentials.new("invalid", "invalid")
+
+          expect do
+            post password_token_endpoint_url(
+              resource_owner: @resource_owner,
+            ), headers: { "HTTP_AUTHORIZATION" => basic_auth_header_for_client(invalid_client) }
+          end.not_to(change { Doorkeeper::AccessToken.count })
+
+          expect(response.status).to eq(401)
+          expect(json_response).to match(
+            "error" => "invalid_client",
+            "error_description" => an_instance_of(String),
+          )
+        end
       end
 
       context "when disabled" do