From 2c1279b74035c2def23504a5cd69ec1b16d6ce82 Mon Sep 17 00:00:00 2001 From: Marica Odagaki Date: Wed, 15 Feb 2017 00:04:10 -0800 Subject: [PATCH 1/5] Fix typo to be more consistent with other test descriptions --- spec/models/admin_dashboard_data_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/admin_dashboard_data_spec.rb b/spec/models/admin_dashboard_data_spec.rb index 6a0cd5a3ca6..5ba83539984 100644 --- a/spec/models/admin_dashboard_data_spec.rb +++ b/spec/models/admin_dashboard_data_spec.rb @@ -138,7 +138,7 @@ describe AdminDashboardData do SiteSetting.stubs(enable_setting).returns(true) end - it 'returns nil key and secret are set' do + it 'returns nil when key and secret are set' do SiteSetting.stubs(key).returns('12313213') SiteSetting.stubs(secret).returns('12312313123') expect(subject).to be_nil From af9c97ec43a9c7bb1f1e14896e2af1db95b013bb Mon Sep 17 00:00:00 2001 From: Marica Odagaki Date: Wed, 15 Feb 2017 00:05:58 -0800 Subject: [PATCH 2/5] Add failing tests --- spec/models/admin_dashboard_data_spec.rb | 92 ++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/spec/models/admin_dashboard_data_spec.rb b/spec/models/admin_dashboard_data_spec.rb index 5ba83539984..f6e1a2c2940 100644 --- a/spec/models/admin_dashboard_data_spec.rb +++ b/spec/models/admin_dashboard_data_spec.rb @@ -189,6 +189,98 @@ describe AdminDashboardData do end end + describe 's3_config_check' do + shared_examples 'problem detection for s3-dependent setting' do + subject { described_class.new.s3_config_check } + let(:access_keys) { [:s3_access_key_id, :s3_secret_access_key] } + let(:all_cred_keys) { access_keys + [:s3_use_iam_profile] } + let(:all_setting_keys) { all_cred_keys + [bucket_key] } + + def all_setting_permutations(keys) + ['a', ''].repeated_permutation(keys.size) do |*values| + hash = Hash[keys.zip(values)] + hash.each do |key,value| + SiteSetting.stubs(key).returns(value) + end + yield hash + end + end + + context 'when setting is enabled' do + let(:setting_enabled) { true } + before do + SiteSetting.stubs(setting_key).returns(setting_enabled) + SiteSetting.stubs(bucket_key).returns(bucket_value) + end + + context 'when bucket is blank' do + let(:bucket_value) { '' } + + it "always returns a string" do + all_setting_permutations(all_cred_keys) do + expect(subject).to_not be_nil + end + end + end + + context 'when bucket is filled in' do + let(:bucket_value) { 'a' } + before do + SiteSetting.stubs(:s3_use_iam_profile).returns(use_iam_profile) + end + + context 'when using iam profile' do + let(:use_iam_profile) { true } + + it 'always returns nil' do + all_setting_permutations(access_keys) do + expect(subject).to be_nil + end + end + end + + context 'when not using iam profile' do + let(:use_iam_profile) { false } + + it 'returns nil only if both access key fields are filled in' do + all_setting_permutations(access_keys) do |settings| + if settings.values.all? + expect(subject).to be_nil + else + expect(subject).to_not be_nil + end + end + end + end + end + end + + context 'when setting is not enabled' do + before do + SiteSetting.stubs(setting_key).returns(false) + end + + it "always returns nil" do + all_setting_permutations(all_setting_keys) do + expect(subject).to be_nil + end + end + end + end + + describe 'uploads' do + let(:setting_key) { :enable_s3_uploads } + let(:bucket_key) { :s3_upload_bucket } + include_examples 'problem detection for s3-dependent setting' + end + + describe 'backups' do + let(:setting_key) { :enable_s3_backups } + let(:bucket_key) { :s3_backup_bucket } + include_examples 'problem detection for s3-dependent setting' + end + end + describe 'stats cache' do include_examples 'stats cachable' end From 3bb1b98b0ec5b3105eac53674d888a3d47c86680 Mon Sep 17 00:00:00 2001 From: Marica Odagaki Date: Wed, 15 Feb 2017 00:08:32 -0800 Subject: [PATCH 3/5] FIX: admin dashboard shouldn't complain when using iam profile for s3 access Previous code wasn't working as intended because it was parsed as (bad_keys = (access_key or secret_key)) and !use_iam_profile because of Ruby's operator precedence: `=` binds more eagerly than `and`. http://ruby-doc.org/core-2.3.1/doc/syntax/precedence_rdoc.html See also: https://github.com/bbatsov/ruby-style-guide#no-and-or-or --- app/models/admin_dashboard_data.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index 5ac4bd55c35..b6b0b32ded4 100644 --- a/app/models/admin_dashboard_data.rb +++ b/app/models/admin_dashboard_data.rb @@ -198,7 +198,7 @@ class AdminDashboardData end def s3_config_check - bad_keys = (SiteSetting.s3_access_key_id.blank? or SiteSetting.s3_secret_access_key.blank?) and !SiteSetting.s3_use_iam_profile + bad_keys = (SiteSetting.s3_access_key_id.blank? || SiteSetting.s3_secret_access_key.blank?) && !SiteSetting.s3_use_iam_profile return I18n.t('dashboard.s3_config_warning') if SiteSetting.enable_s3_uploads and (bad_keys or SiteSetting.s3_upload_bucket.blank?) return I18n.t('dashboard.s3_backup_config_warning') if SiteSetting.enable_s3_backups and (bad_keys or SiteSetting.s3_backup_bucket.blank?) From 22e3db703e5fe9164f118eef101ddf919bea3d7b Mon Sep 17 00:00:00 2001 From: Marica Odagaki Date: Wed, 15 Feb 2017 00:09:13 -0800 Subject: [PATCH 4/5] Mention s3_use_iam_profile in problem messages --- config/locales/server.en.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 090b3bca008..480d136d33f 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -871,8 +871,8 @@ en: facebook_config_warning: 'The server is configured to allow signup and log in with Facebook (enable_facebook_logins), but the app id and app secret values are not set. Go to the Site Settings and update the settings. See this guide to learn more.' twitter_config_warning: 'The server is configured to allow signup and log in with Twitter (enable_twitter_logins), but the key and secret values are not set. Go to the Site Settings and update the settings. See this guide to learn more.' github_config_warning: 'The server is configured to allow signup and log in with GitHub (enable_github_logins), but the client id and secret values are not set. Go to the Site Settings and update the settings. See this guide to learn more.' - s3_config_warning: 'The server is configured to upload files to s3, but at least one the following setting is not set: s3_access_key_id, s3_secret_access_key or s3_upload_bucket. Go to the Site Settings and update the settings. See "How to set up image uploads to S3?" to learn more.' - s3_backup_config_warning: 'The server is configured to upload backups to s3, but at least one the following setting is not set: s3_access_key_id, s3_secret_access_key or s3_backup_bucket. Go to the Site Settings and update the settings. See "How to set up image uploads to S3?" to learn more.' + s3_config_warning: 'The server is configured to upload files to s3, but at least one the following setting is not set: s3_access_key_id, s3_secret_access_key, s3_use_iam_profile, or s3_upload_bucket. Go to the Site Settings and update the settings. See "How to set up image uploads to S3?" to learn more.' + s3_backup_config_warning: 'The server is configured to upload backups to s3, but at least one the following setting is not set: s3_access_key_id, s3_secret_access_key, s3_use_iam_profile, or s3_backup_bucket. Go to the Site Settings and update the settings. See "How to set up image uploads to S3?" to learn more.' image_magick_warning: 'The server is configured to create thumbnails of large images, but ImageMagick is not installed. Install ImageMagick using your favorite package manager or download the latest release.' failing_emails_warning: 'There are %{num_failed_jobs} email jobs that failed. Check your app.yml and ensure that the mail server settings are correct. See the failed jobs in Sidekiq.' subfolder_ends_in_slash: "Your subfolder setup is incorrect; the DISCOURSE_RELATIVE_URL_ROOT ends in a slash." From a9a585f66a931ac50085fa88e2b2fe025e900f43 Mon Sep 17 00:00:00 2001 From: Marica Odagaki Date: Wed, 15 Feb 2017 00:21:17 -0800 Subject: [PATCH 5/5] Use && and || consistently so that there's less chance of copy paste errors in the future --- app/models/admin_dashboard_data.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index b6b0b32ded4..b4113d609ea 100644 --- a/app/models/admin_dashboard_data.rb +++ b/app/models/admin_dashboard_data.rb @@ -169,7 +169,7 @@ class AdminDashboardData def sidekiq_check last_job_performed_at = Jobs.last_job_performed_at - I18n.t('dashboard.sidekiq_warning') if Jobs.queued > 0 and (last_job_performed_at.nil? or last_job_performed_at < 2.minutes.ago) + I18n.t('dashboard.sidekiq_warning') if Jobs.queued > 0 && (last_job_performed_at.nil? || last_job_performed_at < 2.minutes.ago) end def queue_size_check @@ -178,7 +178,7 @@ class AdminDashboardData end def ram_check - I18n.t('dashboard.memory_warning') if MemInfo.new.mem_total and MemInfo.new.mem_total < 1_000_000 + I18n.t('dashboard.memory_warning') if MemInfo.new.mem_total && MemInfo.new.mem_total < 1_000_000 end def google_oauth2_config_check @@ -190,23 +190,23 @@ class AdminDashboardData end def twitter_config_check - I18n.t('dashboard.twitter_config_warning') if SiteSetting.enable_twitter_logins and (SiteSetting.twitter_consumer_key.blank? or SiteSetting.twitter_consumer_secret.blank?) + I18n.t('dashboard.twitter_config_warning') if SiteSetting.enable_twitter_logins && (SiteSetting.twitter_consumer_key.blank? || SiteSetting.twitter_consumer_secret.blank?) end def github_config_check - I18n.t('dashboard.github_config_warning') if SiteSetting.enable_github_logins and (SiteSetting.github_client_id.blank? or SiteSetting.github_client_secret.blank?) + I18n.t('dashboard.github_config_warning') if SiteSetting.enable_github_logins && (SiteSetting.github_client_id.blank? || SiteSetting.github_client_secret.blank?) end def s3_config_check bad_keys = (SiteSetting.s3_access_key_id.blank? || SiteSetting.s3_secret_access_key.blank?) && !SiteSetting.s3_use_iam_profile - return I18n.t('dashboard.s3_config_warning') if SiteSetting.enable_s3_uploads and (bad_keys or SiteSetting.s3_upload_bucket.blank?) - return I18n.t('dashboard.s3_backup_config_warning') if SiteSetting.enable_s3_backups and (bad_keys or SiteSetting.s3_backup_bucket.blank?) + return I18n.t('dashboard.s3_config_warning') if SiteSetting.enable_s3_uploads && (bad_keys || SiteSetting.s3_upload_bucket.blank?) + return I18n.t('dashboard.s3_backup_config_warning') if SiteSetting.enable_s3_backups && (bad_keys || SiteSetting.s3_backup_bucket.blank?) nil end def image_magick_check - I18n.t('dashboard.image_magick_warning') if SiteSetting.create_thumbnails and !system("command -v convert >/dev/null;") + I18n.t('dashboard.image_magick_warning') if SiteSetting.create_thumbnails && !system("command -v convert >/dev/null;") end def failing_emails_check