Skip to content

Commit

Permalink
Lazy evaluate Doorkeeper config when loading files and executing init…
Browse files Browse the repository at this point in the history
…ializers
  • Loading branch information
nbulaj committed Jan 30, 2023
1 parent a0a1e63 commit 53d70a8
Show file tree
Hide file tree
Showing 13 changed files with 114 additions and 72 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions lib/doorkeeper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
6 changes: 6 additions & 0 deletions lib/doorkeeper/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class << self

def configure(&block)
@config = Config::Builder.new(&block).build
setup
@config
end

# @return [Doorkeeper::Config] configuration instance
Expand All @@ -33,6 +35,10 @@ def configuration
@config || (raise MissingConfiguration)
end

def configured?
!@config.nil?
end

alias config configuration

def setup
Expand Down
10 changes: 6 additions & 4 deletions lib/doorkeeper/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions lib/doorkeeper/models/concerns/polymorphic_resource_owner.rb
Original file line number Diff line number Diff line change
@@ -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

11 changes: 10 additions & 1 deletion lib/doorkeeper/orm/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 0 additions & 6 deletions lib/doorkeeper/orm/active_record/mixins/access_grant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 0 additions & 4 deletions lib/doorkeeper/orm/active_record/mixins/access_token.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down
4 changes: 0 additions & 4 deletions lib/doorkeeper/orm/active_record/mixins/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 6 additions & 1 deletion lib/doorkeeper/rails/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
2 changes: 0 additions & 2 deletions spec/factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
33 changes: 12 additions & 21 deletions spec/lib/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -356,34 +356,26 @@
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
enable_application_owner

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
Expand All @@ -395,23 +387,21 @@
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
enable_application_owner confirmation: true

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
Expand Down Expand Up @@ -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
Expand Down
71 changes: 42 additions & 29 deletions spec/models/doorkeeper/application_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down Expand Up @@ -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
Expand All @@ -131,22 +120,14 @@ 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
expect(new_application).not_to be_valid
end

it "is valid with an owner" do
new_application.owner = @owner
new_application.owner = owner
expect(new_application).to be_valid
end
end
Expand Down Expand Up @@ -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
Expand All @@ -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

0 comments on commit 53d70a8

Please sign in to comment.