From 53d70a894e79aff5116cf47f030d329e635c859e Mon Sep 17 00:00:00 2001 From: Nikita Bulai Date: Thu, 12 Jan 2023 17:53:17 +0300 Subject: [PATCH 1/3] 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 From ee8547dd771e3dd4ae94c6c87923da162ac1f15d Mon Sep 17 00:00:00 2001 From: Nikita Bulai Date: Fri, 13 Jan 2023 15:34:30 +0300 Subject: [PATCH 2/3] [ci skip] --- Appraisals | 26 -------------------------- bin/console | 3 --- 2 files changed, 29 deletions(-) delete mode 100644 Appraisals diff --git a/Appraisals b/Appraisals deleted file mode 100644 index 9753d0fa6..000000000 --- a/Appraisals +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -appraise "rails-5-2" do - gem "rails", "~> 5.2.0" - gem "sqlite3", "~> 1.3", "< 1.4", platform: %i[ruby mswin mingw x64_mingw] -end - -appraise "rails-6-0" do - gem "rails", "~> 6.0.0" - gem "sqlite3", "~> 1.4", platform: %i[ruby mswin mingw x64_mingw] -end - -appraise "rails-6-1" do - gem "rails", "~> 6.1.0" - gem "sqlite3", "~> 1.4", platform: %i[ruby mswin mingw x64_mingw] -end - -appraise "rails-7-0" do - gem "rails", "~> 7.0.0alpha1" - gem "sqlite3", "~> 1.4", platform: %i[ruby mswin mingw x64_mingw] -end - -appraise "rails-main" do - gem "rails", git: "https://github.com/rails/rails" - gem "sqlite3", "~> 1.4", platform: %i[ruby mswin mingw x64_mingw] -end diff --git a/bin/console b/bin/console index 942c45c4d..019ffb0df 100755 --- a/bin/console +++ b/bin/console @@ -32,8 +32,5 @@ ActiveRecord::Base.establish_connection( # Load database schema load File.expand_path("../spec/dummy/db/schema.rb", __dir__) -# Call engine #to_prepare block -Doorkeeper.setup - require "irb" IRB.start(__FILE__) From 1e0a796536c66b637858813637c00a5f1238720d Mon Sep 17 00:00:00 2001 From: Nikita Bulai Date: Mon, 23 Jan 2023 20:37:31 +0300 Subject: [PATCH 3/3] Don't raise an error without configuration, use default config instead --- lib/doorkeeper.rb | 76 +++++++++++++++++++++-- lib/doorkeeper/config.rb | 76 ----------------------- lib/doorkeeper/config/abstract_builder.rb | 2 +- spec/lib/config_spec.rb | 15 ----- 4 files changed, 72 insertions(+), 97 deletions(-) diff --git a/lib/doorkeeper.rb b/lib/doorkeeper.rb index cd63e453f..6153e958c 100644 --- a/lib/doorkeeper.rb +++ b/lib/doorkeeper.rb @@ -114,11 +114,77 @@ module SecretStoring autoload :BCrypt, "doorkeeper/secret_storing/bcrypt" end - def self.authenticate(request, methods = Doorkeeper.config.access_token_methods) - OAuth::Token.authenticate(request, *methods) - end + class << self + attr_reader :orm_adapter + + def configure(&block) + @config = Config::Builder.new(&block).build + setup + @config + end + + # @return [Doorkeeper::Config] configuration instance + # + def configuration + @config || configure + end + + def configured? + !@config.nil? + end + + alias config configuration + + def setup + setup_orm_adapter + run_orm_hooks + config.clear_cache! + + # Deprecated, will be removed soon + unless configuration.orm == :active_record + setup_orm_models + setup_application_owner + end + end + + def setup_orm_adapter + @orm_adapter = "doorkeeper/orm/#{configuration.orm}".classify.constantize + rescue NameError => e + raise e, "ORM adapter not found (#{configuration.orm})", <<-ERROR_MSG.strip_heredoc + [DOORKEEPER] ORM adapter not found (#{configuration.orm}), or there was an error + trying to load it. - def self.gem_version - ::Gem::Version.new(::Doorkeeper::VERSION::STRING) + You probably need to add the related gem for this adapter to work with + doorkeeper. + ERROR_MSG + end + + def run_orm_hooks + if @orm_adapter.respond_to?(:run_hooks) + @orm_adapter.run_hooks + else + ::Kernel.warn <<~MSG.strip_heredoc + [DOORKEEPER] ORM "#{configuration.orm}" should move all it's setup logic under `#run_hooks` method for + the #{@orm_adapter.name}. Later versions of Doorkeeper will no longer support `setup_orm_models` and + `setup_application_owner` API. + MSG + end + end + + def setup_orm_models + @orm_adapter.initialize_models! + end + + def setup_application_owner + @orm_adapter.initialize_application_owner! + end + + def authenticate(request, methods = Doorkeeper.config.access_token_methods) + OAuth::Token.authenticate(request, *methods) + end + + def gem_version + ::Gem::Version.new(::Doorkeeper::VERSION::STRING) + end end end diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index a91513ba1..de2fbece4 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -5,87 +5,11 @@ require "doorkeeper/config/validations" module Doorkeeper - # Defines a MissingConfiguration error for a missing Doorkeeper configuration - # - class MissingConfiguration < StandardError - def initialize - super("Configuration for doorkeeper missing. Do you have doorkeeper initializer?") - end - end - # Doorkeeper option DSL could be reused in extensions to build their own # configurations. To use the Option DSL gems need to define `builder_class` method # that returns configuration Builder class. This exception raises when they don't # define it. # - class MissingConfigurationBuilderClass < StandardError; end - - class << self - attr_reader :orm_adapter - - def configure(&block) - @config = Config::Builder.new(&block).build - setup - @config - end - - # @return [Doorkeeper::Config] configuration instance - # - def configuration - @config || (raise MissingConfiguration) - end - - def configured? - !@config.nil? - end - - alias config configuration - - def setup - setup_orm_adapter - run_orm_hooks - config.clear_cache! - - # Deprecated, will be removed soon - unless configuration.orm == :active_record - setup_orm_models - setup_application_owner - end - end - - def setup_orm_adapter - @orm_adapter = "doorkeeper/orm/#{configuration.orm}".classify.constantize - rescue NameError => e - raise e, "ORM adapter not found (#{configuration.orm})", <<-ERROR_MSG.strip_heredoc - [DOORKEEPER] ORM adapter not found (#{configuration.orm}), or there was an error - trying to load it. - - You probably need to add the related gem for this adapter to work with - doorkeeper. - ERROR_MSG - end - - def run_orm_hooks - if @orm_adapter.respond_to?(:run_hooks) - @orm_adapter.run_hooks - else - ::Kernel.warn <<~MSG.strip_heredoc - [DOORKEEPER] ORM "#{configuration.orm}" should move all it's setup logic under `#run_hooks` method for - the #{@orm_adapter.name}. Later versions of Doorkeeper will no longer support `setup_orm_models` and - `setup_application_owner` API. - MSG - end - end - - def setup_orm_models - @orm_adapter.initialize_models! - end - - def setup_application_owner - @orm_adapter.initialize_application_owner! - end - end - class Config # Default Doorkeeper configuration builder class Builder < AbstractBuilder diff --git a/lib/doorkeeper/config/abstract_builder.rb b/lib/doorkeeper/config/abstract_builder.rb index 0ed4a586d..682ca2634 100644 --- a/lib/doorkeeper/config/abstract_builder.rb +++ b/lib/doorkeeper/config/abstract_builder.rb @@ -12,7 +12,7 @@ class AbstractBuilder # def initialize(config = Config.new, &block) @config = config - instance_eval(&block) + instance_eval(&block) if block_given? end # Builds and validates configuration. diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 662b86a78..c61ea55f5 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -501,21 +501,6 @@ class ApplicationWithOwner < ActiveRecord::Base end end - it "raises an exception when configuration is not set" do - old_config = Doorkeeper.config - Doorkeeper.module_eval do - @config = nil - end - - expect do - Doorkeeper.config - end.to raise_error Doorkeeper::MissingConfiguration - - Doorkeeper.module_eval do - @config = old_config - end - end - describe "access_token_generator" do it "is 'Doorkeeper::OAuth::Helpers::UniqueToken' by default" do expect(Doorkeeper.configuration.access_token_generator).to(