Merge branch 'jej/prevent-ldap-sign-in' into 'master'

Option to prevent LDAP sign in

Closes #15626

See merge request gitlab-org/gitlab!18749

app/controllers/ldap/omniauth_callbacks_controller.rb

@@ -4,7 +4,7 @@ class Ldap::OmniauthCallbacksController < OmniauthCallbacksController
   extend ::Gitlab::Utils::Override
 
   def self.define_providers!
-    return unless Gitlab::Auth::LDAP::Config.enabled?
+    return unless Gitlab::Auth::LDAP::Config.sign_in_enabled?
 
     Gitlab::Auth::LDAP::Config.available_servers.each do |server|
       alias_method server['provider_name'], :ldap
@@ -14,6 +14,8 @@ class Ldap::OmniauthCallbacksController < OmniauthCallbacksController
   # We only find ourselves here
   # if the authentication to LDAP was successful.
   def ldap
+    return unless Gitlab::Auth::LDAP::Config.sign_in_enabled?
+
     sign_in_user_flow(Gitlab::Auth::LDAP::User)
   end
 

app/controllers/sessions_controller.rb

@@ -270,7 +270,13 @@ class SessionsController < Devise::SessionsController
   end
 
   def ldap_servers
-    @ldap_servers ||= Gitlab::Auth::LDAP::Config.available_servers
+    @ldap_servers ||= begin
+      if Gitlab::Auth::LDAP::Config.sign_in_enabled?
+        Gitlab::Auth::LDAP::Config.available_servers
+      else
+        []
+      end
+    end
   end
 
   def unverified_anonymous_user?

app/helpers/auth_helper.rb

@@ -8,6 +8,10 @@ module AuthHelper
     Gitlab::Auth::LDAP::Config.enabled?
   end
 
+  def ldap_sign_in_enabled?
+    Gitlab::Auth::LDAP::Config.sign_in_enabled?
+  end
+
   def omniauth_enabled?
     Gitlab::Auth.omniauth_enabled?
   end
@@ -56,6 +60,16 @@ module AuthHelper
     auth_providers.select { |provider| form_based_provider?(provider) }
   end
 
+  def any_form_based_providers_enabled?
+    form_based_providers.any? { |provider| form_enabled_for_sign_in?(provider) }
+  end
+
+  def form_enabled_for_sign_in?(provider)
+    return true unless provider.to_s.match?(LDAP_PROVIDER)
+
+    ldap_sign_in_enabled?
+  end
+
   def crowd_enabled?
     auth_providers.include? :crowd
   end

app/views/admin/sessions/_signin_box.html.haml

-- if form_based_providers.any?
+- if any_form_based_providers_enabled?
 
   - if password_authentication_enabled_for_web?
     .login-box.tab-pane{ id: 'login-pane', role: 'tabpanel' }

app/views/devise/sessions/new.html.haml

 - page_title "Sign in"
 
 #signin-container
-  - if form_based_providers.any?
+  - if any_form_based_providers_enabled?
     = render 'devise/shared/tabs_ldap'
   - else
     - unless experiment_enabled?(:signup_flow)
       = render 'devise/shared/tabs_normal'
   .tab-content
-    - if password_authentication_enabled_for_web? || ldap_enabled? || crowd_enabled?
+    - if password_authentication_enabled_for_web? || ldap_sign_in_enabled? || crowd_enabled?
       = render 'devise/shared/signin_box'
 
     -# Signup only makes sense if you can also sign-in
@@ -15,7 +15,7 @@
       = render 'devise/shared/signup_box'
 
   -# Show a message if none of the mechanisms above are enabled
-  - if !password_authentication_enabled_for_web? && !ldap_enabled? && !(omniauth_enabled? && devise_mapping.omniauthable?)
+  - if !password_authentication_enabled_for_web? && !ldap_sign_in_enabled? && !(omniauth_enabled? && devise_mapping.omniauthable?)
     %div
       No authentication methods configured.
 

app/views/devise/shared/_signin_box.html.haml

-- if form_based_providers.any?
+- if any_form_based_providers_enabled?
   - if crowd_enabled?
     .login-box.tab-pane{ id: "crowd", role: 'tabpanel', class: active_when(form_based_auth_provider_has_active_class?(:crowd)) }
       .login-body

app/views/devise/shared/_tabs_ldap.html.haml

