From 46fc57222f24add74d2d075eb36663565967622c Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Sat, 2 Jun 2018 19:27:52 +0530 Subject: [PATCH] FEATURE: improve handling of site setting secrets --- .../admin/mixins/setting-component.js.es6 | 9 ++++++-- .../templates/components/site-setting.hbs | 5 +++- .../components/site-settings/string.hbs | 2 ++ .../stylesheets/common/admin/admin_base.scss | 2 +- config/site_settings.yml | 17 +++++++++++--- lib/site_setting_extension.rb | 13 +++++++++-- .../components/site_setting_extension_spec.rb | 23 ++++++++++++++++++- 7 files changed, 61 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/admin/mixins/setting-component.js.es6 b/app/assets/javascripts/admin/mixins/setting-component.js.es6 index 71fc679f8d0..e0da9d0a824 100644 --- a/app/assets/javascripts/admin/mixins/setting-component.js.es6 +++ b/app/assets/javascripts/admin/mixins/setting-component.js.es6 @@ -16,6 +16,7 @@ export default Ember.Mixin.create({ classNameBindings: [':row', ':setting', 'setting.overridden', 'typeClass'], content: Ember.computed.alias('setting'), validationMessage: null, + isSecret: Ember.computed.oneWay('setting.secret'), @computed("buffered.value", "setting.value") dirty(bufferVal, settingVal) { @@ -95,13 +96,17 @@ export default Ember.Mixin.create({ }); }, + cancel() { + this.rollbackBuffer(); + }, + resetDefault() { this.set('buffered.value', this.get('setting.default')); this._save(); }, - cancel() { - this.rollbackBuffer(); + toggleSecret() { + this.toggleProperty('isSecret'); } } }); diff --git a/app/assets/javascripts/admin/templates/components/site-setting.hbs b/app/assets/javascripts/admin/templates/components/site-setting.hbs index e91922f7a77..03e5433fd05 100644 --- a/app/assets/javascripts/admin/templates/components/site-setting.hbs +++ b/app/assets/javascripts/admin/templates/components/site-setting.hbs @@ -2,7 +2,7 @@

{{unbound settingName}}

