Fixed bug when external wiki is enabled

When the external wiki is enabled, the internal wiki link is replaced
by the external wiki url. But the internal wiki is still accessible.
In this change the external wiki will have its own tab in the sidebar
and only if the services are disabled the tab (and access rights)
will not be displayed.

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 e67c327f7f8..085debcaf0e 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -313,19 +313,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 3146f26bed5..c336d5907d9 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -311,7 +311,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 e516c76400a..2321eebf1f3 100644
--- a/app/views/layouts/nav/sidebar/_project.html.haml
+++ b/app/views/layouts/nav/sidebar/_project.html.haml
@@ -275,19 +275,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 f25f9cc531a..3d376085da0 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -3013,6 +3013,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 88b5d87f087..c2dd666f9df 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -358,6 +358,26 @@ describe ProjectsHelper do
         is_expected.not_to include(:pipelines)
       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 9cb20854f6e..54d52b91d61 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 ec20c346234..9d7f1d47598 100644
--- a/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
+++ b/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
@@ -75,4 +75,58 @@ describe 'layouts/nav/sidebar/_project' do
       end
     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