Merge branch 'master' into 2525-java-sdk
authorradhika <radhika@curoverse.com>
Tue, 27 May 2014 15:15:03 +0000 (11:15 -0400)
committerradhika <radhika@curoverse.com>
Tue, 27 May 2014 15:15:03 +0000 (11:15 -0400)
23 files changed:
apps/workbench/app/assets/javascripts/select_modal.js [new file with mode: 0644]
apps/workbench/app/assets/stylesheets/folders.css.scss
apps/workbench/app/assets/stylesheets/select_modal.css.scss [new file with mode: 0644]
apps/workbench/app/controllers/application_controller.rb
apps/workbench/app/controllers/folders_controller.rb
apps/workbench/app/models/arvados_base.rb
apps/workbench/app/views/folders/_choose.html.erb [new file with mode: 0644]
apps/workbench/app/views/folders/_choose.js.erb [new file with mode: 0644]
apps/workbench/app/views/folders/_show_folders.html.erb [new file with mode: 0644]
apps/workbench/app/views/folders/_show_my_folders.html.erb [deleted file]
apps/workbench/app/views/folders/_show_shared_with_me.html.erb [deleted file]
apps/workbench/app/views/folders/show.html.erb
apps/workbench/app/views/layouts/application.html.erb
apps/workbench/config/routes.rb
apps/workbench/test/integration/folders_test.rb
sdk/python/arvados/keep.py
sdk/python/run_test_server.py
sdk/python/test_keep_client.py
services/api/app/models/group.rb
services/keep/src/keep/handler_test.go
services/keep/src/keep/keep.go
services/keep/src/keep/keep_test.go
services/keep/src/keep/perms.go

diff --git a/apps/workbench/app/assets/javascripts/select_modal.js b/apps/workbench/app/assets/javascripts/select_modal.js
new file mode 100644 (file)
index 0000000..85d97c9
--- /dev/null
@@ -0,0 +1,39 @@
+$(document).on('click', '.selectable', function() {
+    var $this = $(this);
+    if (!$this.hasClass('multiple')) {
+        $this.closest('.selectable-container').
+            find('.selectable').
+            removeClass('active');
+    }
+    $this.toggleClass('active');
+}).on('click', '.modal button[data-action-href]', function() {
+    var selection = [];
+    var data = {};
+    var $modal = $(this).closest('.modal');
+    $modal.find('.modal-error').removeClass('hide').hide();
+    $modal.find('.selectable.active[data-object-uuid]').each(function() {
+        selection.push($(this).attr('data-object-uuid'));
+    });
+    data[$(this).data('action-data').selection_param] = selection[0];
+    $.ajax($(this).attr('data-action-href'),
+           {dataType: 'json',
+            type: $(this).attr('data-method'),
+            data: data,
+            context: {modal: $modal}}).
+        fail(function(jqxhr, status, error) {
+            if (jqxhr.readyState == 0 || jqxhr.status == 0) {
+                message = "Cancelled."
+            } else if (jqxhr.responseJSON && jqxhr.responseJSON.errors) {
+                message = jqxhr.responseJSON.errors.join("; ");
+            } else {
+                message = "Request failed.";
+            }
+            this.modal.find('.modal-error').
+                html('<div class="alert alert-danger">' + message + '</div>').
+                show();
+        }).
+        success(function() {
+            this.modal.find('.modal-error').hide();
+            window.location.reload();
+        });
+});
index 1dea7914396a76ec59030e3bde76fae961ed50a3..a033e87058a29988def67052d58043a931c4864f 100644 (file)
@@ -1,3 +1,13 @@
-// Place all the styles related to the folders controller here.
-// They will automatically be included in application.css.
-// You can use Sass (SCSS) here: http://sass-lang.com/
+.arv-folder-list > .row {
+    padding-top: 5px;
+    padding-bottom: 5px;
+    padding-right: 1em;
+}
+.arv-folder-list > .row.folder:hover {
+    background: #d9edf7;
+}
+.arv-folder-list > .row.folder.active,
+.arv-folder-list > .row.folder.active:hover {
+    background: #428bca;
+    color: #fff;
+}
diff --git a/apps/workbench/app/assets/stylesheets/select_modal.css.scss b/apps/workbench/app/assets/stylesheets/select_modal.css.scss
new file mode 100644 (file)
index 0000000..6fe5651
--- /dev/null
@@ -0,0 +1,3 @@
+.selectable.active, .selectable:hover {
+    background: #d9edf7;
+}
index ade586c47422b88d848612a9adf4a19330eae1bd..59ca3503c879edaddcd40579ae2004894250e260 100644 (file)
@@ -12,7 +12,6 @@ class ApplicationController < ActionController::Base
   before_filter :check_user_agreements, except: ERROR_ACTIONS
   before_filter :check_user_notifications, except: ERROR_ACTIONS
   before_filter :find_object_by_uuid, except: [:index] + ERROR_ACTIONS
-  before_filter :check_my_folders, :except => ERROR_ACTIONS
   theme :select_theme
 
   begin
@@ -121,21 +120,21 @@ class ApplicationController < ActionController::Base
   end
 
   def update
-    updates = params[@object.class.to_s.underscore.singularize.to_sym]
-    updates.keys.each do |attr|
+    @updates ||= params[@object.class.to_s.underscore.singularize.to_sym]
+    @updates.keys.each do |attr|
       if @object.send(attr).is_a? Hash
