From 31e801ffa4fce48c3990728a1ebcf2513a921c3a Mon Sep 17 00:00:00 2001 From: ulferts Date: Mon, 20 Jun 2022 16:59:02 +0200 Subject: [PATCH 1/4] replace structured mentions of user upon deletion --- .../users/replace_mentions_service.rb | 162 ++++++++ app/workers/principals/delete_job.rb | 17 +- ...place_mentions_service_integration_spec.rb | 378 ++++++++++++++++++ .../principals/delete_job_integration_spec.rb | 42 +- 4 files changed, 591 insertions(+), 8 deletions(-) create mode 100644 app/services/users/replace_mentions_service.rb create mode 100644 spec/services/users/replace_mentions_service_integration_spec.rb diff --git a/app/services/users/replace_mentions_service.rb b/app/services/users/replace_mentions_service.rb new file mode 100644 index 0000000000..63a8cff32b --- /dev/null +++ b/app/services/users/replace_mentions_service.rb @@ -0,0 +1,162 @@ +# OpenProject is an open source project management software. +# Copyright (C) 2010-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. + +# Rewrites mentions in user provided text (e.g. work package journals) from one user to another. +# No data is to be removed. +module Users + class ReplaceMentionsService + include ActiveRecord::Sanitization + + def self.replacements + [ + { class: WorkPackage, column: :description }, + { class: Journal::WorkPackageJournal, column: :description }, + { class: Document, column: :description }, + { class: Journal, column: :notes }, + { class: Comment, column: :comments }, + { class: CustomValue, column: :value }, + { class: Journal::CustomizableJournal, column: :value }, + { class: MeetingContent, column: :text }, + { class: Journal::MeetingContentJournal, column: :text }, + { class: Message, column: :content }, + { class: Journal::MessageJournal, column: :content }, + { class: News, column: :description }, + { class: Journal::NewsJournal, column: :description }, + { class: Project, column: :description }, + { class: Projects::Status, column: :explanation }, + { class: WikiContent, column: :text }, + { class: Journal::WikiContentJournal, column: :text } + ] + end + + def call(from:, to:) + check_input(from, to) + + rewrite(from, to) + + ServiceResult.success + end + + private + + def check_input(from, to) + raise ArgumentError unless from.is_a?(User) && to.is_a?(User) + end + + def rewrite(from, to) + self.class.replacements.each do |replacement| + rewrite_column(replacement[:class], replacement[:column], from, to) + end + end + + def rewrite_column(klass, column, from, to) + sql = <<~SQL.squish + UPDATE #{klass.table_name} sink + SET #{column} = source.replacement + FROM + (SELECT + #{klass.table_name}.id, + #{replace_sql(klass, "#{klass.table_name}.#{column}", from, to)} replacement + FROM + #{klass.table_name} + WHERE + #{condition_sql(klass, column, from)} + ) source + WHERE + source.id = sink.id + SQL + + klass + .connection + .execute sql + end + + def replace_sql(klass, source, from, to) + hash_replace(klass, + mention_tag_replace(klass, + source, + from, + to), + from, + to) + end + + def condition_sql(klass, column, from) + sql = <<~SQL.squish + #{klass.table_name}.#{column} SIMILAR TO '_*(()|(user#((%i)|("%s")|("%s")\s)))_*' + SQL + + klass.sanitize_sql_for_conditions [sql, + from.id, + from.id, + klass.sanitize_sql_like(from.mail), + klass.sanitize_sql_like(from.login)] + end + + def mention_tag_replace(klass, source, from, to) + regexp_replace( + klass, + source, + '', + '@%s', + [from.id, + to.id, + klass.sanitize_sql_like(to.name), + klass.sanitize_sql_like(to.name)] + ) + end + + def hash_replace(klass, source, from, to) + regexp_replace( + klass, + source, + 'user#((%i)|("%s")|("%s")\s)', + 'user#%i ', + [from.id, + klass.sanitize_sql_like(from.login), + klass.sanitize_sql_like(from.mail), + to.id] + ) + end + + def regexp_replace(klass, source, search, replacement, values) + sql = <<~SQL.squish + REGEXP_REPLACE( + #{source}, + '#{search}', + '#{replacement}', + 'g' + ) + SQL + + klass.sanitize_sql_for_conditions [sql].concat(values) + end + + def journal_classes + [Journal] + Journal::BaseJournal.subclasses + end + end +end diff --git a/app/workers/principals/delete_job.rb b/app/workers/principals/delete_job.rb index b32c2befe7..10aba98f79 100644 --- a/app/workers/principals/delete_job.rb +++ b/app/workers/principals/delete_job.rb @@ -33,6 +33,7 @@ class Principals::DeleteJob < ApplicationJob Principal.transaction do delete_associated(principal) replace_references(principal) + replace_mentions(principal) update_cost_queries(principal) remove_members(principal) @@ -46,9 +47,19 @@ class Principals::DeleteJob < ApplicationJob Principals::ReplaceReferencesService .new .call(from: principal, to: DeletedUser.first) - .tap do |call| - raise ActiveRecord::Rollback if call.failure? - end + .on_failure { raise ActiveRecord::Rollback } + end + + def replace_mentions(principal) + # Breaking abstraction here. + # Doing the replacement is a very costly operation while at the same time, + # groups and placeholder users can't be mentioned. + return unless principal.is_a?(User) + + Users::ReplaceMentionsService + .new + .call(from: principal, to: DeletedUser.first) + .on_failure { raise ActiveRecord::Rollback } end def delete_associated(principal) diff --git a/spec/services/users/replace_mentions_service_integration_spec.rb b/spec/services/users/replace_mentions_service_integration_spec.rb new file mode 100644 index 0000000000..1da485fd4a --- /dev/null +++ b/spec/services/users/replace_mentions_service_integration_spec.rb @@ -0,0 +1,378 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require 'spec_helper' + +describe Users::ReplaceMentionsService, 'integration' do + subject(:service_call) { instance.call(from: user, to: to_user) } + + shared_let(:other_user) { create(:user, firstname: 'Frank', lastname: 'Herbert') } + shared_let(:user) { create(:user, firstname: 'Isaac', lastname: 'Asimov') } + shared_let(:to_user) { create :user, firstname: 'Philip K.', lastname: 'Dick' } + + let(:instance) do + described_class.new + end + + it 'is successful' do + expect(service_call) + .to be_success + end + + shared_examples_for 'text replacement' do |attribute| + before do + service_call + model.reload + end + + it "replaces #{attribute}" do + expect(model.send(attribute)) + .to be_html_eql expected_text + end + end + + shared_examples_for 'rewritten mention' do |factory, attribute| + let(:additional_properties) { {} } + let!(:model) do + create(factory, attribute => text, **additional_properties) + end + + context 'with the replaced user in mention tags' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + + @#{user.name} + + TEXT + end + let(:expected_text) do + <<~TEXT.squish + @#{to_user.name} + TEXT + end + end + end + + context 'with a different user in mention tags' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + + @#{other_user.name} + + TEXT + end + let(:expected_text) do + <<~TEXT.squish + @#{other_user.name} + TEXT + end + end + end + + context 'with the replaced user in a user#ID notation' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + Lorem ipsum + + user##{user.id} Lorem ipsum + + Lorem ipsum + TEXT + end + let(:expected_text) do + <<~TEXT + Lorem ipsum + + user##{to_user.id} Lorem ipsum + + Lorem ipsum + TEXT + end + end + end + + context 'with a different user in a user#ID notation' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + Lorem ipsum + + user##{other_user.id} Lorem ipsum + + Lorem ipsum + TEXT + end + let(:expected_text) do + <<~TEXT + Lorem ipsum + + user##{other_user.id} Lorem ipsum + + Lorem ipsum + TEXT + end + end + end + + context 'with the replaced user in a user#"LOGIN" notation' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + Lorem ipsum + + user#"#{user.login}" Lorem ipsum + + Lorem ipsum + TEXT + end + let(:expected_text) do + <<~TEXT + Lorem ipsum + + user##{to_user.id} Lorem ipsum + + Lorem ipsum + TEXT + end + end + end + + context 'with a different user in a user#"LOGIN" notation' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + Lorem ipsum + + user#"#{other_user.login}" Lorem ipsum + + Lorem ipsum + TEXT + end + let(:expected_text) do + <<~TEXT + Lorem ipsum + + user#"#{other_user.login}" Lorem ipsum + + Lorem ipsum + TEXT + end + end + end + + context 'with the replaced user in a user#"MAIL" notation' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + Lorem ipsum + + user#"#{user.mail}" Lorem ipsum + + Lorem ipsum + TEXT + end + let(:expected_text) do + <<~TEXT + Lorem ipsum + + user##{to_user.id} Lorem ipsum + + Lorem ipsum + TEXT + end + end + end + + context 'with a different user in a user#"MAIL" notation' do + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + Lorem ipsum + + user#"#{other_user.mail}" Lorem ipsum + + Lorem ipsum + TEXT + end + let(:expected_text) do + <<~TEXT + Lorem ipsum + + user#"#{other_user.mail}" Lorem ipsum + + Lorem ipsum + TEXT + end + end + end + end + + context 'for work package description' do + it_behaves_like 'rewritten mention', :work_package, :description + end + + context 'for work package description with dangerous mails' do + subject(:service_call) { instance.call(from: dangerous_user, to: to_user) } + + let(:dangerous_user) do + build(:user, + firstname: 'Dangerous', + lastname: 'User', + mail: "'); DELETE FROM work_packages; SELECT ('").tap do |user| + user.save(validate: false) + end + end + let(:user) { dangerous_user } + + it 'escapes the malicious input' do + expect { service_call } + .not_to raise_error + end + end + + context 'for work package journal description' do + it_behaves_like 'rewritten mention', :journal_work_package_journal, :description + end + + context 'for journal notes' do + it_behaves_like 'rewritten mention', :journal, :notes + end + + context 'for comment comments' do + it_behaves_like 'rewritten mention', :comment, :comments + end + + context 'for custom_value value' do + it_behaves_like 'rewritten mention', :custom_value, :value do + let(:additional_properties) { { custom_field: create(:string_wp_custom_field) } } + end + end + + context 'for customizable_journal value' do + it_behaves_like 'rewritten mention', :journal_customizable_journal, :value do + let(:additional_properties) { { journal: create(:journal), custom_field: create(:string_wp_custom_field) } } + end + end + + context 'for documents description' do + it_behaves_like 'rewritten mention', :document, :description + end + + context 'for meeting_contents text' do + it_behaves_like 'rewritten mention', :meeting_agenda, :text + end + + context 'for meeting_content_journals text' do + it_behaves_like 'rewritten mention', :journal_meeting_content_journal, :text + end + + context 'for messages content' do + it_behaves_like 'rewritten mention', :message, :content + end + + context 'for message_journals content' do + it_behaves_like 'rewritten mention', :journal_message_journal, :content do + let(:additional_properties) { { forum_id: 1 } } + end + end + + context 'for news description' do + it_behaves_like 'rewritten mention', :news, :description + end + + context 'for news_journals description' do + it_behaves_like 'rewritten mention', :journal_news_journal, :description + end + + context 'for project description' do + it_behaves_like 'rewritten mention', :project, :description + end + + context 'for project_status explanation' do + it_behaves_like 'rewritten mention', :project_status, :explanation + end + + context 'for wiki_content text' do + it_behaves_like 'rewritten mention', :wiki_content, :text + end + + context 'for wiki_content_journals text' do + it_behaves_like 'rewritten mention', :journal_wiki_content_journal, :text + end + + context 'for a group for from' do + subject(:service_call) { instance.call(from: create(:group), to: to_user) } + + it 'raises an error' do + expect { service_call } + .to raise_error ArgumentError + end + end + + context 'for a group for to' do + subject(:service_call) { instance.call(from: user, to: create(:group)) } + + it 'raises an error' do + expect { service_call } + .to raise_error ArgumentError + end + end + + context 'for a placeholder user for from' do + subject(:service_call) { instance.call(from: create(:placeholder_user), to: to_user) } + + it 'raises an error' do + expect { service_call } + .to raise_error ArgumentError + end + end + + context 'for a placeholder user for to' do + subject(:service_call) { instance.call(from: user, to: create(:placeholder_user)) } + + it 'raises an error' do + expect { service_call } + .to raise_error ArgumentError + end + end +end diff --git a/spec/workers/principals/delete_job_integration_spec.rb b/spec/workers/principals/delete_job_integration_spec.rb index d3533ff0d7..8eb73d4bac 100644 --- a/spec/workers/principals/delete_job_integration_spec.rb +++ b/spec/workers/principals/delete_job_integration_spec.rb @@ -96,7 +96,7 @@ describe Principals::DeleteJob, type: :model do job end - it { expect(LaborBudgetItem.find_by_id(item.id)).to eq(item) } + it { expect(LaborBudgetItem.find_by(id: item.id)).to eq(item) } it { expect(item.user_id).to eq(principal.id) } end @@ -107,7 +107,7 @@ describe Principals::DeleteJob, type: :model do user: principal, project: work_package.project, units: 100.0, - spent_on: Date.today, + spent_on: Time.zone.today, work_package:, comments: '') end @@ -159,7 +159,7 @@ describe Principals::DeleteJob, type: :model do job end - it { expect(HourlyRate.find_by_id(hourly_rate.id)).to eq(hourly_rate) } + it { expect(HourlyRate.find_by(id: hourly_rate.id)).to eq(hourly_rate) } it { expect(hourly_rate.reload.user_id).to eq(principal.id) } end @@ -257,7 +257,7 @@ describe Principals::DeleteJob, type: :model do it 'removes the query' do job - expect(CostQuery.find_by_id(query.id)).to be_nil + expect(CostQuery.find_by(id: query.id)).to be_nil end end @@ -271,7 +271,7 @@ describe Principals::DeleteJob, type: :model do end it 'leaves the query' do - expect(CostQuery.find_by_id(query.id)).to eq(query) + expect(CostQuery.find_by(id: query.id)).to eq(query) end it 'rewrites the user reference' do @@ -366,6 +366,37 @@ describe Principals::DeleteJob, type: :model do end end + shared_examples_for 'mention rewriting' do + let(:text) do + <<~TEXT + + @#{principal.name} + + TEXT + end + let(:expected_text) do + <<~TEXT.squish + @#{deleted_user.name} + TEXT + end + let!(:work_package) { create(:work_package, description: text) } + + before do + job + end + + it 'rewrites the mentioning in the text' do + expect(work_package.reload.description) + .to include expected_text + end + end + context 'with a user' do it_behaves_like 'removes the principal' it_behaves_like 'work_package handling' @@ -381,6 +412,7 @@ describe Principals::DeleteJob, type: :model do it_behaves_like 'private cost_query handling' it_behaves_like 'public cost_query handling' it_behaves_like 'cost_query handling' + it_behaves_like 'mention rewriting' end context 'with a group' do From 9673221783f1f07932a4c6854c7ee4f1eb617a61 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 28 Jun 2022 09:57:00 +0200 Subject: [PATCH 2/4] limit mention replacement to `text` cfs --- .../users/replace_mentions_service.rb | 62 +++++++++++++------ ...place_mentions_service_integration_spec.rb | 4 +- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/app/services/users/replace_mentions_service.rb b/app/services/users/replace_mentions_service.rb index 63a8cff32b..015a06f44d 100644 --- a/app/services/users/replace_mentions_service.rb +++ b/app/services/users/replace_mentions_service.rb @@ -37,8 +37,30 @@ module Users { class: Document, column: :description }, { class: Journal, column: :notes }, { class: Comment, column: :comments }, - { class: CustomValue, column: :value }, - { class: Journal::CustomizableJournal, column: :value }, + { + class: CustomValue, + column: :value, + condition: <<~SQL.squish + EXISTS ( + SELECT 1 + FROM #{CustomField.table_name} + WHERE field_format = 'text' + AND #{CustomValue.table_name}.custom_field_id = custom_fields.id + ) + SQL + }, + { + class: Journal::CustomizableJournal, + column: :value, + condition: <<~SQL.squish + EXISTS ( + SELECT 1 + FROM #{CustomField.table_name} + WHERE field_format = 'text' + AND #{Journal::CustomizableJournal.table_name}.custom_field_id = custom_fields.id + ) + SQL + }, { class: MeetingContent, column: :text }, { class: Journal::MeetingContentJournal, column: :text }, { class: Message, column: :content }, @@ -68,28 +90,28 @@ module Users def rewrite(from, to) self.class.replacements.each do |replacement| - rewrite_column(replacement[:class], replacement[:column], from, to) + rewrite_column(replacement, from, to) end end - def rewrite_column(klass, column, from, to) + def rewrite_column(replacement, from, to) sql = <<~SQL.squish - UPDATE #{klass.table_name} sink - SET #{column} = source.replacement + UPDATE #{replacement[:class].table_name} sink + SET #{replacement[:column]} = source.replacement FROM (SELECT - #{klass.table_name}.id, - #{replace_sql(klass, "#{klass.table_name}.#{column}", from, to)} replacement + #{replacement[:class].table_name}.id, + #{replace_sql(replacement[:class], "#{replacement[:class].table_name}.#{replacement[:column]}", from, to)} replacement FROM - #{klass.table_name} + #{replacement[:class].table_name} WHERE - #{condition_sql(klass, column, from)} + #{condition_sql(replacement, from)} ) source WHERE source.id = sink.id SQL - klass + replacement[:class] .connection .execute sql end @@ -104,16 +126,20 @@ module Users to) end - def condition_sql(klass, column, from) + def condition_sql(replacement, from) sql = <<~SQL.squish - #{klass.table_name}.#{column} SIMILAR TO '_*(()|(user#((%i)|("%s")|("%s")\s)))_*' + #{replacement[:class].table_name}.#{replacement[:column]} SIMILAR TO '_*(()|(user#((%i)|("%s")|("%s")\s)))_*' SQL - klass.sanitize_sql_for_conditions [sql, - from.id, - from.id, - klass.sanitize_sql_like(from.mail), - klass.sanitize_sql_like(from.login)] + if replacement.has_key?(:condition) + sql += "AND #{replacement[:condition]}" + end + + replacement[:class].sanitize_sql_for_conditions [sql, + from.id, + from.id, + replacement[:class].sanitize_sql_like(from.mail), + replacement[:class].sanitize_sql_like(from.login)] end def mention_tag_replace(klass, source, from, to) diff --git a/spec/services/users/replace_mentions_service_integration_spec.rb b/spec/services/users/replace_mentions_service_integration_spec.rb index 1da485fd4a..3b9dbd9854 100644 --- a/spec/services/users/replace_mentions_service_integration_spec.rb +++ b/spec/services/users/replace_mentions_service_integration_spec.rb @@ -284,13 +284,13 @@ describe Users::ReplaceMentionsService, 'integration' do context 'for custom_value value' do it_behaves_like 'rewritten mention', :custom_value, :value do - let(:additional_properties) { { custom_field: create(:string_wp_custom_field) } } + let(:additional_properties) { { custom_field: create(:text_wp_custom_field) } } end end context 'for customizable_journal value' do it_behaves_like 'rewritten mention', :journal_customizable_journal, :value do - let(:additional_properties) { { journal: create(:journal), custom_field: create(:string_wp_custom_field) } } + let(:additional_properties) { { journal: create(:journal), custom_field: create(:text_wp_custom_field) } } end end From cf32d4e46b6efd44a82cf7007d7fb77df5934019 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 28 Jun 2022 10:26:40 +0200 Subject: [PATCH 3/4] restructure to avoid multi valued method signatures --- .../users/replace_mentions_service.rb | 107 ++++++++++++------ 1 file changed, 72 insertions(+), 35 deletions(-) diff --git a/app/services/users/replace_mentions_service.rb b/app/services/users/replace_mentions_service.rb index 015a06f44d..c9574f1930 100644 --- a/app/services/users/replace_mentions_service.rb +++ b/app/services/users/replace_mentions_service.rb @@ -74,6 +74,14 @@ module Users ] end + def initialize(*classes) + self.replacements = if classes.any? + self.class.replacements.select { |r| classes.include?(r[:class]) } + else + self.class.replacements + end + end + def call(from:, to:) check_input(from, to) @@ -84,91 +92,96 @@ module Users private + attr_accessor :replacements + def check_input(from, to) raise ArgumentError unless from.is_a?(User) && to.is_a?(User) end def rewrite(from, to) - self.class.replacements.each do |replacement| - rewrite_column(replacement, from, to) + # If we have only one replacement configured for this instance of the service, we do it ourselves. + # Otherwise, we instantiate instances of the service's class for every class for which replacements + # are to be carried out. + # That way, instance variables can be used which prevents having method interfaces with 4 or 5 parameters. + if replacements.length == 1 + rewrite_column(from, to) + else + replacements.map do |replacement| + self.class.new(replacement[:class]).call(from:, to:) + end end end - def rewrite_column(replacement, from, to) + def rewrite_column(from, to) sql = <<~SQL.squish - UPDATE #{replacement[:class].table_name} sink - SET #{replacement[:column]} = source.replacement + UPDATE #{table_name} sink + SET #{column_name} = source.replacement FROM (SELECT - #{replacement[:class].table_name}.id, - #{replace_sql(replacement[:class], "#{replacement[:class].table_name}.#{replacement[:column]}", from, to)} replacement + #{table_name}.id, + #{replace_sql(from, to)} replacement FROM - #{replacement[:class].table_name} + #{table_name} WHERE - #{condition_sql(replacement, from)} + #{condition_sql(from)} ) source WHERE source.id = sink.id SQL - replacement[:class] + klass .connection .execute sql end - def replace_sql(klass, source, from, to) - hash_replace(klass, - mention_tag_replace(klass, - source, - from, + def replace_sql(from, to) + hash_replace(mention_tag_replace(from, to), from, to) end - def condition_sql(replacement, from) + def condition_sql(from) sql = <<~SQL.squish - #{replacement[:class].table_name}.#{replacement[:column]} SIMILAR TO '_*(()|(user#((%i)|("%s")|("%s")\s)))_*' + #{table_name}.#{column_name} SIMILAR TO '_*(()|(user#((%i)|("%s")|("%s")\s)))_*' SQL - if replacement.has_key?(:condition) - sql += "AND #{replacement[:condition]}" + if condition + sql += "AND #{condition}" end - replacement[:class].sanitize_sql_for_conditions [sql, - from.id, - from.id, - replacement[:class].sanitize_sql_like(from.mail), - replacement[:class].sanitize_sql_like(from.login)] + sanitize_sql_for_conditions [sql, + from.id, + from.id, + sanitize_sql_like(from.mail), + sanitize_sql_like(from.login)] end - def mention_tag_replace(klass, source, from, to) + def mention_tag_replace(from, to) regexp_replace( - klass, - source, + "#{table_name}.#{column_name}", '', '@%s', [from.id, to.id, - klass.sanitize_sql_like(to.name), - klass.sanitize_sql_like(to.name)] + sanitize_sql_like(to.name), + sanitize_sql_like(to.name)] ) end - def hash_replace(klass, source, from, to) + def hash_replace(source, from, to) regexp_replace( - klass, source, 'user#((%i)|("%s")|("%s")\s)', 'user#%i ', [from.id, - klass.sanitize_sql_like(from.login), - klass.sanitize_sql_like(from.mail), + sanitize_sql_like(from.login), + sanitize_sql_like(from.mail), to.id] ) end - def regexp_replace(klass, source, search, replacement, values) + def regexp_replace(source, search, replacement, values) sql = <<~SQL.squish REGEXP_REPLACE( #{source}, @@ -178,11 +191,35 @@ module Users ) SQL - klass.sanitize_sql_for_conditions [sql].concat(values) + sanitize_sql_for_conditions [sql].concat(values) + end + + def sanitize_sql_like(string) + klass.sanitize_sql_like(string) + end + + def sanitize_sql_for_conditions(string) + klass.sanitize_sql_for_conditions string end def journal_classes [Journal] + Journal::BaseJournal.subclasses end + + def klass + replacements[0][:class] + end + + def table_name + replacements[0][:class].table_name + end + + def column_name + replacements[0][:column] + end + + def condition + replacements[0][:condition] + end end end From a0ad5eed3949fb1986db27ec9d96402e98f3f319 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 28 Jun 2022 10:53:56 +0200 Subject: [PATCH 4/4] add mention replacements for groups --- .../users/replace_mentions_service.rb | 51 ++++++++--- app/workers/principals/delete_job.rb | 4 +- ...place_mentions_service_integration_spec.rb | 84 ++++++++++++++++--- .../principals/delete_job_integration_spec.rb | 1 + 4 files changed, 115 insertions(+), 25 deletions(-) diff --git a/app/services/users/replace_mentions_service.rb b/app/services/users/replace_mentions_service.rb index c9574f1930..85b4087040 100644 --- a/app/services/users/replace_mentions_service.rb +++ b/app/services/users/replace_mentions_service.rb @@ -95,7 +95,7 @@ module Users attr_accessor :replacements def check_input(from, to) - raise ArgumentError unless from.is_a?(User) && to.is_a?(User) + raise ArgumentError unless (from.is_a?(User) || from.is_a?(Group)) && to.is_a?(User) end def rewrite(from, to) @@ -142,8 +142,24 @@ module Users end def condition_sql(from) + mention = '' + hash = if from.is_a?(User) + 'user#((%i)|("%s")|("%s"))\s' + else + 'group#%i\s' + end + + hash_values = if from.is_a?(User) + [ + sanitize_sql_like(from.mail), + sanitize_sql_like(from.login) + ] + else + [] + end + sql = <<~SQL.squish - #{table_name}.#{column_name} SIMILAR TO '_*(()|(user#((%i)|("%s")|("%s")\s)))_*' + #{table_name}.#{column_name} SIMILAR TO '_*((#{mention})|(#{hash}))_*' SQL if condition @@ -152,9 +168,7 @@ module Users sanitize_sql_for_conditions [sql, from.id, - from.id, - sanitize_sql_like(from.mail), - sanitize_sql_like(from.login)] + from.id] + hash_values end def mention_tag_replace(from, to) @@ -170,14 +184,31 @@ module Users end def hash_replace(source, from, to) + search = if from.is_a?(User) + 'user#((%i)|("%s")|("%s"))\s' + else + 'group#%i\s' + end + + values = if from.is_a?(User) + [ + from.id, + sanitize_sql_like(from.login), + sanitize_sql_like(from.mail), + to.id + ] + else + [ + from.id, + to.id + ] + end + regexp_replace( source, - 'user#((%i)|("%s")|("%s")\s)', + search, 'user#%i ', - [from.id, - sanitize_sql_like(from.login), - sanitize_sql_like(from.mail), - to.id] + values ) end diff --git a/app/workers/principals/delete_job.rb b/app/workers/principals/delete_job.rb index 10aba98f79..7c4f6d1695 100644 --- a/app/workers/principals/delete_job.rb +++ b/app/workers/principals/delete_job.rb @@ -53,8 +53,8 @@ class Principals::DeleteJob < ApplicationJob def replace_mentions(principal) # Breaking abstraction here. # Doing the replacement is a very costly operation while at the same time, - # groups and placeholder users can't be mentioned. - return unless principal.is_a?(User) + # placeholder users can't be mentioned. + return unless principal.is_a?(User) || principal.is_a?(Group) Users::ReplaceMentionsService .new diff --git a/spec/services/users/replace_mentions_service_integration_spec.rb b/spec/services/users/replace_mentions_service_integration_spec.rb index 3b9dbd9854..bcb6278b0b 100644 --- a/spec/services/users/replace_mentions_service_integration_spec.rb +++ b/spec/services/users/replace_mentions_service_integration_spec.rb @@ -29,16 +29,19 @@ require 'spec_helper' describe Users::ReplaceMentionsService, 'integration' do - subject(:service_call) { instance.call(from: user, to: to_user) } + subject(:service_call) { instance.call(from: principal, to: to_user) } shared_let(:other_user) { create(:user, firstname: 'Frank', lastname: 'Herbert') } shared_let(:user) { create(:user, firstname: 'Isaac', lastname: 'Asimov') } + shared_let(:group) { create(:group, lastname: 'Sci-Fi') } shared_let(:to_user) { create :user, firstname: 'Philip K.', lastname: 'Dick' } let(:instance) do described_class.new end + let(:principal) { user } + it 'is successful' do expect(service_call) .to be_success @@ -63,6 +66,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with the replaced user in mention tags' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -86,6 +91,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with a different user in mention tags' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -109,6 +116,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with the replaced user in a user#ID notation' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -132,6 +141,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with a different user in a user#ID notation' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -155,6 +166,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with the replaced user in a user#"LOGIN" notation' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -178,6 +191,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with a different user in a user#"LOGIN" notation' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -201,6 +216,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with the replaced user in a user#"MAIL" notation' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -224,6 +241,8 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'with a different user in a user#"MAIL" notation' do + let(:principal) { user } + it_behaves_like 'text replacement', attribute do let(:text) do <<~TEXT @@ -245,6 +264,56 @@ describe Users::ReplaceMentionsService, 'integration' do end end end + + context 'with the replaced group in mention tags' do + let(:principal) { group } + + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + + @#{group.name} + + TEXT + end + let(:expected_text) do + <<~TEXT.squish + @#{to_user.name} + TEXT + end + end + end + + context 'with the replaced group in a group#ID notation' do + let(:principal) { group } + + it_behaves_like 'text replacement', attribute do + let(:text) do + <<~TEXT + Lorem ipsum + + group##{group.id} Lorem ipsum + + Lorem ipsum + TEXT + end + let(:expected_text) do + <<~TEXT + Lorem ipsum + + user##{to_user.id} Lorem ipsum + + Lorem ipsum + TEXT + end + end + end end context 'for work package description' do @@ -252,8 +321,6 @@ describe Users::ReplaceMentionsService, 'integration' do end context 'for work package description with dangerous mails' do - subject(:service_call) { instance.call(from: dangerous_user, to: to_user) } - let(:dangerous_user) do build(:user, firstname: 'Dangerous', @@ -262,7 +329,7 @@ describe Users::ReplaceMentionsService, 'integration' do user.save(validate: false) end end - let(:user) { dangerous_user } + let(:principal) { dangerous_user } it 'escapes the malicious input' do expect { service_call } @@ -340,15 +407,6 @@ describe Users::ReplaceMentionsService, 'integration' do it_behaves_like 'rewritten mention', :journal_wiki_content_journal, :text end - context 'for a group for from' do - subject(:service_call) { instance.call(from: create(:group), to: to_user) } - - it 'raises an error' do - expect { service_call } - .to raise_error ArgumentError - end - end - context 'for a group for to' do subject(:service_call) { instance.call(from: user, to: create(:group)) } diff --git a/spec/workers/principals/delete_job_integration_spec.rb b/spec/workers/principals/delete_job_integration_spec.rb index 8eb73d4bac..69b1731fbb 100644 --- a/spec/workers/principals/delete_job_integration_spec.rb +++ b/spec/workers/principals/delete_job_integration_spec.rb @@ -422,6 +422,7 @@ describe Principals::DeleteJob, type: :model do it_behaves_like 'removes the principal' it_behaves_like 'work_package handling' it_behaves_like 'member handling' + it_behaves_like 'mention rewriting' context 'with user only in project through group' do let(:user) do