Merge pull request #1646 from polleverywhere/confidential-skip-auth

Block public clients automatic authorization skip

Author: nbulaj

app/controllers/doorkeeper/authorizations_controller.rb

@@ -31,7 +31,7 @@ def destroy
     private
 
     def render_success
-      if skip_authorization? || matching_token?
+      if skip_authorization? || (matching_token? && pre_auth.client.application.confidential?)
         redirect_or_render(authorize_response)
       elsif Doorkeeper.configuration.api_only
         render json: pre_auth

spec/controllers/authorizations_controller_spec.rb

@@ -2,7 +2,7 @@
 
 require "spec_helper"
 
-RSpec.describe Doorkeeper::AuthorizationsController do
+RSpec.describe Doorkeeper::AuthorizationsController, type: :controller do
   include AuthorizationRequestHelper
 
   class ActionDispatch::TestResponse
@@ -736,6 +736,50 @@ def query_params
     end
   end
 
+  describe "GET #new with skip_authorization false" do
+    let(:params) do
+      {
+        client_id: client.uid,
+        response_type: "token",
+        redirect_uri: client.redirect_uri,
+      }
+    end
+
+    before do
+      allow(Doorkeeper.config.access_token_model).to receive(:matching_token_for).and_return(true)
+      client.update_attribute :confidential, confidential_client
+
+      get :new, params: params
+    end
+
+    context "with matching token and confidential application" do
+      let(:confidential_client) { true }
+
+      it "redirects immediately" do
+        expect(controller).not_to receive(:render)
+        expect(response).to be_redirect
+        expect(response.location).to match(/^#{client.redirect_uri}/)
+      end
+
+      it "issues a token" do
+        expect(Doorkeeper::AccessToken.count).to be 1
+      end
+    end
+
+    context "with matching token and non-confidential application" do
+      let(:confidential_client) { false }
+
+      it "renders the new view" do
+        expect(response).to be_successful
+        expect(controller).to render_with :new
+      end
+
+      it "doesn't issue a token" do
+        expect(Doorkeeper::AccessToken.count).to be 0
+      end
+    end
+  end
+
   describe "GET #new in API mode" do
     before do
       allow(Doorkeeper.config).to receive(:api_only).and_return(true)

spec/spec_helper.rb

@@ -30,6 +30,7 @@
 Doorkeeper::RSpec.print_configuration_info
 
 require "support/orm/#{DOORKEEPER_ORM}"
+require "support/render_with_matcher"
 
 Dir["#{File.dirname(__FILE__)}/support/{dependencies,helpers,shared}/*.rb"].sort.each { |file| require file }
 

spec/support/render_with_matcher.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+# Adds the `render_with` matcher.
+# Ex:
+#   expect(controller).to render_with(template: :show, locals: { alpha: "beta" })
+#
+module RenderWithMatcher
+  def self.included(base)
+    # Setup spying for our "render_with" matcher
+    base.before do
+      allow(controller).to receive(:render).and_wrap_original do |original, *args, **kwargs, &block|
+        original.call(*args, **kwargs, &block)
+      end
+    end
+  end
+
+  RSpec::Matchers.define :render_with do |expected|
+    match do |actual|
+      have_received(:render).with(expected).matches?(actual)
+    end
+  end
+end
+
+RSpec.configure do |config|
+  config.include RenderWithMatcher, type: :controller
+end