From 39fef575124acd4df0d603caf7689d343d7d63b0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Fri, 9 Apr 2021 18:20:16 +0200
Subject: [PATCH 1/9] Disallow password authentication for users provisioned by
 group

Add feature flag to blocking password auth for saml users method

Add possibility to readd saml user to the group

Fix unneeded change"

Change Let_it_be to let to avoid specs failure

Add spec coverage for omniauth flow

Add specs for group saml user

Fix specs file in user

Add logging out after leaving group by provisioned by this group member

Add log out after disconnecting saml

Rename method for more clarity

Changelog: added
---
 .../concerns/membership_actions.rb            |  2 +
 .../block_password_auth_for_saml_users.yml    |  8 +++
 .../concerns/ee/membership_actions.rb         | 16 +++++
 ee/app/controllers/groups/sso_controller.rb   |  6 +-
 ee/app/models/ee/user.rb                      |  6 ++
 ee/app/models/ee/user_detail.rb               |  4 ++
 ee/lib/gitlab/auth/group_saml/user.rb         | 21 +++++-
 .../omniauth_callbacks_controller_spec.rb     | 42 +++++++++++
 .../controllers/groups/sso_controller_spec.rb | 39 ++++++++--
 .../groups/members/leave_group_spec.rb        | 71 +++++++++++++++++++
 .../lib/gitlab/auth/group_saml/user_spec.rb   | 60 ++++++++++++++--
 ee/spec/models/ee/user_spec.rb                | 44 +++++++++++-
 ee/spec/models/user_detail_spec.rb            | 16 +++++
 lib/gitlab/auth/o_auth/user.rb                |  2 +
 spec/features/groups_spec.rb                  |  2 +-
 15 files changed, 323 insertions(+), 16 deletions(-)
 create mode 100644 config/feature_flags/ops/block_password_auth_for_saml_users.yml
 create mode 100644 ee/app/controllers/concerns/ee/membership_actions.rb
 create mode 100644 ee/spec/features/groups/members/leave_group_spec.rb

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index 7bbee8ba79eb3b..869661d95ba44c 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -186,3 +186,5 @@ def requested_relations
     end
   end
 end
+
+MembershipActions.prepend_if_ee('EE::MembershipActions')
diff --git a/config/feature_flags/ops/block_password_auth_for_saml_users.yml b/config/feature_flags/ops/block_password_auth_for_saml_users.yml
new file mode 100644
index 00000000000000..492c00f2dd5a5e
--- /dev/null
+++ b/config/feature_flags/ops/block_password_auth_for_saml_users.yml
@@ -0,0 +1,8 @@
+---
+name: block_password_auth_for_saml_users
+introduced_by_url:
+rollout_issue_url:
+milestone: '13.11'
+type: ops
+group: group::access
+default_enabled: false
diff --git a/ee/app/controllers/concerns/ee/membership_actions.rb b/ee/app/controllers/concerns/ee/membership_actions.rb
new file mode 100644
index 00000000000000..1a3c2f180e5383
--- /dev/null
+++ b/ee/app/controllers/concerns/ee/membership_actions.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module EE
+  module MembershipActions
+    extend ::Gitlab::Utils::Override
+
+    override :leave
+    def leave
+      super
+
+      if membershipable == current_user.provisioned_by_group && current_user.authorized_by_provisioned_by_group?
+        sign_out current_user
+      end
+    end
+  end
+end
diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb
index 004c61c81652bf..12f6548f1fe705 100644
--- a/ee/app/controllers/groups/sso_controller.rb
+++ b/ee/app/controllers/groups/sso_controller.rb
@@ -33,7 +33,11 @@ def unlink
 
     GroupSaml::Identity::DestroyService.new(linked_identity).execute
 
-    redirect_to profile_account_path
+    if current_user.authorized_by_provisining_group? && unauthenticated_group == current_user.provisioned_by_group
+      sign_out current_user
+    else
+      redirect_to profile_account_path
+    end
   end
 
   def sign_up_form
diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index b8a5a6b5d142b1..1a38b9dba6fdcc 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -298,6 +298,10 @@ def group_managed_account?
       managing_group.present?
     end
 
+    def authorized_by_provisining_group?
+      ::Feature.enabled?(:block_password_auth_for_saml_users, type: :ops) && user_detail.provisioned_by_group?
+    end
+
     def managed_by?(user)
       self.group_managed_account? && self.managing_group.owned_by?(user)
     end
