Commit 4493ec08 authored by Robert Speicher's avatar Robert Speicher Committed by Stan Hu

Merge branch 'jej/fix-disabled-oauth-access-10-3' into 'security-10-3'

[10.3] Prevent login with disabled OAuth providers

See merge request gitlab/gitlabhq!2296

(cherry picked from commit 4936650427ffc88e6ee927aedbb2c724d24b094c)

a0f9d222 Prevents login with disabled OAuth providers
parent 54636e1d
......@@ -112,6 +112,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
continue_login_process
end
rescue Gitlab::OAuth::SigninDisabledForProviderError
handle_disabled_provider
rescue Gitlab::OAuth::SignupDisabledError
handle_signup_error
end
......@@ -168,6 +170,13 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
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"
redirect_to new_user_session_path
end
def log_audit_event(user, options = {})
AuditEventService.new(user, user, options)
.for_authentication.security_event
......
---
title: Prevent OAuth login POST requests when a provider has been disabled
merge_request:
author:
type: security
module Gitlab
module OAuth
SignupDisabledError = Class.new(StandardError)
SigninDisabledForProviderError = Class.new(StandardError)
end
end
......@@ -5,8 +5,6 @@
#
module Gitlab
module OAuth
SignupDisabledError = Class.new(StandardError)
class User
attr_accessor :auth_hash, :gl_user
......@@ -29,7 +27,8 @@ module Gitlab
end
def save(provider = 'OAuth')
unauthorized_to_create unless gl_user
raise SigninDisabledForProviderError if oauth_provider_disabled?
raise SignupDisabledError unless gl_user
block_after_save = needs_blocking?
......@@ -226,8 +225,10 @@ module Gitlab
Gitlab::AppLogger
end
def unauthorized_to_create
raise SignupDisabledError
def oauth_provider_disabled?
Gitlab::CurrentSettings.current_application_settings
.disabled_oauth_sign_in_sources
.include?(auth_hash.provider)
end
end
end
......
require 'spec_helper'
describe OmniauthCallbacksController do
include LoginHelpers
let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: provider) }
let(:provider) { :github }
before do
mock_auth_hash(provider.to_s, 'my-uid', user.email)
stub_omniauth_provider(provider, context: request)
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') }
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
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
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
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
......@@ -10,8 +10,7 @@ feature 'OAuth Login', :js, :allow_forgery_protection do
def stub_omniauth_config(provider)
OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new(provider: provider.to_s, uid: "12345"))
set_devise_mapping(context: Rails.application)
Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider]
stub_omniauth_provider(provider)
end
providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2,
......
......@@ -2,13 +2,16 @@ module DeviseHelpers
# explicitly tells Devise which mapping to use
# this is needed when we are testing a Devise controller bypassing the router
def set_devise_mapping(context:)
env =
env = env_from_context(context)
env['devise.mapping'] = Devise.mappings[:user] if env
end
def env_from_context(context)
if context.respond_to?(:env_config)
context.env_config
elsif context.respond_to?(:env)
context.env
end
env['devise.mapping'] = Devise.mappings[:user] if env
end
end
......@@ -125,6 +125,13 @@ module LoginHelpers
})
end
def stub_omniauth_provider(provider, context: Rails.application)
env = env_from_context(context)
set_devise_mapping(context: context)
env['omniauth.auth'] = OmniAuth.config.mock_auth[provider]
end
def stub_omniauth_saml_config(messages)
set_devise_mapping(context: Rails.application)
Rails.application.routes.disable_clear_and_finalize = true
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment