Merge branch 'security-28802-respect-fork-parent-visibility' into 'master'

Check permissions before showing a forked project's source

See merge request gitlab/gitlabhq!3554

10 files changed, 164 insertions, 27 deletions

diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 64d61bba71a..466c782fc77 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -110,19 +110,26 @@ module ProjectsHelper
       { project_full_name: project.full_name }
   end
 
-  def remove_fork_project_message(project)
-    _("You are going to remove the fork relationship to source project %{forked_from_project}. Are you ABSOLUTELY sure?") %
-      { forked_from_project: fork_source_name(project) }
-  end
+  def remove_fork_project_description_message(project)
+    source = visible_fork_source(project)
 
-  def fork_source_name(project)
-    if @project.fork_source
-      @project.fork_source.full_name
+    if source
+      _('This will remove the fork relationship between this project and %{fork_source}.') %
+        { fork_source: link_to(source.full_name, project_path(source)) }
     else
-      @project.fork_network&.deleted_root_project_name
+      _('This will remove the fork relationship between this project and other projects in the fork network.')
     end
   end
 
+  def remove_fork_project_warning_message(project)
+    _("You are going to remove the fork relationship from %{project_full_name}. Are you ABSOLUTELY sure?") %
+      { project_full_name: project.full_name }
+  end
+
+  def visible_fork_source(project)
+    project.fork_source if project.fork_source && can?(current_user, :read_project, project.fork_source)
+  end
+
   def project_nav_tabs
     @nav_tabs ||= get_project_nav_tabs(@project, current_user)
   end
diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml
index b7c4114d485..daedc52f298 100644
--- a/app/views/projects/_home_panel.html.haml
+++ b/app/views/projects/_home_panel.html.haml
@@ -74,13 +74,12 @@
 
     - if @project.forked?
       %p
-        - if @project.fork_source
+        - source = visible_fork_source(@project)
+        - if source
           #{ s_('ForkedFromProjectPath|Forked from') }
-          = link_to project_path(@project.fork_source) do
-            = fork_source_name(@project)
+          = link_to source.full_name, project_path(source)
         - else
-          - deleted_message = s_('ForkedFromProjectPath|Forked from %{project_name} (deleted)')
-          = deleted_message % { project_name: fork_source_name(@project) }
+          = s_('ForkedFromProjectPath|Forked from an inaccessible project')
 
     = render_if_exists "projects/home_mirror"
 
diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml
index b5e24cbbffb..328fdd0be10 100644
--- a/app/views/projects/edit.html.haml
+++ b/app/views/projects/edit.html.haml
@@ -126,17 +126,12 @@
     - if @project.forked? && can?(current_user, :remove_fork_project, @project)
       .sub-section
         %h4.danger-title= _('Remove fork relationship')
-        %p
-          = _('This will remove the fork relationship to source project')
-          = succeed "." do
-            - if @project.fork_source
-              = link_to(fork_source_name(@project), project_path(@project.fork_source))
-            - else
-              = fork_source_name(@project)
+        %p= remove_fork_project_description_message(@project)
+
         = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_project_path(@project), method: :delete, remote: true, html: { class: 'transfer-project' }) do |f|
           %p
             %strong= _('Once removed, the fork relationship cannot be restored and you will no longer be able to send merge requests to the source.')
-          = button_to _('Remove fork relationship'), '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) }
+          = button_to _('Remove fork relationship'), '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_warning_message(@project) }
 
     - if can?(current_user, :remove_project, @project)
       .sub-section
diff --git a/changelogs/unreleased/security-28802-respect-fork-parent-visibility-ee.yml b/changelogs/unreleased/security-28802-respect-fork-parent-visibility-ee.yml
new file mode 100644
index 00000000000..8872b73a0cc
--- /dev/null
+++ b/changelogs/unreleased/security-28802-respect-fork-parent-visibility-ee.yml
@@ -0,0 +1,5 @@
+---
+title: Check permissions before showing a forked project's source
+merge_request:
+author:
+type: security
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 21648b0f59a..cba4ec2c18f 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -283,7 +283,9 @@ module API
       expose :shared_runners_enabled
       expose :lfs_enabled?, as: :lfs_enabled
       expose :creator_id
