Merge pull request #1151 from doorkeeper-gem/fix-refresh-token

Add proper client validation for Refresh Token strategy

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
 

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
 

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

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

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'

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)}"