From 53d70a894e79aff5116cf47f030d329e635c859e Mon Sep 17 00:00:00 2001 From: Nikita Bulai Date: Thu, 12 Jan 2023 17:53:17 +0300 Subject: [PATCH] Lazy evaluate Doorkeeper config when loading files and executing initializers --- CHANGELOG.md | 1 + lib/doorkeeper.rb | 1 + lib/doorkeeper/config.rb | 6 ++ lib/doorkeeper/engine.rb | 10 +-- .../concerns/polymorphic_resource_owner.rb | 30 ++++++++ lib/doorkeeper/orm/active_record.rb | 11 ++- .../orm/active_record/mixins/access_grant.rb | 6 -- .../orm/active_record/mixins/access_token.rb | 4 -- .../orm/active_record/mixins/application.rb | 4 -- lib/doorkeeper/rails/routes.rb | 7 +- spec/factories.rb | 2 - spec/lib/config_spec.rb | 33 ++++----- spec/models/doorkeeper/application_spec.rb | 71 +++++++++++-------- 13 files changed, 114 insertions(+), 72 deletions(-) create mode 100644 lib/doorkeeper/models/concerns/polymorphic_resource_owner.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 113578c0e..5b21d8cf2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ User-visible changes worth mentioning. - [#1626] Remove deprecated `active_record_options` config option. - [#1631] Fix regression with redirect behavior after token lookup optimizations (redirect to app URI when found). - [#1630] Special case unique index creation for refresh_token on SQL Server. +- [#1627] Lazy evaluate Doorkeeper config when loading files and executing initializers. ## 5.6.2 diff --git a/lib/doorkeeper.rb b/lib/doorkeeper.rb index c92a74038..cd63e453f 100644 --- a/lib/doorkeeper.rb +++ b/lib/doorkeeper.rb @@ -90,6 +90,7 @@ module Models autoload :Expirable, "doorkeeper/models/concerns/expirable" autoload :ExpirationTimeSqlMath, "doorkeeper/models/concerns/expiration_time_sql_math" autoload :Orderable, "doorkeeper/models/concerns/orderable" + autoload :PolymorphicResourceOwner, "doorkeeper/models/concerns/polymorphic_resource_owner" autoload :Scopes, "doorkeeper/models/concerns/scopes" autoload :Reusable, "doorkeeper/models/concerns/reusable" autoload :ResourceOwnerable, "doorkeeper/models/concerns/resource_ownerable" diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index 180863d48..a91513ba1 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -25,6 +25,8 @@ class << self def configure(&block) @config = Config::Builder.new(&block).build + setup + @config end # @return [Doorkeeper::Config] configuration instance @@ -33,6 +35,10 @@ def configuration @config || (raise MissingConfiguration) end + def configured? + !@config.nil? + end + alias config configuration def setup diff --git a/lib/doorkeeper/engine.rb b/lib/doorkeeper/engine.rb index 91294aa01..41a41b852 100644 --- a/lib/doorkeeper/engine.rb +++ b/lib/doorkeeper/engine.rb @@ -2,10 +2,12 @@ module Doorkeeper class Engine < Rails::Engine - initializer "doorkeeper.params.filter" do |app| - parameters = %w[client_secret authentication_token access_token refresh_token] - parameters << "code" if Doorkeeper.config.grant_flows.include?("authorization_code") - app.config.filter_parameters << /^(#{Regexp.union(parameters)})$/ + initializer "doorkeeper.params.filter", after: :load_config_initializers do |app| + if Doorkeeper.configured? + parameters = %w[client_secret authentication_token access_token refresh_token] + parameters << "code" if Doorkeeper.config.grant_flows.include?("authorization_code") + app.config.filter_parameters << /^(#{Regexp.union(parameters)})$/ + end end initializer "doorkeeper.routes" do diff --git a/lib/doorkeeper/models/concerns/polymorphic_resource_owner.rb b/lib/doorkeeper/models/concerns/polymorphic_resource_owner.rb new file mode 100644 index 000000000..e89ee02c3 --- /dev/null +++ b/lib/doorkeeper/models/concerns/polymorphic_resource_owner.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Doorkeeper + module Models + module PolymorphicResourceOwner + module ForAccessGrant + extend ActiveSupport::Concern + + included do + if Doorkeeper.config.polymorphic_resource_owner? + belongs_to :resource_owner, polymorphic: true, optional: false + else + validates :resource_owner_id, presence: true + end + end + end + + module ForAccessToken + extend ActiveSupport::Concern + + included do + if Doorkeeper.config.polymorphic_resource_owner? + belongs_to :resource_owner, polymorphic: true, optional: true + end + end + end + end + end +end + diff --git a/lib/doorkeeper/orm/active_record.rb b/lib/doorkeeper/orm/active_record.rb index deef164c3..8cfa49e34 100644 --- a/lib/doorkeeper/orm/active_record.rb +++ b/lib/doorkeeper/orm/active_record.rb @@ -29,7 +29,16 @@ module Mixins end def self.run_hooks - # nop + initialize_configured_associations + end + + def self.initialize_configured_associations + if Doorkeeper.config.enable_application_owner? + Doorkeeper.config.application_model.include ::Doorkeeper::Models::Ownership + end + + Doorkeeper.config.access_grant_model.include ::Doorkeeper::Models::PolymorphicResourceOwner::ForAccessGrant + Doorkeeper.config.access_token_model.include ::Doorkeeper::Models::PolymorphicResourceOwner::ForAccessToken end end end diff --git a/lib/doorkeeper/orm/active_record/mixins/access_grant.rb b/lib/doorkeeper/orm/active_record/mixins/access_grant.rb index b9ba92b6b..85c52beed 100644 --- a/lib/doorkeeper/orm/active_record/mixins/access_grant.rb +++ b/lib/doorkeeper/orm/active_record/mixins/access_grant.rb @@ -14,12 +14,6 @@ module AccessGrant optional: true, inverse_of: :access_grants - if Doorkeeper.config.polymorphic_resource_owner? - belongs_to :resource_owner, polymorphic: true, optional: false - else - validates :resource_owner_id, presence: true - end - validates :application_id, :token, :expires_in, diff --git a/lib/doorkeeper/orm/active_record/mixins/access_token.rb b/lib/doorkeeper/orm/active_record/mixins/access_token.rb index ebf98743d..300f80f9b 100644 --- a/lib/doorkeeper/orm/active_record/mixins/access_token.rb +++ b/lib/doorkeeper/orm/active_record/mixins/access_token.rb @@ -14,10 +14,6 @@ module AccessToken inverse_of: :access_tokens, optional: true - if Doorkeeper.config.polymorphic_resource_owner? - belongs_to :resource_owner, polymorphic: true, optional: true - end - validates :token, presence: true, uniqueness: { case_sensitive: true } validates :refresh_token, uniqueness: { case_sensitive: true }, if: :use_refresh_token? diff --git a/lib/doorkeeper/orm/active_record/mixins/application.rb b/lib/doorkeeper/orm/active_record/mixins/application.rb index 3c4d45fea..c2fab3594 100644 --- a/lib/doorkeeper/orm/active_record/mixins/application.rb +++ b/lib/doorkeeper/orm/active_record/mixins/application.rb @@ -10,10 +10,6 @@ module Application include ::Doorkeeper::ApplicationMixin - if Doorkeeper.config.enable_application_owner? - include ::Doorkeeper::Models::Ownership - end - has_many :access_grants, foreign_key: :application_id, dependent: :delete_all, diff --git a/lib/doorkeeper/rails/routes.rb b/lib/doorkeeper/rails/routes.rb index 842d7e14e..1a2bf7c48 100644 --- a/lib/doorkeeper/rails/routes.rb +++ b/lib/doorkeeper/rails/routes.rb @@ -36,7 +36,7 @@ def generate_routes!(options) map_route(:authorizations, :authorization_routes) map_route(:tokens, :token_routes) map_route(:tokens, :revoke_routes) - map_route(:tokens, :introspect_routes) unless Doorkeeper.config.allow_token_introspection.is_a?(FalseClass) + map_route(:tokens, :introspect_routes) if introspection_routes? map_route(:applications, :application_routes) map_route(:authorized_applications, :authorized_applications_routes) map_route(:token_info, :token_info_routes) @@ -100,6 +100,11 @@ def authorized_applications_routes(mapping) def native_authorization_code_route Doorkeeper.configuration.native_authorization_code_route end + + def introspection_routes? + Doorkeeper.configured? && + !Doorkeeper.config.allow_token_introspection.is_a?(FalseClass) + end end end end diff --git a/spec/factories.rb b/spec/factories.rb index 018aa1db2..53830cd03 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -22,8 +22,6 @@ factory :application, class: "Doorkeeper::Application" do sequence(:name) { |n| "Application #{n}" } redirect_uri { "https://app.com/callback" } - - factory :application_with_owner, class: "ApplicationWithOwner" end # do not name this factory :user, otherwise it will conflict with factories diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 6b5e6d8f1..662b86a78 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -356,17 +356,15 @@ end describe "enable_application_owner" do - let(:application_with_owner_class) do - Class.new(::ActiveRecord::Base) do - include Doorkeeper::Orm::ActiveRecord::Mixins::Application - end - end - it "is disabled by default" do expect(Doorkeeper.config.enable_application_owner?).not_to be(true) end context "when enabled without confirmation" do + class ApplicationWithOwner < ActiveRecord::Base + include Doorkeeper::Orm::ActiveRecord::Mixins::Application + end + before do Doorkeeper.configure do orm DOORKEEPER_ORM @@ -374,16 +372,10 @@ application_class "ApplicationWithOwner" end - - Object.const_set("ApplicationWithOwner", application_with_owner_class) - end - - after do - Object.send(:remove_const, :ApplicationWithOwner) end it "adds support for application owner" do - instance = FactoryBot.build(:application_with_owner) + instance = ApplicationWithOwner.new(FactoryBot.attributes_for(:application)) expect(instance).to respond_to :owner expect(instance).to be_valid @@ -395,6 +387,10 @@ end context "when enabled with confirmation set to true" do + class ApplicationWithOwner < ActiveRecord::Base + include Doorkeeper::Orm::ActiveRecord::Mixins::Application + end + before do Doorkeeper.configure do orm DOORKEEPER_ORM @@ -402,16 +398,10 @@ application_class "ApplicationWithOwner" end - - Object.const_set("ApplicationWithOwner", application_with_owner_class) - end - - after do - Object.send(:remove_const, :ApplicationWithOwner) end it "adds support for application owner" do - instance = FactoryBot.build(:application_with_owner) + instance = ApplicationWithOwner.new(FactoryBot.attributes_for(:application)) expect(instance).to respond_to :owner expect(instance).not_to be_valid @@ -624,7 +614,8 @@ end if DOORKEEPER_ORM == :active_record - class FakeCustomModel; end + class FakeCustomModel < ::ActiveRecord::Base + end describe "access_token_class" do it "uses default doorkeeper value" do diff --git a/spec/models/doorkeeper/application_spec.rb b/spec/models/doorkeeper/application_spec.rb index 54a80b552..5225d6349 100644 --- a/spec/models/doorkeeper/application_spec.rb +++ b/spec/models/doorkeeper/application_spec.rb @@ -4,12 +4,8 @@ require "bcrypt" RSpec.describe Doorkeeper::Application do - let(:application_with_owner_class) do - Class.new(::ActiveRecord::Base) do - include Doorkeeper::Orm::ActiveRecord::Mixins::Application - end - end let(:new_application) { FactoryBot.build(:application) } + let(:owner) { FactoryBot.build_stubbed(:doorkeeper_testing_user) } let(:uid) { SecureRandom.hex(8) } let(:secret) { SecureRandom.hex(8) } @@ -104,21 +100,14 @@ def self.generate end context "when application_owner is enabled" do - let(:new_application) { FactoryBot.build(:application_with_owner) } - context "when application owner is not required" do before do Doorkeeper.configure do orm DOORKEEPER_ORM enable_application_owner end - - Object.const_set("ApplicationWithOwner", application_with_owner_class) end - after do - Object.send(:remove_const, :ApplicationWithOwner) - end it "is valid given valid attributes" do expect(new_application).to be_valid @@ -131,14 +120,6 @@ def self.generate orm DOORKEEPER_ORM enable_application_owner confirmation: true end - - @owner = FactoryBot.build_stubbed(:doorkeeper_testing_user) - - Object.const_set("ApplicationWithOwner", application_with_owner_class) - end - - after do - Object.send(:remove_const, :ApplicationWithOwner) end it "is invalid without an owner" do @@ -146,7 +127,7 @@ def self.generate end it "is valid with an owner" do - new_application.owner = @owner + new_application.owner = owner expect(new_application).to be_valid end end @@ -506,21 +487,14 @@ def self.generate end context "when called with authorized resource owner" do - let(:owner) { FactoryBot.create(:doorkeeper_testing_user) } let(:other_owner) { FactoryBot.create(:doorkeeper_testing_user) } - let(:app) { FactoryBot.create(:application_with_owner, secret: "123123123", owner: owner) } + let(:app) { FactoryBot.create(:application, secret: "123123123", owner: owner) } before do Doorkeeper.configure do orm DOORKEEPER_ORM enable_application_owner confirmation: false end - - Object.const_set("ApplicationWithOwner", application_with_owner_class) - end - - after do - Object.send(:remove_const, :ApplicationWithOwner) end it "includes all the attributes" do @@ -538,4 +512,43 @@ def self.generate end end end + + context "when custom model class configured" do + class CustomApp < ::ActiveRecord::Base + include Doorkeeper::Orm::ActiveRecord::Mixins::Application + end + + let(:new_application) { CustomApp.new(FactoryBot.attributes_for(:application)) } + + context "without confirmation" do + before do + Doorkeeper.configure do + orm DOORKEEPER_ORM + application_class "CustomApp" + enable_application_owner confirmation: false + end + end + + it "is valid given valid attributes" do + expect(new_application).to be_valid + end + end + + context "without confirmation" do + before do + Doorkeeper.configure do + orm DOORKEEPER_ORM + application_class "CustomApp" + enable_application_owner confirmation: true + end + end + + it "is invalid without owner" do + expect(new_application).not_to be_valid + new_application.owner = owner + expect(new_application).to be_valid + end + end + + end end