Merge branch 'otzy007/gitlab-ce-disable_oauth_sign_in_sources'

14 files changed, 130 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 777b211daab..efb6dc6f610 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -46,6 +46,7 @@ v 8.8.0 (unreleased)
   - Fix adding a todo for private group members (Ahmad Sherif)
   - Bump ace-rails-ap gem version from 2.0.1 to 4.0.2 which upgrades Ace Editor from 1.1.2 to 1.2.3
   - Total method execution timings are no longer tracked
+  - Allow Admins to remove the Login with buttons for OAuth services and still be able to import !4034. (Andrei Gliga)
 
 v 8.7.5
   - Fix relative links in wiki pages. !4050
diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb
index ec22548ddeb..b9eb7ae7921 100644
--- a/app/controllers/admin/application_settings_controller.rb
+++ b/app/controllers/admin/application_settings_controller.rb
@@ -53,6 +53,12 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       end
     end
 
+    enabled_oauth_sign_in_sources = params[:application_setting].delete(:enabled_oauth_sign_in_sources)
+
+    params[:application_setting][:disabled_oauth_sign_in_sources] =
+      AuthHelper.button_based_providers.map(&:to_s) -
+      Array(enabled_oauth_sign_in_sources)
+
     params.require(:application_setting).permit(
       :default_projects_limit,
       :default_branch_protection,
@@ -95,7 +101,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :repository_checks_enabled,
       :metrics_packet_size,
       restricted_visibility_levels: [],
-      import_sources: []
+      import_sources: [],
+      disabled_oauth_sign_in_sources: []
     )
   end
 end
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index 914b0ef6042..03080d25931 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -60,4 +60,18 @@ module ApplicationSettingsHelper
       end
     end
   end
+
+  def oauth_providers_checkboxes
+    button_based_providers.map do |source|
+      disabled = current_application_settings.disabled_oauth_sign_in_sources.include?(source.to_s)
+      css_class = 'btn'
+      css_class << ' active' unless disabled
+      checkbox_name = 'application_setting[enabled_oauth_sign_in_sources][]'
+
+      label_tag(checkbox_name, class: css_class) do
+        check_box_tag(checkbox_name, source, !disabled,
+                      autocomplete: 'off') + Gitlab::OAuth::Provider.label_for(source)
+      end
+    end
+  end
 end
diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index b4f80fd9b3e..b05fa0a14d6 100644
--- a/app/helpers/auth_helper.rb
+++ b/app/helpers/auth_helper.rb
@@ -38,6 +38,16 @@ module AuthHelper
     auth_providers.reject { |provider| form_based_provider?(provider) }
   end
 
+  def enabled_button_based_providers
+    disabled_providers = current_application_settings.disabled_oauth_sign_in_sources || []
+
+    button_based_providers.map(&:to_s) - disabled_providers
+  end
+
+  def button_based_providers_enabled?
+    enabled_button_based_providers.any?
+  end
+
   def provider_image_tag(provider, size = 64)
     label = label_for_provider(provider)
 
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 7039db2d41e..c143cf215e6 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -6,6 +6,7 @@ class ApplicationSetting < ActiveRecord::Base
 
   serialize :restricted_visibility_levels
   serialize :import_sources
+  serialize :disabled_oauth_sign_in_sources
   serialize :restricted_signup_domains, Array
   attr_accessor :restricted_signup_domains_raw
 
@@ -69,6 +70,16 @@ class ApplicationSetting < ActiveRecord::Base
     end
   end
 
+  validates_each :disabled_oauth_sign_in_sources do |record, attr, value|
+    unless value.nil?
+      value.each do |source|
+        unless Devise.omniauth_providers.include?(source.to_sym)
+          record.errors.add(attr, "'#{source}' is not an OAuth sign-in source")
+        end
+      end
+    end
+  end
+
   before_save :ensure_runners_registration_token
 
   after_commit do
@@ -107,6 +118,7 @@ class ApplicationSetting < ActiveRecord::Base
       recaptcha_enabled: false,
       akismet_enabled: false,
       repository_checks_enabled: true,
