Merge branch 'rs-kerberos-default-clone' into 'master'

Set KRB5 as default clone URL when Kerberos is enabled and user is logged in

_Originally opened at !363 by @cernvcs._

- - -

In GitLab instances where Kerberos authentication (gitlab-org/gitlab-ee!6) is enabled, this protocol becomes the preferred one.

This merge request extends the functionality provided in gitlab-org/gitlab-ee!70 where Kerberos url is displayed alongside the SSH and HTTP urls when Kerberos authentication is enabled.

The implementation proposed here makes Kerberos the default clone procotol when Kerberos is enabled. When a logged in user visits the main page for a project or creates a new one, the first url offered is the Kerberos one.

This merge request also aligns the [implementation](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/helpers/button_helper.rb) and [usage](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/views/shared/_clone_panel.html.haml) of the 3 clone buttons, adds additional tests and updates some others to consider all the possible scenarios.

See merge request !372

CHANGELOG-EE

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.8.0 (unreleased)
   - [Elastic] Database indexer prints its status
   - [Elastic][Fix] Database indexer skips projects with invalid HEAD reference
+  - Set KRB5 as default clone protocol when Kerberos is enabled and user is logged in (Borja Aparicio)
 
 v 8.7.2
   - Fix MR notifications for slack and hipchat when approvals are fullfiled. !325

app/helpers/button_helper.rb

@@ -55,18 +55,17 @@ module ButtonHelper
   end
 
   def kerberos_clone_button(project)
-    klass = 'btn js-protocol-switch'
-    klass << ' active'      if default_clone_protocol == 'kerberos'
+    klass = 'kerberos-selector'
     klass << ' has-tooltip'
 
-    content_tag :button, 'KRB5',
+    content_tag :a, 'KRB5',
       class: klass,
+      href: project.kerberos_url_to_repo,
       data: {
-        clone: project.kerberos_url_to_repo,
-        container: 'body',
         html: 'true',
-        title: 'Get a Kerberos token for your account with kinit<br>to pull or push via Kerberos.'
-      },
-      type: :button
+        placement: 'right',
+        container: 'body',
+        title: 'Get a Kerberos token for your<br>account with kinit.'
+      }
   end
 end

app/helpers/projects_helper.rb

@@ -209,7 +209,10 @@ module ProjectsHelper
   end
 
   def default_url_to_repo(project = @project)
-    if default_clone_protocol == "ssh"
+    case default_clone_protocol
+    when 'krb5'
+      project.kerberos_url_to_repo
+    when 'ssh'
       project.ssh_url_to_repo
     else
       project.http_url_to_repo
@@ -217,7 +220,9 @@ module ProjectsHelper
   end
 
   def default_clone_protocol
-    if !current_user || current_user.require_ssh_key?
+    if alternative_kerberos_url? && current_user
+      "krb5"
+    elsif !current_user || current_user.require_ssh_key?
       gitlab_config.protocol
     else
       "ssh"

app/views/shared/_clone_panel.html.haml

@@ -13,8 +13,7 @@
         = http_clone_button(project)
       - if alternative_kerberos_url?
         %li
-          %a#kerberos-btn{href: @project.kerberos_url_to_repo}
-            KRB5
+          = kerberos_clone_button(project)
 
   = text_field_tag :project_clone, default_url_to_repo(project), class: "js-select-on-focus form-control", readonly: true
   .input-group-btn

features/project/create.feature

@@ -7,37 +7,8 @@ Feature: Project Create
   @javascript
   Scenario: User create a project
     Given I sign in as a user
-    When I visit new project page
-    And I have an ssh key
-    And fill project form with valid data
-    Then I should see project page
-    And I should see empty project instuctions
-
-  @javascript
-  Scenario: Empty project instructions with Kerberos disabled
-    Given I sign in as a user
-    And KRB5 disabled
     And I have an ssh key
     When I visit new project page
     And fill project form with valid data
-    Then I see empty project instuctions
-    And I click on HTTP
-    Then Remote url should update to http link
-    And If I click on SSH
-    Then Remote url should update to ssh link
-
-  @javascript
-  Scenario: Empty project instructions with Kerberos enabled
-    Given I sign in as a user
-    Given KRB5 enabled
-    And I have an ssh key
-    When I visit new project page
-    And fill project form with valid data
-    Then I see empty project instuctions
-    And I click on HTTP
-    Then Remote url should update to http link
-    And If I click on SSH
-    Then Remote url should update to ssh link
-    And If I click on KRB5
-    Then Remote url should update to kerberos link
-    And KRB5 disabled
+    Then I should see project page
+    And I should see empty project instructions

features/steps/project/create.rb

@@ -13,52 +13,9 @@ class Spinach::Features::ProjectCreate < Spinach::FeatureSteps
     expect(current_path).to eq namespace_project_path(Project.last.namespace, Project.last)
   end
 
-  step 'I should see empty project instuctions' do
+  step 'I should see empty project instructions' do
     expect(page).to have_content "git init"
     expect(page).to have_content "git remote"
     expect(page).to have_content Project.last.url_to_repo
   end
