Fixes #35432 - Use Rails 6.1 defaults #9748
Conversation
|
Issues: #35432 |
| # disabled in FIPS mode | ||
|
|
||
| require 'digest/sha1' | ||
| ActiveSupport::Digest.hash_digest_class = ::Digest::SHA1 |
There was a problem hiding this comment.
Ah, good catch. I didn't notice this one myself.
| # Rails 5.0 changed this to true, but a lot of code depends on this | ||
| config.active_record.belongs_to_required_by_default = false | ||
|
|
||
| # Rails 5.2 changed this to true, but we already do this in app/controllers/application_controller.rb#7 |
There was a problem hiding this comment.
Would it be better to remove it from there and rely on the defaults?
There was a problem hiding this comment.
I've added a comment there. I think it's better to leave as it is, so we have fewer changes until we actually decide to do them. Basically, this setting will add protect_from_forgery to ActionController::Base, which we use for ApplicationController, Api::BaseContoller and Api::GraphqlController. The Rails' default is the same what we have for ApplicationController, but we change the behaviour a bit for API related controllers.
ofedoren
force-pushed
the
feat-35432-load-rails-defaults
branch
3 times, most recently
from
a05d7d6
to
7a359de
Compare
| # changing default cipher from aes-256-cbc to aes-256-gcm. | ||
| # Leaving this disabled, since the application worked with aes-256-cbc. | ||
| # Failed tests on aes-256-gcm require revisit application to ensure we can do the switch. | ||
| config.active_support.use_authenticated_message_encryption = false |
There was a problem hiding this comment.
There is also use_authenticated_cookie_encryption setting, which it seems should be safer to enable (default), but I'm not quite sure, since we don't have a test for that.
WDYT, @ekohl ? Encryption is not my strongest part :/
There was a problem hiding this comment.
It's not a regression compared to today so we don't have to fix it right now. We should still fix it since cbc ciphers are considered weak nowadays so I'd be fine with opening a separate ticket for that.
|
Sorry for the noise, was going to undraft since the tests are green, but forgot that this PR contains a CPed commit just to fix the unrelated failures :/ |
|
I saw fog-vsphere 3.6.2 was released so perhaps you can just rebase and leave out the commit. Since I've seen it's green, I can also merge it with failing tests. |
ofedoren
force-pushed
the
feat-35432-load-rails-defaults
branch
from
7a359de
to
cc6257e
Compare
| config.active_record.belongs_to_required_by_default = false | ||
|
|
||
| # Rails 5.1 changed this to false, re-enabling this due to https://github.com/theforeman/foreman/pull/9711/files#r1247901552 | ||
| config.assets.unknown_asset_fallback = true |
There was a problem hiding this comment.
I've also added this one, see https://github.com/theforeman/foreman/pull/9711/files#r1247901552
|
Otherwise it seems like we can finally use at least some defaults. I couldn't check all the parts, but hammer seems to be working, hosts can be provisioned, the tests are green 🤷 |
|
Thanks for moving this forward! Even if we didn't use all the defaults, this is a great step forward. |
No description provided.