+      disabled_oauth_sign_in_sources: []
     )
   end
 
diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml
index e0d8d16a954..f7c799c968f 100644
--- a/app/views/admin/application_settings/_form.html.haml
+++ b/app/views/admin/application_settings/_form.html.haml
@@ -109,6 +109,13 @@
           = f.label :signin_enabled do
             = f.check_box :signin_enabled
             Sign-in enabled
+    - if omniauth_enabled? && button_based_providers.any?
+      .form-group
+        = f.label :enabled_oauth_sign_in_sources, 'Enabled OAuth Sign-In sources', class: 'control-label col-sm-2'
+        .col-sm-10
+          .btn-group{ data: { toggle: 'buttons' } }
+            - oauth_providers_checkboxes.each do |source|
+              = source
     .form-group
       = f.label :two_factor_authentication, 'Two-factor authentication', class: 'control-label col-sm-2'
       .col-sm-10
diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml
index d65fa60025c..28194506acc 100644
--- a/app/views/devise/sessions/new.html.haml
+++ b/app/views/devise/sessions/new.html.haml
@@ -4,7 +4,7 @@
     = render 'devise/shared/signin_box'
 
   -# Omniauth fits between signin/ldap signin and signup and does not have a surrounding box
-  - if omniauth_enabled? && devise_mapping.omniauthable?
+  - if omniauth_enabled? && devise_mapping.omniauthable? && button_based_providers_enabled?
     .clearfix.prepend-top-20
       = render 'devise/shared/omniauth_box'
 
diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml
index ecf680e7b23..de18bc2d844 100644
--- a/app/views/devise/shared/_omniauth_box.html.haml
+++ b/app/views/devise/shared/_omniauth_box.html.haml
@@ -1,7 +1,7 @@
 %p
   %span.light
     Sign in with &nbsp;
-  - providers = button_based_providers
+  - providers = enabled_button_based_providers
   - providers.each do |provider|
     %span.light
       - has_icon = provider_has_icon?(provider)
diff --git a/db/migrate/20160504091942_add_disabled_oauth_sign_in_sources_to_application_settings.rb b/db/migrate/20160504091942_add_disabled_oauth_sign_in_sources_to_application_settings.rb
new file mode 100644
index 00000000000..facd33875ba
--- /dev/null
+++ b/db/migrate/20160504091942_add_disabled_oauth_sign_in_sources_to_application_settings.rb
@@ -0,0 +1,5 @@
+class AddDisabledOauthSignInSourcesToApplicationSettings < ActiveRecord::Migration
+  def change
+    add_column :application_settings, :disabled_oauth_sign_in_sources, :text
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 71d953afe30..74facd12088 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -80,6 +80,7 @@ ActiveRecord::Schema.define(version: 20160508194200) do
     t.boolean  "repository_checks_enabled",         default: false
     t.text     "shared_runners_text"
     t.integer  "metrics_packet_size",               default: 1
+    t.text     "disabled_oauth_sign_in_sources"
   end
 
   create_table "audit_events", force: :cascade do |t|
