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

app/helpers/projects_helper.rb

@@ -110,17 +110,24 @@ 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

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"
 

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

changelogs/unreleased/security-28802-respect-fork-parent-visibility-ee.yml

+---
+title: Check permissions before showing a forked project's source
+merge_request:
+author:
+type: security

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|

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?"

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

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

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

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