Merge remote-tracking branch 'origin/master' into origin-2228-check-filter-uuid-columns
authorPeter Amstutz <peter.amstutz@curoverse.com>
Fri, 11 Apr 2014 20:04:28 +0000 (16:04 -0400)
committerPeter Amstutz <peter.amstutz@curoverse.com>
Fri, 11 Apr 2014 20:04:28 +0000 (16:04 -0400)
Conflicts:
services/api/app/models/arvados_model.rb

1  2 
services/api/app/models/arvados_model.rb
services/api/app/models/log.rb
services/api/db/migrate/20140325175653_remove_kind_columns.rb
services/api/db/schema.rb
services/api/test/integration/permissions_test.rb
services/api/test/unit/log_test.rb

index 0cc26c5efc46bf849012b8296bdedc0132e5b617,921d2b17c5152ed04fc742459cf927652056e14e..08bb5ea96d06d5b5de4c84b4ac9c1400e189b4d2
@@@ -11,11 -12,13 +12,15 @@@ class ArvadosModel < ActiveRecord::Bas
    before_create :ensure_permission_to_create
    before_update :ensure_permission_to_update
    before_destroy :ensure_permission_to_destroy
-   before_validation :maybe_update_modified_by_fields
 +
+   before_create :update_modified_by_fields
+   before_update :maybe_update_modified_by_fields
+   after_create :log_create
+   after_update :log_update
+   after_destroy :log_destroy
    validate :ensure_serialized_attribute_type
    validate :normalize_collection_uuids
 +  validate :ensure_valid_uuids
  
    has_many :permissions, :foreign_key => :head_uuid, :class_name => 'Link', :primary_key => :uuid, :conditions => "link_class = 'permission'"
  
index bf3c8d3eb43c537ad4fc1cdd5732b4db648c2e64,af0b533c7525c8ad20fcbfb252c5d25047d01662..923a681b0d3a03dc55f671b82063abc286a2601a
@@@ -12,7 -13,32 +12,31 @@@ class Log < ArvadosMode
      t.add :event_at
      t.add :event_type
      t.add :summary
-     t.add :info
+     t.add :properties
+   end
+   def fill_object(thing)
 -    self.object_kind ||= thing.kind
+     self.object_uuid ||= thing.uuid
+     self.summary ||= "#{self.event_type} of #{thing.uuid}"
+     self
+   end
+   def fill_properties(age, etag_prop, attrs_prop)
+     self.properties.merge!({"#{age}_etag" => etag_prop,
+                              "#{age}_attributes" => attrs_prop})
+   end
+   def update_to(thing)
+     fill_properties('new', thing.andand.etag, thing.andand.logged_attributes)
+     case event_type
+     when "create"
+       self.event_at = thing.created_at
+     when "update"
+       self.event_at = thing.modified_at
+     when "destroy"
+       self.event_at = Time.now
+     end
+     self
    end
  
    protected
    def set_default_event_at
      self.event_at ||= Time.now
    end
+   def log_change(event_type)
+     # Don't log changes to logs.
+   end
++
++  def ensure_valid_uuids
++    # logs can have references to deleted objects
++  end
  end
index 1ecd5d527c5d424ba2f30c0967d9bf33e8040964,0000000000000000000000000000000000000000..d77130d5c2cf3ae477bc729a7fb9ff15de045a2e
mode 100644,000000..100644
--- /dev/null
@@@ -1,21 -1,0 +1,27 @@@
 +class RemoveKindColumns < ActiveRecord::Migration
 +  include CurrentApiClient
 +
 +  def up
 +    remove_column :links, :head_kind
 +    remove_column :links, :tail_kind
++    remove_column :log, :object_kind
 +  end
 +
 +  def down
 +    add_column :links, :head_kind, :string
 +    add_column :links, :tail_kind, :string
++    add_column :log, :object_kind, :string
 +
 +    act_as_system_user do
 +      Link.all.each do |l|
 +        l.head_kind = ArvadosModel::resource_class_for_uuid(l.head_uuid).kind if l.head_uuid
 +        l.tail_kind = ArvadosModel::resource_class_for_uuid(l.tail_uuid).kind if l.tail_uuid
 +        l.save
 +      end
++      Log.all.each do |l|
++        l.object_kind = ArvadosModel::resource_class_for_uuid(l.object_uuid).kind if l.object_uuid
++        l.save
++      end
 +    end
 +  end
 +end
index a2b89e39116d58146c2144f4cea8d43636c1e190,2ef90d27322bc0cf7e52d3651b78d0925f619818..e2301e5be971717b13a58ec67ecea0fe3d4e365f
  #
  # It's strongly recommended to check this file into your version control system.
  
