[ARVADOS] created: cb0944391d311c7e0c0180941c5fa60a60395e1e

git at public.curoverse.com git at public.curoverse.com
Thu May 15 16:38:10 EDT 2014


        at  cb0944391d311c7e0c0180941c5fa60a60395e1e (commit)


commit cb0944391d311c7e0c0180941c5fa60a60395e1e
Author: Brett Smith <brett at curoverse.com>
Date:   Thu May 15 16:38:12 2014 -0400

    2764: Add wget-friendly Collections file page.
    
    This new route will become the way you share authless Collection links
    with others.  They can pass it to `wget -r` to download the whole
    collection, nicely organized, with nothing extraneous.  Since it
    doesn't try to load user information or look up related Arvados items,
    it can be rendered using an API token with a very narrow scope.
    
    Because wget respects robots.txt, this branch stops using that in
    favor of the corresponding <meta> tag.  The new view only limits
    indexing, so wget can follow the links on the page.
    
    Refs #2764.

diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index cd45b48..6231e17 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -91,8 +91,7 @@ class CollectionsController < ApplicationController
   def show_file_links
     Thread.current[:reader_tokens] = [params[:reader_token]]
     find_object_by_uuid
-    show
-    render 'show'
+    render layout: false
   end
 
   def show_file
diff --git a/apps/workbench/app/views/collections/show_file_links.html.erb b/apps/workbench/app/views/collections/show_file_links.html.erb
new file mode 100644
index 0000000..9d61a6a
--- /dev/null
+++ b/apps/workbench/app/views/collections/show_file_links.html.erb
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>
+    <% if content_for? :page_title %>
+    <%= yield :page_title %> / <%= Rails.configuration.site_name %>
+    <% else %>
+    <%= Rails.configuration.site_name %>
+    <% end %>
+  </title>
+  <meta name="description" content="">
+  <meta name="author" content="">
+  <meta name="robots" content="NOINDEX">
+</head>
+<body>
+<% content_for :page_title do %>
+  <%= (@object.respond_to?(:properties) ? @object.properties[:page_title] : nil) ||
+        @object.friendly_link_name %>
+<% end %>
+
+<% if @object.andand.files.andand.any? %>
+  <% link_opts = {controller: 'collections', action: 'show_file',
+                  uuid: @object.uuid, reader_token: params[:reader_token]} %>
+  <ul>
+  <% @object.files.map { |spec|
+       CollectionsHelper::file_path(spec)
+     }.each do |path| %>
+    <li><%= link_to(path, link_opts.merge(file: path)) %></li>
+  <% end %>
+  </ul>
+<% else %>
+  <p>No files in this collection.</p>
+<% end %>
+</body>
+</html>
diff --git a/apps/workbench/app/views/layouts/application.html.erb b/apps/workbench/app/views/layouts/application.html.erb
index 2d3c4c0..17783bb 100644
--- a/apps/workbench/app/views/layouts/application.html.erb
+++ b/apps/workbench/app/views/layouts/application.html.erb
@@ -14,6 +14,7 @@
   <link rel="shortcut icon" href="/favicon.ico" type="image/x-icon">
   <meta name="description" content="">
   <meta name="author" content="">
+  <meta name="robots" content="NOINDEX, NOFOLLOW">
   <%= stylesheet_link_tag    "application", :media => "all" %>
   <%= javascript_include_tag "application" %>
   <%= csrf_meta_tags %>
diff --git a/apps/workbench/public/robots.txt b/apps/workbench/public/robots.txt
index c6742d8..e69de29 100644
--- a/apps/workbench/public/robots.txt
+++ b/apps/workbench/public/robots.txt
@@ -1,2 +0,0 @@
-User-Agent: *
-Disallow: /
diff --git a/apps/workbench/test/integration/collections_test.rb b/apps/workbench/test/integration/collections_test.rb
index 4a33693..911daa0 100644
--- a/apps/workbench/test/integration/collections_test.rb
+++ b/apps/workbench/test/integration/collections_test.rb
@@ -3,7 +3,6 @@ require 'selenium-webdriver'
 require 'headless'
 
 class CollectionsTest < ActionDispatch::IntegrationTest
