Merge branch 'master' into expiration-date-on-memberships

CHANGELOG

@@ -51,6 +51,7 @@ v 8.11.0 (unreleased)
   - Get issue and merge request description templates from repositories
   - Add hover state to todos !5361 (winniehell)
   - Fix icon alignment of star and fork buttons !5451 (winniehell)
+  - Enforce 2FA restrictions on API authentication endpoints !5820
   - Limit git rev-list output count to one in forced push check
   - Show deployment status on merge requests with external URLs
   - Clean up unused routes (Josef Strzibny)
@@ -67,6 +68,7 @@ v 8.11.0 (unreleased)
   - Upgrade Grape from 0.13.0 to 0.15.0. !4601
   - Trigram indexes for the "ci_runners" table have been removed to speed up UPDATE queries
   - Fix devise deprecation warnings.
+  - Check for 2FA when using Git over HTTP and only allow PersonalAccessTokens as password in that case !5764
   - Update version_sorter and use new interface for faster tag sorting
   - Optimize checking if a user has read access to a list of issues !5370
   - Store all DB secrets in secrets.yml, under descriptive names !5274

Gemfile

@@ -20,7 +20,7 @@ gem 'pg', '~> 0.18.2', group: :postgres
 
 # Authentication libraries
 gem 'devise',                 '~> 4.0'
-gem 'doorkeeper',             '~> 4.0'
+gem 'doorkeeper',             '~> 4.2.0'
 gem 'omniauth',               '~> 1.3.1'
 gem 'omniauth-auth0',         '~> 1.4.1'
 gem 'omniauth-azure-oauth2',  '~> 0.0.6'
@@ -77,7 +77,7 @@ gem 'rack-cors',    '~> 0.4.0', require: 'rack/cors'
 gem 'kaminari', '~> 0.17.0'
 
 # HAML
-gem 'hamlit', '~> 2.5'
+gem 'hamlit', '~> 2.6.1'
 
 # Files attachments
 gem 'carrierwave', '~> 0.10.0'

Gemfile.lock

@@ -176,7 +176,7 @@ GEM
     diff-lcs (1.2.5)
     diffy (3.0.7)
     docile (1.1.5)
-    doorkeeper (4.0.0)
+    doorkeeper (4.2.0)
       railties (>= 4.2)
     dropzonejs-rails (0.7.2)
       rails (> 3.1)
@@ -322,7 +322,7 @@ GEM
     grape-entity (0.4.8)
       activesupport
       multi_json (>= 1.3.2)
-    hamlit (2.5.0)
+    hamlit (2.6.1)
       temple (~> 0.7.6)
       thor
       tilt
@@ -836,7 +836,7 @@ DEPENDENCIES
   devise (~> 4.0)
   devise-two-factor (~> 3.0.0)
   diffy (~> 3.0.3)
-  doorkeeper (~> 4.0)
+  doorkeeper (~> 4.2.0)
   dropzonejs-rails (~> 0.7.1)
   email_reply_parser (~> 0.5.8)
   email_spec (~> 1.6.0)
@@ -867,7 +867,7 @@ DEPENDENCIES
   gon (~> 6.1.0)
   grape (~> 0.15.0)
   grape-entity (~> 0.4.2)
-  hamlit (~> 2.5)
+  hamlit (~> 2.6.1)
   health_check (~> 2.1.0)
   hipchat (~> 1.5.0)
   html-pipeline (~> 1.11.0)

app/controllers/concerns/issuable_collections.rb

@@ -66,6 +66,11 @@ module IssuableCollections
     key = 'issuable_sort'
 
     cookies[key] = params[:sort] if params[:sort].present?
+
+    # id_desc and id_asc are old values for these two.
+    cookies[key] = sort_value_recently_created if cookies[key] == 'id_desc'
+    cookies[key] = sort_value_oldest_created if cookies[key] == 'id_asc'
+
     params[:sort] = cookies[key]
   end
 

app/controllers/projects/git_http_client_controller.rb

@@ -27,6 +27,9 @@ class Projects::GitHttpClientController < Projects::ApplicationController
         @ci = true
       elsif auth_result.type == :oauth && !download_request?
         # Not allowed
+      elsif auth_result.type == :missing_personal_token
+        render_missing_personal_token
+        return # Render above denied access, nothing left to do
       else
         @user = auth_result.user
       end
@@ -91,6 +94,13 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     [nil, nil]
   end
 
+  def render_missing_personal_token
+    render plain: "HTTP Basic: Access denied\n" \
+                  "You have 2FA enabled, please use a personal access token for Git over HTTP.\n" \
+                  "You can generate one at #{profile_personal_access_tokens_url}",
+           status: 401
+  end
+
   def repository
     _, suffix = project_id_with_suffix
     if suffix == '.wiki.git'

app/views/profiles/personal_access_tokens/index.html.haml

