Merge branch '207869-group-wikis-git-support' into 'master'

Support Git access for group wikis

See merge request gitlab-org/gitlab!45892

app/controllers/concerns/wiki_actions.rb

@@ -186,6 +186,10 @@ module WikiActions
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
+  def git_access
+    render 'shared/wikis/git_access'
+  end
+
   private
 
   def container

app/controllers/projects/wikis_controller.rb

@@ -6,7 +6,4 @@ class Projects::WikisController < Projects::ApplicationController
   alias_method :container, :project
 
   feature_category :wiki
-
-  def git_access
-  end
 end

app/models/concerns/case_sensitivity.rb

@@ -11,12 +11,14 @@ module CaseSensitivity
     def iwhere(params)
       criteria = self
 
-      params.each do |key, value|
+      params.each do |column, value|
+        column = arel_table[column] unless column.is_a?(Arel::Attribute)
+
         criteria = case value
                    when Array
-                     criteria.where(value_in(key, value))
+                     criteria.where(value_in(column, value))
                    else
-                     criteria.where(value_equal(key, value))
+                     criteria.where(value_equal(column, value))
                    end
       end
 
@@ -28,7 +30,7 @@ module CaseSensitivity
     def value_equal(column, value)
       lower_value = lower_value(value)
 
-      lower_column(arel_table[column]).eq(lower_value).to_sql
+      lower_column(column).eq(lower_value).to_sql
     end
 
     def value_in(column, values)
@@ -36,7 +38,7 @@ module CaseSensitivity
         lower_value(value)
       end
 
-      lower_column(arel_table[column]).in(lower_values).to_sql
+      lower_column(column).in(lower_values).to_sql
     end
 
     def lower_value(value)

app/models/concerns/routable.rb

@@ -4,6 +4,36 @@
 # Object must have name and path db fields and respond to parent and parent_changed? methods.
 module Routable
   extend ActiveSupport::Concern
+  include CaseSensitivity
+
+  # Finds a Routable object by its full path, without knowing the class.
+  #
+  # Usage:
+  #
+  #     Routable.find_by_full_path('groupname')             # -> Group
+  #     Routable.find_by_full_path('groupname/projectname') # -> Project
+  #
+  # Returns a single object, or nil.
+  def self.find_by_full_path(path, follow_redirects: false, route_scope: Route, redirect_route_scope: RedirectRoute)
+    return unless path.present?
+
+    # Case sensitive match first (it's cheaper and the usual case)
+    # If we didn't have an exact match, we perform a case insensitive search
+    #
+    # We need to qualify the columns with the table name, to support both direct lookups on
+    # Route/RedirectRoute, and scoped lookups through the Routable classes.
+    route =
+      route_scope.find_by(routes: { path: path }) ||
+      route_scope.iwhere(Route.arel_table[:path] => path).take
+
+    if follow_redirects
+      route ||= redirect_route_scope.iwhere(RedirectRoute.arel_table[:path] => path).take
+    end
+
+    return unless route
+
+    route.is_a?(Routable) ? route : route.source
+  end
 
   included do
     # Remove `inverse_of: source` when upgraded to rails 5.2
@@ -30,15 +60,14 @@ module Routable
     #
     # Returns a single object, or nil.
     def find_by_full_path(path, follow_redirects: false)
-      # Case sensitive match first (it's cheaper and the usual case)
-      # If we didn't have an exact match, we perform a case insensitive search
-      found = includes(:route).find_by(routes: { path: path }) || where_full_path_in([path]).take
-
-      return found if found
-
-      if follow_redirects
-        joins(:redirect_routes).find_by("LOWER(redirect_routes.path) = LOWER(?)", path)
-      end
+      # TODO: Optimize these queries by avoiding joins
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/292252
+      Routable.find_by_full_path(
+        path,
+        follow_redirects: follow_redirects,
+        route_scope: includes(:route).references(:routes),
+        redirect_route_scope: joins(:redirect_routes)
+      )
     end
 
     # Builds a relation to find multiple objects by their full paths.