@@ -314,6 +318,7 @@ def admin_unsubscribe!
     override :allow_password_authentication_for_web?
     def allow_password_authentication_for_web?(*)
       return false if group_managed_account?
+      return false if authorized_by_provisining_group?
 
       super
     end
@@ -321,6 +326,7 @@ def allow_password_authentication_for_web?(*)
     override :allow_password_authentication_for_git?
     def allow_password_authentication_for_git?(*)
       return false if group_managed_account?
+      return false if authorized_by_provisining_group?
 
       super
     end
diff --git a/ee/app/models/ee/user_detail.rb b/ee/app/models/ee/user_detail.rb
index 24d026ec6c9444..2c041b71b1058a 100644
--- a/ee/app/models/ee/user_detail.rb
+++ b/ee/app/models/ee/user_detail.rb
@@ -7,5 +7,9 @@ module UserDetail
     prepended do
       belongs_to :provisioned_by_group, class_name: 'Group', optional: true, inverse_of: :provisioned_user_details
     end
+
+    def provisioned_by_group?
+      !!provisioned_by_group
+    end
   end
 end
diff --git a/ee/lib/gitlab/auth/group_saml/user.rb b/ee/lib/gitlab/auth/group_saml/user.rb
index 1048b64cca0542..8cb205c9074219 100644
--- a/ee/lib/gitlab/auth/group_saml/user.rb
+++ b/ee/lib/gitlab/auth/group_saml/user.rb
@@ -17,8 +17,9 @@ def initialize(auth_hash)
 
         override :find_and_update!
         def find_and_update!
-          save("GroupSaml Provider ##{saml_provider.id}")
+          add_or_update_user_identities
 
+          save("GroupSaml Provider ##{@saml_provider.id}")
           # Do not return un-persisted user so user is prompted
           # to sign-in to existing account.
           return unless valid_sign_in?
@@ -37,7 +38,7 @@ def bypass_two_factor?
         override :gl_user
         def gl_user
           strong_memoize(:gl_user) do
-            identity&.user || build_new_user
+            identity&.user || find_by_email || build_new_user
           end
         end
 
@@ -47,6 +48,15 @@ def identity
           end
         end
 
+        override :find_by_email
+        def find_by_email
+          user = super
+
+          return user if user&.authorized_by_provisining_group? && user&.provisioned_by_group_id == saml_provider.group_id
+
+          false
+        end
+
         override :build_new_user
         def build_new_user(skip_confirmation: false)
           super.tap do |user|
@@ -72,6 +82,13 @@ def user_attributes
           end
         end
 
+        override :add_or_update_user_identities
+        def add_or_update_user_identities
+          super.tap do |identity|
+            identity.saml_provider_id = @saml_provider.id
+          end
+        end
+
         def update_group_membership
           MembershipUpdater.new(gl_user, saml_provider, auth_hash).execute
         end
diff --git a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
index a90496f5fd18fc..93c99e19af131d 100644
--- a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
+++ b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
@@ -115,6 +115,48 @@ def stub_last_request_id(id)
       end
     end
 
+    context 'when used to be a member of a group' do
+      before do
+        user.provisioned_by_group = group
+        user.save!
+      end
+
+      it "displays a flash message verifying group sign in" do
+        post provider, params: { group_id: group }
+
+        expect(flash[:notice]).to match(/Signed in with SAML/i)
+      end
+
+      it 'adds linked identity' do
+        expect { post provider, params: { group_id: group } }.to change(linked_accounts, :count)
+      end
+
+      it 'adds group membership' do
+        expect { post provider, params: { group_id: group } }.to change { group.members.count }
+      end
+    end
+
+    context 'when user was provisioned by other group' do
+      before do
+        user.provisioned_by_group = create(:group)
+        user.save!
+      end
+
+      it "displays a flash message verifying group sign in" do
+        post provider, params: { group_id: group }
+
+        expect(flash[:notice]).to eq('Login to a GitLab account to link with your SAML identity')
+      end
+
+      it 'adds linked identity' do
+        expect { post provider, params: { group_id: group } }.not_to change(linked_accounts, :count)
+      end
+
+      it 'adds group membership' do
+        expect { post provider, params: { group_id: group } }.not_to change { group.members.count }
+      end
+    end
+
     context "when signed in" do
       before do
         sign_in(user)