-
-  step 'KRB5 enabled' do
-    # Enable Kerberos in an alternative port to force Kerberos button and URL to show up in the UI
-    allow(Gitlab.config.kerberos).to receive(:enabled).and_return(true)
-    allow(Gitlab.config.kerberos).to receive(:use_dedicated_port).and_return(true)
-  end
-
-  step 'KRB5 disabled' do
-    allow(Gitlab.config.kerberos).to receive(:enabled).and_return(false)
-  end
-
-  step 'I see empty project instuctions' do
-    expect(page).to have_content "git init"
-    expect(page).to have_content "git remote"
-    expect(page).to have_content Project.last.url_to_repo
-  end
-
-  step 'I click on HTTP' do
-    find('#clone-dropdown').click
-    find('.http-selector').click
-  end
-
-  step 'Remote url should update to http link' do
-    expect(page).to have_content "git remote add origin #{Project.last.http_url_to_repo}"
-  end
-
-  step 'If I click on SSH' do
-    find('#clone-dropdown').click
-    find('.ssh-selector').click
-  end
-
-  step 'Remote url should update to ssh link' do
-    expect(page).to have_content "git remote add origin #{Project.last.url_to_repo}"
-  end
-
-  step 'If I click on KRB5' do
-    find('#clone-dropdown').click
-    find('#kerberos-btn').click
-  end
-
-  step 'Remote url should update to kerberos link' do
-    expect(page).to have_content "git remote add origin #{Project.last.kerberos_url_to_repo}"
-  end
 end

spec/factories/projects.rb

@@ -92,6 +92,12 @@ FactoryGirl.define do
       mirror true
       mirror_user_id { creator_id }
     end
+
+    trait :empty_repo do
+      after(:create) do |project|
+        project.create_repository
+      end
+    end
   end
 
   # Project with empty repository
@@ -99,9 +105,7 @@ FactoryGirl.define do
   # This is a case when you just created a project
   # but not pushed any code there yet
   factory :project_empty_repo, parent: :empty_project do
-    after :create do |project|
-      project.create_repository
-    end
+    empty_repo
   end
 
   # Project with test repository

spec/features/projects/developer_views_empty_project_instructions_spec.rb

+require 'rails_helper'
+
+feature 'Developer views empty project instructions', feature: true do
+  let(:project) { create(:empty_project, :empty_repo) }
+  let(:developer) { create(:user) }
+
+  background do
+    project.team << [developer, :developer]
+
+    login_as(developer)
+  end
+
+  context 'without an SSH key' do
+    scenario 'defaults to HTTP' do
+      visit_project
+
+      expect_instructions_for('http')
+    end
+
+    scenario 'switches to SSH', js: true do
+      visit_project
+
+      select_protocol('SSH')
+
+      expect_instructions_for('ssh')
+    end
+  end
+
+  context 'with an SSH key' do
+    background do
+      create(:personal_key, user: developer)
+    end
+
+    scenario 'defaults to SSH' do
+      visit_project
+
+      expect_instructions_for('ssh')
+    end
+
+    scenario 'switches to HTTP', js: true do
+      visit_project
+
+      select_protocol('HTTP')
+
+      expect_instructions_for('http')
+    end
+  end
+
+  context 'with Kerberos enabled' do
+    background do
+      allow(Gitlab.config.kerberos).to receive(:enabled).and_return(true)
+    end
+
+    scenario 'defaults to KRB5' do
+      visit_project
+
+      expect_instructions_for('kerberos')
+    end
+  end
+
+  def visit_project
+    visit namespace_project_path(project.namespace, project)
+  end
+
+  def select_protocol(protocol)
+    find('#clone-dropdown').click
+    find(".#{protocol.downcase}-selector").click
+  end
+
+  def expect_instructions_for(protocol)
+    msg = :"#{protocol.downcase}_url_to_repo"
+
+    expect(page).to have_content("git clone #{project.send(msg)}")
+  end
+end

spec/helpers/projects_helper_spec.rb

@@ -88,22 +88,31 @@ describe ProjectsHelper do
   end
 
   describe 'default_clone_protocol' do
-    describe 'using HTTP' do
+    context 'when user is not logged in and gitlab protocol is HTTP' do
       it 'returns HTTP' do
-        expect(helper).to receive(:current_user).and_return(nil)
+        allow(helper).to receive(:current_user).and_return(nil)
 
         expect(helper.send(:default_clone_protocol)).to eq('http')
       end
     end
 
-    describe 'using HTTPS' do
+    context 'when user is not logged in and gitlab protocol is HTTPS' do
       it 'returns HTTPS' do
-        allow(Gitlab.config.gitlab).to receive(:protocol).and_return('https')
-        expect(helper).to receive(:current_user).and_return(nil)
+        stub_config_setting(protocol: 'https')
+        allow(helper).to receive(:current_user).and_return(nil)
 
         expect(helper.send(:default_clone_protocol)).to eq('https')
       end
     end
+
+    context 'when gitlab.config.kerberos is enabled and user is logged in' do
+      it 'returns krb5 as default protocol' do
+        allow(Gitlab.config.kerberos).to receive(:enabled).and_return(true)
+        allow(helper).to receive(:current_user).and_return(double)
+
+        expect(helper.send(:default_clone_protocol)).to eq('krb5')
+      end
+    end
   end
 
   describe '#license_short_name' do