-        if updates[attr].is_a? String
-          updates[attr] = Oj.load updates[attr]
+        if @updates[attr].is_a? String
+          @updates[attr] = Oj.load @updates[attr]
         end
         if params[:merge] || params["merge_#{attr}".to_sym]
           # Merge provided Hash with current Hash, instead of
           # replacing.
-          updates[attr] = @object.send(attr).with_indifferent_access.
-            deep_merge(updates[attr].with_indifferent_access)
+          @updates[attr] = @object.send(attr).with_indifferent_access.
+            deep_merge(@updates[attr].with_indifferent_access)
         end
       end
     end
-    if @object.update_attributes updates
+    if @object.update_attributes @updates
       show
     else
       self.render_error status: 422
@@ -404,15 +403,6 @@ class ApplicationController < ActionController::Base
     }
   }
 
-  def check_my_folders
-    @my_top_level_folders = lambda do
-      @top_level_folders ||= Group.
-        filter([['group_class','=','folder'],
-                ['owner_uuid','=',current_user.uuid]]).
-        sort_by { |x| x.name || '' }
-    end
-  end
-
   def check_user_notifications
     @notification_count = 0
     @notifications = []
index 86ee42b10db54ecd389254d2470c210bc30c64e1..8ebb1a34b101bf8bdbab1050a3485f9330ab8d0f 100644 (file)
@@ -4,7 +4,7 @@ class FoldersController < ApplicationController
   end
 
   def index_pane_list
-    %w(My_folders Shared_with_me)
+    %w(Folders)
   end
 
   def remove_item
@@ -38,33 +38,47 @@ class FoldersController < ApplicationController
   end
 
   def index
-    @my_folders = []
-    @shared_with_me = []
     @objects = Group.where(group_class: 'folder').order('name')
-    owner_of = {}
-    moretodo = true
-    while moretodo
-      moretodo = false
-      @objects.each do |folder|
-        if !owner_of[folder.uuid]
-          moretodo = true
-          owner_of[folder.uuid] = folder.owner_uuid
-        end
-        if owner_of[folder.owner_uuid]
-          if owner_of[folder.uuid] != owner_of[folder.owner_uuid]
-            owner_of[folder.uuid] = owner_of[folder.owner_uuid]
-            moretodo = true
-          end
-        end
+    parent_of = {current_user.uuid => 'me'}
+    @objects.each do |ob|
+      parent_of[ob.uuid] = ob.owner_uuid
+    end
+    children_of = {false => [], 'me' => [current_user]}
+    @objects.each do |ob|
+      if ob.owner_uuid != current_user.uuid and
+          not parent_of.has_key? ob.owner_uuid
+        parent_of[ob.uuid] = false
       end
+      children_of[parent_of[ob.uuid]] ||= []
+      children_of[parent_of[ob.uuid]] << ob
     end
-    @objects.each do |folder|
-      if owner_of[folder.uuid] == current_user.uuid
-        @my_folders << folder
-      else
-        @shared_with_me << folder
+    buildtree = lambda do |children_of, root_uuid=false|
+      tree = {}
+      children_of[root_uuid].andand.each do |ob|
+        tree[ob] = buildtree.call(children_of, ob.uuid)
+      end
+      tree
+    end
+    sorted_paths = lambda do |tree, depth=0|
+      paths = []
+      tree.keys.sort_by { |ob|
+        ob.is_a?(String) ? ob : ob.friendly_link_name
+      }.each do |ob|
+        paths << {object: ob, depth: depth}
+        paths += sorted_paths.call tree[ob], depth+1
       end
+      paths
     end
+    @my_folder_tree =
+      sorted_paths.call buildtree.call(children_of, 'me')
+    @shared_folder_tree =
+      sorted_paths.call({'Shared with me' =>
+                          buildtree.call(children_of, false)})
+  end
+
+  def choose
+    index
+    render partial: 'choose'
   end
 
   def show
@@ -96,4 +110,9 @@ class FoldersController < ApplicationController
     @new_resource_attrs[:name] ||= 'New folder'
     super
   end
+
+  def update
+    @updates = params['folder']
+    super
+  end
 end
index 1ad0230512318bf114e6e30de95d9dc1eb21371f..7d8603782ee252a89399aa9a184d83678d6cfdf8 100644 (file)
@@ -293,7 +293,8 @@ class ArvadosBase < ActiveRecord::Base
     (current_user and current_user.is_active and
      (current_user.is_admin or
       current_user.uuid == self.owner_uuid or
-      new_record?))
+      new_record? or
+      (writable_by.include? current_user.uuid rescue false)))
   end
 
   def attribute_editable?(attr)
@@ -301,12 +302,10 @@ class ArvadosBase < ActiveRecord::Base
       false
     elsif not (current_user.andand.is_active)
       false
-    elsif "uuid owner_uuid".index(attr.to_s) or current_user.is_admin
+    elsif attr == 'uuid'
       current_user.is_admin
     else
-      current_user.uuid == self.owner_uuid or
-        current_user.uuid == self.uuid or
-        new_record?
+      editable?
     end
   end
 
@@ -351,6 +350,10 @@ class ArvadosBase < ActiveRecord::Base
     friendly_link_name
   end
 
+  def owner
+    ArvadosBase.find(owner_uuid) rescue nil
+  end
+
   protected
 
   def forget_uuid!
