Prioritize letting the application find the secret_key_base

[albus522]

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.

[albus522]

I removed this because it is redundant. Both credentials and application.secret_key_base were added in Rails 5.2 and if the key is found in credentials it will be returned.

[grk]

Suggested change

	if @application.respond_to?(:secret_key_base) && key_exists?(@application)
	if @application.respond_to?(:secret_key_base)

I think if Rails version is recent enough to respond here, the method is guaranteed to return a value or raise.

[alexpls]

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

[tomascco]

Hi guys.

Thanks @albus522 for the good work. Now that #5600 has been merged, the minimum Rails version for main is 6.0. So I think we can simplify the whole find method to just:

def find
  @application.secret_key_base
end

or just delete the whole file, given that the only purpose of this class is to find the secret_key_base and this is not a problem anymore.

[dan-jensen]

@tomascco looks like your suggestion was applied so you can re-review 🙂

[albus522]

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

[albus522]

@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]

@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:

• These commits came later. Although this PR was created first, it originally followed a completely different approach (it was similar to, and possibly a duplicate of, PR #5604). This PR was re-written on Nov 17, 2023, and become a duplicate of PR #5645, which was committed Oct 26, 2023.
• This PR is less complete. (This does not remove the autoload line from lib/devise.rb.)

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