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
This commit is contained in:
parent
95ced3bb5f
commit
140cb0c092
2
Gemfile
2
Gemfile
|
@ -25,7 +25,7 @@ gem 'devise', '~> 4.2'
|
||||||
gem 'doorkeeper', '~> 4.3'
|
gem 'doorkeeper', '~> 4.3'
|
||||||
gem 'doorkeeper-openid_connect', '~> 1.3'
|
gem 'doorkeeper-openid_connect', '~> 1.3'
|
||||||
gem 'omniauth', '~> 1.4.2'
|
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-azure-oauth2', '~> 0.0.9'
|
||||||
gem 'omniauth-cas3', '~> 1.1.4'
|
gem 'omniauth-cas3', '~> 1.1.4'
|
||||||
gem 'omniauth-facebook', '~> 4.0.0'
|
gem 'omniauth-facebook', '~> 4.0.0'
|
||||||
|
|
|
@ -527,8 +527,8 @@ GEM
|
||||||
omniauth (1.4.3)
|
omniauth (1.4.3)
|
||||||
hashie (>= 1.2, < 4)
|
hashie (>= 1.2, < 4)
|
||||||
rack (>= 1.6.2, < 3)
|
rack (>= 1.6.2, < 3)
|
||||||
omniauth-auth0 (1.4.1)
|
omniauth-auth0 (2.0.0)
|
||||||
omniauth-oauth2 (~> 1.1)
|
omniauth-oauth2 (~> 1.4)
|
||||||
omniauth-authentiq (0.3.1)
|
omniauth-authentiq (0.3.1)
|
||||||
omniauth-oauth2 (~> 1.3, >= 1.3.1)
|
omniauth-oauth2 (~> 1.3, >= 1.3.1)
|
||||||
omniauth-azure-oauth2 (0.0.9)
|
omniauth-azure-oauth2 (0.0.9)
|
||||||
|
@ -1105,7 +1105,7 @@ DEPENDENCIES
|
||||||
oauth2 (~> 1.4)
|
oauth2 (~> 1.4)
|
||||||
octokit (~> 4.8)
|
octokit (~> 4.8)
|
||||||
omniauth (~> 1.4.2)
|
omniauth (~> 1.4.2)
|
||||||
omniauth-auth0 (~> 1.4.1)
|
omniauth-auth0 (~> 2.0.0)
|
||||||
omniauth-authentiq (~> 0.3.1)
|
omniauth-authentiq (~> 0.3.1)
|
||||||
omniauth-azure-oauth2 (~> 0.0.9)
|
omniauth-azure-oauth2 (~> 0.0.9)
|
||||||
omniauth-cas3 (~> 1.1.4)
|
omniauth-cas3 (~> 1.1.4)
|
||||||
|
|
|
@ -95,6 +95,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
|
||||||
handle_omniauth
|
handle_omniauth
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def auth0
|
||||||
|
if oauth['uid'].blank?
|
||||||
|
fail_auth0_login
|
||||||
|
else
|
||||||
|
handle_omniauth
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def handle_omniauth
|
def handle_omniauth
|
||||||
|
@ -170,6 +178,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
|
||||||
redirect_to new_user_session_path
|
redirect_to new_user_session_path
|
||||||
end
|
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
|
def handle_disabled_provider
|
||||||
label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
|
label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
|
||||||
flash[:alert] = "Signing in using #{label} has been disabled"
|
flash[:alert] = "Signing in using #{label} has been disabled"
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Fix GitLab Auth0 integration signing in the wrong user
|
||||||
|
merge_request:
|
||||||
|
author:
|
||||||
|
type: security
|
|
@ -1,5 +0,0 @@
|
||||||
---
|
|
||||||
title: 'Remove unecessary validate: true from belongs_to :project'
|
|
||||||
merge_request:
|
|
||||||
author:
|
|
||||||
type: fixed
|
|
|
@ -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
|
|
@ -56,7 +56,8 @@ for initial settings.
|
||||||
"name" => "auth0",
|
"name" => "auth0",
|
||||||
"args" => { client_id: 'YOUR_AUTH0_CLIENT_ID',
|
"args" => { client_id: 'YOUR_AUTH0_CLIENT_ID',
|
||||||
client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
|
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: {
|
args: {
|
||||||
client_id: 'YOUR_AUTH0_CLIENT_ID',
|
client_id: 'YOUR_AUTH0_CLIENT_ID',
|
||||||
client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
|
client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
|
||||||
namespace: 'YOUR_AUTH0_DOMAIN'
|
domain: 'YOUR_AUTH0_DOMAIN',
|
||||||
}
|
scope: 'openid profile email' }
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
|
|
|
@ -3,73 +3,90 @@ require 'spec_helper'
|
||||||
describe OmniauthCallbacksController do
|
describe OmniauthCallbacksController do
|
||||||
include LoginHelpers
|
include LoginHelpers
|
||||||
|
|
||||||
let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: provider) }
|
let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) }
|
||||||
let(:provider) { :github }
|
|
||||||
|
|
||||||
before do
|
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)
|
stub_omniauth_provider(provider, context: request)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'allows sign in' do
|
context 'github' do
|
||||||
post provider
|
let(:extern_uid) { 'my-uid' }
|
||||||
|
let(:provider) { :github }
|
||||||
|
|
||||||
expect(request.env['warden']).to be_authenticated
|
it 'allows sign in' do
|
||||||
end
|
|
||||||
|
|
||||||
shared_context 'sign_up' do
|
|
||||||
let(:user) { double(email: 'new@example.com') }
|
|
||||||
|
|
||||||
before do
|
|
||||||
stub_omniauth_setting(block_auto_created_users: false)
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
context 'sign up' do
|
|
||||||
include_context 'sign_up'
|
|
||||||
|
|
||||||
it 'is allowed' do
|
|
||||||
post provider
|
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
|
shared_context 'sign_up' do
|
||||||
before do
|
let(:user) { double(email: 'new@example.com') }
|
||||||
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
|
before do
|
||||||
post provider
|
stub_omniauth_setting(block_auto_created_users: false)
|
||||||
|
end
|
||||||
expect(request.env['warden']).not_to be_authenticated
|
|
||||||
end
|
|
||||||
|
|
||||||
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
|
|
||||||
|
|
||||||
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
|
end
|
||||||
|
|
||||||
context 'sign up' do
|
context 'sign up' do
|
||||||
include_context 'sign_up'
|
include_context 'sign_up'
|
||||||
|
|
||||||
it 'is prevented' do
|
it 'is allowed' do
|
||||||
|
post provider
|
||||||
|
|
||||||
|
expect(request.env['warden']).to be_authenticated
|
||||||
|
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
|
||||||
|
|
||||||
|
it 'prevents login via POST' do
|
||||||
post provider
|
post provider
|
||||||
|
|
||||||
expect(request.env['warden']).not_to be_authenticated
|
expect(request.env['warden']).not_to be_authenticated
|
||||||
end
|
end
|
||||||
|
|
||||||
|
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
|
||||||
|
|
||||||
|
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
|
||||||
|
|
||||||
|
context 'sign up' do
|
||||||
|
include_context 'sign_up'
|
||||||
|
|
||||||
|
it 'is prevented' do
|
||||||
|
post provider
|
||||||
|
|
||||||
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -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
|
Loading…
Reference in New Issue