Remove fork network relations upon project visibility decrease

It handles scenarios where the project would also decrease
the visibility of its forks. Instead of doing so, we remove
this project from the fork network, which will disallow
it sending or receiving MRs from other projects in this
network.

app/models/merge_request.rb

@@ -199,6 +199,9 @@ class MergeRequest < ApplicationRecord
   scope :by_milestone, ->(milestone) { where(milestone_id: milestone) }
   scope :of_projects, ->(ids) { where(target_project_id: ids) }
   scope :from_project, ->(project) { where(source_project_id: project.id) }
+  scope :from_and_to_forks, ->(project) do
+    where('source_project_id <> target_project_id AND (source_project_id = ? OR target_project_id = ?)', project.id, project.id)
+  end
   scope :merged, -> { with_state(:merged) }
   scope :closed_and_merged, -> { with_states(:closed, :merged) }
   scope :open_and_closed, -> { with_states(:opened, :closed) }

app/models/project.rb

@@ -179,6 +179,7 @@ class Project < ApplicationRecord
   has_one :forked_from_project, through: :fork_network_member
   has_many :forked_to_members, class_name: 'ForkNetworkMember', foreign_key: 'forked_from_project_id'
   has_many :forks, through: :forked_to_members, source: :project, inverse_of: :forked_from_project
+  has_many :fork_network_projects, through: :fork_network, source: :projects
 
   has_one :import_state, autosave: true, class_name: 'ProjectImportState', inverse_of: :project
   has_one :import_export_upload, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
@@ -715,6 +716,10 @@ class Project < ApplicationRecord
     Feature.enabled?(:project_daily_statistics, self, default_enabled: true)
   end
 
+  def unlink_forks_upon_visibility_decrease_enabled?
+    Feature.enabled?(:unlink_fork_network_upon_visibility_decrease, self)
+  end
+
   def empty_repo?
     repository.empty?
   end
@@ -1533,6 +1538,7 @@ class Project < ApplicationRecord
 
   # update visibility_level of forks
   def update_forks_visibility_level
+    return if unlink_forks_upon_visibility_decrease_enabled?
     return unless visibility_level < visibility_level_before_last_save
 
     forks.each do |forked_project|

app/services/projects/destroy_service.rb

@@ -31,13 +31,6 @@ module Projects
 
       Projects::UnlinkForkService.new(project, current_user).execute
 
-      # The project is not necessarily a fork, so update the fork network originating
-      # from this project
-      if fork_network = project.root_of_fork_network
-        fork_network.update(root_project: nil,
-                            deleted_root_project_name: project.full_name)
-      end
-
       attempt_destroy_transaction(project)
 
       system_hook_service.execute_hooks_for(project, :destroy)

app/services/projects/overwrite_project_service.rb

@@ -7,7 +7,9 @@ module Projects
 
       Project.transaction do
         move_before_destroy_relationships(source_project)
-        destroy_old_project(source_project)
+        # Reset is required in order to get the proper
+        # uncached fork network method calls value.
+        destroy_old_project(source_project.reset)
         rename_project(source_project.name, source_project.path)
 
         @project

app/services/projects/unlink_fork_service.rb

@@ -2,34 +2,67 @@
 
 module Projects
   class UnlinkForkService < BaseService
-    # rubocop: disable CodeReuse/ActiveRecord
+    # If a fork is given, it:
+    #
+    # - Saves LFS objects to the root project
+    # - Close existing MRs coming from it
+    # - Is removed from the fork network
+    #
+    # If a root of fork(s) is given, it does the same,
+    # but not updating LFS objects (there'll be no related root to cache it).
     def execute
-      return unless @project.forked?
+      fork_network = @project.fork_network
 
-      if fork_source = @project.fork_source
-        fork_source.lfs_objects.find_each do |lfs_object|
-          lfs_object.projects << @project unless lfs_object.projects.include?(@project)
-        end
+      return unless fork_network
 
-        refresh_forks_count(fork_source)
-      end
+      save_lfs_objects
 
-      merge_requests = @project.fork_network
+      merge_requests = fork_network
                          .merge_requests
                          .opened
-                         .where.not(target_project: @project)
-                         .from_project(@project)
+                         .from_and_to_forks(@project)
 
-      merge_requests.each do |mr|
+      merge_requests.find_each do |mr|
         ::MergeRequests::CloseService.new(@project, @current_user).execute(mr)
       end
 
