[STC-794] Ensure work package identifiers are not changed on accident

https://community.openproject.org/wp/STC-794

app/models/work_package/semantic_identifier.rb

@@ -48,6 +48,14 @@ module WorkPackage::SemanticIdentifier
   # specifically when needed.
   class UnsupportedLookup < ArgumentError; end
 
+  # Validation context that permits deliberate changes to identifier/sequence_number
+  # on a persisted work package:
+  #
+  #   work_package.save(context: WorkPackage::SemanticIdentifier::IDENTIFIER_REWRITE_CONTEXT)
+  #
+  # See #semantic_identifier_fields_not_accidentally_changed.
+  IDENTIFIER_REWRITE_CONTEXT = :identifier_rewrite
+
   included do
     has_many :semantic_aliases,
              class_name: "WorkPackageSemanticAlias",
@@ -69,7 +77,7 @@ module WorkPackage::SemanticIdentifier
 
     after_create :allocate_and_register_semantic_id, if: -> { Setting::WorkPackageIdentifier.semantic? && !skip_semantic_id_allocation }
 
-    validate :semantic_identifier_fields_consistent
+    validate :validate_semantic_identifier_fields
   end
 
   class_methods do
@@ -194,6 +202,11 @@ module WorkPackage::SemanticIdentifier
 
   private
 
+  def validate_semantic_identifier_fields
+    semantic_identifier_fields_consistent
+    semantic_identifier_fields_not_accidentally_changed
+  end
+
   # Ensures identifier and sequence_number are always written together.
   # One field set without the other indicates a partial write and is never valid.
   def semantic_identifier_fields_consistent
@@ -201,4 +214,30 @@ module WorkPackage::SemanticIdentifier
 
     errors.add(:identifier, :semantic_identifier_incomplete)
   end
+
+  # Guards against accidental edits, e.g. from a Rails console: identifiers are
+  # allocated automatically and resolved through the alias table, so a
+  # hand-written change silently breaks links and identifier history.
+  # Deliberate changes must be saved with the IDENTIFIER_REWRITE_CONTEXT
+  # validation context.
+  def semantic_identifier_fields_not_accidentally_changed
+    return unless persisted?
+    return if Array(validation_context).include?(IDENTIFIER_REWRITE_CONTEXT)
+    return if cleared_for_project_move?
+
+    %w[identifier sequence_number].each do |attribute|
+      next unless attribute_changed?(attribute)
+
+      errors.add(attribute, :must_not_be_changed, context: IDENTIFIER_REWRITE_CONTEXT)
+    end
+  end
+
+  # Clearing both fields while moving to another project is part of the move
+  # operation itself: the identifier belongs to the source project and must be
+  # cleared in the same UPDATE that changes project_id (unique index on
+  # project_id/sequence_number). A fresh identifier is allocated after the move.
+  # See WorkPackages::SetAttributesService#clear_semantic_identifier.
+  def cleared_for_project_move?
+    project_id_changed? && identifier.nil? && sequence_number.nil?
+  end
 end

app/services/work_packages/set_attributes_service.rb

@@ -274,6 +274,10 @@ class WorkPackages::SetAttributesService < BaseServices::SetAttributes
     end
   end
 
+  # The identifier belongs to the source project; a fresh one is allocated
+  # after the move (WorkPackages::UpdateService#update_semantic_ids). The
+  # fields must be cleared in the same UPDATE that changes project_id because
+  # of the unique index on (project_id, sequence_number).
   def clear_semantic_identifier
     work_package.sequence_number = nil
     work_package.identifier = nil

config/locales/en.yml

@@ -2515,6 +2515,9 @@ en:
               blank: "ID can't be blank."
             identifier:
               semantic_identifier_incomplete: "and sequence_number must both be set at the same time."
