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

8 files changed, 134 insertions, 50 deletions

diff --git a/Gemfile b/Gemfile
index 880ed483c34..97d40857e11 100644
--- a/Gemfile
+++ b/Gemfile
@@ -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 22c4fc0ef28..4022e289611 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -529,8 +529,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)
@@ -1106,7 +1106,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 83c9a3f035e..fecc3145833 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::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/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