- ActiveRecord::Schema.define(:version => 20140402001908) do
+ ActiveRecord::Schema.define(:version => 20140407184311) do
  
    create_table "api_client_authorizations", :force => true do |t|
 -    t.string   "api_token",                                             :null => false
 -    t.integer  "api_client_id",                                         :null => false
 -    t.integer  "user_id",                                               :null => false
 +    t.string   "api_token",                                           :null => false
 +    t.integer  "api_client_id",                                       :null => false
 +    t.integer  "user_id",                                             :null => false
      t.string   "created_by_ip_address"
      t.string   "last_used_by_ip_address"
      t.datetime "last_used_at"
index f2afee2dc91a37e88e03b9ea802b9531d9d17d2f,28980335788414076a642c221f39a25fac6cbd65..3876775916f321ba6b7ed5269499f1123baf46f9
  require 'test_helper'
  
  class LogTest < ActiveSupport::TestCase
-   # test "the truth" do
-   #   assert true
-   # end
+   include CurrentApiClient
+   EVENT_TEST_METHODS = {
+     :create => [:created_at, :assert_nil, :assert_not_nil],
+     :update => [:modified_at, :assert_not_nil, :assert_not_nil],
+     :destroy => [nil, :assert_not_nil, :assert_nil],
+   }
+   def setup
+     @start_time = Time.now
+     @log_count = 1
+   end
+   def assert_properties(test_method, event, props, *keys)
+     verb = (test_method == :assert_nil) ? 'have nil' : 'define'
+     keys.each do |prop_name|
+       assert_includes(props, prop_name, "log properties missing #{prop_name}")
+       self.send(test_method, props[prop_name],
+                 "#{event.to_s} log should #{verb} #{prop_name}")
+     end
+   end
+   def get_logs_about(thing)
+     Log.where(object_uuid: thing.uuid).order("created_at ASC").all
+   end
+   def assert_logged(thing, event_type)
+     logs = get_logs_about(thing)
+     assert_equal(@log_count, logs.size, "log count mismatch")
+     @log_count += 1
+     log = logs.last
+     props = log.properties
+     assert_equal(current_user.andand.uuid, log.owner_uuid,
+                  "log is not owned by current user")
+     assert_equal(current_user.andand.uuid, log.modified_by_user_uuid,
+                  "log is not 'modified by' current user")
+     assert_equal(current_api_client.andand.uuid, log.modified_by_client_uuid,
+                  "log is not 'modified by' current client")
 -    assert_equal(thing.kind, log.object_kind, "log kind mismatch")