-
   def change_persist oldstate, newstate
     find "div[data-persistent-state='#{oldstate}']"
     page.assert_no_selector "div[data-persistent-state='#{newstate}']"
@@ -48,4 +47,30 @@ class CollectionsTest < ActionDispatch::IntegrationTest
     # isn't only showing up in an error message.
     assert(page.has_link?('foo'), "Collection page did not include file link")
   end
+
+  test "can download an entire collection with a reader token" do
+    uuid = api_fixture('collections')['foo_file']['uuid']
+    token = api_fixture('api_client_authorizations')['active_all_collections']['api_token']
+    url_head = "/collections/download/#{uuid}/#{token}/"
+    visit url_head
+    # It seems that Capybara can't inspect tags outside the body, so this is
+    # a very blunt approach.
+    assert_no_match(/<\s*meta[^>]+\bnofollow\b/i, page.html,
+                    "wget prohibited from recursing the collection page")
+    # TODO: When we can test against a Keep server, actually follow links
+    # and check their contents, rather than testing the href directly
+    # (this is too closely tied to implementation details).
+    hrefs = page.all('a').map do |anchor|
+      link = anchor[:href] || ''
+      if link.start_with? url_head
+        link[url_head.size .. -1]
+      elsif link.start_with? '/'
+        nil
+      else
+        link
+      end
+    end
+    assert_equal(['foo'], hrefs.compact.sort,
+                 "download page did provide strictly file links")
+  end
 end
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index 9901ec4..aca6d7a 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -58,6 +58,13 @@ admin_noscope:
   expires_at: 2038-01-01 00:00:00
   scopes: []
 
+active_all_collections:
+  api_client: untrusted
+  user: active
+  api_token: activecollectionsabcdefghijklmnopqrstuvwxyz1234567
+  expires_at: 2038-01-01 00:00:00
+  scopes: ["GET /arvados/v1/collections/"]
+
 active_userlist:
   api_client: untrusted
   user: active

commit 42f9144be807fb0a59e684518e9d52241bbb64cc
Author: Brett Smith <brett at curoverse.com>
Date:   Thu May 15 15:54:32 2014 -0400

    2764: Introduce show_file_links route.
    
    Right now this just takes advantage of the provided reader token, but
    we're going to extend it to provide a wget-friendly view.

diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 32118ef..cd45b48 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -1,7 +1,8 @@
 class CollectionsController < ApplicationController
-  skip_around_filter :thread_with_mandatory_api_token, only: [:show_file]
-  skip_before_filter :find_object_by_uuid, only: [:provenance, :show_file]
-  skip_before_filter :check_user_agreements, only: [:show_file]
+  skip_around_filter(:thread_with_mandatory_api_token,
+                     only: [:show_file, :show_file_links])
+  skip_before_filter(:find_object_by_uuid,
+                     only: [:provenance, :show_file, :show_file_links])
 
   def show_pane_list
     %w(Files Attributes Metadata Provenance_graph Used_by JSON API)
@@ -87,6 +88,13 @@ class CollectionsController < ApplicationController
     @request_url = request.url
   end
 
+  def show_file_links
+    Thread.current[:reader_tokens] = [params[:reader_token]]
+    find_object_by_uuid
+    show
+    render 'show'
+  end
+
   def show_file
     # We pipe from arv-get to send the file to the user.  Before we start it,
     # we ask the API server if the file actually exists.  This serves two
diff --git a/apps/workbench/config/routes.rb b/apps/workbench/config/routes.rb
index 350576e..7354457 100644
--- a/apps/workbench/config/routes.rb
+++ b/apps/workbench/config/routes.rb
@@ -43,9 +43,10 @@ ArvadosWorkbench::Application.routes.draw do
   resources :collections do
     post 'set_persistent', on: :member
   end
-  get '/collections/:uuid/*file' => 'collections#show_file', :format => false
   get('/collections/download/:uuid/:reader_token/*file' => 'collections#show_file',
       format: false)
+  get '/collections/download/:uuid/:reader_token' => 'collections#show_file_links'
+  get '/collections/:uuid/*file' => 'collections#show_file', :format => false
   resources :folders do
     match 'remove/:item_uuid', on: :member, via: :delete, action: :remove_item
   end
diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index 70a0b9d..5fc2780 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -50,11 +50,10 @@ class CollectionsControllerTest < ActionController::TestCase
   end
 
   test "viewing collection files with a reader token" do
-    skip  # Need a new route+view for this.
     params = collection_params(:foo_file)
     params[:reader_token] =
       api_fixture('api_client_authorizations')['active']['api_token']
-    get(:show, params)
+    get(:show_file_links, params)
     assert_response :success
     assert_equal([['.', 'foo', 3]], assigns(:object).files)
     assert_no_session

commit 601284ad2b8225fdc3f461f0c90576ddd36a5b0d
Author: Brett Smith <brett at curoverse.com>
Date:   Thu May 15 15:39:16 2014 -0400

    2765: Tear out general Workbench reader_tokens support.
    
    Refs #2765.  This approach is not tenable in its current form, and the
    callbacks are making further development confusing, so I'm getting rid
    of them.

diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index b62838f..ade586c 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -7,12 +7,10 @@ class ApplicationController < ActionController::Base
   ERROR_ACTIONS = [:render_error, :render_not_found]
 
   around_filter :thread_clear
-  around_filter(:thread_with_mandatory_api_token,
-                except: [:index, :show] + ERROR_ACTIONS)
+  around_filter :thread_with_mandatory_api_token, except: ERROR_ACTIONS
   around_filter :thread_with_optional_api_token
   before_filter :check_user_agreements, except: ERROR_ACTIONS
   before_filter :check_user_notifications, except: ERROR_ACTIONS
-  around_filter :using_reader_tokens, only: [:index, :show]
   before_filter :find_object_by_uuid, except: [:index] + ERROR_ACTIONS
   before_filter :check_my_folders, :except => ERROR_ACTIONS
   theme :select_theme
@@ -214,23 +212,6 @@ class ApplicationController < ActionController::Base
     false  # For convenience to return from callbacks
   end
 
-  def using_reader_tokens(login_optional=false)
-    if params[:reader_tokens].is_a?(Array) and params[:reader_tokens].any?
-      Thread.current[:reader_tokens] = params[:reader_tokens]
-    end
-    begin
-      yield
-    rescue ArvadosApiClient::NotLoggedInException
-      if login_optional
-        raise
-      else
-        return redirect_to_login
-      end
-    ensure
-      Thread.current[:reader_tokens] = nil
-    end
-  end
-
   def using_specific_api_token(api_token)
     start_values = {}
     [:arvados_api_token, :user].each do |key|
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 7ef85a5..32118ef 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -93,7 +93,8 @@ class CollectionsController < ApplicationController
     # purposes: it lets us return a useful status code for common errors, and
     # helps us figure out which token to provide to arv-get.
     coll = nil
-    usable_token = find_usable_token do
+    tokens = [Thread.current[:arvados_api_token], params[:reader_token]].compact
+    usable_token = find_usable_token(tokens) do
       coll = Collection.find(params[:uuid])
     end
     if usable_token.nil?
@@ -173,18 +174,14 @@ class CollectionsController < ApplicationController
 
   protected
 
-  def find_usable_token
-    # Iterate over every token available to make it the current token and
+  def find_usable_token(token_list)
+    # Iterate over every given token to make it the current token and
     # yield the given block.
     # If the block succeeds, return the token it used.
     # Otherwise, render an error response based on the most specific
     # error we encounter, and return nil.
-    read_tokens = [Thread.current[:arvados_api_token]].compact
-    if params[:reader_tokens].is_a? Array
-      read_tokens += params[:reader_tokens]
-    end
     most_specific_error = [401]
-    read_tokens.each do |api_token|
+    token_list.each do |api_token|
       using_specific_api_token(api_token) do
         begin
           yield
diff --git a/apps/workbench/config/routes.rb b/apps/workbench/config/routes.rb
index e0c93ec..350576e 100644
--- a/apps/workbench/config/routes.rb
+++ b/apps/workbench/config/routes.rb
@@ -44,6 +44,8 @@ ArvadosWorkbench::Application.routes.draw do
     post 'set_persistent', on: :member
   end
   get '/collections/:uuid/*file' => 'collections#show_file', :format => false
