From 220755f52ad6e3fdfa43c62e0a4a4051721246dc Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 18 Aug 2016 23:00:54 +0000 Subject: Merge branch '2fa-api-check' into 'master' 2FA checks for API workflows ## What does this MR do? It adds a check to the API `/session` endpoint that will deny authentication requests to users that have 2FA enabled. In the error message it will instruct them to use a Personal Access Token instead. It adds a check to the `/oauth/token` endpoint, when `grant_type: 'password'` is used, so that no OAuth2 access token can be generated if the user has 2FA enabled. This endpoint should not be used by OAuth applications, anyway. OAuth apps should follow the flow of redirecting the user to GitLab, where 2FA access restrictions apply and logging them in there. Once successfully authenticated, the OAuth token is passed to the client. ## Why was this MR needed? No 2FA check on API endpoints. ## What are the relevant issue numbers? Fixes #2979 See merge request !5820 --- CHANGELOG | 4 ++++ config/initializers/doorkeeper.rb | 3 ++- doc/api/oauth2.md | 2 +- doc/api/session.md | 2 +- lib/api/session.rb | 1 + spec/requests/api/oauth_tokens_spec.rb | 33 +++++++++++++++++++++++++++++++++ spec/requests/api/session_spec.rb | 11 +++++++++++ 7 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 spec/requests/api/oauth_tokens_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 8f318d3bd94..c8cc8ce8fdb 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -47,6 +47,10 @@ v 8.11.0 (unreleased) - Various redundant database indexes have been removed - Update `timeago` plugin to use multiple string/locale settings - Remove unused images (ClemMakesApps) + - 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) 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/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 -- cgit v1.2.1