app/models/redirect_route.rb

 # frozen_string_literal: true
 
 class RedirectRoute < ApplicationRecord
+  include CaseSensitivity
+
   belongs_to :source, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
 
   validates :source, presence: true

app/models/route.rb

@@ -4,7 +4,7 @@ class Route < ApplicationRecord
   include CaseSensitivity
   include Gitlab::SQL::Pattern
 
-  belongs_to :source, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
+  belongs_to :source, polymorphic: true, inverse_of: :route # rubocop:disable Cop/PolymorphicAssociations
   validates :source, presence: true
 
   validates :path,

app/views/shared/wikis/_sidebar.html.haml

@@ -4,11 +4,10 @@
       %a.gutter-toggle.float-right.d-block.d-md-none.js-sidebar-wiki-toggle{ href: "#" }
         = sprite_icon('chevron-double-lg-right', css_class: 'gl-icon')
 
-      - if @wiki.container.is_a?(Project)
-        - git_access_url = wiki_path(@wiki, action: :git_access)
-        = link_to git_access_url, class: active_nav_link?(path: 'wikis#git_access') ? 'active' : '', data: { qa_selector: 'clone_repository_link' } do
-          = sprite_icon('download', css_class: 'gl-mr-2')
-          %span= _("Clone repository")
+      - git_access_url = wiki_path(@wiki, action: :git_access)
+      = link_to git_access_url, class: active_nav_link?(path: 'wikis#git_access') ? 'active' : '', data: { qa_selector: 'clone_repository_link' } do
+        = sprite_icon('download', css_class: 'gl-mr-2')
+        %span= _("Clone repository")
 
     - if @sidebar_error.present?
       = render 'shared/alert_info', body: s_('Wiki|The sidebar failed to load. You can reload the page to try again.')

changelogs/unreleased/207869-group-wikis-git-support.yml

+---
+title: Support Git access for group wikis
+merge_request: 45892
+author:
+type: added

config/routes/git_http.rb

@@ -9,7 +9,7 @@ scope(path: '*repository_path', format: false) do
       end
 
       # NOTE: LFS routes are exposed on all repository types, but we still check for
-      # LFS availability on the repository container in LfsRequest#require_lfs_enabled!
+      # LFS availability on the repository container in LfsRequest#lfs_check_access!
 
       # Git LFS API (metadata)
       scope(path: 'info/lfs/objects', controller: :lfs_api) do

doc/user/group/index.md

@@ -404,7 +404,7 @@ and above.
 
 There are a few limitations compared to project wikis:
 
