From 2513d3b1ebbb418d1f067d3232806a9a5f7e0f70 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Fri, 23 Aug 2024 16:02:58 +0200 Subject: [PATCH 1/2] Remove remaining warnings on unpermitted params --- app/assets/javascripts/labels_index.coffee | 2 +- app/controllers/concerns/task_parameters.rb | 2 +- app/controllers/labels_controller.rb | 6 +++++- app/controllers/messages_controller.rb | 9 ++------- .../users/registrations_controller.rb | 3 ++- app/views/messages/_form.html.slim | 2 +- .../account_links_controller_spec.rb | 8 ++++---- spec/controllers/collections_controller_spec.rb | 15 ++++++++++----- spec/controllers/groups_controller_spec.rb | 2 +- .../task_contributions_controller_spec.rb | 2 +- spec/controllers/tasks_controller_spec.rb | 17 ++++++++++------- spec/factories/collection.rb | 2 ++ spec/requests/collections_spec.rb | 2 +- spec/requests/groups_spec.rb | 2 +- spec/requests/tasks_spec.rb | 8 ++++---- spec/requests/users_spec.rb | 2 +- .../shared_examples/standard_examples.rb | 10 +++++----- 17 files changed, 52 insertions(+), 42 deletions(-) diff --git a/app/assets/javascripts/labels_index.coffee b/app/assets/javascripts/labels_index.coffee index 5cf47fbad..dc56c7a9f 100644 --- a/app/assets/javascripts/labels_index.coffee +++ b/app/assets/javascripts/labels_index.coffee @@ -174,7 +174,7 @@ class LabelsTable $.ajax({ url: "/labels/#{id}", type: "PATCH", - data: {color: new_color} + data: {label: {color: new_color}} }) ); $.when.apply($, requests).then(@reload_table); diff --git a/app/controllers/concerns/task_parameters.rb b/app/controllers/concerns/task_parameters.rb index 3edcb1e43..a9206aa38 100644 --- a/app/controllers/concerns/task_parameters.rb +++ b/app/controllers/concerns/task_parameters.rb @@ -24,7 +24,7 @@ def task_params end def group_tasks_params - params.permit(group_tasks: {group_ids: []})[:group_tasks] + params.require(:group_tasks).permit(group_ids: []) end def import_confirm_params diff --git a/app/controllers/labels_controller.rb b/app/controllers/labels_controller.rb index 185e5aaab..7f12e77d2 100644 --- a/app/controllers/labels_controller.rb +++ b/app/controllers/labels_controller.rb @@ -7,7 +7,7 @@ class LabelsController < ApplicationController def index; end def update - if @label.update(params.permit(:color)) + if @label.update(label_params) render json: @label.to_h else flash.now[:alert] = @label.errors.full_messages @@ -49,6 +49,10 @@ def search # rubocop:disable Metrics/AbcSize private + def label_params + params.require(:label).permit(:color) + end + def load_and_authorize_label @label = Label.find(params[:id]) authorize @label diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb index 34cb76155..5489e78f0 100644 --- a/app/controllers/messages_controller.rb +++ b/app/controllers/messages_controller.rb @@ -25,15 +25,11 @@ def new authorize @message end - # rubocop:disable Metrics/AbcSize def create + recipient = params.require(:message).delete(:recipient) @message = Message.new(message_params) @message.sender = current_user - @message.recipient = if params[:message][:recipient] - User.find_by(email: params[:message][:recipient]) - else - User.find(params[:message][:recipient_hidden]) - end + @message.recipient = User.find_by(email: recipient) if recipient.present? authorize @message if @message.save @@ -42,7 +38,6 @@ def create render :new end end - # rubocop:enable Metrics/AbcSize def destroy @message.mark_as_deleted(current_user) diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index e44053eaf..ea6a979d4 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -24,8 +24,9 @@ class RegistrationsController < Devise::RegistrationsController # PUT /resource def update + avatar_present = params.require(:user).delete(:avatar_present) super do |resource| - resource.avatar.purge if params[:user][:avatar].nil? && params[:user][:avatar_present] == 'false' + resource.avatar.purge if params[:user][:avatar].nil? && avatar_present == 'false' if resource.password_set_changed? # If a user tried to set a password but failed, we need to reset the password_set flag. # Further, we need to remove the current_password error, since the user didn't enter their current password. diff --git a/app/views/messages/_form.html.slim b/app/views/messages/_form.html.slim index 39084772d..065eef2f3 100644 --- a/app/views/messages/_form.html.slim +++ b/app/views/messages/_form.html.slim @@ -3,7 +3,7 @@ = render('shared/form_errors', object: @message) - if @recipient - = f.hidden_field :recipient_hidden, value: @recipient.id + = f.hidden_field :recipient_id, value: @recipient.id - else .field-element = f.label :recipient, Message.human_attribute_name('recipient'), class: 'form-label' diff --git a/spec/controllers/account_links_controller_spec.rb b/spec/controllers/account_links_controller_spec.rb index a967ec13b..e532d47c6 100644 --- a/spec/controllers/account_links_controller_spec.rb +++ b/spec/controllers/account_links_controller_spec.rb @@ -11,7 +11,7 @@ let(:account_link) { create(:account_link, user:) } let(:account_link_from_another_user) { create(:account_link, user: another_user) } - let(:valid_attributes) { attributes_for(:account_link).merge(user:) } + let(:valid_attributes) { attributes_for(:account_link) } let(:invalid_attributes) do {api_key: ''} end @@ -65,14 +65,14 @@ context 'with valid attributes' do it 'updates the requested account_link' do - account_link = AccountLink.create! valid_attributes + account_link = AccountLink.create! valid_attributes.merge(user:) put :update, params: empty_params.merge(id: account_link.to_param, account_link: new_attributes) account_link.reload expect(account_link.api_key).to eq(new_attributes[:api_key]) end it 'redirects to the user' do - account_link = AccountLink.create! valid_attributes + account_link = AccountLink.create! valid_attributes.merge(user:) put :update, params: empty_params.merge(id: account_link.to_param, account_link: valid_attributes) expect(response).to redirect_to(user) end @@ -82,7 +82,7 @@ describe 'DELETE #destroy' do include_examples 'destroy examples', klass: AccountLink, resource: :account_link it 'with valid attributes redirects to the user' do - account_link = AccountLink.create! valid_attributes + account_link = AccountLink.create! valid_attributes.merge(user:) delete :destroy, params: empty_params.merge(id: account_link.to_param) expect(response).to redirect_to(user) end diff --git a/spec/controllers/collections_controller_spec.rb b/spec/controllers/collections_controller_spec.rb index e575e46c3..232bbdace 100644 --- a/spec/controllers/collections_controller_spec.rb +++ b/spec/controllers/collections_controller_spec.rb @@ -6,7 +6,7 @@ render_views let(:valid_attributes) do - attributes_for(:collection) + build(:collection).attributes.except('id', 'created_at', 'updated_at') end let(:invalid_attributes) do @@ -274,6 +274,11 @@ context 'with valid params' do let(:collection_params) { valid_attributes } + let(:collection_tasks_attributes) do + collection.collection_tasks.map do |collection_task| + collection_task.attributes.symbolize_keys.except(:collection_id, :created_at, :task_id, :updated_at) + end + end it 'assigns the requested collection as @collection' do put_update @@ -296,8 +301,8 @@ context 'with new task order' do let(:collection_params) do {collection_tasks_attributes: { - '0': collection.collection_tasks.first.attributes.merge(rank: 0), - '1': collection.collection_tasks.second.attributes.merge(rank: 1), + '0': collection_tasks_attributes.first.merge(rank: 0), + '1': collection_tasks_attributes.second.merge(rank: 1), }} end @@ -309,8 +314,8 @@ context 'when removing a task' do let(:collection_params) do {collection_tasks_attributes: { - '0': collection.collection_tasks.first.attributes.merge(rank: 0), - '1': collection.collection_tasks.second.attributes.merge(_destroy: 1), + '0': collection_tasks_attributes.first.merge(rank: 0), + '1': collection_tasks_attributes.second.merge(_destroy: 1), }} end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index c90ab123d..2a6125c0b 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -13,7 +13,7 @@ let(:admin_locale) { :de } let(:valid_post_attributes) do - attributes_for(:group, users: [user]) + build(:group, users: [user]).attributes.except('id', 'created_at', 'updated_at') end let(:invalid_attributes) do diff --git a/spec/controllers/task_contributions_controller_spec.rb b/spec/controllers/task_contributions_controller_spec.rb index 299d6ce48..d42bcca39 100644 --- a/spec/controllers/task_contributions_controller_spec.rb +++ b/spec/controllers/task_contributions_controller_spec.rb @@ -49,7 +49,7 @@ describe 'POST #create' do subject(:post_request) { post :create, params: {task_id: task.id, task: task_params} } - let(:task_params) { attributes_for(:task) } + let(:task_params) { attributes_for(:task).except(:uuid) } let(:suggestion) { assigns(:task_contribution).suggestion } context 'with valid params' do diff --git a/spec/controllers/tasks_controller_spec.rb b/spec/controllers/tasks_controller_spec.rb index b1f40f065..c0fd30a4d 100644 --- a/spec/controllers/tasks_controller_spec.rb +++ b/spec/controllers/tasks_controller_spec.rb @@ -260,7 +260,7 @@ let(:valid_params) do { title: 'title', - descriptions_attributes: {'0' => {text: 'description', primary: true}}, + description: 'description', programming_language_id: create(:programming_language, :ruby).id, license_id: create(:license), language: 'de', @@ -387,13 +387,12 @@ let(:existing_label) { create(:label) } let(:new_label) { create(:label) } - let!(:task) { create(:task, valid_attributes) } + let!(:task) { create(:task, valid_attributes.merge(user:)) } let(:valid_attributes) do { - user:, title: 'title', - license: create(:license, name: 'old_license'), - labels: [existing_label], + license_id: create(:license, name: 'old_license').id, + label_names: [existing_label.name], } end let(:changed_attributes) do @@ -471,9 +470,13 @@ let(:test) { build(:test) } let(:new_testing_framework) { create(:testing_framework) } - let!(:task) { create(:task, valid_attributes.merge(tests: [test])) } + let!(:task) { create(:task, valid_attributes.merge(tests: [test], user:)) } - let(:tests_attributes) { {'0': test.attributes.symbolize_keys.merge(title: 'new_test_title', testing_framework_id: new_testing_framework.id)} } + let(:tests_attributes) do + {'0': test.attributes.symbolize_keys + .except(:configuration, :created_at, :meta_data, :task_id, :updated_at) + .merge(title: 'new_test_title', testing_framework_id: new_testing_framework.id)} + end it 'updates the requested task' do expect { put_update }.to change { task.reload.title }.to('new_title') diff --git a/spec/factories/collection.rb b/spec/factories/collection.rb index 73bb6e416..bbdd20cea 100644 --- a/spec/factories/collection.rb +++ b/spec/factories/collection.rb @@ -3,6 +3,8 @@ FactoryBot.define do factory :collection do title { 'Some Collection' } + description { 'Some Description' } + visibility_level { :private } users { build_list(:user, 1) } tasks { build_list(:task, 2) } end diff --git a/spec/requests/collections_spec.rb b/spec/requests/collections_spec.rb index 584bbcec4..5dddc42db 100644 --- a/spec/requests/collections_spec.rb +++ b/spec/requests/collections_spec.rb @@ -7,7 +7,7 @@ let(:user) { create(:user) } let(:task) { create(:task) } let(:collection) { create(:collection, title: 'Some Collection', users: [user], tasks: [task]) } - let(:collection_params) { attributes_for(:collection) } + let(:collection_params) { build(:collection).attributes.except('id', 'created_at', 'updated_at') } before do sign_in user diff --git a/spec/requests/groups_spec.rb b/spec/requests/groups_spec.rb index 56ff0a9f7..998dc7c63 100644 --- a/spec/requests/groups_spec.rb +++ b/spec/requests/groups_spec.rb @@ -6,7 +6,7 @@ context 'when logged in' do let(:user) { create(:user) } let(:group) { create(:group) } - let(:group_params) { attributes_for(:group) } + let(:group_params) { build(:group).attributes.except('id', 'created_at', 'updated_at') } before do group.add(user, role: :admin) diff --git a/spec/requests/tasks_spec.rb b/spec/requests/tasks_spec.rb index e48d2ae5e..ae176b50c 100644 --- a/spec/requests/tasks_spec.rb +++ b/spec/requests/tasks_spec.rb @@ -9,8 +9,8 @@ let(:valid_params) do { title: 'title', - descriptions_attributes: {'0' => {text: 'description', primary: true}}, - programming_language: create(:programming_language, :ruby).id, + description: 'description', + programming_language_id: create(:programming_language, :ruby).id, license_id: create(:license).id, language: 'de', } @@ -19,8 +19,8 @@ let(:update_params) do { title: 'new_title', - descriptions_attributes: {'0' => {text: 'description'}}, - programming_language: create(:programming_language, :ruby).id, + description: 'description', + programming_language_id: create(:programming_language, :ruby).id, license_id: create(:license).id, } end diff --git a/spec/requests/users_spec.rb b/spec/requests/users_spec.rb index 99017d697..af994f5ee 100644 --- a/spec/requests/users_spec.rb +++ b/spec/requests/users_spec.rb @@ -15,7 +15,7 @@ end context 'when logged in' do - let(:user_params) { attributes_for(:user, current_password: user.password) } + let(:user_params) { attributes_for(:user, current_password: user.password).except(:confirmed_at) } before do sign_in user diff --git a/spec/support/shared_examples/standard_examples.rb b/spec/support/shared_examples/standard_examples.rb index 940bbd474..b70206761 100644 --- a/spec/support/shared_examples/standard_examples.rb +++ b/spec/support/shared_examples/standard_examples.rb @@ -20,7 +20,7 @@ RSpec.shared_examples 'edit examples' do |klass:, resource:| it "assigns the requested #{resource} as @#{resource}" do - object = klass.create! valid_attributes + object = klass.create! valid_attributes.merge(user:) get :edit, params: empty_params.merge(id: object.to_param) expect(assigns(resource)).to eq(object) end @@ -70,7 +70,7 @@ RSpec.shared_examples 'update examples' do |klass:, resource:| context 'with valid params' do it "assigns the requested #{resource} as @#{resource}" do - object = klass.create! valid_attributes + object = klass.create! valid_attributes.merge(user:) put :update, params: empty_params.merge(:id => object.to_param, resource => valid_attributes) expect(assigns(resource)).to eq(object) end @@ -78,13 +78,13 @@ context 'with invalid params' do it "assigns the #{resource} as @#{resource}" do - object = klass.create! valid_attributes + object = klass.create! valid_attributes.merge(user:) put :update, params: empty_params.merge(:id => object.to_param, resource => invalid_attributes) expect(assigns(resource)).to eq(object) end it "re-renders the 'edit' template" do - object = klass.create! valid_attributes + object = klass.create! valid_attributes.merge(user:) put :update, params: empty_params.merge(:id => object.to_param, resource => invalid_attributes) expect(response).to render_template('edit') end @@ -93,7 +93,7 @@ RSpec.shared_examples 'destroy examples' do |klass:, resource:| it "destroys the requested #{resource}" do - object = klass.create! valid_attributes + object = klass.create! valid_attributes.merge(user:) expect do delete :destroy, params: empty_params.merge(id: object.to_param) end.to change(klass, :count).by(-1) From 0fe2eefc3ba26ce11950d86333dac99b58a8c20c Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Sun, 25 Aug 2024 12:22:58 +0200 Subject: [PATCH 2/2] Raise on unpermitted parameters in dev and test env This setting should ensure we don't re-introduce any code that silently "accepts" unpermitted parameters. --- config/environments/development.rb | 3 +++ config/environments/test.rb | 3 +++ 2 files changed, 6 insertions(+) diff --git a/config/environments/development.rb b/config/environments/development.rb index 63e9b3387..281b8265a 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -91,6 +91,9 @@ # Raises error for missing translations. config.i18n.raise_on_missing_translations = true + # Raises error for unpermitted parameters (default is `:log`). + config.action_controller.action_on_unpermitted_parameters = :raise + # Annotate rendered view with file names. config.action_view.annotate_rendered_view_with_filenames = true diff --git a/config/environments/test.rb b/config/environments/test.rb index 688ff0752..1b9f17aa0 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -71,6 +71,9 @@ # Raises error for missing translations. config.i18n.raise_on_missing_translations = true + # Raises error for unpermitted parameters (default is `:log`). + config.action_controller.action_on_unpermitted_parameters = :raise + # Annotate rendered view with file names. config.action_view.annotate_rendered_view_with_filenames = true