From 3d9ae7aa091c74b170b82ab421d862122c0ee4fa Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:22:07 +0100 Subject: [PATCH 01/29] Migrate to strong parameters for usage limits --- .../admin/api/application_plan_metric_limits_controller.rb | 2 +- app/controllers/api/metric_visibilities_controller.rb | 4 ---- app/controllers/api/usage_limits_controller.rb | 2 +- app/models/usage_limit.rb | 1 - 4 files changed, 2 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/api/application_plan_metric_limits_controller.rb b/app/controllers/admin/api/application_plan_metric_limits_controller.rb index 75aafed67d..49a4d3ef99 100644 --- a/app/controllers/admin/api/application_plan_metric_limits_controller.rb +++ b/app/controllers/admin/api/application_plan_metric_limits_controller.rb @@ -61,7 +61,7 @@ def usage_limit end def usage_limit_params - params.fetch(:usage_limit) + params.fetch(:usage_limit).permit(:period, :value) end end diff --git a/app/controllers/api/metric_visibilities_controller.rb b/app/controllers/api/metric_visibilities_controller.rb index 48a1724e28..d461b07585 100644 --- a/app/controllers/api/metric_visibilities_controller.rb +++ b/app/controllers/api/metric_visibilities_controller.rb @@ -76,8 +76,4 @@ def find_metric def authorize_section authorize! :manage, :plans end - - def usage_limit_params - params.require(:usage_limit) - end end diff --git a/app/controllers/api/usage_limits_controller.rb b/app/controllers/api/usage_limits_controller.rb index fb1d497c3b..38aad3a17d 100644 --- a/app/controllers/api/usage_limits_controller.rb +++ b/app/controllers/api/usage_limits_controller.rb @@ -79,6 +79,6 @@ def authorize_action end def usage_limit_params - params.require(:usage_limit) + params.require(:usage_limit).permit(:period, :value) end end diff --git a/app/models/usage_limit.rb b/app/models/usage_limit.rb index 7adc72e2aa..d6c3fd6f04 100644 --- a/app/models/usage_limit.rb +++ b/app/models/usage_limit.rb @@ -1,6 +1,5 @@ class UsageLimit < ApplicationRecord include Symbolize - attr_accessible :period, :value, :metric, :plan include Backend::ModelExtensions::UsageLimit From d60b5c052bf40d97ea0076be51444f4dccef5a36 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:23:52 +0100 Subject: [PATCH 02/29] Migrate to strong parameters for access tokens --- app/models/access_token.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/access_token.rb b/app/models/access_token.rb index 7cdaeb0cb3..461396d7b0 100644 --- a/app/models/access_token.rb +++ b/app/models/access_token.rb @@ -106,8 +106,6 @@ def self.allowed_scopes after_initialize :generate_if_missing, if: :new_record? - attr_accessible :owner, :name, :scopes, :permission, :expires_at - def self.compute_digest(plaintext_value) return nil if plaintext_value.blank? From 6fc27bffa983de443d63ddbba3ad55dff698f4a0 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:25:13 +0100 Subject: [PATCH 03/29] Migrate to strong parameters for CMS redirects --- .../admin/cms/redirects_controller.rb | 7 +- app/models/cms/redirect.rb | 2 - .../admin/cms/redirects/index.html.erb | 4 +- .../admin/cms/redirects_controller_test.rb | 78 +++++++++++++++++++ 4 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 test/integration/provider/admin/cms/redirects_controller_test.rb diff --git a/app/controllers/provider/admin/cms/redirects_controller.rb b/app/controllers/provider/admin/cms/redirects_controller.rb index 2da32dfb18..e88f2dc16c 100644 --- a/app/controllers/provider/admin/cms/redirects_controller.rb +++ b/app/controllers/provider/admin/cms/redirects_controller.rb @@ -16,7 +16,7 @@ def edit end def create - @redirect = redirects.build(params[:cms_redirect]) + @redirect = redirects.build(redirect_params) if @redirect.save redirect_to({ action: :index }, success: t('.success')) @@ -26,7 +26,7 @@ def create end def update - if redirect.update(params[:cms_redirect]) + if redirect.update(redirect_params) redirect_to provider_admin_cms_redirects_path, success: t('.success') else render :edit @@ -48,4 +48,7 @@ def redirects @_redirects ||= current_account.redirects end + def redirect_params + @redirect_params ||= params.require(:cms_redirect).permit(:source, :target) + end end diff --git a/app/models/cms/redirect.rb b/app/models/cms/redirect.rb index 5a69d8d5c7..b763aab170 100644 --- a/app/models/cms/redirect.rb +++ b/app/models/cms/redirect.rb @@ -7,8 +7,6 @@ class CMS::Redirect < ApplicationRecord validates :source, uniqueness: { scope: [:provider_id], case_sensitive: true }, length: { maximum: 255 } validates :target, length: { maximum: 255 } - attr_accessible :source, :target - include NormalizePathAttribute verify_path_format :source, :target end diff --git a/app/views/provider/admin/cms/redirects/index.html.erb b/app/views/provider/admin/cms/redirects/index.html.erb index 8e32257606..7b6b5fe5da 100644 --- a/app/views/provider/admin/cms/redirects/index.html.erb +++ b/app/views/provider/admin/cms/redirects/index.html.erb @@ -20,8 +20,8 @@ <% @redirects.each do |redirect| %> - <%= link_to h(redirect.source), edit_provider_admin_cms_redirect_path(redirect) %> - <%= redirect.target %> + <%= link_to h(redirect.source), edit_provider_admin_cms_redirect_path(redirect) %> + <%= redirect.target %>
diff --git a/test/integration/provider/admin/cms/redirects_controller_test.rb b/test/integration/provider/admin/cms/redirects_controller_test.rb new file mode 100644 index 0000000000..e044d57b64 --- /dev/null +++ b/test/integration/provider/admin/cms/redirects_controller_test.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Provider::Admin::CMS::RedirectsControllerTest < ActionDispatch::IntegrationTest + def setup + @provider = FactoryBot.create(:provider_account) + login! provider + end + + attr_reader :provider + + test 'index lists all redirects' do + provider.redirects.create!(source: '/one/from', target: '/one/to') + provider.redirects.create!(source: '/two/from', target: '/two/to') + + get provider_admin_cms_redirects_path + assert_response :success + + page = Nokogiri::HTML4::Document.parse(response.body) + rows = page.xpath('//tbody[@role="rowgroup"]/tr[@role="row"]') + assert_equal 2, rows.size + + from_values = page.xpath('//td[@data-label="From"]/a').map { |element| element.text.strip } + to_values = page.xpath('//td[@data-label="To"]').map { |element| element.text.strip } + + assert_same_elements %w[/one/from /two/from], from_values + assert_same_elements %w[/one/to /two/to], to_values + end + + test 'create a new redirect' do + assert_empty provider.redirects + assert_difference 'CMS::Redirect.count', 1 do + post provider_admin_cms_redirects_path, params: { + cms_redirect: { + source: '/source', + target: '/target' + } + } + end + + assert_redirected_to provider_admin_cms_redirects_path + assert_equal 'Redirect created', flash[:success] + + redirect = provider.redirects.first + assert_equal '/source', redirect.source + assert_equal '/target', redirect.target + end + + test 'update a redirect' do + redirect = provider.redirects.create!(source: '/from', target: '/to') + + put provider_admin_cms_redirect_path(redirect), params: { + cms_redirect: { + source: '/new-from', + target: '/new-to' + } + } + + assert_redirected_to provider_admin_cms_redirects_path + redirect.reload + + assert_equal '/new-from', redirect.source + assert_equal '/new-to', redirect.target + end + + test 'delete redirect' do + redirect = provider.redirects.create!(source: '/from', target: '/to') + assert_equal 1, provider.redirects.count + + assert_difference 'CMS::Redirect.count', -1 do + delete provider_admin_cms_redirect_path(redirect) + end + + assert_redirected_to provider_admin_cms_redirects_path + assert_equal 0, provider.redirects.count + end +end From 4d435b2a60863fb08f4e90f8c7228c63977da03d Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:25:50 +0100 Subject: [PATCH 04/29] Migrate to strong parameters for CMS groups --- .../provider/admin/cms/groups_controller.rb | 16 +++------------- app/models/cms/group.rb | 2 -- .../provider/admin/cms/groups_controller_test.rb | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/controllers/provider/admin/cms/groups_controller.rb b/app/controllers/provider/admin/cms/groups_controller.rb index 53b85c3026..ccf300aaa1 100644 --- a/app/controllers/provider/admin/cms/groups_controller.rb +++ b/app/controllers/provider/admin/cms/groups_controller.rb @@ -53,28 +53,18 @@ def destroy redirect_to({ action: :index }, success: t('.success')) end - - protected def sections_params - params[:cms_group].dup.tap do |params| - if section_ids = params[:section_ids].presence - section_ids.reject!(&:empty?) - params[:sections] = section_ids.map do |sec_id| - current_account.sections.find(sec_id.to_i) - end - end - end - + params.require(:cms_group).permit(:name, section_ids: []) end def available_groups - @available_groups= current_account.provided_groups + @available_groups = current_account.provided_groups end def available_sections - @available_sections= current_account.provided_sections + @available_sections = current_account.provided_sections end def authorize_groups diff --git a/app/models/cms/group.rb b/app/models/cms/group.rb index 3894acb668..91079d32d0 100644 --- a/app/models/cms/group.rb +++ b/app/models/cms/group.rb @@ -1,6 +1,4 @@ class CMS::Group < ApplicationRecord - attr_accessible :provider, :sections, :name, :accounts - # This is BuyerGroup self.table_name = :cms_groups diff --git a/test/integration/provider/admin/cms/groups_controller_test.rb b/test/integration/provider/admin/cms/groups_controller_test.rb index 22f201536d..943a558869 100644 --- a/test/integration/provider/admin/cms/groups_controller_test.rb +++ b/test/integration/provider/admin/cms/groups_controller_test.rb @@ -17,5 +17,19 @@ def setup g = @provider.provided_groups.last assert_equal @section, g.sections.last end + + test '#update allowed sections' do + group = @provider.provided_groups.create(name: 'Group') + group.sections << @section + assert_equal ['Group'], @provider.provided_groups.pluck(:name) + assert_equal [@section.id], group.sections.pluck(:id) + + another_section = FactoryBot.create(:cms_section, parent: @provider.sections.first, provider: @provider, public: false) + + put provider_admin_cms_group_path(group), params: { cms_group: { name: 'New Name', section_ids: ['', another_section.id] } } + + assert_equal ['New Name'], @provider.provided_groups.pluck(:name) + assert_equal [another_section.id], group.sections.pluck(:id) + end end From 3f4281112d8403d39f0a24f34ed52ad3b256f7c6 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:26:48 +0100 Subject: [PATCH 05/29] Migrate to strong parameters for invoice --- app/controllers/buyers/invoices_controller.rb | 6 +++++- app/controllers/finance/api/invoices_controller.rb | 2 +- app/controllers/finance/provider/invoices_controller.rb | 6 +++++- app/models/invoice.rb | 2 -- test/integration/provider/invoices_controller_test.rb | 8 +++++++- 5 files changed, 18 insertions(+), 6 deletions(-) diff --git a/app/controllers/buyers/invoices_controller.rb b/app/controllers/buyers/invoices_controller.rb index ef0e041d79..d88bd4839f 100644 --- a/app/controllers/buyers/invoices_controller.rb +++ b/app/controllers/buyers/invoices_controller.rb @@ -40,7 +40,7 @@ def create def update @invoice = @account.invoices.find(params[:id]) - if @invoice.update(params[:invoice]) + if @invoice.update(invoice_params) redirect_to admin_buyers_account_invoice_url(@account, @invoice), success: t('.success') else render :edit @@ -52,4 +52,8 @@ def update def find_account @account = current_account.buyers.find(params[:account_id]) end + + def invoice_params + params.require(:invoice).permit(:period) + end end diff --git a/app/controllers/finance/api/invoices_controller.rb b/app/controllers/finance/api/invoices_controller.rb index 5b37cbd237..b12bb98e89 100644 --- a/app/controllers/finance/api/invoices_controller.rb +++ b/app/controllers/finance/api/invoices_controller.rb @@ -51,7 +51,7 @@ def charge # Invoice Update # PUT /api/invoices/{id}.xml def update - invoice.update(invoice_params_update, without_protection: true) + invoice.update(invoice_params_update) respond_with(invoice) end diff --git a/app/controllers/finance/provider/invoices_controller.rb b/app/controllers/finance/provider/invoices_controller.rb index ee89baa6ec..3bde8274e0 100644 --- a/app/controllers/finance/provider/invoices_controller.rb +++ b/app/controllers/finance/provider/invoices_controller.rb @@ -61,7 +61,7 @@ def create end def update - if @invoice.update(params[:invoice]) + if @invoice.update(invoice_params) redirect_to admin_finance_invoice_url(@invoice), success: t('.success') else render :edit @@ -96,4 +96,8 @@ def find_buyer(options = {}) def find_invoice @invoice = collection.find(params[:id]) end + + def invoice_params + @invoice_params ||= params.require(:invoice).permit(:friendly_id, :period) + end end diff --git a/app/models/invoice.rb b/app/models/invoice.rb index 738ac61b17..fbe59c15f0 100644 --- a/app/models/invoice.rb +++ b/app/models/invoice.rb @@ -39,8 +39,6 @@ class InvalidInvoiceStateException < RuntimeError; end has_attached_file :pdf, url: ':url_root/:class/:id/:attachment/:style/:basename.:extension' do_not_validate_attachment_file_type :pdf - attr_accessible :provider_account, :buyer_account, :friendly_id, :period - validates :provider_account, :buyer_account, :friendly_id, presence: true validates :period, presence: { :message => 'Billing period format should be YYYY-MM' } diff --git a/test/integration/provider/invoices_controller_test.rb b/test/integration/provider/invoices_controller_test.rb index 2001804a6d..9c5cbc74a5 100644 --- a/test/integration/provider/invoices_controller_test.rb +++ b/test/integration/provider/invoices_controller_test.rb @@ -68,9 +68,15 @@ class Finance::Provider::InvoicesControllerTest < ActionDispatch::IntegrationTes end test 'update' do - put admin_finance_invoice_path @invoice + new_period = Month.new(1.month.from_now) + new_period_param = new_period.to_param + new_friendly_id = "#{new_period_param}-00000001" + put admin_finance_invoice_path(@invoice), params: { invoice: { friendly_id: new_friendly_id, period: new_period_param } } assert_response :redirect + @invoice.reload assert_equal @invoice, assigns(:invoice) + assert_equal new_friendly_id, @invoice.friendly_id + assert_equal new_period, @invoice.period end test '#charge does not invoke invoice automatic charging' do From 365124f498e9756ff0dc15600d6808054e93eecf Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:27:59 +0100 Subject: [PATCH 06/29] Migrate to strong parameters for ApiDocs::Service --- app/controllers/admin/api/api_docs_services_controller.rb | 4 ++-- app/controllers/admin/api_docs/base_controller.rb | 4 ++-- app/models/api_docs/service.rb | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/controllers/admin/api/api_docs_services_controller.rb b/app/controllers/admin/api/api_docs_services_controller.rb index da4294f921..72e1fbed8c 100644 --- a/app/controllers/admin/api/api_docs_services_controller.rb +++ b/app/controllers/admin/api/api_docs_services_controller.rb @@ -27,7 +27,7 @@ def index # ActiveDocs Spec Create # POST /admin/api/active_docs.json def create - @api_docs_service = current_account.api_docs_services.create(api_docs_params(:system_name), without_protection: true) + @api_docs_service = current_account.api_docs_services.create(api_docs_params(:system_name)) respond_with(@api_docs_service) end @@ -40,7 +40,7 @@ def show # ActiveDocs Spec Update # PUT /admin/api/active_docs/{id}.json def update - @api_docs_service.update(api_docs_params, without_protection: true) + @api_docs_service.update(api_docs_params) respond_with(@api_docs_service) end diff --git a/app/controllers/admin/api_docs/base_controller.rb b/app/controllers/admin/api_docs/base_controller.rb index d1994d05bd..c5a0ad70f9 100644 --- a/app/controllers/admin/api_docs/base_controller.rb +++ b/app/controllers/admin/api_docs/base_controller.rb @@ -18,7 +18,7 @@ def new end def create - @api_docs_service = api_docs_services.new(api_docs_params(:system_name), without_protection: true) + @api_docs_service = api_docs_services.new(api_docs_params(:system_name)) if @api_docs_service.save redirect_to preview_admin_api_docs_service_path(@api_docs_service), success: t('admin.api_docs.create.success') else @@ -72,7 +72,7 @@ def edit; end def update respond_to do |format| - if api_docs_service.update(api_docs_params, without_protection: true) + if api_docs_service.update(api_docs_params) msg = t('admin.api_docs.update.success') format.html { redirect_to preview_admin_api_docs_service_path(api_docs_service), success: msg } format.js do diff --git a/app/models/api_docs/service.rb b/app/models/api_docs/service.rb index b3cb34f33f..57a4c39afc 100644 --- a/app/models/api_docs/service.rb +++ b/app/models/api_docs/service.rb @@ -8,7 +8,6 @@ class ApiDocs::Service < ApplicationRecord belongs_to :account, required: true belongs_to :service, class_name: '::Service', inverse_of: :api_docs_services - attr_accessible :account, :body, :name, :description, :published, :skip_swagger_validations attr_readonly :system_name self.table_name = "api_docs_services" From 6409e77bfd4af999e57995affd004aa503bc3312 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:31:12 +0100 Subject: [PATCH 07/29] Remove protected attributes dependency in SSOToken --- .../admin/api/sso_tokens_controller.rb | 6 +- app/models/sso_token.rb | 73 ++++++++++--------- spec/acceptance/api/sso_token_spec.rb | 3 +- .../api/sso_tokens_controller_test.rb | 2 + test/unit/sso_token_test.rb | 5 +- 5 files changed, 47 insertions(+), 42 deletions(-) diff --git a/app/controllers/admin/api/sso_tokens_controller.rb b/app/controllers/admin/api/sso_tokens_controller.rb index 96dcfaec33..f74ec70de7 100644 --- a/app/controllers/admin/api/sso_tokens_controller.rb +++ b/app/controllers/admin/api/sso_tokens_controller.rb @@ -1,6 +1,8 @@ +# frozen_string_literal: true + class Admin::Api::SSOTokensController < Admin::Api::BaseController - wrap_parameters :sso_token, :include => [:user_id, :username, :expires_in, :redirect_url, :protocol], :format => [ :url_encoded_form ] + wrap_parameters :sso_token, :include => %i[user_id username expires_in redirect_url protocol], :format => [:url_encoded_form] # parameters: # * user_id @@ -8,7 +10,7 @@ class Admin::Api::SSOTokensController < Admin::Api::BaseController # * provider_key # * protocol def create - sso_token = SSOToken.new **sso_token_params.to_h + sso_token = SSOToken.new(sso_token_params) sso_token.account = domain_account sso_token.save diff --git a/app/models/sso_token.rb b/app/models/sso_token.rb index ee8a11075e..64bd7c12ef 100644 --- a/app/models/sso_token.rb +++ b/app/models/sso_token.rb @@ -1,10 +1,11 @@ +# frozen_string_literal: true + class SSOToken include ActiveModel::Validations - include ActiveModel::MassAssignmentSecurity + include ActiveModel::ForbiddenAttributesProtection include ActiveModel::Serializers::Xml attr_accessor :user_id, :username, :expires_in, :redirect_url, :protocol, :account - attr_accessible :user_id, :username, :expires_in, :redirect_url, :protocol attr_reader :encrypted_token, :expires_at validates :expires_in, :numericality => { :only_integer => true, :greater_than => 30.seconds, :less_than_or_equal_to => 1.day, :allow_nil => true } @@ -15,9 +16,9 @@ class SSOToken validate :one_of_user_id_or_username_is_required validate :account_is_provider_and_user_of_provider, :if => Proc.new {|o| o.account && o.user_id || o.username } - def initialize(**attributes) - assign_attributes({:expires_in => 10.minutes, :protocol => 'https'}.merge(attributes)) - @new_record= true + def initialize(attributes = {}) + assign_attributes(attributes.reverse_merge(expires_in: 10.minutes, protocol: 'https')) + @new_record = true end def save @@ -47,9 +48,9 @@ def to_xml options = {} xml.to_xml end - def assign_attributes values - sanitize_for_mass_assignment(values, nil).each do |k, v| - send("#{k}=", v) + def assign_attributes(values) + sanitize_for_mass_assignment(values).each do |key, value| + send("#{key}=", value) end end @@ -75,42 +76,42 @@ def sso_url!(host: nil, port: nil) protected - def generate_token - @expires_at= Time.now.utc + expires_in.to_i - @encrypted_token= ThreeScale::SSO::Encryptor.new(account.settings.sso_key, expires_at.to_i).encrypt_token user_id, username - @new_record= false - end + def generate_token + @expires_at= Time.now.utc + expires_in.to_i + @encrypted_token= ThreeScale::SSO::Encryptor.new(account.settings.sso_key, expires_at.to_i).encrypt_token user_id, username + @new_record= false + end private - def parsable_redirect_url - Addressable::URI.parse redirect_url - rescue - errors.add :redirect_url, :invalid - end + def parsable_redirect_url + Addressable::URI.parse redirect_url + rescue + errors.add :redirect_url, :invalid + end - def one_of_user_id_or_username_is_required - if user_id.nil? && username.nil? - errors.add :base, :one_of_user_id_or_username_is_required - end + def one_of_user_id_or_username_is_required + if user_id.nil? && username.nil? + errors.add :base, :one_of_user_id_or_username_is_required end + end - def account_is_provider_and_user_of_provider - unless account.is_a?(Account) && account.provider? - errors.add :account, :invalid - return - end + def account_is_provider_and_user_of_provider + unless account.is_a?(Account) && account.provider? + errors.add :account, :invalid + return + end - # if we have a user-id and we can't find the user for this provider we fail to generate the token - unless user_id.blank? - if account.managed_users.find_by_id(user_id).nil? - errors.add :user_id, :invalid - end - return + # if we have a user-id and we can't find the user for this provider we fail to generate the token + unless user_id.blank? + if account.managed_users.find_by_id(user_id).nil? + errors.add :user_id, :invalid end + return + end - if account.managed_users.find_by_username(username).nil? - errors.add :username, :invalid - end + if account.managed_users.find_by_username(username).nil? + errors.add :username, :invalid end + end end diff --git a/spec/acceptance/api/sso_token_spec.rb b/spec/acceptance/api/sso_token_spec.rb index 6ba0816809..23f2ab959b 100644 --- a/spec/acceptance/api/sso_token_spec.rb +++ b/spec/acceptance/api/sso_token_spec.rb @@ -15,7 +15,8 @@ api 'sso token' do before do - expect(SSOToken).to receive(:new).with(user_id: user_id.to_s, expires_in: expires_in.to_s) + expected_params = ActionController::Parameters.new(user_id: user_id.to_s, expires_in: expires_in.to_s).permit! + expect(SSOToken).to receive(:new).with(expected_params) .and_return(sso_token) end diff --git a/test/integration/api/sso_tokens_controller_test.rb b/test/integration/api/sso_tokens_controller_test.rb index e2f2f0bdac..4de8b705c0 100644 --- a/test/integration/api/sso_tokens_controller_test.rb +++ b/test/integration/api/sso_tokens_controller_test.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'test_helper' class Admin::Api::SsoTokensControllerTest < ActionDispatch::IntegrationTest diff --git a/test/unit/sso_token_test.rb b/test/unit/sso_token_test.rb index c1cd31c9b8..d845b494c7 100644 --- a/test/unit/sso_token_test.rb +++ b/test/unit/sso_token_test.rb @@ -10,11 +10,10 @@ def setup test "creating a valid sso token using user_id" do buyer = FactoryBot.create(:buyer_account, :provider_account => @provider) - sso_token = SSOToken.new :user_id => buyer.users.first.id, :account => @provider, :expires_in => 6000 + sso_token = SSOToken.new :user_id => buyer.users.first.id, :expires_in => 6000 - # mass-assignment should take care of this. assert_nil sso_token.account - sso_token.account= @provider + sso_token.account = @provider assert sso_token.save From 3afb3ca98aa700611053292ff6e4bc9cd8465ac0 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:31:52 +0100 Subject: [PATCH 08/29] Migrate to strong parameters for pricing rules --- app/controllers/api/pricing_rules_controller.rb | 2 +- app/models/pricing_rule.rb | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/api/pricing_rules_controller.rb b/app/controllers/api/pricing_rules_controller.rb index 46b2b0b8b4..54042b4c6c 100644 --- a/app/controllers/api/pricing_rules_controller.rb +++ b/app/controllers/api/pricing_rules_controller.rb @@ -76,6 +76,6 @@ def authorize_action end def pricing_rule_params - params.require(:pricing_rule) + params.require(:pricing_rule).permit(:min, :max, :cost_per_unit) end end diff --git a/app/models/pricing_rule.rb b/app/models/pricing_rule.rb index 4ff59d202d..f8b631968f 100644 --- a/app/models/pricing_rule.rb +++ b/app/models/pricing_rule.rb @@ -5,8 +5,6 @@ class PricingRule < ApplicationRecord belongs_to :plan belongs_to :metric - attr_protected :metric_id, :plan_id, :plan_type, :tenant_id, :audit_ids - # TODO: add validations that check that the intervals are well defined. TODO: # min must be always > 0 # From e4a0cf1f9a2a751f65d3013f55e407df36eb1941 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:33:15 +0100 Subject: [PATCH 09/29] Migrate to strong parameters for plan features --- .reek.yml | 3 + .../admin/api/account_features_controller.rb | 2 +- .../admin/api/service_features_controller.rb | 8 +- app/models/feature.rb | 2 - app/models/features_plan.rb | 2 - doc/active_docs/account_management_api.json | 23 +- .../account_features_test.rb | 254 ++++++++++++++++++ .../service_features_test.rb | 17 +- 8 files changed, 299 insertions(+), 12 deletions(-) create mode 100644 test/integration/user-management-api/account_features_test.rb diff --git a/.reek.yml b/.reek.yml index 9329f55ef5..aa0ba8b4ed 100644 --- a/.reek.yml +++ b/.reek.yml @@ -39,3 +39,6 @@ directories: "app/mailers": InstanceVariableAssumption: enabled: false + "test": + InstanceVariableAssumption: + enabled: false diff --git a/app/controllers/admin/api/account_features_controller.rb b/app/controllers/admin/api/account_features_controller.rb index e1da6e07a7..5d1055172f 100644 --- a/app/controllers/admin/api/account_features_controller.rb +++ b/app/controllers/admin/api/account_features_controller.rb @@ -41,7 +41,7 @@ def destroy protected def feature_params - params.fetch(:feature) + params.require(:feature).permit(:name, :system_name, :description) end def features diff --git a/app/controllers/admin/api/service_features_controller.rb b/app/controllers/admin/api/service_features_controller.rb index caaeadf0b5..fc70941513 100644 --- a/app/controllers/admin/api/service_features_controller.rb +++ b/app/controllers/admin/api/service_features_controller.rb @@ -26,7 +26,7 @@ def show # Service Feature Update # PUT /admin/api/services/{service_id}/features/{id}.xml def update - feature.update(feature_params) + feature.update(feature_update_params) respond_with(feature) end @@ -42,7 +42,11 @@ def destroy protected def feature_params - params.fetch(:feature) + params.require(:feature).permit(:name, :system_name, :description, :scope) + end + + def feature_update_params + feature_params.except(:scope) end def features diff --git a/app/models/feature.rb b/app/models/feature.rb index 0f3f8c5985..efd7c1a8e2 100644 --- a/app/models/feature.rb +++ b/app/models/feature.rb @@ -23,8 +23,6 @@ class Feature < ApplicationRecord where(:scope => object.class.model_name.to_s) end - attr_protected :featurable_id, :featurable_type, :tenant_id, :audit_ids - validates :system_name, uniqueness: { scope: [:featurable_id, :featurable_type], case_sensitive: true } validates :system_name, :name, length: { maximum: 255 } diff --git a/app/models/features_plan.rb b/app/models/features_plan.rb index e11fccc147..68400a8aab 100644 --- a/app/models/features_plan.rb +++ b/app/models/features_plan.rb @@ -18,8 +18,6 @@ def hash [self.class, feature_id, plan_id].hash end - attr_protected :plan_id, :feature_id, :plan_type, :tenant_id - private def feature_scope_matches_plan_class? diff --git a/doc/active_docs/account_management_api.json b/doc/active_docs/account_management_api.json index 2119bd38c6..332591a4f5 100644 --- a/doc/active_docs/account_management_api.json +++ b/doc/active_docs/account_management_api.json @@ -6643,7 +6643,11 @@ }, "system_name": { "type": "string", - "description": "System Name of the object to be created. System names cannot be modified after creation, they are used as the key to identify the objects." + "description": "System Name of the feature." + }, + "description": { + "type": "string", + "description": "Description of the feature." } }, "required": [ @@ -6727,11 +6731,18 @@ "name": { "type": "string", "description": "Name of the feature." + }, + "system_name": { + "type": "string", + "description": "System Name of the feature." + }, + "description": { + "type": "string", + "description": "Description of the feature." } }, "required": [ - "access_token", - "name" + "access_token" ] } } @@ -9739,7 +9750,7 @@ }, "system_name": { "type": "string", - "description": "System Name of the object to be created. System names cannot be modified after creation, they are used as the key to identify the objects." + "description": "System Name of the feature." }, "description": { "type": "string", @@ -9855,6 +9866,10 @@ "type": "string", "description": "Name of the feature." }, + "system_name": { + "type": "string", + "description": "System Name of the feature." + }, "description": { "type": "string", "description": "Description of the feature." diff --git a/test/integration/user-management-api/account_features_test.rb b/test/integration/user-management-api/account_features_test.rb new file mode 100644 index 0000000000..b35c036afa --- /dev/null +++ b/test/integration/user-management-api/account_features_test.rb @@ -0,0 +1,254 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Admin::Api::AccountFeaturesTest < ActionDispatch::IntegrationTest + def setup + @provider = FactoryBot.create(:provider_account, domain: 'provider.example.com') + @user = FactoryBot.create(:admin, account: @provider) + @token = FactoryBot.create(:access_token, owner: @user, scopes: 'account_management').plaintext_value + + host! @provider.external_admin_domain + end + + test 'index with provider_key' do + FactoryBot.create(:feature, featurable: provider, name: 'Feature 1') + FactoryBot.create(:feature, featurable: provider, name: 'Feature 2') + + get admin_api_features_path, params: { provider_key: provider.api_key, format: :xml } + + assert_response :success + + xml = Nokogiri::XML::Document.parse(@response.body) + + assert_equal 2, xml.xpath('.//features/feature').count + assert xml.xpath('.//feature/name[text()="Feature 1"]').present? + assert xml.xpath('.//feature/name[text()="Feature 2"]').present? + end + + test 'unauthorized without credentials' do + feature = FactoryBot.create(:feature, featurable: provider) + + get admin_api_feature_path(feature) + assert_response :forbidden + end + + test 'index' do + FactoryBot.create(:feature, featurable: provider, name: 'Feature 1') + FactoryBot.create(:feature, featurable: provider, name: 'Feature 2') + + get admin_api_features_path, params: { access_token: token, format: :xml } + + assert_response :success + + xml = Nokogiri::XML::Document.parse(response.body) + + assert_equal 2, xml.xpath('.//features/feature').count + assert xml.xpath('.//feature/name[text()="Feature 1"]').present? + assert xml.xpath('.//feature/name[text()="Feature 2"]').present? + end + + test 'show' do + feature = FactoryBot.create(:feature, featurable: provider, name: 'Test Feature', system_name: 'test_feature') + + get admin_api_feature_path(id: feature.id), params: { access_token: token, format: :xml } + + assert_response :success + + xml = Nokogiri::XML::Document.parse(@response.body) + + assert_equal 'Test Feature', xml.xpath('.//feature/name').text + assert_equal 'test_feature', xml.xpath('.//feature/system_name').text + assert_equal provider.id.to_s, xml.xpath('.//feature/account_id').text + end + + test 'show with wrong id' do + get admin_api_feature_path(id: 0), params: { access_token: token, format: :xml } + + assert_response :not_found + end + + test 'create' do + assert_difference 'provider.features.count', 1 do + post admin_api_features_path, params: { + access_token: token, + format: :xml, + name: 'New Feature', + system_name: 'new_feature', + description: 'A new feature' + } + end + + assert_response :success + + xml = Nokogiri::XML::Document.parse(@response.body) + + assert_equal 'New Feature', xml.xpath('.//feature/name').text + assert_equal 'new_feature', xml.xpath('.//feature/system_name').text + assert_equal 'A new feature', xml.xpath('.//feature/description').text + assert_equal provider.id.to_s, xml.xpath('.//feature/account_id').text + + feature = provider.features.reload.last + assert_equal 'New Feature', feature.name + assert_equal 'new_feature', feature.system_name + assert_equal 'A new feature', feature.description + end + + test 'create with minimal params' do + assert_difference 'provider.features.count', 1 do + post admin_api_features_path, params: { + access_token: token, + format: :xml, + name: 'Minimal Feature' + } + end + + assert_response :success + + feature = provider.features.reload.last + assert_equal 'Minimal Feature', feature.name + end + + test 'create with invalid params' do + assert_no_difference 'provider.features.count' do + post admin_api_features_path, params: { + access_token: token, + format: :xml, + name: 'x' * 256 # Exceeds max length of 255 + } + end + + assert_response :unprocessable_entity + assert_xml_error @response.body, "Name" + end + + test 'create with duplicate system_name' do + FactoryBot.create(:feature, featurable: provider, system_name: 'existing_feature') + + assert_no_difference 'provider.features.count' do + post admin_api_features_path, params: { + access_token: token, + format: :xml, + name: 'Duplicate', + system_name: 'existing_feature' + } + end + + assert_response :unprocessable_entity + assert_xml_error @response.body, "System name" + end + + test 'update' do + feature = FactoryBot.create(:feature, featurable: provider, name: 'Old Name', system_name: 'old_system_name', description: 'Old description') + + put admin_api_feature_path(id: feature.id), params: { + access_token: token, + format: :xml, + name: 'New Name', + system_name: 'new_system_name', + description: 'New description' + } + + assert_response :success + + xml = Nokogiri::XML::Document.parse(@response.body) + + assert_equal 'New Name', xml.xpath('.//feature/name').text + assert_equal 'new_system_name', xml.xpath('.//feature/system_name').text + assert_equal 'New description', xml.xpath('.//feature/description').text + + feature.reload + assert_equal 'New Name', feature.name + assert_equal 'new_system_name', feature.system_name + assert_equal 'New description', feature.description + end + + test 'updating scope is not allowed' do + feature = FactoryBot.create(:feature, featurable: provider) + + assert_equal 'AccountPlan', feature.scope + + put admin_api_feature_path(id: feature.id), params: { + access_token: token, + format: :xml, + scope: 'ApplicationPlan' + } + + assert_response :success + assert_equal 'AccountPlan', feature.reload.scope + end + + test 'update with wrong id' do + put admin_api_feature_path(id: 0), params: { + access_token: token, + format: :xml, + name: 'New Name' + } + + assert_response :not_found + end + + test 'update with invalid params' do + feature = FactoryBot.create(:feature, featurable: provider, name: 'Valid Name') + + put admin_api_feature_path(id: feature.id), params: { + access_token: token, + format: :xml, + name: 'x' * 256 # Exceeds max length of 255 + } + + assert_response :unprocessable_entity + assert_xml_error @response.body, "Name" + + feature.reload + assert_equal 'Valid Name', feature.name + end + + test 'destroy' do + feature = FactoryBot.create(:feature, featurable: provider) + + assert_difference 'provider.features.count', -1 do + delete admin_api_feature_path(id: feature.id), params: { + access_token: token, + format: :xml + } + end + + assert_response :success + + assert_empty_xml @response.body + + assert_raise ActiveRecord::RecordNotFound do + feature.reload + end + end + + test 'destroy with wrong id' do + delete admin_api_feature_path(id: 0), params: { + access_token: token, + format: :xml + } + + assert_response :not_found + end + + test 'destroy with feature in use' do + feature = FactoryBot.create(:feature, featurable: provider) + plan = FactoryBot.create(:account_plan, issuer: provider) + plan.features << feature + + # Features can be deleted even when in use (no validation prevents this) + assert_difference 'provider.features.count', -1 do + delete admin_api_feature_path(id: feature.id), params: { + access_token: token, + format: :xml + } + end + + assert_response :success + end + + private + + attr_reader :provider, :user, :token +end diff --git a/test/integration/user-management-api/service_features_test.rb b/test/integration/user-management-api/service_features_test.rb index 23219dfc29..7d7dbf73f6 100644 --- a/test/integration/user-management-api/service_features_test.rb +++ b/test/integration/user-management-api/service_features_test.rb @@ -2,7 +2,7 @@ require 'test_helper' -class EnterpriseApiFeaturesTest < ActionDispatch::IntegrationTest +class Admin::Api::ServiceFeaturesTest < ActionDispatch::IntegrationTest def setup @provider = FactoryBot.create(:provider_account, domain: 'provider.example.com') @service = FactoryBot.create(:service, account: @provider) @@ -116,6 +116,21 @@ def setup assert_equal "new_system_name", feature.system_name end + test 'updating scope is not allowed' do + feature = FactoryBot.create(:feature, featurable: @service) + + assert_equal 'ApplicationPlan', feature.scope + + put admin_api_service_feature_path(@service, id: feature.id), params: { + provider_key: @provider.api_key, + format: :xml, + scope: 'ServicePlan' + } + + assert_response :success + assert_equal 'ApplicationPlan', feature.reload.scope + end + pending_test 'update with wrong id' pending_test 'update errors xml' From dab9e58a783dd8fd8698d8e56286005ecd9ced91 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:34:15 +0100 Subject: [PATCH 10/29] Migrate to strong parameters for field definitions --- .../admin/fields_definitions_controller.rb | 31 +++++++++++++------ app/models/fields_definition.rb | 1 - .../fields_definitions_controller_test.rb | 16 ++++++++++ 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/app/controllers/admin/fields_definitions_controller.rb b/app/controllers/admin/fields_definitions_controller.rb index 39eb170fc1..f2567b07db 100644 --- a/app/controllers/admin/fields_definitions_controller.rb +++ b/app/controllers/admin/fields_definitions_controller.rb @@ -5,18 +5,21 @@ class Admin::FieldsDefinitionsController < Sites::BaseController activate_menu :audience, :accounts, :fields_definitions def index - @possible_targets = FieldsDefinition.targets + @possible_targets = available_targets respond_with(field_definitions) end def new - @fields_definition = field_definitions.build(field_definition_params) + target = field_definition_params[:target] + target = available_targets.first unless available_targets.include?(target) + + @fields_definition = field_definitions.build(target: target) + target_class = @fields_definition.target_class - @optional_fields = @fields_definition.target_class.builtin_fields - - current_account.fields_definitions.by_target(target).map{ |f|f.name } + @optional_fields = target_class.builtin_fields - existing_fields_names_by_target(target) - @required_fields = @fields_definition.target_class.required_fields + @required_fields = target_class.required_fields @optional_fields.unshift "[new field]" @@ -61,9 +64,9 @@ def destroy end def sort - fields = current_account.fields_definitions.find(field_definition_params).index_by(&:id) + fields = current_account.fields_definitions.find(sort_params).index_by(&:id) - field_definition_params.each_with_index do |field_id, index| + sort_params.each_with_index do |field_id, index| fields.fetch(field_id.to_i).update_attribute(:pos, index + 1) end @@ -73,11 +76,11 @@ def sort private def field_definition_params - params[:fields_definition] || {} + params.fetch(:fields_definition, {}).permit(:target, :name, :label, :required, :hidden, :read_only, :choices_for_views) end - def target - field_definition_params[:target] + def sort_params + @sort_params = params.fetch(:fields_definition, []) end def field_definitions @@ -87,4 +90,12 @@ def field_definitions def field_definition @fields_definition ||= field_definitions.find(params[:id]) end + + def available_targets + @available_targets ||= FieldsDefinition.targets + end + + def existing_fields_names_by_target(target) + current_account.fields_definitions.by_target(target).map(&:name) + end end diff --git a/app/models/fields_definition.rb b/app/models/fields_definition.rb index ddf5663d7b..1103613b4e 100644 --- a/app/models/fields_definition.rb +++ b/app/models/fields_definition.rb @@ -48,7 +48,6 @@ def self.editable_by(user) default_scope { by_position } scope :by_position, -> { order(:pos) } - attr_protected :account_id, :tenant_id attr_readonly :account_id, :tenant_id, :target, :name # The attribute `pos` is exposed as `position` in the API diff --git a/test/functional/admin/fields_definitions_controller_test.rb b/test/functional/admin/fields_definitions_controller_test.rb index ff79f80799..c3a85c55a2 100644 --- a/test/functional/admin/fields_definitions_controller_test.rb +++ b/test/functional/admin/fields_definitions_controller_test.rb @@ -19,6 +19,22 @@ def field_definition assert_response :success end + test 'new' do + default_target = FieldsDefinition.targets.first + + get :new + + assert_response :success + assert_select('h1', text: "New field definition for #{default_target}") + assert_select 'input#fields_definition_target', type: 'hidden', value: 'Account' + + get :new, params: { fields_definition: { target: 'User' } } + + assert_response :success + assert_select('h1', text: "New field definition for User") + assert_select 'input#fields_definition_target', type: 'hidden', value: 'Account' + end + test 'edit' do get :edit, params: { id: field_definition } From 41262472fcd5868eddc0b2472ca71aded32a046d Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:36:10 +0100 Subject: [PATCH 11/29] Migrate to strong parameters for settings --- .rubocop.yml | 4 ++ app/controllers/sites/settings_controller.rb | 15 +++---- .../sites/usage_rules_controller.rb | 10 ++++- app/models/settings.rb | 2 - .../sites/usage_rules_controller_test.rb | 45 +++++++++++++++++++ .../sites/emails_controller_test.rb | 30 +++++++++++++ .../sites/settings_controller_test.rb | 39 ++++++++++++++++ 7 files changed, 134 insertions(+), 11 deletions(-) create mode 100644 test/integration/sites/emails_controller_test.rb diff --git a/.rubocop.yml b/.rubocop.yml index e4c738d766..1cc9becde0 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -17,6 +17,10 @@ Performance: Metrics: Enabled: true +Metrics/BlockLength: + Exclude: + - 'test/**/*_test.rb' + Lint/AssignmentInCondition: AllowSafeAssignment: true diff --git a/app/controllers/sites/settings_controller.rb b/app/controllers/sites/settings_controller.rb index 1564c475fa..423e7004ea 100644 --- a/app/controllers/sites/settings_controller.rb +++ b/app/controllers/sites/settings_controller.rb @@ -2,7 +2,6 @@ class Sites::SettingsController < Sites::BaseController provider_required before_action :find_settings - before_action :find_service, :only => [:edit, :policies, :accessrules] layout 'provider' activate_menu :audience, :finance, :credit_card_policies @@ -11,22 +10,22 @@ def show redirect_to :action => :edit end - def edit - end - - def accessrules - end + def edit; end def update - if @settings.update(params[:settings]) + if @settings.update(settings_params) redirect_to edit_admin_site_settings_path, success: t('.success') else - render :accessrules + redirect_to edit_admin_site_settings_path, danger: t('.error') end end private + def settings_params + params.require(:settings).permit(:cc_terms_path, :cc_privacy_path, :cc_refunds_path) + end + def find_settings @settings = current_account.settings end diff --git a/app/controllers/sites/usage_rules_controller.rb b/app/controllers/sites/usage_rules_controller.rb index 4e714db860..46c372518d 100644 --- a/app/controllers/sites/usage_rules_controller.rb +++ b/app/controllers/sites/usage_rules_controller.rb @@ -7,7 +7,7 @@ def edit end def update - if @settings.update(params[:settings]) + if @settings.update(settings_params) redirect_back_or_to admin_site_settings_url, success: t('.success') else render :edit @@ -20,4 +20,12 @@ def find_settings @settings = current_account.settings end + def settings_params + allowed_attrs = %i[ + useraccountarea_enabled signups_enabled strong_passwords_enabled public_search + account_plans_ui_visible change_account_plan_permission + service_plans_ui_visible change_service_plan_permission + ] + params.require(:settings).permit(*allowed_attrs) + end end diff --git a/app/models/settings.rb b/app/models/settings.rb index 4ca2fb1040..17ee8c8c3f 100644 --- a/app/models/settings.rb +++ b/app/models/settings.rb @@ -4,8 +4,6 @@ class Settings < ApplicationRecord audited allow_mass_assignment: true - attr_protected :account_id, :tenant_id, :product, :audit_ids, :sso_key - validates :product, inclusion: { in: %w[connect enterprise].freeze } validates :change_account_plan_permission, :change_service_plan_permission, inclusion: { in: %w[request none credit_card request_credit_card direct].freeze } validates :bg_colour, :link_colour, :text_colour, :menu_bg_colour, :link_label, :link_url, :menu_link_colour, :token_api, diff --git a/test/functional/sites/usage_rules_controller_test.rb b/test/functional/sites/usage_rules_controller_test.rb index 3e82e48374..0e6dc82bca 100644 --- a/test/functional/sites/usage_rules_controller_test.rb +++ b/test/functional/sites/usage_rules_controller_test.rb @@ -38,4 +38,49 @@ def setup assert_select 'input#settings_account_approval_required' assert_select '.pf-c-check__description', 'Set per account plan from Account Plans.' end + + test 'update with invalid params' do + put :update, params: { settings: { change_account_plan_permission: 'invalid_value' } } + + assert_response :success + assert_template :edit + end + + test 'update with valid params' do + @settings.update(strong_passwords_enabled: true, public_search: true) + %i[useraccountarea_enabled signups_enabled + account_plans_ui_visible service_plans_ui_visible + strong_passwords_enabled public_search].each do |setting| + assert @settings.send(setting), "#{setting} setting is not true as expected" + end + + assert_equal "request", @settings.change_account_plan_permission + assert_equal "request", @settings.change_service_plan_permission + + put :update, params: { + settings: { + useraccountarea_enabled: '0', + signups_enabled: '0', + strong_passwords_enabled: '0', + public_search: '0', + account_plans_ui_visible: '0', + service_plans_ui_visible: '0', + change_account_plan_permission: 'credit_card', + change_service_plan_permission: 'credit_card' + } + } + + assert_redirected_to admin_site_settings_url + assert_equal "Settings updated", flash[:success] + + @settings.reload + + %i[useraccountarea_enabled signups_enabled + account_plans_ui_visible service_plans_ui_visible + strong_passwords_enabled public_search].each do |setting| + assert_not @settings.send(setting), "#{setting} setting is not false as expected" + end + assert_equal "credit_card", @settings.change_account_plan_permission + assert_equal "credit_card", @settings.change_service_plan_permission + end end diff --git a/test/integration/sites/emails_controller_test.rb b/test/integration/sites/emails_controller_test.rb new file mode 100644 index 0000000000..945ae8380e --- /dev/null +++ b/test/integration/sites/emails_controller_test.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Sites::SettingsControllerTest < ActionDispatch::IntegrationTest + + test 'show emails tab if not master account' do + provider = FactoryBot.create(:provider_account) + + login_provider provider + + get edit_admin_site_emails_path + + assert_response :success + end + + test 'do not show emails tab if master account' do + ThreeScale.config.stubs(onpremises: true, tenant_mode: 'master') + + member = FactoryBot.create(:simple_admin, account: master_account) + member.activate! + + login! master_account, user: member + + get edit_admin_site_emails_path + + assert_response :forbidden + end + +end diff --git a/test/integration/sites/settings_controller_test.rb b/test/integration/sites/settings_controller_test.rb index 945ae8380e..10a6c49dba 100644 --- a/test/integration/sites/settings_controller_test.rb +++ b/test/integration/sites/settings_controller_test.rb @@ -27,4 +27,43 @@ class Sites::SettingsControllerTest < ActionDispatch::IntegrationTest assert_response :forbidden end + test 'update credit card policies paths successfully' do + provider = FactoryBot.create(:provider_account) + login_provider provider + + put admin_site_settings_path, params: { + settings: { + cc_terms_path: '/terms', + cc_privacy_path: '/privacy', + cc_refunds_path: '/refunds' + } + } + + assert_redirected_to edit_admin_site_settings_path + assert_equal 'Settings updated', flash[:success] + + provider.settings.reload + assert_equal '/terms', provider.settings.cc_terms_path + assert_equal '/privacy', provider.settings.cc_privacy_path + assert_equal '/refunds', provider.settings.cc_refunds_path + end + + test 'update with empty values clears settings' do + provider = FactoryBot.create(:provider_account) + provider.settings.update(cc_terms_path: '/terms', cc_privacy_path: '/privacy') + login_provider provider + + put admin_site_settings_path, params: { + settings: { + cc_terms_path: '', + cc_privacy_path: '' + } + } + + assert_redirected_to edit_admin_site_settings_path + + provider.settings.reload + assert_equal '', provider.settings.cc_terms_path + assert_equal '', provider.settings.cc_privacy_path + end end From a55d6d29ae8f785addf3cbc9e83fc321e010e78b Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:39:27 +0100 Subject: [PATCH 12/29] Migrate to strong parameters for webhook --- .../admin/api/web_hooks_controller.rb | 2 +- .../provider/admin/webhooks_controller.rb | 8 +- app/models/web_hook.rb | 2 - .../admin/webhooks_controller_test.rb | 188 ++++++++++++++++++ 4 files changed, 195 insertions(+), 5 deletions(-) create mode 100644 test/integration/provider/admin/webhooks_controller_test.rb diff --git a/app/controllers/admin/api/web_hooks_controller.rb b/app/controllers/admin/api/web_hooks_controller.rb index 797f18e84f..3d7d0cb387 100644 --- a/app/controllers/admin/api/web_hooks_controller.rb +++ b/app/controllers/admin/api/web_hooks_controller.rb @@ -19,7 +19,7 @@ def webhook end def allowed_params - %w(url active provider_actions) + WebHook.switchable_attributes + %w[url active provider_actions] + WebHook.switchable_attributes end def webhook_params diff --git a/app/controllers/provider/admin/webhooks_controller.rb b/app/controllers/provider/admin/webhooks_controller.rb index a185636913..db8c3f31f8 100644 --- a/app/controllers/provider/admin/webhooks_controller.rb +++ b/app/controllers/provider/admin/webhooks_controller.rb @@ -39,7 +39,7 @@ def show end def create - @webhook ||= current_account.build_web_hook(params[:web_hook]) + @webhook ||= current_account.build_web_hook(webhook_params) if @webhook.save redirect_to({ action: :edit }, success: t('.success')) @@ -49,7 +49,7 @@ def create end def update - if @webhook.update(params[:web_hook]) + if @webhook.update(webhook_params) redirect_to({ action: :edit }, success: t('.success')) else flash.now[:danger] = t('.error') @@ -59,6 +59,10 @@ def update protected + def webhook_params + params.require(:web_hook).permit(%w[url active provider_actions] + WebHook.switchable_attributes) + end + def find_webhook @webhook = current_account.web_hook end diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb index dd373b5bc8..1a975939e4 100644 --- a/app/models/web_hook.rb +++ b/app/models/web_hook.rb @@ -3,8 +3,6 @@ class WebHook < ApplicationRecord alias provider account - attr_protected :account_id, :tenant_id - validates :account_id, presence: true validates :url, format: { :with => URI::DEFAULT_PARSER.make_regexp(%w[http https]), :if => :active }, length: { maximum: 255 } diff --git a/test/integration/provider/admin/webhooks_controller_test.rb b/test/integration/provider/admin/webhooks_controller_test.rb new file mode 100644 index 0000000000..f69aec4e41 --- /dev/null +++ b/test/integration/provider/admin/webhooks_controller_test.rb @@ -0,0 +1,188 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Provider::Admin::WebhooksControllerTest < ActionDispatch::IntegrationTest + def setup + @provider = FactoryBot.create(:provider_account) + @provider.settings.allow_web_hooks! + login_provider @provider + end + + test 'new when webhook does not exist' do + get new_provider_admin_webhooks_path + + assert_response :success + assert_template :edit + assert_not_nil assigns(:webhook) + assert assigns(:webhook).new_record? + end + + test 'new redirects to edit when webhook already exists' do + FactoryBot.create(:web_hook, account: @provider) + + get new_provider_admin_webhooks_path + + assert_redirected_to edit_provider_admin_webhooks_path + end + + test 'edit when webhook exists' do + webhook = FactoryBot.create(:web_hook, account: @provider, url: 'http://example.com/hook') + + get edit_provider_admin_webhooks_path + + assert_response :success + assert_template :edit + assert_equal webhook, assigns(:webhook) + end + + test 'edit redirects to new when webhook does not exist' do + get edit_provider_admin_webhooks_path + + assert_redirected_to new_provider_admin_webhooks_path + end + + test 'create webhook successfully' do + assert_nil @provider.web_hook + + post provider_admin_webhooks_path, params: { + web_hook: { + url: 'http://example.com/webhook', + active: true, + account_created_on: true, + application_created_on: true + } + } + + assert_redirected_to edit_provider_admin_webhooks_path + assert_equal 'Webhooks settings were successfully updated', flash[:success] + + webhook = @provider.reload.web_hook + assert_equal 'http://example.com/webhook', webhook.url + assert webhook.active + assert webhook.account_created_on + assert webhook.application_created_on + end + + test 'create webhook with invalid params shows errors' do + assert_nil @provider.web_hook + + post provider_admin_webhooks_path, params: { + web_hook: { + url: '', # invalid - blank URL + active: true + } + } + + assert_nil @provider.reload.web_hook + + assert_response :success + assert_template :edit + assert_not_nil assigns(:webhook) + assert_equal ["Must be a valid URL such as http://example.com"], assigns(:webhook).errors[:url] + end + + test 'update webhook successfully' do + webhook = FactoryBot.create(:web_hook, account: @provider, url: 'http://old.com/hook', active: false) + assert_not webhook.provider_actions + + put provider_admin_webhooks_path, params: { + web_hook: { + url: 'http://new.com/hook', + active: true, + provider_actions: true, + user_created_on: true + } + } + + assert_redirected_to edit_provider_admin_webhooks_path + assert_equal 'Webhooks settings were successfully updated', flash[:success] + + webhook.reload + assert_equal 'http://new.com/hook', webhook.url + assert webhook.active + assert webhook.provider_actions + assert webhook.user_created_on + end + + test 'update webhook with invalid params shows errors' do + webhook = FactoryBot.create(:web_hook, account: @provider, url: 'http://example.com/hook') + + put provider_admin_webhooks_path, params: { + web_hook: { + url: '' # invalid + } + } + + assert_response :success + assert_template :edit + assert_equal 'Webhooks settings could not be updated', flash[:danger] + + webhook.reload + assert_equal 'http://example.com/hook', webhook.url + end + + test 'pings webhook and returns success' do + FactoryBot.create(:web_hook, account: @provider, url: 'http://example.com/hook', active: true) + + # Mock successful ping + ping_response = mock('ping_response') + ping_response.stubs(:status).returns(200) + ping_response.stubs(:respond_to?).with(:status).returns(true) + WebHook.any_instance.stubs(:ping).returns(ping_response) + + get provider_admin_webhooks_path(format: :json) + + assert_response :success + json = JSON.parse(@response.body) + assert_equal 'success', json['type'] + assert_includes json['message'], 'http://example.com/hook' + assert_includes json['message'], '200' + end + + test 'show pings webhook and returns failure' do + FactoryBot.create(:web_hook, account: @provider, url: 'http://example.com/hook') + + # Mock failed ping + ping_response = mock('ping_response') + ping_response.stubs(:message).returns('Connection refused') + ping_response.stubs(:respond_to?).with(:status).returns(false) + WebHook.any_instance.stubs(:ping).returns(ping_response) + + get provider_admin_webhooks_path(format: :json) + + assert_response :success + json = JSON.parse(@response.body) + assert_equal 'danger', json['type'] + assert_includes json['message'], 'Connection refused' + end + + test 'show returns error when webhook does not exist' do + get provider_admin_webhooks_path(format: :json) + + assert_response :success + json = JSON.parse(@response.body) + assert_equal 'danger', json['type'] + assert_equal 'Nowhere to ping', json['message'] + end + + test 'requires authorization to manage webhooks' do + member = FactoryBot.create(:member, account: @provider, admin_sections: [:partners]) + member.activate! + + logout! + login_provider @provider, user: member + + get new_provider_admin_webhooks_path + + assert_response :forbidden + end + + test 'requires provider login' do + logout! + + get new_provider_admin_webhooks_path + + assert_redirected_to provider_login_path + end +end From 04a8016eebeed35a5b5822923a250c7b29ae8bf0 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:42:04 +0100 Subject: [PATCH 13/29] Migrate to strong parameters for message --- .../provider/admin/messages/inbox_controller.rb | 4 ++-- app/models/message.rb | 2 -- .../admin/messages/inbox_controller.rb | 6 +++++- .../admin/messages/inbox_controller_test.rb | 13 +++++++++++++ 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/app/controllers/provider/admin/messages/inbox_controller.rb b/app/controllers/provider/admin/messages/inbox_controller.rb index 6db4c1309b..3d9f7ee4fb 100644 --- a/app/controllers/provider/admin/messages/inbox_controller.rb +++ b/app/controllers/provider/admin/messages/inbox_controller.rb @@ -26,7 +26,7 @@ def destroy def reply reply = @message.reply - reply.attributes = message_params + reply.assign_attributes(message_params.merge(origin: "web")) if reply.save && reply.deliver redirect_to({ action: :index }, success: t('.success')) @@ -43,7 +43,7 @@ def mark_as_read private def message_params - params.fetch(:message).merge(:origin => "web") + params.require(:message).permit(:subject, :body) end def find_message diff --git a/app/models/message.rb b/app/models/message.rb index be3e67bc54..52e5f5ca49 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -37,8 +37,6 @@ class Message < ApplicationRecord after_save :update_recipients after_initialize :default_values - attr_protected :tenant_id - scope :visible, -> { where(hidden_at: nil) } scope :hidden, -> { where.not(hidden_at: nil) } scope :latest_first, -> { order(created_at: :desc) } diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/messages/inbox_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/messages/inbox_controller.rb index d1394fcef8..cc295a59b2 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/messages/inbox_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/messages/inbox_controller.rb @@ -38,7 +38,7 @@ def destroy def create @message = current_account.received_messages.find(params[:reply_to]) reply = @message.reply - reply.attributes = params[:message].merge(origin: "web") + reply.assign_attributes(message_params.merge(origin: "web")) reply.save! reply.deliver! @@ -52,4 +52,8 @@ def create def find_message @message = current_account.received_messages.find(params[:id]) end + + def message_params + params.require(:message).permit(:body) + end end diff --git a/test/functional/developer_portal/admin/messages/inbox_controller_test.rb b/test/functional/developer_portal/admin/messages/inbox_controller_test.rb index ff21624b30..b6b35bd96a 100644 --- a/test/functional/developer_portal/admin/messages/inbox_controller_test.rb +++ b/test/functional/developer_portal/admin/messages/inbox_controller_test.rb @@ -5,6 +5,7 @@ class DeveloperPortal::Admin::Messages::InboxControllerTest < DeveloperPortal::A def setup provider = FactoryBot.create(:provider_account) @user = FactoryBot.create(:user, account: provider) + @message = FactoryBot.create(:received_message, receiver: provider) host! provider.internal_domain @@ -20,4 +21,16 @@ def test_index assert assigned_drop_variables.include?('messages') assert assigned_drop_variables.include?('pagination') end + + test 'creates valid reply' do + post :create, params: { message: { body: "reply message" }, reply_to: @message.id } + + MessageWorker.drain + msg = Message.last + + assert_equal 'Reply was sent.', flash[:notice] + assert_equal "Re: #{@message.subject}", msg.subject + assert_equal 'sent', msg.state + assert_equal 'reply message', msg.body + end end From 89a9e96c34263242c1fbd90235a6377580035205 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:43:56 +0100 Subject: [PATCH 14/29] Permit service attributes in onboarding wizard (product) --- .../onboarding/wizard/product_controller.rb | 2 +- app/models/service.rb | 2 -- .../wizard/product_controller_test.rb | 26 +++++++++++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 test/integration/provider/admin/onboarding/wizard/product_controller_test.rb diff --git a/app/controllers/provider/admin/onboarding/wizard/product_controller.rb b/app/controllers/provider/admin/onboarding/wizard/product_controller.rb index e167d6192d..49200ffd91 100644 --- a/app/controllers/provider/admin/onboarding/wizard/product_controller.rb +++ b/app/controllers/provider/admin/onboarding/wizard/product_controller.rb @@ -26,6 +26,6 @@ def update private def service_params - params.require(:service) + params.require(:service).permit(:name) end end diff --git a/app/models/service.rb b/app/models/service.rb index 413e0597ac..3f693e22c1 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -69,8 +69,6 @@ class Service < ApplicationRecord # rubocop:disable Metrics/ClassLength belongs_to :default_service_plan, class_name: 'ServicePlan' belongs_to :default_application_plan, class_name: 'ApplicationPlan' - attr_protected :account_id, :tenant_id, :audit_ids - # LEGACY alias plans application_plans diff --git a/test/integration/provider/admin/onboarding/wizard/product_controller_test.rb b/test/integration/provider/admin/onboarding/wizard/product_controller_test.rb new file mode 100644 index 0000000000..eedb2fc785 --- /dev/null +++ b/test/integration/provider/admin/onboarding/wizard/product_controller_test.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Provider::Admin::Onboarding::Wizard::ProductControllerTest < ActionDispatch::IntegrationTest + setup do + @provider = FactoryBot.create(:provider_account) + host! @provider.external_admin_domain + login_provider @provider + end + + test 'new' do + get new_provider_admin_onboarding_wizard_product_path + assert_response :success + end + + test 'update with valid params' do + service = @provider.first_service! + + put provider_admin_onboarding_wizard_product_path, params: { service: { name: 'New API name' } } + assert_redirected_to new_provider_admin_onboarding_wizard_connect_path + + service.reload + assert_equal 'New API name', service.name + end +end From 16f07b324db65b123b83693c7a001800d24b8d11 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:45:00 +0100 Subject: [PATCH 15/29] Remove attr_protected for Plan --- app/models/plan.rb | 2 -- test/test_helpers/provider.rb | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/plan.rb b/app/models/plan.rb index 9a7fbbc09a..0a915e9d3a 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -31,8 +31,6 @@ class PeriodRangeCalculationError < StandardError; end validates :state, inclusion: { in: %w(hidden published) } before_validation(:on => :create) { set_state_to_hidden_if_nil } # otherwise the validation above fails as state machine hasnt kicked in yet (ugly) - attr_protected :issuer_id, :original_id, :type, :issuer_type, :tenant_id, :audit_ids, :state - state_machine :initial => :hidden do state :hidden state :published diff --git a/test/test_helpers/provider.rb b/test/test_helpers/provider.rb index 7426df75ac..460143026a 100644 --- a/test/test_helpers/provider.rb +++ b/test/test_helpers/provider.rb @@ -24,7 +24,7 @@ def create_a_complete_provider FactoryBot.create(:limit_alert, account: provider, cinstance: provider.application_contracts.take) app_plan = FactoryBot.create(:application_plan, issuer: service) app_plan.customize - FactoryBot.create(:application_plan, name: "somehow_broken", original_id: app_plan.id, issuer: service).update(issuer_id: service.id + 100) + FactoryBot.create(:application_plan, name: "somehow_broken", original_id: app_plan.id, issuer: service) metric = service.metrics.take FactoryBot.create(:usage_limit, plan: app_plan, metric:) FactoryBot.create(:plan_metric, plan: app_plan, metric:, visible: false, limits_only_text: false) From a7c345ddd5113a9e5559ad4a3cbf39a8c513e826 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:49:58 +0100 Subject: [PATCH 16/29] Remove attr_accessible from multiple models: - AuthenticationProvider::RedhatCustomerPortal - AuthenticationProvider::ServiceDiscoveryProvider - CMS::GroupSection - CMS::Permission - ApplicationKey - BackendEvent - LineItem - Onboarding - Partner - Post - ReferrerFilter --- app/models/application_key.rb | 2 -- app/models/authentication_provider/redhat_customer_portal.rb | 2 -- .../authentication_provider/service_discovery_provider.rb | 2 -- app/models/backend_event.rb | 2 -- app/models/cms/group_section.rb | 1 - app/models/cms/permission.rb | 5 ++--- app/models/line_item.rb | 2 -- app/models/onboarding.rb | 2 -- app/models/partner.rb | 3 +-- app/models/post.rb | 2 -- app/models/referrer_filter.rb | 2 -- test/unit/cms/portlet_test.rb | 1 - 12 files changed, 3 insertions(+), 23 deletions(-) diff --git a/app/models/application_key.rb b/app/models/application_key.rb index ef467d2227..f6f6093dab 100644 --- a/app/models/application_key.rb +++ b/app/models/application_key.rb @@ -7,8 +7,6 @@ class ApplicationKey < ApplicationRecord belongs_to :application, :class_name => 'Cinstance', :inverse_of => :application_keys - attr_accessible :application, :value - validates :application, presence: true # The following characters are accepted: diff --git a/app/models/authentication_provider/redhat_customer_portal.rb b/app/models/authentication_provider/redhat_customer_portal.rb index 9ba81721ff..bf9bdf20b0 100644 --- a/app/models/authentication_provider/redhat_customer_portal.rb +++ b/app/models/authentication_provider/redhat_customer_portal.rb @@ -5,8 +5,6 @@ class AuthenticationProvider::RedhatCustomerPortal < AuthenticationProvider::Key CONFIG_ATTRIBUTES = %i[client_id client_secret realm system_name skip_ssl_certificate_verification].freeze private_constant :CONFIG_ATTRIBUTES - attr_accessible(*CONFIG_ATTRIBUTES, :account) - def self.build(options = {}) config = ThreeScale.config.redhat_customer_portal.to_h .slice(*CONFIG_ATTRIBUTES) diff --git a/app/models/authentication_provider/service_discovery_provider.rb b/app/models/authentication_provider/service_discovery_provider.rb index 500223504a..c0d39ac8cd 100644 --- a/app/models/authentication_provider/service_discovery_provider.rb +++ b/app/models/authentication_provider/service_discovery_provider.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class AuthenticationProvider::ServiceDiscoveryProvider < AuthenticationProvider - attr_accessible :account, :token_url, :authorize_url, :user_info_url, :client_id, :client_secret, :system_name, - :kind, :skip_ssl_certificate_verification def self.build(options = {}) new do |provider| diff --git a/app/models/backend_event.rb b/app/models/backend_event.rb index a57a3e1bf1..7711211e2a 100644 --- a/app/models/backend_event.rb +++ b/app/models/backend_event.rb @@ -2,8 +2,6 @@ class BackendEvent < ApplicationRecord serialize :data - attr_accessible :id, :data - validates :id, presence: true validates :data, length: { maximum: 65535 } end diff --git a/app/models/cms/group_section.rb b/app/models/cms/group_section.rb index 4f478cbcbd..5be54918e3 100644 --- a/app/models/cms/group_section.rb +++ b/app/models/cms/group_section.rb @@ -1,7 +1,6 @@ class CMS::GroupSection < ApplicationRecord self.table_name = :cms_group_sections - attr_accessible :section, :group belongs_to :group, :class_name => 'CMS::Group' belongs_to :section, :class_name => 'CMS::Section' end diff --git a/app/models/cms/permission.rb b/app/models/cms/permission.rb index e228b10781..72950f735c 100644 --- a/app/models/cms/permission.rb +++ b/app/models/cms/permission.rb @@ -1,11 +1,10 @@ -class CMS::Permission < ApplicationRecord - attr_accessible :group, :account +# frozen_string_literal: true +class CMS::Permission < ApplicationRecord self.table_name = :cms_permissions validates :name, length: { maximum: 255 } belongs_to :group, :class_name => 'CMS::Group', inverse_of: :permissions belongs_to :account, inverse_of: :permissions - end diff --git a/app/models/line_item.rb b/app/models/line_item.rb index 1cc21d41a1..0f7eee6fd2 100644 --- a/app/models/line_item.rb +++ b/app/models/line_item.rb @@ -7,8 +7,6 @@ class LineItem < ApplicationRecord audited associated_with: :invoice, allow_mass_assignment: true - attr_accessible :name, :description, :cost, :finished_at, :quantity, :started_at - validates :name, :description, :type, :contract_type, length: { maximum: 255 } validates :type, inclusion: {in: [LineItem::PlanCost, LineItem::VariableCost].flat_map { |klass| [klass, klass.to_s]}, allow_blank: true} diff --git a/app/models/onboarding.rb b/app/models/onboarding.rb index b7f3072338..0aadd9ca15 100644 --- a/app/models/onboarding.rb +++ b/app/models/onboarding.rb @@ -1,6 +1,4 @@ class Onboarding < ApplicationRecord - attr_accessible :wizard_state - belongs_to :account validates :account_id, presence: true diff --git a/app/models/partner.rb b/app/models/partner.rb index b50046f7c3..4aea0b6107 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -1,8 +1,7 @@ class Partner < ApplicationRecord include SystemName - has_system_name protected: true, uniqueness_scope: true - attr_accessible :api_key, :name + has_system_name uniqueness_scope: true has_many :providers, class_name: "Account" has_many :application_plans diff --git a/app/models/post.rb b/app/models/post.rb index 83cc5fcecf..7ccb462b8d 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -4,8 +4,6 @@ class Post < ApplicationRecord after_commit(:on => :destroy) { update_cached_fields } after_commit :update_topic_delta_index - attr_accessible :body, :markup_type, :anonymous_user - include Indices::TopicIndex::ForPost # author of post diff --git a/app/models/referrer_filter.rb b/app/models/referrer_filter.rb index d4c719c958..15041784b5 100644 --- a/app/models/referrer_filter.rb +++ b/app/models/referrer_filter.rb @@ -12,8 +12,6 @@ class ReferrerFilter < ApplicationRecord validate :keys_limit_reached - attr_accessible :application, :value - attr_readonly :value after_commit :destroy_backend_value, on: :destroy, unless: :destroyed_by_association diff --git a/test/unit/cms/portlet_test.rb b/test/unit/cms/portlet_test.rb index 16edb806dd..3f3e67618c 100644 --- a/test/unit/cms/portlet_test.rb +++ b/test/unit/cms/portlet_test.rb @@ -25,7 +25,6 @@ class CMS::PortletTest < ActiveSupport::TestCase class CustomPortlet < CMS::Portlet::Base attributes :fancyness - attr_accessible :fancyness validates_presence_of :fancyness end From bb31d0134ca11e695094c77feadc4c862b427a8d Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 19 Mar 2026 00:53:49 +0100 Subject: [PATCH 17/29] Remove attr_protected from multiple models: - Switches - SystemName - Finance::BillingStrategy - Alert - Forum - Invitation - MessageRecipient - Metric - Moderatorship - PaymentDetail - PaymentTransaction - PlanMetric - Profile - Topic - TopicCategory - UserTopic --- app/lib/switches.rb | 1 - app/lib/system_name.rb | 1 - app/models/alert.rb | 2 -- app/models/finance/billing_strategy.rb | 2 -- app/models/forum.rb | 2 -- app/models/invitation.rb | 2 -- app/models/message_recipient.rb | 2 -- app/models/metric.rb | 1 - app/models/moderatorship.rb | 2 -- app/models/payment_detail.rb | 2 -- app/models/payment_transaction.rb | 2 -- app/models/plan_metric.rb | 2 -- app/models/profile.rb | 2 -- app/models/topic.rb | 1 - app/models/topic_category.rb | 2 -- app/models/user_topic.rb | 2 -- 16 files changed, 28 deletions(-) diff --git a/app/lib/switches.rb b/app/lib/switches.rb index 6851e07beb..c2d9d01802 100644 --- a/app/lib/switches.rb +++ b/app/lib/switches.rb @@ -164,7 +164,6 @@ def assign_switches(*switch_names) included do SWITCHES.each do |name| attr_name = "#{name}_switch" - attr_protected attr_name # Switches State Machine # diff --git a/app/lib/system_name.rb b/app/lib/system_name.rb index d6d16fef97..3ac3ff95af 100644 --- a/app/lib/system_name.rb +++ b/app/lib/system_name.rb @@ -20,7 +20,6 @@ def validates_system_name(opts = {}) end def has_system_name(opts = {}) - attr_protected :system_name if opts.delete(:protected) self._system_human_name = opts.delete(:human_name) || :name validates_system_name(opts) end diff --git a/app/models/alert.rb b/app/models/alert.rb index c38bb312d0..1738492338 100644 --- a/app/models/alert.rb +++ b/app/models/alert.rb @@ -15,8 +15,6 @@ class Alert < ApplicationRecord has_one :user_account, through: :cinstance - attr_protected :account_id, :cinstance_id, :tenant_id - validates :account, :timestamp, :state, presence: true validates :utilization, :level, :alert_id, :cinstance, presence: true validates :alert_id, uniqueness: { scope: :account_id, case_sensitive: true } diff --git a/app/models/finance/billing_strategy.rb b/app/models/finance/billing_strategy.rb index 2a73a3b719..73326c6eca 100644 --- a/app/models/finance/billing_strategy.rb +++ b/app/models/finance/billing_strategy.rb @@ -20,8 +20,6 @@ class << self belongs_to :account alias_method :provider, :account - attr_protected :account_id, :tenant_id, :audit_ids - accepts_nested_attributes_for :account validates :currency, inclusion: { in: CURRENCIES.values, message: :invalid } diff --git a/app/models/forum.rb b/app/models/forum.rb index 95199ffa06..1cecd60e33 100644 --- a/app/models/forum.rb +++ b/app/models/forum.rb @@ -27,8 +27,6 @@ class Forum < ApplicationRecord scope :ordered, -> { order('position') } - attr_protected :topics_count, :posts_count, :account_id, :tenant_id - def latest_posts posts.joins(:topic).order('posts.created_at DESC') end diff --git a/app/models/invitation.rb b/app/models/invitation.rb index 4e431b1828..bd5698338d 100644 --- a/app/models/invitation.rb +++ b/app/models/invitation.rb @@ -18,8 +18,6 @@ class Invitation < ApplicationRecord # so after first call it sets the sent_at attribute and breaks the infinite loop after_commit :notify_invitee, :on => :create, :unless => :sent? - attr_protected :accepted_at, :tenant_id - default_scope -> { ordering { System::Database.oracle? ? sent_at.desc.op('', sql('NULLS LAST')) : sent_at.desc } } scope :pending, -> { where(accepted_at: nil) } diff --git a/app/models/message_recipient.rb b/app/models/message_recipient.rb index abb92a810e..232451cd82 100644 --- a/app/models/message_recipient.rb +++ b/app/models/message_recipient.rb @@ -29,8 +29,6 @@ class MessageRecipient < ApplicationRecord validates :message_id, :kind, :receiver_id, :receiver_type, presence: true validates :receiver_type, :kind, :state, length: { maximum: 255 } - attr_protected :message_id, :receiver_id, :receiver_type, :tenant_id - # Make this class look like the actual message delegate :sender, :subject, :body, :recipients, :to, :cc, :bcc, :created_at, :sender_name, :to => :message diff --git a/app/models/metric.rb b/app/models/metric.rb index 5db3afa2b2..2e0c822d33 100644 --- a/app/models/metric.rb +++ b/app/models/metric.rb @@ -26,7 +26,6 @@ class Metric < ApplicationRecord acts_as_tree - attr_protected :service_id, :parent_id, :tenant_id, :audit_ids validates :unit, presence: true, unless: :child? validates :friendly_name, uniqueness: { scope: %i[owner_type owner_id], case_sensitive: true }, presence: true validates :system_name, :unit, :friendly_name, :owner_type, length: { maximum: 255 } diff --git a/app/models/moderatorship.rb b/app/models/moderatorship.rb index 45d095309c..4a8d9381ee 100644 --- a/app/models/moderatorship.rb +++ b/app/models/moderatorship.rb @@ -4,8 +4,6 @@ class Moderatorship < ApplicationRecord validates :user_id, :forum_id, presence: true validate :uniqueness_of_relationship - attr_protected :forum_id, :user_id, :tenant_id - protected def uniqueness_of_relationship diff --git a/app/models/payment_detail.rb b/app/models/payment_detail.rb index 40b9cdb2f8..4600bd5f8d 100644 --- a/app/models/payment_detail.rb +++ b/app/models/payment_detail.rb @@ -12,8 +12,6 @@ class PaymentDetail < ApplicationRecord audited allow_mass_assignment: true - attr_protected :account_id, :audit_ids - validates :credit_card_partial_number, length: { :maximum => 4, :allow_blank => true, :message => "must be the final 4 digits only" } diff --git a/app/models/payment_transaction.rb b/app/models/payment_transaction.rb index 3b298bf2da..6c8f367236 100644 --- a/app/models/payment_transaction.rb +++ b/app/models/payment_transaction.rb @@ -13,8 +13,6 @@ class PaymentTransaction < ApplicationRecord validates :currency, length: {maximum: 4} validates :message, :reference, :action, length: {maximum: 255} - attr_protected :account_id, :invoice_id, :success, :test, :tenant_id - scope :failed, -> { where(:success => false) } scope :succeeded, -> { where(:success => true) } scope :oldest_first, -> { order(:created_at) } diff --git a/app/models/plan_metric.rb b/app/models/plan_metric.rb index 2250582d2f..d4ab8bb13b 100644 --- a/app/models/plan_metric.rb +++ b/app/models/plan_metric.rb @@ -2,8 +2,6 @@ class PlanMetric < ApplicationRecord belongs_to :plan, :polymorphic => true belongs_to :metric - attr_protected :plan_id, :metric_id, :plan_type, :tenant_id - validates :plan, presence: true validates :metric, presence: true end diff --git a/app/models/profile.rb b/app/models/profile.rb index 81463cfc9f..fe5b4df767 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -13,8 +13,6 @@ class Profile < ApplicationRecord # with the account validation that causes profile#valid? to be false before_save :validate_presence_of_account - attr_protected :account_id, :tenant_id, :audit_ids - after_initialize :set_company_size audited diff --git a/app/models/topic.rb b/app/models/topic.rb index bc3057c672..702337d40e 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -61,7 +61,6 @@ def self.with_post_by(user) end attr_accessor :body, :quiz, :quiz_id, :first_name, :last_name, :email, :human, :markup_type, :anonymous_user - attr_accessible :markup_type, :title, :body, :quiz, :quiz_id, :first_name, :last_name, :email, :tag_list, :anonymous_user, :tag_list attr_readonly :posts_count, :hits diff --git a/app/models/topic_category.rb b/app/models/topic_category.rb index 5df6d7693e..429fe1f4e6 100644 --- a/app/models/topic_category.rb +++ b/app/models/topic_category.rb @@ -3,8 +3,6 @@ class TopicCategory < ApplicationRecord #TODO: Rails 4 changes to has_many …, -> { order(…) } has_many :topics, -> { sticky_first.last_updated_first }, :foreign_key => :category_id - attr_protected :forum_id, :tenant_id - validates :name, presence: true, length: {maximum: 255} validates :name, uniqueness: { scope: :forum_id, case_sensitive: true } diff --git a/app/models/user_topic.rb b/app/models/user_topic.rb index 4eec433390..a59a19fc7c 100644 --- a/app/models/user_topic.rb +++ b/app/models/user_topic.rb @@ -3,8 +3,6 @@ class UserTopic < ApplicationRecord belongs_to :user belongs_to :topic - attr_protected :user_id, :topic_id, :tenant_id - validates :user, presence: true validates :topic, presence: true From 3614e00d2ddba34ade0eecbfde29c27afadea6cd Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Tue, 24 Mar 2026 12:51:24 +0100 Subject: [PATCH 18/29] Fix a groups controller section ID issue --- .../provider/admin/cms/groups_controller.rb | 24 ++++++++++---- .../admin/cms/groups_controller_test.rb | 31 ++++++++++++++++++- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/app/controllers/provider/admin/cms/groups_controller.rb b/app/controllers/provider/admin/cms/groups_controller.rb index ccf300aaa1..cca387a58b 100644 --- a/app/controllers/provider/admin/cms/groups_controller.rb +++ b/app/controllers/provider/admin/cms/groups_controller.rb @@ -1,7 +1,10 @@ +# frozen_string_literal: true + class Provider::Admin::CMS::GroupsController < Provider::Admin::CMS::BaseController - before_action :available_groups, :only => [:edit, :new ] - before_action :available_sections, :only => [:edit, :new ] + before_action :available_groups, :only => %i[edit new] + before_action :available_sections, :only => %i[edit new] before_action :authorize_groups + before_action :validate_section_ids, only: %i[create update] activate_menu :audience, :cms, :groups @@ -24,7 +27,7 @@ def edit end def create - @group = current_account.provided_groups.build(sections_params) + @group = current_account.provided_groups.build(group_params) if @group.save redirect_to({ action: :index }, success: t('.success')) @@ -38,7 +41,7 @@ def create def update @group = current_account.provided_groups.find(params[:id]) - if @group.update(sections_params) + if @group.update(group_params) redirect_to({ action: :index }, success: t('.success')) else available_groups @@ -55,8 +58,8 @@ def destroy protected - def sections_params - params.require(:cms_group).permit(:name, section_ids: []) + def group_params + @group_params ||= params.require(:cms_group).permit(:name, section_ids: []) end def available_groups @@ -71,4 +74,13 @@ def authorize_groups authorize! :manage, :groups end + # Ensure that all section IDs provided in the parameters belong to the account, which is the group owner + # Providing a non-existent section ID, or the one that belongs to another provider results in a Not Found error + def validate_section_ids + section_ids = group_params[:section_ids] + section_ids.reject!(&:empty?) + exising_section_ids = current_account.section_ids.map(&:to_s) + + render_error(:not_found, status: :not_found) if (section_ids.map(&:to_s) - exising_section_ids).any? + end end diff --git a/test/integration/provider/admin/cms/groups_controller_test.rb b/test/integration/provider/admin/cms/groups_controller_test.rb index 943a558869..4adac652c3 100644 --- a/test/integration/provider/admin/cms/groups_controller_test.rb +++ b/test/integration/provider/admin/cms/groups_controller_test.rb @@ -31,5 +31,34 @@ def setup assert_equal ['New Name'], @provider.provided_groups.pluck(:name) assert_equal [another_section.id], group.sections.pluck(:id) end -end + test '#update allowed sections with invalid IDs' do + group = @provider.provided_groups.create(name: 'Group') + group.sections << @section + assert_equal ['Group'], @provider.provided_groups.pluck(:name) + assert_equal [@section.id], group.sections.pluck(:id) + + invalid_section_id = CMS::Section.maximum(:id) + 1 + + assert_no_change of: -> { group.reload.sections.pluck(:id) } do + put provider_admin_cms_group_path(group), params: { cms_group: { name: 'New Name', section_ids: ['', invalid_section_id] } } + end + + assert_response :not_found + end + + test "#update group with another provider's section" do + group = @provider.provided_groups.create(name: 'Group') + assert_empty group.sections + + another_provider = FactoryBot.create(:provider_account) + another_provider.settings.allow_groups! + another_section = FactoryBot.create(:cms_section, parent: another_provider.sections.first, provider: another_provider, public: false) + + assert_no_difference group.sections.method(:count) do + put provider_admin_cms_group_path(group), params: { cms_group: { section_ids: ['', @section.id, another_section.id] } } + end + + assert_response :not_found + end +end From 900488b1359433add7ebd46e3878132ed61ed10c Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Tue, 24 Mar 2026 13:20:43 +0100 Subject: [PATCH 19/29] Fix sites settings controller --- app/controllers/sites/settings_controller.rb | 3 +- config/locales/en.yml | 1 + .../sites/emails_controller_test.rb | 2 +- .../sites/settings_controller_test.rb | 48 ++++++++++--------- 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/app/controllers/sites/settings_controller.rb b/app/controllers/sites/settings_controller.rb index 423e7004ea..ca7feedb1c 100644 --- a/app/controllers/sites/settings_controller.rb +++ b/app/controllers/sites/settings_controller.rb @@ -16,7 +16,8 @@ def update if @settings.update(settings_params) redirect_to edit_admin_site_settings_path, success: t('.success') else - redirect_to edit_admin_site_settings_path, danger: t('.error') + flash.now[:danger] = t('.error') + render action: 'edit' end end diff --git a/config/locales/en.yml b/config/locales/en.yml index fed1697fda..9942cb065e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1287,6 +1287,7 @@ en: settings: update: success: Settings updated + error: Settings could not be updated securities: edit: selector_label: Protection against bots on the developer portal diff --git a/test/integration/sites/emails_controller_test.rb b/test/integration/sites/emails_controller_test.rb index 945ae8380e..768b0ac477 100644 --- a/test/integration/sites/emails_controller_test.rb +++ b/test/integration/sites/emails_controller_test.rb @@ -2,7 +2,7 @@ require 'test_helper' -class Sites::SettingsControllerTest < ActionDispatch::IntegrationTest +class Sites::EmailsControllerTest < ActionDispatch::IntegrationTest test 'show emails tab if not master account' do provider = FactoryBot.create(:provider_account) diff --git a/test/integration/sites/settings_controller_test.rb b/test/integration/sites/settings_controller_test.rb index 10a6c49dba..8af078c879 100644 --- a/test/integration/sites/settings_controller_test.rb +++ b/test/integration/sites/settings_controller_test.rb @@ -4,29 +4,6 @@ class Sites::SettingsControllerTest < ActionDispatch::IntegrationTest - test 'show emails tab if not master account' do - provider = FactoryBot.create(:provider_account) - - login_provider provider - - get edit_admin_site_emails_path - - assert_response :success - end - - test 'do not show emails tab if master account' do - ThreeScale.config.stubs(onpremises: true, tenant_mode: 'master') - - member = FactoryBot.create(:simple_admin, account: master_account) - member.activate! - - login! master_account, user: member - - get edit_admin_site_emails_path - - assert_response :forbidden - end - test 'update credit card policies paths successfully' do provider = FactoryBot.create(:provider_account) login_provider provider @@ -66,4 +43,29 @@ class Sites::SettingsControllerTest < ActionDispatch::IntegrationTest assert_equal '', provider.settings.cc_terms_path assert_equal '', provider.settings.cc_privacy_path end + + test 'update with invalid values shows errors' do + provider = FactoryBot.create(:provider_account) + login_provider provider + + long_value = "x"*256 + + put admin_site_settings_path, params: { + settings: { + cc_terms_path: long_value, + cc_privacy_path: long_value, + cc_refunds_path: long_value + } + } + + assert_response :success + assert_template :edit + assert_equal "Settings could not be updated", flash[:danger] + + page = Nokogiri::HTML4::Document.parse(response.body) + errors = page.xpath("//p[@class='pf-c-form__helper-text pf-m-error']") + + assert_equal 3, errors.length + assert_equal ["is too long (maximum is 255 characters)"], errors.map(&:text).uniq + end end From 0500870936a6661f21347d758589ea21349527cd Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Tue, 24 Mar 2026 13:38:53 +0100 Subject: [PATCH 20/29] Small fixes after peer review --- .../admin/fields_definitions_controller.rb | 2 +- features/support/paths.rb | 4 ---- test/unit/sso_token_test.rb | 14 ++++++++++++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/app/controllers/admin/fields_definitions_controller.rb b/app/controllers/admin/fields_definitions_controller.rb index f2567b07db..f0a9e2d802 100644 --- a/app/controllers/admin/fields_definitions_controller.rb +++ b/app/controllers/admin/fields_definitions_controller.rb @@ -80,7 +80,7 @@ def field_definition_params end def sort_params - @sort_params = params.fetch(:fields_definition, []) + @sort_params ||= params.fetch(:fields_definition, []) end def field_definitions diff --git a/features/support/paths.rb b/features/support/paths.rb index 404e334209..090924da06 100644 --- a/features/support/paths.rb +++ b/features/support/paths.rb @@ -700,10 +700,6 @@ def path_to(page_name, *args) # rubocop:disable Metrics/AbcSize, Metrics/Cycloma when 'the edit webhooks page' edit_provider_admin_webhooks_path - #Previous routes still used. - when 'the provider access rules page' - '/admin/settings/accessrules' - when 'the terms of service page' '/termsofservice' when 'the privacy policy page' diff --git a/test/unit/sso_token_test.rb b/test/unit/sso_token_test.rb index d845b494c7..0578dd06a3 100644 --- a/test/unit/sso_token_test.rb +++ b/test/unit/sso_token_test.rb @@ -21,6 +21,20 @@ def setup assert_not_nil sso_token.expires_at end + test "SSOToken rejects unpermitted parameters" do + buyer = FactoryBot.create(:buyer_account, provider_account: @provider) + user_id = buyer.users.first.id + params = ActionController::Parameters.new({ user_id: user_id, expires_in: 6000 }) + + assert_raises ActiveModel::ForbiddenAttributesError do + SSOToken.new params + end + + sso_token = SSOToken.new params.permit(:user_id, :expires_in) + assert_equal user_id, sso_token.user_id + assert_equal 6000, sso_token.expires_in + end + test "creating a valid sso token using username" do buyer = FactoryBot.create(:buyer_account, :provider_account => @provider) From eeb38396d3a2e60f38db670c2ebe32994e995863 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 16 Apr 2026 23:39:30 +0200 Subject: [PATCH 21/29] Update controllers and test related to Settings model --- .../sites/developer_portals_controller.rb | 4 +- app/controllers/sites/forums_controller.rb | 6 +- .../admin/securities_controller_test.rb} | 83 +++++++++++++++---- .../developer_portals_controller_test.rb | 66 +++++++++++++++ ..._test.rb => securities_controller_test.rb} | 70 +++++++++++++--- 5 files changed, 196 insertions(+), 33 deletions(-) rename test/integration/{sites/admin_security_test.rb => provider/admin/securities_controller_test.rb} (80%) create mode 100644 test/integration/sites/developer_portals_controller_test.rb rename test/integration/sites/{security_test.rb => securities_controller_test.rb} (75%) diff --git a/app/controllers/sites/developer_portals_controller.rb b/app/controllers/sites/developer_portals_controller.rb index c2e0b6aa99..6694405700 100644 --- a/app/controllers/sites/developer_portals_controller.rb +++ b/app/controllers/sites/developer_portals_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Sites::DeveloperPortalsController < Sites::BaseController activate_menu :audience, :cms @@ -17,7 +19,7 @@ def update private def settings_params - params[:settings] + params.require(:settings).permit(:cms_escape_draft_html, :cms_escape_published_html) end def settings diff --git a/app/controllers/sites/forums_controller.rb b/app/controllers/sites/forums_controller.rb index a145e80a08..00f352dee4 100644 --- a/app/controllers/sites/forums_controller.rb +++ b/app/controllers/sites/forums_controller.rb @@ -8,7 +8,7 @@ def edit end def update - if @settings.update(params[:settings]) + if @settings.update(settings_params) redirect_to edit_admin_site_forum_url, success: t('.success') else flash.now[:danger] = t('.error') @@ -29,4 +29,8 @@ def find_settings def active_settings_menu activate_menu :audience, :cms, :forum_settings end + + def settings_params + params.require(:settings).permit(:forum_enabled, :forum_public, :anonymous_posts_enabled) + end end diff --git a/test/integration/sites/admin_security_test.rb b/test/integration/provider/admin/securities_controller_test.rb similarity index 80% rename from test/integration/sites/admin_security_test.rb rename to test/integration/provider/admin/securities_controller_test.rb index 1fb2e59366..cf6fef25d7 100644 --- a/test/integration/sites/admin_security_test.rb +++ b/test/integration/provider/admin/securities_controller_test.rb @@ -2,12 +2,11 @@ require 'test_helper' -class Sites::AdminSecurityTest < ActionDispatch::IntegrationTest +class Provider::Admin::SecuritiesControllerTest < ActionDispatch::IntegrationTest setup do @provider = FactoryBot.create(:provider_account) login_provider @provider - host! @provider.external_admin_domain Rails.cache.clear end @@ -18,18 +17,50 @@ class Sites::AdminSecurityTest < ActionDispatch::IntegrationTest assert_xpath '//input[@type="radio"][@name="settings[admin_bot_protection_level]"][@value="none"][@checked]' end - test 'edit without captcha' do + test 'edit without captcha configured shows warning' do Recaptcha.expects(:captcha_configured?).returns(false).twice + get edit_provider_admin_security_path + assert_response :success assert_match 'reCAPTCHA has not been configured correctly', response.body end - test 'edit with captcha' do + test 'edit with captcha configured shows hint' do Recaptcha.expects(:captcha_configured?).returns(true).twice + get edit_provider_admin_security_path + + assert_response :success + assert_match 'reCAPTCHA v3 will invisibly verify interactions', response.body + end + + test 'updates admin bot protection level setting' do + assert_equal :none, @provider.settings.admin_bot_protection_level + + put provider_admin_security_path, params: { + settings: { + admin_bot_protection_level: 'captcha' + } + } + + assert_redirected_to edit_provider_admin_security_path + assert_equal 'Security settings updated', flash[:success] + + @provider.settings.reload + assert_equal :captcha, @provider.settings.admin_bot_protection_level + end + + test 'update with invalid value shows error' do + put provider_admin_security_path, params: { + settings: { + admin_bot_protection_level: 'x' * 256 + } + } + assert_response :success - assert_not_match 'reCAPTCHA has not been configured correctly', response.body + assert_template :edit + assert_equal 'There were problems saving the settings', flash[:danger] end test 'displays Permissions-Policy header field' do @@ -55,19 +86,6 @@ class Sites::AdminSecurityTest < ActionDispatch::IntegrationTest assert_equal policy_value, setting.value end - test 'updates admin bot protection level setting' do - put provider_admin_security_path, params: { - settings: { - admin_bot_protection_level: 'captcha' - } - } - - assert_redirected_to edit_provider_admin_security_path - - @provider.settings.reload - assert_equal :captcha, @provider.settings.admin_bot_protection_level - end - test 'omitting header field deletes existing setting' do @provider.account_settings.create!( type: 'PermissionsPolicyHeaderAdmin', @@ -203,4 +221,33 @@ class Sites::AdminSecurityTest < ActionDispatch::IntegrationTest assert_response :success assert response.headers.key?('Permissions-Policy'), 'Permissions-Policy header should be present' end + + test 'requires authentication' do + logout! + + get edit_provider_admin_security_path + assert_redirected_to provider_login_path + + put provider_admin_security_path, params: { + settings: { admin_bot_protection_level: 'captcha' } + } + assert_redirected_to provider_login_path + end + + test 'member can update the settings' do + member = FactoryBot.create(:member, account: @provider) + member.activate! + logout! + login_provider @provider, user: member + + get edit_provider_admin_security_path + assert_response :success + + put provider_admin_security_path, params: { + settings: { admin_bot_protection_level: 'captcha' } + } + + assert_redirected_to edit_provider_admin_security_url + assert_equal 'Security settings updated', flash[:success] + end end diff --git a/test/integration/sites/developer_portals_controller_test.rb b/test/integration/sites/developer_portals_controller_test.rb new file mode 100644 index 0000000000..38f1425416 --- /dev/null +++ b/test/integration/sites/developer_portals_controller_test.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Sites::DeveloperPortalsControllerTest < ActionDispatch::IntegrationTest + + setup do + @provider = FactoryBot.create(:provider_account) + login_provider @provider + end + + test 'edit shows developer portal settings' do + get edit_admin_site_developer_portal_path + + assert_response :success + assert_match 'Cross-Site Scripting Protection', response.body + end + + test 'update to enable both settings' do + put admin_site_developer_portal_path, params: { + settings: { + cms_escape_draft_html: '1', + cms_escape_published_html: '1' + } + } + + assert_redirected_to edit_admin_site_developer_portal_path + assert_equal 'Developer Portal settings updated', flash[:success] + + @provider.settings.reload + assert @provider.settings.cms_escape_draft_html? + assert @provider.settings.cms_escape_published_html? + end + + test 'update to disable both settings' do + @provider.settings.update(cms_escape_draft_html: true, cms_escape_published_html: true) + + put admin_site_developer_portal_path, params: { + settings: { + cms_escape_draft_html: '0', + cms_escape_published_html: '0' + } + } + + assert_redirected_to edit_admin_site_developer_portal_path + assert_equal 'Developer Portal settings updated', flash[:success] + + @provider.settings.reload + assert_not @provider.settings.cms_escape_draft_html? + assert_not @provider.settings.cms_escape_published_html? + end + + test 'update with invalid params shows error' do + Settings.any_instance.stubs(:update).returns(false) + + put admin_site_developer_portal_path, params: { + settings: { + cms_escape_draft_html: true + } + } + + assert_response :success + assert_template :edit + assert_equal 'There were problems saving the settings', flash[:danger] + end +end diff --git a/test/integration/sites/security_test.rb b/test/integration/sites/securities_controller_test.rb similarity index 75% rename from test/integration/sites/security_test.rb rename to test/integration/sites/securities_controller_test.rb index 0731d2c723..3bd35f6ed1 100644 --- a/test/integration/sites/security_test.rb +++ b/test/integration/sites/securities_controller_test.rb @@ -2,27 +2,58 @@ require 'test_helper' -class Sites::SecurityTest < ActionDispatch::IntegrationTest +class Sites::SecuritiesControllerTest < ActionDispatch::IntegrationTest setup do @provider = FactoryBot.create(:provider_account) login_provider @provider - host! @provider.external_admin_domain Rails.cache.clear end - test 'edit without captcha' do + test 'edit without captcha configured shows warning' do Recaptcha.expects(:captcha_configured?).returns(false).twice + get edit_admin_site_security_path + assert_response :success assert_match 'reCAPTCHA has not been configured correctly', response.body end - test 'edit with captcha' do + test 'edit with captcha configured shows hint' do Recaptcha.expects(:captcha_configured?).returns(true).twice + get edit_admin_site_security_path + + assert_response :success + assert_match 'reCAPTCHA v3 will invisibly verify interactions', response.body + end + + test 'updates spam protection level setting' do + assert_equal :none, @provider.settings.spam_protection_level + + put admin_site_security_path, params: { + settings: { + spam_protection_level: 'captcha' + } + } + + assert_redirected_to edit_admin_site_security_path + assert_equal 'Security settings updated', flash[:success] + + @provider.settings.reload + assert_equal :captcha, @provider.settings.spam_protection_level + end + + test 'update spam protection with invalid value shows error' do + put admin_site_security_path, params: { + settings: { + spam_protection_level: 'x' * 256 + } + } + assert_response :success - assert_not_match 'reCAPTCHA has not been configured correctly', response.body + assert_template :edit + assert_equal 'There were problems saving the settings', flash[:danger] end test 'displays Permissions-Policy header field' do @@ -138,17 +169,30 @@ class Sites::SecurityTest < ActionDispatch::IntegrationTest assert_equal "default-src 'none'", setting.value end - test 'updates spam protection level setting' do + test 'requires authentication' do + logout! + + get edit_admin_site_security_path + assert_redirected_to provider_login_path + put admin_site_security_path, params: { - settings: { - spam_protection_level: 'captcha' - } + settings: { spam_protection_level: 'captcha' } } + assert_redirected_to provider_login_path + end - assert_redirected_to edit_admin_site_security_path + test 'requires authorization to manage settings' do + member = FactoryBot.create(:member, account: @provider) + member.activate! + logout! + login_provider @provider, user: member - @provider.settings.reload - assert_equal :captcha, @provider.settings.spam_protection_level - end + get edit_admin_site_security_path + assert_response :forbidden + put admin_site_security_path, params: { + settings: { spam_protection_level: 'captcha' } + } + assert_response :forbidden + end end From d2538b9acc1144cbc4b9ad8f4a062bce9e6c8719 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Fri, 17 Apr 2026 13:25:21 +0200 Subject: [PATCH 22/29] Fix DeveloperPortal::Admin::Messages::OutboxController and add tests --- .../admin/messages/outbox_controller.rb | 15 ++--- .../outbox_controller_integration_test.rb | 60 +++++++++++++++++++ 2 files changed, 66 insertions(+), 9 deletions(-) diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/messages/outbox_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/messages/outbox_controller.rb index 07d7254d2a..f2633d3984 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/messages/outbox_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/messages/outbox_controller.rb @@ -1,13 +1,12 @@ # encoding: UTF-8 class DeveloperPortal::Admin::Messages::OutboxController < DeveloperPortal::BaseController before_action :ensure_buyer_domain - before_action :build_message, :only => [:new, :create] activate_menu :dashboard, :messages liquify prefix: 'messages/outbox' def index - messages = current_account.messages.not_system.latest_first.paginate(page: message_params[:page]) + messages = current_account.messages.not_system.latest_first.paginate(page: params[:page]) collection = Liquid::Drops::Collection.for_drop(Liquid::Drops::Message).new(messages) pagination = Liquid::Drops::Pagination.new(messages, self) @@ -15,17 +14,18 @@ def index end def show - message = current_account.messages.find(message_params[:id]) + message = current_account.messages.find(params[:id]) assign_drops message: Liquid::Drops::Message.new(message) end def new + @message = current_account.messages.build @message.to current_account.provider_account assign_drops message: Liquid::Drops::Message.new(@message) end def destroy - @message = current_account.messages.find(message_params[:id]) + @message = current_account.messages.find(params[:id]) @message.hide! flash[:notice] = 'Message was deleted.' @@ -33,6 +33,7 @@ def destroy end def create + @message = current_account.messages.build(**message_params, origin: "web") if @message.valid? enqueue_message_and_respond else @@ -55,11 +56,7 @@ def enqueue_message_and_respond end end - def build_message - @message = current_account.messages.build((message_params[:message] || {}).merge(:origin => "web")) - end - def message_params - params.permit!.to_h + params.require(:message).permit(:subject, :body) end end diff --git a/test/integration/developer_portal/admin/messages/outbox_controller_integration_test.rb b/test/integration/developer_portal/admin/messages/outbox_controller_integration_test.rb index 44fe69d731..c2c90b4822 100644 --- a/test/integration/developer_portal/admin/messages/outbox_controller_integration_test.rb +++ b/test/integration/developer_portal/admin/messages/outbox_controller_integration_test.rb @@ -27,4 +27,64 @@ def test_show assert_equal sent_message.id, assigns(:_assigned_drops)['message'].id end + + def test_new + get admin_messages_new_path + assert_response :success + + assigned_message = assigns(:_assigned_drops)['message'] + assert_not_nil assigned_message + assert_equal @buyer.name, assigned_message.sender + assert_equal @provider.org_name, assigned_message.to + end + + def test_destroy + sent_message = FactoryBot.create(:message, sender: @buyer) + assert_not sent_message.hidden? + + delete admin_messages_outbox_path(sent_message) + + assert_response :redirect + assert_redirected_to admin_messages_outbox_index_path + assert_equal 'Message was deleted.', flash[:notice] + assert sent_message.reload.hidden? + end + + def test_create_with_valid_message + assert_difference 'Message.count', 0 do # Message created after worker processes + post admin_messages_outbox_index_path, params: { + message: { subject: 'Test Subject', body: 'Test Body' } + } + end + + assert_response :redirect + assert_redirected_to admin_messages_root_path + assert_equal 'Message was sent.', flash[:notice] + + # Process the message worker + MessageWorker.drain + + message = Message.last + assert_not_nil message + assert_equal 'Test Subject', message.subject + assert_equal 'Test Body', message.body + assert_equal 'web', message.origin + assert_equal @buyer, message.sender + end + + def test_create_with_invalid_message + assert_no_difference 'Message.count' do + post admin_messages_outbox_index_path, params: { + message: { subject: '', body: 'Body without subject' } + } + end + + assert_response :redirect + assert_redirected_to admin_messages_new_path + assert_not_nil flash[:error] + assert_match(/subject/i, flash[:error]) + + MessageWorker.drain + assert_nil Message.last + end end From 356a4334e1b09e29bfdd3034c137809fcdf2a093 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Fri, 17 Apr 2026 16:00:36 +0200 Subject: [PATCH 23/29] Small fixes/improvements after peer review --- .../admin/fields_definitions_controller.rb | 2 +- .../provider/admin/cms/groups_controller.rb | 2 ++ .../provider/admin/cms/groups_controller_test.rb | 11 +++++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/fields_definitions_controller.rb b/app/controllers/admin/fields_definitions_controller.rb index f0a9e2d802..cd39d203b2 100644 --- a/app/controllers/admin/fields_definitions_controller.rb +++ b/app/controllers/admin/fields_definitions_controller.rb @@ -80,7 +80,7 @@ def field_definition_params end def sort_params - @sort_params ||= params.fetch(:fields_definition, []) + @sort_params ||= params.permit(fields_definition: [])[:fields_definition] || [] end def field_definitions diff --git a/app/controllers/provider/admin/cms/groups_controller.rb b/app/controllers/provider/admin/cms/groups_controller.rb index cca387a58b..dce706e7d7 100644 --- a/app/controllers/provider/admin/cms/groups_controller.rb +++ b/app/controllers/provider/admin/cms/groups_controller.rb @@ -78,6 +78,8 @@ def authorize_groups # Providing a non-existent section ID, or the one that belongs to another provider results in a Not Found error def validate_section_ids section_ids = group_params[:section_ids] + return if section_ids.blank? + section_ids.reject!(&:empty?) exising_section_ids = current_account.section_ids.map(&:to_s) diff --git a/test/integration/provider/admin/cms/groups_controller_test.rb b/test/integration/provider/admin/cms/groups_controller_test.rb index 4adac652c3..4ea266f26f 100644 --- a/test/integration/provider/admin/cms/groups_controller_test.rb +++ b/test/integration/provider/admin/cms/groups_controller_test.rb @@ -32,6 +32,17 @@ def setup assert_equal [another_section.id], group.sections.pluck(:id) end + test '#update when no sections selected' do + group = @provider.provided_groups.create(name: 'Group') + group.sections << @section + assert_equal ['Group'], @provider.provided_groups.pluck(:name) + assert_equal [@section.id], group.sections.pluck(:id) + + put provider_admin_cms_group_path(group), params: { cms_group: { name: 'Group', section_ids: [''] } } + + assert_empty group.sections.to_a + end + test '#update allowed sections with invalid IDs' do group = @provider.provided_groups.create(name: 'Group') group.sections << @section From 04506463c706a45bc481d92a13296624ae971dd6 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Mon, 20 Apr 2026 22:41:34 +0200 Subject: [PATCH 24/29] Fixes from peer review --- test/unit/sso_token_test.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/unit/sso_token_test.rb b/test/unit/sso_token_test.rb index 0578dd06a3..3b89e02056 100644 --- a/test/unit/sso_token_test.rb +++ b/test/unit/sso_token_test.rb @@ -8,17 +8,15 @@ def setup end test "creating a valid sso token using user_id" do - buyer = FactoryBot.create(:buyer_account, :provider_account => @provider) - - sso_token = SSOToken.new :user_id => buyer.users.first.id, :expires_in => 6000 + buyer = FactoryBot.create(:buyer_account, provider_account: @provider) - assert_nil sso_token.account - sso_token.account = @provider + sso_token = SSOToken.new user_id: buyer.users.first.id, account: @provider, expires_in: 6000 assert sso_token.save assert_not_nil sso_token.encrypted_token assert_not_nil sso_token.expires_at + assert_equal 'https', sso_token.protocol end test "SSOToken rejects unpermitted parameters" do From b25800be611e9bae6aa3b0cc3013bb3cc7db1cf7 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Wed, 22 Apr 2026 18:05:48 +0200 Subject: [PATCH 25/29] Make some controllers more secure by whitelising parameters --- .../admin/api/registry/policies_controller.rb | 2 + .../braintree_blue_controller.rb | 13 +++-- .../sites/usage_rules_controller.rb | 3 +- app/services/policies_config_params.rb | 2 +- .../account/braintree_blue_controller.rb | 8 ++- .../sites/usage_rules_controller_test.rb | 8 ++- .../api/registry/policies_controller_test.rb | 51 +++++++++++++++++++ .../proxy/policies_controller_test.rb | 26 ++++++++++ 8 files changed, 100 insertions(+), 13 deletions(-) diff --git a/app/controllers/admin/api/registry/policies_controller.rb b/app/controllers/admin/api/registry/policies_controller.rb index 32f6b1aa7d..b2c58bc619 100644 --- a/app/controllers/admin/api/registry/policies_controller.rb +++ b/app/controllers/admin/api/registry/policies_controller.rb @@ -54,6 +54,8 @@ def authorize_policies def policy_params policy_params = params.require(:policy) final_params = policy_params.permit(:name, :version) + # permit! is necessary here because the schema field accepts arbitrary nested JSON structures + # that cannot be whitelisted in advance. final_params.merge(schema: policy_params.require(:schema)).permit! end diff --git a/app/controllers/provider/admin/account/payment_gateways/braintree_blue_controller.rb b/app/controllers/provider/admin/account/payment_gateways/braintree_blue_controller.rb index a1157455dd..caf52c4485 100644 --- a/app/controllers/provider/admin/account/payment_gateways/braintree_blue_controller.rb +++ b/app/controllers/provider/admin/account/payment_gateways/braintree_blue_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Provider::Admin::Account::PaymentGateways::BraintreeBlueController < Provider::Admin::Account::BaseController after_action :check_multiple_payment_failures, only: [:hosted_success] @@ -7,8 +9,7 @@ class Provider::Admin::Account::PaymentGateways::BraintreeBlueController < Provi prepend_before_action :deny_on_premises activate_menu :account, :billing, :payment_details - def show - end + def show; end def edit current_account.require_billing_information! @@ -33,8 +34,7 @@ def update end def hosted_success - customer_info = params.require(:customer).permit!.to_h - braintree_response = braintree_blue_crypt.confirm(customer_info, params.require(:braintree).require(:nonce)) + braintree_response = braintree_blue_crypt.confirm(customer_params.to_h, params.require(:braintree).require(:nonce)) @payment_result = braintree_response&.success? if @payment_result @@ -97,4 +97,9 @@ def hack_errors current_account.billing_address.errors.instance_variable_set('@errors', new_errors) end + + def customer_params + billing_address_params = %i[company street_address postal_code locality region country_name] + params.require(:customer).permit(:first_name, :last_name, :phone, credit_card: { billing_address: billing_address_params }) + end end diff --git a/app/controllers/sites/usage_rules_controller.rb b/app/controllers/sites/usage_rules_controller.rb index 46c372518d..3d0acbd8b6 100644 --- a/app/controllers/sites/usage_rules_controller.rb +++ b/app/controllers/sites/usage_rules_controller.rb @@ -22,9 +22,10 @@ def find_settings def settings_params allowed_attrs = %i[ - useraccountarea_enabled signups_enabled strong_passwords_enabled public_search + useraccountarea_enabled signups_enabled public_search account_plans_ui_visible change_account_plan_permission service_plans_ui_visible change_service_plan_permission + account_approval_required hide_service cas_server_url ] params.require(:settings).permit(*allowed_attrs) end diff --git a/app/services/policies_config_params.rb b/app/services/policies_config_params.rb index 26b32afa99..27caf43e44 100644 --- a/app/services/policies_config_params.rb +++ b/app/services/policies_config_params.rb @@ -12,7 +12,7 @@ def call return policies_config_params if json_param? [policies_config_params].flatten.map do |policies_config| - policies_config.try(:permit!) + policies_config.try(:permit, :name, :version, :enabled, :removable, :id, :humanName, configuration: {}) end end diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/account/braintree_blue_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/account/braintree_blue_controller.rb index abbb81c266..e0126d123a 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/account/braintree_blue_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/account/braintree_blue_controller.rb @@ -19,8 +19,7 @@ def edit end def hosted_success - customer_info = params.require(:customer).permit!.to_h - braintree_response = braintree_blue_crypt.confirm(customer_info, params.require(:braintree).require(:nonce)) + braintree_response = braintree_blue_crypt.confirm(customer_params.to_h, params.require(:braintree).require(:nonce)) @payment_result = braintree_response&.success? if @payment_result update_user_and_perform_action!(braintree_response) @@ -51,5 +50,10 @@ def braintree_blue_crypt def after_hosted_success_without_plan_changes_path admin_account_braintree_blue_url end + + def customer_params + billing_address_params = %i[company street_address postal_code locality region country_name] + params.require(:customer).permit(:first_name, :last_name, :phone, credit_card: { billing_address: billing_address_params }) + end end end diff --git a/test/functional/sites/usage_rules_controller_test.rb b/test/functional/sites/usage_rules_controller_test.rb index 0e6dc82bca..6df97a9fc8 100644 --- a/test/functional/sites/usage_rules_controller_test.rb +++ b/test/functional/sites/usage_rules_controller_test.rb @@ -48,9 +48,8 @@ def setup test 'update with valid params' do @settings.update(strong_passwords_enabled: true, public_search: true) - %i[useraccountarea_enabled signups_enabled - account_plans_ui_visible service_plans_ui_visible - strong_passwords_enabled public_search].each do |setting| + %i[useraccountarea_enabled signups_enabled public_search + account_plans_ui_visible service_plans_ui_visible].each do |setting| assert @settings.send(setting), "#{setting} setting is not true as expected" end @@ -61,7 +60,6 @@ def setup settings: { useraccountarea_enabled: '0', signups_enabled: '0', - strong_passwords_enabled: '0', public_search: '0', account_plans_ui_visible: '0', service_plans_ui_visible: '0', @@ -77,7 +75,7 @@ def setup %i[useraccountarea_enabled signups_enabled account_plans_ui_visible service_plans_ui_visible - strong_passwords_enabled public_search].each do |setting| + hide_service public_search].each do |setting| assert_not @settings.send(setting), "#{setting} setting is not false as expected" end assert_equal "credit_card", @settings.change_account_plan_permission diff --git a/test/integration/admin/api/registry/policies_controller_test.rb b/test/integration/admin/api/registry/policies_controller_test.rb index d4318a78ea..3450fafbd9 100644 --- a/test/integration/admin/api/registry/policies_controller_test.rb +++ b/test/integration/admin/api/registry/policies_controller_test.rb @@ -22,6 +22,56 @@ def setup policy_params[:policy].each { |key, value| assert_equal(value, policy.public_send(key)) } end + test 'POST create accepts nested schema hash with complex structure from fixture' do + new_schema = JSON.parse(file_fixture('policies/apicast-policy.json').read) + + assert_difference(@provider.policies.method(:count), 1) do + post admin_api_registry_policies_path( + policy: { + name: new_schema['name'], + version: new_schema['version'], + schema: new_schema + }, + access_token: @access_token.plaintext_value + ) + end + assert_response :created + + policy = @provider.policies.last! + assert_equal new_schema['name'], policy.name + assert_equal new_schema['version'], policy.version + assert_equal new_schema, policy.schema + + # Verify deeply nested structure is preserved + assert_equal 'string', policy.schema.dig('configuration', 'properties', 'rules', 'items', 'properties', 'regex', 'type') + end + + test 'POST create accepts nested schema as a string' do + new_schema = file_fixture('policies/apicast-policy.json').read + parsed_schema = JSON.parse(new_schema) + + assert_difference(@provider.policies.method(:count), 1) do + post admin_api_registry_policies_path( + policy: { + name: parsed_schema['name'], + version: parsed_schema['version'], + schema: new_schema + }, + access_token: @access_token.plaintext_value + ) + end + assert_response :created + + policy = @provider.policies.last! + assert_equal parsed_schema['name'], policy.name + assert_equal parsed_schema['version'], policy.version + assert_equal parsed_schema, policy.schema + + # Verify deeply nested structure is preserved + assert_equal 'string', policy.schema.dig('configuration', 'properties', 'rules', 'items', 'properties', 'regex', 'type') + end + + test 'POST create responds with an error message when it is incorrect' do FactoryBot.create(:policy, policy_params[:policy].merge(account: @provider)) assert_no_difference(Policy.method(:count)) do @@ -266,3 +316,4 @@ def policy_params(token = @access_token.plaintext_value) { policy: @policy_attributes, access_token: token } end end + diff --git a/test/integration/admin/api/services/proxy/policies_controller_test.rb b/test/integration/admin/api/services/proxy/policies_controller_test.rb index 04612c8705..e78a2bb1e1 100644 --- a/test/integration/admin/api/services/proxy/policies_controller_test.rb +++ b/test/integration/admin/api/services/proxy/policies_controller_test.rb @@ -18,6 +18,32 @@ def setup end end + test 'updates policies config when policies_config is a JSON string' do + config = [ + { name: 'apicast', version: 'builtin', configuration: {}, enabled: true } + ] + + put admin_api_service_proxy_policies_path(@service, access_token: @access_token, format: :json), params: { + proxy: { policies_config: config.to_json } + }, as: :json + + assert_response :success + assert_equal config.map(&:stringify_keys), @service.reload.proxy.policies_config.as_json + end + + test 'updates policies config when policies_config is not a JSON string' do + config = [ + { name: 'apicast', version: 'builtin', configuration: {}, enabled: true } + ] + + put admin_api_service_proxy_policies_path(@service, access_token: @access_token, format: :json), params: { + proxy: { policies_config: config } + }, as: :json + + assert_response :success + assert_equal config.map(&:stringify_keys), @service.reload.proxy.policies_config.as_json + end + test 'prints chain level errors' do valid_config = { name: 'apicast', configuration: {}, version: 'builtin', enabled: true, removable: false } From b94b2de079688b1df66eb97259d04297edb1382b Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Mon, 18 May 2026 17:04:22 +0200 Subject: [PATCH 26/29] Fix settings test for Oracle --- test/integration/sites/settings_controller_test.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/integration/sites/settings_controller_test.rb b/test/integration/sites/settings_controller_test.rb index 8af078c879..4580d6a525 100644 --- a/test/integration/sites/settings_controller_test.rb +++ b/test/integration/sites/settings_controller_test.rb @@ -40,8 +40,9 @@ class Sites::SettingsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to edit_admin_site_settings_path provider.settings.reload - assert_equal '', provider.settings.cc_terms_path - assert_equal '', provider.settings.cc_privacy_path + # Oracle converts empty strings to NULL, so we check for blank (accepts both '' and nil) + assert provider.settings.cc_terms_path.blank? + assert provider.settings.cc_privacy_path.blank? end test 'update with invalid values shows errors' do From d9685fb3bd7eb743222fd22c1ba2246c0639cffc Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Tue, 19 May 2026 12:13:13 +0200 Subject: [PATCH 27/29] Wait for the template to get saved --- features/provider/cms/template_versions.feature | 1 + 1 file changed, 1 insertion(+) diff --git a/features/provider/cms/template_versions.feature b/features/provider/cms/template_versions.feature index fa7be8dfce..2b7578d355 100644 --- a/features/provider/cms/template_versions.feature +++ b/features/provider/cms/template_versions.feature @@ -37,6 +37,7 @@ Feature: CMS Template versions Scenario: Revert to version from the version itself Given they fill the template draft with "
Dammit, I Changed Again
" And press "Save" + And they should see a toast alert with text "Template saved" When they follow "Versions" And follow "24 Dec 2012 12:00:00 UTC" And the version template should contain "Original Prankster" From c7af98d476895cae280117c41a58d6ce46498a90 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Tue, 19 May 2026 20:39:49 +0200 Subject: [PATCH 28/29] Apply suggestions from peer review --- app/controllers/admin/api/sso_tokens_controller.rb | 2 +- app/controllers/provider/admin/cms/groups_controller.rb | 7 ++----- .../provider/admin/securities_controller_test.rb | 1 + 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/api/sso_tokens_controller.rb b/app/controllers/admin/api/sso_tokens_controller.rb index f74ec70de7..8cb8676e55 100644 --- a/app/controllers/admin/api/sso_tokens_controller.rb +++ b/app/controllers/admin/api/sso_tokens_controller.rb @@ -2,7 +2,7 @@ class Admin::Api::SSOTokensController < Admin::Api::BaseController - wrap_parameters :sso_token, :include => %i[user_id username expires_in redirect_url protocol], :format => [:url_encoded_form] + wrap_parameters :sso_token, include: %i[user_id username expires_in redirect_url protocol], format: [:url_encoded_form] # parameters: # * user_id diff --git a/app/controllers/provider/admin/cms/groups_controller.rb b/app/controllers/provider/admin/cms/groups_controller.rb index dce706e7d7..c89af41778 100644 --- a/app/controllers/provider/admin/cms/groups_controller.rb +++ b/app/controllers/provider/admin/cms/groups_controller.rb @@ -77,12 +77,9 @@ def authorize_groups # Ensure that all section IDs provided in the parameters belong to the account, which is the group owner # Providing a non-existent section ID, or the one that belongs to another provider results in a Not Found error def validate_section_ids - section_ids = group_params[:section_ids] + section_ids = group_params[:section_ids]&.reject(&:empty?)&.uniq return if section_ids.blank? - section_ids.reject!(&:empty?) - exising_section_ids = current_account.section_ids.map(&:to_s) - - render_error(:not_found, status: :not_found) if (section_ids.map(&:to_s) - exising_section_ids).any? + render_error(:not_found, status: :not_found) unless current_account.sections.where(id: section_ids).count == section_ids.size end end diff --git a/test/integration/provider/admin/securities_controller_test.rb b/test/integration/provider/admin/securities_controller_test.rb index cf6fef25d7..efa15c57a7 100644 --- a/test/integration/provider/admin/securities_controller_test.rb +++ b/test/integration/provider/admin/securities_controller_test.rb @@ -234,6 +234,7 @@ class Provider::Admin::SecuritiesControllerTest < ActionDispatch::IntegrationTes assert_redirected_to provider_login_path end + # TODO: review if this behavior is actually desired, and if not, restrict this page for member users test 'member can update the settings' do member = FactoryBot.create(:member, account: @provider) member.activate! From 31ac83d6ee5e9ee1a61f1be8976381ba4d38df64 Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Wed, 20 May 2026 16:42:37 +0200 Subject: [PATCH 29/29] Return 400 when fields definitions is invalid --- .../admin/fields_definitions_controller.rb | 51 +++++++++++-------- .../fields_definitions_controller_test.rb | 22 ++++---- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/app/controllers/admin/fields_definitions_controller.rb b/app/controllers/admin/fields_definitions_controller.rb index cd39d203b2..526477f9dd 100644 --- a/app/controllers/admin/fields_definitions_controller.rb +++ b/app/controllers/admin/fields_definitions_controller.rb @@ -4,17 +4,19 @@ class Admin::FieldsDefinitionsController < Sites::BaseController respond_to :html activate_menu :audience, :accounts, :fields_definitions + before_action :set_fields_definition_params, only: %i[new create update] + before_action :validate_target, only: [:new] + def index @possible_targets = available_targets - respond_with(field_definitions) + respond_with(fields_definitions) end def new - target = field_definition_params[:target] - target = available_targets.first unless available_targets.include?(target) + target = fields_definition_params[:target] - @fields_definition = field_definitions.build(target: target) + @fields_definition = fields_definitions.build(target: target) target_class = @fields_definition.target_class @optional_fields = target_class.builtin_fields - existing_fields_names_by_target(target) @@ -27,14 +29,14 @@ def new end def edit - @optional_fields = field_definition.target_class.builtin_fields - @required_fields = field_definition.target_class.required_fields + @optional_fields = fields_definition.target_class.builtin_fields + @required_fields = fields_definition.target_class.required_fields - respond_with(field_definition) + respond_with(fields_definition) end def create - @fields_definition = field_definitions.build(field_definition_params) + @fields_definition = fields_definitions.build(fields_definition_params) if @fields_definition.save flash[:success] = t('.success') @@ -51,16 +53,14 @@ def create def update @required_fields = [] - if field_definition.update(field_definition_params) - @required_fields = field_definition.target_class.required_fields - end + @required_fields = fields_definition.target_class.required_fields if fields_definition.update(fields_definition_params) - respond_with(field_definition, location: admin_fields_definitions_path) + respond_with(fields_definition, location: admin_fields_definitions_path) end def destroy - field_definition.destroy - respond_with(field_definition, location: admin_fields_definitions_path) + fields_definition.destroy + respond_with(fields_definition, location: admin_fields_definitions_path) end def sort @@ -75,20 +75,18 @@ def sort private - def field_definition_params - params.fetch(:fields_definition, {}).permit(:target, :name, :label, :required, :hidden, :read_only, :choices_for_views) - end + attr_reader :fields_definition_params def sort_params @sort_params ||= params.permit(fields_definition: [])[:fields_definition] || [] end - def field_definitions + def fields_definitions @fields_definitions ||= current_account.fields_definitions end - def field_definition - @fields_definition ||= field_definitions.find(params[:id]) + def fields_definition + @fields_definition ||= fields_definitions.find(params[:id]) end def available_targets @@ -98,4 +96,17 @@ def available_targets def existing_fields_names_by_target(target) current_account.fields_definitions.by_target(target).map(&:name) end + + def set_fields_definition_params + fd_params = params.fetch(:fields_definition) + if fd_params.respond_to? :permit + @fields_definition_params = fd_params.permit(:target, :name, :label, :required, :hidden, :read_only, :choices_for_views) + else + render_error "invalid fields definition", status: :bad_request unless fd_params.is_a?(Hash) + end + end + + def validate_target + render_error "invalid fields definition target", status: :bad_request unless available_targets.include?(fields_definition_params[:target]) + end end diff --git a/test/functional/admin/fields_definitions_controller_test.rb b/test/functional/admin/fields_definitions_controller_test.rb index c3a85c55a2..a8a614ef72 100644 --- a/test/functional/admin/fields_definitions_controller_test.rb +++ b/test/functional/admin/fields_definitions_controller_test.rb @@ -20,19 +20,23 @@ def field_definition end test 'new' do - default_target = FieldsDefinition.targets.first - get :new + assert_response :bad_request + assert_equal 'Required parameter missing: fields_definition', @response.body - assert_response :success - assert_select('h1', text: "New field definition for #{default_target}") - assert_select 'input#fields_definition_target', type: 'hidden', value: 'Account' + get :new, params: { fields_definition: 'invalid' } + assert_response :bad_request + assert_equal 'invalid fields definition', @response.body + + get :new, params: { fields_definition: { target: 'non-existent' } } + assert_response :bad_request + assert_equal 'invalid fields definition target', @response.body get :new, params: { fields_definition: { target: 'User' } } assert_response :success - assert_select('h1', text: "New field definition for User") - assert_select 'input#fields_definition_target', type: 'hidden', value: 'Account' + assert_select 'h1', text: "New field definition for User" + assert_select 'input#fields_definition_target[value=?][type=?]', 'User', 'hidden' end test 'edit' do @@ -63,9 +67,9 @@ def field_definition test 'update fails' do FieldsDefinition.any_instance.stubs(:save).returns(false) put :update, params: { id: field_definition } - assert assigns(:required_fields) - end + assert_response :bad_request + end test 'destroy' do delete :destroy, params: { id: field_definition }