diff --git a/ee/spec/controllers/groups/sso_controller_spec.rb b/ee/spec/controllers/groups/sso_controller_spec.rb
index 8b9ef436e2bf1d..4e2fe2940c6fa1 100644
--- a/ee/spec/controllers/groups/sso_controller_spec.rb
+++ b/ee/spec/controllers/groups/sso_controller_spec.rb
@@ -34,12 +34,41 @@
       expect(assigns[:group_name]).to eq 'our-group'
     end
 
-    it 'allows account unlinking' do
-      create(:group_saml_identity, saml_provider: saml_provider, user: user)
+    context 'unlinking user' do
+      before do
+        create(:group_saml_identity, saml_provider: saml_provider, user: user)
+        user.update!(provisioned_by_group: saml_provider.group)
+      end
+
+      it 'allows account unlinking' do
+        expect do
+          delete :unlink, params: { group_id: group }
+        end.to change(Identity, :count).by(-1)
+      end
+
+      context 'with block_password_auth_for_saml_users feature flag switched off' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: false)
+        end
 
-      expect do
-        delete :unlink, params: { group_id: group }
-      end.to change(Identity, :count).by(-1)
+        it 'does not sign out user provisioned by this group' do
+          delete :unlink, params: { group_id: group }
+
+          expect(response).to redirect_to(profile_account_path)
+        end
+      end
+
+      context 'with block_password_auth_for_saml_users feature flag switched on' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: true)
+        end
+
+        it 'signs out user provisioned by this group' do
+          delete :unlink, params: { group_id: group }
+
+          expect(subject.current_user).to eq(nil)
+        end
+      end
     end
 
     context 'when SAML is disabled for the group' do
diff --git a/ee/spec/features/groups/members/leave_group_spec.rb b/ee/spec/features/groups/members/leave_group_spec.rb
new file mode 100644
index 00000000000000..b6854ff327f3d5
--- /dev/null
+++ b/ee/spec/features/groups/members/leave_group_spec.rb
@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Groups > Members > Leave group' do
+  include Spec::Support::Helpers::Features::MembersHelpers
+
+  let(:user) { create(:user) }
+  let(:other_user) { create(:user) }
+  let(:group) { create(:group) }
+
+  before do
+    user.update!(provisioned_by_group: group)
+    sign_in(user)
+  end
+
+  context 'with block_password_auth_for_saml_users feature flag switched on' do
+    it 'guest provisoned by this group leaves the group' do
+      group.add_guest(user)
+      group.add_owner(other_user)
+
+      visit group_path(group)
+      click_link 'Leave group'
+
+      expect(group.users).not_to include(user)
+      expect(current_path).to eq(new_user_session_path)
+    end
+
+    it 'guest leaves the group by url param', :js do
+      group.add_guest(user)
+      group.add_owner(other_user)
+
+      visit group_path(group, leave: 1)
+
+      page.accept_confirm
+
+      wait_for_all_requests
+      expect(current_path).to eq(new_user_session_path)
+      expect(group.users).not_to include(user)
+    end
+  end
+
+  context 'with block_password_auth_for_saml_users feature flag switched off' do
+    before do
+      stub_feature_flags(block_password_auth_for_saml_users: false)
+    end
+
+    it 'guest leaves the group by url param', :js do
+      group.add_guest(user)
+      group.add_owner(other_user)
+
+      visit group_path(group, leave: 1)
+
+      page.accept_confirm
+
+      wait_for_all_requests
+      expect(current_path).to eq(dashboard_groups_path)
+      expect(group.users).not_to include(user)
+    end
+
+    it 'guest leaves the group as last member' do
+      group.add_guest(user)
+
+      visit group_path(group)
+      click_link 'Leave group'
+
+      expect(current_path).to eq(dashboard_groups_path)
+      expect(group.users).not_to include(user)
+    end
+  end
+end
diff --git a/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb
index ce53ad54a6211e..7c3425da769e9d 100644
--- a/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb
+++ b/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb
@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 require 'spec_helper'
 
-require 'spec_helper'
-
 RSpec.describe Gitlab::Auth::GroupSaml::User do
   let(:uid) { 1234 }
   let(:saml_provider) { create(:saml_provider) }
@@ -107,9 +105,7 @@ def create_existing_identity
       end
 
       context 'when a conflicting user already exists' do
-        before do
-          create(:user, email: info_hash[:email])
-        end
+        let!(:user) { create(:user, email: info_hash[:email]) }
 
         it 'does not update membership' do
           expect { find_and_update }.not_to change { group.members.count }
