diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index c1c4a110aae4e61f602048e7f866f122cbd4804b..927b50245a40718dda9a788a5d2f37bd8e917bf5 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -9,7 +9,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
after_action :verify_known_sign_in
- protect_from_forgery except: [:saml, :cas3, :failure] + AuthHelper.saml_providers, with: :exception, prepend: true
+ protect_from_forgery except: [:cas3, :failure] + AuthHelper.saml_providers, with: :exception, prepend: true
feature_category :authentication_and_authorization
diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index 6ac4a12bcd5c1a137e3b3702a9e9baebc5f61fd4..07152133402bd2520bbc7af47a7c13ee9704ff7c 100644
--- a/app/helpers/auth_helper.rb
+++ b/app/helpers/auth_helper.rb
@@ -91,7 +91,9 @@ def form_based_providers
end
def saml_providers
- auth_providers.select { |provider| auth_strategy_class(provider) == 'OmniAuth::Strategies::SAML' }
+ auth_providers.select do |provider|
+ provider == :saml || auth_strategy_class(provider) == 'OmniAuth::Strategies::SAML'
+ end
end
def auth_strategy_class(provider)
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index fca1b6edb74853d5262c71f230af78994e95ed7a..c3fbf1db5e759941565d85529a8e237254970816 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -305,13 +305,21 @@ def saml_discovery_token
end
def saml_enabled?
- return false unless saml_provider
+ group_saml_enabled? || global_saml_enabled?
+ end
+
+ def saml_group_sync_available?
+ feature_available?(:saml_group_sync) && root_ancestor.saml_enabled?
+ end
+
+ def group_saml_enabled?
+ return false unless saml_provider && ::Gitlab::Auth::GroupSaml::Config.enabled?
saml_provider.persisted? && saml_provider.enabled?
end
- def saml_group_sync_available?
- feature_available?(:group_saml_group_sync) && root_ancestor.saml_enabled?
+ def global_saml_enabled?
+ ::Gitlab::Auth::Saml::Config.enabled?
end
override :multiple_issue_boards_available?
diff --git a/ee/app/models/gitlab_subscriptions/features.rb b/ee/app/models/gitlab_subscriptions/features.rb
index 79ccc7bf5f33416bdce14d82f504d9655719db2a..72bf6243de18812c76f7401d69a121ce6af43667 100644
--- a/ee/app/models/gitlab_subscriptions/features.rb
+++ b/ee/app/models/gitlab_subscriptions/features.rb
@@ -114,7 +114,6 @@ class Features
group_project_templates
group_repository_analytics
group_saml
- group_saml_group_sync
group_scoped_ci_variables
group_wikis
incident_sla
@@ -140,6 +139,7 @@ class Features
project_aliases
protected_environments
reject_unsigned_commits
+ saml_group_sync
scoped_labels
smartcard_auth
swimlanes
diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb
index ddc3780f29e4a9d947ac5c10ff55e241f1663ef8..5c4eb49d63289be6a5327ba3d4b07019907edc52 100644
--- a/ee/app/policies/ee/group_policy.rb
+++ b/ee/app/policies/ee/group_policy.rb
@@ -96,16 +96,24 @@ module GroupPolicy
@subject.feature_available?(:cluster_deployments)
end
- condition(:group_saml_config_enabled, scope: :global) do
- ::Gitlab::Auth::GroupSaml::Config.enabled?
+ condition(:global_saml_enabled, scope: :global) do
+ ::Gitlab::Auth::Saml::Config.enabled?
end
- condition(:group_saml_available, scope: :subject) do
- !@subject.subgroup? && @subject.feature_available?(:group_saml)
+ condition(:global_saml_group_sync_enabled, scope: :global) do
+ ::Gitlab::Auth::Saml::Config.group_sync_enabled?
+ end
+
+ condition(:group_saml_globally_enabled, scope: :global) do
+ ::Gitlab::Auth::GroupSaml::Config.enabled?
end
condition(:group_saml_enabled, scope: :subject) do
- @subject.saml_enabled?
+ @subject.group_saml_enabled?
+ end
+
+ condition(:group_saml_available, scope: :subject) do
+ !@subject.subgroup? && @subject.feature_available?(:group_saml)
end
condition(:group_saml_group_sync_available, scope: :subject) do
@@ -286,9 +294,13 @@ module GroupPolicy
enable :read_group_audit_events
end
- rule { group_saml_config_enabled & group_saml_available & (admin | owner) }.enable :admin_group_saml
+ rule { group_saml_globally_enabled & group_saml_available & (admin | owner) }.enable :admin_group_saml
+
+ rule { group_saml_group_sync_available & (admin | owner) }.policy do
+ enable :admin_saml_group_links
+ end
- rule { group_saml_config_enabled & group_saml_group_sync_available & (admin | owner) }.policy do
+ rule { global_saml_enabled & global_saml_group_sync_enabled & (admin | owner) }.policy do
enable :admin_saml_group_links
end
diff --git a/ee/lib/ee/gitlab/auth/saml/config.rb b/ee/lib/ee/gitlab/auth/saml/config.rb
index b5963d40f2452b29fb5ae1116c6ebf00e61a6a18..92752a492159573cef0e44cf58ec0203983a8793 100644
--- a/ee/lib/ee/gitlab/auth/saml/config.rb
+++ b/ee/lib/ee/gitlab/auth/saml/config.rb
@@ -15,6 +15,10 @@ def auditor_groups
def required_groups
Array(options[:required_groups])
end
+
+ def group_sync_enabled?
+ enabled? && groups.present? && ::License.feature_available?(:saml_group_sync)
+ end
end
end
end
diff --git a/ee/lib/ee/gitlab/auth/saml/identity_linker.rb b/ee/lib/ee/gitlab/auth/saml/identity_linker.rb
new file mode 100644
index 0000000000000000000000000000000000000000..bdfe9dd047f457cfacfc2a425635514a16f63e67
--- /dev/null
+++ b/ee/lib/ee/gitlab/auth/saml/identity_linker.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module EE
+ module Gitlab
+ module Auth
+ module Saml
+ module IdentityLinker
+ extend ::Gitlab::Utils::Override
+
+ override :link
+ def link
+ super
+
+ update_group_membership unless failed?
+ end
+
+ def update_group_membership
+ auth_hash = ::Gitlab::Auth::Saml::AuthHash.new(oauth)
+ ::Gitlab::Auth::Saml::MembershipUpdater.new(current_user, auth_hash).execute
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/ee/lib/ee/gitlab/auth/saml/user.rb b/ee/lib/ee/gitlab/auth/saml/user.rb
index 602c56e03216712dff2d3cfe75a3299106a66102..4fe0d3127d12cee0cc66f0638b5aa996d63b830e 100644
--- a/ee/lib/ee/gitlab/auth/saml/user.rb
+++ b/ee/lib/ee/gitlab/auth/saml/user.rb
@@ -29,6 +29,14 @@ def find_user
user
end
+ override :find_and_update!
+ def find_and_update!
+ save
+
+ update_group_membership
+ gl_user
+ end
+
protected
def block_user(user, reason)
@@ -53,6 +61,10 @@ def admin_groups_enabled?
def auditor_groups_enabled?
!saml_config.auditor_groups.blank?
end
+
+ def update_group_membership
+ ::Gitlab::Auth::Saml::MembershipUpdater.new(gl_user, auth_hash).execute
+ end
end
end
end
diff --git a/ee/lib/gitlab/auth/group_saml/identity_linker.rb b/ee/lib/gitlab/auth/group_saml/identity_linker.rb
index 5c7337dfcfd3f579395d0ac6ab48b139e5761da0..d5f11e74a0968f8ade34c0901292b11e8299976a 100644
--- a/ee/lib/gitlab/auth/group_saml/identity_linker.rb
+++ b/ee/lib/gitlab/auth/group_saml/identity_linker.rb
@@ -12,13 +12,6 @@ def initialize(current_user, oauth, session, saml_provider)
@saml_provider = saml_provider
end
- override :link
- def link
- super
-
- update_group_membership unless failed?
- end
-
protected
# rubocop: disable CodeReuse/ActiveRecord
@@ -30,6 +23,7 @@ def identity
end
# rubocop: enable CodeReuse/ActiveRecord
+ override :update_group_membership
def update_group_membership
auth_hash = AuthHash.new(oauth)
MembershipUpdater.new(current_user, saml_provider, auth_hash).execute
diff --git a/ee/lib/gitlab/auth/saml/membership_updater.rb b/ee/lib/gitlab/auth/saml/membership_updater.rb
new file mode 100644
index 0000000000000000000000000000000000000000..0e81b7bc2ac59463ca2546deeb51b7892dad3156
--- /dev/null
+++ b/ee/lib/gitlab/auth/saml/membership_updater.rb
@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Auth
+ module Saml
+ class MembershipUpdater
+ include Gitlab::Utils::StrongMemoize
+
+ def initialize(user, auth_hash)
+ @user = user
+ @auth_hash = auth_hash
+ end
+
+ def execute
+ enqueue_group_sync if sync_groups?
+ end
+
+ private
+
+ attr_reader :user, :auth_hash
+
+ def enqueue_group_sync
+ group_link_ids
+ end
+
+ def sync_groups?
+ return false unless user
+
+ sync_enabled? && any_group_links?
+ end
+
+ # rubocop:disable CodeReuse/ActiveRecord
+ def group_link_ids
+ strong_memoize(:group_link_ids) do
+ next [] if group_names_from_saml.empty?
+
+ SamlGroupLink
+ .by_saml_group_name(group_names_from_saml)
+ .pluck(:id)
+ end
+ end
+
+ def any_group_links?
+ strong_memoize(:any_group_links) do
+ SamlGroupLink.any?
+ end
+ end
+ # rubocop:enable CodeReuse/ActiveRecord
+
+ def group_names_from_saml
+ strong_memoize(:group_names_from_saml) do
+ auth_hash.groups || []
+ end
+ end
+
+ def sync_enabled?
+ Gitlab::Auth::Saml::Config.group_sync_enabled?
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/controllers/groups/saml_group_links_controller_spec.rb b/ee/spec/controllers/groups/saml_group_links_controller_spec.rb
index c8ec1d6a6abdfeae044d734f62d2c4de24407eea..8fea5870b19b5e51cc71da20de0c370d140446a8 100644
--- a/ee/spec/controllers/groups/saml_group_links_controller_spec.rb
+++ b/ee/spec/controllers/groups/saml_group_links_controller_spec.rb
@@ -11,7 +11,7 @@
end
before do
- stub_licensed_features(group_saml: true, group_saml_group_sync: true)
+ stub_licensed_features(group_saml: true, saml_group_sync: true)
sign_in(user)
end
diff --git a/ee/spec/features/groups/saml_group_links_spec.rb b/ee/spec/features/groups/saml_group_links_spec.rb
index 1af8ac571ee493436621746a0d34727108c7ecee..76c11f4c71648907335886663ef764703d8c2b4e 100644
--- a/ee/spec/features/groups/saml_group_links_spec.rb
+++ b/ee/spec/features/groups/saml_group_links_spec.rb
@@ -13,7 +13,7 @@
context 'when SAML group links is available' do
before do
- stub_licensed_features(group_saml: true, group_saml_group_sync: true)
+ stub_licensed_features(group_saml: true, saml_group_sync: true)
create(:saml_provider, group: group, enabled: true)
diff --git a/ee/spec/lib/ee/gitlab/auth/saml/identity_linker_spec.rb b/ee/spec/lib/ee/gitlab/auth/saml/identity_linker_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..d5f528c013dc9d845ed5c89ef59e65a5ec41f0a6
--- /dev/null
+++ b/ee/spec/lib/ee/gitlab/auth/saml/identity_linker_spec.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::Saml::IdentityLinker do
+ let(:user) { create(:user) }
+ let(:in_response_to) { '12345' }
+ let(:saml_response) { instance_double(OneLogin::RubySaml::Response, in_response_to: in_response_to) }
+ let(:session) { { 'last_authn_request_id' => in_response_to } }
+
+ let(:oauth) do
+ OmniAuth::AuthHash.new(provider: 'saml', uid: user.email, extra: { response_object: saml_response })
+ end
+
+ subject(:linker) { described_class.new(user, oauth, session) }
+
+ it 'updates membership' do
+ expect(Gitlab::Auth::Saml::MembershipUpdater).to receive(:new).with(user, any_args).and_call_original
+
+ linker.link
+ end
+end
diff --git a/ee/spec/lib/ee/gitlab/auth/saml/membership_updater_spec.rb b/ee/spec/lib/ee/gitlab/auth/saml/membership_updater_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..52a16abcd0d43c40396a5cd43528c3ca5db68a7d
--- /dev/null
+++ b/ee/spec/lib/ee/gitlab/auth/saml/membership_updater_spec.rb
@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::Saml::MembershipUpdater do
+ let_it_be(:user) { create(:user) }
+ let_it_be(:group1) { create(:group) }
+ let_it_be(:group2) { create(:group) }
+
+ let(:auth_hash) do
+ Gitlab::Auth::GroupSaml::AuthHash.new(
+ OmniAuth::AuthHash.new(extra: {
+ raw_info: OneLogin::RubySaml::Attributes.new('groups' => %w(Developers Owners))
+ })
+ )
+ end
+
+ subject(:update_membership) { described_class.new(user, auth_hash).execute }
+
+ context 'when SAML group links exist' do
+ def stub_saml_group_sync_enabled(enabled)
+ allow(::Gitlab::Auth::Saml::Config).to receive(:group_sync_enabled?).and_return(enabled)
+ end
+
+ let!(:group_link) { create(:saml_group_link, saml_group_name: 'Owners', group: group1) }
+
+ context 'when group sync is not available' do
+ before do
+ stub_saml_group_sync_enabled(false)
+ end
+
+ it 'does not enqueue group sync' do
+ expect(::SamlGroupLink).not_to receive(:by_saml_group_name)
+
+ update_membership
+ end
+ end
+
+ context 'when group sync is available' do
+ before do
+ stub_saml_group_sync_enabled(true)
+ end
+
+ it 'enqueues group sync' do
+ expect(::SamlGroupLink).to receive(:by_saml_group_name).and_call_original
+
+ update_membership
+ end
+
+ context 'when auth hash contains no groups' do
+ let(:auth_hash) do
+ Gitlab::Auth::GroupSaml::AuthHash.new(
+ OmniAuth::AuthHash.new(extra: { raw_info: OneLogin::RubySaml::Attributes.new })
+ )
+ end
+
+ it 'enqueues group sync' do
+ expect(::SamlGroupLink).not_to receive(:by_saml_group_name)
+
+ update_membership
+ end
+ end
+
+ context 'when auth hash groups do not match group links' do
+ before do
+ group_link.update!(saml_group_name: 'Web Developers')
+ end
+
+ it 'enqueues group sync' do
+ expect(::SamlGroupLink).to receive(:by_saml_group_name).and_call_original
+
+ update_membership
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/lib/gitlab/auth/saml/config_spec.rb b/ee/spec/lib/gitlab/auth/saml/config_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..a183a3db7ccb6f35ae2a62b51f8dab6180c83519
--- /dev/null
+++ b/ee/spec/lib/gitlab/auth/saml/config_spec.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::Saml::Config do
+ describe '.group_sync_enabled?' do
+ subject { described_class.group_sync_enabled? }
+
+ it { is_expected.to eq(false) }
+
+ context 'when SAML is enabled' do
+ before do
+ allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } })
+ allow(Gitlab::Auth::OAuth::Provider).to receive(:providers).and_return([:saml])
+ end
+
+ it { is_expected.to eq(false) }
+
+ context 'when the group attribute is configured' do
+ before do
+ allow(Gitlab::Auth::Saml::Config).to receive(:groups).and_return(['Groups'])
+ end
+
+ it { is_expected.to eq(false) }
+
+ context 'when the saml_group_sync feature is licensed' do
+ before do
+ stub_licensed_features(saml_group_sync: true)
+ end
+
+ it { is_expected.to eq(true) }
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb
index 95eb2977bf0d052fe8d5c99c5525a309c46b2fc7..76e65d6233c2b553e93bdf4e8fed1c5ed8824982 100644
--- a/ee/spec/models/ee/group_spec.rb
+++ b/ee/spec/models/ee/group_spec.rb
@@ -1653,6 +1653,14 @@
it { is_expected.to eq(false) }
end
+
+ context 'when global SAML is enabled' do
+ before do
+ allow(Gitlab::Auth::OAuth::Provider).to receive(:providers).and_return([:saml])
+ end
+
+ it { is_expected.to eq(true) }
+ end
end
describe '#saml_group_sync_available?' do
@@ -1662,7 +1670,7 @@
context 'with group_saml_group_sync feature licensed' do
before do
- stub_licensed_features(group_saml_group_sync: true)
+ stub_licensed_features(saml_group_sync: true)
end
it { is_expected.to eq(false) }
diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb
index d0a13a742df7bdaa4896e70d49dc6079e5924ea7..360d087236fefed41ef0201f17992540b09a3e10 100644
--- a/ee/spec/policies/group_policy_spec.rb
+++ b/ee/spec/policies/group_policy_spec.rb
@@ -621,6 +621,43 @@ def stub_group_saml_config(enabled)
end
end
+ describe 'admin_saml_group_links for global SAML' do
+ let(:current_user) { owner }
+
+ it { is_expected.to be_disallowed(:admin_saml_group_links) }
+
+ context 'when global SAML is enabled' do
+ before do
+ allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } })
+ allow(Gitlab::Auth::OAuth::Provider).to receive(:providers).and_return([:saml])
+ end
+
+ it { is_expected.to be_disallowed(:admin_saml_group_links) }
+
+ context 'when the groups attribute is configured' do
+ before do
+ allow(Gitlab::Auth::Saml::Config).to receive(:groups).and_return(['Groups'])
+ end
+
+ it { is_expected.to be_disallowed(:admin_saml_group_links) }
+
+ context 'when saml_group_sync feature is licensed' do
+ before do
+ stub_licensed_features(saml_group_sync: true)
+ end
+
+ it { is_expected.to be_allowed(:admin_saml_group_links) }
+
+ context 'when the current user is not an admin or owner' do
+ let(:current_user) { maintainer }
+
+ it { is_expected.to be_disallowed(:admin_saml_group_links) }
+ end
+ end
+ end
+ end
+ end
+
context 'with ip restriction' do
let(:current_user) { developer }
diff --git a/ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb b/ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb
index f635944660c01da93a6e94d291b88824e58c52c2..327a9170b445dc316c10c722c8ed4ab6bf557a58 100644
--- a/ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb
+++ b/ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb
@@ -532,7 +532,7 @@
usage_quotas: true,
group_saml: true,
group_webhooks: true,
- group_saml_group_sync: true,
+ saml_group_sync: true,
ldap_group_sync: true
)
stub_feature_flags(group_administration_nav_item: false)
diff --git a/ee/spec/workers/group_saml_group_sync_worker_spec.rb b/ee/spec/workers/group_saml_group_sync_worker_spec.rb
index a69ef4ad0dd8f32fbc9b09c896d0600eef09d7a6..915580cfb5614d1eb4af26641668cfc97f3f482f 100644
--- a/ee/spec/workers/group_saml_group_sync_worker_spec.rb
+++ b/ee/spec/workers/group_saml_group_sync_worker_spec.rb
@@ -28,7 +28,7 @@
context 'when the group has group_saml_group_sync feature licensed' do
before do
- stub_licensed_features(group_saml_group_sync: true)
+ stub_licensed_features(saml_group_sync: true)
end
context 'when SAML is not enabled' do
diff --git a/lib/gitlab/auth/saml/config.rb b/lib/gitlab/auth/saml/config.rb
index 3f13a264b0acce4bc185fd763afc1636d61644dd..815130aeee2262f828c2f8a129dd31f21b95e0e0 100644
--- a/lib/gitlab/auth/saml/config.rb
+++ b/lib/gitlab/auth/saml/config.rb
@@ -5,6 +5,10 @@ module Auth
module Saml
class Config
class << self
+ def enabled?
+ ::AuthHelper.saml_providers.any?
+ end
+
def options
Gitlab::Auth::OAuth::Provider.config_for('saml')
end
diff --git a/lib/gitlab/auth/saml/identity_linker.rb b/lib/gitlab/auth/saml/identity_linker.rb
index 93195c3189ff84bb04ca0d5903069a4f1f765774..a44a9c2fca51097f0ea5d520552d0e67b1c57c23 100644
--- a/lib/gitlab/auth/saml/identity_linker.rb
+++ b/lib/gitlab/auth/saml/identity_linker.rb
@@ -32,3 +32,5 @@ def saml_response
end
end
end
+
+Gitlab::Auth::Saml::IdentityLinker.prepend_mod
diff --git a/spec/helpers/auth_helper_spec.rb b/spec/helpers/auth_helper_spec.rb
index 4bb09699db4f6121159acc9baa6821c14dc4e577..4b0b44d1325b99d1d850e97b8696c474e58c9c11 100644
--- a/spec/helpers/auth_helper_spec.rb
+++ b/spec/helpers/auth_helper_spec.rb
@@ -500,6 +500,16 @@ def auth_active?
)
end
+ context 'when SAML is enabled without specifying a strategy class' do
+ before do
+ allow(Gitlab::Auth::OAuth::Provider).to receive(:providers).and_return([:saml])
+ end
+
+ it 'returns the saml provider' do
+ expect(saml_providers).to match_array([:saml])
+ end
+ end
+
context 'when configuration specifies no provider' do
before do
allow(Devise).to receive(:omniauth_providers).and_return([])
diff --git a/spec/lib/gitlab/auth/saml/config_spec.rb b/spec/lib/gitlab/auth/saml/config_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..12f5da4887357ea7dab94cc1fcad6c1edd9daa21
--- /dev/null
+++ b/spec/lib/gitlab/auth/saml/config_spec.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::Saml::Config do
+ describe '.enabled?' do
+ subject { described_class.enabled? }
+
+ it { is_expected.to eq(false) }
+
+ context 'when SAML is enabled' do
+ before do
+ allow(Gitlab::Auth::OAuth::Provider).to receive(:providers).and_return([:saml])
+ end
+
+ it { is_expected.to eq(true) }
+ end
+ end
+end