From 8c8c99eb2ccdfe293dc5485601d8718291a85a56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jun 2024 18:23:49 +0200 Subject: [PATCH 1/2] Correctly check permissions in locales controller We were using `authorize_resource`, passing it an unnamed parameter. When that happens, CanCanCan only checks permissions to read that resource. But, in this case, we want to check the permission to update that resource before the `update` action. Most of the time, it doesn't really matter, but, for example, in our demo we're going to restrict the locales configuration so locales cannot be updated on the main tenant (but they can be updated on other tenants). --- app/controllers/admin/locales_controller.rb | 2 +- .../admin/locales_controller_spec.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 spec/controllers/admin/locales_controller_spec.rb diff --git a/app/controllers/admin/locales_controller.rb b/app/controllers/admin/locales_controller.rb index df822a29b..290e549b2 100644 --- a/app/controllers/admin/locales_controller.rb +++ b/app/controllers/admin/locales_controller.rb @@ -1,6 +1,6 @@ class Admin::LocalesController < Admin::BaseController before_action :set_locales_settings - authorize_resource :locales_settings + authorize_resource instance_name: :locales_settings, class: "Setting::LocalesSettings" def show end diff --git a/spec/controllers/admin/locales_controller_spec.rb b/spec/controllers/admin/locales_controller_spec.rb new file mode 100644 index 000000000..c34928b06 --- /dev/null +++ b/spec/controllers/admin/locales_controller_spec.rb @@ -0,0 +1,17 @@ +require "rails_helper" + +describe Admin::LocalesController do + describe "PATCH update" do + it "checks permissions to update locales settings" do + user = create(:administrator).user + restricted_ability = user.ability.tap { |ability| ability.cannot :update, Setting::LocalesSettings } + + sign_in user + allow(controller).to receive(:current_ability).and_return(restricted_ability) + patch :update, params: { setting_locales_settings: { default: :es, enabled: [:en, :fr] }} + + expect(response).to redirect_to "/" + expect(Setting.default_locale).to eq :en + end + end +end From 12e49ff607c5b9136e63ce148ec711ddd074693b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jun 2024 18:45:23 +0200 Subject: [PATCH 2/2] Hide languages link when there's only one language Most existing Consul Democracy installations will have changed their `config.i18n.available_locales` option so only a few locales are available. In many cases, only one locale will be available. In these cases, rendering a form that only offers one option is useless. We've considered adding a text in this case mentioning that, in order to enable more languages, they need to configure their `config.i18n.available_locales`. However, we haven't done it for two reasons. First, if they've changed the available locales to just one, there's a good chance they aren't interested at all in configuring the locales. And, second, if there's only one available locale, administrators will learn to ignore the "languages" link, so they won't realize that locales can be configured if developers change the available locales. If we hide the link, on the other hand, they will notice that locales can now be configured once developers change the available locales. Note we're still allowing access by entering the URL. This is harmless, though, since people accessing it this way will see a form with only one possible option and won't be able to modify anything. --- app/components/admin/menu_component.rb | 2 +- spec/components/admin/menu_component_spec.rb | 24 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/components/admin/menu_component.rb b/app/components/admin/menu_component.rb index bbb9141ac..dba560278 100644 --- a/app/components/admin/menu_component.rb +++ b/app/components/admin/menu_component.rb @@ -462,7 +462,7 @@ class Admin::MenuComponent < ApplicationComponent settings_link, tenants_link, tags_link, - locales_link, + (locales_link if I18n.available_locales.many?), geozones_link, local_census_records_link ) diff --git a/spec/components/admin/menu_component_spec.rb b/spec/components/admin/menu_component_spec.rb index c8ca22c9c..b05683237 100644 --- a/spec/components/admin/menu_component_spec.rb +++ b/spec/components/admin/menu_component_spec.rb @@ -35,4 +35,28 @@ describe Admin::MenuComponent, controller: Admin::NewslettersController do expect(page).to have_css "[aria-current]", exact_text: "Polls" end end + + describe "#locales_link" do + it "is present when two or more locales are available" do + render_inline Admin::MenuComponent.new + + expect(page).to have_link "Languages" + end + + it "is present when two or more locales are available but only one is enabled" do + Setting["locales.enabled"] = "en" + + render_inline Admin::MenuComponent.new + + expect(page).to have_link "Languages" + end + + it "is not present when only one locale is available" do + allow(I18n).to receive(:available_locales).and_return([:en]) + + render_inline Admin::MenuComponent.new + + expect(page).not_to have_link "Languages" + end + end end