diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 109be527c..9094520c6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,7 +24,6 @@ jobs: - '3.0' - '3.1' - '3.2' - - truffleruby-head gemfile: - gemfiles/rails_6_0.gemfile - gemfiles/rails_6_1.gemfile diff --git a/CHANGELOG.md b/CHANGELOG.md index cd92b18d8..683d39a50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,14 @@ User-visible changes worth mentioning. Add your entry here. - [#????] DRY out AuthorizationsController#render_success +- [#1715] Fix token introspection invalid request reason +- [#1714] Fix `Doorkeeper::AccessToken.find_or_create_for` with empty scopes which raises NoMethodError +- [#1712] Add `Pragma: no-cache` to token response + +## 5.7.1 + +- [#1705] Add `force_pkce` option that requires non-confidential clients to use PKCE when requesting an access_token using an authorization code + ## 5.7.0 - [#1696] Add missing `#issued_token` method to `OAuth::TokenResponse` diff --git a/Gemfile b/Gemfile index a4d0f9ea2..f2cdb76b8 100644 --- a/Gemfile +++ b/Gemfile @@ -23,7 +23,7 @@ gem "rubocop-rspec", require: false gem "bcrypt", "~> 3.1", require: false gem "activerecord-jdbcsqlite3-adapter", platform: :jruby -gem "sqlite3", "~> 1.4", platform: %i[ruby mswin mingw x64_mingw] +gem "sqlite3", "~> 2.0", platform: %i[ruby mswin mingw x64_mingw] gem "tzinfo-data", platforms: %i[mingw mswin x64_mingw] gem "timecop" diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index 09fe87d21..277d427d0 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -113,6 +113,12 @@ def revoke_previous_authorization_code_token @config.instance_variable_set(:@revoke_previous_authorization_code_token, true) end + # Require non-confidential apps to use PKCE (send a code_verifier) when requesting + # an access_token using an authorization code (disabled by default) + def force_pkce + @config.instance_variable_set(:@force_pkce, true) + end + # Use an API mode for applications generated with --api argument # It will skip applications controller, disable forgery protection def api_only @@ -492,6 +498,10 @@ def revoke_previous_authorization_code_token? option_set? :revoke_previous_authorization_code_token end + def force_pkce? + option_set? :force_pkce + end + def enforce_configured_scopes? option_set? :enforce_configured_scopes end diff --git a/lib/doorkeeper/errors.rb b/lib/doorkeeper/errors.rb index 1e2e2046f..7832b0b5d 100644 --- a/lib/doorkeeper/errors.rb +++ b/lib/doorkeeper/errors.rb @@ -54,6 +54,7 @@ def self.name_for_response InvalidClient = Class.new(BaseResponseError) InvalidScope = Class.new(BaseResponseError) InvalidRedirectUri = Class.new(BaseResponseError) + InvalidCodeChallenge = Class.new(BaseResponseError) InvalidCodeChallengeMethod = Class.new(BaseResponseError) InvalidGrant = Class.new(BaseResponseError) diff --git a/lib/doorkeeper/models/access_token_mixin.rb b/lib/doorkeeper/models/access_token_mixin.rb index ec6e55224..2c1ce4ace 100644 --- a/lib/doorkeeper/models/access_token_mixin.rb +++ b/lib/doorkeeper/models/access_token_mixin.rb @@ -214,6 +214,8 @@ def custom_attributes_match?(token, custom_attributes) # @return [Doorkeeper::AccessToken] existing record or a new one # def find_or_create_for(application:, resource_owner:, scopes:, **token_attributes) + scopes = Doorkeeper::OAuth::Scopes.from_string(scopes) if scopes.is_a?(String) + if Doorkeeper.config.reuse_access_token custom_attributes = extract_custom_attributes(token_attributes).presence access_token = matching_token_for( diff --git a/lib/doorkeeper/oauth/authorization_code_request.rb b/lib/doorkeeper/oauth/authorization_code_request.rb index be9ba0dbb..4043552ed 100644 --- a/lib/doorkeeper/oauth/authorization_code_request.rb +++ b/lib/doorkeeper/oauth/authorization_code_request.rb @@ -59,10 +59,16 @@ def pkce_supported? Doorkeeper.config.access_grant_model.pkce_supported? end + def confidential? + client&.confidential + end + def validate_params @missing_param = if grant&.uses_pkce? && code_verifier.blank? :code_verifier + elsif !confidential? && Doorkeeper.config.force_pkce? && code_verifier.blank? + :code_verifier elsif redirect_uri.blank? :redirect_uri end diff --git a/lib/doorkeeper/oauth/client.rb b/lib/doorkeeper/oauth/client.rb index f0dc9ad3f..d3cfbbeca 100644 --- a/lib/doorkeeper/oauth/client.rb +++ b/lib/doorkeeper/oauth/client.rb @@ -5,7 +5,7 @@ module OAuth class Client attr_reader :application - delegate :id, :name, :uid, :redirect_uri, :scopes, to: :@application + delegate :id, :name, :uid, :redirect_uri, :scopes, :confidential, to: :@application def initialize(application) @application = application diff --git a/lib/doorkeeper/oauth/pre_authorization.rb b/lib/doorkeeper/oauth/pre_authorization.rb index 1c9fe8c0f..88f744bb4 100644 --- a/lib/doorkeeper/oauth/pre_authorization.rb +++ b/lib/doorkeeper/oauth/pre_authorization.rb @@ -14,6 +14,7 @@ class PreAuthorization validate :response_type, error: Errors::UnsupportedResponseType validate :response_mode, error: Errors::UnsupportedResponseMode validate :scopes, error: Errors::InvalidScope + validate :code_challenge, error: Errors::InvalidCodeChallenge validate :code_challenge_method, error: Errors::InvalidCodeChallengeMethod attr_reader :client, :code_challenge, :code_challenge_method, :missing_param, @@ -143,6 +144,12 @@ def validate_scopes ) end + def validate_code_challenge + return true unless Doorkeeper.config.force_pkce? + return true if client.confidential + code_challenge.present? + end + def validate_code_challenge_method return true unless Doorkeeper.config.access_grant_model.pkce_supported? diff --git a/lib/doorkeeper/oauth/token_introspection.rb b/lib/doorkeeper/oauth/token_introspection.rb index 4dca014ef..002c73cb0 100644 --- a/lib/doorkeeper/oauth/token_introspection.rb +++ b/lib/doorkeeper/oauth/token_introspection.rb @@ -6,7 +6,7 @@ module OAuth # # @see https://datatracker.ietf.org/doc/html/rfc7662 class TokenIntrospection - attr_reader :error + attr_reader :error, :invalid_request_reason def initialize(server, token) @server = server @@ -38,7 +38,6 @@ def to_json(*) private attr_reader :server, :token - attr_reader :invalid_request_reason # If the protected resource uses OAuth 2.0 client credentials to # authenticate to the introspection endpoint and its credentials are diff --git a/lib/doorkeeper/oauth/token_response.rb b/lib/doorkeeper/oauth/token_response.rb index a1d44b493..a31ba7c46 100644 --- a/lib/doorkeeper/oauth/token_response.rb +++ b/lib/doorkeeper/oauth/token_response.rb @@ -30,6 +30,7 @@ def headers { "Cache-Control" => "no-store, no-cache", "Content-Type" => "application/json; charset=utf-8", + "Pragma" => "no-cache", } end end diff --git a/lib/doorkeeper/version.rb b/lib/doorkeeper/version.rb index 4993b3466..59e1d5b3b 100644 --- a/lib/doorkeeper/version.rb +++ b/lib/doorkeeper/version.rb @@ -5,7 +5,7 @@ module VERSION # Semantic versioning MAJOR = 5 MINOR = 7 - TINY = 0 + TINY = 1 PRE = nil # Full version number diff --git a/lib/generators/doorkeeper/templates/initializer.rb b/lib/generators/doorkeeper/templates/initializer.rb index 16696cf1e..7341fbd3c 100644 --- a/lib/generators/doorkeeper/templates/initializer.rb +++ b/lib/generators/doorkeeper/templates/initializer.rb @@ -173,6 +173,11 @@ # # revoke_previous_authorization_code_token + # Require non-confidential clients to use PKCE when using an authorization code + # to obtain an access_token (disabled by default) + # + # force_pkce + # Hash access and refresh tokens before persisting them. # This will disable the possibility to use +reuse_access_token+ # since plain values can no longer be retrieved. diff --git a/spec/controllers/tokens_controller_spec.rb b/spec/controllers/tokens_controller_spec.rb index 012c18622..9d029314a 100644 --- a/spec/controllers/tokens_controller_spec.rb +++ b/spec/controllers/tokens_controller_spec.rb @@ -536,7 +536,7 @@ expect(json_response).to match( "error" => "invalid_request", - "error_description" => an_instance_of(String), + "error_description" => I18n.t("doorkeeper.errors.messages.invalid_request.request_not_authorized"), ) end end diff --git a/spec/lib/oauth/authorization_code_request_spec.rb b/spec/lib/oauth/authorization_code_request_spec.rb index 48a14487b..ae5cf66ab 100644 --- a/spec/lib/oauth/authorization_code_request_spec.rb +++ b/spec/lib/oauth/authorization_code_request_spec.rb @@ -170,6 +170,32 @@ end context "when using PKCE params" do + context "when force_pkce is enabled" do + before do + allow_any_instance_of(Doorkeeper::Config).to receive(:force_pkce?).and_return(true) + end + + context "when the app is confidential" do + it "issues a new token for the client" do + expect do + request.authorize + end.to change { client.reload.access_tokens.count }.by(1) + end + end + + context "when the app is not confidential" do + before do + client.update(confidential: false) + end + + it "does not issue a token" do + expect do + request.authorize + end.not_to change { client.reload.access_tokens.count } + end + end + end + context "when PKCE is supported" do before do allow(Doorkeeper::AccessGrant).to receive(:pkce_supported?).and_return(true) diff --git a/spec/lib/oauth/pre_authorization_spec.rb b/spec/lib/oauth/pre_authorization_spec.rb index 81820b4d5..9aa561ffd 100644 --- a/spec/lib/oauth/pre_authorization_spec.rb +++ b/spec/lib/oauth/pre_authorization_spec.rb @@ -40,6 +40,7 @@ response_type response_mode scopes + code_challenge code_challenge_method ]) end @@ -343,5 +344,49 @@ expect(pre_auth).to be_authorizable end end + + context "when force_pkce is enabled" do + before do + allow_any_instance_of(Doorkeeper::Config).to receive(:force_pkce?).and_return(true) + end + + context "when the app is confidential" do + before do + application.update(confidential: true) + end + + it "accepts a blank code_challenge" do + attributes[:code_challenge] = " " + + expect(pre_auth).to be_authorizable + end + + it "accepts a code challenge" do + attributes[:code_challenge] = "a45a9fea-0676-477e-95b1-a40f72ac3cfb" + attributes[:code_challenge_method] = "plain" + + expect(pre_auth).to be_authorizable + end + end + + context "when the app is not confidential" do + before do + application.update(confidential: false) + end + + it "does not accept a blank code_challenge" do + attributes[:code_challenge] = " " + + expect(pre_auth).not_to be_authorizable + end + + it "accepts a code challenge" do + attributes[:code_challenge] = "a45a9fea-0676-477e-95b1-a40f72ac3cfb" + attributes[:code_challenge_method] = "plain" + + expect(pre_auth).to be_authorizable + end + end + end end end diff --git a/spec/lib/oauth/token_response_spec.rb b/spec/lib/oauth/token_response_spec.rb index 70a7de0a0..f6afb8e37 100644 --- a/spec/lib/oauth/token_response_spec.rb +++ b/spec/lib/oauth/token_response_spec.rb @@ -8,6 +8,7 @@ it "includes access token response headers" do headers = response.headers expect(headers.fetch("Cache-Control")).to eq("no-store, no-cache") + expect(headers.fetch("Pragma")).to eq("no-cache") end it "status is ok" do diff --git a/spec/requests/endpoints/token_spec.rb b/spec/requests/endpoints/token_spec.rb index 6c1a824bd..898af80b1 100644 --- a/spec/requests/endpoints/token_spec.rb +++ b/spec/requests/endpoints/token_spec.rb @@ -17,6 +17,7 @@ expect(headers["Cache-Control"]).to be_in(["no-store", "no-cache, no-store", "private, no-store"]) expect(headers["Content-Type"]).to eq("application/json; charset=utf-8") + expect(headers["Pragma"]).to eq("no-cache") end it "accepts client credentials with basic auth header" do