-%ul.nav-links.new-session-tabs.nav-tabs.nav{ class: ('custom-provider-tabs' if form_based_providers.any?) }
+%ul.nav-links.new-session-tabs.nav-tabs.nav{ class: ('custom-provider-tabs' if any_form_based_providers_enabled?) }
   - if crowd_enabled?
     %li.nav-item
       = link_to "Crowd", "#crowd", class: "nav-link #{active_when(form_based_auth_provider_has_active_class?(:crowd))}", 'data-toggle' => 'tab'

changelogs/unreleased/jej-prevent-ldap-sign-in.yml

+---
+title: Add prevent_ldap_sign_in option so LDAP can be used exclusively for sync
+merge_request: 18749
+author:
+type: added

config/gitlab.yml.example

@@ -494,6 +494,7 @@ production: &base
   #   bundle exec rake gitlab:ldap:check RAILS_ENV=production
   ldap:
     enabled: false
+    prevent_ldap_sign_in: false
 
     # This setting controls the number of seconds between LDAP permission checks
     # for each user. After this time has expired for a given user, their next

config/initializers/1_settings.rb

@@ -5,6 +5,7 @@ require_relative '../smime_signature_settings'
 # Default settings
 Settings['ldap'] ||= Settingslogic.new({})
 Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil?
+Settings.ldap['prevent_ldap_sign_in'] = false if Settings.ldap['prevent_ldap_sign_in'].blank?
 
 Gitlab.ee do
   Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil?

config/routes/user.rb

@@ -13,7 +13,7 @@ def override_omniauth(provider, controller, path_prefix = '/users/auth')
 end
 
 # Use custom controller for LDAP omniauth callback
-if Gitlab::Auth::LDAP::Config.enabled?
+if Gitlab::Auth::LDAP::Config.sign_in_enabled?
   devise_scope :user do
     Gitlab::Auth::LDAP::Config.available_servers.each do |server|
       override_omniauth(server['provider_name'], 'ldap/omniauth_callbacks')

doc/administration/auth/ldap.md

