Fix unknown pkce method error when configured #1747

ransombriggs · 2024-10-31T15:14:11Z

Summary

I am upgrading to get the configuration update from #1735 but noticed that the error message is not correct when it is configured in that it makes it sound like we accept plain.

#1746

ransombriggs · 2024-10-31T15:15:19Z

config/locales/en.yml

@@ -100,7 +100,7 @@ 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: 'The code challenge method must be one of %{challenge_methods}.'


This could probably use some tweaking, just let me know how you think this should read.

You'd probably want to use the one / other syntax here..

Suggested change

invalid_code_challenge_method: 'The code challenge method must be one of %{challenge_methods}.'

invalid_code_challenge_method:

one: 'The code challenge method must be %{challenge_methods}.'

other: 'The code challenge method must be one of %{challenge_methods}.'

Which would need a count on challenge_methods, iirc. count is magic in that it decides whether to pick one or other

Awesome suggestions, I went ahead and updated in ccc9773

ransombriggs · 2024-10-31T15:16:53Z

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),


I was not completely confident what classes request.error are expected here. I can remove the try if we always expect to receive a DoorkeeperError here.

Maybe there's a default that can be set? grab :translate_options or fallback to {}

I default it here in error.rb because that caught all the usages of Error in one place.

More thinking of if the Error somehow doesn't derive from Doorkeeper::OAuth::Error, e.g., it comes from activerecord or something

ThisIsMissEm · 2024-11-01T02:00:00Z

lib/doorkeeper/errors.rb

+    class InvalidCodeChallengeMethod < BaseResponseError
+      def self.translate_options
+        {
+          challenge_methods: Doorkeeper.config.pkce_code_challenge_methods_supported.join(", ")


Re: pluralisation in https://github.com/doorkeeper-gem/doorkeeper/pull/1747/files#r1825345954

Suggested change

challenge_methods: Doorkeeper.config.pkce_code_challenge_methods_supported.join(", ")

challenge_methods: Doorkeeper.config.pkce_code_challenge_methods_supported.join(", "),

count: Doorkeeper.config.pkce_code_challenge_methods_supported.length

Thanks, used this in ccc9773

ThisIsMissEm · 2024-11-01T02:02:27Z

spec/lib/oauth/error_spec.rb

+    context "when there are variables" do
+      subject(:error) do
+        described_class.new(
+          :invalid_code_challenge_method,
+          :some_state,
+          {
+            challenge_methods: "foo, bar"
+          }
+        )
+      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


We should have a test case for the "one" option, i.e., just "foo" not "foo, bar"

(this likely more applies to pre_authorization_spec.

I added a test for %w(foo) and [] in ccc9773

ThisIsMissEm · 2024-11-01T17:14:56Z

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: 'There are no acceptable code_challenge methods'


this should never be reachable, but if it is, say

Suggested change

zero: 'There are no acceptable code_challenge methods'

zero: 'This authorization server does not support PKCE as there a no accepted code_challenge methods'

Would be better?

That is better, tweaked it just a little bit to align with other messages and updated in f042917

ThisIsMissEm · 2024-11-01T17:16:53Z

spec/lib/oauth/pre_authorization_spec.rb

+          attributes[:code_challenge_method] = "plain"
+
+          expect(pre_auth).to_not be_authorizable
+          expect(pre_auth.error_response.description).to eq("There are no acceptable code_challenge methods")


Also, we should use code_challenge or code challenge consistently.

Good catch, this comment made me realize that we should probably be calling it code_challenge_method since that is the actual param, so I made that change in d244e69

ThisIsMissEm · 2024-11-01T19:56:48Z

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 is no accepted code_challenge_method'


Suggested change

zero: 'The authorization server does not support PKCE as there is no accepted code_challenge_method'

zero: 'The authorization server does not support PKCE as there are no accepted code_challenge_method values.'

I updated this message in 7e2bf4b

ThisIsMissEm

Tiny comment, but LGTMO

ThisIsMissEm

Looks good!

nbulaj

LGTM 👍

Thanks!

Fix unknown pkce method error when configured

8cbe710

ransombriggs mentioned this pull request Oct 31, 2024

Add pkce_code_challenge_methods option #1735

Merged

ransombriggs commented Oct 31, 2024

View reviewed changes

ThisIsMissEm reviewed Nov 1, 2024

View reviewed changes

ransombriggs added 2 commits November 1, 2024 09:15

Use the count feature of i18n to fix plurals

ccc9773

Fix broken error_spec

4d5be91

ThisIsMissEm reviewed Nov 1, 2024

View reviewed changes

ransombriggs added 2 commits November 1, 2024 13:15

Clean up the error message for no challenge method

f042917

Using param value in error messages

d244e69

ThisIsMissEm reviewed Nov 1, 2024

View reviewed changes

ThisIsMissEm approved these changes Nov 1, 2024

View reviewed changes

ransombriggs added 3 commits November 4, 2024 09:08

Fix the invalid_code_challenge_method.zero message

7e2bf4b

Merge branch 'main' into update-pkce-error-message

a8ba995

Update CHANGELOG.md

45494da

ThisIsMissEm approved these changes Nov 4, 2024

View reviewed changes

nbulaj approved these changes Nov 6, 2024

View reviewed changes

nbulaj merged commit edceb80 into doorkeeper-gem:main Nov 6, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unknown pkce method error when configured #1747

Fix unknown pkce method error when configured #1747

ransombriggs commented Oct 31, 2024

ransombriggs Oct 31, 2024

ThisIsMissEm Nov 1, 2024 •

edited

Loading

ransombriggs Nov 1, 2024

ransombriggs Oct 31, 2024

ThisIsMissEm Nov 1, 2024

ransombriggs Nov 1, 2024

ThisIsMissEm Nov 1, 2024

ThisIsMissEm Nov 1, 2024

ransombriggs Nov 1, 2024

ThisIsMissEm Nov 1, 2024

ransombriggs Nov 1, 2024

ThisIsMissEm Nov 1, 2024

ransombriggs Nov 1, 2024

ThisIsMissEm Nov 1, 2024

ransombriggs Nov 1, 2024

ThisIsMissEm Nov 1, 2024 •

edited

Loading

ransombriggs Nov 4, 2024

ThisIsMissEm left a comment

ThisIsMissEm left a comment

nbulaj left a comment

	challenge_methods: Doorkeeper.config.pkce_code_challenge_methods_supported.join(", ")
	challenge_methods: Doorkeeper.config.pkce_code_challenge_methods_supported.join(", "),
	count: Doorkeeper.config.pkce_code_challenge_methods_supported.length

	zero: 'There are no acceptable code_challenge methods'
	zero: 'This authorization server does not support PKCE as there a no accepted code_challenge methods'

	zero: 'The authorization server does not support PKCE as there is no accepted code_challenge_method'
	zero: 'The authorization server does not support PKCE as there are no accepted code_challenge_method values.'

Fix unknown pkce method error when configured #1747

Fix unknown pkce method error when configured #1747

Conversation

ransombriggs commented Oct 31, 2024

Summary

Choose a reason for hiding this comment

ThisIsMissEm Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMissEm Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMissEm left a comment

Choose a reason for hiding this comment

ThisIsMissEm left a comment

Choose a reason for hiding this comment

nbulaj left a comment

Choose a reason for hiding this comment

ThisIsMissEm Nov 1, 2024 •

edited

Loading

ThisIsMissEm Nov 1, 2024 •

edited

Loading