-      @project.fork_network_member.destroy
+      Project.transaction do
+        # Get out of the fork network as a member and
+        # remove references from all its direct forks.
+        @project.fork_network_member.destroy
+        @project.forked_to_members.update_all(forked_from_project_id: nil)
+
+        # The project is not necessarily a fork, so update the fork network originating
+        # from this project
+        if fork_network = @project.root_of_fork_network
+          fork_network.update(root_project: nil, deleted_root_project_name: @project.full_name)
+        end
+      end
+
+      # When the project getting out of the network is a node with parent
+      # and children, both the parent and the node needs a cache refresh.
+      [@project.forked_from_project, @project].compact.each do |project|
+        refresh_forks_count(project)
+      end
     end
-    # rubocop: enable CodeReuse/ActiveRecord
+
+    private
 
     def refresh_forks_count(project)
       Projects::ForksCountService.new(project).refresh_cache
     end
+
+    def save_lfs_objects
+      return unless @project.forked?
+
+      lfs_storage_project = @project.lfs_storage_project
+
+      return unless lfs_storage_project
+      return if lfs_storage_project == @project # that project is being unlinked
+
+      lfs_storage_project.lfs_objects.find_each do |lfs_object|
+        lfs_object.projects << @project unless lfs_object.projects.include?(@project)
+      end
+    end
   end
 end

app/services/projects/update_service.rb

@@ -65,7 +65,7 @@ module Projects
       )
       project_changed_feature_keys = project.project_feature.previous_changes.keys
 
-      if project.previous_changes.include?(:visibility_level) && project.private?
+      if project.visibility_level_previous_changes && project.private?
         # don't enqueue immediately to prevent todos removal in case of a mistake
         TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, nil, project.id)
         TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id)
@@ -79,6 +79,11 @@ module Projects
         system_hook_service.execute_hooks_for(project, :update)
       end
 
+      if project.visibility_level_decreased? && project.unlink_forks_upon_visibility_decrease_enabled?
+        # It's a system-bounded operation, so no extra authorization check is required.
+        Projects::UnlinkForkService.new(project, current_user).execute
+      end
+
       update_pages_config if changing_pages_related_config?
     end
 

changelogs/unreleased/osw-delete-fork-relation-upon-visibility-change.yml

+---
+title: Adjust fork network relations upon project visibility change
+merge_request: 20466
+author:
+type: added

ee/app/controllers/ee/projects/merge_requests/application_controller.rb

@@ -62,7 +62,7 @@ module EE
           target_project = if @merge_request # rubocop:disable Gitlab/ModuleWithInstanceVariables
                              @merge_request.target_project # rubocop:disable Gitlab/ModuleWithInstanceVariables
                            elsif project.forked? && project.id.to_s != mr_params[:target_project_id]
-                             project.forked_from_project
+                             project.fork_network_projects.find(mr_params[:target_project_id])
                            else
                              project
                            end

lib/gitlab/visibility_level.rb

@@ -115,6 +115,18 @@ module Gitlab
       end
     end
 
+    def visibility_level_decreased?
+      return false unless visibility_level_previous_changes
+
+      before, after = visibility_level_previous_changes
+
+      before && after && after < before
+    end
+
+    def visibility_level_previous_changes
+      previous_changes[:visibility_level]
+    end
+
     def private?
       visibility_level_value == PRIVATE
     end

spec/lib/gitlab/import_export/all_models.yml

@@ -365,6 +365,7 @@ project:
 - root_of_fork_network
 - fork_network_member
 - fork_network
+- fork_network_projects
 - custom_attributes
 - lfs_file_locks
 - project_badges

spec/lib/gitlab/visibility_level_spec.rb

@@ -95,4 +95,28 @@ describe Gitlab::VisibilityLevel do
       expect(described_class.valid_level?(described_class::PUBLIC)).to be_truthy
     end
   end
+
+  describe '#visibility_level_decreased?' do
+    let(:project) { create(:project, :internal) }
+
+    context 'when visibility level decreases' do
+      before do
+        project.update!(visibility_level: described_class::PRIVATE)
+      end
+
+      it 'returns true' do
+        expect(project.visibility_level_decreased?).to be(true)
+      end
+    end
+
+    context 'when visibility level does not decrease' do
+      before do
+        project.update!(visibility_level: described_class::PUBLIC)
+      end
+
+      it 'returns false' do
+        expect(project.visibility_level_decreased?).to be(false)
+      end
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -33,6 +33,21 @@ describe MergeRequest do
     end
   end
 
