From: Lucas Di Pentima Date: Tue, 15 Sep 2020 14:56:33 +0000 (-0300) Subject: Merge branch '16826-unlogged-attrs-fix' X-Git-Tag: 2.1.0~62 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/e2d623bd4c686100772924b2b15ab808bbb147d0?hp=d6598fd6339e6219a7103781433356dfde546527 Merge branch '16826-unlogged-attrs-fix' Closes #16826 Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 6fb8ff2b33..709b4b4d1c 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -454,7 +454,7 @@ class ArvadosModel < ApplicationRecord end def logged_attributes - attributes.except(*Rails.configuration.AuditLogs.UnloggedAttributes.keys) + attributes.except(*Rails.configuration.AuditLogs.UnloggedAttributes.stringify_keys.keys) end def self.full_text_searchable_columns diff --git a/services/api/lib/config_loader.rb b/services/api/lib/config_loader.rb index cf16993ca5..f421fb5b2a 100644 --- a/services/api/lib/config_loader.rb +++ b/services/api/lib/config_loader.rb @@ -147,14 +147,14 @@ class ConfigLoader 'Ki' => 1 << 10, 'M' => 1000000, 'Mi' => 1 << 20, - "G" => 1000000000, - "Gi" => 1 << 30, - "T" => 1000000000000, - "Ti" => 1 << 40, - "P" => 1000000000000000, - "Pi" => 1 << 50, - "E" => 1000000000000000000, - "Ei" => 1 << 60, + "G" => 1000000000, + "Gi" => 1 << 30, + "T" => 1000000000000, + "Ti" => 1 << 40, + "P" => 1000000000000000, + "Pi" => 1 << 50, + "E" => 1000000000000000000, + "Ei" => 1 << 60, }[mt[2]] end end diff --git a/services/api/lib/enable_jobs_api.rb b/services/api/lib/enable_jobs_api.rb index 1a96a81ad6..cef76f08a5 100644 --- a/services/api/lib/enable_jobs_api.rb +++ b/services/api/lib/enable_jobs_api.rb @@ -2,16 +2,19 @@ # # SPDX-License-Identifier: AGPL-3.0 -Disable_update_jobs_api_method_list = {"jobs.create"=>{}, - "pipeline_instances.create"=>{}, - "pipeline_templates.create"=>{}, - "jobs.update"=>{}, - "pipeline_instances.update"=>{}, - "pipeline_templates.update"=>{}, - "job_tasks.create"=>{}, - "job_tasks.update"=>{}} +Disable_update_jobs_api_method_list = ConfigLoader.to_OrderedOptions({ + "jobs.create"=>{}, + "pipeline_instances.create"=>{}, + "pipeline_templates.create"=>{}, + "jobs.update"=>{}, + "pipeline_instances.update"=>{}, + "pipeline_templates.update"=>{}, + "job_tasks.create"=>{}, + "job_tasks.update"=>{} + }) -Disable_jobs_api_method_list = {"jobs.create"=>{}, +Disable_jobs_api_method_list = ConfigLoader.to_OrderedOptions({ + "jobs.create"=>{}, "pipeline_instances.create"=>{}, "pipeline_templates.create"=>{}, "jobs.get"=>{}, @@ -36,7 +39,7 @@ Disable_jobs_api_method_list = {"jobs.create"=>{}, "jobs.show"=>{}, "pipeline_instances.show"=>{}, "pipeline_templates.show"=>{}, - "job_tasks.show"=>{}} + "job_tasks.show"=>{}}) def check_enable_legacy_jobs_api # Create/update is permanently disabled (legacy functionality has been removed) diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb index ff89cd2129..f413188b54 100644 --- a/services/api/test/functional/arvados/v1/groups_controller_test.rb +++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb @@ -430,7 +430,8 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase end test 'get contents with jobs and pipeline instances disabled' do - Rails.configuration.API.DisabledAPIs = {'jobs.index'=>{}, 'pipeline_instances.index'=>{}} + Rails.configuration.API.DisabledAPIs = ConfigLoader.to_OrderedOptions( + {'jobs.index'=>{}, 'pipeline_instances.index'=>{}}) authorize_with :active get :contents, params: { diff --git a/services/api/test/functional/arvados/v1/schema_controller_test.rb b/services/api/test/functional/arvados/v1/schema_controller_test.rb index 3dd343b13c..764f3a8d1d 100644 --- a/services/api/test/functional/arvados/v1/schema_controller_test.rb +++ b/services/api/test/functional/arvados/v1/schema_controller_test.rb @@ -65,8 +65,8 @@ class Arvados::V1::SchemaControllerTest < ActionController::TestCase end test "non-empty disable_api_methods" do - Rails.configuration.API.DisabledAPIs = - {'jobs.create'=>{}, 'pipeline_instances.create'=>{}, 'pipeline_templates.create'=>{}} + Rails.configuration.API.DisabledAPIs = ConfigLoader.to_OrderedOptions( + {'jobs.create'=>{}, 'pipeline_instances.create'=>{}, 'pipeline_templates.create'=>{}}) get :index assert_response :success discovery_doc = JSON.parse(@response.body) diff --git a/services/api/test/functional/user_sessions_controller_test.rb b/services/api/test/functional/user_sessions_controller_test.rb index cd475dea4d..d979208d38 100644 --- a/services/api/test/functional/user_sessions_controller_test.rb +++ b/services/api/test/functional/user_sessions_controller_test.rb @@ -68,7 +68,7 @@ class UserSessionsControllerTest < ActionController::TestCase test "login to LoginCluster" do Rails.configuration.Login.LoginCluster = 'zbbbb' - Rails.configuration.RemoteClusters['zbbbb'] = {'Host' => 'zbbbb.example.com'} + Rails.configuration.RemoteClusters['zbbbb'] = ConfigLoader.to_OrderedOptions({'Host' => 'zbbbb.example.com'}) api_client_page = 'http://client.example.com/home' get :login, params: {return_to: api_client_page} assert_response :redirect diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb index c99a57aaff..5dc77cb98a 100644 --- a/services/api/test/test_helper.rb +++ b/services/api/test/test_helper.rb @@ -62,7 +62,7 @@ class ActiveSupport::TestCase include ArvadosTestSupport include CurrentApiClient - teardown do + setup do Thread.current[:api_client_ip_address] = nil Thread.current[:api_client_authorization] = nil Thread.current[:api_client_uuid] = nil @@ -72,6 +72,14 @@ class ActiveSupport::TestCase restore_configuration end + teardown do + # Confirm that any changed configuration doesn't include non-symbol keys + $arvados_config.keys.each do |conf_name| + conf = Rails.configuration.send(conf_name) + confirm_keys_as_symbols(conf, conf_name) if conf.respond_to?('keys') + end + end + def assert_equal(expect, *args) if expect.nil? assert_nil(*args) @@ -99,6 +107,14 @@ class ActiveSupport::TestCase end end + def confirm_keys_as_symbols(conf, conf_name) + assert(conf.is_a?(ActiveSupport::OrderedOptions), "#{conf_name} should be an OrderedOptions object") + conf.keys.each do |k| + assert(k.is_a?(Symbol), "Key '#{k}' on section '#{conf_name}' should be a Symbol") + confirm_keys_as_symbols(conf[k], "#{conf_name}.#{k}") if conf[k].respond_to?('keys') + end + end + def restore_configuration # Restore configuration settings changed during tests ConfigLoader.copy_into_config $arvados_config, Rails.configuration diff --git a/services/api/test/unit/application_test.rb b/services/api/test/unit/application_test.rb index 679dddf223..e1565ec627 100644 --- a/services/api/test/unit/application_test.rb +++ b/services/api/test/unit/application_test.rb @@ -7,7 +7,7 @@ require 'test_helper' class ApplicationTest < ActiveSupport::TestCase include CurrentApiClient - test "test act_as_system_user" do + test "act_as_system_user" do Thread.current[:user] = users(:active) assert_equal users(:active), Thread.current[:user] act_as_system_user do @@ -17,7 +17,7 @@ class ApplicationTest < ActiveSupport::TestCase assert_equal users(:active), Thread.current[:user] end - test "test act_as_system_user is exception safe" do + test "act_as_system_user is exception safe" do Thread.current[:user] = users(:active) assert_equal users(:active), Thread.current[:user] caught = false @@ -33,4 +33,12 @@ class ApplicationTest < ActiveSupport::TestCase assert caught assert_equal users(:active), Thread.current[:user] end + + test "config maps' keys are returned as symbols" do + assert Rails.configuration.Users.AutoSetupUsernameBlacklist.is_a? ActiveSupport::OrderedOptions + assert Rails.configuration.Users.AutoSetupUsernameBlacklist.keys.size > 0 + Rails.configuration.Users.AutoSetupUsernameBlacklist.keys.each do |k| + assert k.is_a? Symbol + end + end end diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb index addea83062..48cae5afee 100644 --- a/services/api/test/unit/collection_test.rb +++ b/services/api/test/unit/collection_test.rb @@ -1044,10 +1044,10 @@ class CollectionTest < ActiveSupport::TestCase end test "create collections with managed properties" do - Rails.configuration.Collections.ManagedProperties = { + Rails.configuration.Collections.ManagedProperties = ConfigLoader.to_OrderedOptions({ 'default_prop1' => {'Value' => 'prop1_value'}, 'responsible_person_uuid' => {'Function' => 'original_owner'} - } + }) # Test collection without initial properties act_as_user users(:active) do c = create_collection 'foo', Encoding::US_ASCII @@ -1076,9 +1076,9 @@ class CollectionTest < ActiveSupport::TestCase end test "update collection with protected managed properties" do - Rails.configuration.Collections.ManagedProperties = { + Rails.configuration.Collections.ManagedProperties = ConfigLoader.to_OrderedOptions({ 'default_prop1' => {'Value' => 'prop1_value', 'Protected' => true}, - } + }) act_as_user users(:active) do c = create_collection 'foo', Encoding::US_ASCII assert c.valid? diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index b91910d2d6..90de800b2f 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -576,7 +576,7 @@ class ContainerRequestTest < ActiveSupport::TestCase test "Container.resolve_container_image(pdh)" do set_user_from_auth :active [[:docker_image, 'v1'], [:docker_image_1_12, 'v2']].each do |coll, ver| - Rails.configuration.Containers.SupportedDockerImageFormats = {ver=>{}} + Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({ver=>{}}) pdh = collections(coll).portable_data_hash resolved = Container.resolve_container_image(pdh) assert_equal resolved, pdh @@ -602,7 +602,7 @@ class ContainerRequestTest < ActiveSupport::TestCase end test "migrated docker image" do - Rails.configuration.Containers.SupportedDockerImageFormats = {'v2'=>{}} + Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v2'=>{}}) add_docker19_migration_link # Test that it returns only v2 images even though request is for v1 image. @@ -620,7 +620,7 @@ class ContainerRequestTest < ActiveSupport::TestCase end test "use unmigrated docker image" do - Rails.configuration.Containers.SupportedDockerImageFormats = {'v1'=>{}} + Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v1'=>{}}) add_docker19_migration_link # Test that it returns only supported v1 images even though there is a @@ -639,7 +639,7 @@ class ContainerRequestTest < ActiveSupport::TestCase end test "incompatible docker image v1" do - Rails.configuration.Containers.SupportedDockerImageFormats = {'v1'=>{}} + Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v1'=>{}}) add_docker19_migration_link # Don't return unsupported v2 image even if we ask for it directly. @@ -652,7 +652,7 @@ class ContainerRequestTest < ActiveSupport::TestCase end test "incompatible docker image v2" do - Rails.configuration.Containers.SupportedDockerImageFormats = {'v2'=>{}} + Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v2'=>{}}) # No migration link, don't return unsupported v1 image, set_user_from_auth :active diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb index 0e8cc48538..c529aab8b6 100644 --- a/services/api/test/unit/job_test.rb +++ b/services/api/test/unit/job_test.rb @@ -117,7 +117,7 @@ class JobTest < ActiveSupport::TestCase 'locator' => BAD_COLLECTION, }.each_pair do |spec_type, image_spec| test "Job validation fails with nonexistent Docker image #{spec_type}" do - Rails.configuration.RemoteClusters = {} + Rails.configuration.RemoteClusters = ConfigLoader.to_OrderedOptions({}) job = Job.new job_attrs(runtime_constraints: {'docker_image' => image_spec}) assert(job.invalid?, "nonexistent Docker image #{spec_type} #{image_spec} was valid") diff --git a/services/api/test/unit/log_test.rb b/services/api/test/unit/log_test.rb index 016a0e4eb4..58f0cddb70 100644 --- a/services/api/test/unit/log_test.rb +++ b/services/api/test/unit/log_test.rb @@ -282,7 +282,7 @@ class LogTest < ActiveSupport::TestCase end test "non-empty configuration.unlogged_attributes" do - Rails.configuration.AuditLogs.UnloggedAttributes = {"manifest_text"=>{}} + Rails.configuration.AuditLogs.UnloggedAttributes = ConfigLoader.to_OrderedOptions({"manifest_text"=>{}}) txt = ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo\n" act_as_system_user do @@ -297,7 +297,7 @@ class LogTest < ActiveSupport::TestCase end test "empty configuration.unlogged_attributes" do - Rails.configuration.AuditLogs.UnloggedAttributes = {} + Rails.configuration.AuditLogs.UnloggedAttributes = ConfigLoader.to_OrderedOptions({}) txt = ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo\n" act_as_system_user do diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb index 7fcd36d709..b6d66230db 100644 --- a/services/api/test/unit/user_test.rb +++ b/services/api/test/unit/user_test.rb @@ -110,7 +110,7 @@ class UserTest < ActiveSupport::TestCase end test "new username set avoiding blacklist" do - Rails.configuration.Users.AutoSetupUsernameBlacklist = {"root"=>{}} + Rails.configuration.Users.AutoSetupUsernameBlacklist = ConfigLoader.to_OrderedOptions({"root"=>{}}) check_new_username_setting("root", "root2") end @@ -340,48 +340,52 @@ class UserTest < ActiveSupport::TestCase assert_equal(user.first_name, 'first_name_for_newly_created_user_updated') end + active_notify_list = ConfigLoader.to_OrderedOptions({"active-notify@example.com"=>{}}) + inactive_notify_list = ConfigLoader.to_OrderedOptions({"inactive-notify@example.com"=>{}}) + empty_notify_list = ConfigLoader.to_OrderedOptions({}) + test "create new user with notifications" do set_user_from_auth :admin - create_user_and_verify_setup_and_notifications true, {'active-notify-address@example.com'=>{}}, {'inactive-notify-address@example.com'=>{}}, nil, nil - create_user_and_verify_setup_and_notifications true, {'active-notify-address@example.com'=>{}}, {}, nil, nil - create_user_and_verify_setup_and_notifications true, {}, [], nil, nil - create_user_and_verify_setup_and_notifications false, {'active-notify-address@example.com'=>{}}, {'inactive-notify-address@example.com'=>{}}, nil, nil - create_user_and_verify_setup_and_notifications false, {}, {'inactive-notify-address@example.com'=>{}}, nil, nil - create_user_and_verify_setup_and_notifications false, {}, {}, nil, nil + create_user_and_verify_setup_and_notifications true, active_notify_list, inactive_notify_list, nil, nil + create_user_and_verify_setup_and_notifications true, active_notify_list, empty_notify_list, nil, nil + create_user_and_verify_setup_and_notifications true, empty_notify_list, empty_notify_list, nil, nil + create_user_and_verify_setup_and_notifications false, active_notify_list, inactive_notify_list, nil, nil + create_user_and_verify_setup_and_notifications false, empty_notify_list, inactive_notify_list, nil, nil + create_user_and_verify_setup_and_notifications false, empty_notify_list, empty_notify_list, nil, nil end [ # Easy inactive user tests. - [false, {}, {}, "inactive-none@example.com", false, false, "inactivenone"], - [false, {}, {}, "inactive-vm@example.com", true, false, "inactivevm"], - [false, {}, {}, "inactive-repo@example.com", false, true, "inactiverepo"], - [false, {}, {}, "inactive-both@example.com", true, true, "inactiveboth"], + [false, empty_notify_list, empty_notify_list, "inactive-none@example.com", false, false, "inactivenone"], + [false, empty_notify_list, empty_notify_list, "inactive-vm@example.com", true, false, "inactivevm"], + [false, empty_notify_list, empty_notify_list, "inactive-repo@example.com", false, true, "inactiverepo"], + [false, empty_notify_list, empty_notify_list, "inactive-both@example.com", true, true, "inactiveboth"], # Easy active user tests. - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "active-none@example.com", false, false, "activenone"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "active-vm@example.com", true, false, "activevm"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "active-repo@example.com", false, true, "activerepo"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "active-both@example.com", true, true, "activeboth"], + [true, active_notify_list, inactive_notify_list, "active-none@example.com", false, false, "activenone"], + [true, active_notify_list, inactive_notify_list, "active-vm@example.com", true, false, "activevm"], + [true, active_notify_list, inactive_notify_list, "active-repo@example.com", false, true, "activerepo"], + [true, active_notify_list, inactive_notify_list, "active-both@example.com", true, true, "activeboth"], # Test users with malformed e-mail addresses. - [false, {}, {}, nil, true, true, nil], - [false, {}, {}, "arvados", true, true, nil], - [false, {}, {}, "@example.com", true, true, nil], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "*!*@example.com", true, false, nil], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "*!*@example.com", false, false, nil], + [false, empty_notify_list, empty_notify_list, nil, true, true, nil], + [false, empty_notify_list, empty_notify_list, "arvados", true, true, nil], + [false, empty_notify_list, empty_notify_list, "@example.com", true, true, nil], + [true, active_notify_list, inactive_notify_list, "*!*@example.com", true, false, nil], + [true, active_notify_list, inactive_notify_list, "*!*@example.com", false, false, nil], # Test users with various username transformations. - [false, {}, {}, "arvados@example.com", false, false, "arvados2"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "arvados@example.com", false, false, "arvados2"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "root@example.com", true, false, "root2"], - [false, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "root@example.com", true, false, "root2"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "roo_t@example.com", false, true, "root2"], - [false, {}, {}, "^^incorrect_format@example.com", true, true, "incorrectformat"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "&4a_d9.@example.com", true, true, "ad9"], - [true, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "&4a_d9.@example.com", false, false, "ad9"], - [false, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "&4a_d9.@example.com", true, true, "ad9"], - [false, {"active-notify@example.com"=>{}}, {"inactive-notify@example.com"=>{}}, "&4a_d9.@example.com", false, false, "ad9"], + [false, empty_notify_list, empty_notify_list, "arvados@example.com", false, false, "arvados2"], + [true, active_notify_list, inactive_notify_list, "arvados@example.com", false, false, "arvados2"], + [true, active_notify_list, inactive_notify_list, "root@example.com", true, false, "root2"], + [false, active_notify_list, inactive_notify_list, "root@example.com", true, false, "root2"], + [true, active_notify_list, inactive_notify_list, "roo_t@example.com", false, true, "root2"], + [false, empty_notify_list, empty_notify_list, "^^incorrect_format@example.com", true, true, "incorrectformat"], + [true, active_notify_list, inactive_notify_list, "&4a_d9.@example.com", true, true, "ad9"], + [true, active_notify_list, inactive_notify_list, "&4a_d9.@example.com", false, false, "ad9"], + [false, active_notify_list, inactive_notify_list, "&4a_d9.@example.com", true, true, "ad9"], + [false, active_notify_list, inactive_notify_list, "&4a_d9.@example.com", false, false, "ad9"], ].each do |active, new_user_recipients, inactive_recipients, email, auto_setup_vm, auto_setup_repo, expect_username| test "create new user with auto setup #{active} #{email} #{auto_setup_vm} #{auto_setup_repo}" do set_user_from_auth :admin @@ -569,7 +573,6 @@ class UserTest < ActiveSupport::TestCase assert_not_nil resp_user, 'expected user object' assert_not_nil resp_user['uuid'], 'expected user object' assert_equal email, resp_user['email'], 'expected email not found' - end def verify_link (link_object, link_class, link_name, tail_uuid, head_uuid) @@ -648,7 +651,7 @@ class UserTest < ActiveSupport::TestCase if not new_user_recipients.empty? then assert_not_nil new_user_email, 'Expected new user email after setup' assert_equal Rails.configuration.Users.UserNotifierEmailFrom, new_user_email.from[0] - assert_equal new_user_recipients.keys.first, new_user_email.to[0] + assert_equal new_user_recipients.stringify_keys.keys.first, new_user_email.to[0] assert_equal new_user_email_subject, new_user_email.subject else assert_nil new_user_email, 'Did not expect new user email after setup' @@ -658,7 +661,7 @@ class UserTest < ActiveSupport::TestCase if not inactive_recipients.empty? then assert_not_nil new_inactive_user_email, 'Expected new inactive user email after setup' assert_equal Rails.configuration.Users.UserNotifierEmailFrom, new_inactive_user_email.from[0] - assert_equal inactive_recipients.keys.first, new_inactive_user_email.to[0] + assert_equal inactive_recipients.stringify_keys.keys.first, new_inactive_user_email.to[0] assert_equal "#{Rails.configuration.Users.EmailSubjectPrefix}New inactive user notification", new_inactive_user_email.subject else assert_nil new_inactive_user_email, 'Did not expect new inactive user email after setup' @@ -667,7 +670,6 @@ class UserTest < ActiveSupport::TestCase assert_nil new_inactive_user_email, 'Expected no inactive user email after setting up active user' end ActionMailer::Base.deliveries = [] - end def verify_link_exists link_exists, head_uuid, tail_uuid, link_class, link_name, property_name=nil, property_value=nil