@@ -118,6 +114,60 @@ def create_existing_identity
         it 'does not return a user' do
           expect(find_and_update).to eq nil
         end
+
+        context 'when user was provisioned by this group' do
+          before do
+            user.update!(provisioned_by_group: group)
+          end
+
+          it 'updates membership' do
+            expect { find_and_update }.to change { group.members.count }.by(1)
+          end
+
+          it 'returns a user' do
+            expect(find_and_update).to eq user
+          end
+
+          it 'updates idenitity' do
+            expect { find_and_update }.to change { user.group_saml_identities.count }.by(1)
+          end
+
+          context 'without feature flag turned on' do
+            before do
+              stub_feature_flags(block_password_auth_for_saml_users: false)
+            end
+
+            it 'does not update membership' do
+              expect { find_and_update }.not_to change { group.members.count }
+            end
+
+            it 'does not return a user' do
+              expect(find_and_update).to eq nil
+            end
+
+            it 'does not update identity' do
+              expect { find_and_update }.not_to change { user.group_saml_identities.count }
+            end
+          end
+        end
+
+        context 'when user was provisioned by different group' do
+          before do
+            user.update!(provisioned_by_group: create(:group))
+          end
+
+          it 'does not update membership' do
+            expect { find_and_update }.not_to change { group.members.count }
+          end
+
+          it 'does not return a user' do
+            expect(find_and_update).to eq nil
+          end
+
+          it 'does not update identity' do
+            expect { find_and_update }.not_to change { user.group_saml_identities.count }
+          end
+        end
       end
     end
   end
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index a94793dc5f0c4d..d271ab1117d585 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -753,25 +753,65 @@
   describe '#allow_password_authentication_for_web?' do
     context 'when user has managing group linked' do
       before do
-        user.managing_group = Group.new
+        user.managing_group = build(:group)
       end
 
       it 'is false' do
         expect(user.allow_password_authentication_for_web?).to eq false
       end
     end
+
+    context 'when user is provisioned by group' do
+      before do
+        user.user_detail.provisioned_by_group = build(:group)
+      end
+
+      it 'is false' do
+        expect(user.allow_password_authentication_for_web?).to eq false
+      end
+
+      context 'with feature flag switched off' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: false)
+        end
+
+        it 'is true' do
+          expect(user.allow_password_authentication_for_web?).to eq true
+        end
+      end
+    end
   end
 
   describe '#allow_password_authentication_for_git?' do
     context 'when user has managing group linked' do
       before do
-        user.managing_group = Group.new
+        user.managing_group = build(:group)
       end
 
       it 'is false' do
         expect(user.allow_password_authentication_for_git?).to eq false
       end
     end
+
+    context 'when user is provisioned by group' do
+      before do
+        user.user_detail.provisioned_by_group = build(:group)
+      end
+
+      it 'is false' do
+        expect(user.allow_password_authentication_for_git?).to eq false
+      end
+
+      context 'with feature flag switched off' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: false)
+        end
+
+        it 'is true' do
+          expect(user.allow_password_authentication_for_git?).to eq true
+        end
+      end
+    end
   end
 
   describe '#using_license_seat?' do
diff --git a/ee/spec/models/user_detail_spec.rb b/ee/spec/models/user_detail_spec.rb
index 4b1c0e12b6834a..051735b10cda9c 100644
--- a/ee/spec/models/user_detail_spec.rb
+++ b/ee/spec/models/user_detail_spec.rb
@@ -4,4 +4,20 @@
 
 RSpec.describe UserDetail do
   it { is_expected.to belong_to(:provisioned_by_group) }
+
+  describe '#provisioned_by_group?' do
+    let(:user) { create(:user, provisioned_by_group: build(:group)) }
+
+    subject { user.user_detail.provisioned_by_group? }
+
+    it 'returns true when user is provisoned by group' do
+      expect(subject).to eq(true)
+    end
+
+    it 'returns true when user is provisoned by group' do
+      user.user_detail.update!(provisioned_by_group: nil)
+
+      expect(subject).to eq(false)
+    end
+  end
 end
diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb
index fe1bf730e76b9a..5f29bb8f433831 100644
--- a/lib/gitlab/auth/o_auth/user.rb
+++ b/lib/gitlab/auth/o_auth/user.rb
@@ -115,6 +115,8 @@ def add_or_update_user_identities
             log.info "Correct LDAP account has been found. identity to user: #{gl_user.username}."
             gl_user.identities.build(provider: ldap_person.provider, extern_uid: ldap_person.dn)
           end
