From: Peter Amstutz <peter.amstutz@curoverse.com>
Date: Wed, 23 Apr 2014 14:57:31 +0000 (-0400)
Subject: - Added object_owner_uuid to logs table, which records the owner of the object
X-Git-Tag: 1.1.0~2596^2~27^2~20
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/923e2bd7d962f1a0feefc73ae4f4531c8235a591?hp=04cc77648cc62c73433801475c27ede4ceb76c8b

- Added object_owner_uuid to logs table, which records the owner of the object
  being described at the time of the log entry.  Added migration.

- Added special case permission checks for reading logs table, and added
  documentation comments on how permission checks work.

- Eventbus reuses readable_by and record_filters in determining what to publish
  so provide consistent behavior between eventbus and regular API queries.

- Put most of apply_where_limit_order_params back into application_controller,
  refactored record_filters into lib/

- All of this still needs testing.
---

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 9c5497bed6..663baa8fa7 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -108,6 +108,103 @@ class ApplicationController < ActionController::Base
 
   protected
 
+  def apply_where_limit_order_params
+    ft = record_filters @filters
+    if ft[:cond_out].any?
+      @objects = @objects.where(ft[:cond_out].join(' AND '), *ft[:param_out])
+    end
+
+    if @where.is_a? Hash and @where.any?
+      conditions = ['1=1']
+      @where.each do |attr,value|
+        if attr.to_s == 'any'
+          if value.is_a?(Array) and
+              value.length == 2 and
+              value[0] == 'contains' then
+            ilikes = []
+            model_class.searchable_columns('ilike').each do |column|
+              ilikes << "#{table_name}.#{column} ilike ?"
+              conditions << "%#{value[1]}%"
+            end
+            if ilikes.any?
+              conditions[0] << ' and (' + ilikes.join(' or ') + ')'
+            end
+          end
+        elsif attr.to_s.match(/^[a-z][_a-z0-9]+$/) and
+            model_class.columns.collect(&:name).index(attr.to_s)
+          if value.nil?
+            conditions[0] << " and #{table_name}.#{attr} is ?"
+            conditions << nil
+          elsif value.is_a? Array
+            if value[0] == 'contains' and value.length == 2
+              conditions[0] << " and #{table_name}.#{attr} like ?"
+              conditions << "%#{value[1]}%"
+            else
+              conditions[0] << " and #{table_name}.#{attr} in (?)"
+              conditions << value
+            end
+          elsif value.is_a? String or value.is_a? Fixnum or value == true or value == false
+            conditions[0] << " and #{table_name}.#{attr}=?"
+            conditions << value
+          elsif value.is_a? Hash
+            # Not quite the same thing as "equal?" but better than nothing?
+            value.each do |k,v|
+              if v.is_a? String
+                conditions[0] << " and #{table_name}.#{attr} ilike ?"
+                conditions << "%#{k}%#{v}%"
+              end
+            end
+          end
+        end
+      end
+      if conditions.length > 1
+        conditions[0].sub!(/^1=1 and /, '')
+        @objects = @objects.
+          where(*conditions)
+      end
+    end
+
+    if params[:limit]
+      begin
+        @limit = params[:limit].to_i
+      rescue
+        raise ArgumentError.new("Invalid value for limit parameter")
+      end
+    else
+      @limit = 100
+    end
+    @objects = @objects.limit(@limit)
+
+    orders = []
+
+    if params[:offset]
+      begin
+        @objects = @objects.offset(params[:offset].to_i)
+        @offset = params[:offset].to_i
+      rescue
+        raise ArgumentError.new("Invalid value for limit parameter")
+      end
+    else
+      @offset = 0
+    end
+
+    orders = []
+    if params[:order]
+      params[:order].split(',').each do |order|
+        attr, direction = order.strip.split " "
+        direction ||= 'asc'
+        if attr.match /^[a-z][_a-z0-9]+$/ and
+            model_class.columns.collect(&:name).index(attr) and
+            ['asc','desc'].index direction.downcase
+          orders << "#{table_name}.#{attr} #{direction.downcase}"
+        end
+      end
+    end
+    if orders.empty?
+      orders << "#{table_name}.modified_at desc"
+    end
+    @objects = @objects.order(orders.join ", ")
+  end
 
   def find_objects_for_index
     @objects ||= model_class.readable_by(current_user)
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 3a7981071f..449a061cc9 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -79,11 +79,33 @@ class ArvadosModel < ActiveRecord::Base
     sanitized_uuid_list = uuid_list.
       collect { |uuid| sanitize(uuid) }.join(', ')
     or_references_me = ''