- {{component componentName setting=setting value=buffered.value validationMessage=validationMessage preview=preview}} + {{component componentName setting=setting value=buffered.value validationMessage=validationMessage preview=preview isSecret=isSecret}}
{{#if dirty}}
@@ -10,5 +10,8 @@ {{d-button class="cancel" action="cancel" icon="times"}}
{{else if setting.overridden}} + {{#if setting.secret}} + {{d-button action="toggleSecret" icon="eye-slash"}} + {{/if}} {{d-button action="resetDefault" icon="undo" label="admin.settings.reset"}} {{/if}} diff --git a/app/assets/javascripts/admin/templates/components/site-settings/string.hbs b/app/assets/javascripts/admin/templates/components/site-settings/string.hbs index 77e254b42f2..a2c53a1ee6f 100644 --- a/app/assets/javascripts/admin/templates/components/site-settings/string.hbs +++ b/app/assets/javascripts/admin/templates/components/site-settings/string.hbs @@ -1,5 +1,7 @@ {{#if setting.textarea}} {{textarea value=value classNames="input-setting-textarea"}} +{{else if isSecret}} + {{input type="password" value=value classNames="input-setting-string"}} {{else}} {{text-field value=value classNames="input-setting-string"}} {{/if}} diff --git a/app/assets/stylesheets/common/admin/admin_base.scss b/app/assets/stylesheets/common/admin/admin_base.scss index 5557f7c795e..9ec5ee5f9e2 100644 --- a/app/assets/stylesheets/common/admin/admin_base.scss +++ b/app/assets/stylesheets/common/admin/admin_base.scss @@ -575,7 +575,7 @@ $mobile-breakpoint: 700px; } .setting.overridden.string { - input[type=text], textarea { + input[type=text], input[type=password], textarea { background-color: $highlight-medium; } } diff --git a/config/site_settings.yml b/config/site_settings.yml index 6686036b76e..6b4bd9d0a1d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -8,6 +8,7 @@ # regex - A regex that the value must match. # validator - The name of the class that will be use to validate the value of the setting. # allow_any - For choice settings allow items not specified in the choice list (default true) +# secret - Set to true if input type should be password and value needs to be scrubbed from logs (default false). # enum - The setting has a fixed set of allowed values, and only one can be chosen. # Set to the class name that defines the set. # shadowed_by_global - "Shadow" a site setting with a GlobalSetting. If the GlobalSetting @@ -274,7 +275,9 @@ login: client: true default: false google_oauth2_client_id: '' - google_oauth2_client_secret: '' + google_oauth2_client_secret: + default: '' + secret: true google_oauth2_prompt: default: '' type: list @@ -297,6 +300,7 @@ login: twitter_consumer_secret: default: '' regex: "^[\\w+-]+$" + secret: true enable_instagram_logins: client: true default: false @@ -306,6 +310,7 @@ login: instagram_consumer_secret: default: '' regex: "^[a-z0-9]+$" + secret: true enable_facebook_logins: client: true default: false @@ -315,6 +320,7 @@ login: facebook_app_secret: default: '' regex: "^[a-f0-9]+$" + secret: true enable_github_logins: client: true default: false @@ -324,6 +330,7 @@ login: github_client_secret: default: '' regex: "^[a-f0-9]+$" + secret: true enable_sso: client: true default: false @@ -337,7 +344,9 @@ login: sso_url: default: '' regex: '^https?:\/\/.+[^\/]$' - sso_secret: '' + sso_secret: + default: '' + secret: true sso_overrides_groups: false sso_overrides_bio: false sso_overrides_email: @@ -884,7 +893,9 @@ files: client: true s3_use_iam_profile: false s3_access_key_id: '' - s3_secret_access_key: '' + s3_secret_access_key: + default: '' + secret: true s3_region: default: 'us-east-1' enum: 'S3RegionSiteSetting' diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 412ded8bfdd..8efee070324 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -66,6 +66,10 @@ module SiteSettingExtension @previews ||= {} end + def secret_settings + @secret_settings ||= [] + end + def setting(name_arg, default = nil, opts = {}) name = name_arg.to_sym @@ -106,6 +110,10 @@ module SiteSettingExtension previews[name] = opts[:preview] end + if opts[:secret] + secret_settings << name + end + type_supervisor.load_setting( name, opts.extract!(*SiteSettings::TypeSupervisor::CONSUMED_OPTS) @@ -149,7 +157,8 @@ module SiteSettingExtension default: defaults[s].to_s, value: value.to_s, category: categories[s], - preview: previews[s] + preview: previews[s], + secret: secret_settings.include?(s) }.merge(type_supervisor.type_hash(s)) opts @@ -284,7 +293,7 @@ module SiteSettingExtension prev_value = send(name) set(name, value) if has_setting?(name) - value = prev_value = "[FILTERED]" if name.to_s =~ /_secret/ + value = prev_value = "[FILTERED]" if secret_settings.include?(name.to_sym) StaffActionLogger.new(user).log_site_setting_change(name, prev_value, value) end end diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb index 4171b9db4cb..1c8491c0099 100644 --- a/spec/components/site_setting_extension_spec.rb +++ b/spec/components/site_setting_extension_spec.rb @@ -439,7 +439,7 @@ describe SiteSettingExtension do describe ".set_and_log" do before do - settings.setting(:s3_secret_access_key, "old_secret_key") + settings.setting(:s3_secret_access_key, "old_secret_key", secret: true) settings.setting(:title, "Discourse v1") settings.refresh! end @@ -592,6 +592,27 @@ describe SiteSettingExtension do end end + describe "secret" do + before do + settings.setting(:superman_identity, 'Clark Kent', secret: true) + settings.refresh! + end + + it "is in the `secret_settings` collection" do + expect(settings.secret_settings.include?(:superman_identity)).to eq(true) + end + + it "can be retrieved" do + expect(settings.superman_identity).to eq("Clark Kent") + end + + it "is present in all_settings by default" do + secret_setting = settings.all_settings.find { |s| s[:setting] == :superman_identity } + expect(secret_setting).to be_present + expect(secret_setting[:secret]).to eq(true) + end + end + describe 'locale default overrides are respected' do before do settings.setting(:test_override, 'default', locale_default: { zh_CN: 'cn' })