diff --git a/lib/import/base_resource.rb b/lib/import/base_resource.rb index 1072a918e..08fc7c245 100644 --- a/lib/import/base_resource.rb +++ b/lib/import/base_resource.rb @@ -6,7 +6,10 @@ module Import def initialize(resource, *args) handle_args(resource, *args) + initialize_associations_states import(resource, *args) + return if @resource.blank? + store_associations(:after, @resource) end def import_class @@ -30,8 +33,7 @@ module Import end def attributes_changed? - return true if changed_attributes.present? - @associations_init != associations_state(@resource) + changed_attributes.present? || changed_associations.present? end def changed_attributes @@ -42,6 +44,15 @@ module Import @resource.previous_changes end + def changed_associations + changes = {} + tracked_associations.each do |association| + next if @associations[:before][association] == @associations[:after][association] + changes[association] = [@associations[:before][association], @associations[:after][association]] + end + changes + end + def created? return false if @resource.blank? # dry run @@ -52,6 +63,13 @@ module Import private + def initialize_associations_states + @associations = {} + %i(before after).each do |state| + @associations[state] ||= {} + end + end + def import(resource, *args) create_or_update(map(resource, *args), *args) rescue => e @@ -96,13 +114,13 @@ module Import return if !synced_instance instance = import_class.find_by(id: synced_instance.o_id) - store_associations_state(instance) + store_associations(:before, instance) instance end - def store_associations_state(instance) - @associations_init = associations_state(instance) + def store_associations(state, instance) + @associations[state] = associations_state(instance) end def associations_state(instance) diff --git a/lib/import/ldap/user.rb b/lib/import/ldap/user.rb index 26525b449..a71415aea 100644 --- a/lib/import/ldap/user.rb +++ b/lib/import/ldap/user.rb @@ -31,7 +31,6 @@ module Import def create_or_update(resource, *args) return if skip?(resource) - resource[:role_ids] = role_ids(resource) result = super(resource, *args) ldap_log( @@ -58,14 +57,49 @@ module Import return @signup_role_ids if !dn # check if roles are mapped for the found dn roles = @dn_roles[ dn.downcase ] - # use signup roles if no mapped roles were found - return @signup_role_ids if !roles # return found roles - 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 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 end def updated?(resource, *_args) - super + + # 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 + + user_found = super + + # in case a User was found and we had no roles + # to set/update we have to note the currently + # assigned roles so that our action check won't + # falsly detect changes to the User + if user_found && update_roles.blank? + resource[:role_ids] = @resource.role_ids + end + + user_found rescue => e ldap_log( action: "update -> #{resource[:login]}", @@ -94,7 +128,7 @@ module Import remote: resource, ) - store_associations_state(instance) + store_associations(:before, instance) end instance @@ -122,6 +156,11 @@ 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 super rescue => e ldap_log( diff --git a/spec/lib/import/ldap/user_spec.rb b/spec/lib/import/ldap/user_spec.rb index 69f46ad22..f112b4ecd 100644 --- a/spec/lib/import/ldap/user_spec.rb +++ b/spec/lib/import/ldap/user_spec.rb @@ -99,7 +99,12 @@ RSpec.describe Import::Ldap::User do context 'update' do before(:each) do - user = create(:user, login: uid) + user = create(:user, + login: uid, + role_ids: [ + Role.find_by(name: 'Agent').id, + Role.find_by(name: 'Admin').id + ]) ExternalSync.create( source: 'Ldap::User', @@ -119,6 +124,14 @@ RSpec.describe Import::Ldap::User do } 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)