[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