diff --git a/apps/workbench/app/views/folders/_choose.html.erb b/apps/workbench/app/views/folders/_choose.html.erb
new file mode 100644 (file)
index 0000000..c27d669
--- /dev/null
@@ -0,0 +1,42 @@
+<div class="modal">
+  <div class="modal-dialog">
+    <div class="modal-content">
+
+      <div class="modal-header">
+        <button type="button" class="close" onClick="reset_form()" data-dismiss="modal" aria-hidden="true">&times;</button>
+        <h4 class="modal-title"><%= params[:title] || 'Choose folder' %></h4>
+      </div>
+
+      <div class="modal-body">
+        <div class="container-fluid arv-folder-list selectable-container" style="height: 15em; overflow-y: scroll">
+          <% [@my_folder_tree, @shared_folder_tree].each do |tree| %>
+            <% tree.each do |foldernode| %>
+              <% if foldernode[:object].is_a? String %>
+                <div class="row" style="padding-left: <%= 1 + foldernode[:depth] %>em;">
+                  <i class="fa fa-fw fa-folder-open-o"></i>
+                  <%= foldernode[:object] %>
+                </div>
+              <% else %>
+                <div class="<%= 'selectable folder' if !params[:editable] || foldernode[:object].editable? %> row" style="padding-left: <%= 1 + foldernode[:depth] %>em;" data-object-uuid="<%= foldernode[:object].uuid %>">
+                  <i class="fa fa-fw fa-folder-o"></i>
+                  <% if foldernode[:object].uuid == current_user.uuid %>
+                    My Folders
+                  <% else %>
+                    <%= foldernode[:object].friendly_link_name || 'New folder' %>
+                  <% end %>
+                </div>
+              <% end %>
+            <% end %>
+          <% end %>
+        </div>
+      </div>
+
+      <div class="modal-footer">
+        <button class="btn btn-default" data-dismiss="modal" aria-hidden="true">Cancel</button>
+        <button class="btn btn-primary" aria-hidden="true"><%= params[:action_name] || 'Select' %></button>
+        <div class="modal-error hide" style="text-align: left; margin-top: 1em;">
+        </div>
+      </div>
+    </div>
+  </div>
+</div>
diff --git a/apps/workbench/app/views/folders/_choose.js.erb b/apps/workbench/app/views/folders/_choose.js.erb
new file mode 100644 (file)
index 0000000..4ecd072
--- /dev/null
@@ -0,0 +1,8 @@
+$('body > .modal-container').html("<%= escape_javascript(render partial: 'choose.html') %>");
+$('body > .modal-container .modal').modal('show');
+$('body > .modal-container .modal .modal-footer .btn-primary').
+    addClass('<%= j params[:action_class] %>').
+    attr('data-action-href', '<%= j params[:action_href] %>').
+    attr('data-method', '<%= j params[:action_method] %>').
+    data('action-data', <%= raw params[:action_data] %>);
+$(document).trigger('ajax:complete');
diff --git a/apps/workbench/app/views/folders/_show_folders.html.erb b/apps/workbench/app/views/folders/_show_folders.html.erb
new file mode 100644 (file)
index 0000000..2ecfd6e
--- /dev/null
@@ -0,0 +1,29 @@
+<div class="container-fluid arv-folder-list">
+  <% [@my_folder_tree, @shared_folder_tree].each do |tree| %>
+    <% tree.each do |foldernode| %>
+      <% rowtype = foldernode[:object].class %>
+      <div class="<%= 'folder' if rowtype == Group %> row" style="padding-left: <%= 1 + foldernode[:depth] %>em;">
+        <% if rowtype == String %>
+          <i class="fa fa-fw fa-folder-open-o"></i>
+          <%= foldernode[:object] %>
+        <% elsif rowtype == User %>
+          <% if foldernode[:object].uuid == current_user.andand.uuid %>
+            <i class="fa fa-fw fa-folder-open-o"></i>
+            My Folders
+          <% else %>
+            <i class="fa fa-fw fa-folder-o"></i>
+            <%= foldernode[:object].friendly_link_name %>
+          <% end %>
+        <% else %>
+          <i class="fa fa-fw fa-folder-o"></i>
+          <%= link_to foldernode[:object] do %>
+            <%= foldernode[:object].friendly_link_name %>
+          <% end %>
+          <div class="pull-right">
+            <%= render partial: 'delete_object_button', locals: {object: foldernode[:object]} %>
+          </div>
+        <% end %>
+      </div>
+    <% end %>
+  <% end %>
+</div>
diff --git a/apps/workbench/app/views/folders/_show_my_folders.html.erb b/apps/workbench/app/views/folders/_show_my_folders.html.erb
deleted file mode 100644 (file)
index b009acf..0000000
+++ /dev/null
@@ -1,2 +0,0 @@
-<%= render(partial: 'show_recent',
-    locals: { comparable: comparable, objects: @my_folders }) %>
diff --git a/apps/workbench/app/views/folders/_show_shared_with_me.html.erb b/apps/workbench/app/views/folders/_show_shared_with_me.html.erb
deleted file mode 100644 (file)
index 6ccf983..0000000
+++ /dev/null
@@ -1,2 +0,0 @@
-<%= render(partial: 'show_recent',
-    locals: { comparable: comparable, objects: @shared_with_me }) %>
index 9a7fccf5b84f5333f7d2d8bff63e66ad796b12ae..ebf504ef7ec34416d382728f4121a4564cd21db8 100644 (file)
         <input type="text" class="form-control" placeholder="Search"/>
         -->
         <div style="height:0.5em;"></div>
