diff options
author | James Lopez <james@gitlab.com> | 2018-03-15 15:01:13 +0000 |
---|---|---|
committer | James Lopez <james@jameslopez.es> | 2018-03-16 16:15:16 +0100 |
commit | 1e21892f753aee9e6ec0ef612b53db07763bf03c (patch) | |
tree | f19e32048db3f42f6cbe2cab80135cfc2e6f7e15 | |
parent | 2d6c50bf02f975f1d28d35580c4ab10276787093 (diff) | |
download | gitlab-ce-1e21892f753aee9e6ec0ef612b53db07763bf03c.tar.gz |
Merge branch 'fix/auth0-unsafe-login-10-6' into 'security-10-6'
[10.6] Fix GitLab Auth0 integration signs in the wrong user
See merge request gitlab/gitlabhq!2354
-rw-r--r-- | Gemfile | 2 | ||||
-rw-r--r-- | Gemfile.lock | 6 | ||||
-rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 14 | ||||
-rw-r--r-- | changelogs/unreleased/fix-auth0-unsafe-login.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/remove-unnecessary-validate-project.yml | 5 | ||||
-rw-r--r-- | db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb | 25 | ||||
-rw-r--r-- | doc/integration/auth0.md | 7 | ||||
-rw-r--r-- | spec/controllers/omniauth_callbacks_controller_spec.rb | 103 | ||||
-rw-r--r-- | spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb | 22 |
9 files changed, 134 insertions, 55 deletions
@@ -25,7 +25,7 @@ gem 'devise', '~> 4.2' gem 'doorkeeper', '~> 4.2.0' gem 'doorkeeper-openid_connect', '~> 1.2.0' gem 'omniauth', '~> 1.4.2' -gem 'omniauth-auth0', '~> 1.4.1' +gem 'omniauth-auth0', '~> 2.0.0' gem 'omniauth-azure-oauth2', '~> 0.0.9' gem 'omniauth-cas3', '~> 1.1.4' gem 'omniauth-facebook', '~> 4.0.0' diff --git a/Gemfile.lock b/Gemfile.lock index fa99ec3febe..8957f1f3867 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -531,8 +531,8 @@ GEM omniauth (1.4.2) hashie (>= 1.2, < 4) rack (>= 1.0, < 3) - omniauth-auth0 (1.4.1) - omniauth-oauth2 (~> 1.1) + omniauth-auth0 (2.0.0) + omniauth-oauth2 (~> 1.4) omniauth-authentiq (0.3.1) omniauth-oauth2 (~> 1.3, >= 1.3.1) omniauth-azure-oauth2 (0.0.9) @@ -1113,7 +1113,7 @@ DEPENDENCIES octokit (~> 4.6.2) oj (~> 2.17.4) omniauth (~> 1.4.2) - omniauth-auth0 (~> 1.4.1) + omniauth-auth0 (~> 2.0.0) omniauth-authentiq (~> 0.3.1) omniauth-azure-oauth2 (~> 0.0.9) omniauth-cas3 (~> 1.1.4) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 8440945ab43..fff249577a2 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -95,6 +95,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController handle_omniauth end + def auth0 + if oauth['uid'].blank? + fail_auth0_login + else + handle_omniauth + end + end + private def handle_omniauth @@ -170,6 +178,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController redirect_to new_user_session_path end + def fail_auth0_login + flash[:alert] = 'Wrong extern UID provided. Make sure Auth0 is configured correctly.' + + redirect_to new_user_session_path + end + def handle_disabled_provider label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider']) flash[:alert] = "Signing in using #{label} has been disabled" diff --git a/changelogs/unreleased/fix-auth0-unsafe-login.yml b/changelogs/unreleased/fix-auth0-unsafe-login.yml new file mode 100644 index 00000000000..01c6ea69dcc --- /dev/null +++ b/changelogs/unreleased/fix-auth0-unsafe-login.yml @@ -0,0 +1,5 @@ +--- +title: Fix GitLab Auth0 integration signing in the wrong user +merge_request: +author: +type: security diff --git a/changelogs/unreleased/remove-unnecessary-validate-project.yml b/changelogs/unreleased/remove-unnecessary-validate-project.yml deleted file mode 100644 index ebc8da03dd8..00000000000 --- a/changelogs/unreleased/remove-unnecessary-validate-project.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: 'Remove unecessary validate: true from belongs_to :project' -merge_request: -author: -type: fixed diff --git a/db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb b/db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb new file mode 100644 index 00000000000..2d5a8617169 --- /dev/null +++ b/db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb @@ -0,0 +1,25 @@ +class RemoveEmptyExternUidAuth0Identities < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class Identity < ActiveRecord::Base + self.table_name = 'identities' + include EachBatch + end + + def up + broken_auth0_identities.each_batch do |identity| + identity.delete_all + end + end + + def broken_auth0_identities + Identity.where(provider: 'auth0', extern_uid: [nil, '']) + end + + def down + end +end diff --git a/doc/integration/auth0.md b/doc/integration/auth0.md index c39d7ab57c6..a75836a915a 100644 --- a/doc/integration/auth0.md +++ b/doc/integration/auth0.md @@ -56,7 +56,8 @@ for initial settings. "name" => "auth0", "args" => { client_id: 'YOUR_AUTH0_CLIENT_ID', client_secret: 'YOUR_AUTH0_CLIENT_SECRET', - namespace: 'YOUR_AUTH0_DOMAIN' + domain: 'YOUR_AUTH0_DOMAIN', + scope: 'openid profile email' } } ] @@ -69,8 +70,8 @@ for initial settings. args: { client_id: 'YOUR_AUTH0_CLIENT_ID', client_secret: 'YOUR_AUTH0_CLIENT_SECRET', - namespace: 'YOUR_AUTH0_DOMAIN' - } + domain: 'YOUR_AUTH0_DOMAIN', + scope: 'openid profile email' } } ``` diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index c639ad32ec6..9fd129e4ee9 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -3,73 +3,90 @@ require 'spec_helper' describe OmniauthCallbacksController do include LoginHelpers - let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: provider) } - let(:provider) { :github } + let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) } before do - mock_auth_hash(provider.to_s, 'my-uid', user.email) + mock_auth_hash(provider.to_s, extern_uid, user.email) stub_omniauth_provider(provider, context: request) end - it 'allows sign in' do - post provider + context 'github' do + let(:extern_uid) { 'my-uid' } + let(:provider) { :github } - expect(request.env['warden']).to be_authenticated - end + it 'allows sign in' do + post provider + + expect(request.env['warden']).to be_authenticated + end - shared_context 'sign_up' do - let(:user) { double(email: 'new@example.com') } + shared_context 'sign_up' do + let(:user) { double(email: 'new@example.com') } - before do - stub_omniauth_setting(block_auto_created_users: false) + before do + stub_omniauth_setting(block_auto_created_users: false) + end end - end - context 'sign up' do - include_context 'sign_up' + context 'sign up' do + include_context 'sign_up' - it 'is allowed' do - post provider + it 'is allowed' do + post provider - expect(request.env['warden']).to be_authenticated + expect(request.env['warden']).to be_authenticated + end end - end - context 'when OAuth is disabled' do - before do - stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') - settings = Gitlab::CurrentSettings.current_application_settings - settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) - end + context 'when OAuth is disabled' do + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + settings = Gitlab::CurrentSettings.current_application_settings + settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) + end - it 'prevents login via POST' do - post provider + it 'prevents login via POST' do + post provider - expect(request.env['warden']).not_to be_authenticated - end + expect(request.env['warden']).not_to be_authenticated + end - it 'shows warning when attempting login' do - post provider + it 'shows warning when attempting login' do + post provider - expect(response).to redirect_to new_user_session_path - expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') - end + expect(response).to redirect_to new_user_session_path + expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') + end - it 'allows linking the disabled provider' do - user.identities.destroy_all - sign_in(user) + it 'allows linking the disabled provider' do + user.identities.destroy_all + sign_in(user) - expect { post provider }.to change { user.reload.identities.count }.by(1) - end + expect { post provider }.to change { user.reload.identities.count }.by(1) + end - context 'sign up' do - include_context 'sign_up' + context 'sign up' do + include_context 'sign_up' - it 'is prevented' do - post provider + it 'is prevented' do + post provider - expect(request.env['warden']).not_to be_authenticated + expect(request.env['warden']).not_to be_authenticated + end end end end + + context 'auth0' do + let(:extern_uid) { '' } + let(:provider) { :auth0 } + + it 'does not allow sign in without extern_uid' do + post 'auth0' + + expect(request.env['warden']).not_to be_authenticated + expect(response.status).to eq(302) + expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.') + end + end end diff --git a/spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb b/spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb new file mode 100644 index 00000000000..441c4295a40 --- /dev/null +++ b/spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20180220150310_remove_empty_extern_uid_auth0_identities.rb') + +describe RemoveEmptyExternUidAuth0Identities, :migration do + let(:identities) { table(:identities) } + + before do + identities.create(provider: 'auth0', extern_uid: '') + identities.create(provider: 'auth0', extern_uid: 'valid') + identities.create(provider: 'github', extern_uid: '') + + migrate! + end + + it 'leaves the correct auth0 identity' do + expect(identities.where(provider: 'auth0').pluck(:extern_uid)).to eq(['valid']) + end + + it 'leaves the correct github identity' do + expect(identities.where(provider: 'github').count).to eq(1) + end +end |