-- Local Git access is not supported yet.
+- Git LFS is not supported.
 - Group wikis are not included in global search, group exports, backups, and Geo replication.
 - Changes to group wikis don't show up in the group's activity feed.
 - Group wikis [can't be moved](../../api/project_repository_storage_moves.md#limitations) using the project

ee/app/assets/javascripts/pages/groups/wikis/index.js

 import initWikis from '~/pages/shared/wikis';
+import initClonePanel from '~/clone_panel';
 
 initWikis();
+initClonePanel();

ee/spec/controllers/repositories/git_http_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Repositories::GitHttpController do
+  context 'when repository container is a group wiki' do
+    include WikiHelpers
+
+    let_it_be(:group) { create(:group, :wiki_repo) }
+    let_it_be(:user) { create(:user) }
+
+    before_all do
+      group.add_owner(user)
+    end
+
+    before do
+      stub_group_wikis(true)
+    end
+
+    it_behaves_like Repositories::GitHttpController do
+      let(:container) { group.wiki }
+      let(:access_checker_class) { Gitlab::GitAccessWiki }
+    end
+  end
+end

ee/spec/features/groups/wikis_spec.rb

@@ -23,4 +23,5 @@ RSpec.describe 'Group wikis' do
   it_behaves_like 'User views a wiki page'
   it_behaves_like 'User views wiki pages'
   it_behaves_like 'User views wiki sidebar'
+  it_behaves_like 'User views Git access wiki page'
 end

ee/spec/requests/lfs_http_spec.rb

@@ -21,6 +21,27 @@ RSpec.describe 'Git LFS API and storage' do
   let(:sample_oid) { lfs_object.oid }
   let(:sample_size) { lfs_object.size }
 
+  context 'with group wikis' do
+    let_it_be(:group) { create(:group) }
+
+    # LFS is not supported on group wikis, so we override the shared examples
+    # to expect 404 responses instead.
+    [
+      'LFS http 200 response',
+      'LFS http 200 blob response',
+      'LFS http 403 response'
+    ].each do |examples|
+      shared_examples_for(examples) { it_behaves_like 'LFS http 404 response' }
+    end
+
+    it_behaves_like 'LFS http requests' do
+      let(:container) { create(:group_wiki, :empty_repo, group: group) }
+      let(:authorize_guest) { group.add_guest(user) }
+      let(:authorize_download) { group.add_reporter(user) }
+      let(:authorize_upload) { group.add_developer(user) }
+    end
+  end
+
   describe 'when handling lfs batch request' do
     let(:update_lfs_permissions) { }
     let(:update_user_permissions) { }

ee/spec/requests/repositories/git_http_controller_spec.rb

@@ -131,6 +131,24 @@ RSpec.describe Repositories::GitHttpController, type: :request do
         it_behaves_like 'triggers Geo'
       end
 
+      context 'with a group wiki' do
+        include WikiHelpers
+
+        let_it_be(:group) { create(:group, :wiki_repo) }
+        let_it_be(:user) { create(:user) }
+        let(:path) { "#{group.wiki.full_path}.git" }
+
+        before_all do
+          group.add_owner(user)
+        end
+
+        before do
+          stub_group_wikis(true)
+        end
+
+        it_behaves_like 'triggers Geo'
+      end
+
       context 'with a personal snippet' do
         let_it_be(:snippet) { create(:personal_snippet, :repository, author: user) }
         let_it_be(:path) { "snippets/#{snippet.id}.git" }

lib/gitlab/gl_repository.rb

@@ -20,6 +20,7 @@ module Gitlab
       end,
       container_class: ProjectWiki,
       project_resolver: -> (wiki) { wiki.try(:project) },
+      guest_read_ability: :download_wiki_code,
       suffix: :wiki
     ).freeze
     SNIPPET = RepoType.new(

lib/gitlab/repo_path.rb

@@ -4,6 +4,13 @@ module Gitlab
   module RepoPath
     NotFoundError = Class.new(StandardError)
 
+    # Returns an array containing:
+    # - The repository container
+    # - The related project (if available)
+    # - The repository type
+    # - The original container path (if redirected)
+    #
+    # @returns [HasRepository, Project, String, String]
     def self.parse(path)
       repo_path = path.delete_prefix('/').delete_suffix('.git')
       redirected_path = nil
@@ -30,7 +37,15 @@ module Gitlab
       [nil, nil, Gitlab::GlRepository.default_type, nil]
     end
 
+    # Returns an array containing:
+    # - The repository container
+    # - The related project (if available)
+    # - The original container path (if redirected)
+    #
+    # @returns [HasRepository, Project, String]
     def self.find_container(type, full_path)
+      return [nil, nil, nil] if full_path.blank?
+
       if type.snippet?
         snippet, redirected_path = find_snippet(full_path)
 
@@ -47,26 +62,24 @@ module Gitlab
     end
 
     def self.find_project(project_path)
-      return [nil, nil] if project_path.blank?
-
       project = Project.find_by_full_path(project_path, follow_redirects: true)
-      redirected_path = redirected?(project, project_path) ? project_path : nil
+      redirected_path = project_path if redirected?(project, project_path)
 
       [project, redirected_path]
     end
 
-    def self.redirected?(project, project_path)
-      project && project.full_path.casecmp(project_path) != 0
+    def self.redirected?(container, container_path)
+      container && container.full_path.casecmp(container_path) != 0
     end
 
     # Snippet_path can be either:
     # - snippets/1
     # - h5bp/html5-boilerplate/snippets/53
     def self.find_snippet(snippet_path)
-      return [nil, nil] if snippet_path.blank?
-
       snippet_id, project_path = extract_snippet_info(snippet_path)
-      project, redirected_path = find_project(project_path)
+      return [nil, nil] unless snippet_id
+
+      project, redirected_path = find_project(project_path) if project_path
 
       [Snippet.find_by_id_and_project(id: snippet_id, project: project), redirected_path]
     end
@@ -74,19 +87,23 @@ module Gitlab
     # Wiki path can be either:
     # - namespace/project
     # - group/subgroup/project
-    def self.find_wiki(wiki_path)
-      return [nil, nil] if wiki_path.blank?
-
-      project, redirected_path = find_project(wiki_path)
-
-      [project&.wiki, redirected_path]
+    #
+    # And also in EE:
+    # - group
+    # - group/subgroup
+    def self.find_wiki(container_path)
+      container = Routable.find_by_full_path(container_path, follow_redirects: true)
+      redirected_path = container_path if redirected?(container, container_path)
+
+      # In CE, Group#wiki is not available so this will return nil for a group path.
+      [container&.try(:wiki), redirected_path]
     end
 
     def self.extract_snippet_info(snippet_path)
       path_segments = snippet_path.split('/')
       snippet_id = path_segments.pop
-      path_segments.pop # Remove snippets from path
-      project_path = File.join(path_segments)
+      path_segments.pop # Remove 'snippets' from path
+      project_path = File.join(path_segments).presence
 
       [snippet_id, project_path]
     end

spec/controllers/repositories/git_http_controller_spec.rb

@@ -3,8 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Repositories::GitHttpController do
-  include GitHttpHelpers
-
   let_it_be(:project) { create(:project, :public, :repository) }
   let_it_be(:personal_snippet) { create(:personal_snippet, :public, :repository) }
   let_it_be(:project_snippet) { create(:project_snippet, :public, :repository, project: project) }

spec/models/concerns/case_sensitivity_spec.rb

@@ -4,16 +4,16 @@ require 'spec_helper'
 
 RSpec.describe CaseSensitivity do
   describe '.iwhere' do
-    let(:connection) { ActiveRecord::Base.connection }
-    let(:model) do
+    let_it_be(:connection) { ActiveRecord::Base.connection }
+    let_it_be(:model) do
       Class.new(ActiveRecord::Base) do
         include CaseSensitivity
         self.table_name = 'namespaces'
       end
     end
 
-    let!(:model_1) { model.create!(path: 'mOdEl-1', name: 'mOdEl 1') }
-    let!(:model_2) { model.create!(path: 'mOdEl-2', name: 'mOdEl 2') }
+    let_it_be(:model_1) { model.create!(path: 'mOdEl-1', name: 'mOdEl 1') }
+    let_it_be(:model_2) { model.create!(path: 'mOdEl-2', name: 'mOdEl 2') }
 
     it 'finds a single instance by a single attribute regardless of case' do
       expect(model.iwhere(path: 'MODEL-1')).to contain_exactly(model_1)
@@ -28,6 +28,10 @@ RSpec.describe CaseSensitivity do
         .to contain_exactly(model_1)
     end
 
+    it 'finds instances by custom Arel attributes' do
+      expect(model.iwhere(model.arel_table[:path] => 'MODEL-1')).to contain_exactly(model_1)
+    end
+
     it 'builds a query using LOWER' do
       query = model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1').to_sql
       expected_query = <<~QRY.strip

spec/models/concerns/routable_spec.rb

@@ -2,8 +2,69 @@
 
 require 'spec_helper'
 
+RSpec.shared_examples '.find_by_full_path' do
+  describe '.find_by_full_path', :aggregate_failures do
+    it 'finds records by their full path' do
+      expect(described_class.find_by_full_path(record.full_path)).to eq(record)
+      expect(described_class.find_by_full_path(record.full_path.upcase)).to eq(record)
+    end
+
+    it 'returns nil for unknown paths' do
+      expect(described_class.find_by_full_path('unknown')).to be_nil
+    end
+
+    it 'includes route information when loading a record' do
+      control_count = ActiveRecord::QueryRecorder.new do
+        described_class.find_by_full_path(record.full_path)
+      end.count
+
+      expect do
+        described_class.find_by_full_path(record.full_path).route
+      end.not_to exceed_all_query_limit(control_count)
+    end
+
+    context 'with redirect routes' do
+      let_it_be(:redirect_route) { create(:redirect_route, source: record) }
+
+      context 'without follow_redirects option' do
+        it 'does not find records by their redirected path' do
+          expect(described_class.find_by_full_path(redirect_route.path)).to be_nil
+          expect(described_class.find_by_full_path(redirect_route.path.upcase)).to be_nil
+        end
+      end
+
+      context 'with follow_redirects option set to true' do
+        it 'finds records by their canonical path' do
+          expect(described_class.find_by_full_path(record.full_path, follow_redirects: true)).to eq(record)
+          expect(described_class.find_by_full_path(record.full_path.upcase, follow_redirects: true)).to eq(record)
+        end
+
+        it 'finds records by their redirected path' do
+          expect(described_class.find_by_full_path(redirect_route.path, follow_redirects: true)).to eq(record)
+          expect(described_class.find_by_full_path(redirect_route.path.upcase, follow_redirects: true)).to eq(record)
+        end
+
+        it 'returns nil for unknown paths' do
+          expect(described_class.find_by_full_path('unknown', follow_redirects: true)).to be_nil
+        end
+      end
+    end
+  end
+end
+
+RSpec.describe Routable do
+  it_behaves_like '.find_by_full_path' do
+    let_it_be(:record) { create(:group) }
+  end
+
+  it_behaves_like '.find_by_full_path' do
+    let_it_be(:record) { create(:project) }
+  end
+end
+
 RSpec.describe Group, 'Routable' do
-  let!(:group) { create(:group, name: 'foo') }
+  let_it_be_with_reload(:group) { create(:group, name: 'foo') }
+  let_it_be(:nested_group) { create(:group, parent: group) }
 
   describe 'Validations' do
     it { is_expected.to validate_presence_of(:route) }
@@ -59,61 +120,20 @@ RSpec.describe Group, 'Routable' do
   end
 
   describe '.find_by_full_path' do
-    let!(:nested_group) { create(:group, parent: group) }
-
-    context 'without any redirect routes' do
-      it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) }
-      it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) }
-      it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) }
-      it { expect(described_class.find_by_full_path('unknown')).to eq(nil) }
-
-      it 'includes route information when loading a record' do
-        path = group.to_param
-        control_count = ActiveRecord::QueryRecorder.new { described_class.find_by_full_path(path) }.count
-
-        expect { described_class.find_by_full_path(path).route }.not_to exceed_all_query_limit(control_count)
-      end
+    it_behaves_like '.find_by_full_path' do
+      let_it_be(:record) { group }
     end
 