+
+          identity
         end
 
         def find_or_build_ldap_user
diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb
index 33d2ac50628b72..27b75d15d3b9c9 100644
--- a/spec/features/groups_spec.rb
+++ b/spec/features/groups_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe 'Group' do
-  let_it_be(:user) { create(:user) }
+  let(:user) { create(:user) }
 
   before do
     sign_in(user)
-- 
GitLab


From 456796f340abaf895ec4df00f38cd67c67fe9f64 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Sun, 2 May 2021 15:30:05 +0200
Subject: [PATCH 2/9] Add changelog entry

---
 .../300191-enterprise-users-disallow-password-auth.yml      | 6 ++++++
 1 file changed, 6 insertions(+)
 create mode 100644 changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml

diff --git a/changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml b/changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml
new file mode 100644
index 00000000000000..9955bf9124e1ea
--- /dev/null
+++ b/changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml
@@ -0,0 +1,6 @@
+---
+title: Disallow password authentication for users provisioned by group behind a feature
+  flag
+merge_request: 59673
+author:
+type: added
-- 
GitLab


From 55c74619291072d8d819059a3ed74563e0e74546 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Sun, 2 May 2021 15:44:07 +0200
Subject: [PATCH 3/9] Fix typo in method name

---
 ee/app/controllers/concerns/ee/membership_actions.rb | 2 +-
 ee/app/controllers/groups/sso_controller.rb          | 2 +-
 ee/app/models/ee/user.rb                             | 6 +++---
 ee/lib/gitlab/auth/group_saml/user.rb                | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ee/app/controllers/concerns/ee/membership_actions.rb b/ee/app/controllers/concerns/ee/membership_actions.rb
index 1a3c2f180e5383..441aa949b010f6 100644
--- a/ee/app/controllers/concerns/ee/membership_actions.rb
+++ b/ee/app/controllers/concerns/ee/membership_actions.rb
@@ -8,7 +8,7 @@ module MembershipActions
     def leave
       super
 
-      if membershipable == current_user.provisioned_by_group && current_user.authorized_by_provisioned_by_group?
+      if current_user.authorized_by_provisioned_by_group? && membershipable == current_user.provisioned_by_group
         sign_out current_user
       end
     end
diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb
index 12f6548f1fe705..e92e8dbb1181c3 100644
--- a/ee/app/controllers/groups/sso_controller.rb
+++ b/ee/app/controllers/groups/sso_controller.rb
@@ -33,7 +33,7 @@ def unlink
 
     GroupSaml::Identity::DestroyService.new(linked_identity).execute
 
-    if current_user.authorized_by_provisining_group? && unauthenticated_group == current_user.provisioned_by_group
+    if current_user.authorized_by_provisioning_group? && unauthenticated_group == current_user.provisioned_by_group
       sign_out current_user
     else
       redirect_to profile_account_path
diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 1a38b9dba6fdcc..0663a7ba10fb79 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -298,7 +298,7 @@ def group_managed_account?
       managing_group.present?
     end
 
-    def authorized_by_provisining_group?
+    def authorized_by_provisioning_group?
       ::Feature.enabled?(:block_password_auth_for_saml_users, type: :ops) && user_detail.provisioned_by_group?
     end
 
@@ -318,7 +318,7 @@ def admin_unsubscribe!
     override :allow_password_authentication_for_web?
     def allow_password_authentication_for_web?(*)
       return false if group_managed_account?
-      return false if authorized_by_provisining_group?
+      return false if authorized_by_provisioning_group?
 
       super
     end
@@ -326,7 +326,7 @@ def allow_password_authentication_for_web?(*)
     override :allow_password_authentication_for_git?
     def allow_password_authentication_for_git?(*)
       return false if group_managed_account?
-      return false if authorized_by_provisining_group?
+      return false if authorized_by_provisioning_group?
 
       super
     end
diff --git a/ee/lib/gitlab/auth/group_saml/user.rb b/ee/lib/gitlab/auth/group_saml/user.rb
index 8cb205c9074219..6f05d1966a8ce8 100644
--- a/ee/lib/gitlab/auth/group_saml/user.rb
+++ b/ee/lib/gitlab/auth/group_saml/user.rb
@@ -52,7 +52,7 @@ def identity
         def find_by_email
           user = super
 
