From d2846293d067cdf38d2816768e059d901a960a22 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 3 Nov 2015 16:37:39 +0100 Subject: [PATCH 1/9] Move visibility_level check for forked projects to Project model --- app/helpers/visibility_level_helper.rb | 3 +-- app/models/project.rb | 5 +++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index b52cd23aba2..bc594dc53c1 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -89,7 +89,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 diff --git a/app/models/project.rb b/app/models/project.rb index 74b89aad499..a4c634bdb5c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -783,4 +783,9 @@ class Project < ActiveRecord::Base service.active = true service.save end + + def visibility_level_allowed?(level) + return true unless forked? + Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level) + end end From 9e1db139eb1387fa9658ed68592d93eca61efb6b Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 3 Nov 2015 17:23:19 +0100 Subject: [PATCH 2/9] Move level_name resolving to Gitlan::VisibilityLevel --- app/services/base_service.rb | 5 +---- lib/gitlab/visibility_level.rb | 9 +++++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/services/base_service.rb b/app/services/base_service.rb index f00ec7408b6..b48ca67d4d2 100644 --- a/app/services/base_service.rb +++ b/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, diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index 335dc44be19..d0ffe24f827 100644 --- a/lib/gitlab/visibility_level.rb +++ b/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 + end + + level_name + end end def private? From 7cb442eed4f488e378b3f20008ebe6ed3b53d31d Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 3 Nov 2015 18:23:48 +0100 Subject: [PATCH 3/9] Fix Project update service When project is updated and it is a fork, then visibility_level should not be less restrictive than in its parent project. --- app/models/project.rb | 2 +- app/services/projects/update_service.rb | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index a4c634bdb5c..7f2dd37a3cc 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -786,6 +786,6 @@ class Project < ActiveRecord::Base def visibility_level_allowed?(level) return true unless forked? - Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level) + Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level.to_i) end end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 69bdd045ddf..0a42f3e02aa 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -11,6 +11,15 @@ module Projects end end + unless project.visibility_level_allowed?(new_visibility) + level_name = Gitlab::VisibilityLevel.level_name(new_visibility) + project.errors.add( + :visibility_level, + "#{level_name} could not be set as visibility level of this project - parent project settings are more restrictive" + ) + return false + end + new_branch = params[:default_branch] if project.repository.exists? && new_branch && new_branch != project.default_branch From 3bc012db77e1b59986362d8de0660b97a15c9d1f Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 4 Nov 2015 22:13:40 +0100 Subject: [PATCH 4/9] Fix GitlabV::isibilityLevel::level_name method --- lib/gitlab/visibility_level.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index d0ffe24f827..3160a3c7582 100644 --- a/lib/gitlab/visibility_level.rb +++ b/lib/gitlab/visibility_level.rb @@ -55,7 +55,7 @@ module Gitlab def level_name(level) level_name = 'Unknown' options.each do |name, lvl| - level_name = name if lvl == level + level_name = name if lvl == level.to_i end level_name From 89ecba5e6cec6632c2f8e3baef6604b1b8ea0d45 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 4 Nov 2015 23:40:43 +0100 Subject: [PATCH 5/9] Update forks visibility_level after parent project visibility_level change --- app/models/project.rb | 6 +++- app/services/projects/update_service.rb | 48 ++++++++++++++++++------- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 7f2dd37a3cc..f287e59b6df 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -731,7 +731,11 @@ class Project < ActiveRecord::Base end def forks_count - ForkedProjectLink.where(forked_from_project_id: self.id).count + forks.count + end + + def forks + ForkedProjectLink.where(forked_from_project_id: self.id) end def find_label(name) diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 0a42f3e02aa..0d2665f298d 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -3,21 +3,17 @@ 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 - end - unless project.visibility_level_allowed?(new_visibility) - level_name = Gitlab::VisibilityLevel.level_name(new_visibility) - project.errors.add( - :visibility_level, - "#{level_name} could not be set as visibility level of this project - parent project settings are more restrictive" - ) - return false + return false unless visibility_level_allowed?(new_visibility) + update_forks_visibility_level(new_visibility) end new_branch = params[:default_branch] @@ -32,5 +28,31 @@ 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 + + def update_forks_visibility_level(new_level) + project.forks.each do |forked_link| + forked_project = forked_link.forked_to_project + fork_level = forked_project.visibility_level + + if fork_level > new_level.to_i + forked_project.visibility_level = new_level.to_i + forked_project.save! + end + end + end end end From 6f41e3d9caa28b2a654fb775202be632673f2299 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 18 Nov 2015 22:11:15 +0100 Subject: [PATCH 6/9] Change forks method to has_many relation --- app/models/project.rb | 11 +++++------ app/services/projects/update_service.rb | 3 +-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index f287e59b6df..2562c441abe 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -97,9 +97,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 @@ -734,10 +737,6 @@ class Project < ActiveRecord::Base forks.count end - def forks - ForkedProjectLink.where(forked_from_project_id: self.id) - end - def find_label(name) labels.find_by(name: name) end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 0d2665f298d..67e3429c0bd 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -44,8 +44,7 @@ module Projects end def update_forks_visibility_level(new_level) - project.forks.each do |forked_link| - forked_project = forked_link.forked_to_project + project.forks.each do |forked_project| fork_level = forked_project.visibility_level if fork_level > new_level.to_i From 945e4293cb8f745b2e1a1aebdcf4df3eeba338cc Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 19 Nov 2015 23:40:29 +0100 Subject: [PATCH 7/9] Prevent unnecessary forks iteration at parent update --- app/services/projects/update_service.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 67e3429c0bd..cd7a8b18218 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -13,7 +13,7 @@ module Projects end return false unless visibility_level_allowed?(new_visibility) - update_forks_visibility_level(new_visibility) + update_forks_visibility_level(new_visibility.to_i) end new_branch = params[:default_branch] @@ -44,11 +44,13 @@ module Projects end def update_forks_visibility_level(new_level) + return unless new_level < project.visibility_level + project.forks.each do |forked_project| fork_level = forked_project.visibility_level - if fork_level > new_level.to_i - forked_project.visibility_level = new_level.to_i + if fork_level > new_level + forked_project.visibility_level = new_level forked_project.save! end end From 1144b70ab624ee1c1e7f2de0c92de021a7b5ea8e Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Fri, 20 Nov 2015 00:13:56 +0100 Subject: [PATCH 8/9] Change update_forks_visibility_level into after_update hook in Project model --- app/models/project.rb | 13 +++++++++++++ app/services/projects/update_service.rb | 14 -------------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 2562c441abe..b8495c6cd4f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -61,6 +61,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 diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index cd7a8b18218..895e089bea3 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -13,7 +13,6 @@ module Projects end return false unless visibility_level_allowed?(new_visibility) - update_forks_visibility_level(new_visibility.to_i) end new_branch = params[:default_branch] @@ -42,18 +41,5 @@ module Projects false end - - def update_forks_visibility_level(new_level) - return unless new_level < project.visibility_level - - project.forks.each do |forked_project| - fork_level = forked_project.visibility_level - - if fork_level > new_level - forked_project.visibility_level = new_level - forked_project.save! - end - end - end end end From c301ab0404229b0f3b0ff2cdd0482f1e0c0196ea Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 24 Dec 2015 10:33:26 +0100 Subject: [PATCH 9/9] Add some specs for forked project visibility_level cases --- spec/models/project_spec.rb | 24 ++++++++++++ spec/services/projects/update_service_spec.rb | 39 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c4d3813e9c9..400bdf2d962 100644 --- a/spec/models/project_spec.rb +++ b/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 diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index c36d4581989..3c06a890163 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/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