+  describe '.from_and_to_forks' do
+    it 'returns only MRs from and to forks (with no internal MRs)' do
+      project = create(:project)
+      fork = fork_project(project)
+      fork_2 = fork_project(project)
+      mr_from_fork = create(:merge_request, source_project: fork, target_project: project)
+      mr_to_fork = create(:merge_request, source_project: project, target_project: fork)
+
+      create(:merge_request, source_project: fork, target_project: fork_2)
+      create(:merge_request, source_project: project, target_project: project)
+
+      expect(described_class.from_and_to_forks(project)).to contain_exactly(mr_from_fork, mr_to_fork)
+    end
+  end
+
   describe 'locking' do
     using RSpec::Parameterized::TableSyntax
 

spec/services/projects/destroy_service_spec.rb

@@ -296,9 +296,12 @@ describe Projects::DestroyService do
   end
 
   context 'as the root of a fork network' do
-    let!(:fork_network) { create(:fork_network, root_project: project) }
+    let!(:fork_1) { fork_project(project, user) }
+    let!(:fork_2) { fork_project(project, user) }
 
     it 'updates the fork network with the project name' do
+      fork_network = project.fork_network
+
       destroy_project(project, user)
 
       fork_network.reload

spec/services/projects/unlink_fork_service_spec.rb

@@ -2,13 +2,13 @@
 
 require 'spec_helper'
 
-describe Projects::UnlinkForkService do
+describe Projects::UnlinkForkService, :use_clean_rails_memory_store_caching do
   include ProjectForksHelper
 
   subject { described_class.new(forked_project, user) }
 
   let(:project) { create(:project, :public) }
-  let(:forked_project) { fork_project(project, user) }
+  let!(:forked_project) { fork_project(project, user) }
   let(:user) { create(:user) }
 
   context 'with opened merge request on the source project' do
@@ -86,4 +86,169 @@ describe Projects::UnlinkForkService do
       expect { subject.execute }.not_to raise_error
     end
   end
