diff --git a/NEWS.md b/NEWS.md index d13bc829f..4a36c9266 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,6 +11,7 @@ User-visible changes worth mentioning. - [#1138] Revert regression bug (check for token expiration in Authorizations controller so authorization triggers every time) - [#1149] Fix for `URIChecker#valid_for_authorization?` false negative when query is blank, but `?` present. +- [#1151] Fix Refresh Token strategy: add proper validation of client credentials both for Public & Private clients. ## 5.0.0 diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index fb7222f61..2e361d1d9 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -107,7 +107,7 @@ def access_token_methods(*methods) def use_refresh_token(enabled = true, &block) @config.instance_variable_set( :@refresh_token_enabled, - block ? block : enabled + block || enabled ) end diff --git a/lib/doorkeeper/oauth/refresh_token_request.rb b/lib/doorkeeper/oauth/refresh_token_request.rb index 774f327e2..af80850d2 100644 --- a/lib/doorkeeper/oauth/refresh_token_request.rb +++ b/lib/doorkeeper/oauth/refresh_token_request.rb @@ -82,11 +82,17 @@ def validate_token end def validate_client - !credentials || !!client + return true if credentials.blank? + + client.present? end + # @see https://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-1.5 + # def validate_client_match - !client || refresh_token.application_id == client.id + return true if refresh_token.application_id.blank? + + client && refresh_token.application_id == client.id end def validate_scope diff --git a/spec/grape/grape_integration_spec.rb b/spec/grape/grape_integration_spec.rb index 8934b0b7b..8d5921c74 100644 --- a/spec/grape/grape_integration_spec.rb +++ b/spec/grape/grape_integration_spec.rb @@ -68,7 +68,7 @@ def app def json_body JSON.parse(last_response.body) end - + let(:client) { FactoryBot.create(:application) } let(:resource) { FactoryBot.create(:doorkeeper_testing_user, name: 'Joe', password: 'sekret') } let(:access_token) { client_is_authorized(client, resource) } diff --git a/spec/requests/flows/refresh_token_spec.rb b/spec/requests/flows/refresh_token_spec.rb index 8fcb61bfd..ad5852d36 100644 --- a/spec/requests/flows/refresh_token_spec.rb +++ b/spec/requests/flows/refresh_token_spec.rb @@ -6,6 +6,7 @@ orm DOORKEEPER_ORM use_refresh_token end + client_exists end @@ -95,6 +96,62 @@ end end + context "public & private clients" do + let(:public_client) do + FactoryBot.create( + :application, + confidential: false + ) + end + + let(:token_for_private_client) do + FactoryBot.create( + :access_token, + application: @client, + resource_owner_id: 1, + use_refresh_token: true + ) + end + + let(:token_for_public_client) do + FactoryBot.create( + :access_token, + application: public_client, + resource_owner_id: 1, + use_refresh_token: true + ) + end + + it 'issues a new token without client_secret when refresh token was issued to a public client' do + post refresh_token_endpoint_url( + client_id: public_client.uid, + refresh_token: token_for_public_client.refresh_token + ) + + new_token = Doorkeeper::AccessToken.last + should_have_json 'access_token', new_token.token + should_have_json 'refresh_token', new_token.refresh_token + end + + it 'returns an error without credentials' do + post refresh_token_endpoint_url(refresh_token: token_for_private_client.refresh_token) + + should_not_have_json 'refresh_token' + should_have_json 'error', 'invalid_grant' + end + + it 'returns an error with wrong credentials' do + post refresh_token_endpoint_url( + client_id: '1', + client_secret: '1', + refresh_token: token_for_private_client.refresh_token + ) + + should_not_have_json 'refresh_token' + should_have_json 'error', 'invalid_client' + end + end + it 'client gets an error for invalid refresh token' do post refresh_token_endpoint_url(client: @client, refresh_token: 'invalid') should_not_have_json 'refresh_token' diff --git a/spec/support/helpers/url_helper.rb b/spec/support/helpers/url_helper.rb index 48c4fd2f0..c4cd96815 100644 --- a/spec/support/helpers/url_helper.rb +++ b/spec/support/helpers/url_helper.rb @@ -2,9 +2,9 @@ module UrlHelper def token_endpoint_url(options = {}) parameters = { code: options[:code], - client_id: options[:client_id] || (options[:client] ? options[:client].uid : nil), - client_secret: options[:client_secret] || (options[:client] ? options[:client].secret : nil), - redirect_uri: options[:redirect_uri] || (options[:client] ? options[:client].redirect_uri : nil), + client_id: options[:client_id] || options[:client].try(:uid), + client_secret: options[:client_secret] || options[:client].try(:secret), + redirect_uri: options[:redirect_uri] || options[:client].try(:redirect_uri), grant_type: options[:grant_type] || 'authorization_code', code_verifier: options[:code_verifier], code_challenge_method: options[:code_challenge_method] @@ -15,10 +15,10 @@ def token_endpoint_url(options = {}) def password_token_endpoint_url(options = {}) parameters = { code: options[:code], - client_id: options[:client_id] || (options[:client] ? options[:client].uid : nil), - client_secret: options[:client_secret] || (options[:client] ? options[:client].secret : nil), - username: options[:resource_owner_username] || (options[:resource_owner] ? options[:resource_owner].name : nil), - password: options[:resource_owner_password] || (options[:resource_owner] ? options[:resource_owner].password : nil), + client_id: options[:client_id] || options[:client].try(:uid), + client_secret: options[:client_secret] || options[:client].try(:secret), + username: options[:resource_owner_username] || options[:resource_owner].try(:name), + password: options[:resource_owner_password] || options[:resource_owner].try(:password), scope: options[:scope], grant_type: 'password' } @@ -27,8 +27,8 @@ def password_token_endpoint_url(options = {}) def authorization_endpoint_url(options = {}) parameters = { - client_id: options[:client_id] || options[:client].uid, - redirect_uri: options[:redirect_uri] || options[:client].redirect_uri, + client_id: options[:client_id] || options[:client].try(:uid), + redirect_uri: options[:redirect_uri] || options[:client].try(:redirect_uri), response_type: options[:response_type] || 'code', scope: options[:scope], state: options[:state], @@ -41,8 +41,8 @@ def authorization_endpoint_url(options = {}) def refresh_token_endpoint_url(options = {}) parameters = { refresh_token: options[:refresh_token], - client_id: options[:client_id] || options[:client].uid, - client_secret: options[:client_secret] || options[:client].secret, + client_id: options[:client_id] || options[:client].try(:uid), + client_secret: options[:client_secret] || options[:client].try(:secret), grant_type: options[:grant_type] || 'refresh_token' } "/oauth/token?#{build_query(parameters)}"