Merge pull request #10852 from opf/bug/41499-deleted-users-are-not-properly-anonymized-in-tagged-messages

replace structured mentions of user upon deletion

app/services/users/replace_mentions_service.rb

@@ -0,0 +1,256 @@
+#  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,
+          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 },
+        { 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 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)
+
+      rewrite(from, to)
+
+      ServiceResult.success
+    end
+
+    private
+
+    attr_accessor :replacements
+
+    def check_input(from, to)
+      raise ArgumentError unless (from.is_a?(User) || from.is_a?(Group)) && to.is_a?(User)
+    end
+
+    def rewrite(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(from, to)
+      sql = <<~SQL.squish
+        UPDATE #{table_name} sink
+        SET #{column_name} = source.replacement
+        FROM
+          (SELECT
+              #{table_name}.id,
+              #{replace_sql(from, to)} replacement
+           FROM
+             #{table_name}
+           WHERE
+             #{condition_sql(from)}
+          ) source
+        WHERE
+          source.id = sink.id
+      SQL
+
+      klass
+        .connection
+        .execute sql
+    end
+
+    def replace_sql(from, to)
+      hash_replace(mention_tag_replace(from,
+                                       to),
+                   from,
+                   to)
+    end
+
+    def condition_sql(from)
+      mention = '<mention_*data-id="%i"_*</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 '_*((#{mention})|(#{hash}))_*'
+      SQL
+
+      if condition
+        sql += "AND #{condition}"
+      end
+
+      sanitize_sql_for_conditions [sql,
+                                   from.id,
+                                   from.id] + hash_values
+    end
+
+    def mention_tag_replace(from, to)
+      regexp_replace(
+        "#{table_name}.#{column_name}",
+        '<mention.+data-id="%i".+</mention>',
+        '<mention class="mention" data-id="%i" data-type="user" data-text="@%s">@%s</mention>',
+        [from.id,
+         to.id,
+         sanitize_sql_like(to.name),
+         sanitize_sql_like(to.name)]
+      )
+    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,
+        search,
+        'user#%i ',
+        values
+      )
+    end
+
+    def regexp_replace(source, search, replacement, values)
+      sql = <<~SQL.squish
+        REGEXP_REPLACE(
+          #{source},
+          '#{search}',
+          '#{replacement}',
+          'g'
+        )
+      SQL
+
+      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

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,
+    # placeholder users can't be mentioned.
+    return unless principal.is_a?(User) || principal.is_a?(Group)
+
+    Users::ReplaceMentionsService
+      .new
+      .call(from: principal, to: DeletedUser.first)
+      .on_failure { raise ActiveRecord::Rollback }
   end
 
   def delete_associated(principal)

spec/services/users/replace_mentions_service_integration_spec.rb

@@ -0,0 +1,436 @@
+#-- 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: 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
+  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
+      let(:principal) { user }
+
+      it_behaves_like 'text replacement', attribute do
+        let(:text) do
+          <<~TEXT
+            <mention class="mention"
+                     data-id="#{user.id}"
+                     data-type="user"
+                     data-text="@#{user.name}">
+                     @#{user.name}
+            </mention>
+          TEXT
+        end
+        let(:expected_text) do
+          <<~TEXT.squish
+            <mention class="mention"
+                     data-id="#{to_user.id}"
+                     data-type="user"
+                     data-text="@#{to_user.name}">@#{to_user.name}</mention>
+          TEXT
+        end
+      end
+    end
+
+    context 'with a different user in mention tags' do
+      let(:principal) { user }
+
+      it_behaves_like 'text replacement', attribute do
+        let(:text) do
+          <<~TEXT
+            <mention class="mention"
+                     data-id="#{other_user.id}"
+                     data-type="user"
+                     data-text="@#{other_user.name}">
+                     @#{other_user.name}
+            </mention>
+          TEXT
+        end
+        let(:expected_text) do
+          <<~TEXT.squish
+            <mention class="mention"
+                     data-id="#{other_user.id}"
+                     data-type="user"
+                     data-text="@#{other_user.name}">@#{other_user.name}</mention>
+          TEXT
+        end
+      end
+    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
+            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
+      let(:principal) { user }
+
+      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
+      let(:principal) { user }
+
+      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
+      let(:principal) { user }
+
+      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
+      let(:principal) { user }
+
+      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
+      let(:principal) { user }
+
+      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
+
+    context 'with the replaced group in mention tags' do
+      let(:principal) { group }
+
+      it_behaves_like 'text replacement', attribute do
+        let(:text) do
+          <<~TEXT
+            <mention class="mention"
+                     data-id="#{group.id}"
+                     data-type="group"
+                     data-text="@#{group.name}">
+                     @#{group.name}
+            </mention>
+          TEXT
+        end
+        let(:expected_text) do
+          <<~TEXT.squish
+            <mention class="mention"
+                     data-id="#{to_user.id}"
+                     data-type="user"
+                     data-text="@#{to_user.name}">@#{to_user.name}</mention>
+          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
+    it_behaves_like 'rewritten mention', :work_package, :description
+  end
+
+  context 'for work package description with dangerous mails' do
+    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(:principal) { 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(: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(:text_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 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

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
+          <mention class="mention"
+                   data-id="#{principal.id}"
+                   data-type="user"
+                   data-text="@#{principal.name}">
+                   @#{principal.name}
+          </mention>
+        TEXT
+      end
+      let(:expected_text) do
+        <<~TEXT.squish
+          <mention class="mention"
+                   data-id="#{deleted_user.id}"
+                   data-type="user"
+                   data-text="@#{deleted_user.name}">@#{deleted_user.name}</mention>
+        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
@@ -390,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