diff --git a/doc/integration/img/enabled-oauth-sign-in-sources.png b/doc/integration/img/enabled-oauth-sign-in-sources.png
new file mode 100644
index 00000000000..95f8bbdcd24
--- /dev/null
+++ b/doc/integration/img/enabled-oauth-sign-in-sources.png
diff --git a/doc/integration/omniauth.md b/doc/integration/omniauth.md
index cab329c0dec..820f40f81a9 100644
--- a/doc/integration/omniauth.md
+++ b/doc/integration/omniauth.md
@@ -11,6 +11,7 @@ of the configured mechanisms.
 - [Supported Providers](#supported-providers)
 - [Enable OmniAuth for an Existing User](#enable-omniauth-for-an-existing-user)
 - [OmniAuth configuration sample when using Omnibus GitLab](https://gitlab.com/gitlab-org/omnibus-gitlab/tree/master#omniauth-google-twitter-github-login)
+- [Enable or disable Sign In with an OmniAuth provider without disabling import sources](#enable-or-disable-sign-in-with-an-omniauth-provider-without-disabling-import-sources)
 
 ## Supported Providers
 
@@ -191,3 +192,17 @@ experience [in the public Wiki](https://github.com/gitlabhq/gitlab-public-wiki/w
 
 While we can't officially support every possible authentication mechanism out there,
 we'd like to at least help those with specific needs.
+
+## Enable or disable Sign In with an OmniAuth provider without disabling import sources
+
+>**Note:**
+This setting was introduced with version 8.8 of GitLab.
+
+Administrators are able to enable or disable Sign In via some OmniAuth providers.
+
+>**Note:**
+By default Sign In is enabled via all the OAuth Providers that have been configured in `config/gitlab.yml`.
+
+In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings -> Sign-in Restrictions section -> Enabled OAuth Sign-In sources and select the providers you want to enable or disable.
+
+![Enabled OAuth Sign-In sources](img/enabled-oauth-sign-in-sources.png)
diff --git a/spec/helpers/auth_helper_spec.rb b/spec/helpers/auth_helper_spec.rb
index e47a54fdac5..16fbb5dcecb 100644
--- a/spec/helpers/auth_helper_spec.rb
+++ b/spec/helpers/auth_helper_spec.rb
@@ -2,7 +2,7 @@ require "spec_helper"
 
 describe AuthHelper do
   describe "button_based_providers" do
-    it 'returns all enabled providers' do
+    it 'returns all enabled providers from devise' do
       allow(helper).to receive(:auth_providers) { [:twitter, :github] }
       expect(helper.button_based_providers).to include(*[:twitter, :github])
     end
@@ -17,4 +17,49 @@ describe AuthHelper do
       expect(helper.button_based_providers).to eq([])
     end
   end
+
+  describe 'enabled_button_based_providers' do
+    before do
+      allow(helper).to receive(:auth_providers) { [:twitter, :github] }
+    end
+
+    context 'all providers are enabled to sign in' do
+      it 'returns all the enabled providers from settings' do
+        expect(helper.enabled_button_based_providers).to include('twitter', 'github')
+      end
+    end
+
+    context 'GitHub OAuth sign in is disabled from application setting' do
+      it "doesn't return github as provider" do
+        stub_application_setting(
+          disabled_oauth_sign_in_sources: ['github']
+        )
+
+        expect(helper.enabled_button_based_providers).to include('twitter')
+        expect(helper.enabled_button_based_providers).to_not include('github')
+      end
+    end
+  end
+
+  describe 'button_based_providers_enabled?' do
+    before do
+      allow(helper).to receive(:auth_providers) { [:twitter, :github] }
+    end
+
+    context 'button based providers enabled' do
+      it 'returns true' do
+        expect(helper.button_based_providers_enabled?).to be true
+      end
+    end
+
+    context 'all the button based providers are disabled via application_setting' do
+      it 'returns false' do
+        stub_application_setting(
+          disabled_oauth_sign_in_sources: ['github', 'twitter']
+        )
+
+        expect(helper.button_based_providers_enabled?).to be false
+      end
+    end
+  end
 end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 1ce22feed5c..d84f3e998f5 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -20,6 +20,15 @@ describe ApplicationSetting, models: true do
     it { is_expected.to allow_value(https).for(:after_sign_out_path) }
     it { is_expected.not_to allow_value(ftp).for(:after_sign_out_path) }
 
+    describe 'disabled_oauth_sign_in_sources validations' do
+      before do
+        allow(Devise).to receive(:omniauth_providers).and_return([:github])
+      end
+
+      it { is_expected.to allow_value(['github']).for(:disabled_oauth_sign_in_sources) }
+      it { is_expected.not_to allow_value(['test']).for(:disabled_oauth_sign_in_sources) }
+    end
+
     it { is_expected.to validate_presence_of(:max_attachment_size) }
 
     it do