We were calling `parse_remote_to_hash` in the Devise initializer, which
runs when the application starts.
That meant that, if we got an exception when calling that method, the
application wouldn't start. We got exceptions if the single sign-on
(SSO) URL isn't available or we aren't providing the right credentials.
So we're moving the call to `parse_remote_to_hash` to
`OmniauthTenantSetup`, which is only called when actually trying to sign
in with SAML.
Since we're moving the code there, we're also unifying the code so SAML
settings are configured the same way for the main tenant and other
tenants, like we did for OpenID Connect in commit c3b523290.
In order to keep the existing behavior, we're caching the result of
`parse_remote_to_hash` in an instance variable. Not sure about the
advantages and disadvantages of doing so over parsing the remote URL
metadata on every SAML-related request.
Note that the SAML tests in `OmniauthTenantSetup` use the `stub_secrets`
method. But this method is called after the application has started,
meaning it doesn't stub calls to `Rails.application.secrets` in
`config/initializers/`. So, before this commit, the code that parsed the
IDP metadata URL wasn't executed in the tests. Since now we've moved the
code but we don't want to depend on external URLs when running the
tests, we need to stub the call to the external URL. Since we're now
stubbing the call, we're adding expectations in the tests to check that
we correctly use the settings returned in that call.
We were following the same pattern as we used for other providers like
twitter or facebook, but for OIDC we aren't passing the key and the
secret as separate attributes but only a hash of options. This means we
don't need to duplicate the same logic in the devise initializer and the
`OmniauthTenantSetup` class.
Thanks to these changes, we'll be able to introduce dynamic redirect
URLs for both the default tenant and the other tenants (see next commit).
Note that we could probably apply similar changes for the SAML provider.
We might do so in the future. For other providers, removing the
references to `Rails.application.secrets` broke their configuration when
we tested it back in 2022 as part of the multitenancy feature. We might
check whether that's no longer the case (or whether we made a mistake
during our tests in 2022) in the future.
- name: :oidc → Identifier for this login provider in the app.
- scope: [:openid, :email, :profile] → Tells the provider we want the user’s ID (openid), their email, and basic profile info (name, picture, etc.).
- response_type: :code → Uses Authorization Code Flow, which is more secure because tokens are not exposed in the URL.
- issuer: Rails.application.secrets.oidc_issuer → The base URL of the OIDC provider (e.g., Auth0). Used to find its config.
- discovery: true → Automatically fetches the provider’s endpoints from its discovery document instead of manually setting them.
- client_auth_method: :basic → Sends client ID and secret using HTTP Basic Auth when exchanging the code for tokens.
Add system tests for OIDC Auth
Edit the oauth docs to support OIDC auth
Without this change the IdpMetaParser would give an error
in the Devise initializer when starting the application.
I found it annoying to have to connect to the VPN so
I decided to add this condition.
Reviewer, feel free to consider this commit unnecessary
and ask to revert it.
We were having an issue because there was a difference of about 11
seconds between the local times of our machines and the time of the IDP
server. Since right now we can't guarantee the time of these machines is
fully synchronized, for now we're adding a margin of error of one
minute.
Ahoy 2.0.0 [1] introduced automatic UUID generation for visit_token and
visitor_token. As a result, the custom ensure_uuid method is no longer
needed and can be safely removed from the initializer.
Since we aren't manually generating UUIDs anymore, we no longer need
the uuidtools dependency.
[1] https://github.com/ankane/ahoy/blob/v2.0.0/README.md#token-generation
We're still using YAML to serialize the legislation_annotations ranges
column. I'm not sure whether changing the serializer can have
consequences on existing data, and I'm not sure which serializer we
should provide instead. Quoting the Rails configuration guide [1]:
> Unfortunately there isn't really any suitable defaults available in
> Ruby's standard library. JSON could work as a format, but the json
> gems will cast unsupported types to strings which may lead to bugs.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-active-record-default-column-serializer
I think this doesn't affect us because we use RSpec instead of Rails
test classes. In any case, if it ever affects us, we'll get notified
when a test fails.
As mentioned in the pull request introducing this change [1]:
> FATAL is documented in the Ruby Logger docs as being for "An
> unhandleable error that results in a program crash.", which does not
> really apply to this case since DebugExceptions is handling the error.
So we're using the new default value, which makes more sense.
[1] Pull request 48575 in https://github.com/rails/rails
Since we use a version of Loofah supporting HTML5 since db2d0bb80, the
`Rails::HTML::Sanitizer.best_supported_vendor` method will return the
HTML5 sanitizer. As mentioned in the pull request introducting this
change [1], the libxml2 maintainer wrote:
> it's still a bad idea to use a 20+ years old, unmaintained HTML 4
> parser to sanitize input for the modern web
So we're going with the new default sanitizer.
Note we aren't uncommenting the `action_text.sanitizer_vendor` option
because we don't use Action Text and so it doesn't affect us , and
uncommeting it will raise an error.
Also note we need to change one test because the new sanitizer handles
whitespace slightly differently.
[1] Pull request 48293 in https://github.com/rails/rails
Just like we mentioned in commit 001eee3d6, 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
It doesn't really affect us because we never use `return`, `break` or
`throw` inside transactions, since it would be confusing exactly because
it wouldn't be 100% clear whether the transaction is committed or not.
So we're using the new default value, which will be the only available
option in Rails 7.2 [1].
[1] Commit eccc6061f4 in https://github.com/rails/rails
This change doesn't affect us, since we don't use `after_commit`
callbacks, and, among our dependencies, AFAIK only the Devise gem uses
them, and it only defines one after_commit callback when creating a
record and another one when updating it, so we're never going to have
more than one callback being executed after a transaction is finished.
This doesn't really affect us because we don't use `before_committed`
callbacks (and neither do any of our dependencies), so we're using the
new default value.
This is done for performance reasons. Quoting the pull request
introducing this option [1]:
> A config might be overkill, but I wanted to provide an escape hatch
> for any upgraded apps that might be testing the exact value of the
> action_dispatch.parameter_filter header.
Since we don't test the exact value of action_dispatch.parameter_filter,
we can enable this option.
[1] Pull request 46452 in https://github.com/rails/rails
Before this change, every time we saved a record, the association was
validated if we had `belongs_to :something, required: true`. After this
change [1], it's only validated if the `something_id` column is nil (or
`something_type` for polymorphic associations) or if the `something_id`
attribute has changed.
The main difference is that we no longer get validation errors if the
associated record has been deleted. Doesn't affect us much, so we're
going with the new default value.
[1] Pull request 46522 in https://github.com/rails/rails
This is the new default option, and its only dangerous when deploying to
applications with multiple servers. Since this isn't our case, we can
enable it.
The new serializer can decrypt legacy messages using the `marshal`
serializer, so there's no risk of losing data when upgrading. Since we
aren't using applications with several servers, where upgrading some
servers might cause issues on the servers that aren't upgraded yet,
we're enabling the option.
[1] See comments in pull request 42846 in https://github.com/rails/rails
It doesn't really affect us (unless some of our dependencies make this
mistake) because we only use `expires_in/expires_at` once and we do it
correctly, but it might be help us detect this issue if we ever
introduce it in the future.
This option won't even exist in Rails 7.2 [1], and the possibility to
disable it was only added to guarantee safe upgrades in Rails
applications with multiple replicas [2].
Since we don't have applications with multiple replicas, where one
replica could be using Active Job 7.0 and another one could be using
Active Job 7.1 while upgrading, we can enable this options.
[1] Commit 2a761d23d2 in https://github.com/rails/rails
[2] Commit bc1f323338 in https://github.com/rails/rails
The only change between these headers and the ones sent by Rails 7.0
application is that the `"X-Download-Options" => "noopen"` is no longer
sent. Only Internet Explorer used that header, and uploading, previewing
and downloading attachments still works fine on Internet Explorer 11
after this change.
[1] Pull request 43968 in https://github.com/rails/rails
Quoting the Rails configuration guide [1]:
> applications running in :zeitwerk mode do not need require_dependency,
> so models, controllers, jobs, etc. do not need to be in $LOAD_PATH.
> Setting this to false saves Ruby from checking these directories when
> resolving require calls with relative paths, and saves Bootsnap work
> and RAM, since it does not need to build an index for them.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-add-autoload-paths-to-load-path
This is an error we've only been able to reproduce on one specific
machine and only when using the development environment.
It looks like Rails 7.1.5.1 uses `ActiveStorage::PreviewImageJob` when
we attach a PDF. However, that raises an IntegrityError because we're
removing the metadata from PDFs. That is, the final PDF is no longer the
same PDF that was uploaded.
This issue wasn't present in the original Rails 7.1.0 release, but was
introduced in Rails 7.1.4 [1] and has already been fixed in Rails 7.2.0
[2].
So we're applying the same solution that was applied in Rails 7.2.0:
disabling automatic previews for PDFs when no variants require them by
changing a method in `ActiveStorage::Attachment`.
[1] See commit 6f729dd39 in https://github.com/rails/rails/
[2] See pull request 51351 in https://github.com/rails/rails/
We're disabling `action_controller.raise_on_missing_callback_actions`
because sometimes we include `before_action :something, only: actions`
in concerns, and we include these concerns in controllers that don't
have all these actions.
Note that Rails 7.1 logs to STDOUT by default [1]; we're re-adding the
condition `if ENV["RAILS_LOG_TO_STDOUT"].present?` because we're still
using files and we're rotating the logs to avoid running out of space.
Also note that the GraphQL controller test (which is actually a request
test, since it's got `type: :request`) that was raising an exception no
longer does it thanks to the new default value for the
`config.action_dispatch.show_exceptions` configuration option. So we're
updating the test accordingly. This option doesn't affect regular
controller tests (without the `type: :request` option), so in other
tests we're still checking exceptions.
[1] Pull request 47138 in https://github.com/rails/rails
The "foundation_rails_helper" gem is no longer maintained and is
incompatible with Rails 7.1. To avoid blocking the upgrade, we've vendored
the vendor/foundation_rails_helper/form_builder.rb as a copy of the
original FormBuilder class.
To mantain compatibility with auto_labels and button_class variables, that
are used in the original builder, we are overwriting in the foundation
form builder initializer.
The gem has been removed from the Gemfile and replaced with this vendored
fallback. This workaround is safe to remove once legacy Foundation CSS
support is dropped.
All vendored code retains the original MIT license and attribution.
ros-apartment 3.0.0+ includes official support for connection handling in Rails 7,
so we no longer need to override `ActiveRecord::ConnectionHandling#connected_to`.
References: PR #194 and #243 in ros-apartment
These initializers either don't contain any code at all or contain code
that we never use. Since this was the case in most Rails applications,
they're no longer generated in new Rails applications [1].
So we're removing them as well.
[1] See pull requests 42538 and 43237 in https://github.com/rails/rails
In commit 64bcedc8b, we upgraded Sass from version 1.70 to version 1.80.
However, since then we've noticed a couple of inconvenient things.
First, we're getting some deprecation warnings when compiling the
assets, related to a usage of `map-has-key` in Foundation which is
deprecated since Sass 1.80. Even with the `quiet_deps` flag, these
warnings aren't silenced.
Second, version 1.79 changed the way color functions work [1]. In
particular, functions like `color.adjust` or `darken` would now generate
RGB colors that include float numbers [2]. Browser support for float values
in RGB colors is about 98% at the time of writing [3], meaning some
browsers that are at least 5 or 6 years old will not render these
colors, sometimes resulting in white text over a white background, which
is of course impossible to read.
Finally, we get some deprecation warnings in our code when we remove the
`quiet_deps` flag, caused by the breaking change in mixed declaration
[4] from version 1.77.7 [5]. This warning is tricky; consider the following
code:
```
@mixin normal-selection {
&::selection,
*::selection {
@include background-with-text-contrast($brand, brand, $check-invert-selection: false);
}
}
.button.hollow {
@include normal-selection;
border: 1px solid;
}
```
In this scenario, since normal-selection is a mixin that generates a
nested rule, we're doing a declaration after a nested rule, which is now
deprecated.
The situation gets even more complicated when we define mixins that have
both nested rules and rules that apply to the element itself. Currently,
we sometimes include a mixin and then override some of the properties
the mixin defines, but we wouldn't be able to do so if we can't define
properties after including the mixin.
Right now, the solution seems to be adding `& { }` selectors
after including a mixin, like this:
```
.button.hollow {
@include normal-selection;
& {
border: 1px solid;
}
}
```
Which is incredibly cumbersome.
So, for now, we're downgrading to version 1.77.5 (we would downgrade to
version 1.77.6, but the sass-embedded gem skipped that version). That
version was released in June 2024, so it isn't very old yet. In the
future, we'll see what to do about the issues mentioned above, since
we'll have to upgrade at some point.
Note we're removing the `silence_deprecations` flag because Sass 1.77
doesn't raise warnings about using `import`.
[1] https://sass-lang.com/documentation/breaking-changes/color-functions/
[2] https://sass-lang.com/documentation/modules/color/#adjust
[3] https://caniuse.com/mdn-css_types_color_rgb_float_values
[4] https://sass-lang.com/documentation/breaking-changes/mixed-decls/
[5] https://sass-lang.com/documentation/js-api/interfaces/deprecations/#mixed_decls
This code was added in commit b3f570512 in order to rotate existing
cookies used by Consul Democracy 2.1 and earlier. Since the code was
included in Consul Democracy 2.2, existing installation using Consul
Democracy 2.2 will have already rotated the old cookies, which means we
don't need the cookie rotator anymore.
In commit 96ae69fe9, we stopped using cookies to track Ahoy visits and
started using a combination of the IP and the browser agent instead.
However, since we're still using the legacy logic from Ahoy 1.x to track
visits (which we had to add in commit b5220effd), this way of tracking
visits doesn't work and counts every page visited by a user as an
independent visit.
Maybe we could migrate existing data, which uses the `visitor_id` column
so it uses the new `visit_token` and `visitor_token` columns, but
there's no mention in the Ahoy documentation regarding how to do so.
While deciding what to do about this, we found something interesting.
For two years, we've been seeing random failures in the
`system/admin/tenants_spec.rb` tests, with messages like:
```
1) Tenants Create Tenant with subdomain
Failure/Error:
raise TenantNotFound, <<~EXCEPTION_MESSAGE
Could not set search path to schemas, they may be invalid:
"#{tenant}" #{full_search_path}.
Original error: #{exception.class}: #{exception}
EXCEPTION_MESSAGE
Apartment::TenantNotFound:
Could not set search path to schemas, they may be invalid:
"earth" "public", "shared_extensions".
Original error:
ActiveRecord::StatementInvalid: Could not find schema earth
```
And we've found one of the causes: the AJAX requests done by Ahoy to
track visits. Sometimes a test that creates or updates a tenant finishes
but the Ahoy AJAX request to, say, `earth.lvh.me/ahoy/visits`, is
handled by the next test, when the `earth` schema no longer exists, thus
raising an `Apartment::TenantNotFound` exception.
So by disabling these AJAX requests and tracking the visits in the
server instead, we're killing two birds in one stone: we're fixing the
bug regarding the visits count and we're reducing the flakiness in our
test suite. It looks like we're also removing the "phantom ahoy cookie"
we were getting since the mentioned commit b5220effd: an ahoy cookie was
quickly set and unset in the browser.
Note that, even though we aren't migrating any data, we're still adding
the new fields, because some tests started to fail because, when
tracking visits in the server without cookies, Ahoy expects the Visit
model to have a `visit_token` field.
Note: Since we update to 1.80.1 deprecation warnings are appear when execute the assets:precompile command.
In order to silence this deprecation, we add silence_deprecation option in sass.rb initializer.
The code has also been updated to remove the deprecation warnings that appeared related to the function
darken(), lighten() and "Using / for division" instead of the function calc().
Bumps [sassc-embedded](https://github.com/sass-contrib/sassc-embedded-shim-ruby) from 1.70.1 to 1.80.1.
- [Commits](https://github.com/sass-contrib/sassc-embedded-shim-ruby/compare/v1.70.1...v1.80.1)
---
updated-dependencies:
- dependency-name: sassc-embedded
dependency-type: direct:production
update-type: version-update:semver-minor
...
Signed-off-by: dependabot[bot] <support@github.com>
Having a class named `Poll::Question::Answer` and another class named
`Poll::Answer` was so confusing that no developer working on the project
has ever been capable of remembering which is which for more than a few
seconds.
Furthermore, we're planning to add open answers to polls, and we might
add a reference from the `poll_answers` table to the
`poll_question_answers` table to property differentiate between open
answers and closed answers. Having yet another thing named answer would
be more than what our brains can handle (we know it because we did this
once in a prototype).
So we're renaming `Poll::Question::Answer` to `Poll::Question::Option`.
Hopefully that'll make it easier to remember. The name is also (more or
less) consistent with the `Legislation::QuestionOption` class, which is
similar.
We aren't changing the table or columns names for now in order to avoid
possible issues when upgrading (old code running with the new database
tables/columns after running the migrations but before deployment has
finished, for instance). We might do it in the future.
I've tried not to change the internationalization keys either so
existing translations would still be valid. However, since we have to
change the keys in `activerecord.yml` so methods like
`human_attribute_name` keep working, I'm also changing them in places
where similar keys were used (like `poll_question_answer` or
`poll/question/answer`).
Note that it isn't clear whether we should use `option` or
`question_option` in some cases. In order to keep things simple, we're
using `option` where we were using `answer` and `question_option` where
we were using `question_answer`.
Also note we're adding tests for the admin menu component, since at
first I forgot to change the `answers` reference there and all tests
passed.
As mentioned in Ahoy's README [1]:
> Ahoy provides a number of options to help with GDPR compliance.
> Update config/initializers/ahoy.rb with:
>
> class Ahoy::Store < Ahoy::DatabaseStore
> def authenticate(data)
> # disables automatic linking of visits and users
> end
> end
>
> Ahoy.mask_ips = true
> Ahoy.cookies = :none
As also mentioned in the README:
> If Ahoy was installed before v5, add an index before making this
> change.
> (...)
> For Active Record, create a migration with:
> add_index :ahoy_visits, [:visitor_token, :started_at]
However, the `visitor_token` doesn't exist in our table, since we
generated the `visits` table when Ahoy used the `visitor_id` column. So
we're using this column for the index.
Note we also need to change the `visit` method, since otherwise we get
an exception [2]. As mentioned on the issue reporting the exception:
> you'll need to copy the latest version of that method and adapt it to
> your model. I believe you'll want to replace:
>
> where(visit_token: ahoy.visit_token) with
> where(id: ensure_uuid(ahoy.visit_token))
>
> where(visitor_token: ahoy.visitor_token) with
> where(visitor_id: ensure_uuid(ahoy.visitor_token))
So we're copying the latest version of that method and changing it
accordingly.
[1] https://github.com/ankane/ahoy/blob/v5.0.2/README.md
[2] Issue 549 in https://github.com/ankane/ahoy
