Working on issue #981 - Make user filter and role assignments default behaviour configurable.

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) =>

app/assets/javascripts/app/views/integration/ldap_wizard.jst.eco

@@ -169,7 +169,6 @@
         </form>
 
         <h2><%- @T('Roles') %></h2>
-        <p><%- @T('Note: All not mapped users will get the default signup roles.') %></p>
         <form class="js-groupRoleForm">
           <table class="settings-list js-groupRoleMap" style="width: 100%;">
             <colgroup>
@@ -193,6 +192,25 @@
           </table>
         </form>
 
+        <h2><%- @T('Expert') %></h2>
+        <form class="js-expertForm">
+          <table class="settings-list js-expert" style="width: 100%;">
+            <thead>
+              <tr>
+                <th width="30%"><%- @T('Name') %>
+                <th width="70%"><%- @T('Value') %>
+            </thead>
+            <tbody>
+              <tr>
+                <td class="settings-list-row-control"><%- @T('User filter') %>
+                <td class="settings-list-control-cell"><input type="text" name="user_filter" class="form-control form-control--small" value="" placeholder="" autocomplete="new-password">
+              <tr>
+                <td class="settings-list-row-control"><%- @T('Users without assigned LDAP groups') %>
+                <td class="settings-list-control-cell js-unassignedUsers">
+            </tbody>
+          </table>
+        </form>
+
       </div>
     </div>
     <div class="modal-footer">

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,6 +31,11 @@ module Import
 
       def create_or_update(resource, *args)
         return if skip?(resource)
+
+        result = nil
+        catch(:no_roles_assigned) do
+          determine_role_ids(resource)
+
           result = super(resource, *args)
 
           ldap_log(
@@ -38,6 +43,7 @@ module Import
             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)
+      def determine_role_ids(resource)
-        # remove remporary added and get value
+        # remove temporary added and get value
         dn = resource.delete(:dn)
-        # use signup roles if no dn is present
+        raise "Missing 'dn' attribute for remote id '#{@remote_id}'" if dn.blank?
-        return @signup_role_ids if !dn
+
+        if @dn_roles.present?
           # 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?
 
-        # special case: there is no LDAP role mapping configured
+          if roles.present?
-        #
+            # LDAP is the leading source if
-        # if there is no role mapping in general the signup roles
+            # a mapping entry is present
-        # should only get used for newly created users (see .create method)
+            @update_role_ids = roles
-        # otherwise users might overwrite their local assigned
+            @create_role_ids = roles
-        # roles with less privileged roles and lock theirselfs out
+          elsif @ldap_config[:unassigned_users] == 'skip_sync'
-        #
+            throw :no_roles_assigned
-        # see issue 350#issuecomment-295252402
+          else
-        #
+            use_signup_roles
-        # this method only gets called from the .updated? method
+          end
-        # so we won't return any roles which will keep the current
+        else
-        # role assignment for the User
+          use_signup_roles
-        # the signup roles will be taken in this case direcly in
+        end
-        # the .create method
+      end
-        nil
+
+      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
+        resource[:role_ids] = @update_role_ids if @update_role_ids
-        #
-        # 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
 
         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
+        # in case an User was found and we had no roles
-        # to set/update we have to note the currently
+        # 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
+        # falsly detect changes to the User (from nil to current)
-        if user_found && update_roles.blank?
+        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
+        resource[:role_ids] = @create_role_ids
-        #
-        # in case we have no role IDs yet we have to fall back to the signup roles
-        resource[:role_ids] ||= @signup_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

lib/import/ldap/user_factory.rb

@@ -17,7 +17,10 @@ module Import
         @ldap   = ldap
 
         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)

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