+
     if self == Link and user
       or_references_me = "OR (#{table_name}.link_class in (#{sanitize 'permission'}, #{sanitize 'resources'}) AND #{sanitize user.uuid} IN (#{table_name}.head_uuid, #{table_name}.tail_uuid))"
     end
-    joins("LEFT JOIN links permissions ON permissions.head_uuid in (#{table_name}.owner_uuid, #{table_name}.uuid) AND permissions.tail_uuid in (#{sanitized_uuid_list}) AND permissions.link_class='permission'").
-      where("?=? OR #{table_name}.owner_uuid in (?) OR #{table_name}.uuid=? OR permissions.head_uuid IS NOT NULL #{or_references_me}",
+
+    if self == Log and user
+      object_owner = ", #{table_name}.object_owner_uuid"
+      or_object_owner = "OR (#{table_name}.object_owner_uuid in (#{sanitized_uuid_list}))"
+    end
+
+    # Link head points to this row, or to the owner of this row
+    # (or owner of object described by this row, for logs table only)
+    # Link tail originates from this user, or a group that is readable by this user
+    # Link is any permission link ('write' and 'manage' implicitly grant 'read')
+    # or
+    # This row is owned by this user, or owned by a group readable by this user
+    # or
+    # This row uuid is equal this user uuid
+    # or
+    # This is the links table
+    # This row is a permission link
+    # The current user is referenced in either the head or tail of the link
+    # or
+    # This is the logs table
+    # This object described by this row is owned by this user, or owned by a group readable by this user
+    joins("LEFT JOIN links permissions ON permissions.head_uuid in (#{table_name}.owner_uuid, #{table_name}.uuid #{object_owner}) AND permissions.tail_uuid in (#{sanitized_uuid_list}) AND permissions.link_class='permission'").
+      where("?=? OR #{table_name}.owner_uuid in (?) OR #{table_name}.uuid=? OR permissions.head_uuid IS NOT NULL #{or_references_me} #{or_object_owner}",
             true, user.is_admin,
             uuid_list,
             user.uuid)
diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index f8e337b201..e7e5c1a833 100644
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -8,7 +8,7 @@ class Log < ArvadosModel
 
   api_accessible :user, extend: :common do |t|
     t.add :object_uuid
-    t.add :object, :if => :object
+    t.add :object_owner_uuid
     t.add :object_kind
     t.add :event_at
     t.add :event_type
@@ -24,6 +24,7 @@ class Log < ArvadosModel
 
   def fill_object(thing)
     self.object_uuid ||= thing.uuid
+    self.object_owner_uuid = thing.owner_uuid
     self.summary ||= "#{self.event_type} of #{thing.uuid}"
     self
   end
@@ -69,4 +70,5 @@ class Log < ArvadosModel
   def ensure_valid_uuids
     # logs can have references to deleted objects
   end
+
 end
diff --git a/services/api/db/migrate/20140423132913_add_object_owner_to_logs.rb b/services/api/db/migrate/20140423132913_add_object_owner_to_logs.rb
new file mode 100644
index 0000000000..7fa47028d0
--- /dev/null
+++ b/services/api/db/migrate/20140423132913_add_object_owner_to_logs.rb
@@ -0,0 +1,21 @@
+class AddObjectOwnerToLogs < ActiveRecord::Migration
+  include CurrentApiClient
+
+  def up
+    add_column :logs, :object_owner_uuid, :string
+    act_as_system_user do
+      Log.all.each do |log|
+        if log.properties[:new_attributes]
+          log.object_owner_uuid = log.properties[:new_attributes][:owner_uuid]
+        elsif log.properties[:old_attributes]
+          log.object_owner_uuid = log.properties[:old_attributes][:owner_uuid]
+        end
+        log.save!
+      end
+    end
+  end
+
+  def down
+    remove_column :logs, :object_owner_uuid
+  end
+end
diff --git a/services/api/db/schema.rb b/services/api/db/schema.rb
index e2301e5be9..fc8e43601e 100644
--- a/services/api/db/schema.rb
+++ b/services/api/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20140407184311) do
+ActiveRecord::Schema.define(:version => 20140423132913) do
 
   create_table "api_client_authorizations", :force => true do |t|
     t.string   "api_token",                                           :null => false
