From 31e801ffa4fce48c3990728a1ebcf2513a921c3a Mon Sep 17 00:00:00 2001 From: ulferts Date: Mon, 20 Jun 2022 16:59:02 +0200 Subject: [PATCH 1/9] 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 e185de4a168153341ad68a140ff2d2c39e73eeb7 Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Tue, 28 Jun 2022 08:33:36 +0200 Subject: [PATCH 2/9] Avoid that two banners are shown in case a wp is a child AND follows another wp. In that case only the warning should be shown. --- .../datepicker/banner/datepicker-banner.component.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/app/shared/components/datepicker/banner/datepicker-banner.component.html b/frontend/src/app/shared/components/datepicker/banner/datepicker-banner.component.html index c405120aaf..14752803d4 100644 --- a/frontend/src/app/shared/components/datepicker/banner/datepicker-banner.component.html +++ b/frontend/src/app/shared/components/datepicker/banner/datepicker-banner.component.html @@ -20,7 +20,7 @@ Date: Tue, 28 Jun 2022 09:57:00 +0200 Subject: [PATCH 3/9] 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 4/9] 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 5/9] 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 From abda4eebce9c931ec25c58e25c6c43503b09fb90 Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Tue, 28 Jun 2022 12:17:24 +0200 Subject: [PATCH 6/9] Disable all dates blocked by a preceds/follows relationship in the team planner --- .../datepicker/datepicker.modal.service.ts | 8 +++- .../components/datepicker/datepicker.modal.ts | 46 +++++++++++++++++-- .../global_styles/content/_datepicker.sass | 22 +++++---- 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/frontend/src/app/shared/components/datepicker/datepicker.modal.service.ts b/frontend/src/app/shared/components/datepicker/datepicker.modal.service.ts index 9e6b11effa..6162d796d9 100644 --- a/frontend/src/app/shared/components/datepicker/datepicker.modal.service.ts +++ b/frontend/src/app/shared/components/datepicker/datepicker.modal.service.ts @@ -55,12 +55,16 @@ export class DatepickerModalService { private changeset:WorkPackageChangeset = this.locals.changeset as WorkPackageChangeset; - precedingWorkPackages$:Observable<{ id:string }[]> = this + precedingWorkPackages$:Observable<{ id:string, dueDate?:string, date?:string }[]> = this .apiV3Service .work_packages .signalled( ApiV3Filter('precedes', '=', [this.changeset.id]), - ['elements/id'], + [ + 'elements/id', + 'elements/dueDate', + 'elements/date', + ], ) .pipe( map((collection:IHALCollection<{ id:string }>) => collection._embedded.elements || []), diff --git a/frontend/src/app/shared/components/datepicker/datepicker.modal.ts b/frontend/src/app/shared/components/datepicker/datepicker.modal.ts index e65d7bd766..d740768d25 100644 --- a/frontend/src/app/shared/components/datepicker/datepicker.modal.ts +++ b/frontend/src/app/shared/components/datepicker/datepicker.modal.ts @@ -55,6 +55,10 @@ import { } from 'flatpickr/dist/types/instance'; import flatpickr from 'flatpickr'; import { DatepickerModalService } from 'core-app/shared/components/datepicker/datepicker.modal.service'; +import { + map, + take, +} from 'rxjs/operators'; export type DateKeys = 'date'|'start'|'end'; @@ -134,9 +138,15 @@ export class DatePickerModalComponent extends OpModalComponent implements AfterV } ngAfterViewInit():void { - this.initializeDatepicker(); - - this.onDataChange(); + this + .datepickerService + .precedingWorkPackages$ + .pipe( + take(1), + ).subscribe((relation) => { + this.initializeDatepicker(this.minimalDateFromPrecedingRelationship(relation)); + this.onDataChange(); + }); } changeSchedulingMode():void { @@ -225,7 +235,7 @@ export class DatePickerModalComponent extends OpModalComponent implements AfterV return !this.scheduleManually && !!this.changeset.value('scheduleManually'); } - private initializeDatepicker() { + private initializeDatepicker(minimalDate?:Date|null) { this.datePickerInstance?.destroy(); this.datePickerInstance = new DatePicker( this.injector, @@ -248,6 +258,10 @@ export class DatePickerModalComponent extends OpModalComponent implements AfterV dayElem.classList.add('flatpickr-non-working-day'); } + if (minimalDate && dayElem.dateObj <= minimalDate) { + dayElem.classList.add('flatpickr-disabled'); + } + dayElem.setAttribute('data-iso-date', dayElem.dateObj.toISOString()); }, }, @@ -349,4 +363,28 @@ export class DatePickerModalComponent extends OpModalComponent implements AfterV instance.calendarContainer.classList.add('disabled'); } } + + private minimalDateFromPrecedingRelationship(relations:{ id:string, dueDate?:string, date?:string }[]):Date|null { + if (relations.length === 0) { + return null; + } + + let minimalDate:Date|null = null; + + relations.forEach((relation) => { + if (!relation.dueDate && !relation.date) { + return; + } + + const relationDate = relation.dueDate || relation.date; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const parsedRelationDate = this.datepickerService.parseDate(relationDate!); + + if (!minimalDate || minimalDate < parsedRelationDate) { + minimalDate = parsedRelationDate === '' ? null : parsedRelationDate; + } + }); + + return minimalDate; + } } diff --git a/frontend/src/global_styles/content/_datepicker.sass b/frontend/src/global_styles/content/_datepicker.sass index 41f2d0099a..07c6f6da86 100644 --- a/frontend/src/global_styles/content/_datepicker.sass +++ b/frontend/src/global_styles/content/_datepicker.sass @@ -28,6 +28,17 @@ $datepicker--border-radius: 5px +@mixin disabled-day + background: $spot-color-basic-white + color: $spot-color-basic-gray-4 !important + border-radius: 0 + background: $spot-color-basic-white + pointer-events: none + cursor: not-allowed + + &:hover + border-color: transparent + .flatpickr-current-month display: flex !important @@ -37,7 +48,8 @@ $datepicker--border-radius: 5px .flatpickr-day - &.flatpickr-disabled, + &.flatpickr-disabled + @include disabled-day &.flatpickr-non-working-day background: $spot-color-basic-gray-6 border-radius: 0 @@ -132,13 +144,7 @@ $datepicker--border-radius: 5px &.disabled .flatpickr-days .flatpickr-day - color: $spot-color-basic-gray-4 - background: $spot-color-basic-white - pointer-events: none - cursor: not-allowed - - &:hover - border-color: transparent + @include disabled-day &.selected, &.startRange, From dd15f7b97035f831c86b2044f82cbf667cdd1447 Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Tue, 28 Jun 2022 14:46:06 +0200 Subject: [PATCH 7/9] Reposition the modal after the calendar is drawn since it affects the modal height and could thus overlap the window border in some cases --- .../app/shared/components/datepicker/datepicker.modal.ts | 7 +++---- .../table/scheduling/manual_scheduling_spec.rb | 1 + spec/support/edit_fields/date_edit_field.rb | 6 ++++++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/frontend/src/app/shared/components/datepicker/datepicker.modal.ts b/frontend/src/app/shared/components/datepicker/datepicker.modal.ts index d740768d25..00ae8718fb 100644 --- a/frontend/src/app/shared/components/datepicker/datepicker.modal.ts +++ b/frontend/src/app/shared/components/datepicker/datepicker.modal.ts @@ -55,10 +55,8 @@ import { } from 'flatpickr/dist/types/instance'; import flatpickr from 'flatpickr'; import { DatepickerModalService } from 'core-app/shared/components/datepicker/datepicker.modal.service'; -import { - map, - take, -} from 'rxjs/operators'; +import { take } from 'rxjs/operators'; +import { activeFieldContainerClassName } from 'core-app/shared/components/fields/edit/edit-form/edit-form'; export type DateKeys = 'date'|'start'|'end'; @@ -247,6 +245,7 @@ export class DatePickerModalComponent extends OpModalComponent implements AfterV inline: true, onReady: (selectedDates, dateStr, instance) => { this.toggleDisabledState(instance); + this.reposition(jQuery(this.modalContainer.nativeElement), jQuery(`.${activeFieldContainerClassName}`)); }, onChange: (dates:Date[]) => { this.handleDatePickerChange(dates); diff --git a/spec/features/work_packages/table/scheduling/manual_scheduling_spec.rb b/spec/features/work_packages/table/scheduling/manual_scheduling_spec.rb index 4405ea011e..3e60583f50 100644 --- a/spec/features/work_packages/table/scheduling/manual_scheduling_spec.rb +++ b/spec/features/work_packages/table/scheduling/manual_scheduling_spec.rb @@ -98,6 +98,7 @@ describe 'Manual scheduling', js: true do start_date.toggle_scheduling_mode start_date.expect_scheduling_mode manually: true + start_date.expect_calendar # Expect not editable start_date.within_modal do diff --git a/spec/support/edit_fields/date_edit_field.rb b/spec/support/edit_fields/date_edit_field.rb index e04abc99ee..0fa9f322f6 100644 --- a/spec/support/edit_fields/date_edit_field.rb +++ b/spec/support/edit_fields/date_edit_field.rb @@ -89,6 +89,12 @@ class DateEditField < EditField expect(page).to have_no_selector("#{modal_selector} #{input_selector}") end + def expect_calendar + within_modal do + expect(page).to have_selector(".flatpickr-calendar") + end + end + def update(value, save: true, expect_failure: false) # Retry to set attributes due to reloading the page after setting # an attribute, which may cause an input not to open properly. From 74494e2c8c4f1f17c4d9ecc4bb5a27c874b0caea Mon Sep 17 00:00:00 2001 From: nathannaveen <42319948+nathannaveen@users.noreply.github.com> Date: Wed, 29 Jun 2022 00:59:07 +0000 Subject: [PATCH 8/9] chore: Set permissions for GitHub actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restrict the GitHub token permissions only to the required ones; this way, even if the attackers will succeed in compromising your workflow, they won’t be able to do much. - Included permissions for the action. https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs [Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com> --- .github/workflows/brakeman-scan-core.yml | 6 ++++++ .github/workflows/continuous-delivery.yml | 5 +++++ .github/workflows/test-core.yml | 3 +++ 3 files changed, 14 insertions(+) diff --git a/.github/workflows/brakeman-scan-core.yml b/.github/workflows/brakeman-scan-core.yml index 25cce7f6fb..3722bf183f 100644 --- a/.github/workflows/brakeman-scan-core.yml +++ b/.github/workflows/brakeman-scan-core.yml @@ -10,8 +10,14 @@ on: schedule: - cron: '10 6 * * 1' +permissions: + contents: read + jobs: brakeman-scan: + permissions: + contents: read # for actions/checkout to fetch code + security-events: write # for github/codeql-action/upload-sarif to upload SARIF results if: github.repository == 'opf/openproject' name: Brakeman Scan runs-on: ubuntu-latest diff --git a/.github/workflows/continuous-delivery.yml b/.github/workflows/continuous-delivery.yml index f0fe5b441f..06dc824b03 100644 --- a/.github/workflows/continuous-delivery.yml +++ b/.github/workflows/continuous-delivery.yml @@ -4,8 +4,13 @@ on: branches: - dev - release/* +permissions: + contents: read + jobs: trigger_downstream_workflow: + permissions: + contents: none if: github.repository == 'opf/openproject' runs-on: ubuntu-latest steps: diff --git a/.github/workflows/test-core.yml b/.github/workflows/test-core.yml index b86148a369..59044c7163 100644 --- a/.github/workflows/test-core.yml +++ b/.github/workflows/test-core.yml @@ -14,6 +14,9 @@ on: - 'docs/**' - 'help/**' +permissions: + contents: read + jobs: units: name: Units From 9a26490e66ac572ac7513ff9e652d39f668542de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Wed, 29 Jun 2022 11:04:47 +0200 Subject: [PATCH 9/9] Fix asterisk in csp config --- config/initializers/secure_headers.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/config/initializers/secure_headers.rb b/config/initializers/secure_headers.rb index 54d6195a02..09b56f44ae 100644 --- a/config/initializers/secure_headers.rb +++ b/config/initializers/secure_headers.rb @@ -38,7 +38,9 @@ Rails.application.config.after_initialize do # Add proxy configuration for Angular CLI to csp if FrontendAssetHelper.assets_proxied? - proxied = ['ws://localhost:*', 'http://localhost:*', FrontendAssetHelper.cli_proxy] + proxied = ['ws://localhost:3000', 'http://localhost:3000', + 'ws://localhost:4200', 'http://localhost:4200', + FrontendAssetHelper.cli_proxy] connect_src += proxied assets_src += proxied end