heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
24.02k stars 5.55k forks source link

Prioritize letting the application find the secret_key_base #5634

Closed albus522 closed 10 months ago

albus522 commented 1 year ago

Starting in Rails 5.2 Rails.application.secret_key_base is available to find or create the secret key. By prioritizing letting Rails tell us what the secret key is, we can avoid being the trigger for the secrets deprecation warning generated in Rails 7.1. Also when devise triggers the deprecation, the warning is really hard to trace:

DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from <top (required)> at /app/config/environment.rb:5)

However, there is a potential for a breaking change here. Rails uses a different priority order for secret key lookup than this key finder, so it is possible for us to find a secret key base that is different from what the app is using. Rails will use ENV['SECRET_KEY_BASE'] over anything else, so if someone has a different key set in credentials or secrets, we are currently choosing a different key than the application.

alexpls commented 1 year ago

Thanks for raising this issue. I've started seeing this deprecation warning too since upgrading to Rails 7.1. In my development environment, the secret_key_base is stored in tmp/local_secret.txt, and in my production environment it's passed in via the SECRET_KEY_BASE environment variable.

When the SecretKeyFinder tries to resolve it, it always checks @application.secrets prior to @application.secret_key_base, leading to the deprecation warning being shown.

For now in my own codebase I've added this patch (🙈) into my initializers which silences the deprecation warning:

patched_version = '4.9.3'

unless Gem.loaded_specs['devise'].version == patched_version
  raise "Patch for Devise::SecretKeyFinder has not been tested with the " \
    "installed Devise version. Review whether it's still needed, and either " \
    "remove it or increment the patched_version."
end

# Patches Devise to skip using deprecated Application#secrets method.
# Can remove once https://github.com/heartcombo/devise/pull/5634 is resolved.
Devise::SecretKeyFinder.class_eval do
  def find
    @application.secret_key_base
  end
end
albus522 commented 1 year ago

This is now simplified to rely on app.secret_key_base since all supported rails versions use this.

albus522 commented 1 year ago

@alexpls less hacky workaround is to update your devise initalizer to set the secret key instead of waiting for Devise to find it. This is what I am currently doing in the app that initiated this PR.

Devise.setup do |config|
...
  config.secret_key = Rails.application.secret_key_base
...
end
dan-jensen commented 10 months ago

@albus522 with great respect for the work you did here, I think this PR should be closed as a duplicate of PR #5645. Would you be willing to close this PR so we can all focus on that one?

Reasons:

PS: I have no connection to PR #5645 or its author.