+
+  context 'when given project is a source of forks' do
+    let!(:forked_project_2) { fork_project(project, user) }
+    let!(:fork_of_fork) { fork_project(forked_project, user) }
+
+    subject { described_class.new(project, user) }
+
+    context 'with opened merge requests from fork back to root project' do
+      let!(:merge_request) { create(:merge_request, source_project: project, target_project: forked_project) }
+      let!(:merge_request2) { create(:merge_request, source_project: project, target_project: fork_project(project)) }
+      let!(:merge_request_in_fork) { create(:merge_request, source_project: forked_project, target_project: forked_project) }
+
+      let(:mr_close_service) { MergeRequests::CloseService.new(project, user) }
+
+      before do
+        allow(MergeRequests::CloseService).to receive(:new)
+                                                .with(project, user)
+                                                .and_return(mr_close_service)
+      end
+
+      it 'closes all pending merge requests' do
+        expect(mr_close_service).to receive(:execute).with(merge_request)
+        expect(mr_close_service).to receive(:execute).with(merge_request2)
+
+        subject.execute
+      end
+
+      it 'does not close merge requests that do not come from the project being unlinked' do
+        expect(mr_close_service).not_to receive(:execute).with(merge_request_in_fork)
+
+        subject.execute
+      end
+    end
+
+    it 'removes its link to the fork network and updates direct network members' do
+      expect(project.fork_network_member).to be_present
+      expect(project.fork_network).to be_present
+      expect(project.forked_to_members.count).to eq(2)
+      expect(forked_project.forked_to_members.count).to eq(1)
+      expect(fork_of_fork.forked_to_members.count).to eq(0)
+
+      subject.execute
+
+      project.reload
+      forked_project.reload
+      fork_of_fork.reload
+
+      expect(project.fork_network_member).to be_nil
+      expect(project.fork_network).to be_nil
+      expect(forked_project.fork_network).to have_attributes(root_project_id: nil,
+                                                             deleted_root_project_name: project.full_name)
+      expect(project.forked_to_members.count).to eq(0)
+      expect(forked_project.forked_to_members.count).to eq(1)
+      expect(fork_of_fork.forked_to_members.count).to eq(0)
+    end
+
+    it 'refreshes the forks count cache of the given project' do
+      expect(project.forks_count).to eq(2)
+
+      subject.execute
+
+      expect(project.forks_count).to be_zero
+    end
+
+    context 'when given project is a fork of an unlinked parent' do
+      let!(:fork_of_fork) { fork_project(forked_project, user) }
+      let(:lfs_object) { create(:lfs_object) }
+
+      before do
+        lfs_object.projects << project
+      end
+
+      it 'saves lfs objects to the root project' do
+        # Remove parent from network
+        described_class.new(forked_project, user).execute
+
+        described_class.new(fork_of_fork, user).execute
+
+        expect(lfs_object.projects).to include(fork_of_fork)
+      end
+    end
+
+    context 'and is node with a parent' do
+      subject { described_class.new(forked_project, user) }
+
+      context 'with opened merge requests from and to given project' do
+        let!(:mr_from_parent) { create(:merge_request, source_project: project, target_project: forked_project) }
+        let!(:mr_to_parent) { create(:merge_request, source_project: forked_project, target_project: project) }
+        let!(:mr_to_child) { create(:merge_request, source_project: forked_project, target_project: fork_of_fork) }
+        let!(:mr_from_child) { create(:merge_request, source_project: fork_of_fork, target_project: forked_project) }
+        let!(:merge_request_in_fork) { create(:merge_request, source_project: forked_project, target_project: forked_project) }
+
+        let(:mr_close_service) { MergeRequests::CloseService.new(forked_project, user) }
+
+        before do
+          allow(MergeRequests::CloseService).to receive(:new)
+            .with(forked_project, user)
+            .and_return(mr_close_service)
+        end
+
+        it 'close all pending merge requests' do
+          merge_requests = [mr_from_parent, mr_to_parent, mr_from_child, mr_to_child]
+
+          merge_requests.each do |mr|
+            expect(mr_close_service).to receive(:execute).with(mr).and_call_original
+          end
+
+          subject.execute
+
+          merge_requests = MergeRequest.where(id: merge_requests)
+
+          expect(merge_requests).to all(have_attributes(state: 'closed'))
+        end
+
+        it 'does not close merge requests which do not come from the project being unlinked' do
+          expect(mr_close_service).not_to receive(:execute).with(merge_request_in_fork)
+
+          subject.execute
+        end
+      end
+
+      it 'refreshes the forks count cache of the parent and the given project' do
+        expect(project.forks_count).to eq(2)
+        expect(forked_project.forks_count).to eq(1)
+
+        subject.execute
+
+        expect(project.forks_count).to eq(1)
+        expect(forked_project.forks_count).to eq(0)
+      end
+
+      it 'removes its link to the fork network and updates direct network members' do
+        expect(project.fork_network).to be_present
+        expect(forked_project.fork_network).to be_present
+        expect(fork_of_fork.fork_network).to be_present
+
+        expect(project.forked_to_members.count).to eq(2)
+        expect(forked_project.forked_to_members.count).to eq(1)
+        expect(fork_of_fork.forked_to_members.count).to eq(0)
+
+        subject.execute
+        project.reload
+        forked_project.reload
+        fork_of_fork.reload
+
+        expect(project.fork_network).to be_present
+        expect(forked_project.fork_network).to be_nil
+        expect(fork_of_fork.fork_network).to be_present
+
+        expect(project.forked_to_members.count).to eq(1) # 1 child is gone
+        expect(forked_project.forked_to_members.count).to eq(0)
+        expect(fork_of_fork.forked_to_members.count).to eq(0)
+      end
+    end
+  end
+
+  context 'when given project is not part of a fork network' do
+    let!(:project_without_forks) { create(:project, :public) }
+
+    subject { described_class.new(project_without_forks, user) }
+
+    it 'does not raise errors' do
+      expect { subject.execute }.not_to raise_error
+    end
+  end
 end

spec/services/projects/update_service_spec.rb

@@ -16,7 +16,14 @@ describe Projects::UpdateService do
     let(:admin) { create(:admin) }
 
     context 'when changing visibility level' do
