-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Removed now obsolete SecretKeyFinder #5645
base: main
Are you sure you want to change the base?
Conversation
The best bugfix is one that removes code. :) |
Any news? |
For those looking for a workaround, you can set the secret key in your config.secret_key = Rails.application.secret_key_base and you won't see the deprecation warning anymore. |
Would be nice to have this published. cc @carlosantoniodasilva? 🙏 |
Known issue and waiting on PR merge and new release. In the meantime, we can apply this simple fix. ``` 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 /home/runner/work/demo.activeadmin.info/demo.activeadmin.info/config/environment.rb:5) ``` heartcombo/devise#5645 (comment)
Known issue and waiting on PR merge and new release. In the meantime, we can apply this simple fix. ``` 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 /home/runner/work/demo.activeadmin.info/demo.activeadmin.info/config/environment.rb:5) ``` heartcombo/devise#5645 (comment)
Known issue and waiting on PR merge and new release. In the meantime, we can apply this simple fix. ``` 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 /home/runner/work/demo.activeadmin.info/demo.activeadmin.info/config/environment.rb:5) ``` heartcombo/devise#5645 (comment)
* Add initial system tests This covers current functionality. Asserting redirects isn't available in system tests so we'll need to create integration tests instead for that assertion. * Switch to headless_chrome This avoids bringing up the actual browser when running tests and should also just work in CI too. * Disable admin user fixtures These cause errors since they have database constraints and we'd need to fill these out. For now we'll just have a helper to create a default admin user. * Add simplecov gem setup * Add tests CI workflow * Fix Rails secrets deprecation from Devise Known issue and waiting on PR merge and new release. In the meantime, we can apply this simple fix. ``` 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 /home/runner/work/demo.activeadmin.info/demo.activeadmin.info/config/environment.rb:5) ``` heartcombo/devise#5645 (comment)
Yes! I was just about to submit a similar change, but you went a step further. 👍 |
It is probably worth noting somewhere that this is a potentially breaking change. As I noted in #5634, Rails and Devise use a different priority order in what they choose. For certain old app configurations this could result in the key unintentionally changing. |
@albus522 good call. And we could even consider this a bugfix, because Devise shouldn't have been choosing a different key than the application. Seems like bumping the version to 4.10 and noting this breaking change in CHANGELOG.md would be sufficient. |
Even I'm using Rails credentials I got this deprecation warning from Devise, Rails secrets will be removed on Rails 7.2, and Devise still having it and generating a secure key. heartcombo/devise#5645 heartcombo/devise#5648
Even I'm using Rails credentials I got this deprecation warning from Devise, Rails secrets will be removed on Rails 7.2, and Devise still having it and generating a secure key. heartcombo/devise#5645 heartcombo/devise#5648
It looks like this PR lost momentum and still addresses an open issue. What's needed to move it forward? |
SecretKeyFinder was required to handle rails configuration pre 6.0 which is no longer supported. Secret key can (and should!) be now read directly from rails application.
Fixes: #5644
Probably surpasses: #5604