Merge branch '918-restrict-group-owners-to-admins' into 'master'

Adds restrict group owners to admins option to admin application settings dashboard

Closes #918

See merge request !2529

app/controllers/groups/ldap_group_links_controller.rb

@@ -2,6 +2,7 @@ class Groups::LdapGroupLinksController < Groups::ApplicationController
   before_action :group
   before_action :require_ldap_enabled
   before_action :authorize_admin_group!
+  before_action :authorize_manage_ldap_group_links!
   layout 'group_settings'
@@ -31,6 +32,12 @@ def destroy
   private
+  def authorize_manage_ldap_group_links!
+    unless can?(current_user, :admin_ldap_group_links, group)
+      return render_404
+    end
+  end
+
   def require_ldap_enabled
     render_404 unless Gitlab.config.ldap.enabled
   end

app/helpers/ee/application_settings_helper.rb

@@ -21,7 +21,8 @@ def visible_attributes
         :slack_app_enabled,
         :slack_app_id,
         :slack_app_secret,
-        :slack_app_verification_token
+        :slack_app_verification_token,
+        :allow_group_owners_to_manage_ldap
       ]
     end

app/helpers/members_helper.rb

 module MembersHelper
   # Returns a `<action>_<source>_member` association, e.g.:
   # - admin_project_member, update_project_member, destroy_project_member
-  # - admin_group_member, update_group_member, destroy_group_member
+  # - admin_group_member, update_group_member, destroy_group_member, override_group_member
   def action_member_permission(action, member)
     "#{action}_#{member.type.underscore}".to_sym
   end

app/models/ee/application_setting.rb

@@ -39,7 +39,8 @@ def defaults
           repository_size_limit: 0,
           mirror_max_delay: Settings.gitlab['mirror_max_delay'],
           mirror_max_capacity: Settings.gitlab['mirror_max_capacity'],
-          mirror_capacity_threshold: Settings.gitlab['mirror_capacity_threshold']
+          mirror_capacity_threshold: Settings.gitlab['mirror_capacity_threshold'],
+          allow_group_owners_to_manage_ldap: true
         )
       end
     end

app/policies/ee/group_policy.rb

@@ -6,19 +6,19 @@ module GroupPolicy
       with_scope :subject
       condition(:ldap_synced) { @subject.ldap_synced? }
-      rule { ldap_synced }.prevent :admin_group_member
-
-      rule { ldap_synced & admin }.policy do
-        enable :override_group_member
-        enable :update_group_member
-      end
-
-      rule { ldap_synced & owner }.policy do
-        enable :override_group_member
-        enable :update_group_member
+      condition(:can_owners_manage_ldap, scope: :global) do
+        current_application_settings.allow_group_owners_to_manage_ldap
       end
       rule { auditor }.enable :read_group
+
+      rule { admin | (can_owners_manage_ldap & owner) }.enable :admin_ldap_group_links
+
+      rule { ldap_synced }.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
     end
   end
 end

app/views/admin/application_settings/_form.html.haml

@@ -48,6 +48,17 @@
         = select(:application_setting, :enabled_git_access_protocol, [['Both SSH and HTTP(S)', nil], ['Only SSH', 'ssh'], ['Only HTTP(S)', 'http']], {}, class: 'form-control')
         %span.help-block#clone-protocol-help
           Allow only the selected protocols to be used for Git access.
+    - if ldap_enabled?
+      .form-group
+        = f.label :allow_group_owners_to_manage_ldap, 'LDAP settings', class: 'control-label col-sm-2'
+        .col-sm-10
+          .checkbox
+            = f.label :allow_group_owners_to_manage_ldap do
+              = f.check_box :allow_group_owners_to_manage_ldap
+              Allow group owners to manage LDAP-related settings
+          %span.help-block
+            If checked, group owners can manage LDAP group links and LDAP member overrides
+            = link_to icon('question-circle'), help_page_path('administration/auth/ldap-ee')
   %fieldset
     %legend Account and Limit Settings

app/views/groups/ee/_settings_nav.html.haml

-- if Gitlab::LDAP::Config.enabled_extras?
+- if Gitlab::LDAP::Config.enabled_extras? && can?(current_user, :admin_ldap_group_links, @group)
   = nav_link(path: 'ldap_group_links#index') do
     = link_to group_ldap_group_links_path(@group), title: 'LDAP Group' do
       %span

changelogs/unreleased-ee/918-restrict-group-owners-to-admins.yml

+---
+title: Add admin application setting to allow group owners to manage LDAP.
+merge_request: 2529
+author:

db/migrate/20170726111039_add_restrict_group_owners_to_admins_option_to_application_settings.rb

+class AddRestrictGroupOwnersToAdminsOptionToApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:application_settings, :allow_group_owners_to_manage_ldap, :boolean, default: true)
+  end
+
+  def down
+    remove_column(:application_settings, :allow_group_owners_to_manage_ldap)
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20170719182937) do
+ActiveRecord::Schema.define(version: 20170726111039) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -149,6 +149,7 @@
     t.string "slack_app_verification_token"
     t.integer "performance_bar_allowed_group_id"
     t.boolean "password_authentication_enabled"
+    t.boolean "allow_group_owners_to_manage_ldap", default: true, null: false
   end
   create_table "approvals", force: :cascade do |t|

spec/controllers/ee/admin/application_settings_controller_spec.rb

