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.
The `issuer` setting was renamed to `sp_entity_id` in omniauth-saml [1],
and it's been deprecated in ruby-saml since version 1.11.0, released on
July 24, 2019 [2].
The ruby-saml code currently uses:
```
def sp_entity_id
@sp_entity_id || @issuer
end
```
So setting `issuer` to the same value as `sp_entity_id` if
`sp_entity_id` is present, as we were doing, has no effect.
On the other hand, neither omniauth-saml nor ruby-saml use the
`idp_metadata_url` and `idp_metadata` settings.
[1] https://github.com/omniauth/omniauth-saml/commit/74ed8dfb3aed
[2] https://github.com/SAML-Toolkits/ruby-saml/releases/tag/v1.11.0
When we first added OIDC support, we were configuring the redirect URI
in the devise initializer, just like we did for other providers.
Thanks to the changes in the previous commit, that code is no longer in
the devise initializer, which means we can use `url_helpers` to get the
redirect URI.
This means we no longer need to define this URI in the secrets. This is
particularly useful for multitenancy; previously, we had to define the
redirect URI for every tenant because different tenants use different
domains or different subdomains.
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.
We were using the `client_options` hash for the default tenant, defined
in the Devise initializer, but we forgot to include that key in the
multitenant code. This means OIDC wasn't working when different tenants
used different configurations.
Introduce a dedicated finder for partial results, reusing the logic
of PollOptionFinder. This will be used in rake tasks to avoid code
duplication and make the intent clearer.
- 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
Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.71.2 to 1.75.8.
- [Release notes](https://github.com/rubocop/rubocop/releases)
- [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rubocop/rubocop/compare/v1.71.2...v1.75.8)
---
updated-dependencies:
- dependency-name: rubocop
dependency-version: 1.75.8
dependency-type: direct:development
update-type: version-update:semver-minor
...
Notes:
This commit also includes several style and lint fixes required after
updating RuboCop:
- Removed redundant parentheses now detected by improved
'Style/RedundantParentheses' (1.72 and 1.75.3).
- Replaced ternary expressions with logical OR when the ternary was
returning 'true', as flagged by 'Style/RedundantCondition' (1.73).
- Adjusted block variables to resolve new 'Lint/ShadowingOuterLocalVariable'
offenses (1.75), helping avoid future conflicts during upgrades with
'rails app:updates'
Signed-off-by: dependabot[bot] <support@github.com>
We were getting an exception in this case, which was OK I guess since
this shouldn't happen if the application is used in a normal way, but we
can simplify the code a little bit if we make the `recipients` code
return an empty list of users.
Note that the behavior of the `AdminNotification#list_of_recipients` and
`Newsletter#list_of_recipient_emails` methods is now slightly different;
previously they returned `nil` when given an invalid segment recipient,
while now they return an empty array. I haven't found a place where this
change is relevant. For example, in both of these models, the `deliver`
method used to raise an exception when given an invalid segment while
now it doesn't, but we always check the user segment is valid before
calling the `deliver` method anyway, so it doesn't really affect the
application.
We were getting a warning by CodeQL regarding a possible code injection
in the `send(segment)` code.
In practice, this wasn't a big deal because the `self.recipients` method
is only called in the admin section, meaning only admin users could try
to take advantage of the code injection, and because this code is rarely
called with an invalid segment due to several conditions in the code
checking that the user segment is valid, with the only exception being
the `generate_csv` action in the `Admin::EmailsDownloadController`.
In any case, now we're checking that the segment is valid before calling
the `send` method. Since now we're making sure that the segment is valid
in the `recipients` method itself, we can remove this check from methods
calling it.
The default `date_select` used in fields presents an accessibility
issue, because in generates three select controls but only one label.
That means that there are two controls without a label.
So we're using a date field instead. This type is field is supported by
about 99% of the browsers, and we've already got JavaScript code
converting this field to a jQuery UI datepicker in case the browser
doesn't support date fields.
Note that, since we no longer need to parse the three date fields into
one, we can simplify the code in both the models and the tests.
Another slight improvement is that, previously, we couldn't restrict the
month and day controls in order to set the minimum date, so the maximum
selectable date was always the 31st of December of the year set by the
minimum age setting. As seen in the component test, now that we use only
one field, we can set a specific date as the maximum one.
There are many possible ways to implement this feature:
* Adding a custom middleware
* Using rack-attack with a blocklist
* Using routes constraints
We're choosing to use a controller concern with a redirect because it's
what we do to handle unauthorized cancancan exceptions.
This method was added in Rails 7.0 and makes the code slihgtly more
readable.
The downside is that it generates two queries instead of one, so it
might generate some confusion when debugging SQL queries. Its impact on
performance is probably negligible.
This rule was added in rubocop 1.64.0.
For clarity, in order to make it obvious that we're modifying the object
we received, we're excluding the Ahoy initializer, whose code was copied
from the Ahoy documentation.
We're also changing the `Types::BaseObject` class so we don't use a
variable with the same name as the parameter and we don't get a false
positive for this rule.
This rule was added in rubocop-rails 2.25.0. Applying it allows us to
simplify the code a little bit. For example, now there's no need to
specify the `proposals` table in proposal scopes, which was actually
causing a bug in the `Legislation::Proposal` model, which was using the
`proposals` table instead of the `legislation_proposals` table (but,
since we don't use this scope, it didn't affect the application).
Note that, for everything to work consistently, we need to make sure
that the default locale is one of the available locales.
Also note that we aren't overwriting the `#save ` method set by
globalize. I didn't feel too comfortable changing a monkey-patch which
ideally shouldn't be there in the first place, I haven't found a case
where `Globalize.locale` is `nil` (since it defaults to `I18n.locale`,
which should never be `nil`), so using `I18n.default_locale` probably
doesn't affect us.
We were defining one builder in the `app/lib/` folder and another one
inside a helper module.
So now we're grouping them together. This way we're following the "one
class per file" convention that we follow most of the time. And, by
extracting the `TranslatableFormBuilder` class to its own file, it'll be
easier to add tests for it.
Note that, for consistency, we're renaming the
`TranslationsFieldsBuilder` class so it ends in `FormBuilder`.
Back in commit 383909e16, we said:
> Even if this class looks very simple now, we're trying a few things
> related to these stats. Having a class for it makes future changes
> easier and, if there weren't any future changes, at least it makes
> current experiments easier.
Since there haven't been any changes in the last 5 years and we've found
cases where using the GeozoneStats class results in a slightly worse
performance, we're removing this class. The code is now a bit easier to
read, and is consistent with the way we calculate participants by age.
This reverts commit 383909e16.
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.
In Rails 6.1, the classic autoloader is deprecated.
We were getting an error because we were using `autoload` in the
ActiveStorage plugin for CKEditor:
expected file app/lib/ckeditor/backend/active_storage.rb to define
constant Ckeditor::Backend::ActiveStorage
So we're removing the line causing the error.
Finally, we can now restore all the tests that that failed sometimes
with the classic autoloader and that we modified in commits 2af1fc72f
and 8ba37b295.
In order for `include SentencesParser` to work with Zeitwerk, we'd have
to change the code slightly, so it follows Ruby conventions to resolve
constants:
```
module RemoteTranslations::Microsoft
class Client
include SentencesParser
# (...)
end
end
```
This would mean changing the indentation of the whole file. While we can
do that, changing the indentation of a file makes it harder to use
commands like `git blame` or `git log` with the file, so we're doing the
change the easy way.
We were getting a few errors when trying out Zeitwerk:
```
expected file lib/sms_api.rb to define constant SmsApi
expected file app/components/layout/common_html_attributes_component.rb
to define constant Layout::CommonHtmlAttributesComponent
```
In these cases, we aren't using an inflection because we also define the
`Verification::SmsController` and a few migrations containing `Html` in
their class name, and none of them would work if we defined the
inflection.
We were also getting an error regarding classes containing WYSIWYG in
its name:
```
NameError: uninitialized constant WYSIWYGSanitizer
Did you mean? WysiwygSanitizer
```
In this case, adding the acronym is easier, since we never use "Wysiwyg"
in the code but we use "WYSIWYG" in many places.
The purpose of the lib folder is to have code that doesn't necessary
belong in the application but can be shared with other applications.
However, we don't have other applications and, if we did, the way to
share code between them would be using a gem or even a git submodule.
So having both the `app/` and the `lib/` folders is confusing IMHO, and
it causes unnecessary problems with autoloading.
So we're moving the `lib/` folder to `app/lib/`. Originally, some of
these files were in the `app/services/` folder and then they were moved
to the `lib/` folder. We're using `app/lib/` instead of `app/services/`
so the upgrade is less confusing.
There's an exception, though. The `OmniAuth::Strategies::Wordpress`
class needs to be available in the Devise initializer. Since this is an
initializer and trying to autoload a class here will be problematic when
switching to Zeitwerk, we'll keep the `require` clause on top of the
Devise initializer in order to load the file and so it will be loaded
even if it isn't in the autoload paths anymore.
