From 6bf628107c04846ee541f45672668757d880de32 Mon Sep 17 00:00:00 2001 From: Cesium Date: Mon, 23 Dec 2024 02:09:25 -0800 Subject: [PATCH 1/3] use permitted_attributes instead --- app/controllers/languages_controller.rb | 14 +------------- app/policies/language_policy.rb | 12 ++++++++++++ config/environments/test.rb | 2 +- config/locales/controllers/en.yml | 3 --- spec/controllers/languages_controller_spec.rb | 19 +++++++++---------- 5 files changed, 23 insertions(+), 27 deletions(-) diff --git a/app/controllers/languages_controller.rb b/app/controllers/languages_controller.rb index d56edce2313..ecaa6209b9a 100644 --- a/app/controllers/languages_controller.rb +++ b/app/controllers/languages_controller.rb @@ -30,20 +30,8 @@ def edit def update @language = Language.find_by(short: params[:id]) authorize @language - - if !policy(@language).can_edit_non_abuse_fields? && ((language_params[:name].present? && language_params[:name] != @language.name) || (language_params[:short].present? && @language.short != language_params[:short]) || (language_params[:sortable_name].present? && @language.sortable_name != language_params[:sortable_name]) || (language_params[:support_available].present? && @language.support_available != (language_params[:support_available] == "1"))) - flash[:error] = t("languages.update.non_abuse_field_error") - redirect_to languages_path - return - end - - if !policy(@language).can_edit_abuse_fields? && language_params[:abuse_support_available].present? && (@language.abuse_support_available != (language_params[:abuse_support_available] == "1")) - flash[:error] = t("languages.update.abuse_field_error") - redirect_to languages_path - return - end - if @language.update(language_params) + if @language.update(permitted_attributes(@language)) flash[:notice] = t("languages.successfully_updated") redirect_to languages_path else diff --git a/app/policies/language_policy.rb b/app/policies/language_policy.rb index 25b92d0189c..4ad836f4d66 100644 --- a/app/policies/language_policy.rb +++ b/app/policies/language_policy.rb @@ -10,6 +10,18 @@ def edit? user_has_roles?(LANGUAGE_EDIT_ACCESS) end + # Define which roles can update which attributes + ALLOWED_ATTRIBUTES_BY_ROLES = { + "superadmin" => [:name, :short, :support_available, :abuse_support_available, :sortable_name], + "translation" => [:name, :short, :support_available, :abuse_support_available, :sortable_name], + "support" => [:name, :short, :support_available, :sortable_name], + "policy_and_abuse" => [:abuse_support_available] + }.freeze + + def permitted_attributes + ALLOWED_ATTRIBUTES_BY_ROLES.values_at(*user.roles).compact.flatten + end + def can_edit_abuse_fields? user_has_roles?(%w[superadmin translation policy_and_abuse]) end diff --git a/config/environments/test.rb b/config/environments/test.rb index ab3fa19c3dd..4ea50106c28 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -53,7 +53,7 @@ config.active_support.deprecation = :stderr # Configure strong parameters to raise an exception if an unpermitted attribute is used - config.action_controller.action_on_unpermitted_parameters = :raise + config.action_controller.action_on_unpermitted_parameters = false # Make sure that we don't have a host mismatch: config.action_controller.default_url_options = { host: "http://www.example.com", port: nil } diff --git a/config/locales/controllers/en.yml b/config/locales/controllers/en.yml index b310e500791..5c3f82168b8 100644 --- a/config/locales/controllers/en.yml +++ b/config/locales/controllers/en.yml @@ -107,9 +107,6 @@ en: languages: successfully_added: Language was successfully added. successfully_updated: Language was successfully updated. - update: - abuse_field_error: Sorry, only an authorized admin can update the 'Abuse support available' field. - non_abuse_field_error: Sorry, only an authorized admin can update fields other than 'Abuse support available'. muted: users: create: diff --git a/spec/controllers/languages_controller_spec.rb b/spec/controllers/languages_controller_spec.rb index 2465a78995b..8ebc75b7cb6 100644 --- a/spec/controllers/languages_controller_spec.rb +++ b/spec/controllers/languages_controller_spec.rb @@ -174,7 +174,6 @@ name: "Suomi", short: "fi", support_available: "1", - abuse_support_available: "1", sortable_name: "" } } @@ -184,11 +183,7 @@ { id: finnish.short, language: { - name: "Suomi", - short: "fi", - support_available: "0", - abuse_support_available: "0", - sortable_name: "" + abuse_support_available: "0" } } end @@ -245,8 +240,10 @@ fake_login_admin(admin) put :update, params: language_params end - it "redirects with error" do - it_redirects_to_with_error(languages_path, "Sorry, only an authorized admin can update fields other than 'Abuse support available'.") + it "doesn't save changes to non-abuse field" do + finnish.reload + expect(finnish.support_available).to eq(false) + expect(finnish.abuse_support_available).to eq(false) end end @@ -277,8 +274,10 @@ fake_login_admin(admin) put :update, params: language_params end - it "redirects with error" do - it_redirects_to_with_error(languages_path, "Sorry, only an authorized admin can update the 'Abuse support available' field.") + it "doesn't save changes to abuse_support_available field" do + finnish.reload + expect(finnish.support_available).to eq(true) + expect(finnish.abuse_support_available).to eq(true) end end From 55f1b49dd04eb4b423ec329389331ff5b15f5696 Mon Sep 17 00:00:00 2001 From: Cesium Date: Mon, 23 Dec 2024 02:42:57 -0800 Subject: [PATCH 2/3] fix UTs --- config/environments/test.rb | 2 +- spec/controllers/languages_controller_spec.rb | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/config/environments/test.rb b/config/environments/test.rb index 4ea50106c28..ab3fa19c3dd 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -53,7 +53,7 @@ config.active_support.deprecation = :stderr # Configure strong parameters to raise an exception if an unpermitted attribute is used - config.action_controller.action_on_unpermitted_parameters = false + config.action_controller.action_on_unpermitted_parameters = :raise # Make sure that we don't have a host mismatch: config.action_controller.default_url_options = { host: "http://www.example.com", port: nil } diff --git a/spec/controllers/languages_controller_spec.rb b/spec/controllers/languages_controller_spec.rb index 8ebc75b7cb6..f445d1e410b 100644 --- a/spec/controllers/languages_controller_spec.rb +++ b/spec/controllers/languages_controller_spec.rb @@ -238,12 +238,13 @@ let(:admin) { create(:admin, roles: ["policy_and_abuse"]) } before do fake_login_admin(admin) - put :update, params: language_params end - it "doesn't save changes to non-abuse field" do + it "throws error and doesn't save changes to non-abuse field" do + expect do + put :update, params: language_params + end.to raise_exception(ActionController::UnpermittedParameters) finnish.reload expect(finnish.support_available).to eq(false) - expect(finnish.abuse_support_available).to eq(false) end end @@ -272,11 +273,12 @@ let(:admin) { create(:admin, roles: ["support"]) } before do fake_login_admin(admin) - put :update, params: language_params end - it "doesn't save changes to abuse_support_available field" do + it "throws error and doesn't save changes to abuse_support_available field" do + expect do + put :update, params: language_params + end.to raise_exception(ActionController::UnpermittedParameters) finnish.reload - expect(finnish.support_available).to eq(true) expect(finnish.abuse_support_available).to eq(true) end end From d6264c37ce128eecd083f594f5af510d54eda908 Mon Sep 17 00:00:00 2001 From: Brian Austin Date: Mon, 23 Dec 2024 09:15:43 -0500 Subject: [PATCH 3/3] Symbol list style --- app/policies/language_policy.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/policies/language_policy.rb b/app/policies/language_policy.rb index 4ad836f4d66..37810ec1863 100644 --- a/app/policies/language_policy.rb +++ b/app/policies/language_policy.rb @@ -12,10 +12,10 @@ def edit? # Define which roles can update which attributes ALLOWED_ATTRIBUTES_BY_ROLES = { - "superadmin" => [:name, :short, :support_available, :abuse_support_available, :sortable_name], - "translation" => [:name, :short, :support_available, :abuse_support_available, :sortable_name], - "support" => [:name, :short, :support_available, :sortable_name], - "policy_and_abuse" => [:abuse_support_available] + "superadmin" => %i[name short support_available abuse_support_available sortable_name], + "translation" => %i[name short support_available abuse_support_available sortable_name], + "support" => %i[name short support_available sortable_name], + "policy_and_abuse" => %i[abuse_support_available] }.freeze def permitted_attributes