Merge branch '1793-ldap-lock-owner-policies-and-controller' into 'master'

Policies and controllers for unlock membership to ldap for Groups

See merge request gitlab-org/gitlab!26520

ee/app/controllers/groups/ldap_settings_controller.rb

+# frozen_string_literal: true
+
+class Groups::LdapSettingsController < Groups::ApplicationController
+  before_action :group
+  before_action :require_ldap_enabled
+  before_action :authorize_admin_group!
+  before_action :authorize_manage_ldap_settings!
+
+  def update
+    if @group.update(ldap_settings_params)
+      redirect_back_or_default(default: group_ldap_group_links_path(@group), options: { notice: _('LDAP settings updated') })
+    else
+      redirect_back_or_default(default: group_ldap_group_links_path(@group), options: { alert: _('Could not update the LDAP settings') })
+    end
+  end
+
+  private
+
+  def authorize_manage_ldap_settings!
+    render_404 unless Feature.enabled?(:ldap_settings_unlock_groups_by_owners)
+    render_404 unless can?(current_user, :admin_ldap_group_settings, group)
+  end
+
+  def require_ldap_enabled
+    render_404 unless Gitlab::Auth::Ldap::Config.enabled?
+  end
+
+  def ldap_settings_params
+    attrs = %i[unlock_membership_to_ldap]
+
+    params.require(:group).permit(attrs)
+  end
+end

ee/app/policies/ee/group_policy.rb

@@ -30,6 +30,10 @@ module EE
         ::Gitlab::CurrentSettings.lock_memberships_to_ldap?
       end
 
+      condition(:owners_bypass_ldap_lock) do
+        ldap_lock_bypassable?
+      end
+
       condition(:security_dashboard_enabled) do
         @subject.feature_available?(:security_dashboard)
       end
@@ -130,15 +134,18 @@ module EE
 
       rule { admin | owner }.enable :admin_group_saml
 
-      rule { admin | (can_owners_manage_ldap & owner) }.enable :admin_ldap_group_links
+      rule { admin | (can_owners_manage_ldap & owner) }.policy do
+        enable :admin_ldap_group_links
+        enable :admin_ldap_group_settings
+      end
 
-      rule { ldap_synced }.prevent :admin_group_member
+      rule { ldap_synced & ~owners_bypass_ldap_lock }.prevent :admin_group_member
 
       rule { ldap_synced & (admin | owner) }.enable :update_group_member
 
       rule { ldap_synced & (admin | (can_owners_manage_ldap & owner)) }.enable :override_group_member
 
-      rule { memberships_locked_to_ldap & ~admin }.policy do
+      rule { memberships_locked_to_ldap & ~admin & ~owners_bypass_ldap_lock }.policy do
         prevent :admin_group_member
         prevent :update_group_member
         prevent :override_group_member
@@ -179,6 +186,13 @@ module EE
       super
     end
 
+    def ldap_lock_bypassable?
+      return false unless ::Feature.enabled?(:ldap_settings_unlock_groups_by_owners)
+      return false unless ::Gitlab::CurrentSettings.allow_group_owners_to_manage_ldap?
+
+      !!subject.unlock_membership_to_ldap? && subject.owned_by?(user)
+    end
+
     def sso_enforcement_prevents_access?
       return false unless subject.persisted?
       return false if user&.admin?

ee/config/routes/group.rb

@@ -42,6 +42,8 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
       end
     end
 
+    resource :ldap_settings, only: [:update]
+
     resource :issues_analytics, only: [:show]
 
     resource :insights, only: [:show], trailing_slash: true do

ee/spec/controllers/groups/ldap_settings_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Groups::LdapSettingsController do
+  include LdapHelpers
+
+  let(:group) { create(:group) }
+  let(:user)  { create(:user) }
+
+  before do
+    stub_ldap_setting(enabled: true)
+    stub_feature_flags(ldap_settings_unlock_groups_by_owners: true)
+
+    sign_in(user)
+  end
+
+  describe 'PUT #update' do
+    describe 'as an owner' do
+      before do
+        group.add_owner(user)
+      end
+
+      describe 'admin allows owners to modify ldap settings' do
+        before do
+          allow(::Gitlab::CurrentSettings).to receive(:allow_group_owners_to_manage_ldap?).and_return(true)
+        end
+
+        it 'changes the value of unlock_membership_to_ldap' do
+          expect do
+            put :update, params: { group_id: group.to_param, group: { unlock_membership_to_ldap: true } }
+          end.to change { group.reload.unlock_membership_to_ldap }
+        end
+
+        describe 'ldap_settings_unlock_groups_by_owners is disabled' do
+          before do
+            stub_feature_flags(ldap_settings_unlock_groups_by_owners: false)
+          end
+
+          it 'does not change the value of the unlock_membership_to_ldap' do
+            expect do
+              put :update, params: { group_id: group.to_param, group: { unlock_membership_to_ldap: true } }
+            end.not_to change { group.reload.unlock_membership_to_ldap }
+          end
+        end
+      end
+
+      describe 'admin disallow owners to modify ldap settings' do
+        before do
+          allow(::Gitlab::CurrentSettings).to receive(:allow_group_owners_to_manage_ldap?).and_return(false)
+        end
+
+        it 'does not change the value of unlock_membership_to_ldap' do
+          expect do
+            put :update, params: { group_id: group.to_param, group: { unlock_membership_to_ldap: true } }
+          end.not_to change { group.reload.unlock_membership_to_ldap }
+        end
+      end
+    end
+
+    describe 'as a maintainer' do
+      before do
+        group.add_maintainer(user)
+        allow(::Gitlab::CurrentSettings).to receive(:allow_group_owners_to_manage_ldap?).and_return(true)
+      end
+
+      it 'does not change the value of unlock_membership_to_ldap' do
+        expect do
+          put :update, params: { group_id: group.to_param, group: { unlock_membership_to_ldap: true } }
+        end.not_to change { group.reload.unlock_membership_to_ldap }
+      end
+    end
+  end
+end