+              must_not_be_changed: "must not be changed manually. Work package identifiers are allocated automatically and resolved through the alias table, so changing them by hand breaks links and identifier history. If this change is deliberate, save with `save(context: :%{context})` and keep the work_package_semantic_aliases table consistent."
+            sequence_number:
+              must_not_be_changed: "must not be changed manually. It is allocated automatically together with the identifier. If this change is deliberate, save with `save(context: :%{context})`."
             assigned_to:
               format: "%{message}"
             done_ratio:

spec/models/work_package/semantic_identifier_spec.rb

@@ -693,6 +693,69 @@ RSpec.describe WorkPackage::SemanticIdentifier do
     end
   end
 
+  describe "semantic_identifier_fields_not_accidentally_changed validation" do
+    it "forbids changing the identifier in the default validation context" do
+      work_package.identifier = "MYPROJ-99"
+      work_package.sequence_number = 99
+
+      expect(work_package).not_to be_valid
+      expect(work_package.errors[:identifier]).to include(a_string_matching(/must not be changed manually/))
+    end
+
+    it "points to the rewrite context in the error message" do
+      work_package.identifier = "MYPROJ-99"
+      work_package.sequence_number = 99
+
+      work_package.validate
+      expect(work_package.errors[:identifier]).to include(a_string_matching(/identifier_rewrite/))
+    end
+
+    it "forbids changing the sequence_number" do
+      work_package.sequence_number = 99
+
+      expect(work_package).not_to be_valid
+      expect(work_package.errors[:sequence_number]).to include(a_string_matching(/must not be changed manually/))
+    end
+
+    it "forbids clearing the identifier fields" do
+      work_package.identifier = nil
+      work_package.sequence_number = nil
+
+      expect(work_package).not_to be_valid
+    end
+
+    it "allows clearing the fields as part of a project move" do
+      work_package.project = create(:project)
+      work_package.identifier = nil
+      work_package.sequence_number = nil
+
+      work_package.validate
+      expect(work_package.errors[:identifier]).to be_empty
+      expect(work_package.errors[:sequence_number]).to be_empty
+    end
+
+    it "allows the change when saved with the rewrite context" do
+      work_package.identifier = "MYPROJ-99"
+      work_package.sequence_number = 99
+
+      expect(work_package.save(context: WorkPackage::SemanticIdentifier::IDENTIFIER_REWRITE_CONTEXT)).to be(true)
+      expect(work_package.reload.identifier).to eq("MYPROJ-99")
+      expect(work_package.reload.sequence_number).to eq(99)
+    end
+
+    it "does not interfere with unrelated changes" do
+      work_package.subject = "Updated subject"
+
+      expect(work_package).to be_valid
+    end
+
+    it "does not apply to new records" do
+      wp = build(:work_package, project:, identifier: "MYPROJ-5", sequence_number: 5)
+
+      expect(wp).to be_valid
+    end
+  end
+
   describe "#allocate_and_register_semantic_id", with_settings: { work_packages_identifier: "semantic" } do
     let(:project) { create(:project, identifier: "PROJ", wp_sequence_counter: 0) }
     let(:target_project) { create(:project, identifier: "OTHER", wp_sequence_counter: 0) }

spec/services/work_packages/semantic_ids/integration_spec.rb

@@ -279,6 +279,21 @@ RSpec.describe "SemanticIds registry integration",
         result = WorkPackages::UpdateService.new(user:, model: wp).call(subject: "Updated subject")
         expect(result).to be_success
       end
+
+      it "UpdateService clears stale identifier fields on a project move" do
+        wp = WorkPackages::CreateService.new(user:).call(**attributes).result
+        # Simulate a WP that received a semantic id before the instance was
+        # switched (back) to classic mode.
+        wp.update_columns(identifier: "OLDPROJ-7", sequence_number: 7)
+
+        result = WorkPackages::UpdateService.new(user:, model: wp).call(project: target_project)
+
+        expect(result).to be_success
+        expect(wp.identifier).to be_nil
+        expect(wp.sequence_number).to be_nil
+        expect(wp.reload.identifier).to be_nil
+        expect(wp.sequence_number).to be_nil
+      end
     end
 
     context "in semantic mode", with_settings: { work_packages_identifier: "semantic" } do