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

app/controllers/concerns/membership_actions.rb

@@ -186,3 +186,5 @@ module MembershipActions
     end
   end
 end
+
+MembershipActions.prepend_if_ee('EE::MembershipActions')

config/feature_flags/ops/block_password_auth_for_saml_users.yml

+---
+name: block_password_auth_for_saml_users
+introduced_by_url:
+rollout_issue_url:
+milestone: '13.11'
+type: ops
+group: group::access
+default_enabled: false

ee/app/controllers/concerns/ee/membership_actions.rb

+# 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

ee/app/controllers/groups/sso_controller.rb

@@ -33,7 +33,11 @@ class Groups::SsoController < Groups::ApplicationController
 
     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

ee/app/models/ee/user.rb

@@ -298,6 +298,10 @@ module EE
       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 @@ module EE
     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 @@ module EE
     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

ee/app/models/ee/user_detail.rb

@@ -7,5 +7,9 @@ module EE
     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

ee/lib/gitlab/auth/group_saml/user.rb

@@ -17,8 +17,9 @@ module Gitlab
 
         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 @@ module Gitlab
         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 @@ module Gitlab
           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 @@ module Gitlab
           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

ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb

@@ -115,6 +115,48 @@ RSpec.describe Groups::OmniauthCallbacksController do
       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)

ee/spec/controllers/groups/sso_controller_spec.rb

@@ -34,12 +34,41 @@ RSpec.describe Groups::SsoController do
       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

ee/spec/features/groups/members/leave_group_spec.rb

+# 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

ee/spec/lib/gitlab/auth/group_saml/user_spec.rb

 # 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 @@ RSpec.describe Gitlab::Auth::GroupSaml::User do
       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 @@ RSpec.describe Gitlab::Auth::GroupSaml::User do
         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

ee/spec/models/ee/user_spec.rb

@@ -753,25 +753,65 @@ RSpec.describe User do
   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

ee/spec/models/user_detail_spec.rb

@@ -4,4 +4,20 @@ require 'spec_helper'
 
 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

lib/gitlab/auth/o_auth/user.rb

@@ -115,6 +115,8 @@ module Gitlab
             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

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)