From a7e5a22d5cc1b8cfea2b3fd628846bd09c4b8bd0 Mon Sep 17 00:00:00 2001 From: Gilbert Cherrie Date: Thu, 16 Oct 2025 09:42:17 -0400 Subject: [PATCH 1/2] Delete ldap forest tech debt --- app/controllers/ops_controller/settings.rb | 111 ------------------- app/views/ops/_ldap_forest_entries.html.haml | 90 --------------- config/routes.rb | 4 - 3 files changed, 205 deletions(-) delete mode 100644 app/views/ops/_ldap_forest_entries.html.haml diff --git a/app/controllers/ops_controller/settings.rb b/app/controllers/ops_controller/settings.rb index 25f7b6de09b..89b3bc3576e 100644 --- a/app/controllers/ops_controller/settings.rb +++ b/app/controllers/ops_controller/settings.rb @@ -35,117 +35,6 @@ def apply_imports redirect_to(:action => 'explorer', :no_refresh => true) end - def forest_get_form_vars - @edit = session[:edit] - @ldap_info = {} - @ldap_info[:mode] = params[:user_proxies_mode] if params[:user_proxies] && params[:user_proxies_mode] - @ldap_info[:ldaphost] = params[:user_proxies][:ldaphost] if params[:user_proxies] && params[:user_proxies][:ldaphost] - @ldap_info[:ldapport] = params[:user_proxies][:ldapport] if params[:user_proxies] && params[:user_proxies][:ldapport] - @ldap_info[:basedn] = params[:user_proxies][:basedn] if params[:user_proxies] && params[:user_proxies][:basedn] - @ldap_info[:bind_dn] = params[:user_proxies][:bind_dn] if params[:user_proxies] && params[:user_proxies][:bind_dn] - @ldap_info[:bind_pwd] = params[:user_proxies][:bind_pwd] if params[:user_proxies] && params[:user_proxies][:bind_pwd] - nil - end - - def forest_form_field_changed - assert_privileges("ops_settings") - - @edit = session[:edit] # Need to reload @edit so it stays in the session - port = params[:user_proxies_mode] == "ldap" ? "389" : "636" - render :update do |page| - page << javascript_prologue - page << "$('#user_proxies_ldapport').val('#{port}');" - end - end - - # AJAX driven routine to select a classification entry - def forest_select - assert_privileges("ops_settings") - - forest_get_form_vars - if params[:ldaphost_id] == "new" - render :update do |page| - page << javascript_prologue - page.replace("flash_msg_div", :partial => "layouts/flash_msg") - page << "miqScrollTop();" if @flash_array.present? - page.replace("forest_entries_div", :partial => "ldap_forest_entries", :locals => {:entry => "new", :edit => true}) - end - session[:entry] = "new" - else - entry = nil - @edit[:new][:authentication][:user_proxies].each do |f| - entry = f if f[:ldaphost] == params[:ldaphost_id] - end - render :update do |page| - page << javascript_prologue - page.replace("flash_msg_div", :partial => "layouts/flash_msg") - page << "miqScrollTop();" if @flash_array.present? - page.replace("forest_entries_div", :partial => "ldap_forest_entries", :locals => {:entry => entry, :edit => true}) - end - session[:entry] = entry - end - end - - # AJAX driven routine to delete a classification entry - def forest_delete - assert_privileges("ops_settings") - - forest_get_form_vars - idx = nil - @edit[:new][:authentication][:user_proxies].each_with_index do |f, i| - idx = i if f[:ldaphost] == params[:ldaphost_id] - end - @edit[:new][:authentication][:user_proxies].delete_at(idx) unless idx.nil? - @changed = (@edit[:new] != @edit[:current]) - render :update do |page| - page << javascript_prologue - page.replace("flash_msg_div", :partial => "layouts/flash_msg") - page << "miqScrollTop();" if @flash_array.present? - page << javascript_for_miq_button_visibility(@changed) - page.replace("forest_entries_div", :partial => "ldap_forest_entries", :locals => {:entry => nil, :edit => false}) - end - end - - # AJAX driven routine to add/update a classification entry - def forest_accept - assert_privileges("ops_settings") - - forest_get_form_vars - no_changes = true - if @ldap_info[:ldaphost] == "" - add_flash(_("LDAP Host is required"), :error) - no_changes = false - elsif @edit[:new][:authentication][:user_proxies].blank? || @edit[:new][:authentication][:user_proxies][0].blank? # if adding forest first time, delete a blank record - @edit[:new][:authentication][:user_proxies].delete_at(0) - else - @edit[:new][:authentication][:user_proxies].each do |f| - # check to make sure ldaphost already doesn't exist and ignore if existing record is being edited. - next unless f[:ldaphost] == @ldap_info[:ldaphost] && session[:entry] == 'new' - - no_changes = false - add_flash(_("LDAP Host should be unique"), :error) - break - end - end - if no_changes - if session[:entry] == "new" - @edit[:new][:authentication][:user_proxies].push(@ldap_info) - else - @edit[:new][:authentication][:user_proxies].each_with_index do |f, i| - @edit[:new][:authentication][:user_proxies][i] = @ldap_info if f[:ldaphost] == session[:entry][:ldaphost] - end - end - end - @changed = (@edit[:new] != @edit[:current]) - render :update do |page| - page << javascript_prologue - page << javascript_for_miq_button_visibility(@changed) - page.replace("flash_msg_div", :partial => "layouts/flash_msg") - page << "miqScrollTop();" if @flash_array.present? - page.replace("forest_entries_div", :partial => "ldap_forest_entries", :locals => {:entry => nil, :edit => false}) if no_changes - end - end - def region_edit assert_privileges("region_edit") diff --git a/app/views/ops/_ldap_forest_entries.html.haml b/app/views/ops/_ldap_forest_entries.html.haml deleted file mode 100644 index 8ad8689929a..00000000000 --- a/app/views/ops/_ldap_forest_entries.html.haml +++ /dev/null @@ -1,90 +0,0 @@ -- url = url_for_only_path(:action => 'forest_form_field_changed', :id => @sb[:active_tab].split('_').last) -#forest_entries_div - %h3= _("Trusted Forest Settings") - = form_tag({:action => 'forest_accept', - :id => "ldap_form"}, - :remote => true, - :method => :post) do - %table.table.table-striped.table-bordered.table-hover - %thead - %tr - %th.table-view-pf-select - %th= _("LDAP Hostname") - %th= _("Mode") - %th= _("LDAP Port") - %th= _("Base DN") - %th= _("Bind DN") - %th= _("Bind Password") - %tbody - - if entry != "new" - %tr - %td - %button.btn.btn-default{:type => 'button', :onclick => remote_function(:url => {:action => 'forest_select', :ldaphost_id => "new"}), :title => _("Click to add a new forest")} - %i.fa.fa-plus - %td - = _("") - %td - %td - %td - %td - %td - - else - %tr - %td - %button.btn.btn-default#accept{:type => 'submit', :name => 'accept', :title => _("Add this entry")} - %i.pficon.pficon-import - %td - = text_field("user_proxies", "ldaphost", "maxlength" => 50) - %td - = select_tag('user_proxies_mode', - options_for_select([[_("ldap"), "ldap"], [_("ldaps"), "ldaps"]], "ldap"), - "data-miq_observe" => {:url => url}.to_json) - %td - = text_field("user_proxies", "ldapport", "size" => 20, "maxlength" => 50, "value" => "389") - %td - = text_field("user_proxies", "basedn", "size" => 20, "maxlength" => 50) - %td - = text_field("user_proxies", "bind_dn", "size" => 20, "maxlength" => 50) - %td - = password_field("user_proxies", "bind_pwd", "size" => 20, "maxlength" => 128) - - @edit[:new][:authentication][:user_proxies].sort_by { |a| a[:ldaphost].to_s }.each do |forest| - - if entry != nil && entry != "new" && entry[:ldaphost] == forest[:ldaphost] - %tr - %td - %button.btn.btn-default#accept{:type => :submit, :name => 'accept', :title => _("Update this entry")} - %i.pficon.pficon-import - %td - = text_field("user_proxies", "ldaphost", "maxlength" => 50, "value" => entry[:ldaphost]) - %td - = select_tag('user_proxies_mode', - options_for_select([[_("ldap"), "ldap"], [_("ldaps"), "ldaps"]], entry[:mode]), - "data-miq_observe" => {:url => url}.to_json) - %td - = text_field("user_proxies", "ldapport", "size" => 20, "maxlength" => 6, "value" => entry[:ldapport]) - %td - = text_field("user_proxies", "basedn", "size" => 20, "maxlength" => 50, "value" => entry[:basedn]) - %td - = text_field("user_proxies", "bind_dn", "size" => 20, "maxlength" => 50, "value" => entry[:bind_dn]) - %td - = password_field("user_proxies", "bind_pwd", "size" => 20, "maxlength" => 128, "value" => entry[:bind_pwd]) - - else - - unless forest.blank? - %tr - %td - %button.btn.btn-default{:type => 'button', :onclick => remote_function(:url => {:action => 'forest_delete', - :ldaphost_id => forest[:ldaphost].to_s}, :confirm => _("Are you sure you want to delete forest %{ldaphost} ?") % {:ldaphost => forest[:ldaphost]}), - :title => _("Click to delete this forest")} - %i.pficon.pficon-delete - %td{:onclick => remote_function(:url => {:action => 'forest_select', :ldaphost_id => forest[:ldaphost].to_s}), :title => _("Click to edit this forest")} - = h(forest[:ldaphost]) - %td{:onclick => remote_function(:url => {:action => 'forest_select', :ldaphost_id => forest[:ldaphost].to_s}), :title => _("Click to edit this forest")} - = h(forest[:mode]) - %td{:onclick => remote_function(:url => {:action => 'forest_select', :ldaphost_id => forest[:ldaphost].to_s}), :title => _("Click to edit this forest")} - = h(forest[:ldapport]) - %td{:onclick => remote_function(:url => {:action => 'forest_select', :ldaphost_id => forest[:ldaphost].to_s}), :title => _("Click to edit this forest")} - = h(forest[:basedn]) - %td{:onclick => remote_function(:url => {:action => 'forest_select', :ldaphost_id => forest[:ldaphost].to_s}), :title => _("Click to edit this forest")} - = h(forest[:bind_dn]) - %td{:onclick => remote_function(:url => {:action => 'forest_select', :ldaphost_id => forest[:ldaphost].to_s}), :title => _("Click to edit this forest")} - - if forest[:bind_pwd].to_s.length > 0 - ****** diff --git a/config/routes.rb b/config/routes.rb index ce1fd14bf9a..a503440991d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -2446,10 +2446,6 @@ diagnostics_tree_select explorer fetch_target_ids - forest_accept - forest_delete - forest_form_field_changed - forest_select help_menu_form_field_changed label_tag_mapping_delete label_tag_mapping_edit From 9d10a7c0eb04a11c25adb084ea2adc617c430526 Mon Sep 17 00:00:00 2001 From: Gilbert Cherrie Date: Thu, 16 Oct 2025 10:34:07 -0400 Subject: [PATCH 2/2] Fix specs --- .../ops_controller/settings/common_spec.rb | 15 ----- spec/controllers/ops_settings_spec.rb | 59 ------------------- spec/routing/ops_routing_spec.rb | 4 -- 3 files changed, 78 deletions(-) diff --git a/spec/controllers/ops_controller/settings/common_spec.rb b/spec/controllers/ops_controller/settings/common_spec.rb index dc886144873..73c47f8eff9 100644 --- a/spec/controllers/ops_controller/settings/common_spec.rb +++ b/spec/controllers/ops_controller/settings/common_spec.rb @@ -152,21 +152,6 @@ :active_tab => 'settings_authentication') controller.x_node = "svr-#{miq_server.id}" end - - it "sets ldap_role to false to make forest entries div hidden" do - controller.params = {:id => 'authentication', - :authentication_mode => 'database'} - controller.send(:settings_get_form_vars) - expect(assigns(:edit)[:new][:authentication][:ldap_role]).to eq(false) - end - - it "resets ldap_role to it's original state so forest entries div can be displayed" do - session[:edit][:new][:authentication][:mode] = 'database' - controller.params = {:id => 'authentication', - :authentication_mode => 'ldap'} - controller.send(:settings_get_form_vars) - expect(assigns(:edit)[:new][:authentication][:ldap_role]).to eq(true) - end end describe "#pglogical_save_subscriptions" do diff --git a/spec/controllers/ops_settings_spec.rb b/spec/controllers/ops_settings_spec.rb index 4b71618da2b..cdcd30d7d12 100644 --- a/spec/controllers/ops_settings_spec.rb +++ b/spec/controllers/ops_settings_spec.rb @@ -136,65 +136,6 @@ allow(controller).to receive(:data_for_breadcrumbs).and_return({}) end end - - context '#forest_accept' do - let(:user) { FactoryBot.create(:user, :features => %w(ops_settings)) } - before do - login_as user - end - - context 'adding an LDAP Trusted Forest' do - before do - EvmSpecHelper.create_guid_miq_server_zone - @user_proxies = {:ldaphost => 'ldap.manageiq1.org', - :ldapport => '389', - :mode => 'ldap', - :basedn => 'cn=groups,cn=accounts,dc=miq', - :bind_dn => 'uid=admin,cn=users,cn=accounts,dc=miq,dc=e', - :bind_pwd => '******'} - @vmdb = ::Settings.to_hash - expect(controller).to receive(:render) - end - - after(:each) { expect(response.status).to eq(200) } - - it 'is a new record' do - session[:edit] = {:current => @vmdb, :new => {:authentication => {:user_proxies => []}}} - session[:entry] = 'new' - controller.send(:forest_accept) - end - - it 'is an existing record' do - controller.params = {:user_proxies_mode => '', :user_proxies => @user_proxies} - session[:edit] = {:current => @vmdb, :new => {:authentication => {:user_proxies => [@user_proxies]}}} - session[:entry] = @user_proxies - controller.send(:forest_accept) - end - - it 'LDAP Host exists' do - @user_proxies[:ldaphost] = '' - controller.params = {:user_proxies_mode => '', :user_proxies => @user_proxies} - session[:edit] = {:current => @vmdb, :new => {:authentication => {:user_proxies => [@user_proxies]}}} - session[:entry] = @user_proxies - - controller.send(:forest_accept) - - flash_messages = controller.instance_variable_get(:@flash_array) - expect(flash_messages.first).to eq(:message => 'LDAP Host is required', :level => :error) - end - - it 'LDAP Host is unique' do - controller.params = {:user_proxies_mode => '', :user_proxies => @user_proxies} - session[:edit] = {:current => @vmdb, :new => {:authentication => {:user_proxies => [@user_proxies, @user_proxies]}}} - session[:entry] = 'new' - - controller.send(:forest_accept) - - flash_messages = controller.instance_variable_get(:@flash_array) - expect(flash_messages.first).to eq(:message => 'LDAP Host should be unique', :level => :error) - end - end - end end context "replace_right_cell" do diff --git a/spec/routing/ops_routing_spec.rb b/spec/routing/ops_routing_spec.rb index dca02dba90f..ed943c93851 100644 --- a/spec/routing/ops_routing_spec.rb +++ b/spec/routing/ops_routing_spec.rb @@ -42,10 +42,6 @@ diagnostics_server_list diagnostics_tree_select explorer - forest_accept - forest_delete - forest_form_field_changed - forest_select label_tag_mapping_delete label_tag_mapping_edit label_tag_mapping_update