@@ -268,6 +268,7 @@ ActiveRecord::Schema.define(:version => 20140407184311) do
     t.datetime "created_at",              :null => false
     t.datetime "updated_at",              :null => false
     t.datetime "modified_at"
+    t.string   "object_owner_uuid"
   end
 
   add_index "logs", ["created_at"], :name => "index_logs_on_created_at"
diff --git a/services/api/lib/eventbus.rb b/services/api/lib/eventbus.rb
index 646ee83a89..516f5fa71c 100644
--- a/services/api/lib/eventbus.rb
+++ b/services/api/lib/eventbus.rb
@@ -2,6 +2,7 @@ require 'eventmachine'
 require 'oj'
 require 'faye/websocket'
 require 'record_filters'
+require 'load_param'
 
 module Faye
   class WebSocket
@@ -28,27 +29,9 @@ class Filter
   end
 end
 
-class FilterController
-  include RecordFilters
-
-  def initialize(f, o)
-    @filters = f
-    @objects = o
-    apply_where_limit_order_params
-  end
-
-  def each &b
-    @objects.each &b
-  end
-
-  def params
-    {}
-  end
-
-end
-
 class EventBus
   include CurrentApiClient
+  include RecordFilters
 
   def initialize
     @channel = EventMachine::Channel.new
@@ -65,23 +48,58 @@ class EventBus
 
     ws.user = current_user
     ws.filters = []
+    ws.last_log_id = nil
 
     sub = @channel.subscribe do |msg|
-      Log.where(id: msg.to_i).each do |l|
+      # Must have at least one filter set up to receive events
+      if ws.filters.length > 0
+
+        # Start with log rows readable by user, sorted in ascending order
+        logs = Log.readable_by(ws.user).order("id asc")
+
+        if ws.last_log_id
+          # Only get log rows that are new
+          logs = logs.where("log.id > ? and log.id <= ?", ws.last_log_id, msg.to_i)
+        else
+          # No last log id, so only look at the most recently changed row
+          logs = logs.where("log.id = ?", msg.to_i)
+        end
+
+        # Record the most recent row
         ws.last_log_id = msg.to_i
-        if rsc = ArvadosModel::resource_class_for_uuid(l.object_uuid)
-          permitted = rsc.readable_by(ws.user).where(uuid: l.object_uuid)
-          ws.filters.each do |filter|
-            FilterController.new(filter, permitted).each do
-              ws.send(l.as_api_response.to_json)
-            end
-          end
+
+        # Now process filters provided by client
+        cond_out = []
+        param_out = []
+        ws.filters.each do |filter|
+          ft = record_filters filter.filters
+          cond_out += ft[:cond_out]
+          param_out += ft[:param_out]
         end
+
+        # Add filters to query
+        if cond_out.any?
+          logs = logs.where(cond_out.join(' OR '), *param_out)
+        end
+
+        # Finally execute query and send matching rows
+        logs.each do |l|
+          ws.send(l.as_api_response.to_json)
+        end
+      else
+        # No filters set up, so just record the sequence number
+        ws.last_log_id.nil = msg.to_i
       end
     end
 
     ws.on :message do |event|
-      ws.filters = Filter.new oj.parse(event.data)
+      p = oj.parse(event.data)
+      if p[:method] == 'subscribe'
+        if p[:starting_log_id]
+          ws.last_log_id = p[:starting_log_id].to_i
+        end
+        ws.filters.push(Filter.new p)
+      end
     end
 
     ws.on :close do |event|