-          return user if user&.authorized_by_provisining_group? && user&.provisioned_by_group_id == saml_provider.group_id
+          return user if user&.authorized_by_provisioning_group? && user&.provisioned_by_group_id == saml_provider.group_id
 
           false
         end
-- 
GitLab


From 4b5a2f75c9636801cdf24ae0f6c4dc64aca15fc6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Sun, 2 May 2021 15:50:34 +0200
Subject: [PATCH 4/9] Fix naming in specs

---
 .../groups/omniauth_callbacks_controller_spec.rb            | 6 +++---
 ee/spec/features/groups/members/leave_group_spec.rb         | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
index 93c99e19af131d..ac23e6210bdb1f 100644
--- a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
+++ b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb
@@ -115,7 +115,7 @@ def stub_last_request_id(id)
       end
     end
 
-    context 'when used to be a member of a group' do
+    context 'when user used to be a member of a group' do
       before do
         user.provisioned_by_group = group
         user.save!
@@ -148,11 +148,11 @@ def stub_last_request_id(id)
         expect(flash[:notice]).to eq('Login to a GitLab account to link with your SAML identity')
       end
 
-      it 'adds linked identity' do
+      it 'does not add linked identity' do
         expect { post provider, params: { group_id: group } }.not_to change(linked_accounts, :count)
       end
 
-      it 'adds group membership' do
+      it 'does not add group membership' do
         expect { post provider, params: { group_id: group } }.not_to change { group.members.count }
       end
     end
diff --git a/ee/spec/features/groups/members/leave_group_spec.rb b/ee/spec/features/groups/members/leave_group_spec.rb
index b6854ff327f3d5..6834f7a52184be 100644
--- a/ee/spec/features/groups/members/leave_group_spec.rb
+++ b/ee/spec/features/groups/members/leave_group_spec.rb
@@ -15,7 +15,7 @@
   end
 
   context 'with block_password_auth_for_saml_users feature flag switched on' do
-    it 'guest provisoned by this group leaves the group' do
+    it 'guest provisoned by this group leaves the group and is signed off' do
       group.add_guest(user)
       group.add_owner(other_user)
 
@@ -26,7 +26,7 @@
       expect(current_path).to eq(new_user_session_path)
     end
 
-    it 'guest leaves the group by url param', :js do
+    it 'guest leaves the group by url param and is signed off', :js do
       group.add_guest(user)
       group.add_owner(other_user)
 
-- 
GitLab


From 064095cfb9d9e820dd739dc6c4379618c9da6e27 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Sun, 2 May 2021 19:22:39 +0200
Subject: [PATCH 5/9] Fix wrong naming

---
 ee/app/controllers/concerns/ee/membership_actions.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/app/controllers/concerns/ee/membership_actions.rb b/ee/app/controllers/concerns/ee/membership_actions.rb
index 441aa949b010f6..608f31fc33ccfa 100644
--- a/ee/app/controllers/concerns/ee/membership_actions.rb
+++ b/ee/app/controllers/concerns/ee/membership_actions.rb
@@ -8,7 +8,7 @@ module MembershipActions
     def leave
       super
 
-      if current_user.authorized_by_provisioned_by_group? && membershipable == current_user.provisioned_by_group
+      if current_user.authorized_by_provisioning_group? && membershipable == current_user.provisioned_by_group
         sign_out current_user
       end
     end
-- 
GitLab


From 1ffd2cb04c606a15babdc64eb54d3df63c8c646c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Mon, 3 May 2021 11:46:17 +0200
Subject: [PATCH 6/9] Rename method based on cr comment

---
 .../concerns/ee/membership_actions.rb            |  2 +-
 ee/app/controllers/groups/sso_controller.rb      |  2 +-
 ee/app/models/ee/user.rb                         | 16 ++++++++++------
 ee/lib/gitlab/auth/group_saml/user.rb            |  4 ++--
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/ee/app/controllers/concerns/ee/membership_actions.rb b/ee/app/controllers/concerns/ee/membership_actions.rb
index 608f31fc33ccfa..165e4260510154 100644
--- a/ee/app/controllers/concerns/ee/membership_actions.rb
+++ b/ee/app/controllers/concerns/ee/membership_actions.rb
@@ -8,7 +8,7 @@ module MembershipActions
     def leave
       super
 
