From 58a8c047bb44bdd74e938f5758018ff8453d0de5 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Wed, 10 Oct 2018 08:23:45 +0200 Subject: [PATCH] Prevent LDAP sync from needlessly deactivating inactive users (fixes #2256) --- .../common/model/associations/assign.rb | 2 +- .../user/attributes/role_ids/unassigned.rb | 6 +- .../common/model/associations/assign_spec.rb | 14 +++ .../attributes/role_ids/unassigned_spec.rb | 98 +++++++++++++++++++ 4 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 spec/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned_spec.rb diff --git a/lib/sequencer/unit/import/common/model/associations/assign.rb b/lib/sequencer/unit/import/common/model/associations/assign.rb index b55fdc833..633900cba 100644 --- a/lib/sequencer/unit/import/common/model/associations/assign.rb +++ b/lib/sequencer/unit/import/common/model/associations/assign.rb @@ -27,7 +27,7 @@ class Sequencer # always returns true def log_associations_error - return true if %i[failed deactivated].include?(action) + return true if %i[skipped failed deactivated].include?(action) logger.error { 'associations cannot be nil' } if associations.nil? true diff --git a/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned.rb b/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned.rb index 4161fa31f..eae44201a 100644 --- a/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned.rb +++ b/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned.rb @@ -10,7 +10,7 @@ class Sequencer skip_any_action - uses :resource, :dn_roles, :ldap_config, :mapped + uses :dn_roles, :ldap_config, :mapped, :instance provides :action def process @@ -25,9 +25,7 @@ class Sequencer # if unassigned users should not get skipped return if ldap_config[:unassigned_users] != 'skip_sync' - instance = state.optional(:instance) - - if instance.present? + if instance&.active # deactivate instance if role assignment is lost instance.update!(active: false) state.provide(:action, :deactivated) diff --git a/spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb b/spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb index 02d8e13b8..cb0cfa5db 100644 --- a/spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb +++ b/spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb @@ -56,6 +56,20 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Associations::Assign, seq let(:instance) { create(:user) } let(:associations) { nil } + context 'and `action == :skipped`' do + let(:action) { :skipped } + + it 'makes no changes' do + allow(Rails.logger).to receive(:error).and_call_original + + provided = process(parameters) + + expect(Rails.logger).not_to have_received(:error) + expect(provided).to include(action: action) + expect(instance.changed?).to be(false) + end + end + context 'and `action == :failed`' do let(:action) { :failed } diff --git a/spec/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned_spec.rb b/spec/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned_spec.rb new file mode 100644 index 000000000..5eeb108ef --- /dev/null +++ b/spec/lib/sequencer/unit/import/ldap/user/attributes/role_ids/unassigned_spec.rb @@ -0,0 +1,98 @@ +require 'rails_helper' + +RSpec.describe Sequencer::Unit::Import::Ldap::User::Attributes::RoleIds::Unassigned, sequencer: :unit do + let(:subject) { process(parameters) } + + let(:parameters) do + { resource: resource, + dn_roles: dn_roles, + ldap_config: ldap_config, + mapped: mapped, + instance: instance } + end + + let(:resource) do + { dn: 'uid=jane,ou=People,dc=example,dc=org', + uid: 'jane_doe', + sn: 'Doe', + givenname: 'Jane' }.with_indifferent_access + end + + # this sequencer unit only runs when this parameter is PRESENT + let(:dn_roles) do + { 'uid=john,ou=people,dc=example,dc=org' => [1] }.with_indifferent_access + end + + # this sequencer unit only runs when this parameter's "unassigned_users" key is "skip_sync" + let(:ldap_config) do + { unassigned_users: 'skip_sync' }.with_indifferent_access + end + + # this sequencer unit only runs when this parameter's "role_ids" key is BLANK + let(:mapped) do + { firstname: 'Jane', + lastname: 'Doe', + login: 'jane_doe' }.with_indifferent_access + end + + let(:instance) { create(:agent_user, mapped) } + + context 'when user exists from previous import' do + context 'and is active' do + before { instance.update(active: true) } + + it 'deactivates user (with action: :deactivated)' do + expect(subject).to include(action: :deactivated) + expect(instance.reload.active).to be(false) + end + end + + context 'and is inactive' do + before { instance.update(active: false) } + + it 'skips user (with action: :skipped)' do + expect(subject).to include(action: :skipped) + expect(instance.reload.active).to be(false) + end + end + end + + context 'when user has not been imported yet' do + let(:instance) { nil } + + it 'skips user (with action: :skipped)' do + expect(subject).to include(action: :skipped) + end + end + + describe 'parameters' do + before { allow(instance).to receive(:update).and_call_original } + + context 'without :dn_roles (hash map of LDAP DNs ↔ Zammad role IDs for ALL USERS)' do + before { parameters[:dn_roles].clear } + + it 'skips user (with NO action)' do + expect(subject).to include(action: nil) + expect(instance).not_to have_received(:update) + end + end + + context 'with :mapped[:role_ids] (hash map of LDAP attributes ↔ Zammad attributes for GIVEN USER)' do + before { parameters[:mapped].merge!(role_ids: [2]) } + + it 'skips user (with NO action)' do + expect(subject).to include(action: nil) + expect(instance).not_to have_received(:update) + end + end + + context 'when :ldap_config[:unassigned_users] != "skip_sync"' do + before { parameters[:ldap_config].merge!(unassigned_users: 'sigup_roles') } + + it 'skips user (with NO action)' do + expect(subject).to include(action: nil) + expect(instance).not_to have_received(:update) + end + end + end +end