Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy evaluate Doorkeeper config when loading files and executing initializers #1627

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

nbulaj
Copy link
Member

@nbulaj nbulaj commented Jan 12, 2023

Root of the issue is the fact that Rails or some specific gems (like Sorbet) could load model files before initializers have been run. Since some models have constructions like:

class Some
  if Doorkeper.config.blabla
    # do smth
  end
end

in their class body - it will trigger an error because @config still doesn't exist (Doorkeeper.configure block wasn't called).

The same is applied to Doorkeeper railtie (engine): it adds some custom parameters filters based on the OAuth configuration, but it does it even for the new Rails apps which have not yet generated an initializer (just added a gem and run rails g doorkeeper:install).

What should be done:

  • Avoid checking config values in railtie initializers (config could be nil for Rails apps which just installed the gem. see above issues)
  • Avoid checking config in class definitions (move adding Polymorphic resource owner into an ORM hook)

Addresses:

@nbulaj nbulaj force-pushed the fixes/lazy-load-associations branch 2 times, most recently from c27b7ef to 0ba7896 Compare January 12, 2023 16:32
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|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double whitespace before do here

@@ -33,6 +35,10 @@ def configuration
@config || (raise MissingConfiguration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line still raises an error.

@nbulaj nbulaj changed the title Move polymorphic resource owner and application owner associations in a lazy ORM hooks Lazy evaluate Doorkeeper config when loading files Jan 13, 2023
@nbulaj nbulaj changed the title Lazy evaluate Doorkeeper config when loading files Lazy evaluate Doorkeeper config when loading files and executing initializers Jan 13, 2023
@nbulaj nbulaj force-pushed the fixes/lazy-load-associations branch 4 times, most recently from d361df7 to 8838264 Compare January 13, 2023 10:07
@nbulaj nbulaj force-pushed the fixes/lazy-load-associations branch from 8e907d9 to 1e0a796 Compare January 30, 2023 11:55
@nbulaj nbulaj merged commit 931c344 into main Jan 30, 2023
@nbulaj nbulaj deleted the fixes/lazy-load-associations branch January 30, 2023 11:59
@zavan
Copy link
Contributor

zavan commented Jan 30, 2023

This broke my initializer in development (I haven't tested in production):

Jan 30 12:08:52 ubuntu-jammy rails[186539]: /usr/local/lib/ruby/gems/3.2.0/gems/activesupport-7.0.4.2/lib/active_support/inflector/methods.rb:280:in `constantize': uninitialized constant DoorkeeperAccessToken (NameError)
Jan 30 12:08:52 ubuntu-jammy rails[186539]:       Object.const_get(camel_cased_word)
Jan 30 12:08:52 ubuntu-jammy rails[186539]:             ^^^^^^^^^^
Jan 30 12:08:52 ubuntu-jammy rails[186539]:         from /usr/local/lib/ruby/gems/3.2.0/gems/activesupport-7.0.4.2/lib/active_support/core_ext/string/inflections.rb:74:in `constantize'
Jan 30 12:08:52 ubuntu-jammy rails[186539]:         from /usr/local/lib/ruby/gems/3.2.0/gems/doorkeeper-5.6.3/lib/doorkeeper/config.rb:425:in `access_token_model'

I have in my initializer/config:

Doorkeeper.configure do
  # ...
  access_token_class 'DoorkeeperAccessToken'
  # ...
end

Rails v7.0.4.2

@nbulaj
Copy link
Member Author

nbulaj commented Jan 30, 2023

Yeah, can I see your DoorkeeperAccessToken implementation? Where it is located? How it looks like @zavan ?

@zavan
Copy link
Contributor

zavan commented Jan 30, 2023

Yeah, can I see your DoorkeeperAccessToken implementation? Where it is located? How it looks like @zavan ?

It's a regular rails model in app/models/doorkeeper_access_token.rb:

class DoorkeeperAccessToken < ApplicationRecord
  include ::Doorkeeper::Orm::ActiveRecord::Mixins::AccessToken

  after_save :sync_legacy_token

  private

  def sync_legacy_token
    #
  end
end

Downgrading to v5.6.2 works.

@nbulaj
Copy link
Member Author

nbulaj commented Jan 30, 2023

@zavan could you please try with fix-custom-models branch ?

@zavan
Copy link
Contributor

zavan commented Jan 30, 2023

@zavan could you please try with fix-custom-models branch ?

It works 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants