From 165a594bf8606864c62f86405e318c68c2426c38 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Thu, 19 Mar 2015 01:50:53 -0400 Subject: [PATCH] 5198: Remove href attributes from remote:true links to prevent ctrl-click et al. from doing the wrong thing. --- .../app/assets/javascripts/link_to_remote.js | 23 +++++++++++++++ .../app/assets/javascripts/select_modal.js | 2 +- .../app/helpers/application_helper.rb | 28 +++++++++++++++++++ .../controllers/projects_controller_test.rb | 2 +- .../test/diagnostics/pipeline_test.rb | 2 +- .../test/helpers/share_object_helper.rb | 5 ++-- 6 files changed, 56 insertions(+), 6 deletions(-) create mode 100644 apps/workbench/app/assets/javascripts/link_to_remote.js diff --git a/apps/workbench/app/assets/javascripts/link_to_remote.js b/apps/workbench/app/assets/javascripts/link_to_remote.js new file mode 100644 index 0000000000..f4f4f3e1f4 --- /dev/null +++ b/apps/workbench/app/assets/javascripts/link_to_remote.js @@ -0,0 +1,23 @@ +$.rails.href = function(element) { + if (element.is('a')) { + // data-remote=true links must put their remote targets in + // data-remote-href="..." instead of href="...". This helps + // us avoid accidentally using the same href="..." in both the + // remote (Rails UJS) and non-remote (native browser) handlers + // -- which differ greatly in how they use that value -- and + // forgetting to test any non-remote cases like "open in new + // tab". If you really want copy-link-address/open-in-new-tab + // to work on a data-remote=true link, supply the + // copy-and-pastable URI in href in addition to the AJAX URI + // in data-remote-href. + // + // (Currently, the only places we make any remote links are + // link_to() in ApplicationHelper, which renames href="..." to + // data-remote-href="...", and select_modal, which builds a + // data-remote=true link on the client side.) + return element.data('remote-href'); + } else { + // Normal rails-ujs behavior. + return element.attr('href'); + } +} diff --git a/apps/workbench/app/assets/javascripts/select_modal.js b/apps/workbench/app/assets/javascripts/select_modal.js index 3b51faad6a..d9ad7f8c39 100644 --- a/apps/workbench/app/assets/javascripts/select_modal.js +++ b/apps/workbench/app/assets/javascripts/select_modal.js @@ -151,7 +151,7 @@ $(document).on('click', '.selectable', function() { return false; } $(''). - attr('href', $form.attr('data-search-modal')). + attr('data-remote-href', $form.attr('data-search-modal')). attr('data-remote', 'true'). attr('data-method', 'GET'). hide(). diff --git a/apps/workbench/app/helpers/application_helper.rb b/apps/workbench/app/helpers/application_helper.rb index d02d058e3a..794ce1b77e 100644 --- a/apps/workbench/app/helpers/application_helper.rb +++ b/apps/workbench/app/helpers/application_helper.rb @@ -52,6 +52,34 @@ module ApplicationHelper ArvadosBase::resource_class_for_uuid(attrvalue, opts) end + # When using {remote:true} or {method:...}, move the target URI from + # href to data-remote-href. Otherwise, browsers offer features like + # "open in new window" and "copy link address" which bypass Rails' + # click handler and therefore end up at incorrect/nonexistent routes + # (by ignoring data-method) and expect to receive pages rather than + # javascript responses. + # + # See assets/javascripts/link_to_remote.js for supporting code. + def link_to *args, &block + if (args.last and args.last.is_a? Hash and + (args.last[:remote] or + (args.last[:method] and + args.last[:method].to_s.upcase != 'GET'))) + if Rails.env.test? + # Capybara/phantomjs can't click_link without an href, even if + # the click handler means it never gets used. + raw super.gsub(' href="', ' href="#" data-remote-href="') + else + # Regular browsers work as desired: users can click A elements + # without hrefs, and click handlers fire; but there's no "copy + # link address" option in the right-click menu. + raw super.gsub(' href="', ' data-remote-href="') + end + else + super + end + end + ## # Returns HTML that links to the Arvados object specified in +attrvalue+ # Provides various output control and styling options. diff --git a/apps/workbench/test/controllers/projects_controller_test.rb b/apps/workbench/test/controllers/projects_controller_test.rb index c2089ad18d..ec17e8e422 100644 --- a/apps/workbench/test/controllers/projects_controller_test.rb +++ b/apps/workbench/test/controllers/projects_controller_test.rb @@ -28,7 +28,7 @@ class ProjectsControllerTest < ActionController::TestCase id: readonly_project_uuid }, session_for(which_user) buttons = css_select('[data-method=post]').select do |el| - el.attributes['href'].match /project.*owner_uuid.*#{readonly_project_uuid}/ + el.attributes['data-remote-href'].match /project.*owner_uuid.*#{readonly_project_uuid}/ end if should_show assert_not_empty(buttons, "did not offer to create a subproject") diff --git a/apps/workbench/test/diagnostics/pipeline_test.rb b/apps/workbench/test/diagnostics/pipeline_test.rb index 168656d4c3..f9e324ca41 100644 --- a/apps/workbench/test/diagnostics/pipeline_test.rb +++ b/apps/workbench/test/diagnostics/pipeline_test.rb @@ -39,7 +39,7 @@ class PipelineTest < DiagnosticsTest wait_for_ajax # All needed input are filled in. Run this pipeline now - click_link 'Components' + find('a,button', text: 'Components').click find('a,button', text: 'Run').click # Pipeline is running. We have a "Stop" button instead now. diff --git a/apps/workbench/test/helpers/share_object_helper.rb b/apps/workbench/test/helpers/share_object_helper.rb index 783e2bb1c9..ba09acc810 100644 --- a/apps/workbench/test/helpers/share_object_helper.rb +++ b/apps/workbench/test/helpers/share_object_helper.rb @@ -55,10 +55,9 @@ module ShareObjectHelper # poltergeist returns true for confirm(), so we don't need to accept. end end - wait_for_ajax + # Ensure revoked permission disappears from page. using_wait_time(Capybara.default_wait_time * 3) do - assert(page.has_no_text?(name), - "new share row still exists after being revoked") + assert_no_text name assert_equal(start_rows.size - 1, share_rows.size, "revoking share did not remove row from sharing table") end -- 2.30.2