Merge branch 'ldap_overrides_by_owner_only' into 'master'

Only group owners can set ldap overrides

See merge request !1025

app/policies/ee/group_policy.rb

+module EE
+  module GroupPolicy
+    def rules
+      raise NotImplementedError unless defined?(super)
+
+      super
+
+      return unless @user
+
+      if @subject.ldap_synced?
+        cannot! :admin_group_member
+        can! :override_group_member if @user.admin? || @subject.has_owner?(@user)
+      end
+    end
+  end
+end

app/policies/group_policy.rb

 class GroupPolicy < BasePolicy
+  prepend EE::GroupPolicy
+
   def rules
     can! :read_group if @subject.public?
     return unless @user
@@ -34,8 +36,6 @@ class GroupPolicy < BasePolicy
     if globally_viewable && @subject.request_access_enabled && !member
       can! :request_access
     end
-
-    additional_rules!(master)
   end
 
   def can_read_group?
@@ -47,11 +47,4 @@ class GroupPolicy < BasePolicy
 
     GroupProjectsFinder.new(@subject).execute(@user).any?
   end
-
-  def additional_rules!(master)
-    if @subject.ldap_synced?
-      cannot! :admin_group_member
-      can! :override_group_member if master
-    end
-  end
 end

changelogs/unreleased-ee/ldap_overrides_by_owner_only.yml

+---
+title: Only admins or group owners can set LDAP overrides
+merge_request:
+author:

spec/policies/ee/group_policy_spec.rb

+require 'spec_helper'
+
+describe GroupPolicy, models: true do
+  let(:guest) { create(:user) }
+  let(:reporter) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:master) { create(:user) }
+  let(:owner) { create(:user) }
+  let(:auditor) { create(:user, :auditor) }
+  let(:admin) { create(:admin) }
+  let(:group) { create(:group) }
+
+  before do
+    group.add_guest(guest)
+    group.add_reporter(reporter)
+    group.add_developer(developer)
+    group.add_master(master)
+    group.add_owner(owner)
+  end
+
+  subject { described_class.abilities(current_user, group).to_set }
+
+  context 'when LDAP sync is not enabled' do
+    context 'owner' do
+      let(:current_user) { owner }
+
+      it { is_expected.not_to include(:override_group_member) }
+    end
+
+    context 'admin' do
+      let(:current_user) { admin }
+
+      it { is_expected.not_to include(:override_group_member) }
+    end
+  end
+
+  context 'when LDAP sync is enabled' do
+    before do
+      allow(group).to receive(:ldap_synced?).and_return(true)
+    end
+
+    context 'with no user' do
+      let(:current_user) { nil }
+
+      it { is_expected.not_to include(:override_group_member) }
+    end
+
+    context 'guests' do
+      let(:current_user) { guest }
+
+      it { is_expected.not_to include(:override_group_member) }
+    end
+
+    context 'reporter' do
+      let(:current_user) { reporter }
+
+      it { is_expected.not_to include(:override_group_member) }
+    end
+
+    context 'developer' do
+      let(:current_user) { developer }
+
+      it { is_expected.not_to include(:override_group_member) }
+    end
+
+    context 'master' do
+      let(:current_user) { master }
+
+      it { is_expected.not_to include(:override_group_member) }
+    end
+
+    context 'owner' do
+      let(:current_user) { owner }
+
+      it { is_expected.to include(:override_group_member) }
+    end
+
+    context 'admin' do
+      let(:current_user) { admin }
+
+      it { is_expected.to include(:override_group_member) }
+    end
+  end
+end