283 lines
8.9 KiB
Diff
283 lines
8.9 KiB
Diff
From 7fca314680776995b4e6858b55001a4bf56bf17a Mon Sep 17 00:00:00 2001
|
|
From: James Lopez <james@gitlab.com>
|
|
Date: Thu, 15 Mar 2018 14:59:21 +0000
|
|
Subject: [PATCH] Merge branch 'fix/auth0-unsafe-login-10-5' into 'security-10-5'
|
|
|
|
[10.5] Fix GitLab Auth0 integration signs in the wrong user
|
|
|
|
See merge request gitlab/gitlabhq!2353
|
|
---
|
|
Gemfile | 2 +-
|
|
Gemfile.lock | 6 +++---
|
|
app/controllers/omniauth_callbacks_controller.rb | 14 ++++++++++++++
|
|
changelogs/unreleased/fix-auth0-unsafe-login.yml | 5 +++++
|
|
db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb | 25 +++++++++++++++++++++++++
|
|
doc/integration/auth0.md | 7 ++++---
|
|
spec/controllers/omniauth_callbacks_controller_spec.rb | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------
|
|
spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb | 22 ++++++++++++++++++++++
|
|
8 files changed, 134 insertions(+), 50 deletions(-)
|
|
create mode 100644 changelogs/unreleased/fix-auth0-unsafe-login.yml
|
|
create mode 100644 db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb
|
|
create mode 100644 spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb
|
|
|
|
--- a/app/controllers/omniauth_callbacks_controller.rb
|
|
+++ b/app/controllers/omniauth_callbacks_controller.rb
|
|
@@ -78,6 +78,14 @@
|
|
handle_omniauth
|
|
end
|
|
|
|
+ def auth0
|
|
+ if oauth['uid'].blank?
|
|
+ fail_auth0_login
|
|
+ else
|
|
+ handle_omniauth
|
|
+ end
|
|
+ end
|
|
+
|
|
private
|
|
|
|
def handle_omniauth
|
|
@@ -138,6 +146,12 @@
|
|
@oauth ||= request.env['omniauth.auth']
|
|
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::OAuth::Provider.label_for(oauth['provider'])
|
|
flash[:alert] = "Signing in using #{label} has been disabled"
|
|
--- /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
|
|
--- /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
|
|
--- a/doc/integration/auth0.md
|
|
+++ b/doc/integration/auth0.md
|
|
@@ -56,7 +56,8 @@
|
|
"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 @@
|
|
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' }
|
|
}
|
|
```
|
|
|
|
--- a/spec/controllers/omniauth_callbacks_controller_spec.rb
|
|
+++ b/spec/controllers/omniauth_callbacks_controller_spec.rb
|
|
@@ -3,73 +3,90 @@
|
|
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
|
|
--- /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
|
|
--- a/Gemfile
|
|
+++ b/Gemfile
|
|
@@ -21,7 +21,7 @@
|
|
gem 'devise', '~> 4.2'
|
|
gem 'doorkeeper', '~> 4.2'
|
|
gem 'omniauth', '~> 1.3', '>= 1.3.1'
|
|
-gem 'omniauth-auth0', '~> 1.4', '>= 1.4.1'
|
|
+gem 'omniauth-auth0', '~> 2.0'
|
|
gem 'omniauth-azure-oauth2', '~> 0.0.6'
|
|
gem 'omniauth-bitbucket', '~> 0.0.2'
|
|
gem 'omniauth-cas3', '~> 1.1', '>= 1.1.2'
|