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

find auto-generated secret_key_base in development #4869

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
None yet
5 participants
@gencer
Contributor

gencer commented May 9, 2018

With this fix, we will try latest changes in Rails 5.2 together with standard auto-generated secret_key_base in development as a fallback.

If no specified key found, auto-generated value will be used instead.

This fixes #4864

@gencer

This comment has been minimized.

Contributor

gencer commented May 9, 2018

@tegon failed job is something about networking and/or i/o side on Travis. Maybe reschedule will fix the problem.

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

@tegon

This comment has been minimized.

Member

tegon commented May 9, 2018

@gencer thanks! Can you add a new test case for this?

@tegon tegon added the Need tests label May 9, 2018

@gencer

This comment has been minimized.

Contributor

gencer commented May 9, 2018

@tegon I've added test case for secret_key_base, squashed them and pushed back.

@ttilberg

This comment has been minimized.

ttilberg commented May 9, 2018

@tegon @gencer I referenced that branch from my Gemfile and it worked as expected.

The only thing I would suggest is the following consideration:

It seems to be that the Rails team views Rails.application.secret_key_base as the authoritative source for this information based on the fact that this method already aggregates the other configuration locations, in addition to the new dev/test default. As such, I think this should be the first location checked, with the others being the fallbacks to preserve backwards compatibility.

Even in the case where it does have to fall back, it's fewer checks and iterations happening as it's not going to be responding to that method in the first place.

Either way, this is successful in Rails 5.2.

Thanks for doing the PR on this. I understood what needed to be done, but am a little intimidated of the testing side of it.

end
def secret_key_base
'secret_key_base' if Rails.env.development? || Rails.env.test?

This comment has been minimized.

@tegon

tegon May 10, 2018

Member

Why do we need this if statement here?

This comment has been minimized.

@gencer

gencer May 10, 2018

Contributor

Oh! How did I missed that! I was testing something else and I just left them there. I'll fix and squash now.

P.S: Fixed.

find auto-generated secret_key_base in development
With this fix, we will try latest changes in Rails 5.2 together with standard auto-generated secret_key_base in development as a fallback.

If no specified key found, auto-generated value will be used instead.
@gencer

This comment has been minimized.

Contributor

gencer commented May 10, 2018

@tegon the line you were referring was absolutely unnecessary as its already in dev/test environment. I removed that in addition, I had one more blank line which I removed that too.

Squashed&Pushed now.

@tegon tegon merged commit 6c91648 into plataformatec:master May 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gencer gencer deleted the gencer:patch-2 branch May 15, 2018

@issei-m

This comment has been minimized.

issei-m commented Aug 2, 2018

Do you have a plan when this fix released is? I'm currently facing on this same problem on Devise 4.4.3.
Thanks.

@tegon

This comment has been minimized.

Member

tegon commented Aug 2, 2018

@issei-m it was released in 4.4.3. Please move the discussion to #4879, which is an open issue where we're checking this. Also, it'd be good if you provide us your Rails version and the output of the following commands:

Rails.application.credentials.secret_key_base

Rails.application.secrets.secret_key_base

Rails.application.config.secret_key_base
@issei-m

This comment has been minimized.

issei-m commented Aug 3, 2018

@Tagon Oh really? It seems to me, v4.4.3 is not applied changes introduced in this PR.
And I cloned this repository and I hit the some commands to know who has the merge commit right now, then only master branch seems to have, while v4.4.3 doesn't:

$ git branch -r --contains 6c91648
  origin/HEAD -> origin/master
  origin/master

$ git tag --contains 6c91648

@tegon

This comment has been minimized.

Member

tegon commented Aug 3, 2018

@issei-m Ouch, you're right. I've mistaken this with 962cea2, my bad. Thanks for pointing that out.
I'll see which commits we have unreleased and if there's something else to include in the next release and then bump a new version.
Just to make sure, if you use Devise from master it works, right?

@issei-m

This comment has been minimized.

issei-m commented Aug 7, 2018

@tegon Sorry for late response. I tried to checkout master branch, this problem seems to be gone. 🤓

@tegon

This comment has been minimized.

Member

tegon commented Aug 7, 2018

@issei-m thanks, I'll prepare a release as soon as I can.

@tegon

This comment has been minimized.

Member

tegon commented Aug 15, 2018

Released in v4.5.0

@swrobel

This comment has been minimized.

swrobel commented Aug 16, 2018

@gencer @tegon what about ttilberg's comment which points out the incongruity between Devise behavior & Rails.

For example, on Heroku, they set ENV['SECRET_KEY_BASE'] if it isn't already set for Rails apps. As a result, Rails ends up using that key instead of Rails.application.credentials.secret_key_base, although Devise will prefer the latter, resulting in different values for each.

@issei-m

This comment has been minimized.

issei-m commented Aug 16, 2018

@swrobel Simply, you can remove secret_key_base from your credentials.yml.inc, so ENV['SECRET_KEY_BASE'] will be used always in Heroku environment.

@swrobel

This comment has been minimized.

swrobel commented Aug 16, 2018

@issei-m I still don't get why devise should behave differently from Rails

@tegon

This comment has been minimized.

Member

tegon commented Aug 16, 2018

@swrobel Devise's secret_key_base doesn't necessarily need to be the same as Rails's. We only try to get it from Rails for convenience, so that you don't need to manually set it to get started.
When I looked at @ttilberg comment, I thought it was only a matter of order - e.g. where we look first for a key - I didn't realize that Rails looked for the environment variable too.
We can support this in a new version, but this PR was mostly about fixing a bug we introduced in a previous version. Would you mind opening an issue asking to support this?

@ttilberg

This comment has been minimized.

ttilberg commented Aug 16, 2018

@tegon My comment wasn't as much about "where to look first" for the sake of looking in many places, but instead a solution to leverage the configuration of modern rails apps while maintaining backwards compatibility. There's a few links to the relevant parts of the rails source in both my comment above and the original issue this closes that I thought were helpful to understand the "current rails configuration strategy" -- it felt a little all over the place but seems to get unified at Rails.application.secret_key_base accounting for all expected possibilities, including the new dynamic generation for dev/test.

What is the best way to ask for this support in a new issue? Something along the lines of "Feature: Prefer Rails default SECRET_KEY_BASE configuration?" It feels a bit superfluous now, but I think it's a good future-proofing thing to do.

@gencer

This comment has been minimized.

Contributor

gencer commented Aug 16, 2018

What I get so far that we need to also check ENV for SECRET_KEY_BASE (if possible at first) and regardless what we've found so far, we will use this Environment variable as default secret key.

Simply, ENV["SECRET_KEY_BASE"] has higher priority and overrides all (or If found, no need to check further places)

We need an another PR for this. I'll have a look in spare time (No promises) and create PR with tests (if no one contribute in this time of course). Hence, We still need an issue to track those changes.

@ttilberg

This comment has been minimized.

ttilberg commented Aug 16, 2018

Hey @gencer -- I was already in the middle of taking a stab at refactoring the key finder just now, I'll add that consideration (to check ENV first) as well and post a PR in a moment here. In addition, it was about checking the correct "default location in Rails" first, instead of last. Thanks for your help working through this!

@ttilberg

This comment has been minimized.

ttilberg commented Aug 16, 2018

@tegon @gencer Can you guys take a look at PR I just posted above? I don't have a lot of experience/confidence with contributing to such a serious project, so I'd appreciate feedback on:

  • Was this an appropriate way to contribute (going straight to PR without an issue, etc)
  • The refactor decisions

I added a test for the ENV addition @gencer mentioned, and all tests are still passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment