diff --git a/app/assets/javascripts/members.js.es6 b/app/assets/javascripts/members.js.es6 index e3f367a11eb94637229e1ac77c12f1e2272a1930..b097eb765f32e156129530fd4f9d367d418cc3ed 100644 --- a/app/assets/javascripts/members.js.es6 +++ b/app/assets/javascripts/members.js.es6 @@ -1,4 +1,6 @@ /* eslint-disable class-methods-use-this */ +/* eslint-disable no-new */ +/* global Flash */ (() => { window.gl = window.gl || {}; @@ -9,6 +11,8 @@ } addListeners() { + $('.js-ldap-permissions').off('click').on('click', this.showLDAPPermissionsWarning.bind(this)); + $('.js-ldap-override').off('click').on('click', this.toggleMemberAccessToggle.bind(this)); $('.project_member, .group_member').off('ajax:success').on('ajax:success', this.removeRow); $('.js-member-update-control').off('change').on('change', this.formSubmit.bind(this)); $('.js-edit-member-form').off('ajax:success').on('ajax:success', this.formSuccess.bind(this)); @@ -22,6 +26,10 @@ $btn.glDropdown({ selectable: true, isSelectable(selected, $el) { + if ($el.data('revert')) { + return false; + } + return !$el.hasClass('is-active'); }, fieldName: $btn.data('field-name'), @@ -29,10 +37,26 @@ return $el.data('id'); }, toggleLabel(selected, $el) { + if ($el.data('revert')) { + return $btn.text(); + } + return $el.text(); }, clicked: (selected, $link) => { - this.formSubmit(null, $link); + if (!$link.data('revert')) { + this.formSubmit(null, $link); + } else { + const { $memberListItem, $toggle, $dateInput } = this.getMemberListItems($link); + + $toggle.disable(); + $dateInput.disable(); + + this.overrideLdap($memberListItem, $link.data('endpoint'), false).fail(() => { + $toggle.enable(); + $dateInput.enable(); + }); + } }, }); }); @@ -66,6 +90,14 @@ $dateInput.enable(); } + showLDAPPermissionsWarning(e) { + const $btn = $(e.currentTarget); + const { $memberListItem } = this.getMemberListItems($btn); + const $ldapPermissionsElement = $memberListItem.next(); + + $ldapPermissionsElement.toggle(); + } + getMemberListItems($el) { const $memberListItem = $el.is('.member') ? $el : $(`#${$el.data('el-id')}`); @@ -75,6 +107,42 @@ $dateInput: $memberListItem.find('.js-access-expiration-date'), }; } + + toggleMemberAccessToggle(e) { + const $btn = $(e.currentTarget); + const { $memberListItem, $toggle, $dateInput } = this.getMemberListItems($btn); + + $btn.disable(); + + this.overrideLdap($memberListItem, $btn.data('endpoint'), true).then(() => { + this.showLDAPPermissionsWarning(e); + + $toggle.enable(); + $dateInput.enable(); + }).fail((xhr) => { + $btn.enable(); + + if (xhr.status === 403) { + new Flash('You do not have the correct permissions to override the settings from the LDAP group sync.', 'alert'); + } else { + new Flash('An error occured whilst saving LDAP override status. Please try again.', 'alert'); + } + }); + } + + overrideLdap($memberListitem, endpoint, override) { + return $.ajax({ + url: endpoint, + type: 'PATCH', + data: { + group_member: { + override, + }, + }, + }).then(() => { + $memberListitem.toggleClass('is-overriden', override); + }); + } } gl.Members = Members; diff --git a/app/assets/stylesheets/framework/lists.scss b/app/assets/stylesheets/framework/lists.scss index ed4b60faf92c88cf58ff3fe508bb07d1a50b20dc..8409ce179eb874ac3947ac106a4b81a8a614b094 100644 --- a/app/assets/stylesheets/framework/lists.scss +++ b/app/assets/stylesheets/framework/lists.scss @@ -161,6 +161,12 @@ ul.content-list { margin-top: 3px; margin-bottom: 4px; + &.btn-ldap-override { + @media (min-width: $screen-sm-min) { + margin-bottom: 0; + } + } + &:last-child { margin-right: 0; } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index e94f0e09f2f868ae23815e130f6abf856ede1b7f..f4778944b90166a4559743cd8c80e14a8f25b601 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -197,6 +197,7 @@ $divergence-graph-bar-bg: #ccc; $divergence-graph-separator-bg: #ccc; $issue-box-upcoming-bg: #8f8f8f; $pages-group-name-color: #4c4e54; +$ldap-members-override-bg: #fff1e0; /* * Common component specific colors diff --git a/app/assets/stylesheets/pages/members.scss b/app/assets/stylesheets/pages/members.scss index 5f3bbb40ba08a6e8c9dc516632d6843cdf88c1da..dc88cdca77c0a5dea9dd65beba43dcff3aa9f4b3 100644 --- a/app/assets/stylesheets/pages/members.scss +++ b/app/assets/stylesheets/pages/members.scss @@ -4,6 +4,12 @@ } .member { + &.is-overriden { + .btn-ldap-override { + display: none!important; + } + } + .list-item-name { @media (min-width: $screen-sm-min) { float: left; @@ -100,3 +106,47 @@ border: 0; outline: 0; } + +.members-ldap { + -webkit-align-self: center; + align-self: center; + height: 100%; + margin-right: 10px; + margin-left: -49px; +} + +.alert-member-ldap { + background-color: $ldap-members-override-bg; + + @media (min-width: $screen-sm-min) { + line-height: 40px; + } + + > p { + float: left; + margin-bottom: 10px; + color: $orange-normal; + + @media (min-width: $screen-sm-min) { + padding-left: 55px; + margin-bottom: 0; + } + } + + .controls { + width: 100%; + + @media (min-width: $screen-sm-min) { + width: auto; + } + } +} + +.btn-ldap-override { + width: 100%; + + @media (min-width: $screen-sm-min) { + margin-left: 10px; + width: auto; + } +} diff --git a/app/controllers/ee/groups/group_members_controller.rb b/app/controllers/ee/groups/group_members_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..ac678ce1ad9206526f4101c14a9aeb742af8bf7d --- /dev/null +++ b/app/controllers/ee/groups/group_members_controller.rb @@ -0,0 +1,31 @@ +module EE + module Groups + module GroupMembersController + def override + @group_member = @group.group_members.find(params[:id]) + + return render_403 unless can?(current_user, :override_group_member, @group_member) + + if @group_member.update_attributes(override_params) + log_audit_event(@group_member, action: :override) + + respond_to do |format| + format.js { head :ok } + end + end + end + + protected + + def authorize_update_group_member! + unless can?(current_user, :admin_group_member, group) || can?(current_user, :override_group_member, group) + render_403 + end + end + + def override_params + params.require(:group_member).permit(:override) + end + end + end +end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index fa251813c3297ede4edcd4d8dafcd470c87f60f5..379a7e0edabb2f6260b83c2835c24109078ec99c 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -1,8 +1,11 @@ class Groups::GroupMembersController < Groups::ApplicationController + prepend EE::Groups::GroupMembersController + include MembershipActions # Authorize - before_action :authorize_admin_group_member!, except: [:index, :leave, :request_access] + before_action :authorize_admin_group_member!, except: [:index, :leave, :request_access, :update, :override] + before_action :authorize_update_group_member!, only: [:update, :override] def index @project = @group.projects.find(params[:project_id]) if params[:project_id] diff --git a/app/policies/group_member_policy.rb b/app/policies/group_member_policy.rb index 62335527654cd230a2947a5f52ff97807f0f02fc..8709f1d21cad62133c6a5404228c37d4c7610d31 100644 --- a/app/policies/group_member_policy.rb +++ b/app/policies/group_member_policy.rb @@ -15,5 +15,16 @@ def rules elsif @user == target_user can! :destroy_group_member end + + additional_rules! + end + + def additional_rules! + can_override = Ability.allowed?(@user, :override_group_member, @subject.group) + + if can_override + can! :override_group_member if @subject.ldap? + can! :update_group_member unless @subject.ldap? && !@subject.override? + end end end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index cced89212001d354431f5d03a4cace79e0276ce5..0180e10eae241fb94096c820820572c1749b4431 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -34,8 +34,7 @@ def rules can! :request_access end - # EE-only - cannot! :admin_group_member if @subject.ldap_synced? + additional_rules!(master) end def can_read_group? @@ -46,4 +45,11 @@ def can_read_group? 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 diff --git a/app/views/groups/group_members/_ldap_sync.html.haml b/app/views/groups/group_members/_ldap_sync.html.haml index 48c7318329b599030805b74bf4d8e63596401a10..d68ffe180753ccb8356873e97661ac41f39ed6a9 100644 --- a/app/views/groups/group_members/_ldap_sync.html.haml +++ b/app/views/groups/group_members/_ldap_sync.html.haml @@ -1,6 +1,6 @@ - if current_user && @group.ldap_synced? .bs-callout.bs-callout-info - The members of this group are managed using LDAP and cannot be added, changed or removed here. + The members of this group are managed using LDAP and cannot be added or removed here. Because LDAP permissions in GitLab get updated one user at a time and because GitLab caches LDAP check results, changes on your LDAP server or in this group's LDAP sync settings may take up to #{Gitlab.config.ldap['sync_time']}s to show in the list below. %ul - @group.ldap_group_links.each do |ldap_group_link| diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index 659d4c905fc9822bec6913659ae2899668bc806f..3647cb6e38c0b742b51959f9ee69090cbc198215 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -4,7 +4,10 @@ - source = member.source - can_admin_member = can?(current_user, action_member_permission(:update, member), member) -%li.member{ class: dom_class(member), id: dom_id(member) } +-# EE-only +- can_override_member = can?(current_user, action_member_permission(:override, member), member) + +%li.member{ class: [dom_class(member), ("is-overriden" if member.override)], id: dom_id(member) } %span.list-item-name - if user = image_tag avatar_icon(user, 40), class: "avatar s40", alt: '' @@ -45,6 +48,7 @@ = time_ago_with_tooltip(member.created_at) - if show_roles .controls.member-controls + = render 'shared/members/ee/ldap_tag', can_override: can_override_member, visible: false - if show_controls && (member.respond_to?(:group) && @group) || (member.respond_to?(:project) && @project) - if user != current_user = form_for member, remote: true, html: { class: 'form-horizontal js-edit-member-form' } do |f| @@ -65,6 +69,7 @@ = link_to role, "javascript:void(0)", class: ("is-active" if member.access_level == role_id), data: { id: role_id, el_id: dom_id(member) } + = render 'shared/members/ee/revert_ldap_group_sync_option', group: @group, member: member, can_override: can_override_member .prepend-left-5.clearable-input.member-form-control = f.text_field :expires_at, class: 'form-control js-access-expiration-date js-member-update-control', placeholder: 'Expiration date', id: "member_expires_at_#{member.id}", disabled: !can_admin_member, data: { el_id: dom_id(member) } %i.clear-icon.js-clear-input @@ -98,5 +103,8 @@ %span.visible-xs-block Delete = icon('trash', class: 'hidden-xs') + = render 'shared/members/ee/override_member_buttons', group: @group, member: member, user: user, action: :edit, can_override: can_override_member - else %span.member-access-text= member.human_access + += render 'shared/members/ee/override_member_buttons', group: @group, member: member, user: user, action: :confirm, can_override: can_override_member diff --git a/app/views/shared/members/ee/_ldap_tag.html.haml b/app/views/shared/members/ee/_ldap_tag.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..468c524670ed8b5833e638e866f9833afda378ab --- /dev/null +++ b/app/views/shared/members/ee/_ldap_tag.html.haml @@ -0,0 +1,10 @@ +- can_override = local_assigns.fetch(:can_override, false) +- visible = local_assigns.fetch(:visible, false) + +- if can_override + - if visible + %span.label.label-info.pull-right.visible-xs-block + LDAP + - else + %span.label.label-info.members-ldap.hidden-xs + LDAP diff --git a/app/views/shared/members/ee/_override_member_buttons.html.haml b/app/views/shared/members/ee/_override_member_buttons.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..80e8dc6d104c3714afea567677bd930484eb5768 --- /dev/null +++ b/app/views/shared/members/ee/_override_member_buttons.html.haml @@ -0,0 +1,29 @@ +- group = local_assigns.fetch(:group) +- member = local_assigns.fetch(:member) +- user = local_assigns.fetch(:user) +- action = local_assigns.fetch(:action, :edit).to_s.inquiry +- can_override = local_assigns.fetch(:can_override, false) + +- if can_override + - if action.edit? + %button.btn.btn-default.btn-ldap-override.js-ldap-permissions{ type: 'button', + 'aria-label' => 'Edit permissions', + data: { name: user.name, el_id: dom_id(member) } } + %span.visible-xs-block.visible-sm-block + Edit permissions + = icon('pencil', class: 'hidden-xs hidden-sm') + - else + %li.alert.alert-member-ldap{ style: 'display: none;' } + %p + = user.name + is currently an LDAP user. Editing their permissions will override the settings from the LDAP group sync. + .controls + %button.btn.btn-warning.btn-loading.js-ldap-override{ type: 'button', + 'aria-label' => 'Change LDAP member permissions', + data: { el_id: dom_id(member), endpoint: override_group_group_member_path(group, member) } } + = icon('spinner spin') + Change permissions + %button.btn.btn-default.js-ldap-permissions{ type: 'button', + 'aria-label' => 'Close permissions override', + data: { el_id: dom_id(member) } } + = icon('times') diff --git a/app/views/shared/members/ee/_revert_ldap_group_sync_option.html.haml b/app/views/shared/members/ee/_revert_ldap_group_sync_option.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..5a431a3ec2f56a71b185fe5bc5e1037983da3151 --- /dev/null +++ b/app/views/shared/members/ee/_revert_ldap_group_sync_option.html.haml @@ -0,0 +1,9 @@ +- group = local_assigns.fetch(:group) +- member = local_assigns.fetch(:member) +- can_override = local_assigns.fetch(:can_override, false) + +- if can_override + %li.divider + %li + = link_to 'Revert to LDAP group sync settings', 'javascript:void(0)', + data: { revert: 'true', endpoint: override_group_group_member_path(group, member), el_id: dom_id(member) } diff --git a/changelogs/unreleased-ee/group-sync-update-perms.yml b/changelogs/unreleased-ee/group-sync-update-perms.yml new file mode 100644 index 0000000000000000000000000000000000000000..5e6a11cb1f6cfe7462bbb21a462f1006129886ac --- /dev/null +++ b/changelogs/unreleased-ee/group-sync-update-perms.yml @@ -0,0 +1,4 @@ +--- +title: Allow master/owner to change permission levels when LDAP group sync is enabled +merge_request: 822 +author: diff --git a/config/routes/group.rb b/config/routes/group.rb index 7d0b36b22db6be5915af313476b84435e15f8095..e280d84fb382f53fa2ded4456b6d49ac9263cf4c 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -18,6 +18,10 @@ resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do post :resend_invite, on: :member delete :leave, on: :collection + + ## EE-specific + patch :override, on: :member + ## EE-specific end resource :avatar, only: [:destroy] diff --git a/spec/features/groups/members/override_ldap_memberships_spec.rb b/spec/features/groups/members/override_ldap_memberships_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2401462c11bcf0d266ccd2bf9c3ab355fa845e6b --- /dev/null +++ b/spec/features/groups/members/override_ldap_memberships_spec.rb @@ -0,0 +1,70 @@ +require 'spec_helper' + +feature 'Groups > Members > Master/Owner can override LDAP access levels', feature: true do + include WaitForAjax + + let(:johndoe) { create(:user, name: 'John Doe') } + let(:maryjane) { create(:user, name: 'Mary Jane') } + let(:owner) { create(:user) } + let(:group) { create(:group_with_ldap_group_link, :public) } + + let!(:owner_member) { create(:group_member, :owner, group: group, user: owner) } + let!(:ldap_member) { create(:group_member, :guest, group: group, user: johndoe, ldap: true) } + let!(:regular_member) { create(:group_member, :guest, group: group, user: maryjane, ldap: false) } + + background do + # We need to actually activate the LDAP config otherwise `Group#ldap_synced?` will always be false! + allow(Gitlab.config.ldap).to receive_messages(enabled: true) + + login_as(owner) + 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.' + + visit group_group_members_path(group) + + within "#group_member_#{ldap_member.id}" do + expect(page).to have_content 'LDAP' + expect(page).to have_button 'Guest', disabled: true + expect(page).to have_button 'Edit permissions' + + click_button 'Edit permissions' + end + + expect(page).to have_content ldap_override_message + + click_button 'Change permissions' + + expect(page).not_to have_content ldap_override_message + expect(page).not_to have_button 'Change permissions' + + within "#group_member_#{ldap_member.id}" do + expect(page).not_to have_button 'Edit permissions' + expect(page).to have_button 'Guest', disabled: false + + click_button 'Guest' + + within '.dropdown-menu' do + click_link 'Revert to LDAP group sync settings' + end + + wait_for_ajax + + expect(page).to have_button 'Guest', disabled: true + expect(page).to have_button 'Edit permissions' + end + + within "#group_member_#{regular_member.id}" do + expect(page).not_to have_content 'LDAP' + expect(page).not_to have_button 'Edit permissions' + expect(page).to have_button 'Guest', disabled: false + + click_button 'Guest' + + within '.dropdown-menu' do + expect(page).not_to have_content 'Revert to LDAP group sync settings' + end + end + end +end