Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unknown pkce method error when configured #1747

Merged
merged 8 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this message in 7e2bf4b

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.'

Expand Down
15 changes: 14 additions & 1 deletion lib/doorkeeper/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ class DoorkeeperError < StandardError
def type
message
end

def self.translate_options
{}
end
end

class InvalidGrantReuse < DoorkeeperError
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions lib/doorkeeper/oauth/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/doorkeeper/oauth/error_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

state: request.try(:state),
redirect_uri: request.try(:redirect_uri),
),
Expand All @@ -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]
Expand Down
20 changes: 19 additions & 1 deletion spec/lib/oauth/error_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
27 changes: 27 additions & 0 deletions spec/lib/oauth/pre_authorization_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 is no accepted code_challenge_method"
)
end
end

context "when pkce_code_challenge_methods is set to only S256" do
before do
Doorkeeper.configure do
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions spec/requests/flows/authorization_code_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down