-    context 'with redirect routes' do
-      let!(:group_redirect_route) { group.redirect_routes.create!(path: 'bar') }
-      let!(:nested_group_redirect_route) { nested_group.redirect_routes.create!(path: nested_group.path.sub('foo', 'bar')) }
-
-      context 'without follow_redirects option' do
-        context 'with the given path not matching any route' do
-          it { expect(described_class.find_by_full_path('unknown')).to eq(nil) }
-        end
-
-        context 'with the given path matching the canonical route' do
-          it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) }
-          it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) }
-          it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) }
-        end
-
-        context 'with the given path matching a redirect route' do
-          it { expect(described_class.find_by_full_path(group_redirect_route.path)).to eq(nil) }
-          it { expect(described_class.find_by_full_path(group_redirect_route.path.upcase)).to eq(nil) }
-          it { expect(described_class.find_by_full_path(nested_group_redirect_route.path)).to eq(nil) }
-        end
-      end
-
-      context 'with follow_redirects option set to true' do
-        context 'with the given path not matching any route' do
-          it { expect(described_class.find_by_full_path('unknown', follow_redirects: true)).to eq(nil) }
-        end
+    it_behaves_like '.find_by_full_path' do
+      let_it_be(:record) { nested_group }
+    end
 
-        context 'with the given path matching the canonical route' do
-          it { expect(described_class.find_by_full_path(group.to_param, follow_redirects: true)).to eq(group) }
-          it { expect(described_class.find_by_full_path(group.to_param.upcase, follow_redirects: true)).to eq(group) }
-          it { expect(described_class.find_by_full_path(nested_group.to_param, follow_redirects: true)).to eq(nested_group) }
-        end
+    it 'does not find projects with a matching path' do
+      project = create(:project)
+      redirect_route = create(:redirect_route, source: project)
 