-      context 'when visibility_level is INTERNAL' do
+      def expect_to_call_unlink_fork_service
+        service = Projects::UnlinkForkService.new(project, user)
+
+        expect(Projects::UnlinkForkService).to receive(:new).with(project, user).and_return(service)
+        expect(service).to receive(:execute).and_call_original
+      end
+
+      context 'when visibility_level changes to INTERNAL' do
         it 'updates the project to internal' do
           expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in)
 
@@ -25,9 +32,21 @@ describe Projects::UpdateService do
           expect(result).to eq({ status: :success })
           expect(project).to be_internal
         end
+
+        context 'and project is PUBLIC' do
+          before do
+            project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+          end
+
+          it 'unlinks project from fork network' do
+            expect_to_call_unlink_fork_service
+
+            update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          end
+        end
       end
 
-      context 'when visibility_level is PUBLIC' do
+      context 'when visibility_level changes to PUBLIC' do
         it 'updates the project to public' do
           expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in)
 
@@ -36,9 +55,17 @@ describe Projects::UpdateService do
           expect(result).to eq({ status: :success })
           expect(project).to be_public
         end
+
+        context 'and project is PRIVATE' do
+          it 'does not unlink project from fork network' do
+            expect(Projects::UnlinkForkService).not_to receive(:new)
+
+            update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+          end
+        end
       end
 
-      context 'when visibility_level is PRIVATE' do
+      context 'when visibility_level changes to PRIVATE' do
         before do
           project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
         end
@@ -52,6 +79,30 @@ describe Projects::UpdateService do
           expect(result).to eq({ status: :success })
           expect(project).to be_private
         end
+
+        context 'and project is PUBLIC' do
+          before do
+            project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+          end
+
+          it 'unlinks project from fork network' do
+            expect_to_call_unlink_fork_service
+
+            update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+          end
+        end
+
+        context 'and project is INTERNAL' do
+          before do
+            project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          end
+
+          it 'unlinks project from fork network' do
+            expect_to_call_unlink_fork_service
+
+            update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+          end
+        end
       end
 
       context 'when visibility levels are restricted to PUBLIC only' do
@@ -107,28 +158,48 @@ describe Projects::UpdateService do
       let(:project) { create(:project, :internal) }
       let(:forked_project) { fork_project(project) }
 
-      it 'updates forks visibility level when parent set to more restrictive' do
-        opts = { visibility_level: Gitlab::VisibilityLevel::PRIVATE }
+      context 'and unlink forks feature flag is off' do
+        before do
+          stub_feature_flags(unlink_fork_network_upon_visibility_decrease: false)
+        end
+
+        it 'updates forks visibility level when parent set to more restrictive' do
+          opts = { visibility_level: Gitlab::VisibilityLevel::PRIVATE }
+
+          expect(project).to be_internal
+          expect(forked_project).to be_internal
+
+          expect(update_project(project, admin, opts)).to eq({ status: :success })
+
+          expect(project).to be_private
+          expect(forked_project.reload).to be_private
+        end
+
+        it 'does not update forks visibility level when parent set to less restrictive' do
+          opts = { visibility_level: Gitlab::VisibilityLevel::PUBLIC }
 
-        expect(project).to be_internal
-        expect(forked_project).to be_internal
+          expect(project).to be_internal
+          expect(forked_project).to be_internal
 
-        expect(update_project(project, admin, opts)).to eq({ status: :success })
+          expect(update_project(project, admin, opts)).to eq({ status: :success })
 
-        expect(project).to be_private
-        expect(forked_project.reload).to be_private
+          expect(project).to be_public
+          expect(forked_project.reload).to be_internal
+        end
       end
 
-      it 'does not update forks visibility level when parent set to less restrictive' do
-        opts = { visibility_level: Gitlab::VisibilityLevel::PUBLIC }
+      context 'and unlink forks feature flag is on' do
+        it 'does not change visibility of forks' do
+          opts = { visibility_level: Gitlab::VisibilityLevel::PRIVATE }
 
-        expect(project).to be_internal
-        expect(forked_project).to be_internal
+          expect(project).to be_internal
+          expect(forked_project).to be_internal
 
-        expect(update_project(project, admin, opts)).to eq({ status: :success })
+          expect(update_project(project, admin, opts)).to eq({ status: :success })
 
-        expect(project).to be_public
-        expect(forked_project.reload).to be_internal
+          expect(project).to be_private
+          expect(forked_project.reload).to be_internal
+        end
       end
     end