Devise::SecretKeyFinder#find deprioritizes use of the deprecated find method in rails

[soartec-lab]

The rails/rails repository has deprecated rails.application.secrets due to this PR.

Devise::SecretKeyFinder#find sequentially examines the variables stored by searching secret_key_base, but deprecated Rails.application.secrets is searched first, so in many cases A warning will occur. This is noise in many cases.

DEPRECATION WARNING: Rails.application.secrets is deprecated in favor of Rails.application.credentials and will be removed in Rails 7.2. 

To fix this, I changed the search order so that the deprecated Rails.application.secrets doesn't take precedence.

[rafaelfranca]

Can you explain how did you expect to avoid this deprecation by moving it to the end? Does that means that in your application you have config.secret_key_base set but not credentials.secret_key_base?

[soartec-lab]

@rafaelfranca

OK. Specifically, if secret_key_base is specified in ENV["SECRET_KEY_BASE"] or config.secret_key_base, but not in secrets.secret_key_base.

In the current behavior, despite not using secrets.secret_key_base, in the second if statement (elsif @application.respond_to?(:secrets) && key_exists?(@application.secrets)) Since secrets.secret_key_base is accessed, the deprecation warning mentioned above will occur.

# Before

def find
  if @application.respond_to?(:credentials) && key_exists?(@application.credentials)
    @application.credentials.secret_key_base
  # ↓↓ deprecation warning in if statement
  elsif @application.respond_to?(:secrets) && key_exists?(@application.secrets)
    @application.secrets.secret_key_base
  # ↓↓ Actually get the value here
  elsif @application.config.respond_to?(:secret_key_base) && key_exists?(@application.config)
    @application.config.secret_key_base
  elsif @application.respond_to?(:secret_key_base) && key_exists?(@application)
    @application.secret_key_base
  end
end

However, by moving the execution order of the if statement (elsif @application.respond_to?(:secrets) && key_exists?(@application.secrets)) to the end, @application.config.secret_key_base, If the value can be obtained by @application.secret_key_base, the if statement will be exited in the middle, so the if statement that generates a warning will not be executed.

# After

def find
  if @application.respond_to?(:credentials) && key_exists?(@application.credentials)
    @application.credentials.secret_key_base
  # ↓↓ Get the value here and exit the if statement
  elsif @application.config.respond_to?(:secret_key_base) && key_exists?(@application.config)
    @application.config.secret_key_base
  elsif @application.respond_to?(:secret_key_base) && key_exists?(@application)
    @application.secret_key_base
  # ↓↓ Never reach this if statement where the deprecation warning is raised
  elsif @application.respond_to?(:secrets) && key_exists?(@application.secrets)
    @application.secrets.secret_key_base
  end
end

Since the above two acquisition methods are not deprecated, I thought that extra deprecation warnings should not be output.

[rgarver]

My first thought on this was to shift to a Rails version check with three checks in order:

1. secret_key_base on secrets/credentials depending on the rails version (it's deprecated which means secret exists, it's just not preferred and shouldn't be used. They both refer to the same credential storage system.
2. .config.secret_key_base
3. .secret_key_base

This ensures that the priority order is unaffected and respects the deprecations for the version.

def find
  credential_store = Rails.gem_version >= Gem::Version.new("7.2.x") ? :credential : :secrets
  if @application.respond_to?(credential_store) && key_exists?(@application.public_send(credential_store))
    @application.public_send(credential_store).secret_key_base
  elsif @application.config.respond_to?(:secret_key_base) && key_exists?(@application.config)
    @application.config.secret_key_base
  elsif @application.respond_to?(:secret_key_base) && key_exists?(@application)
    @application.secret_key_base
  end
end

[soartec-lab]

@rgarver
Certainly, I think it's a good change to change the reference to the rails version.
However, it seems that I can't solve the problem of extra warnings in rails 7.1, which I see as a problem, what do you think?

[rgarver]

A couple of things:

1. The mock application in the test/secret_key_finder_test, specifically Rails52Secrets doesn't include a #config definition and fails since the #secrets call happens after falling through the other options. You'll likely need to review that test file based on what your changes are. If you need to change something there you'll need to make it clear what the change is. My approach would be to treat the tests as descriptive of expected behavior that should not change since it could cause compatibility issues. Adding new tests to cover as-yet-undefined conditions would be fine.
2. I think my suggestion of doing the Rails version check may be unnecessary in Rails >= 5.2. I looked at the definition for Rails::Application#secret_key_base and it seems to be doing most of the fallback logic already. For Rails 4.1-5.1 we can have a simplified fallback. Eg:

def find
  if @application.respond_to?(:secret_key_base) && key_exists?(@application)
    @application.secret_key_base # Handles fallback logic since Rails 5.2
  elsif @application.respond_to?(:secrets) && key_exists?(@application.secrets)
    # Suppported since Rails 4.1
    @application.secrets.secret_key_base
  elsif @application.config.respond_to?(:secret_key_base) && key_exists?(@application.config)
    # Support really unusual configurations for backward compatibility
    @application.config.secret_key_base
  end
end

[dan-jensen]

@soartec-lab thanks for submitting this PR 👍

If you weren't aware, PR #5634 PR #5645 was submitted to solve the same problem you solved here. However, that one follows a different approach: it removes the Devise:: SecretKeyFinder class entirely. That reduces Devise's complexity and makes it more maintainable in the long-run. If that approach seems viable, would you be willing to close this PR so the community could focus on that one?

EDIT: Found a better PR.