Merge branch 'security-fix-wiki-access-rights-with-external-wiki-enabled-11-6' into 'security-11-6'

[11.6] Fix access to internal wiki when external wiki is enabled

See merge request gitlab/gitlabhq!2801

(cherry picked from commit 1edd23f18210a03ab3e1f6925aa4e434f68cee79)

24a48893 Fixed bug when external wiki is enabled

14 files changed, 131 insertions, 51 deletions

diff --git a/app/helpers/external_wiki_helper.rb b/app/helpers/external_wiki_helper.rb
deleted file mode 100644
index e36d63b2946..00000000000
--- a/app/helpers/external_wiki_helper.rb
+++ /dev/null
@@ -1,12 +0,0 @@
-# frozen_string_literal: true
-
-module ExternalWikiHelper
-  def get_project_wiki_path(project)
-    external_wiki_service = project.external_wiki
-    if external_wiki_service
-      external_wiki_service.properties['external_wiki_url']
-    else
-      project_wiki_path(project, :home)
-    end
-  end
-end
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 9d0fa36b16a..6afec2b6133 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -297,19 +297,24 @@ module ProjectsHelper
       nav_tabs << :operations
     end
 
-    if project.external_issue_tracker
-      nav_tabs << :external_issue_tracker
-    end
-
     tab_ability_map.each do |tab, ability|
       if can?(current_user, ability, project)
         nav_tabs << tab
       end
     end
 
+    nav_tabs << external_nav_tabs(project)
+
     nav_tabs.flatten
   end
 