-        context 'with the given path matching a redirect route' do
-          it { expect(described_class.find_by_full_path(group_redirect_route.path, follow_redirects: true)).to eq(group) }
-          it { expect(described_class.find_by_full_path(group_redirect_route.path.upcase, follow_redirects: true)).to eq(group) }
-          it { expect(described_class.find_by_full_path(nested_group_redirect_route.path, follow_redirects: true)).to eq(nested_group) }
-        end
-      end
+      expect(described_class.find_by_full_path(project.full_path)).to be_nil
+      expect(described_class.find_by_full_path(redirect_route.path, follow_redirects: true)).to be_nil
     end
   end
 
@@ -131,8 +151,6 @@ RSpec.describe Group, 'Routable' do
     end
 
     context 'with valid paths' do
-      let!(:nested_group) { create(:group, parent: group) }
-
       it 'returns the projects matching the paths' do
         result = described_class.where_full_path_in([group.to_param, nested_group.to_param])
 
@@ -148,32 +166,36 @@ RSpec.describe Group, 'Routable' do
   end
 
   describe '#full_path' do
-    let(:group) { create(:group) }
-    let(:nested_group) { create(:group, parent: group) }
-
     it { expect(group.full_path).to eq(group.path) }
     it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") }
   end
 
   describe '#full_name' do