-      expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? }
+      expose :forked_from_project, using: Entities::BasicProjectDetails, if: ->(project, options) do
+        project.forked? && Ability.allowed?(options[:current_user], :read_project, project.forked_from_project)
+      end
       expose :import_status
 
       expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] } do |project|
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 799612af9cd..fa8f36220f1 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -7699,7 +7699,7 @@ msgstr ""
 msgid "ForkedFromProjectPath|Forked from"
 msgstr ""
 
-msgid "ForkedFromProjectPath|Forked from %{project_name} (deleted)"
+msgid "ForkedFromProjectPath|Forked from an inaccessible project"
 msgstr ""
 
 msgid "Forking in progress"
@@ -17909,7 +17909,10 @@ msgstr ""
 msgid "This will redirect you to an external sign in page."
 msgstr ""
 
-msgid "This will remove the fork relationship to source project"
+msgid "This will remove the fork relationship between this project and %{fork_source}."
+msgstr ""
+
+msgid "This will remove the fork relationship between this project and other projects in the fork network."
 msgstr ""
 
 msgid "Those emails automatically become issues (with the comments becoming the email conversation) listed here."
@@ -19825,7 +19828,7 @@ msgstr ""
 msgid "You are going to remove %{project_full_name}. Removed project CANNOT be restored! Are you ABSOLUTELY sure?"
 msgstr ""
 
-msgid "You are going to remove the fork relationship to source project %{forked_from_project}. Are you ABSOLUTELY sure?"
+msgid "You are going to remove the fork relationship from %{project_full_name}. Are you ABSOLUTELY sure?"
 msgstr ""
 
 msgid "You are going to transfer %{project_full_name} to another owner. Are you ABSOLUTELY sure?"
diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb
index 90e48f3c230..47f32e0113c 100644
--- a/spec/features/projects_spec.rb
+++ b/spec/features/projects_spec.rb
@@ -202,13 +202,13 @@ describe 'Project' do
       expect(page).not_to have_content('Forked from')
     end
 
-    it 'shows the name of the deleted project when the source was deleted', :sidekiq_might_not_need_inline do
+    it 'does not show the name of the deleted project when the source was deleted', :sidekiq_might_not_need_inline do
       forked_project
       Projects::DestroyService.new(base_project, base_project.owner).execute
 
       visit project_path(forked_project)
 
-      expect(page).to have_content("Forked from #{base_project.full_name} (deleted)")
+      expect(page).to have_content('Forked from an inaccessible project')
     end
 
     context 'a fork of a fork' do
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index f1447536e0f..cda2dd7d2f4 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -49,6 +49,8 @@ shared_examples 'languages and percentages JSON response' do
 end
 
 describe API::Projects do
+  include ProjectForksHelper
+
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
@@ -1163,6 +1165,18 @@ describe API::Projects do
         expect(json_response.keys).not_to include('permissions')
       end
 
+      context 'the project is a public fork' do
+        it 'hides details of a public fork parent' do
+          public_project = create(:project, :repository, :public)
+          fork = fork_project(public_project)
+
+          get api("/projects/#{fork.id}")
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['forked_from_project']).to be_nil
+        end
+      end
+
       context 'and the project has a private repository' do
         let(:project) { create(:project, :repository, :public, :repository_private) }
         let(:protected_attributes) { %w(default_branch ci_config_path) }
@@ -1479,6 +1493,28 @@ describe API::Projects do
         end
       end
 
