Merge branch '300191-enterprise-users-disallow-password-auth' into 'master'

Resolve "Enterprise users - Disallow Password Auth" [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!59673

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 current_user.authorized_by_provisioning_group?(membershipable)
+        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_provisioning_group?(unauthenticated_group)
+      sign_out current_user
+    else
+      redirect_to profile_account_path
+    end
   end
 
   def sign_up_form

ee/app/models/ee/user.rb

@@ -314,6 +314,7 @@ module EE
     override :allow_password_authentication_for_web?
     def allow_password_authentication_for_web?(*)
       return false if group_managed_account?
+      return false if user_authorized_by_provisioning_group?
 
       super
     end
@@ -321,10 +322,19 @@ module EE
     override :allow_password_authentication_for_git?
     def allow_password_authentication_for_git?(*)
       return false if group_managed_account?
+      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?

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,14 @@ module Gitlab
           end
         end
 
+        override :find_by_email
+        def find_by_email
+          user = super
+          return unless user&.authorized_by_provisioning_group?(saml_provider.group)
+
+          user
+        end
+
         override :build_new_user
         def build_new_user(skip_confirmation: false)
           super.tap do |user|
@@ -72,6 +81,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 user 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 'does not add linked identity' do
+        expect { post provider, params: { group_id: group } }.not_to change(linked_accounts, :count)
+      end
+
+      it 'does not add 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_it_be(:other_user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+
+  let(:user) { create(:user) }
+
+  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 and is signed off' 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 and is signed off', :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,24 +753,148 @@ 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 '#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
 

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)