@@ -118,6 +118,7 @@ LDAP users must have an email address set, regardless of whether it is used to l
 
 ```ruby
 gitlab_rails['ldap_enabled'] = true
+gitlab_rails['prevent_ldap_sign_in'] = false
 gitlab_rails['ldap_servers'] = YAML.load <<-EOS # remember to close this block with 'EOS' below
 ##
 ## 'main' is the GitLab 'provider ID' of this LDAP server
@@ -357,6 +358,7 @@ production:
   # snip...
   ldap:
     enabled: false
+    prevent_ldap_sign_in: false
     servers:
       ##
       ## 'main' is the GitLab 'provider ID' of this LDAP server
@@ -493,6 +495,38 @@ the configuration option `lowercase_usernames`. By default, this configuration o
 
 1. [Restart GitLab](../restart_gitlab.md#installations-from-source) for the changes to take effect.
 
+## Disable LDAP web sign in
+
+It can be be useful to prevent using LDAP credentials through the web UI when
+an alternative such as SAML is preferred. This allows LDAP to be used for group
+sync, while also allowing your SAML identity provider to handle additional
+checks like custom 2FA.
+
+When LDAP web sign in is disabled, users will not see a **LDAP** tab on the sign in page.
+This does not disable [using LDAP credentials for Git access](#git-password-authentication).
+
+**Omnibus configuration**
+
+1. Edit `/etc/gitlab/gitlab.rb`:
+
+   ```ruby
+   gitlab_rails['prevent_ldap_sign_in'] = true
+   ```
+
+1. [Reconfigure GitLab](../restart_gitlab.md#omnibus-gitlab-reconfigure) for the changes to take effect.
+
+**Source configuration**
+
+1. Edit `config/gitlab.yaml`:
+
+   ```yaml
+   production:
+     ldap:
+       prevent_ldap_sign_in: true
+   ```
+
+1. [Restart GitLab](../restart_gitlab.md#installations-from-source) for the changes to take effect.
+
 ## Encryption
 
 ### TLS Server Authentication

ee/app/views/trial_registrations/_signin_box.html.haml

-- if form_based_providers.any?
+- if any_form_based_providers_enabled?
   - if crowd_enabled?
     .login-box.tab-pane{ id: "crowd", role: 'tabpanel', class: active_when(form_based_auth_provider_has_active_class?(:crowd)) }
       .login-body

ee/app/views/trial_registrations/new.html.haml

@@ -4,12 +4,12 @@
   = _('Start a Free Trial')
 
 %div
-  - if form_based_providers.any?
+  - if any_form_based_providers_enabled?
     = render 'devise/shared/tabs_ldap'
   - else
     = render 'devise/shared/tabs_normal'
   .tab-content
-    - if password_authentication_enabled_for_web? || ldap_enabled? || crowd_enabled?
+    - if password_authentication_enabled_for_web? || ldap_sign_in_enabled? || crowd_enabled?
       = render 'signin_box'
 
     -# Signup only makes sense if you can also sign-in
@@ -17,6 +17,6 @@
       = render 'signup_box', user: resource
 
   -# Show a message if none of the mechanisms above are enabled
-  - if !password_authentication_enabled_for_web? && !ldap_enabled? && !(omniauth_enabled? && devise_mapping.omniauthable?)
+  - if !password_authentication_enabled_for_web? && !ldap_sign_in_enabled? && !(omniauth_enabled? && devise_mapping.omniauthable?)
     %div
       = _('No authentication methods configured.')

lib/gitlab/auth/ldap/config.rb

@@ -21,6 +21,14 @@ module Gitlab
           Gitlab.config.ldap.enabled
         end
 
+        def self.sign_in_enabled?
+          enabled? && !prevent_ldap_sign_in?
+        end
+
+        def self.prevent_ldap_sign_in?
+          Gitlab.config.ldap.prevent_ldap_sign_in
+        end
+
         def self.servers
           Gitlab.config.ldap['servers']&.values || []
         end

spec/controllers/ldap/omniauth_callbacks_controller_spec.rb

@@ -11,6 +11,14 @@ describe Ldap::OmniauthCallbacksController do
     expect(request.env['warden']).to be_authenticated
   end
 
+  context 'with sign in prevented' do
+    let(:ldap_settings) { ldap_setting_defaults.merge(prevent_ldap_sign_in: true) }
+
+    it 'does not allow sign in' do
+      expect { post provider }.to raise_error(ActionController::UrlGenerationError)
+    end
+  end
+
   it 'respects remember me checkbox' do
     expect do
       post provider, params: { remember_me: '1' }

spec/controllers/sessions_controller_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe SessionsController do
   include DeviseHelpers
+  include LdapHelpers
 
   describe '#new' do
     before do
@@ -35,6 +36,30 @@ describe SessionsController do
       end
     end
 
+    context 'with LDAP enabled' do
+      before do
+        stub_ldap_setting(enabled: true)
+      end
+
+      it 'assigns ldap_servers' do
+        get(:new)
+
+        expect(assigns[:ldap_servers].first.to_h).to include('label' => 'ldap', 'provider_name' => 'ldapmain')
+      end
+
+      context 'with sign_in disabled' do
+        before do
+          stub_ldap_setting(prevent_ldap_sign_in: true)
+        end
+
+        it 'assigns no ldap_servers' do
+          get(:new)
+
+          expect(assigns[:ldap_servers]).to eq []
+        end
+      end
+    end
+
     describe 'tracking data' do
       context 'when the user is part of the experimental group' do
         before do

spec/helpers/auth_helper_spec.rb

@@ -54,6 +54,23 @@ describe AuthHelper do
     end
   end
 
+  describe 'any_form_based_providers_enabled?' do
+    before do
+      allow(Gitlab::Auth::LDAP::Config).to receive(:enabled?).and_return(true)
+    end
+
+    it 'detects form-based providers' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] }
+      expect(helper.any_form_based_providers_enabled?).to be(true)
+    end
+
+    it 'ignores ldap providers when ldap web sign in is disabled' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] }
+      allow(helper).to receive(:ldap_sign_in_enabled?).and_return(false)
+      expect(helper.any_form_based_providers_enabled?).to be(false)
+    end
+  end
+
   describe 'enabled_button_based_providers' do
     before do
       allow(helper).to receive(:auth_providers) { [:twitter, :github] }

spec/lib/gitlab/auth/ldap/config_spec.rb

@@ -535,4 +535,23 @@ AtlErSqafbECNDSwS5BX8yDpu5yRBJ4xegO/rNlmb8ICRYkuJapD1xXicFOsmfUK
       end
     end
   end
+
+  describe 'sign_in_enabled?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:enabled, :prevent_ldap_sign_in, :result) do
+      true | false | true
+      'true' | false | true
+      true | true | false
+      false | nil | false
+    end
+
+    with_them do
+      it do
+        stub_ldap_setting(enabled: enabled, prevent_ldap_sign_in: prevent_ldap_sign_in)
+
+        expect(described_class.sign_in_enabled?).to eq(result)
+      end
+    end
+  end
 end

spec/routing/routing_spec.rb

@@ -277,6 +277,33 @@ describe "Authentication", "routing" do
   it "PUT /users/password" do
     expect(put("/users/password")).to route_to('passwords#update')
   end
+
+  context 'with LDAP configured' do
+    include LdapHelpers
+
+    let(:ldap_settings) { { enabled: true } }
+
+    before do
+      stub_ldap_setting(ldap_settings)
+      Rails.application.reload_routes!
+    end
+
+    after(:all) do
+      Rails.application.reload_routes!
+    end
+
+    it 'POST /users/auth/ldapmain/callback' do
+      expect(post("/users/auth/ldapmain/callback")).to route_to('ldap/omniauth_callbacks#ldapmain')
+    end
+
+    context 'with LDAP sign-in disabled' do
+      let(:ldap_settings) { { enabled: true, prevent_ldap_sign_in: true } }
+
+      it 'prevents POST /users/auth/ldapmain/callback' do
+        expect(post("/users/auth/ldapmain/callback")).not_to be_routable
+      end
+    end
+  end
 end
 
 describe HealthCheckController, 'routing' do

spec/support/controllers/ldap_omniauth_callbacks_controller_shared_context.rb

@@ -10,6 +10,8 @@ shared_context 'Ldap::OmniauthCallbacksController' do
   let(:provider) { 'ldapmain' }
   let(:valid_login?) { true }
   let(:user) { create(:omniauth_user, extern_uid: uid, provider: provider) }
+  let(:ldap_setting_defaults) { { enabled: true, servers: ldap_server_config } }
+  let(:ldap_settings) { ldap_setting_defaults }
   let(:ldap_server_config) do
     { main: ldap_config_defaults(:main) }
   end
@@ -23,7 +25,7 @@ shared_context 'Ldap::OmniauthCallbacksController' do
   end
 
   before do
-    stub_ldap_setting(enabled: true, servers: ldap_server_config)
+    stub_ldap_setting(ldap_settings)
     described_class.define_providers!
     Rails.application.reload_routes!
 
@@ -36,4 +38,8 @@ shared_context 'Ldap::OmniauthCallbacksController' do
   after do
     Rails.application.env_config['omniauth.auth'] = @original_env_config_omniauth_auth
   end
+
+  after(:all) do
+    Rails.application.reload_routes!
+  end
 end

spec/views/devise/sessions/new.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'devise/sessions/new' do
+  describe 'ldap' do
+    include LdapHelpers
+
+    let(:server) { { provider_name: 'ldapmain', label: 'LDAP' }.with_indifferent_access }
+
+    before do
+      enable_ldap
+      stub_devise
+      disable_captcha
+      disable_sign_up
+      disable_other_signin_methods
+
+      allow(view).to receive(:experiment_enabled?).and_return(false)
+    end
+
+    it 'is shown when enabled' do
+      render
+
+      expect(rendered).to have_selector('.new-session-tabs')
+      expect(rendered).to have_selector('[data-qa-selector="ldap_tab"]')
+      expect(rendered).to have_field('LDAP Username')
+    end
+
+    it 'is not shown when LDAP sign in is disabled' do
+      disable_ldap_sign_in
+
+      render
+
+      expect(rendered).to have_content('No authentication methods configured')
+      expect(rendered).not_to have_selector('[data-qa-selector="ldap_tab"]')
+      expect(rendered).not_to have_field('LDAP Username')
+    end
+  end
+
+  def disable_other_signin_methods
+    allow(view).to receive(:password_authentication_enabled_for_web?).and_return(false)
+    allow(view).to receive(:omniauth_enabled?).and_return(false)
+  end
+
+  def disable_sign_up
+    allow(view).to receive(:allow_signup?).and_return(false)
+  end
+
+  def stub_devise
+    allow(view).to receive(:devise_mapping).and_return(Devise.mappings[:user])
+    allow(view).to receive(:resource).and_return(spy)
+    allow(view).to receive(:resource_name).and_return(:user)
+  end
+
+  def enable_ldap
+    stub_ldap_setting(enabled: true)
+    assign(:ldap_servers, [server])
+    allow(view).to receive(:form_based_providers).and_return([:ldapmain])
+    allow(view).to receive(:omniauth_callback_path).with(:user, 'ldapmain').and_return('/ldapmain')
+  end
+
+  def disable_ldap_sign_in
+    allow(view).to receive(:ldap_sign_in_enabled?).and_return(false)
+    assign(:ldap_servers, [])
+  end
+
+  def disable_captcha
+    allow(view).to receive(:captcha_enabled?).and_return(false)
+    allow(view).to receive(:captcha_on_login_required?).and_return(false)
+  end
+end