From ff367e22fbd1744bd52ae1f3932c4c3965b39587 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 24 Aug 2021 10:46:28 +0300 Subject: [PATCH] FEATURE: Make allow_uploaded_avatars accept TL (#14091) This gives admins more control over who can upload custom profile pictures. --- .../app/controllers/avatar-selector.js | 19 ++++++-- .../discourse/tests/helpers/site-settings.js | 2 +- app/controllers/uploads_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- ...st_level_and_staff_and_disabled_setting.rb | 32 +++++++++++++ config/locales/server.en.yml | 1 + config/site_settings.yml | 3 +- ...819152920_change_allow_uploaded_avatars.rb | 19 ++++++++ lib/composer_messages_finder.rb | 2 +- .../composer_messages_finder_spec.rb | 2 +- spec/requests/uploads_controller_spec.rb | 4 +- spec/requests/users_controller_spec.rb | 48 +++++++++++++++++-- 12 files changed, 122 insertions(+), 14 deletions(-) create mode 100644 app/models/trust_level_and_staff_and_disabled_setting.rb create mode 100644 db/migrate/20210819152920_change_allow_uploaded_avatars.rb diff --git a/app/assets/javascripts/discourse/app/controllers/avatar-selector.js b/app/assets/javascripts/discourse/app/controllers/avatar-selector.js index 965207f83cb..83121e0468e 100644 --- a/app/assets/javascripts/discourse/app/controllers/avatar-selector.js +++ b/app/assets/javascripts/discourse/app/controllers/avatar-selector.js @@ -83,10 +83,23 @@ export default Controller.extend(ModalFunctionality, { } }, - @discourseComputed() - allowAvatarUpload() { + siteSettingMatches(value, user) { + switch (value) { + case "disabled": + return false; + case "staff": + return user.staff; + case "admin": + return user.admin; + default: + return user.trust_level >= parseInt(value, 10) || user.staff; + } + }, + + @discourseComputed("siteSettings.allow_uploaded_avatars") + allowAvatarUpload(allowUploadedAvatars) { return ( - this.siteSettings.allow_uploaded_avatars && + this.siteSettingMatches(allowUploadedAvatars, this.currentUser) && allowsImages(this.currentUser.staff, this.siteSettings) ); }, diff --git a/app/assets/javascripts/discourse/tests/helpers/site-settings.js b/app/assets/javascripts/discourse/tests/helpers/site-settings.js index 2b86811813b..d1e7ade6c6c 100644 --- a/app/assets/javascripts/discourse/tests/helpers/site-settings.js +++ b/app/assets/javascripts/discourse/tests/helpers/site-settings.js @@ -62,7 +62,7 @@ const ORIGINAL_SETTINGS = { max_image_width: 690, max_image_height: 500, allow_profile_backgrounds: true, - allow_uploaded_avatars: true, + allow_uploaded_avatars: "0", tl1_requires_read_posts: 30, enable_long_polling: true, polling_interval: 3000, diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index d2e1619ebf7..2048a9fc169 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -29,7 +29,7 @@ class UploadsController < ApplicationController # 50 characters ought to be enough for the upload type type = (params[:upload_type].presence || params[:type].presence).parameterize(separator: "_")[0..50] - if type == "avatar" && !me.admin? && (SiteSetting.discourse_connect_overrides_avatar || !SiteSetting.allow_uploaded_avatars) + if type == "avatar" && !me.admin? && (SiteSetting.discourse_connect_overrides_avatar || !TrustLevelAndStaffAndDisabledSetting.matches?(SiteSetting.allow_uploaded_avatars, me)) return render json: failed_json, status: 422 end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c09b993117f..83ee6ce9a34 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1138,7 +1138,7 @@ class UsersController < ApplicationController if type.blank? || type == 'system' upload_id = nil - elsif !SiteSetting.allow_uploaded_avatars + elsif !TrustLevelAndStaffAndDisabledSetting.matches?(SiteSetting.allow_uploaded_avatars, user) return render json: failed_json, status: 422 else upload_id = params[:upload_id] diff --git a/app/models/trust_level_and_staff_and_disabled_setting.rb b/app/models/trust_level_and_staff_and_disabled_setting.rb new file mode 100644 index 00000000000..3b1a4336c28 --- /dev/null +++ b/app/models/trust_level_and_staff_and_disabled_setting.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class TrustLevelAndStaffAndDisabledSetting < TrustLevelAndStaffSetting + def self.valid_value?(val) + valid_values.include?(val) || (val.to_i.to_s == val.to_s && valid_values.include?(val.to_i)) + end + + def self.valid_values + ['disabled'] + TrustLevel.valid_range.to_a + special_groups + end + + def self.translation(value) + if value == 'disabled' + I18n.t('site_settings.disabled') + else + super + end + end + + def self.matches?(value, user) + case value + when 'disabled' + false + when 'staff' + user.staff? + when 'admin' + user.admin? + else + user.has_trust_level?(value.to_i) || user.staff? + end + end +end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index f98b153a894..228e1aa2a15 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1450,6 +1450,7 @@ en: watched_word_regexp_error: "The regular expression for '%{action}' watched words is invalid. Please check your Watched Word settings, or disable the 'watched words regular expressions' site setting." site_settings: + disabled: "disabled" display_local_time_in_user_card: "Display the local time based on a user's timezone when their user card is opened." censored_words: "Words that will be automatically replaced with ■■■■" delete_old_hidden_posts: "Auto-delete any hidden posts that stay hidden for more than 30 days." diff --git a/config/site_settings.yml b/config/site_settings.yml index e9f63af61f5..d08c5bc1be9 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1365,7 +1365,8 @@ files: automatically_download_gravatars: true allow_uploaded_avatars: client: true - default: true + default: "0" + enum: "TrustLevelAndStaffAndDisabledSetting" default_avatars: default: "" type: url_list diff --git a/db/migrate/20210819152920_change_allow_uploaded_avatars.rb b/db/migrate/20210819152920_change_allow_uploaded_avatars.rb new file mode 100644 index 00000000000..6556d6cd31f --- /dev/null +++ b/db/migrate/20210819152920_change_allow_uploaded_avatars.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class ChangeAllowUploadedAvatars < ActiveRecord::Migration[6.1] + def up + execute <<~SQL + UPDATE site_settings + SET data_type = 7, value = (CASE WHEN value = 'f' THEN 'disabled' ELSE '0' END) + WHERE name = 'allow_uploaded_avatars' + SQL + end + + def down + execute <<~SQL + UPDATE site_settings + SET data_type = 5, value = (CASE WHEN value = 'disabled' THEN 'f' ELSE 't' END) + WHERE name = 'allow_uploaded_avatars' + SQL + end +end diff --git a/lib/composer_messages_finder.rb b/lib/composer_messages_finder.rb index 14c98917521..06ce89634e0 100644 --- a/lib/composer_messages_finder.rb +++ b/lib/composer_messages_finder.rb @@ -78,7 +78,7 @@ class ComposerMessagesFinder # - "disable avatar education message" is enabled # - "sso overrides avatar" is enabled # - "allow uploaded avatars" is disabled - return if SiteSetting.disable_avatar_education_message || SiteSetting.discourse_connect_overrides_avatar || !SiteSetting.allow_uploaded_avatars + return if SiteSetting.disable_avatar_education_message || SiteSetting.discourse_connect_overrides_avatar || !TrustLevelAndStaffAndDisabledSetting.matches?(SiteSetting.allow_uploaded_avatars, @user) # If we got this far, log that we've nagged them about the avatar UserHistory.create!(action: UserHistory.actions[:notified_about_avatar], target_user_id: @user.id) diff --git a/spec/components/composer_messages_finder_spec.rb b/spec/components/composer_messages_finder_spec.rb index 855b1875c41..15bb3e892fe 100644 --- a/spec/components/composer_messages_finder_spec.rb +++ b/spec/components/composer_messages_finder_spec.rb @@ -144,7 +144,7 @@ describe ComposerMessagesFinder do end it "doesn't notify users if 'allow_uploaded_avatars' setting is disabled" do - SiteSetting.allow_uploaded_avatars = false + SiteSetting.allow_uploaded_avatars = 'disabled' expect(finder.check_avatar_notification).to be_blank end end diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index c14e3f31638..737c992d522 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/spec/requests/uploads_controller_spec.rb @@ -126,7 +126,7 @@ describe UploadsController do end it 'ensures allow_uploaded_avatars is enabled when uploading an avatar' do - SiteSetting.allow_uploaded_avatars = false + SiteSetting.allow_uploaded_avatars = 'disabled' post "/uploads.json", params: { file: logo, type: "avatar" } expect(response.status).to eq(422) end @@ -139,7 +139,7 @@ describe UploadsController do it 'always allows admins to upload avatars' do sign_in(Fabricate(:admin)) - SiteSetting.allow_uploaded_avatars = false + SiteSetting.allow_uploaded_avatars = 'disabled' post "/uploads.json", params: { file: logo, type: "avatar" } expect(response.status).to eq(200) diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 943785b5f73..b22fe7349d3 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -2483,7 +2483,7 @@ describe UsersController do end it "raises an error when selecting the custom/uploaded avatar and allow_uploaded_avatars is disabled" do - SiteSetting.allow_uploaded_avatars = false + SiteSetting.allow_uploaded_avatars = 'disabled' put "/u/#{user.username}/preferences/avatar/pick.json", params: { upload_id: upload.id, type: "custom" } @@ -2491,8 +2491,50 @@ describe UsersController do expect(response.status).to eq(422) end + it "raises an error when selecting the custom/uploaded avatar and allow_uploaded_avatars is admin" do + SiteSetting.allow_uploaded_avatars = 'admin' + put "/u/#{user.username}/preferences/avatar/pick.json", params: { + upload_id: upload.id, type: "custom" + } + expect(response.status).to eq(422) + + user.update!(admin: true) + put "/u/#{user.username}/preferences/avatar/pick.json", params: { + upload_id: upload.id, type: "custom" + } + expect(response.status).to eq(200) + end + + it "raises an error when selecting the custom/uploaded avatar and allow_uploaded_avatars is staff" do + SiteSetting.allow_uploaded_avatars = 'staff' + put "/u/#{user.username}/preferences/avatar/pick.json", params: { + upload_id: upload.id, type: "custom" + } + expect(response.status).to eq(422) + + user.update!(moderator: true) + put "/u/#{user.username}/preferences/avatar/pick.json", params: { + upload_id: upload.id, type: "custom" + } + expect(response.status).to eq(200) + end + + it "raises an error when selecting the custom/uploaded avatar and allow_uploaded_avatars is a trust level" do + SiteSetting.allow_uploaded_avatars = '3' + put "/u/#{user.username}/preferences/avatar/pick.json", params: { + upload_id: upload.id, type: "custom" + } + expect(response.status).to eq(422) + + user.update!(trust_level: 3) + put "/u/#{user.username}/preferences/avatar/pick.json", params: { + upload_id: upload.id, type: "custom" + } + expect(response.status).to eq(200) + end + it 'ignores the upload if picking a system avatar' do - SiteSetting.allow_uploaded_avatars = false + SiteSetting.allow_uploaded_avatars = 'disabled' another_upload = Fabricate(:upload) put "/u/#{user.username}/preferences/avatar/pick.json", params: { @@ -2504,7 +2546,7 @@ describe UsersController do end it 'raises an error if the type is invalid' do - SiteSetting.allow_uploaded_avatars = false + SiteSetting.allow_uploaded_avatars = 'disabled' another_upload = Fabricate(:upload) put "/u/#{user.username}/preferences/avatar/pick.json", params: {