+  get('/collections/download/:uuid/:reader_token/*file' => 'collections#show_file',
+      format: false)
   resources :folders do
     match 'remove/:item_uuid', on: :member, via: :delete, action: :remove_item
   end
diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index 24a2caf..70a0b9d 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -49,32 +49,17 @@ class CollectionsControllerTest < ActionController::TestCase
     assert_equal([['.', 'foo', 3]], assigns(:object).files)
   end
 
-  test "viewing a collection with a reader token" do
+  test "viewing collection files with a reader token" do
+    skip  # Need a new route+view for this.
     params = collection_params(:foo_file)
-    params[:reader_tokens] =
-      [api_fixture('api_client_authorizations')['active']['api_token']]
+    params[:reader_token] =
+      api_fixture('api_client_authorizations')['active']['api_token']
     get(:show, params)
     assert_response :success
     assert_equal([['.', 'foo', 3]], assigns(:object).files)
     assert_no_session
   end
 
-  test "viewing the index with a reader token" do
-    params = {reader_tokens:
-      [api_fixture('api_client_authorizations')['spectator']['api_token']]
-    }
-    get(:index, params)
-    assert_response :success
-    assert_no_session
-    listed_collections = assigns(:collections).map { |c| c.uuid }
-    assert_includes(listed_collections,
-                    api_fixture('collections')['bar_file']['uuid'],
-                    "spectator reader token didn't list bar file")
-    refute_includes(listed_collections,
-                    api_fixture('collections')['foo_file']['uuid'],
-                    "spectator reader token listed foo file")
-  end
-
   test "getting a file from Keep" do
     params = collection_params(:foo_file, 'foo')
     sess = session_for(:active)
@@ -101,7 +86,7 @@ class CollectionsControllerTest < ActionController::TestCase
   test "getting a file from Keep with a good reader token" do
     params = collection_params(:foo_file, 'foo')
     read_token = api_fixture('api_client_authorizations')['active']['api_token']
-    params[:reader_tokens] = [read_token]
+    params[:reader_token] = read_token
     get(:show_file, params)
     assert_response :success
     assert_equal(expected_contents(params, read_token), @response.body,
@@ -112,9 +97,8 @@ class CollectionsControllerTest < ActionController::TestCase
 
   test "trying to get from Keep with an unscoped reader token prompts login" do
     params = collection_params(:foo_file, 'foo')
-    read_token =
+    params[:reader_token] =
       api_fixture('api_client_authorizations')['active_noscope']['api_token']
-    params[:reader_tokens] = [read_token]
     get(:show_file, params)
     assert_response :redirect
   end
@@ -123,7 +107,7 @@ class CollectionsControllerTest < ActionController::TestCase
     params = collection_params(:foo_file, 'foo')
     sess = session_for(:expired)
     read_token = api_fixture('api_client_authorizations')['active']['api_token']
-    params[:reader_tokens] = [read_token]
+    params[:reader_token] = read_token
     get(:show_file, params, sess)
     assert_response :success
     assert_equal(expected_contents(params, read_token), @response.body,
diff --git a/apps/workbench/test/test_helper.rb b/apps/workbench/test/test_helper.rb
index c1eed5c..e833f97 100644
--- a/apps/workbench/test/test_helper.rb
+++ b/apps/workbench/test/test_helper.rb
@@ -36,6 +36,7 @@ class ActiveSupport::TestCase
 
   def teardown
     Thread.current[:arvados_api_token] = nil
+    Thread.current[:reader_tokens] = nil
     super
   end
 end

commit 928bb1abd8163cb1d107bd0b28edf537731ae7df
Author: Brett Smith <brett at curoverse.com>
Date:   Tue May 6 11:30:58 2014 -0400

    1904: Make Keep file EPERM test stricter.
    
    Refs #1904.  Tom said that we want the security benefits of a 404
    result, so make sure that's exactly what we get.

diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index d1a8de2..24a2caf 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -88,7 +88,7 @@ class CollectionsControllerTest < ActionController::TestCase
     params = collection_params(:foo_file, 'foo')
     sess = session_for(:spectator)
     get(:show_file, params, sess)
-    assert_includes([403, 404], @response.code.to_i)
+    assert_response 404
   end
 
   test "trying to get a nonexistent file from Keep returns a 404" do

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list