Merge pull request #1747 from ransombriggs/update-pkce-error-message

Fix unknown pkce method error when configured

CHANGELOG.md

@@ -9,6 +9,8 @@ User-visible changes worth mentioning.
 
 Add your entry here.
 
+- [#1747] Fix unknown pkce method error when configured
+
 ## 5.8.0
 
 - [#1739] Add support for dynamic scopes

config/locales/en.yml

@@ -100,7 +100,10 @@ en:
         unauthorized_client: 'The client is not authorized to perform this request using this method.'
         access_denied: 'The resource owner or authorization server denied the request.'
         invalid_scope: 'The requested scope is invalid, unknown, or malformed.'
-        invalid_code_challenge_method: 'The code challenge method must be plain or S256.'
+        invalid_code_challenge_method:
+          zero: 'The authorization server does not support PKCE as there are no accepted code_challenge_method values.'
+          one: 'The code_challenge_method must be %{challenge_methods}.'
+          other: 'The code_challenge_method must be one of %{challenge_methods}.'
         server_error: 'The authorization server encountered an unexpected condition which prevented it from fulfilling the request.'
         temporarily_unavailable: 'The authorization server is currently unable to handle the request due to a temporary overloading or maintenance of the server.'
 

lib/doorkeeper/errors.rb

@@ -6,6 +6,10 @@ class DoorkeeperError < StandardError
       def type
         message
       end
+
+      def self.translate_options
+        {}
+      end
     end
 
     class InvalidGrantReuse < DoorkeeperError
@@ -45,6 +49,16 @@ def self.name_for_response
       end
     end
 
+    class InvalidCodeChallengeMethod < BaseResponseError
+      def self.translate_options
+        challenge_methods = Doorkeeper.config.pkce_code_challenge_methods_supported
+        {
+          challenge_methods: challenge_methods.join(", "),
+          count: challenge_methods.length
+        }
+      end
+    end
+
     UnableToGenerateToken = Class.new(DoorkeeperError)
     TokenGeneratorNotFound = Class.new(DoorkeeperError)
     NoOrmCleaner = Class.new(DoorkeeperError)
@@ -55,7 +69,6 @@ def self.name_for_response
     InvalidScope = Class.new(BaseResponseError)
     InvalidRedirectUri = Class.new(BaseResponseError)
     InvalidCodeChallenge = Class.new(BaseResponseError)
-    InvalidCodeChallengeMethod = Class.new(BaseResponseError)
     InvalidGrant = Class.new(BaseResponseError)
 
     UnauthorizedClient = Class.new(BaseResponseError)

lib/doorkeeper/oauth/error.rb

@@ -2,13 +2,14 @@
 
 module Doorkeeper
   module OAuth
-    Error = Struct.new(:name, :state) do
+    Error = Struct.new(:name, :state, :translate_options) do
       def description
-        I18n.translate(
-          name,
+        options = (translate_options || {}).merge(
           scope: %i[doorkeeper errors messages],
           default: :server_error,
         )
+
+        I18n.translate(name, **options)
       end
     end
   end

lib/doorkeeper/oauth/error_response.rb

@@ -12,6 +12,7 @@ def self.from_request(request, attributes = {})
           attributes.merge(
             name: error_name_for(request.error),
             exception_class: exception_class_for(request.error),
+            translate_options: request.error.try(:translate_options),
             state: request.try(:state),
             redirect_uri: request.try(:redirect_uri),
           ),
@@ -33,7 +34,7 @@ def self.exception_class_for(error)
       delegate :name, :description, :state, to: :@error
 
       def initialize(attributes = {})
-        @error = OAuth::Error.new(*attributes.values_at(:name, :state))
+        @error = OAuth::Error.new(*attributes.values_at(:name, :state, :translate_options))
         @exception_class = attributes[:exception_class]
         @redirect_uri = attributes[:redirect_uri]
         @response_on_fragment = attributes[:response_on_fragment]

spec/lib/oauth/error_spec.rb

@@ -3,10 +3,11 @@
 require "spec_helper"
 
 RSpec.describe Doorkeeper::OAuth::Error do
-  subject(:error) { described_class.new(:some_error, :some_state) }
+  subject(:error) { described_class.new(:some_error, :some_state, nil) }
 
   it { expect(error).to respond_to(:name) }
   it { expect(error).to respond_to(:state) }
+  it { expect(error).to respond_to(:translate_options) }
 
   describe "#description" do
     it "is translated from translation messages" do
@@ -17,5 +18,22 @@
       )
       error.description
     end
+
+    context "when there are variables" do
+      subject(:error) do
+        described_class.new(
+          :invalid_code_challenge_method,
+          :some_state,
+          {
+            challenge_methods: "foo, bar",
+            count: 2,
+          }
+        )
+      end
+
+      it "is translated from translation messages with variables" do
+        expect(error.description).to eq("The code_challenge_method must be one of foo, bar.")
+      end
+    end
   end
 end

spec/lib/oauth/pre_authorization_spec.rb

@@ -331,6 +331,24 @@
         expect(pre_auth).not_to be_authorizable
       end
 
+      context "when pkce_code_challenge_methods is set to none" do
+        before do
+          Doorkeeper.configure do
+            pkce_code_challenge_methods []
+          end
+        end
+
+        it "rejects plain as a code_challenge_method" do
+          attributes[:code_challenge] = "a45a9fea-0676-477e-95b1-a40f72ac3cfb"
+          attributes[:code_challenge_method] = "plain"
+
+          expect(pre_auth).to_not be_authorizable
+          expect(pre_auth.error_response.description).to eq(
+            "The authorization server does not support PKCE as there are no accepted code_challenge_method values."
+          )
+        end
+      end
+
       context "when pkce_code_challenge_methods is set to only S256" do
         before do
           Doorkeeper.configure do
@@ -350,8 +368,17 @@
           attributes[:code_challenge_method] = "plain"
 
           expect(pre_auth).to_not be_authorizable
+          expect(pre_auth.error_response.description).to eq("The code_challenge_method must be S256.")
         end
       end
+
+      it "rejects unknown as a code_challenge_method" do
+        attributes[:code_challenge] = "a45a9fea-0676-477e-95b1-a40f72ac3cfb"
+        attributes[:code_challenge_method] = "unknown"
+
+        expect(pre_auth).to_not be_authorizable
+        expect(pre_auth.error_response.description).to eq("The code_challenge_method must be one of plain, S256.")
+      end
     end
 
     context "when PKCE is not supported" do

spec/requests/flows/authorization_code_spec.rb

@@ -426,9 +426,9 @@ def authorize(redirect_url)
         )
       end
 
-      scenario "expects to set code_challenge_method explicitely without fallback" do
+      scenario "expects to set code_challenge_method explicitly without fallback" do
         visit authorization_endpoint_url(client: @client, code_challenge: code_challenge)
-        expect(page).to have_content("The code challenge method must be plain or S256.")
+        expect(page).to have_content("The code_challenge_method must be one of plain, S256.")
       end
     end
   end