-        <p>Owner: <%= link_to_if_arvados_object @object.owner_uuid, friendly_name: true %></p>
-        <% if @share_links.any? %>
-        <p>Shared with:
-          <% @share_links.andand.each do |link| %>
-          <br /><%= link_to_if_arvados_object link.tail_uuid, friendly_name: true %>
+        <% if @object.owner %>
+          <p>Permissions inherited from:
+            <br />
+            <% if User == resource_class_for_uuid(@object.owner_uuid) %>
+              <i class="fa fa-fw fa-user"></i>
+            <% else %>
+              <i class="fa fa-fw fa-folder"></i>
+            <% end %>
+            <%= link_to_if_arvados_object @object.owner_uuid, friendly_name: true %>
+            <%= button_to('Move to...',
+                choose_folders_path(
+                 title: 'Move to...',
+                 editable: true,
+                 action_name: 'Move',
+                 action_href: folder_path(@object.uuid),
+                 action_method: 'put',
+                 action_data: {selection_param: 'folder[owner_uuid]'}.to_json),
+                { class: "btn btn-default btn-xs arv-move-to-folder", remote: true, method: 'get' }) %>
+          </p>
+          <hr />
+        <% end %>
+        <p>
+          <% if not @share_links.any? %>
+            <span class="deemphasize">(No additional permissions)</span>
+          <% else %>
+            Also shared with:
+            <% @share_links.andand.each do |link| %>
+              <br /><%= link_to_if_arvados_object link.tail_uuid, friendly_name: true %>
+            <% end %>
           <% end %>
         </p>
-        <% end %>
       </div>
     </div>
   </div>
index a5460c295f7d0ac550f931e92034fec5dfd9f2b6..c2be7c9c34e43bc5aa755b2203344642d0fd751a 100644 (file)
             </li>
 
             <li class="dropdown">
-              <a href="/folders" class="dropdown-toggle" data-toggle="dropdown"><i class="fa fa-lg fa-folder-o fa-fw"></i> Folders <b class="caret"></b></a>
-              <ul class="dropdown-menu">
-                <li><%= link_to raw('<i class="fa fa-plus fa-fw"></i> Create new folder'), folders_path, method: :post %></li>
-                <% @my_top_level_folders.call[0..7].each do |folder| %>
-                <li><%= link_to raw('<i class="fa fa-folder-open fa-fw"></i> ') + folder.name, folder_path(folder) %></li>
-                <% end %>
-                <li><a href="/folders">
-                    <i class="fa fa-ellipsis-h fa-fw"></i> Show all folders
-                </a></li>
-              </ul>
-            </li>
+              <a href="/folders">
+                <i class="fa fa-lg fa-folder-o fa-fw"></i> Folders
+            </a></li>
             <li><a href="/collections">
                 <i class="fa fa-lg fa-briefcase fa-fw"></i> Collections (data files)
             </a></li>
   <%= yield :footer_js %>
   <% end %>
 
+<div class="modal-container"></div>
 </body>
 </html>
index c12cc989d71cd182ac16003909c67e21cecfa04d..77ffda2b4cd1720f2bcf755af9eb0746c5f69efc 100644 (file)
@@ -50,6 +50,7 @@ ArvadosWorkbench::Application.routes.draw do
   get '/collections/:uuid/*file' => 'collections#show_file', :format => false
   resources :folders do
     match 'remove/:item_uuid', on: :member, via: :delete, action: :remove_item
+    get 'choose', on: :collection
   end
 
   post 'actions' => 'actions#post'
index dc51b7724d7832ee8e363206230b764224de2c92..7518d3cb2651e1c9d9b86898f0a157551190f876 100644 (file)
@@ -3,12 +3,14 @@ require 'selenium-webdriver'
 require 'headless'
 
 class FoldersTest < ActionDispatch::IntegrationTest
+  setup do
+    Capybara.current_driver = Capybara.javascript_driver
+  end
 
   test 'Find a folder and edit its description' do
-    Capybara.current_driver = Capybara.javascript_driver
     visit page_with_token 'active', '/'
     find('nav a', text: 'Folders').click
-    find('.side-nav a,button', text: 'A Folder').
+    find('.arv-folder-list a,button', text: 'A Folder').
       click
     within('.panel', text: api_fixture('groups')['afolder']['name']) do
       find('span', text: api_fixture('groups')['afolder']['name']).click
@@ -24,7 +26,6 @@ class FoldersTest < ActionDispatch::IntegrationTest
   end
 
   test 'Add a new name, then edit it, without creating a duplicate' do
-    Capybara.current_driver = Capybara.javascript_driver
     folder_uuid = api_fixture('groups')['afolder']['uuid']
     specimen_uuid = api_fixture('specimens')['owned_by_afolder_with_no_name_link']['uuid']
     visit page_with_token 'active', '/folders/' + folder_uuid
@@ -45,4 +46,38 @@ class FoldersTest < ActionDispatch::IntegrationTest
     end
   end
 
+  test 'Create a folder and move it into a different folder' do
+    visit page_with_token 'active', '/folders'
+    find('input[value="Add a new folder"]').click
+
+    within('.panel', text: 'New folder') do
+      find('.panel-title span', text: 'New folder').click
+      find('.editable-input input').set('Folder 1234')
+      find('.glyphicon-ok').click
+    end
+    wait_for_ajax
+
+    visit '/folders'
+    find('input[value="Add a new folder"]').click
+    within('.panel', text: 'New folder') do
+      find('.panel-title span', text: 'New folder').click
+      find('.editable-input input').set('Folder 5678')
+      find('.glyphicon-ok').click
+    end
+    wait_for_ajax
+
+    find('input[value="Move to..."]').click
+    find('.selectable', text: 'Folder 1234').click
+    find('a,button', text: 'Move').click
+    wait_for_ajax
+
+    # Wait for the page to refresh and show the new parent folder in
+    # the Permissions panel:
+    find('.panel', text: 'Folder 1234')
+
+    assert(find('.panel', text: 'Permissions inherited from').
+           all('*', text: 'Folder 1234').any?,
+           "Folder 5678 should now be inside folder 1234")
+  end
+
 end