ee/spec/policies/group_policy_spec.rb

@@ -273,6 +273,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_disallowed(:override_group_member) }
       it { is_expected.to be_allowed(:admin_ldap_group_links) }
+      it { is_expected.to be_allowed(:admin_ldap_group_settings) }
 
       context 'does not allow group owners to manage ldap' do
         before do
@@ -280,6 +281,7 @@ describe GroupPolicy do
         end
 
         it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+        it { is_expected.to be_disallowed(:admin_ldap_group_settings) }
       end
     end
 
@@ -288,6 +290,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_disallowed(:override_group_member) }
       it { is_expected.to be_allowed(:admin_ldap_group_links) }
+      it { is_expected.to be_allowed(:admin_ldap_group_settings) }
     end
   end
 
@@ -301,6 +304,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_disallowed(:override_group_member) }
       it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_settings) }
     end
 
     context 'guests' do
@@ -308,6 +312,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_disallowed(:override_group_member) }
       it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_settings) }
     end
 
     context 'reporter' do
@@ -315,6 +320,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_disallowed(:override_group_member) }
       it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_settings) }
     end
 
     context 'developer' do
@@ -322,6 +328,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_disallowed(:override_group_member) }
       it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_settings) }
     end
 
     context 'maintainer' do
@@ -329,6 +336,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_disallowed(:override_group_member) }
       it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_settings) }
     end
 
     context 'owner' do
@@ -345,6 +353,7 @@ describe GroupPolicy do
 
         it { is_expected.to be_disallowed(:override_group_member) }
         it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+        it { is_expected.to be_disallowed(:admin_ldap_group_settings) }
       end
     end
 
@@ -353,6 +362,7 @@ describe GroupPolicy do
 
       it { is_expected.to be_allowed(:override_group_member) }
       it { is_expected.to be_allowed(:admin_ldap_group_links) }
+      it { is_expected.to be_allowed(:admin_ldap_group_settings) }
     end
 
     context 'when memberships locked to LDAP' do
@@ -375,6 +385,53 @@ describe GroupPolicy do
         it { is_expected.not_to be_allowed(:override_group_member) }
         it { is_expected.not_to be_allowed(:update_group_member) }
       end
+
+      context 'when LDAP sync is enabled' do
+        let(:current_user) { owner }
+
+        before do
+          allow(group).to receive(:ldap_synced?).and_return(true)
+        end
+
+        context 'Group Owner disable membership lock' do
+          before do
+            group.update!(unlock_membership_to_ldap: true)
+            stub_feature_flags(ldap_settings_unlock_groups_by_owners: true)
+          end
+
+          it { is_expected.to be_allowed(:admin_group_member) }
+          it { is_expected.to be_allowed(:override_group_member) }
+          it { is_expected.to be_allowed(:update_group_member) }
+
+          context 'ldap_settings_unlock_groups_by_owners is disabled' do
+            before do
+              stub_feature_flags(ldap_settings_unlock_groups_by_owners: false)
+            end
+
+            it { is_expected.to be_disallowed(:admin_group_member) }
+            it { is_expected.to be_disallowed(:override_group_member) }
+            it { is_expected.to be_disallowed(:update_group_member) }
+          end
+        end
+
+        context 'Group Owner keeps the membership lock' do
+          before do
+            group.update!(unlock_membership_to_ldap: false)
+          end
+
+          it { is_expected.not_to be_allowed(:admin_group_member) }
+          it { is_expected.not_to be_allowed(:override_group_member) }
+          it { is_expected.not_to be_allowed(:update_group_member) }
+        end
+      end
+
+      context 'when LDAP sync is disable' do
+        let(:current_user) { owner }
+
+        it { is_expected.not_to be_allowed(:admin_group_member) }
+        it { is_expected.not_to be_allowed(:override_group_member) }
+        it { is_expected.not_to be_allowed(:update_group_member) }
+      end
     end
   end
 

locale/gitlab.pot

@@ -5760,6 +5760,9 @@ msgstr ""
 msgid "Could not save prometheus manual configuration"
 msgstr ""
 
+msgid "Could not update the LDAP settings"
+msgstr ""
+
 msgid "Could not upload your designs as one or more files uploaded are not supported."
 msgstr ""
 
@@ -11585,6 +11588,9 @@ msgstr ""
 msgid "LDAP settings"
 msgstr ""
 
+msgid "LDAP settings updated"
+msgstr ""
+
 msgid "LDAP sync in progress. This could take a few minutes. Refresh the page to see the changes."
 msgstr ""