diff options
| -rw-r--r-- | CHANGELOG | 2 | ||||
| -rw-r--r-- | Gemfile | 4 | ||||
| -rw-r--r-- | Gemfile.lock | 8 | ||||
| -rw-r--r-- | app/controllers/concerns/issuable_collections.rb | 5 | ||||
| -rw-r--r-- | app/controllers/projects/git_http_client_controller.rb | 10 | ||||
| -rw-r--r-- | app/views/profiles/personal_access_tokens/index.html.haml | 4 | ||||
| -rw-r--r-- | config/initializers/doorkeeper.rb | 3 | ||||
| -rw-r--r-- | doc/api/oauth2.md | 2 | ||||
| -rw-r--r-- | doc/api/session.md | 2 | ||||
| -rw-r--r-- | lib/api/session.rb | 1 | ||||
| -rw-r--r-- | lib/gitlab/auth.rb | 44 | ||||
| -rw-r--r-- | spec/features/issuables/default_sort_order_spec.rb | 24 | ||||
| -rw-r--r-- | spec/helpers/page_layout_helper_spec.rb | 9 | ||||
| -rw-r--r-- | spec/requests/api/oauth_tokens_spec.rb | 33 | ||||
| -rw-r--r-- | spec/requests/api/session_spec.rb | 11 | ||||
| -rw-r--r-- | spec/requests/git_http_spec.rb | 39 | ||||
| -rw-r--r-- | spec/views/layouts/_head.html.haml_spec.rb | 36 |
17 files changed, 222 insertions, 15 deletions
diff --git a/CHANGELOG b/CHANGELOG index 9a6c6d9730f..a66c7536eb3 100644 --- a/CHANGELOG +++ b/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 @@ -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' diff --git a/Gemfile.lock b/Gemfile.lock index f32d30bccd8..5511d718938 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index c802922e0af..b5e79099e39 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/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 diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb index 7c21bd181dc..a5b4031c30f 100644 --- a/app/controllers/projects/git_http_client_controller.rb +++ b/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' diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml index 71ac367830d..05a2ea67aa2 100644 --- a/app/views/profiles/personal_access_tokens/index.html.haml +++ b/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] diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 618dba74151..fc4b0a72add 100644 --- a/config/initializers/doorkeeper.rb +++ b/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. diff --git a/doc/api/oauth2.md b/doc/api/oauth2.md index 16ef79617c0..0b0fc39ec7e 100644 --- a/doc/api/oauth2.md +++ b/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. --- diff --git a/doc/api/session.md b/doc/api/session.md index 9076c48b899..f776424023e 100644 --- a/doc/api/session.md +++ b/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. --- diff --git a/lib/api/session.rb b/lib/api/session.rb index 56c202f1294..55ec66a6d67 100644 --- a/lib/api/session.rb +++ b/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 diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index db1704af75e..91f0270818a 100644 --- a/lib/gitlab/auth.rb +++ b/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 diff --git a/spec/features/issuables/default_sort_order_spec.rb b/spec/features/issuables/default_sort_order_spec.rb index 9114f751b55..9a2b879e789 100644 --- a/spec/features/issuables/default_sort_order_spec.rb +++ b/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 diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index cf632f594c7..dc07657e101 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/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" http-equiv="refresh"}) + end end end diff --git a/spec/requests/api/oauth_tokens_spec.rb b/spec/requests/api/oauth_tokens_spec.rb new file mode 100644 index 00000000000..7e2cc50e591 --- /dev/null +++ b/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 diff --git a/spec/requests/api/session_spec.rb b/spec/requests/api/session_spec.rb index 519e7ce12ad..acad1365ace 100644 --- a/spec/requests/api/session_spec.rb +++ b/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 diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 8537c252b58..afaf4b7cefb 100644 --- a/spec/requests/git_http_spec.rb +++ b/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 : "" diff --git a/spec/views/layouts/_head.html.haml_spec.rb b/spec/views/layouts/_head.html.haml_spec.rb new file mode 100644 index 00000000000..3fddfb3b62f --- /dev/null +++ b/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 |