+  def external_nav_tabs(project)
+    [].tap do |tabs|
+      tabs << :external_issue_tracker if project.external_issue_tracker
+      tabs << :external_wiki if project.has_external_wiki?
+    end
+  end
+
   def tab_ability_map
     {
       environments:     :read_environment,
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 30c894865fb..4f54a66680a 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -305,7 +305,7 @@ class ProjectPolicy < BasePolicy
     prevent(*create_read_update_admin_destroy(:project_snippet))
   end
 
-  rule { wiki_disabled & ~has_external_wiki }.policy do
+  rule { wiki_disabled }.policy do
     prevent(*create_read_update_admin_destroy(:wiki))
     prevent(:download_wiki_code)
   end
diff --git a/app/views/layouts/nav/sidebar/_project.html.haml b/app/views/layouts/nav/sidebar/_project.html.haml
index bdd0108db0d..48ac1f2928b 100644
--- a/app/views/layouts/nav/sidebar/_project.html.haml
+++ b/app/views/layouts/nav/sidebar/_project.html.haml
@@ -270,19 +270,34 @@
                 %strong.fly-out-top-item-name
                   = _('Registry')
 
-      - if project_nav_tab? :wiki
+      - if project_nav_tab?(:wiki)
+        - wiki_url = project_wiki_path(@project, :home)
         = nav_link(controller: :wikis) do
-          = link_to get_project_wiki_path(@project), class: 'shortcuts-wiki' do
+          = link_to wiki_url, class: 'shortcuts-wiki' do
             .nav-icon-container
               = sprite_icon('book')
             %span.nav-item-name
               = _('Wiki')
           %ul.sidebar-sub-level-items.is-fly-out-only
             = nav_link(controller: :wikis, html_options: { class: "fly-out-top-item" } ) do
-              = link_to get_project_wiki_path(@project) do
+              = link_to wiki_url do
                 %strong.fly-out-top-item-name
                   = _('Wiki')
 
+      - if project_nav_tab?(:external_wiki)
+        - external_wiki_url = @project.external_wiki.external_wiki_url
+        = nav_link do
+          = link_to external_wiki_url, class: 'shortcuts-external_wiki' do
+            .nav-icon-container
+              = sprite_icon('issue-external')
+            %span.nav-item-name
+              = _('External Wiki')
+          %ul.sidebar-sub-level-items.is-fly-out-only
+            = nav_link(html_options: { class: "fly-out-top-item" } ) do
+              = link_to external_wiki_url do
+                %strong.fly-out-top-item-name
+                  = _('External Wiki')
+
       - if project_nav_tab? :snippets
         = nav_link(controller: :snippets) do
           = link_to project_snippets_path(@project), class: 'shortcuts-snippets' do
diff --git a/app/views/projects/blob/viewers/_readme.html.haml b/app/views/projects/blob/viewers/_readme.html.haml
index d8492abc638..c2329a7aa66 100644
--- a/app/views/projects/blob/viewers/_readme.html.haml
+++ b/app/views/projects/blob/viewers/_readme.html.haml
@@ -1,4 +1,4 @@
 = icon('info-circle fw')
 = succeed '.' do
   To learn more about this project, read
-  = link_to "the wiki", get_project_wiki_path(viewer.project)
+  = link_to "the wiki", project_wiki_path(viewer.project, :home)
diff --git a/app/views/projects/wikis/pages.html.haml b/app/views/projects/wikis/pages.html.haml
index aeef64fd7eb..94267b6e0cf 100644
--- a/app/views/projects/wikis/pages.html.haml
+++ b/app/views/projects/wikis/pages.html.haml
@@ -1,5 +1,5 @@
 - @no_container = true
-- add_to_breadcrumbs "Wiki", get_project_wiki_path(@project)
+- add_to_breadcrumbs "Wiki", project_wiki_path(@project, :home)
 - breadcrumb_title s_("Wiki|Pages")
 - page_title s_("Wiki|Pages"), _("Wiki")
 
diff --git a/app/views/projects/wikis/show.html.haml b/app/views/projects/wikis/show.html.haml
index 4d5fd55364c..8b348bb4e4f 100644
--- a/app/views/projects/wikis/show.html.haml
+++ b/app/views/projects/wikis/show.html.haml
@@ -2,7 +2,7 @@
 - breadcrumb_title @page.human_title
 - wiki_breadcrumb_dropdown_links(@page.slug)
 - page_title @page.human_title, _("Wiki")
-- add_to_breadcrumbs _("Wiki"), get_project_wiki_path(@project)
+- add_to_breadcrumbs _("Wiki"), project_wiki_path(@project, :home)
 
 .wiki-page-header.has-sidebar-toggle
   %button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" }
diff --git a/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml b/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml
new file mode 100644
index 00000000000..d5f20b87a90
--- /dev/null
+++ b/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml
@@ -0,0 +1,5 @@
+---
+title: Fix wiki access rights when external wiki is enabled
+merge_request:
+author:
+type: security
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 53fdbf77da0..3cd352867b4 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -2956,6 +2956,9 @@ msgstr ""
 msgid "External URL"
 msgstr ""
 
+msgid "External Wiki"
+msgstr ""
+
 msgid "Facebook"
 msgstr ""
 
diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb
index 28bd2ff0328..fbbe6c04147 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -358,6 +358,26 @@ describe ProjectsHelper do
         end
       end
     end
+
+    context 'when project has external wiki' do
+      before do
+        allow(project).to receive(:has_external_wiki?).and_return(true)
+      end
+
+      it 'includes external wiki tab' do
+        is_expected.to include(:external_wiki)
+      end
+    end
+
+    context 'when project does not have external wiki' do
+      before do
+        allow(project).to receive(:has_external_wiki?).and_return(false)
+      end
+
+      it 'does not include external wiki tab' do
+        is_expected.not_to include(:external_wiki)
+      end
+    end
   end
 
   describe '#show_projects' do
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
index 199f49d0bf2..eee80e9bad7 100644
--- a/spec/models/ability_spec.rb
+++ b/spec/models/ability_spec.rb
@@ -298,7 +298,6 @@ describe Ability do
 
     context 'wiki named abilities' do
       it 'disables wiki abilities if the project has no wiki' do
-        expect(project).to receive(:has_external_wiki?).and_return(false)
         expect(subject).not_to be_allowed(:read_wiki)
         expect(subject).not_to be_allowed(:create_wiki)
         expect(subject).not_to be_allowed(:update_wiki)
diff --git a/spec/models/project_services/external_wiki_service_spec.rb b/spec/models/project_services/external_wiki_service_spec.rb
index 25e6ce7e804..62fd97b038b 100644
--- a/spec/models/project_services/external_wiki_service_spec.rb
+++ b/spec/models/project_services/external_wiki_service_spec.rb
@@ -1,7 +1,6 @@
 require 'spec_helper'
 
 describe ExternalWikiService do
-  include ExternalWikiHelper
   describe "Associations" do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
@@ -25,24 +24,4 @@ describe ExternalWikiService do
       it { is_expected.not_to validate_presence_of(:external_wiki_url) }
     end
   end
-
-  describe 'External wiki' do
-    let(:project) { create(:project) }
-
-    context 'when it is active' do
-      before do
-        properties = { 'external_wiki_url' => 'https://gitlab.com' }
-        @service = project.create_external_wiki_service(active: true, properties: properties)
-      end
-
-      after do
-        @service.destroy!
-      end
-
-      it 'replaces the wiki url' do
-        wiki_path = get_project_wiki_path(project)
-        expect(wiki_path).to match('https://gitlab.com')
-      end
-    end
-  end
 end
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 00aa000375c..4191e4b18d3 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -102,15 +102,27 @@ describe ProjectPolicy do
     expect(Ability).not_to be_allowed(user, :read_issue, project)
   end
 
-  context 'when the feature is disabled' do
+  context 'wiki feature' do
+    let(:permissions) { %i(read_wiki create_wiki update_wiki admin_wiki download_wiki_code) }
+
     subject { described_class.new(owner, project) }
 
-    before do
-      project.project_feature.update_attribute(:wiki_access_level, ProjectFeature::DISABLED)
-    end
+    context 'when the feature is disabled' do
+      before do
+        project.project_feature.update_attribute(:wiki_access_level, ProjectFeature::DISABLED)
+      end
 
-    it 'does not include the wiki permissions' do
-      expect_disallowed :read_wiki, :create_wiki, :update_wiki, :admin_wiki, :download_wiki_code
+      it 'does not include the wiki permissions' do
+        expect_disallowed(*permissions)
+      end
+
+      context 'when there is an external wiki' do
+        it 'does not include the wiki permissions' do
+          allow(project).to receive(:has_external_wiki?).and_return(true)
+
+          expect_disallowed(*permissions)
+        end
+      end
     end
   end
 
diff --git a/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb b/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
index 98d4456b277..7c394ad4bd3 100644
--- a/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
+++ b/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
@@ -49,4 +49,58 @@ describe 'layouts/nav/sidebar/_project' do
       expect(rendered).to have_css('.sidebar-top-level-items > li.active', text: 'Registry')
     end
   end
+
+  describe 'wiki entry tab' do
+    let(:can_read_wiki) { true }
+
+    before do
+      allow(view).to receive(:can?).with(nil, :read_wiki, project).and_return(can_read_wiki)
+    end
+
+    describe 'when wiki is enabled' do
+      it 'shows the wiki tab with the wiki internal link' do
+        render
+
+        expect(rendered).to have_link('Wiki', href: project_wiki_path(project, :home))
+      end
+    end
+
+    describe 'when wiki is disabled' do
+      let(:can_read_wiki) { false }
+
+      it 'does not show the wiki tab' do
+        render
+
+        expect(rendered).not_to have_link('Wiki', href: project_wiki_path(project, :home))
+      end
+    end
+  end
+
+  describe 'external wiki entry tab' do
+    let(:properties) { { 'external_wiki_url' => 'https://gitlab.com' } }
+    let(:service_status) { true }
+
+    before do
+      project.create_external_wiki_service(active: service_status, properties: properties)
+      project.reload
+    end
+
+    context 'when it is active' do
+      it 'shows the external wiki tab with the external wiki service link' do
+        render
+
+        expect(rendered).to have_link('External Wiki', href: properties['external_wiki_url'])
+      end
+    end
+
+    context 'when it is disabled' do
+      let(:service_status) { false }
+
+      it 'does not show the external wiki tab' do
+        render
+
+        expect(rendered).not_to have_link('External Wiki', href: project_wiki_path(project, :home))
+      end
+    end
+  end
 end