From d250b1d2155eca1779a84668da40d2b273188254 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 7 Dec 2017 18:42:25 +0000 Subject: Merge branch 'jej/fix-disabled-oauth-access' into 'security-10-2' Prevent login with disabled OAuth providers See merge request gitlab/gitlabhq!2223 (cherry picked from commit 43b6135f2226625b5e50d9aa2149a0ea74bb1336) a4bb4a5b Prevents login with disabled OAuth providers --- app/controllers/omniauth_callbacks_controller.rb | 9 +++ .../unreleased/jej-fix-disabled-oauth-access.yml | 5 ++ lib/gitlab/o_auth.rb | 6 ++ lib/gitlab/o_auth/user.rb | 11 ++-- .../omniauth_callbacks_controller_spec.rb | 75 ++++++++++++++++++++++ spec/features/oauth_login_spec.rb | 3 +- spec/support/devise_helpers.rb | 15 +++-- spec/support/login_helpers.rb | 7 ++ 8 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/jej-fix-disabled-oauth-access.yml create mode 100644 lib/gitlab/o_auth.rb create mode 100644 spec/controllers/omniauth_callbacks_controller_spec.rb diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 9612b8d8514..b87348fe4e6 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -108,6 +108,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController continue_login_process end + rescue Gitlab::OAuth::SigninDisabledForProviderError + handle_disabled_provider rescue Gitlab::OAuth::SignupDisabledError handle_signup_error end @@ -163,6 +165,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 diff --git a/changelogs/unreleased/jej-fix-disabled-oauth-access.yml b/changelogs/unreleased/jej-fix-disabled-oauth-access.yml new file mode 100644 index 00000000000..3a92c432dc6 --- /dev/null +++ b/changelogs/unreleased/jej-fix-disabled-oauth-access.yml @@ -0,0 +1,5 @@ +--- +title: Prevent OAuth login POST requests when a provider has been disabled +merge_request: +author: +type: security diff --git a/lib/gitlab/o_auth.rb b/lib/gitlab/o_auth.rb new file mode 100644 index 00000000000..5ad8d83bd6e --- /dev/null +++ b/lib/gitlab/o_auth.rb @@ -0,0 +1,6 @@ +module Gitlab + module OAuth + SignupDisabledError = Class.new(StandardError) + SigninDisabledForProviderError = Class.new(StandardError) + end +end diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index b4b3b00c84d..e231d92ca91 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -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? @@ -224,8 +223,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 diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb new file mode 100644 index 00000000000..c639ad32ec6 --- /dev/null +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -0,0 +1,75 @@ +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 diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 49d8e52f861..a5e325ee2e3 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -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, diff --git a/spec/support/devise_helpers.rb b/spec/support/devise_helpers.rb index 890a2d9d287..66874e10f38 100644 --- a/spec/support/devise_helpers.rb +++ b/spec/support/devise_helpers.rb @@ -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 = - if context.respond_to?(:env_config) - context.env_config - elsif context.respond_to?(:env) - context.env - end + 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 + end end diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 50702a0ac88..b52b6a28c54 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -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 -- cgit v1.2.1