+     assert_equal(thing.uuid, log.object_uuid, "log UUID mismatch")
+     assert_equal(event_type.to_s, log.event_type, "log event type mismatch")
+     time_method, old_props_test, new_props_test = EVENT_TEST_METHODS[event_type]
+     if time_method.nil? or (timestamp = thing.send(time_method)).nil?
+       assert(log.event_at >= @start_time, "log timestamp too old")
+     else
+       assert_in_delta(timestamp, log.event_at, 1, "log timestamp mismatch")
+     end
+     assert_properties(old_props_test, event_type, props,
+                       'old_etag', 'old_attributes')
+     assert_properties(new_props_test, event_type, props,
+                       'new_etag', 'new_attributes')
+     yield props if block_given?
+   end
+   def assert_auth_logged_with_clean_properties(auth, event_type)
+     assert_logged(auth, event_type) do |props|
+       ['old_attributes', 'new_attributes'].map { |k| props[k] }.compact
+         .each do |attributes|
+         refute_includes(attributes, 'api_token',
+                         "auth log properties include sensitive API token")
+       end
+       yield props if block_given?
+     end
+   end
+   def set_user_from_auth(auth_name)
+     client_auth = api_client_authorizations(auth_name)
+     Thread.current[:api_client_authorization] = client_auth
+     Thread.current[:api_client] = client_auth.api_client
+     Thread.current[:user] = client_auth.user
+   end
+   test "creating a user makes a log" do
+     set_user_from_auth :admin_trustedclient
+     u = User.new(first_name: "Log", last_name: "Test")
+     u.save!
+     assert_logged(u, :create) do |props|
+       assert_equal(u.etag, props['new_etag'], "new user etag mismatch")
+       assert_equal(u.first_name, props['new_attributes']['first_name'],
+                    "new user first name mismatch")
+       assert_equal(u.last_name, props['new_attributes']['last_name'],
+                    "new user first name mismatch")
+     end
+   end
+   test "updating a virtual machine makes a log" do
+     set_user_from_auth :admin_trustedclient
+     vm = virtual_machines(:testvm)
+     orig_etag = vm.etag
+     vm.hostname = 'testvm.testshell'
+     vm.save!
+     assert_logged(vm, :update) do |props|
+       assert_equal(orig_etag, props['old_etag'], "updated VM old etag mismatch")
+       assert_equal(vm.etag, props['new_etag'], "updated VM new etag mismatch")
+       assert_equal('testvm.shell', props['old_attributes']['hostname'],
+                    "updated VM old name mismatch")
+       assert_equal('testvm.testshell', props['new_attributes']['hostname'],
+                    "updated VM new name mismatch")
+     end
+   end
+   test "destroying an authorization makes a log" do
+     set_user_from_auth :admin_trustedclient
+     auth = api_client_authorizations(:spectator)
+     orig_etag = auth.etag
+     orig_attrs = auth.attributes
+     orig_attrs.delete 'api_token'
+     auth.destroy
+     assert_logged(auth, :destroy) do |props|
+       assert_equal(orig_etag, props['old_etag'], "destroyed auth etag mismatch")
+       assert_equal(orig_attrs, props['old_attributes'],
+                    "destroyed auth attributes mismatch")
+     end
+   end
+   test "saving an unchanged client still makes a log" do
+     set_user_from_auth :admin_trustedclient
+     client = api_clients(:untrusted)
+     client.is_trusted = client.is_trusted
+     client.save!
+     assert_logged(client, :update) do |props|
+       ['old', 'new'].each do |age|
+         assert_equal(client.etag, props["#{age}_etag"],
+                      "unchanged client #{age} etag mismatch")
+         assert_equal(client.attributes, props["#{age}_attributes"],
+                      "unchanged client #{age} attributes mismatch")
+       end
+     end
+   end
+   test "updating a group twice makes two logs" do
+     set_user_from_auth :admin_trustedclient
+     group = groups(:empty_lonely_group)
+     name1 = group.name
+     name2 = "#{name1} under test"
+     group.name = name2
+     group.save!
+     assert_logged(group, :update) do |props|
+       assert_equal(name1, props['old_attributes']['name'],
+                    "group start name mismatch")
+       assert_equal(name2, props['new_attributes']['name'],
+                    "group updated name mismatch")
+     end
+     group.name = name1
+     group.save!
+     assert_logged(group, :update) do |props|
+       assert_equal(name2, props['old_attributes']['name'],
+                    "group pre-revert name mismatch")
+       assert_equal(name1, props['new_attributes']['name'],
+                    "group final name mismatch")
+     end
+   end
+   test "making a log doesn't get logged" do
+     set_user_from_auth :active_trustedclient
+     log = Log.new
+     log.save!
+     assert_equal(0, get_logs_about(log).size, "made a Log about a Log")
+   end
+   test "non-admins can't modify or delete logs" do
+     set_user_from_auth :active_trustedclient
+     log = Log.new(summary: "immutable log test")
+     assert_nothing_raised { log.save! }
+     log.summary = "log mutation test should fail"
+     assert_raise(ArvadosModel::PermissionDeniedError) { log.save! }
+     assert_raise(ArvadosModel::PermissionDeniedError) { log.destroy }
+   end
+   test "admins can modify and delete logs" do
+     set_user_from_auth :admin_trustedclient
+     log = Log.new(summary: "admin log mutation test")
+     assert_nothing_raised { log.save! }
+     log.summary = "admin mutated log test"
+     assert_nothing_raised { log.save! }
+     assert_nothing_raised { log.destroy }
+   end
+   test "failure saving log causes failure saving object" do
+     Log.class_eval do
+       alias_method :_orig_validations, :perform_validations
+       def perform_validations(options)
+         false
+       end
+     end
+     begin
+       set_user_from_auth :active_trustedclient
+       user = users(:active)
+       user.first_name = 'Test'
+       assert_raise(ActiveRecord::RecordInvalid) { user.save! }
+     ensure
+       Log.class_eval do
+         alias_method :perform_validations, :_orig_validations
+       end
+     end
+   end
+   test "don't log changes only to ApiClientAuthorization.last_used_*" do
+     set_user_from_auth :admin_trustedclient
+     auth = api_client_authorizations(:spectator)
+     start_log_count = get_logs_about(auth).size
+     auth.last_used_at = Time.now
+     auth.last_used_by_ip_address = '::1'
+     auth.save!
+     assert_equal(start_log_count, get_logs_about(auth).size,
+                  "log count changed after 'using' ApiClientAuthorization")
+     auth.created_by_ip_address = '::1'
+     auth.save!
+     assert_logged(auth, :update)
+   end
+   test "token isn't included in ApiClientAuthorization logs" do
+     set_user_from_auth :admin_trustedclient
+     auth = ApiClientAuthorization.new
+     auth.user = users(:spectator)
+     auth.api_client = api_clients(:untrusted)
+     auth.save!
+     assert_auth_logged_with_clean_properties(auth, :create)
+     auth.expires_at = Time.now
+     auth.save!
+     assert_auth_logged_with_clean_properties(auth, :update)
+     auth.destroy
+     assert_auth_logged_with_clean_properties(auth, :destroy)
+   end
  end