-    let(:group) { create(:group) }
-    let(:nested_group) { create(:group, parent: group) }
-
     it { expect(group.full_name).to eq(group.name) }
     it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") }
   end
 end
 
 RSpec.describe Project, 'Routable' do
-  describe '#full_path' do
-    let(:project) { build_stubbed(:project) }
+  let_it_be(:project) { create(:project) }
 
+  it_behaves_like '.find_by_full_path' do
+    let_it_be(:record) { project }
+  end
+
+  it 'does not find groups with a matching path' do
+    group = create(:group)
+    redirect_route = create(:redirect_route, source: group)
+
+    expect(described_class.find_by_full_path(group.full_path)).to be_nil
+    expect(described_class.find_by_full_path(redirect_route.path, follow_redirects: true)).to be_nil
+  end
+
+  describe '#full_path' do
     it { expect(project.full_path).to eq "#{project.namespace.full_path}/#{project.path}" }
   end
 
   describe '#full_name' do
-    let(:project) { build_stubbed(:project) }
-
     it { expect(project.full_name).to eq "#{project.namespace.human_name} / #{project.name}" }
   end
 end

spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb

@@ -504,6 +504,17 @@ RSpec.shared_examples 'wiki controller actions' do
     end
   end
 
+  describe '#git_access' do
+    render_views
+
+    it 'renders the git access page' do
+      get :git_access, params: routing_params
+
+      expect(response).to render_template('shared/wikis/git_access')
+      expect(response.body).to include(wiki.http_url_to_repo)
+    end
+  end
+
   def redirect_to_wiki(wiki, page)
     redirect_to(controller.wiki_page_path(wiki, page))
   end

spec/views/shared/wikis/_sidebar.html.haml_spec.rb

@@ -16,16 +16,6 @@ RSpec.describe 'shared/wikis/_sidebar.html.haml' do
     expect(rendered).to have_link('Clone repository')
   end
 
-  context 'the wiki is not a project wiki' do
-    it 'does not include the clone repository link' do
-      allow(wiki).to receive(:container).and_return(create(:group))
-
-      render
-
-      expect(rendered).not_to have_link('Clone repository')
-    end
-  end
-
   context 'the sidebar failed to load' do
     before do
       assign(:sidebar_error, Object.new)