From 65443b3768de3ea5f6da80ede345eca333fe77aa Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 10 May 2017 09:50:48 +0200 Subject: [PATCH] Working on issue #981 - Make user filter and role assignments default behaviour configurable. --- .../app/controllers/_integration/ldap.coffee | 14 ++ .../app/views/integration/ldap_wizard.jst.eco | 20 ++- lib/import/ldap/user.rb | 98 ++++++------- lib/import/ldap/user_factory.rb | 7 +- spec/lib/import/ldap/user_spec.rb | 138 ++++++++++++++---- 5 files changed, 199 insertions(+), 78 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_integration/ldap.coffee b/app/assets/javascripts/app/controllers/_integration/ldap.coffee index e10f2d03b..d861005ca 100644 --- a/app/assets/javascripts/app/controllers/_integration/ldap.coffee +++ b/app/assets/javascripts/app/controllers/_integration/ldap.coffee @@ -158,6 +158,7 @@ class ConnectionWizard extends App.WizardModal '.modal-body': 'body' '.js-userMappingForm': 'userMappingForm' '.js-groupRoleForm': 'groupRoleForm' + '.js-expertForm': 'expertForm' constructor: -> super @@ -368,6 +369,14 @@ class ConnectionWizard extends App.WizardModal @groupRoleForm.find('tbody tr.js-entry').remove() @groupRoleForm.find('tbody tr').before(@buildRowsGroupRole(@wizardConfig.group_role_map)) + @$('.js-mapping input[name="user_filter"]').val(@wizardConfig.user_filter) + + unassigned_users_choices = + sigup_roles: App.i18n.translatePlain('Assign signup roles') + skip_sync: App.i18n.translatePlain('Don\'t synchronize') + + @$('.js-unassignedUsers').html(@createSelection('unassigned_users', unassigned_users_choices, @wizardConfig.unassigned_users || 'sigup_roles')) + mappingChange: (e) => e.preventDefault() @@ -396,6 +405,11 @@ class ConnectionWizard extends App.WizardModal group_role_map_local[group_role_map.source[count]] = group_role_map.dest[count] @wizardConfig.group_role_map = group_role_map_local + expertSettings = @formParam(@expertForm) + + @wizardConfig.user_filter = expertSettings.user_filter + @wizardConfig.unassigned_users = expertSettings.unassigned_users + @tryShow() buildRowsUserMap: (user_attribute_map) => diff --git a/app/assets/javascripts/app/views/integration/ldap_wizard.jst.eco b/app/assets/javascripts/app/views/integration/ldap_wizard.jst.eco index b09550537..1918d3eed 100644 --- a/app/assets/javascripts/app/views/integration/ldap_wizard.jst.eco +++ b/app/assets/javascripts/app/views/integration/ldap_wizard.jst.eco @@ -169,7 +169,6 @@

<%- @T('Roles') %>

-

<%- @T('Note: All not mapped users will get the default signup roles.') %>

@@ -193,6 +192,25 @@
+

<%- @T('Expert') %>

+
+ + + + + + + + +
<%- @T('Name') %> + <%- @T('Value') %> +
<%- @T('User filter') %> + +
<%- @T('Users without assigned LDAP groups') %> + +
+
+
diff --git a/lib/import/ldap/user.rb b/lib/import/ldap/user.rb index 670108da2..17d767ca4 100644 --- a/lib/import/ldap/user.rb +++ b/lib/import/ldap/user.rb @@ -12,7 +12,7 @@ module Import normalized_entry = normalize_entry(resource) # extract the uid attribute and store it as - # the remote ID so we can access later + # the remote ID so we can access it later # when working with ExternalSync @remote_id = normalized_entry[ @ldap_config[:user_uid].to_sym ] @@ -31,13 +31,19 @@ module Import def create_or_update(resource, *args) return if skip?(resource) - result = super(resource, *args) - ldap_log( - action: "#{action} -> #{@resource.login}", - status: 'success', - request: resource, - ) + result = nil + catch(:no_roles_assigned) do + determine_role_ids(resource) + + result = super(resource, *args) + + ldap_log( + action: "#{action} -> #{@resource.login}", + status: 'success', + request: resource, + ) + end result end @@ -50,56 +56,55 @@ module Import !resource.except(*ignored_attributes).values.any?(&:present?) end - def role_ids(resource) - # remove remporary added and get value + def determine_role_ids(resource) + # remove temporary added and get value dn = resource.delete(:dn) - # use signup roles if no dn is present - return @signup_role_ids if !dn - # check if roles are mapped for the found dn - roles = @dn_roles[ dn.downcase ] - # return found roles - return roles if roles - # return signup roles if there is a role mapping in general - # this makes the LDAP the leading source of roles - return @signup_role_ids if @dn_roles.present? + raise "Missing 'dn' attribute for remote id '#{@remote_id}'" if dn.blank? - # special case: there is no LDAP role mapping configured - # - # if there is no role mapping in general the signup roles - # should only get used for newly created users (see .create method) - # otherwise users might overwrite their local assigned - # roles with less privileged roles and lock theirselfs out - # - # see issue 350#issuecomment-295252402 - # - # this method only gets called from the .updated? method - # so we won't return any roles which will keep the current - # role assignment for the User - # the signup roles will be taken in this case direcly in - # the .create method - nil + if @dn_roles.present? + # check if roles are mapped for the found dn + roles = @dn_roles[ dn.downcase ] + + if roles.present? + # LDAP is the leading source if + # a mapping entry is present + @update_role_ids = roles + @create_role_ids = roles + elsif @ldap_config[:unassigned_users] == 'skip_sync' + throw :no_roles_assigned + else + use_signup_roles + end + else + use_signup_roles + end + end + + def use_signup_roles + @update_role_ids = nil # use existing + @create_role_ids = @signup_role_ids end def updated?(resource, *_args) - # this is needed for the special case described in the role_ids method - # - # assign roles only if there are any found in the mapping - # otherwise keep those stored to the User - update_roles = role_ids(resource) - resource[:role_ids] = update_roles if update_roles + resource[:role_ids] = @update_role_ids if @update_role_ids user_found = false import_class.without_callback(:update, :after, :avatar_for_email_check) do user_found = super end - # in case a User was found and we had no roles - # to set/update we have to note the currently + # in case an User was found and we had no roles + # to set/update we have to note the currently locally # assigned roles so that our action check won't - # falsly detect changes to the User - if user_found && update_roles.blank? + # falsly detect changes to the User (from nil to current) + if user_found && @update_role_ids.blank? resource[:role_ids] = @resource.role_ids + + # we have to re-store/overwrite the stored + # associations since we've added the current + # state of the roles just yet + store_associations(:after, resource) end user_found @@ -159,11 +164,7 @@ module Import end def create(resource, *_args) - # this is needed for the special case described in the role_ids method - # - # in case we have no role IDs yet we have to fall back to the signup roles - resource[:role_ids] ||= @signup_role_ids - + resource[:role_ids] = @create_role_ids import_class.without_callback(:create, :after, :avatar_for_email_check) do super end @@ -231,7 +232,6 @@ module Import updated_by_id: 1, ) end - end end end diff --git a/lib/import/ldap/user_factory.rb b/lib/import/ldap/user_factory.rb index 2cdc8cf1c..7b318bf8e 100644 --- a/lib/import/ldap/user_factory.rb +++ b/lib/import/ldap/user_factory.rb @@ -16,8 +16,11 @@ module Import @config = config @ldap = ldap - user_roles = user_roles(ldap: @ldap, config: config) - signup_role_ids = Role.signup_role_ids.sort + user_roles = user_roles(ldap: @ldap, config: config) + + if config[:unassigned_users].blank? || config[:unassigned_users] == 'sigup_roles' + signup_role_ids = Role.signup_role_ids.sort + end @dry_run = kargs[:dry_run] pre_import_hook([], config, user_roles, signup_role_ids, kargs) diff --git a/spec/lib/import/ldap/user_spec.rb b/spec/lib/import/ldap/user_spec.rb index 2aa073275..3048314c4 100644 --- a/spec/lib/import/ldap/user_spec.rb +++ b/spec/lib/import/ldap/user_spec.rb @@ -28,7 +28,9 @@ RSpec.describe Import::Ldap::User do let(:user_roles) do { - user_entry.dn => [1] + user_entry.dn => [ + Role.find_by(name: 'Admin').id + ] } end @@ -65,23 +67,6 @@ RSpec.describe Import::Ldap::User do expect(HttpLog.last.status).to eq('success') end - it 'uses mapped roles from group role' do - described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) - expect(User.last.role_ids).not_to eq(signup_role_ids) - end - - it 'uses Signup roles if no group role mapping was found' do - - # update old - user_roles[ user_entry.dn ] = [1, 2] - - # change dn so no mapping will match - user_entry['dn'] = ['some_unmapped_dn'] - - described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) - expect(User.last.role_ids).to eq(signup_role_ids) - end - it 'skips User entries without attributes' do skip_entry = build(:ldap_entry) @@ -101,6 +86,58 @@ RSpec.describe Import::Ldap::User do expect(HttpLog.last.status).to eq('failed') end + + context 'role assignment' do + + it 'uses mapped roles from group role' do + described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + expect(User.last.role_ids).not_to eq(signup_role_ids) + end + + context 'no mapping entry' do + + before(:each) do + # create mapping that won't match + # since dn will change below + # this is needed since if 'user_roles' + # gets called later it will get initialized + # with the changed dn + user_roles[ user_entry.dn ] = [ + Role.find_by(name: 'Agent').id, + Role.find_by(name: 'Admin').id + ] + + # change dn so no mapping will match + user_entry['dn'] = ['some_unmapped_dn'] + end + + it 'uses signup roles by default' do + described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + expect(User.last.role_ids).to eq(signup_role_ids) + end + + it 'uses signup roles if configured' do + + ldap_config[:unassigned_users] = 'sigup_roles' + + described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + expect(User.last.role_ids).to eq(signup_role_ids) + end + + it 'skips user if configured' do + + ldap_config[:unassigned_users] = 'skip_sync' + + instance = nil + expect do + instance = described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + end.not_to change { + User.count + } + expect(instance.action).to eq(:skipped) + end + end + end end context 'update' do @@ -138,14 +175,6 @@ RSpec.describe Import::Ldap::User do described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) end - it "doesn't change roles if no role mapping is configured" do - expect do - described_class.new(user_entry, ldap_config, {}, signup_role_ids) - end.to not_change { - User.last.role_ids - } - end - it 'creates an HTTP Log entry' do expect do described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) @@ -178,5 +207,62 @@ RSpec.describe Import::Ldap::User do expect(HttpLog.last.status).to eq('failed') end + + context 'no mapping entry' do + + before(:each) do + # create mapping that won't match + # since dn will change below + # this is needed since if 'user_roles' + # gets called later it will get initialized + # with the changed dn + user_roles[ user_entry.dn ] = [ + Role.find_by(name: 'Agent').id, + Role.find_by(name: 'Admin').id + ] + + # change dn so no mapping will match + user_entry['dn'] = ['some_unmapped_dn'] + end + + it 'keeps local roles by default' do + expect do + described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + end.not_to change { + User.last.role_ids + } + end + + it 'keeps local roles if signup roles are configured' do + + ldap_config[:unassigned_users] = 'sigup_roles' + expect do + described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + end.not_to change { + User.last.role_ids + } + end + + it "doesn't detect false changes if signup roles are configured" do + # make sure that the nothing has changed + User.find_by(login: uid).update_attribute(:email, 'example@example.com') + + instance = described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + expect(instance.action).to eq(:unchanged) + end + + it 'skips user if configured' do + + ldap_config[:unassigned_users] = 'skip_sync' + + instance = nil + expect do + instance = described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + end.not_to change { + User.count + } + expect(instance.action).to eq(:skipped) + end + end end end