In commit b3f570512, we changed the key generator hash digest class, and
we wrote:
> Since we haven't seen any Consul Democracy applications using
> encrypted messages and these messages become invalid with this change
> (...)
We didn't realize that ActiveStorage also used the old hash digest class
to generated the signed URLs used to access an image. This doesn't
affect us when we generate images using `image.variant`, because that
generates a new URL on the fly using the new hash digest class. However,
URLs referencing the images generated using the old hash digest class,
like the ones in the HTML content generated with CKEditor, would result
in 404 errors.
So we're rotating the signed IDs generated by earlier versions of
ActiveStorage. This way both new and old images will be correctly
displayed.
Note that, unlike cookies, which will keep working once rotated even if
we delete the code to rotate them, old ActiveStorage URLs will always
need the code rotating them in order to keep working.
Note that we used to have the link to delete images inside the same
<form> tag as the button to update the image. However, using a button
means we're adding a new <form> tag for the action to delete the image.
This isn't valid HTML and, in some browsers, might result in the button
sending the request to the wrong URL.
As explained in commit 5311daadf, to avoid this, we'd need to replace
`button_to` with `button_tag` in the action in order to generate a
button without a form. Then, we could add either a `form` or a
`formaction` attribute to the button.
However, I thik it's easier to move the delete button outside the update
button <form> tag. On the minus side, since the buttons no longer share
a parent, they're harder to style. So we're using a mix of nested flex
layouts with one of the nested elements using a container unit as width.
Since we're at it, we're also improving the styles on small and medium
screens by making sure the "Update" button wraps before the "Delete"
button does (using a container query), by giving enough width to the
column containing this actions on small screens as well (removing
`small-12` and giving it two-thirds of the width on all screen sizes)
and by having a gap between elements.
Note that, at the time of writing, container queries are only supported
by about 91%-93% of the browsers, meaning that some administrators will
see all from controls displayed vertically, one on top of the other, on
all screen sizes. We think this is acceptable, and the page remains
fully functional in this case.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, PUT) requests to the
server has a few issues.
We were already using buttons to destroy pages from the pages index.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
This way we can simplify setting the title and styling the link in the
header. We're also fixing the unnecessary padding introduced by the
`column` classes, which caused the header not to be aligned with the
rest of the elements surrounding it. We're still keeping it the margin
used in the `row` classes so it's aligned with the rest of the form;
ideally, we would remove the `row` classes in the rest of the form and
in the whole admin section, but this isn't something we can tackle right
now.
Note that, in the CSS, the `margin-left: auto` property needs to be
included after `@include regular-button` because that mixin overwrites
the `margin-left` property. Since we're modifying this code, we're
making it compatible with RTL text, using `$global-left` instead of
`left`.
We were already doing that when deleting content blocks from the index
page, and we also ask for confirmation in almost every page in the admin
section.
We were already using button to destroy content blocks from the content
blocks index.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
Note that the AJAX response stopped working after replacing the link
with a button. Not sure about the reason, but, since this is one of the
very few places where we use AJAX calls to delete content, the easiest
solution is to stop using AJAX and be consistent with what we do in the
rest of the admin section.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
Using the standard `confirm` parameter, we can remove all the custom
code we added to do the same thing.
Since the code is similar, we're doing the same when asking for
confirmation to send notifications.
We moved this file to `app/lib/` in commit cb477149c so it would be in
the autoload_paths. However, this class is loaded by ActiveStorage, with
the following method:
```
def resolve(class_name)
require "active_storage/service/#{class_name.to_s.underscore}_service"
ActiveStorage::Service.const_get(:"#{class_name.camelize}Service")
rescue LoadError
raise "Missing service adapter for #{class_name.inspect}"
end
``
So this file needs to be in the $LOAD_PATH, or else ActiveStorage won't
be able to load it when we disable the `add_autoload_paths_to_load_path`
option, which is the default in Rails 7.1 [1].
Moving it to the `lib` folder solves the issue; as mentioned in the
guide to upgrade to Rails 7.1 [2]:
> The lib directory is not affected by this flag, it is added to
> $LOAD_PATH always.
However, we were also referencing this class in the `Tenant` model,
meaning we needed to autoload it as well somehow. So, instead of
directly referencing this class, we're using `respond_to?` in the Tenant
model.
We're changing the test so it fails when the code calls
`is_a?(ActiveStorage::Service::TenantDiskService)`. We need to change
the active storage configurations in the test because, otherwise, the
moment `ActiveStorage::Blob` is loaded, the `TenantDiskService` class is
also loaded, meaning the test will pass when using `is_a?`.
Note that, since this class isn't in the autoload paths anymore, we need
to add a `require` in the tests. We could add an initializer to require
it; we're not doing it in order to be consistent with what ActiveStorage
does: it only loads the service that's going to be used in the current
Rails environment. If somebody changed their production environment in
order to use (for example), S3, and we added an initializer to require
the TenantDiskService, we would still load the TenantDiskService even if
it isn't going to be used.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-add-autoload-paths-to-load-path
[2] https://guides.rubyonrails.org/v7.1/upgrading_ruby_on_rails.html#autoloaded-paths-are-no-longer-in-$load-path
This way we simplify the code a little bit and we create a method unique
to the `TenantDiskService` class, which can be used to check whether
we're using this class without using `is_a?` or similar.
We're keeping the old `apply_stylesheet_media_default` option behavior
because removing `media="screen"` from our stylesheets would completely
break our `print` stylesheet, which would now load the default the
styles defined in `application.css`.
We're also keeping the old `:mini_magick` option to process images so
existing installations don't have to install libvips on their server. We
might change it in the future.
This option was deprecated in Rails 7.0 and removed in Rails 7.1 [1]. It
doesn't really affect us because we weren't using `to_s` with a
parameter anywhere in the application.
The Rubocop rule Rails/ToSWithArgument can be used to detect these cases
but, since we've never used them, and adding them now would cause the
application to crash and so it'll be obvious we've done something wrong,
I don't think it's necessary to add the rule.
[1] https://github.com/rails/rails/commit/e420c3380
This doesn't really affect us because we don'thave any multiple file
inputs in the application, but we're enabling it because it's the new
default configuration option.
Setting it to `true` was deprecated in Rails 7.0 and the option was was
removed in Rails 7.1, so in Rails 7.1 applications it isn't possible to
set it to `true` [1]. So we're setting it to `false` now.
[1] https://github.com/rails/rails/commit/689b27773
According to the Rails configuration guide [1], with this format, Rails
serializes cache entries more efficiently. Most importantly:
> All formats are backward and forward compatible, meaning cache entries
> written in one format can be read when using another format. This
> behavior makes it easy to migrate between formats without invalidating
> the entire cache.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-active-support-cache-format-version
The only change between these headers and the ones sent by Rails 6.1
application is that now the `X-XSS-Protection` header is set to zero. As
mentioned in the pull request introducing the change [1]:
> This header has been deprecated and the XSS auditor it triggered has
> been removed from all major modern browsers (in favour of Content
> Security Policy) that implemented this header to begin with (Firefox
> never did).
[1] Pull request 41769 in https://github.com/rails/rails
This configuration option disappeared in Rails 7.1 [1] (meaning it isn't
possible to set it to `false` in a Rails 7.1 application). Since it's
going to be our only option when upgrading to Rails 7.1, we're already
activating it now.
[1] https://github.com/rails/rails/commit/7b4affc78
As mentioned in the Rails configuration documentation [1] (note the link
points to the configuration guide for Rails 7.1, but only because the
documentation for this option wasn't as good in the configuration guide
for Rails 7.0; the behavior hasn't changed between these two versions),
this was done in the `wrap_parameters` initializer but now it can be
done using a new default configuration option.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-action-controller-wrap-parameters-by-default
This way we'll add an extra layer of protection from attacks that might
cause our application to redirect to an external host.
There's one place where we're allowing redirects to external hosts,
though: administrators can link external resources in notifications, and
we're redirecting to them after marking the notification as read.
Since the tests for the remote translations controller were
(accidentally) using an external redirect, we're updating them to use a
relative URL.
As mentioned in the Rails pull request [1], the main reason for partial
inserts is no longer relevant thanks to the `ignored_columns` method
(which we haven't even needed so far).
I don't have a preference regarding this setting; we're enabling it in
order to reduce the number of settings we customize.
[1] Pull request 42769 in https://github.com/rails/rails
It looks like we can't really benefit from this rule because usually we
need to specify the option anyway (maybe `user has_many :comments` is
one of the few exceptions). We might make some changes in the code when
Rubocop changes its Rails/InverseOf rule so it doesn't report this case
when using Rails 7, but, until then, we aren't changing anything so we
don't have to deal with false positives in Rubocop.
Before Rails 7.0 was released, neither the Mail gem or Rails were
providing a default timeout for SMTP, so there was a risk of processes
being stuck while sending emails.
That's no longer the case, though; we're using version 2.8.x of the Mail
gem, which already provides a default timeout [2].
Since the default timeout provided by the Mail gem is the same as the
default timeout provided by Rails 7.0, it doesn't matter whether we
enable this option. We're enabling because it's easier to just use the
default 7.0 configuration.
[1] Issue 41244 in https://github.com/rails/rails
[2] Pull request 1427 in https://github.com/mikel/mail
Not sure whether this affects us since we use RSpec; in any case, if it
affects us, it seems like a good idea, although we'll have to watch
whether some tests start failing more often.
We aren't getting any warnings when running our test suite, which means
that gems that depended on this method (like graphql [1]) have already
added compatibility for this case.
[1] Pull request 3774 in https://github.com/rmosolgo/graphql-ruby/
This is similar to what we did in commit 00a5dc921 when upgrading to
Rails 5.2. Quoting from that commit:
> Note this change will cause all fragment caching to expire. We
> consider it acceptable considering the page where caching is most
> important (stats) is barely affected by this change, since this change
> only affects the view, and the time-consuming operations are cached in
> the model.
>
> Comments are actually affected, though, and pages with thousands of
> comments might take a few extra seconds to load the first time they're
> accessed after this change. We don't think this is going to be an
> issue on existing CONSUL installations.
Note that enabling this options means all encrypted messages and cookies
generated the application become invalid, so we're adding a cookie
rotator in order to keep sessions from expiring when upgrading the
application, as recommended in the "Upgrading Ruby on Rails" guideline
[1].
Since we haven't seen any Consul Democracy applications using encrypted
messages and these messages become invalid with this change, we're also
removing the pre-Rails 5.2 encryption to authenticate messages
(AES-256-CBC) and switching to the default one since Rails 5.2
(AES-256-GCM). Since the configured encryption is used by the cookie
rotator initializer (through the ActiveSupport::MessageEncryptor.key_len
method), at first I thought this might affect the cookie rotator, but it
doesn't: upgrading works as expected, and existing sessions are still
active.
I'm adding a comment to remove the initializer once all cookies have
been migrated. I've added "Rails 7.1" in the comment because we usually
check for these comments when upgrading Rails, but we rarely check for
them when after releasing new versions of Consul Democracy.
[1] https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256
In Rails 6.1 and earlier, `button_to` generated a <button> tag when it
received the content as a block, but an <input> tag when receiving
the content as the first parameter.
That's why we were using blocks with `button_to` most of the time; for
starters, <button> tags accept pseudocontent and so are easier to style.
In Rails 7.0, `button_to` always generates a <button> tag [1], so we're
simplifying the code what uses `button_to`, passing the content as a
first parameter instead of passing it as a block.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-action-view-button-to-generates-button-tag
The config.file_watcher option still exists but it's no longer included
in the default environtment file. Since we don't use it, we're removing
it.
The config.assets.assets.debug option is no longer true by default [1],
so it isn't included anymore.
The config.active_support.deprecation option is now omitted on
production in favor of config.active_support.report_deprecations, which
is false by default. I think it's OK to keep it this way, since we check
deprecations in the development and test environments but never on
production environments.
As mentioned in the Rails upgrade guide, sprockets-rails is no longer a
rails dependency and we need to explicitly include it in our Gemfile.
The behavior of queries trying to find an invalid enum value has changed
[2], so we're updating the tests accordingly.
The `favicon_link_tag` method has removed the deprecated `shortcut`
link type [3], so we're updating the tests accordingly.
The method `raw_filter` in ActiveSupport callbacks has been renamed to
`filter` [4], so we're updating the code accordingly.
[1] https://github.com/rails/rails/commit/adec7e7ba87e3
[2] https://github.com/rails/rails/commit/b68f0954
[3] Pull request 43850 in https://github.com/rails/rails
[4] Pull request 41598 in https://github.com/rails/rails