@@ -7,6 +7,10 @@
       = page_title
     %p
       You can generate a personal access token for each application you use that needs access to the GitLab API.
+    %p
+      You can also use personal access tokens to authenticate against Git over HTTP.
+      They are the only accepted password when you have Two-Factor Authentication (2FA) enabled.
+
   .col-lg-9
 
     - if flash[:personal_access_token]

config/initializers/doorkeeper.rb

@@ -12,7 +12,8 @@ Doorkeeper.configure do
   end
 
   resource_owner_from_credentials do |routes|
-    Gitlab::Auth.find_with_user_password(params[:username], params[:password])
+    user = Gitlab::Auth.find_with_user_password(params[:username], params[:password])
+    user unless user.try(:two_factor_enabled?)
   end
 
   # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below.

doc/api/oauth2.md

@@ -90,7 +90,7 @@ curl --header "Authorization: Bearer OAUTH-TOKEN" https://localhost:3000/api/v3/
 
 ## Deprecation Notice
 
-1. Starting in GitLab 9.0, the Resource Owner Password Credentials will be *disabled* for users with two-factor authentication turned on.
+1. Starting in GitLab 8.11, the Resource Owner Password Credentials has been *disabled* for users with two-factor authentication turned on.
 2. These users can access the API using [personal access tokens] instead.
 
 ---

doc/api/session.md

@@ -2,7 +2,7 @@
 
 ## Deprecation Notice
 
-1. Starting in GitLab 9.0, this feature will be *disabled* for users with two-factor authentication turned on.
+1. Starting in GitLab 8.11, this feature has been *disabled* for users with two-factor authentication turned on.
 2. These users can access the API using [personal access tokens] instead.
 
 ---

lib/api/session.rb

@@ -14,6 +14,7 @@ module API
       user = Gitlab::Auth.find_with_user_password(params[:email] || params[:login], params[:password])
 
       return unauthorized! unless user
+      return render_api_error!('401 Unauthorized. You have 2FA enabled. Please use a personal access token to access the API', 401) if user.two_factor_enabled?
       present user, with: Entities::UserLogin
     end
   end

lib/gitlab/auth.rb

@@ -10,13 +10,12 @@ module Gitlab
 
         if valid_ci_request?(login, password, project)
           result.type = :ci
-        elsif result.user = find_with_user_password(login, password)
-          result.type = :gitlab_or_ldap
-        elsif result.user = oauth_access_token_check(login, password)
-          result.type = :oauth
+        else
+          result = populate_result(login, password)
         end
 
-        rate_limit!(ip, success: !!result.user || (result.type == :ci), login: login)
+        success = result.user.present? || [:ci, :missing_personal_token].include?(result.type)
+        rate_limit!(ip, success: success, login: login)
         result
       end
 
@@ -76,10 +75,43 @@ module Gitlab
         end
       end
 
+      def populate_result(login, password)
+        result =
+          user_with_password_for_git(login, password) ||
+          oauth_access_token_check(login, password) ||
+          personal_access_token_check(login, password)
+
+        if result
+          result.type = nil unless result.user
+
+          if result.user && result.user.two_factor_enabled? && result.type == :gitlab_or_ldap
+            result.type = :missing_personal_token
+          end
+        end
+
+        result || Result.new
+      end
+
+      def user_with_password_for_git(login, password)
+        user = find_with_user_password(login, password)
+        Result.new(user, :gitlab_or_ldap) if user
+      end
+
       def oauth_access_token_check(login, password)
         if login == "oauth2" && password.present?
           token = Doorkeeper::AccessToken.by_token(password)
-          token && token.accessible? && User.find_by(id: token.resource_owner_id)
+          if token && token.accessible?
+            user = User.find_by(id: token.resource_owner_id)
+            Result.new(user, :oauth)
+          end
+        end
+      end
+
+      def personal_access_token_check(login, password)
+        if login && password
+          user = User.find_by_personal_access_token(password)
+          validation = User.by_login(login)
+          Result.new(user, :personal_token) if user == validation
         end
       end
     end

spec/features/issuables/default_sort_order_spec.rb

@@ -149,6 +149,30 @@ describe 'Projects > Issuables > Default sort order', feature: true do
         expect(last_issue).to include(first_created_issuable.title)
       end
     end
+
+    context 'when the sort in the URL is id_desc' do
+      let(:issuable_type) { :issue }
+
+      before { visit_issues(project, sort: 'id_desc') }
+
+      it 'shows the sort order as last created' do
+        expect(find('.issues-other-filters')).to have_content('Last created')
+        expect(first_issue).to include(last_created_issuable.title)
+        expect(last_issue).to include(first_created_issuable.title)
+      end
+    end
+
+    context 'when the sort in the URL is id_asc' do
+      let(:issuable_type) { :issue }
+
+      before { visit_issues(project, sort: 'id_asc') }
+
+      it 'shows the sort order as oldest created' do
+        expect(find('.issues-other-filters')).to have_content('Oldest created')
+        expect(first_issue).to include(first_created_issuable.title)
+        expect(last_issue).to include(last_created_issuable.title)
+      end
+    end
   end
 
   def selected_sort_order