diff --git a/services/api/lib/record_filters.rb b/services/api/lib/record_filters.rb
index fab16bbc1d..6750797f51 100644
--- a/services/api/lib/record_filters.rb
+++ b/services/api/lib/record_filters.rb
@@ -6,11 +6,11 @@
 #   @objects
 module RecordFilters
 
-  def apply_where_limit_order_params
-    if @filters.is_a? Array and @filters.any?
-      cond_out = []
-      param_out = []
-      @filters.each do |attr, operator, operand|
+  def record_filters filters
+    cond_out = []
+    param_out = []
+    if filters.is_a? Array and filters.any?
+      filters.each do |attr, operator, operand|
         if !model_class.searchable_columns(operator).index attr.to_s
           raise ArgumentError.new("Invalid attribute '#{attr}' in condition")
         end
@@ -46,100 +46,8 @@ module RecordFilters
           cond_out << cond.join(' OR ')
         end
       end
-      if cond_out.any?
-        @objects = @objects.where(cond_out.join(' AND '), *param_out)
-      end
-    end
-    if @where.is_a? Hash and @where.any?
-      conditions = ['1=1']
-      @where.each do |attr,value|
-        if attr.to_s == 'any'
-          if value.is_a?(Array) and
-              value.length == 2 and
-              value[0] == 'contains' then
-            ilikes = []
-            model_class.searchable_columns('ilike').each do |column|
-              ilikes << "#{table_name}.#{column} ilike ?"
-              conditions << "%#{value[1]}%"
-            end
-            if ilikes.any?
-              conditions[0] << ' and (' + ilikes.join(' or ') + ')'
-            end
-          end
-        elsif attr.to_s.match(/^[a-z][_a-z0-9]+$/) and
-            model_class.columns.collect(&:name).index(attr.to_s)
-          if value.nil?
-            conditions[0] << " and #{table_name}.#{attr} is ?"
-            conditions << nil
-          elsif value.is_a? Array
-            if value[0] == 'contains' and value.length == 2
-              conditions[0] << " and #{table_name}.#{attr} like ?"
-              conditions << "%#{value[1]}%"
-            else
-              conditions[0] << " and #{table_name}.#{attr} in (?)"
-              conditions << value
-            end
-          elsif value.is_a? String or value.is_a? Fixnum or value == true or value == false
-            conditions[0] << " and #{table_name}.#{attr}=?"
-            conditions << value
-          elsif value.is_a? Hash
-            # Not quite the same thing as "equal?" but better than nothing?
-            value.each do |k,v|
-              if v.is_a? String
-                conditions[0] << " and #{table_name}.#{attr} ilike ?"
-                conditions << "%#{k}%#{v}%"
-              end
-            end
-          end
-        end
-      end
-      if conditions.length > 1
-        conditions[0].sub!(/^1=1 and /, '')
-        @objects = @objects.
-          where(*conditions)
-      end
-    end
-
-    if params[:limit]
-      begin
-        @limit = params[:limit].to_i
-      rescue
-        raise ArgumentError.new("Invalid value for limit parameter")
-      end
-    else
-      @limit = 100
-    end
-    @objects = @objects.limit(@limit)
-
-    orders = []
-
-    if params[:offset]
-      begin
-        @objects = @objects.offset(params[:offset].to_i)
-        @offset = params[:offset].to_i
-      rescue
-        raise ArgumentError.new("Invalid value for limit parameter")
-      end
-    else
-      @offset = 0
-    end
-
-    orders = []
-    if params[:order]
-      params[:order].split(',').each do |order|
-        attr, direction = order.strip.split " "
-        direction ||= 'asc'
-        if attr.match /^[a-z][_a-z0-9]+$/ and
-            model_class.columns.collect(&:name).index(attr) and
-            ['asc','desc'].index direction.downcase
-          orders << "#{table_name}.#{attr} #{direction.downcase}"
-        end
-      end
-    end
-    if orders.empty?
-      orders << "#{table_name}.modified_at desc"
     end
-    @objects = @objects.order(orders.join ", ")
+    {:cond_out => cond_out, :param_out => param_out}
   end
 
 end