index fcb59ec805680167a9de22caf4d6dc27d30caa92..686b940978b23c1907d19b7403481dad6e3964fb 100644 (file)
@@ -55,6 +55,7 @@ class KeepClient(object):
         def __init__(self, todo):
             self._todo = todo
             self._done = 0
+            self._response = None
             self._todo_lock = threading.Semaphore(todo)
             self._done_lock = threading.Lock()
 
@@ -73,12 +74,23 @@ class KeepClient(object):
             with self._done_lock:
                 return (self._done < self._todo)
 
-        def increment_done(self):
+        def save_response(self, response_body):
             """
-            Report that the current thread was successful.
+            Records a response body (a locator, possibly signed) returned by
+            the Keep server.  It is not necessary to save more than
+            one response, since we presume that any locator returned
+            in response to a successful request is valid.
             """
             with self._done_lock:
                 self._done += 1
+                self._response = response_body
+
+        def response(self):
+            """
+            Returns the body from the response to a PUT request.
+            """
+            with self._done_lock:
+                return self._response
 
         def done(self):
             """
@@ -89,9 +101,9 @@ class KeepClient(object):
 
     class KeepWriterThread(threading.Thread):
         """
-        Write a blob of data to the given Keep server. Call
-        increment_done() of the given ThreadLimiter if the write
-        succeeds.
+        Write a blob of data to the given Keep server. On success, call
+        save_response() of the given ThreadLimiter to save the returned
+        locator.
         """
         def __init__(self, **kwargs):
             super(KeepClient.KeepWriterThread, self).__init__()
@@ -129,7 +141,7 @@ class KeepClient(object):
                                       (str(threading.current_thread()),
                                        self.args['data_hash'],
                                        self.args['service_root']))
-                        return limiter.increment_done()
+                        return limiter.save_response(content.strip())
                     logging.warning("Request fail: PUT %s => %s %s" %
                                     (url, resp['status'], content))
                 except (httplib2.HttpLib2Error, httplib.HTTPException) as e:
@@ -275,7 +287,7 @@ class KeepClient(object):
 
         try:
             for service_root in self.shuffled_service_roots(expect_hash):
-                url = service_root + expect_hash
+                url = service_root + locator
                 api_token = config.get('ARVADOS_API_TOKEN')
                 headers = {'Authorization': "OAuth2 %s" % api_token,
                            'Accept': 'application/octet-stream'}
@@ -287,7 +299,7 @@ class KeepClient(object):
 
             for location_hint in re.finditer(r'\+K@([a-z0-9]+)', locator):
                 instance = location_hint.group(1)
-                url = 'http://keep.' + instance + '.arvadosapi.com/' + expect_hash
+                url = 'http://keep.' + instance + '.arvadosapi.com/' + locator
                 blob = self.get_url(url, {}, expect_hash)
                 if blob:
                     slot.set(blob)
@@ -346,8 +358,9 @@ class KeepClient(object):
         for t in threads:
             t.join()
         have_copies = thread_limiter.done()
+        # If we're done, return the response from Keep
         if have_copies == want_copies:
-            return (data_hash + '+' + str(len(data)))
+            return thread_limiter.response()
         raise arvados.errors.KeepWriteError(
             "Write fail for %s: wanted %d but wrote %d" %
             (data_hash, want_copies, have_copies))
index dbb4ff066d4c880f3af05624950b063a4f366805..4b65d72cd02ce8cd5446921223079a686dc23253 100644 (file)
@@ -123,15 +123,22 @@ def stop():
 
     os.chdir(cwd)
 
-def _start_keep(n):
+def _start_keep(n, keep_args):
     keep0 = tempfile.mkdtemp()
-    kp0 = subprocess.Popen(["bin/keep", "-volumes={}".format(keep0), "-listen=:{}".format(25107+n)])
+    keep_cmd = ["bin/keep",
+                "-volumes={}".format(keep0),
+                "-listen=:{}".format(25107+n)]
+
+    for arg, val in keep_args.iteritems():
+        keep_cmd.append("{}={}".format(arg, val))
+
+    kp0 = subprocess.Popen(keep_cmd)
     with open("tmp/keep{}.pid".format(n), 'w') as f:
         f.write(str(kp0.pid))
     with open("tmp/keep{}.volume".format(n), 'w') as f:
         f.write(keep0)
 
-def run_keep():
+def run_keep(blob_signing_key=None, enforce_permissions=False):
     stop_keep()
 
     cwd = os.getcwd()
@@ -146,8 +153,16 @@ def run_keep():
     if not os.path.exists("tmp"):
         os.mkdir("tmp")
 
-    _start_keep(0)
-    _start_keep(1)
+    keep_args = {}
+    if blob_signing_key:
+        with open("tmp/keep.blob_signing_key", "w") as f:
+            f.write(blob_signing_key)
+        keep_args['--permission-key-file'] = 'tmp/keep.blob_signing_key'
+    if enforce_permissions:
+        keep_args['--enforce-permissions'] = 'true'
+
+    _start_keep(0, keep_args)
+    _start_keep(1, keep_args)
 
 
     os.environ["ARVADOS_API_HOST"] = "127.0.0.1:3001"
@@ -172,6 +187,8 @@ def _stop_keep(n):
     if os.path.exists("tmp/keep{}.volume".format(n)):
         with open("tmp/keep{}.volume".format(n), 'r') as r:
             shutil.rmtree(r.read(), True)
+    if os.path.exists("tmp/keep.blob_signing_key"):
+        os.remove("tmp/keep.blob_signing_key")
 
 def stop_keep():
     cwd = os.getcwd()
index aa79b0d991e3fc172069600e98ee3f68b10434c2..f863ad3c01e03707ffd17b11f903bcbf7f15b40d 100644 (file)
@@ -65,3 +65,124 @@ class KeepTestCase(unittest.TestCase):
         self.assertEqual(arvados.Keep.get(blob_locator),
                          blob_str,
                          'wrong content from Keep.get(md5(<binarydata>))')
+
+class KeepPermissionTestCase(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        try:
+            del os.environ['KEEP_LOCAL_STORE']
+        except KeyError:
+            pass
+        run_test_server.run()
+        run_test_server.run_keep(blob_signing_key='abcdefghijk0123456789',
+                                 enforce_permissions=True)
+
+    @classmethod
+    def tearDownClass(cls):
+        run_test_server.stop()
+        run_test_server.stop_keep()
+
+    def test_KeepBasicRWTest(self):
+        run_test_server.authorize_with('active')
+        foo_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            foo_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + foo_locator)
+        self.assertEqual(arvados.Keep.get(foo_locator),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
+
+        # With Keep permissions enabled, a GET request without a signature will fail.
+        bar_locator = arvados.Keep.put('bar')
+        self.assertRegexpMatches(
+            bar_locator,
+            r'^37b51d194a7513e45b56f6524f2d51f2\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("bar"): ' + bar_locator)
+        self.assertRaises(arvados.errors.NotFoundError,
+                          arvados.Keep.get,
+                          "37b51d194a7513e45b56f6524f2d51f2")
+
+        # A request without an API token will also fail.
+        del arvados.config.settings()["ARVADOS_API_TOKEN"]
+        self.assertRaises(arvados.errors.NotFoundError,
+                          arvados.Keep.get,
+                          bar_locator)
+
+# KeepOptionalPermission: starts Keep with --permission-key-file
+# but not --enforce-permissions (i.e. generate signatures on PUT
+# requests, but do not require them for GET requests)
+#
+# All of these requests should succeed when permissions are optional:
+# * authenticated request, signed locator
+# * authenticated request, unsigned locator
+# * unauthenticated request, signed locator
+# * unauthenticated request, unsigned locator
+
+class KeepOptionalPermission(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        try:
+            del os.environ['KEEP_LOCAL_STORE']
+        except KeyError:
+            pass
+        run_test_server.run()
+        run_test_server.run_keep(blob_signing_key='abcdefghijk0123456789',
+                                 enforce_permissions=False)
+
+    @classmethod
+    def tearDownClass(cls):
+        run_test_server.stop()
+        run_test_server.stop_keep()
+
+    def test_KeepAuthenticatedSignedTest(self):
+        run_test_server.authorize_with('active')
+        signed_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            signed_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + signed_locator)
+        self.assertEqual(arvados.Keep.get(signed_locator),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
+
+    def test_KeepAuthenticatedUnsignedTest(self):
+        run_test_server.authorize_with('active')
+        signed_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            signed_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + signed_locator)
+        self.assertEqual(arvados.Keep.get("acbd18db4cc2f85cedef654fccc4a4d8"),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
+
+    def test_KeepUnauthenticatedSignedTest(self):
+        # Since --enforce-permissions is not in effect, GET requests
+        # need not be authenticated.
+        run_test_server.authorize_with('active')
+        signed_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            signed_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + signed_locator)
+
+        del arvados.config.settings()["ARVADOS_API_TOKEN"]
+        self.assertEqual(arvados.Keep.get(signed_locator),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
+
+    def test_KeepUnauthenticatedUnsignedTest(self):
+        # Since --enforce-permissions is not in effect, GET requests
+        # need not be authenticated.
+        run_test_server.authorize_with('active')
+        signed_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            signed_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + signed_locator)
+
+        del arvados.config.settings()["ARVADOS_API_TOKEN"]
+        self.assertEqual(arvados.Keep.get("acbd18db4cc2f85cedef654fccc4a4d8"),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
index f05571651ddc328079ab770de6e6a6c83e971a1c..d3802441317f21f5be00ccc194d16c2c7338037f 100644 (file)
@@ -5,6 +5,8 @@ class Group < ArvadosModel
   include KindAndEtag
   include CommonApiTemplate
   include CanBeAnOwner
+  after_create :invalidate_permissions_cache
+  after_update :maybe_invalidate_permissions_cache
 
   api_accessible :user, extend: :common do |t|
     t.add :name
@@ -12,4 +14,18 @@ class Group < ArvadosModel
     t.add :description
     t.add :writable_by
   end
+
+  def maybe_invalidate_permissions_cache
+    if uuid_changed? or owner_uuid_changed?
+      # This can change users' permissions on other groups as well as
+      # this one.
+      invalidate_permissions_cache
+    end
+  end
+
+  def invalidate_permissions_cache
+    # Ensure a new group can be accessed by the appropriate users
+    # immediately after being created.
+    User.invalidate_permissions_cache
+  end
 end
index c25b80ddaae4fb782af7d8402ed79f573f3d1466..1b0a90fb4e7096c38d2236629122bd82c8ad3da5 100644 (file)
@@ -16,6 +16,7 @@ import (
        "net/http"
        "net/http/httptest"
        "regexp"
+       "strings"
        "testing"
        "time"
 )
@@ -170,7 +171,9 @@ func TestPutHandler(t *testing.T) {
 
        ExpectStatusCode(t,
                "Unauthenticated request, no server key", http.StatusOK, response)
-       ExpectBody(t, "Unauthenticated request, no server key", TEST_HASH, response)
+       ExpectBody(t,
+               "Unauthenticated request, no server key",
+               TEST_HASH_PUT_RESPONSE, response)
 
        // ------------------
        // With a server key.
@@ -194,10 +197,11 @@ func TestPutHandler(t *testing.T) {
        ExpectStatusCode(t,
                "Authenticated PUT, signed locator, with server key",
                http.StatusOK, response)
-       if !VerifySignature(response.Body.String(), known_token) {
+       response_locator := strings.TrimSpace(response.Body.String())
+       if !VerifySignature(response_locator, known_token) {
                t.Errorf("Authenticated PUT, signed locator, with server key:\n"+
                        "response '%s' does not contain a valid signature",
-                       response.Body.String())
+                       response_locator)
        }
 
        // Unauthenticated PUT, unsigned locator
@@ -214,7 +218,7 @@ func TestPutHandler(t *testing.T) {
                http.StatusOK, response)
        ExpectBody(t,
                "Unauthenticated PUT, unsigned locator, with server key",
-               TEST_HASH, response)
+               TEST_HASH_PUT_RESPONSE, response)
 }
 
 // Test /index requests:
@@ -418,7 +422,7 @@ func IssueRequest(router *mux.Router, rt *RequestTester) *httptest.ResponseRecor
        body := bytes.NewReader(rt.request_body)
        req, _ := http.NewRequest(rt.method, rt.uri, body)
        if rt.api_token != "" {
-               req.Header.Set("Authorization", "OAuth "+rt.api_token)
+               req.Header.Set("Authorization", "OAuth2 "+rt.api_token)
        }
        router.ServeHTTP(response, req)
        return response
index 001e66eba2075bb1b6fde9c3ca5f36c8889d09ba..1dccb12fbaed16738eec05b8d8f2f3d7e2cf3a8c 100644 (file)
@@ -17,6 +17,7 @@ import (
        "os"
        "os/signal"
        "regexp"
+       "runtime"
        "strconv"
        "strings"
        "syscall"
@@ -70,6 +71,7 @@ type KeepError struct {
 }
 
 var (
+       BadRequestError = &KeepError{400, "Bad Request"}
        CollisionError  = &KeepError{400, "Collision"}
        MD5Error        = &KeepError{401, "MD5 Failure"}
        PermissionError = &KeepError{401, "Permission denied"}
@@ -268,11 +270,13 @@ func main() {
 //
 func MakeRESTRouter() *mux.Router {
        rest := mux.NewRouter()
+
        rest.HandleFunc(
                `/{hash:[0-9a-f]{32}}`, GetBlockHandler).Methods("GET", "HEAD")
        rest.HandleFunc(
-               `/{hash:[0-9a-f]{32}}+A{signature:[0-9a-f]+}@{timestamp:[0-9a-f]+}`,
+               `/{hash:[0-9a-f]{32}}+{hints}`,
                GetBlockHandler).Methods("GET", "HEAD")
+
        rest.HandleFunc(`/{hash:[0-9a-f]{32}}`, PutBlockHandler).Methods("PUT")
 
        // For IndexHandler we support:
@@ -291,9 +295,18 @@ func MakeRESTRouter() *mux.Router {
        rest.HandleFunc(
                `/index/{prefix:[0-9a-f]{0,32}}`, IndexHandler).Methods("GET", "HEAD")
        rest.HandleFunc(`/status.json`, StatusHandler).Methods("GET", "HEAD")
+
+       // Any request which does not match any of these routes gets
+       // 400 Bad Request.
+       rest.NotFoundHandler = http.HandlerFunc(BadRequestHandler)
+
        return rest
 }
 
+func BadRequestHandler(w http.ResponseWriter, r *http.Request) {
+       http.Error(w, BadRequestError.Error(), BadRequestError.HTTPCode)
+}
+
 // FindKeepVolumes
 //     Returns a list of Keep volumes mounted on this system.
 //
@@ -330,8 +343,29 @@ func GetBlockHandler(resp http.ResponseWriter, req *http.Request) {
 
        log.Printf("%s %s", req.Method, hash)
 
-       signature := mux.Vars(req)["signature"]
-       timestamp := mux.Vars(req)["timestamp"]
+       hints := mux.Vars(req)["hints"]
+
+       // Parse the locator string and hints from the request.
+       // TODO(twp): implement a Locator type.
+       var signature, timestamp string
+       if hints != "" {
+               signature_pat, _ := regexp.Compile("^A([[:xdigit:]]+)@([[:xdigit:]]{8})$")
+               for _, hint := range strings.Split(hints, "+") {
+                       if match, _ := regexp.MatchString("^[[:digit:]]+$", hint); match {
+                               // Server ignores size hints
+                       } else if m := signature_pat.FindStringSubmatch(hint); m != nil {
+                               signature = m[1]
+                               timestamp = m[2]
+                       } else if match, _ := regexp.MatchString("^[[:upper:]]", hint); match {
+                               // Any unknown hint that starts with an uppercase letter is
+                               // presumed to be valid and ignored, to permit forward compatibility.
+                       } else {
+                               // Unknown format; not a valid locator.
+                               http.Error(resp, BadRequestError.Error(), BadRequestError.HTTPCode)
+                               return
+                       }
+               }
+       }
 
        // If permission checking is in effect, verify this
        // request's permission signature.
@@ -343,8 +377,8 @@ func GetBlockHandler(resp http.ResponseWriter, req *http.Request) {
                        http.Error(resp, ExpiredError.Error(), ExpiredError.HTTPCode)
                        return
                } else {
-                       validsig := MakePermSignature(hash, GetApiToken(req), timestamp)
-                       if signature != validsig {
+                       req_locator := req.URL.Path[1:] // strip leading slash
+                       if !VerifySignature(req_locator, GetApiToken(req)) {
                                http.Error(resp, PermissionError.Error(), PermissionError.HTTPCode)
                                return
                        }
@@ -352,6 +386,13 @@ func GetBlockHandler(resp http.ResponseWriter, req *http.Request) {
        }
 
        block, err := GetBlock(hash)
+
+       // Garbage collect after each GET. Fixes #2865.
+       // TODO(twp): review Keep memory usage and see if there's
+       // a better way to do this than blindly garbage collecting
+       // after every block.
+       defer runtime.GC()
+
        if err != nil {
                // This type assertion is safe because the only errors
                // GetBlock can return are CorruptError or NotFoundError.
@@ -370,6 +411,10 @@ func GetBlockHandler(resp http.ResponseWriter, req *http.Request) {
 }
 
 func PutBlockHandler(resp http.ResponseWriter, req *http.Request) {
+       // Garbage collect after each PUT. Fixes #2865.
+       // See also GetBlockHandler.
+       defer runtime.GC()
+
        hash := mux.Vars(req)["hash"]
 
        log.Printf("%s %s", req.Method, hash)
@@ -386,11 +431,15 @@ func PutBlockHandler(resp http.ResponseWriter, req *http.Request) {
        //
        if buf, err := ReadAtMost(req.Body, BLOCKSIZE); err == nil {
                if err := PutBlock(buf, hash); err == nil {
-                       // Success; sign the locator and return it to the client.
+                       // Success; add a size hint, sign the locator if
+                       // possible, and return it to the client.
+                       return_hash := fmt.Sprintf("%s+%d", hash, len(buf))
                        api_token := GetApiToken(req)
-                       expiry := time.Now().Add(permission_ttl)
-                       signed_loc := SignLocator(hash, api_token, expiry)
-                       resp.Write([]byte(signed_loc))
+                       if PermissionSecret != nil && api_token != "" {
+                               expiry := time.Now().Add(permission_ttl)
+                               return_hash = SignLocator(return_hash, api_token, expiry)
+                       }
+                       resp.Write([]byte(return_hash + "\n"))
                } else {
                        ke := err.(*KeepError)
                        http.Error(resp, ke.Error(), ke.HTTPCode)
@@ -652,13 +701,15 @@ func IsValidLocator(loc string) bool {
        return false
 }
 
-// GetApiToken returns the OAuth token from the Authorization
+// GetApiToken returns the OAuth2 token from the Authorization
 // header of a HTTP request, or an empty string if no matching
 // token is found.
 func GetApiToken(req *http.Request) string {
        if auth, ok := req.Header["Authorization"]; ok {
-               if strings.HasPrefix(auth[0], "OAuth ") {
-                       return auth[0][6:]
+               if pat, err := regexp.Compile(`^OAuth2\s+(.*)`); err != nil {
+                       log.Println(err)
+               } else if match := pat.FindStringSubmatch(auth[0]); match != nil {
+                       return match[1]
                }
        }
        return ""
index 6642c72211a4c1fb44bd0cd02b6a64955c1b515f..7a787c10874b2b06a26f531073f29aa1f55e77b7 100644 (file)
@@ -12,6 +12,7 @@ import (
 
 var TEST_BLOCK = []byte("The quick brown fox jumps over the lazy dog.")
 var TEST_HASH = "e4d909c290d0fb1ca068ffaddf22cbd0"
+var TEST_HASH_PUT_RESPONSE = "e4d909c290d0fb1ca068ffaddf22cbd0+44\n"
 
 var TEST_BLOCK_2 = []byte("Pack my box with five dozen liquor jugs.")
 var TEST_HASH_2 = "f15ac516f788aec4f30932ffb6395c39"
index 0d1b091365085bbb079cc9cb1dd86eb1d6973b54..143815576b33f077a89fead6e46efb795de20f9d 100644 (file)
@@ -83,7 +83,7 @@ func SignLocator(blob_locator string, api_token string, expiry time.Time) string
 // VerifySignature returns true if the signature on the signed_locator
 // can be verified using the given api_token.
 func VerifySignature(signed_locator string, api_token string) bool {
-       if re, err := regexp.Compile(`^(.*)\+A(.*)@(.*)$`); err == nil {
+       if re, err := regexp.Compile(`^([a-f0-9]{32}(\+[0-9]+)?).*\+A[[:xdigit:]]+@([[:xdigit:]]{8})`); err == nil {
                if matches := re.FindStringSubmatch(signed_locator); matches != nil {
                        blob_locator := matches[1]
                        timestamp_hex := matches[3]