+      context 'the project is a fork' do
+        it 'shows details of a visible fork parent' do
+          fork = fork_project(project, user)
+
+          get api("/projects/#{fork.id}", user)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['forked_from_project']).to include('id' => project.id)
+        end
+
+        it 'hides details of a hidden fork parent' do
+          fork = fork_project(project, user)
+          fork_user = create(:user)
+          fork.team.add_developer(fork_user)
+
+          get api("/projects/#{fork.id}", fork_user)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['forked_from_project']).to be_nil
+        end
+      end
+
       describe 'permissions' do
         context 'all projects' do
           before do
diff --git a/spec/views/projects/_home_panel.html.haml_spec.rb b/spec/views/projects/_home_panel.html.haml_spec.rb
index 4d5b369e88e..9956144b601 100644
--- a/spec/views/projects/_home_panel.html.haml_spec.rb
+++ b/spec/views/projects/_home_panel.html.haml_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe 'projects/_home_panel' do
+  include ProjectForksHelper
+
   context 'notifications' do
     let(:project) { create(:project) }
 
@@ -144,4 +146,36 @@ describe 'projects/_home_panel' do
       end
     end
   end
+
+  context 'forks' do
+    let(:source_project) { create(:project, :repository) }
+    let(:project) { fork_project(source_project) }
+    let(:user) { create(:user) }
+
+    before do
+      assign(:project, project)
+
+      allow(view).to receive(:current_user).and_return(user)
+    end
+
+    context 'user can read fork source' do
+      it 'shows the forked-from project' do
+        allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(true)
+
+        render
+
+        expect(rendered).to have_content("Forked from #{source_project.full_name}")
+      end
+    end
+
+    context 'user cannot read fork source' do
+      it 'does not show the forked-from project' do
+        allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(false)
+
+        render
+
+        expect(rendered).to have_content("Forked from an inaccessible project")
+      end
+    end
+  end
 end
diff --git a/spec/views/projects/edit.html.haml_spec.rb b/spec/views/projects/edit.html.haml_spec.rb
index f576093ab45..40927a22dc4 100644
--- a/spec/views/projects/edit.html.haml_spec.rb
+++ b/spec/views/projects/edit.html.haml_spec.rb
@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe 'projects/edit' do
   include Devise::Test::ControllerHelpers
+  include ProjectForksHelper
 
   let(:project) { create(:project) }
   let(:user) { create(:admin) }
@@ -26,4 +27,59 @@ describe 'projects/edit' do
       expect(rendered).not_to have_content('Export project')
     end
   end
+
+  context 'forking' do
+    before do
+      assign(:project, project)
+
+      allow(view).to receive(:current_user).and_return(user)
+    end
+
+    context 'project is not a fork' do
+      it 'hides the remove fork relationship settings' do
+        render
+
+        expect(rendered).not_to have_content('Remove fork relationship')
+      end
+    end
+
+    context 'project is a fork' do
+      let(:source_project) { create(:project) }
+      let(:project) { fork_project(source_project) }
+
+      it 'shows the remove fork relationship settings to an authorized user' do
+        allow(view).to receive(:can?).with(user, :remove_fork_project, project).and_return(true)
+
+        render
+
+        expect(rendered).to have_content('Remove fork relationship')
+      end
+
+      it 'hides the fork relationship settings from an unauthorized user' do
+        allow(view).to receive(:can?).with(user, :remove_fork_project, project).and_return(false)
+
+        render
+
+        expect(rendered).not_to have_content('Remove fork relationship')
+      end
+
+      it 'hides the fork source from an unauthorized user' do
+        allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(false)
+
+        render
+
+        expect(rendered).to have_content('Remove fork relationship')
+        expect(rendered).not_to have_content(source_project.full_name)
+      end
+
+      it 'shows the fork source to an authorized user' do
+        allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(true)
+
+        render
+
+        expect(rendered).to have_content('Remove fork relationship')
+        expect(rendered).to have_content(source_project.full_name)
+      end
+    end
+  end
 end