5198: Remove href attributes from remote:true links to prevent ctrl-click et al....

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 (file)
index 0000000..f4f4f3e
--- /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 3b51faad6a8b67c5dde1b174c1f02410e17677b4..d9ad7f8c397e8d5726ba41ac4932935e750f046e 100644 (file)
--- 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;
         }
         $('<a />').
-            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 d02d058e3aef2601ba5398247e313d8de083feea..794ce1b77e04e545d6e67ca5c1d6b64f6b9b44ba 100644 (file)
--- 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 c2089ad18d816cd47087270b603a655ab22e2dff..ec17e8e4222676e50c7b92266b31a6180e359763 100644 (file)
--- 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 168656d4c3fd273a1fa3cd5e73824c5799e9b0b0..f9e324ca41f84377cf9b4cb6bc284e58b3abebf2 100644 (file)
--- 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 783e2bb1c9058067f94f8982708ae9c02db48e73..ba09acc810dcf0e908cb9de811e07948d4dddf32 100644 (file)
--- 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