Merge branch 'fix/visibility-level-setting-in-forked-projects' into 'master'

Fix/visibility level setting in forked projects

Fixes #3136

/cc @DouweM 

See merge request !1744

app/helpers/visibility_level_helper.rb

@@ -69,7 +69,6 @@ module VisibilityLevelHelper
 
   def skip_level?(form_model, level)
     form_model.is_a?(Project) &&
-    form_model.forked? &&
-    !Gitlab::VisibilityLevel.allowed_fork_levels(form_model.forked_from_project.visibility_level).include?(level)
+    !form_model.visibility_level_allowed?(level)
   end
 end

app/models/project.rb

@@ -64,6 +64,19 @@ class Project < ActiveRecord::Base
     update_column(:last_activity_at, self.created_at)
   end
 
+  # update visibility_levet of forks
+  after_update :update_forks_visibility_level
+  def update_forks_visibility_level
+    return unless visibility_level < visibility_level_was
+
+    forks.each do |forked_project|
+      if forked_project.visibility_level > visibility_level
+        forked_project.visibility_level = visibility_level
+        forked_project.save!
+      end
+    end
+  end
+
   ActsAsTaggableOn.strict_case_match = true
   acts_as_taggable_on :tags
 
@@ -100,9 +113,12 @@ class Project < ActiveRecord::Base
   has_one :gitlab_issue_tracker_service, dependent: :destroy
   has_one :external_wiki_service, dependent: :destroy
 
-  has_one :forked_project_link, dependent: :destroy, foreign_key: "forked_to_project_id"
+  has_one  :forked_project_link,  dependent: :destroy, foreign_key: "forked_to_project_id"
+  has_one  :forked_from_project,  through:   :forked_project_link
+
+  has_many :forked_project_links, foreign_key: "forked_from_project_id"
+  has_many :forks,                through:     :forked_project_links, source: :forked_to_project
 
-  has_one :forked_from_project, through: :forked_project_link
   # Merge Requests for target project should be removed with it
   has_many :merge_requests,     dependent: :destroy, foreign_key: 'target_project_id'
   # Merge requests from source project should be kept when source project was removed
@@ -768,7 +784,7 @@ class Project < ActiveRecord::Base
   end
 
   def forks_count
-    ForkedProjectLink.where(forked_from_project_id: self.id).count
+    forks.count
   end
 
   def find_label(name)
@@ -862,4 +878,9 @@ class Project < ActiveRecord::Base
   def open_issues_count
     issues.opened.count
   end
+
+  def visibility_level_allowed?(level)
+    return true unless forked?
+    Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level.to_i)
+  end
 end

app/services/base_service.rb

@@ -39,10 +39,7 @@ class BaseService
   def deny_visibility_level(model, denied_visibility_level = nil)
     denied_visibility_level ||= model.visibility_level
 
-    level_name = 'Unknown'
-    Gitlab::VisibilityLevel.options.each do |name, level|
-      level_name = name if level == denied_visibility_level
-    end
+    level_name = Gitlab::VisibilityLevel.level_name(denied_visibility_level)
 
     model.errors.add(
       :visibility_level,

app/services/projects/update_service.rb

@@ -3,12 +3,16 @@ module Projects
     def execute
       # check that user is allowed to set specified visibility_level
       new_visibility = params[:visibility_level]
-      if new_visibility && new_visibility.to_i != project.visibility_level
-        unless can?(current_user, :change_visibility_level, project) &&
-          Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
-          deny_visibility_level(project, new_visibility)
-          return project
+      if new_visibility
+        if new_visibility.to_i != project.visibility_level
+          unless can?(current_user, :change_visibility_level, project) &&
+            Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
+            deny_visibility_level(project, new_visibility)
+            return project
+          end
         end
+
+        return false unless visibility_level_allowed?(new_visibility)
       end
 
       new_branch = params[:default_branch]
@@ -23,5 +27,19 @@ module Projects
         end
       end
     end
+
+    private
+
+    def visibility_level_allowed?(level)
+      return true if project.visibility_level_allowed?(level)
+
+      level_name = Gitlab::VisibilityLevel.level_name(level)
+      project.errors.add(
+        :visibility_level,
+        "#{level_name} could not be set as visibility level of this project - parent project settings are more restrictive"
+      )
+
+      false
+    end
   end
 end

lib/gitlab/visibility_level.rb

@@ -51,6 +51,15 @@ module Gitlab
       def allowed_fork_levels(origin_level)
         [PRIVATE, INTERNAL, PUBLIC].select{ |level| level <= origin_level }
       end
+
+      def level_name(level)
+        level_name = 'Unknown'
+        options.each do |name, lvl|
+          level_name = name if lvl == level.to_i
+        end
+
+        level_name
+      end
     end
 
     def private?

spec/models/project_spec.rb

@@ -552,4 +552,28 @@ describe Project, models: true do
       end
     end
   end
+
+  describe '#visibility_level_allowed?' do
+    let(:project) { create :project, visibility_level: Gitlab::VisibilityLevel::INTERNAL }
+
+    context 'when checking on non-forked project' do
+      it { expect(project.visibility_level_allowed?(Gitlab::VisibilityLevel::PRIVATE)).to be_truthy }
+      it { expect(project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_truthy }
+      it { expect(project.visibility_level_allowed?(Gitlab::VisibilityLevel::PUBLIC)).to be_truthy }
+    end
+
+    context 'when checking on forked project' do
+      let(:forked_project) { create :forked_project_with_submodules }
+
+      before do
+        forked_project.build_forked_project_link(forked_to_project_id: forked_project.id, forked_from_project_id: project.id)
+        forked_project.save
+      end
+
+      it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PRIVATE)).to be_truthy }
+      it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_truthy }
+      it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PUBLIC)).to be_falsey }
+    end
+
+  end
 end

spec/services/projects/update_service_spec.rb

@@ -100,6 +100,45 @@ describe Projects::UpdateService, services: true do
     end
   end
 
+  describe :visibility_level do
+    let(:user) { create :user, admin: true }
+    let(:project) { create :project, visibility_level: Gitlab::VisibilityLevel::INTERNAL }
+    let(:forked_project) { create :forked_project_with_submodules, visibility_level: Gitlab::VisibilityLevel::INTERNAL }
+    let(:opts) { {} }
+
+    before do
+      forked_project.build_forked_project_link(forked_to_project_id: forked_project.id, forked_from_project_id: project.id)
+      forked_project.save
+
+      @created_internal = project.internal?
+      @fork_created_internal = forked_project.internal?
+    end
+
+    context 'should update forks visibility level when parent set to more restrictive' do
+      before do
+        opts.merge!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        update_project(project, user, opts).inspect
+      end
+
+      it { expect(@created_internal).to be_truthy }
+      it { expect(@fork_created_internal).to be_truthy }
+      it { expect(project.private?).to be_truthy }
+      it { expect(project.forks.first.private?).to be_truthy }
+    end
+
+    context 'should not update forks visibility level when parent set to less restrictive' do
+      before do
+        opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+        update_project(project, user, opts).inspect
+      end
+
+      it { expect(@created_internal).to be_truthy }
+      it { expect(@fork_created_internal).to be_truthy }
+      it { expect(project.public?).to be_truthy }
+      it { expect(project.forks.first.internal?).to be_truthy }
+    end
+  end
+
   def update_project(project, user, opts)
     Projects::UpdateService.new(project, user, opts).execute
   end