@@ -33,7 +33,8 @@
           slack_app_enabled: true,
           slack_app_id: 'slack_app_id',
           slack_app_secret: 'slack_app_secret',
-          slack_app_verification_token: 'slack_app_verification_token'
+          slack_app_verification_token: 'slack_app_verification_token',
+          allow_group_owners_to_manage_ldap: false
       }
       put :update, application_setting: settings

spec/features/admin/admin_settings_spec.rb

@@ -29,6 +29,29 @@
     expect(find('#application_setting_visibility_level_20')).not_to be_checked
   end
+  describe 'LDAP settings' do
+    context 'with LDAP enabled' do
+      scenario 'Change allow group owners to manage ldap' do
+        allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
+        visit admin_application_settings_path
+
+        find('#application_setting_allow_group_owners_to_manage_ldap').set(false)
+        click_button 'Save'
+
+        expect(page).to have_content('Application settings saved successfully')
+        expect(find('#application_setting_allow_group_owners_to_manage_ldap')).not_to be_checked
+      end
+    end
+
+    context 'with LDAP disabled' do
+      scenario 'Does not show option to allow group owners to manage ldap' do
+        visit admin_application_settings_path
+
+        expect(page).not_to have_css('#application_setting_allow_group_owners_to_manage_ldap')
+      end
+    end
+  end
+
   scenario 'Change application settings' do
     uncheck 'Gravatar enabled'
     fill_in 'Home page URL', with: 'https://about.gitlab.com/'

spec/features/groups/group_settings_spec.rb

@@ -9,6 +9,31 @@
     sign_in(user)
   end
+  describe 'navbar' do
+    context 'with LDAP enabled' do
+      before do
+        allow_any_instance_of(Group).to receive(:ldap_synced?).and_return(true)
+        allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
+      end
+
+      scenario 'is able to navigate to LDAP group section' do
+        visit edit_group_path(group)
+
+        expect(find('div.sub-nav')).to have_content('LDAP Group')
+      end
+
+      context 'with owners not being able to manage LDAP' do
+        scenario 'is not able to navigate to LDAP group section' do
+          stub_application_setting(allow_group_owners_to_manage_ldap: false)
+
+          visit edit_group_path(group)
+
+          expect(find('div.sub-nav')).not_to have_content('LDAP Group')
+        end
+      end
+    end
+  end
+
   describe 'when the group path is changed' do
     let(:new_group_path) { 'bar' }
     let(:old_group_full_path) { "/#{group.path}" }

spec/features/groups/members/override_ldap_memberships_spec.rb

@@ -26,6 +26,18 @@
     expect(page).not_to have_button 'Edit permissions'
   end
+  scenario 'owner cannot override LDAP access level', js: true do
+    stub_application_setting(allow_group_owners_to_manage_ldap: false)
+
+    visit group_group_members_path(group)
+
+    within "#group_member_#{ldap_member.id}" do
+      expect(page).not_to have_content 'LDAP'
+      expect(page).not_to have_button 'Guest'
+      expect(page).not_to have_button 'Edit permissions'
+    end
+  end
+
   scenario 'owner can override LDAP access level', js: true do
     ldap_override_message = 'John Doe is currently an LDAP user. Editing their permissions will override the settings from the LDAP group sync.'

spec/policies/ee/group_policy_spec.rb

@@ -25,12 +25,22 @@
       let(:current_user) { owner }
       it { is_expected.to be_disallowed(:override_group_member) }
+      it { is_expected.to be_allowed(:admin_ldap_group_links) }
+
+      context 'does not allow group owners to manage ldap' do
+        before do
+          stub_application_setting(allow_group_owners_to_manage_ldap: false)
+        end
+
+        it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+      end
     end
     context 'admin' do
       let(:current_user) { admin }
       it { is_expected.to be_disallowed(:override_group_member) }
+      it { is_expected.to be_allowed(:admin_ldap_group_links) }
     end
   end
@@ -43,42 +53,59 @@
       let(:current_user) { nil }
       it { is_expected.to be_disallowed(:override_group_member) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_links) }
     end
     context 'guests' do
       let(:current_user) { guest }
       it { is_expected.to be_disallowed(:override_group_member) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_links) }
     end
     context 'reporter' do
       let(:current_user) { reporter }
       it { is_expected.to be_disallowed(:override_group_member) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_links) }
     end
     context 'developer' do
       let(:current_user) { developer }
       it { is_expected.to be_disallowed(:override_group_member) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_links) }
     end
     context 'master' do
       let(:current_user) { master }
       it { is_expected.to be_disallowed(:override_group_member) }
+      it { is_expected.to be_disallowed(:admin_ldap_group_links) }
     end
     context 'owner' do
       let(:current_user) { owner }
-      it { is_expected.to be_allowed(:override_group_member) }
+      context 'allow group owners to manage ldap' do
+        it { is_expected.to be_allowed(:override_group_member) }
+      end
+
+      context 'does not allow group owners to manage ldap' do
+        before do
+          stub_application_setting(allow_group_owners_to_manage_ldap: false)
+        end
+
+        it { is_expected.to be_disallowed(:override_group_member) }
+        it { is_expected.to be_disallowed(:admin_ldap_group_links) }
+      end
     end
     context 'admin' do
       let(:current_user) { admin }
       it { is_expected.to be_allowed(:override_group_member) }
+      it { is_expected.to be_allowed(:admin_ldap_group_links) }
     end
   end
 end