Open soartec-lab opened 1 year ago
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
?
@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.
My first thought on this was to shift to a Rails version check with three checks in order:
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..config.secret_key_base
.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
@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?
A couple of things:
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.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
@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.
The
rails/rails
repository has deprecatedrails.application.secrets
due to this PR.Devise::SecretKeyFinder#find
sequentially examines the variables stored by searchingsecret_key_base
, but deprecatedRails.application.secrets
is searched first, so in many cases A warning will occur. This is noise in many cases.To fix this, I changed the search order so that the deprecated
Rails.application.secrets
doesn't take precedence.