-      if current_user.authorized_by_provisioning_group? && membershipable == current_user.provisioned_by_group
+      if current_user.authorized_by_provisioning_group?(membershipable)
         sign_out current_user
       end
     end
diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb
index e92e8dbb1181c3..e177d8fd20c840 100644
--- a/ee/app/controllers/groups/sso_controller.rb
+++ b/ee/app/controllers/groups/sso_controller.rb
@@ -33,7 +33,7 @@ def unlink
 
     GroupSaml::Identity::DestroyService.new(linked_identity).execute
 
-    if current_user.authorized_by_provisioning_group? && unauthenticated_group == current_user.provisioned_by_group
+    if current_user.authorized_by_provisioning_group?(unauthenticated_group)
       sign_out current_user
     else
       redirect_to profile_account_path
diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 0663a7ba10fb79..26f3d3fa3109ea 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -298,10 +298,6 @@ def group_managed_account?
       managing_group.present?
     end
 
-    def authorized_by_provisioning_group?
-      ::Feature.enabled?(:block_password_auth_for_saml_users, type: :ops) && user_detail.provisioned_by_group?
-    end
-
     def managed_by?(user)
       self.group_managed_account? && self.managing_group.owned_by?(user)
     end
@@ -318,7 +314,7 @@ def admin_unsubscribe!
     override :allow_password_authentication_for_web?
     def allow_password_authentication_for_web?(*)
       return false if group_managed_account?
-      return false if authorized_by_provisioning_group?
+      return false if user_authorized_by_provisioning_group?
 
       super
     end
@@ -326,11 +322,19 @@ def allow_password_authentication_for_web?(*)
     override :allow_password_authentication_for_git?
     def allow_password_authentication_for_git?(*)
       return false if group_managed_account?
-      return false if authorized_by_provisioning_group?
+      return false if user_authorized_by_provisioning_group?
 
       super
     end
 
+    def user_authorized_by_provisioning_group?
+      ::Feature.enabled?(:block_password_auth_for_saml_users, type: :ops) && user_detail.provisioned_by_group?
+    end
+
+    def authorized_by_provisioning_group?(group)
+      ::Feature.enabled?(:block_password_auth_for_saml_users, type: :ops) && provisioned_by_group == group
+    end
+
     def gitlab_employee?
       strong_memoize(:gitlab_employee) do
         ::Gitlab.com? && ::Feature.enabled?(:gitlab_employee_badge) && gitlab_team_member?
diff --git a/ee/lib/gitlab/auth/group_saml/user.rb b/ee/lib/gitlab/auth/group_saml/user.rb
index 6f05d1966a8ce8..2d6085f60e6ea2 100644
--- a/ee/lib/gitlab/auth/group_saml/user.rb
+++ b/ee/lib/gitlab/auth/group_saml/user.rb
@@ -52,9 +52,9 @@ def identity
         def find_by_email
           user = super
 
-          return user if user&.authorized_by_provisioning_group? && user&.provisioned_by_group_id == saml_provider.group_id
+          return user if user&.authorized_by_provisioning_group?(saml_provider.group)
 
-          false
+          nil
         end
 
         override :build_new_user
-- 
GitLab


From 4b32659fe81e0f6141eb0b0f4f73bda6b4c181ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Mon, 3 May 2021 13:58:01 +0200
Subject: [PATCH 7/9] Add cr remarks

---
 ee/lib/gitlab/auth/group_saml/user.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ee/lib/gitlab/auth/group_saml/user.rb b/ee/lib/gitlab/auth/group_saml/user.rb
index 2d6085f60e6ea2..23f9c2a0a6dcfe 100644
--- a/ee/lib/gitlab/auth/group_saml/user.rb
+++ b/ee/lib/gitlab/auth/group_saml/user.rb
@@ -53,8 +53,6 @@ def find_by_email
           user = super
 
           return user if user&.authorized_by_provisioning_group?(saml_provider.group)
-
-          nil
         end
 
         override :build_new_user
-- 
GitLab


From 18c3f9a4964befdd18f1dbe2a031cca2a614b0bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Mon, 3 May 2021 14:05:06 +0200
Subject: [PATCH 8/9] Remove changelog entry

---
 .../300191-enterprise-users-disallow-password-auth.yml      | 6 ------
 1 file changed, 6 deletions(-)
 delete mode 100644 changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml

diff --git a/changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml b/changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml
deleted file mode 100644
index 9955bf9124e1ea..00000000000000
--- a/changelogs/unreleased/300191-enterprise-users-disallow-password-auth.yml
+++ /dev/null
@@ -1,6 +0,0 @@
----
-title: Disallow password authentication for users provisioned by group behind a feature
-  flag
-merge_request: 59673
-author:
-type: added
-- 
GitLab


From 6f071f20d854ee345704ad2050e88fc1b8902fa4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Mon, 3 May 2021 20:24:19 +0200
Subject: [PATCH 9/9] Add cr remarks

---
 ee/lib/gitlab/auth/group_saml/user.rb         |  3 +-
 .../groups/members/leave_group_spec.rb        |  5 +-
 ee/spec/models/ee/user_spec.rb                | 84 +++++++++++++++++++
 3 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/ee/lib/gitlab/auth/group_saml/user.rb b/ee/lib/gitlab/auth/group_saml/user.rb
index 23f9c2a0a6dcfe..aeaaa6e9629cf4 100644
--- a/ee/lib/gitlab/auth/group_saml/user.rb
+++ b/ee/lib/gitlab/auth/group_saml/user.rb
@@ -51,8 +51,9 @@ def identity
         override :find_by_email
         def find_by_email
           user = super
+          return unless user&.authorized_by_provisioning_group?(saml_provider.group)
 
-          return user if user&.authorized_by_provisioning_group?(saml_provider.group)
+          user
         end
 
         override :build_new_user
diff --git a/ee/spec/features/groups/members/leave_group_spec.rb b/ee/spec/features/groups/members/leave_group_spec.rb
index 6834f7a52184be..4b2ccb0d2c5cf0 100644
--- a/ee/spec/features/groups/members/leave_group_spec.rb
+++ b/ee/spec/features/groups/members/leave_group_spec.rb
@@ -5,9 +5,10 @@
 RSpec.describe 'Groups > Members > Leave group' do
   include Spec::Support::Helpers::Features::MembersHelpers
 
+  let_it_be(:other_user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+
   let(:user) { create(:user) }
-  let(:other_user) { create(:user) }
-  let(:group) { create(:group) }
 
   before do
     user.update!(provisioned_by_group: group)
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index d271ab1117d585..0674c668006ccf 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -814,6 +814,90 @@
     end
   end
 
+  describe '#user_authorized_by_provisioning_group?' do
+    context 'when user is provisioned by group' do
+      before do
+        user.user_detail.provisioned_by_group = build(:group)
+      end
+
+      it 'is true' do
+        expect(user.user_authorized_by_provisioning_group?).to eq true
+      end
+
+      context 'with feature flag switched off' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: false)
+        end
+
+        it 'is false' do
+          expect(user.user_authorized_by_provisioning_group?).to eq false
+        end
+      end
+    end
+
+    context 'when user is not provisioned by group' do
+      it 'is false' do
+        expect(user.user_authorized_by_provisioning_group?).to eq false
+      end
+
+      context 'with feature flag switched off' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: false)
+        end
+
+        it 'is false' do
+          expect(user.user_authorized_by_provisioning_group?).to eq false
+        end
+      end
+    end
+  end
+
+  describe '#authorized_by_provisioning_group?' do
+    let_it_be(:group) { create(:group) }
+
+    context 'when user is provisioned by group' do
+      before do
+        user.user_detail.provisioned_by_group = group
+      end
+
+      it 'is true' do
+        expect(user.authorized_by_provisioning_group?(group)).to eq true
+      end
+
+      context 'when other group is provided' do
+        it 'is false' do
+          expect(user.authorized_by_provisioning_group?(create(:group))).to eq false
+        end
+      end
+
+      context 'with feature flag switched off' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: false)
+        end
+
+        it 'is false' do
+          expect(user.authorized_by_provisioning_group?(group)).to eq false
+        end
+      end
+    end
+
+    context 'when user is not provisioned by group' do
+      it 'is false' do
+        expect(user.authorized_by_provisioning_group?(group)).to eq false
+      end
+
+      context 'with feature flag switched off' do
+        before do
+          stub_feature_flags(block_password_auth_for_saml_users: false)
+        end
+
+        it 'is false' do
+          expect(user.authorized_by_provisioning_group?(group)).to eq false
+        end
+      end
+    end
+  end
+
   describe '#using_license_seat?' do
     let(:user) { create(:user) }
 
-- 
GitLab