Skip to content
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

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Conversation

BroiSatse
Copy link
Contributor

@BroiSatse BroiSatse commented Oct 20, 2023

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

@dgm
Copy link

dgm commented Nov 8, 2023

The best bugfix is one that removes code. :)

@issei-m
Copy link

issei-m commented Dec 29, 2023

Any news?

@grk
Copy link

grk commented Dec 29, 2023

For those looking for a workaround, you can set the secret key in your devise.rb initializer:

config.secret_key = Rails.application.secret_key_base

and you won't see the deprecation warning anymore.

@jordan-brough
Copy link

Would be nice to have this published. cc @carlosantoniodasilva? 🙏

javierjulio added a commit to activeadmin/demo.activeadmin.info that referenced this pull request Jan 6, 2024
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)
javierjulio added a commit to activeadmin/demo.activeadmin.info that referenced this pull request Jan 6, 2024
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)
javierjulio added a commit to activeadmin/demo.activeadmin.info that referenced this pull request Jan 6, 2024
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)
javierjulio added a commit to activeadmin/demo.activeadmin.info that referenced this pull request Jan 6, 2024
* 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)
@jcoyne
Copy link
Contributor

jcoyne commented Jan 19, 2024

Yes! I was just about to submit a similar change, but you went a step further. 👍

@jcoyne jcoyne mentioned this pull request Jan 19, 2024
@albus522
Copy link

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.

@dan-jensen
Copy link

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

JuanVqz added a commit to JuanVqz/doctors that referenced this pull request Mar 5, 2024
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
JuanVqz added a commit to JuanVqz/doctors that referenced this pull request Mar 5, 2024
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
@bbuchalter
Copy link

It looks like this PR lost momentum and still addresses an open issue. What's needed to move it forward?

@rebeccafae
Copy link

rebeccafae commented Oct 22, 2024

Bump. How can we get this in? Rails 7.1 is EOL for bug fixes, and Rails.application.secrets is removed Rails 7.2 preventing upgrade. I'm tagging a couple people that look like admins on this repo, but I am not sure.
@nashby @carlosantoniodasilva

@nashby nashby merged commit 5b15bbe into heartcombo:main Oct 22, 2024
@nashby
Copy link
Collaborator

nashby commented Oct 22, 2024

Thanks everyone!

mjankowski added a commit to mjankowski/mastodon that referenced this pull request Oct 23, 2024
We previously had overridden this to avoid a rails deprecation warning
in regard to how the secret key value is made available:

mastodon#28760

Devise recently merged a PR which totally removes the `SecretKeyFinder`:

heartcombo/devise#5645

With merge of this PR we will no longer see the deprecation noise, and
can go back to just using the devise default (which is the same value we
were configuring).
mjankowski added a commit to mjankowski/mastodon that referenced this pull request Nov 22, 2024
We previously had overridden this to avoid a rails deprecation warning
in regard to how the secret key value is made available:

mastodon#28760

Devise recently merged a PR which totally removes the `SecretKeyFinder`:

heartcombo/devise#5645

With merge of this PR we will no longer see the deprecation noise, and
can go back to just using the devise default (which is the same value we
were configuring).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation - Rails.application.secrets