spec/helpers/page_layout_helper_spec.rb

@@ -97,5 +97,14 @@ describe PageLayoutHelper do
         expect(tags).to include %q(<meta property="twitter:data1" content="bar" />)
       end
     end
+
+    it 'escapes content' do
+      allow(helper).to receive(:page_card_attributes)
+        .and_return(foo: %q{foo" http-equiv="refresh}.html_safe)
+
+      tags = helper.page_card_meta_tags
+
+      expect(tags).to include(%q{content="foo&quot; http-equiv=&quot;refresh"})
+    end
   end
 end

spec/requests/api/oauth_tokens_spec.rb

@@ -0,0 +1,33 @@
+require 'spec_helper'
+
+describe API::API, api: true  do
+  include ApiHelpers
+
+  context 'Resource Owner Password Credentials' do
+    def request_oauth_token(user)
+      post '/oauth/token', username: user.username, password: user.password, grant_type: 'password'
+    end
+
+    context 'when user has 2FA enabled' do
+      it 'does not create an access token' do
+        user = create(:user, :two_factor)
+
+        request_oauth_token(user)
+
+        expect(response).to have_http_status(401)
+        expect(json_response['error']).to eq('invalid_grant')
+      end
+    end
+
+    context 'when user does not have 2FA enabled' do
+      it 'creates an access token' do
+        user = create(:user)
+
+        request_oauth_token(user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response['access_token']).not_to be_nil
+      end
+    end
+  end
+end

spec/requests/api/session_spec.rb

@@ -17,6 +17,17 @@ describe API::API, api: true  do
         expect(json_response['can_create_project']).to eq(user.can_create_project?)
         expect(json_response['can_create_group']).to eq(user.can_create_group?)
       end
+
+      context 'with 2FA enabled' do
+        it 'rejects sign in attempts' do
+          user = create(:user, :two_factor)
+
+          post api('/session'), email: user.email, password: user.password
+
+          expect(response).to have_http_status(401)
+          expect(response.body).to include('You have 2FA enabled.')
+        end
+      end
     end
 
     context 'when email has case-typo and password is valid' do

spec/requests/git_http_spec.rb

@@ -198,6 +198,45 @@ describe 'Git HTTP requests', lib: true do
               end
             end
 
+            context 'when user has 2FA enabled' do
+              let(:user) { create(:user, :two_factor) }
+              let(:access_token) { create(:personal_access_token, user: user) }
+
+              before do
+                project.team << [user, :master]
+              end
+
+              context 'when username and password are provided' do
+                it 'rejects the clone attempt' do
+                  download("#{project.path_with_namespace}.git", user: user.username, password: user.password) do |response|
+                    expect(response).to have_http_status(401)
+                    expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
+                  end
+                end
+
+                it 'rejects the push attempt' do
+                  upload("#{project.path_with_namespace}.git", user: user.username, password: user.password) do |response|
+                    expect(response).to have_http_status(401)
+                    expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
+                  end
+                end
+              end
+
+              context 'when username and personal access token are provided' do
+                it 'allows clones' do
+                  download("#{project.path_with_namespace}.git", user: user.username, password: access_token.token) do |response|
+                    expect(response).to have_http_status(200)
+                  end
+                end
+
+                it 'allows pushes' do
+                  upload("#{project.path_with_namespace}.git", user: user.username, password: access_token.token) do |response|
+                    expect(response).to have_http_status(200)
+                  end
+                end
+              end
+            end
+
             context "when blank password attempts follow a valid login" do
               def attempt_login(include_password)
                 password = include_password ? user.password : ""

spec/views/layouts/_head.html.haml_spec.rb

@@ -0,0 +1,36 @@
+require 'spec_helper'
+
+describe 'layouts/_head' do
+  before do
+    stub_template 'layouts/_user_styles.html.haml' => ''
+  end
+
+  it 'escapes HTML-safe strings in page_title' do
+    stub_helper_with_safe_string(:page_title)
+
+    render
+
+    expect(rendered).to match(%{content="foo" http-equiv="refresh"})
+  end
+
+  it 'escapes HTML-safe strings in page_description' do
+    stub_helper_with_safe_string(:page_description)
+
+    render
+
+    expect(rendered).to match(%{content="foo" http-equiv="refresh"})
+  end
+
+  it 'escapes HTML-safe strings in page_image' do
+    stub_helper_with_safe_string(:page_image)
+
+    render
+
+    expect(rendered).to match(%{content="foo" http-equiv="refresh"})
+  end
+
+  def stub_helper_with_safe_string(method)
+    allow_any_instance_of(PageLayoutHelper).to receive(method)
+      .and_return(%q{foo" http-equiv="refresh}.html_safe)
+  end
+end