From 9563d020541c76858d75896d6dcdc460cba5b062 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Fri, 2 Feb 2024 09:32:45 +1000
Subject: [PATCH] FIX: Serialize uploaded_avatars_allowed_groups check on
 current user (#25515)

Checking group permissions on the client does not work,
since not all groups are serialized to the client all
the time. We can check `uploaded_avatars_allowed_groups`
on the server side and serialize to the current user
instead.
---
 .../app/components/modal/avatar-selector.hbs          |  6 +++---
 .../discourse/app/components/modal/avatar-selector.js | 11 ++---------
 .../tests/acceptance/user-preferences-account-test.js |  2 +-
 app/serializers/current_user_serializer.rb            |  5 +++++
 config/site_settings.yml                              |  1 -
 spec/system/page_objects/modals/avatar_selector.rb    | 11 ++++++++++-
 .../system/user_page/user_preferences_account_spec.rb |  9 ++++++++-
 .../user_page/user_preferences_interface_spec.rb      |  2 +-
 .../user_page/user_preferences_navigation_spec.rb     |  2 +-
 .../user_page/user_preferences_security_spec.rb       |  2 +-
 10 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/modal/avatar-selector.hbs b/app/assets/javascripts/discourse/app/components/modal/avatar-selector.hbs
index e14cba0b396..e8c82a839d6 100644
--- a/app/assets/javascripts/discourse/app/components/modal/avatar-selector.hbs
+++ b/app/assets/javascripts/discourse/app/components/modal/avatar-selector.hbs
@@ -17,11 +17,11 @@
           </a>
         {{/each}}
       </div>
-      {{#if this.showAvatarUploader}}
+      {{#if this.showCustomAvatarSelector}}
         <h4>{{i18n "user.change_avatar.use_custom"}}</h4>
       {{/if}}
     {{/if}}
-    {{#if this.showAvatarUploader}}
+    {{#if this.showCustomAvatarSelector}}
       {{#if this.user.use_logo_small_as_avatar}}
         <div class="avatar-choice">
           <RadioButton
@@ -128,7 +128,7 @@
   </:body>
 
   <:footer>
-    {{#if this.showAvatarUploader}}
+    {{#if this.showCustomAvatarSelector}}
       <DButton
         @action={{this.saveAvatarSelection}}
         @disabled={{this.submitDisabled}}
diff --git a/app/assets/javascripts/discourse/app/components/modal/avatar-selector.js b/app/assets/javascripts/discourse/app/components/modal/avatar-selector.js
index e25bf31f05a..c378a0095ea 100644
--- a/app/assets/javascripts/discourse/app/components/modal/avatar-selector.js
+++ b/app/assets/javascripts/discourse/app/components/modal/avatar-selector.js
@@ -41,7 +41,7 @@ export default class AvatarSelectorModal extends Component {
     return this.siteSettings.selectable_avatars_mode !== "disabled";
   }
 
-  get showAvatarUploader() {
+  get showCustomAvatarSelector() {
     const mode = this.siteSettings.selectable_avatars_mode;
     switch (mode) {
       case "no_one":
@@ -92,18 +92,11 @@ export default class AvatarSelectorModal extends Component {
 
   get allowAvatarUpload() {
     return (
-      this.siteSettingMatches &&
+      this.currentUser.can_upload_avatar &&
       allowsImages(this.currentUser.staff, this.siteSettings)
     );
   }
 
-  get siteSettingMatches() {
-    return this.siteSettings.userInAnyGroups(
-      "uploaded_avatars_allowed_groups",
-      this.currentUser
-    );
-  }
-
   @action
   onSelectedChanged(value) {
     this.selected = value;
diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-account-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-account-test.js
index 094582c69d9..b35d48375a6 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-account-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-account-test.js
@@ -15,7 +15,7 @@ import { cloneJSON } from "discourse-common/lib/object";
 import I18n from "discourse-i18n";
 
 acceptance("User Preferences - Account", function (needs) {
-  needs.user();
+  needs.user({ can_upload_avatar: true });
 
   let customUserProps = {};
   let pickAvatarRequestData = null;
diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb
index faa14d55ff9..25ad837a15f 100644
--- a/app/serializers/current_user_serializer.rb
+++ b/app/serializers/current_user_serializer.rb
@@ -19,6 +19,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :trust_level,
              :can_send_private_email_messages,
              :can_send_private_messages,
+             :can_upload_avatar,
              :can_edit,
              :can_invite_to_forum,
              :no_password,
@@ -120,6 +121,10 @@ class CurrentUserSerializer < BasicUserSerializer
     scope.can_send_private_messages?
   end
 
+  def can_upload_avatar
+    !is_anonymous && object.in_any_groups?(SiteSetting.uploaded_avatars_allowed_groups_map)
+  end
+
   def can_edit
     true
   end
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 4b2d41c37d2..1056ebae857 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1573,7 +1573,6 @@ files:
     enum: "TrustLevelAndStaffAndDisabledSetting"
     hidden: true
   uploaded_avatars_allowed_groups:
-    client: true
     default: 10
     type: group_list
     allow_any: false
diff --git a/spec/system/page_objects/modals/avatar_selector.rb b/spec/system/page_objects/modals/avatar_selector.rb
index b422bcf2b8d..e4decff2ca5 100644
--- a/spec/system/page_objects/modals/avatar_selector.rb
+++ b/spec/system/page_objects/modals/avatar_selector.rb
@@ -4,6 +4,7 @@ module PageObjects
     class AvatarSelector < PageObjects::Modals::Base
       BODY_SELECTOR = ".avatar-selector"
       MODAL_SELECTOR = ".avatar-selector-modal"
+      AVATAR_UPLOAD_BUTTON_SELECTOR = ".avatar-uploader__button"
 
       def select_avatar_upload_option
         body.choose("avatar", option: "custom")
@@ -14,7 +15,15 @@ module PageObjects
       end
 
       def click_avatar_upload_button
-        body.find_button(I18n.t("js.user.change_avatar.upload_title")).click
+        body.find(AVATAR_UPLOAD_BUTTON_SELECTOR).click
+      end
+
+      def has_avatar_upload_button?
+        has_css?(AVATAR_UPLOAD_BUTTON_SELECTOR)
+      end
+
+      def has_no_avatar_upload_button?
+        has_no_css?(AVATAR_UPLOAD_BUTTON_SELECTOR)
       end
 
       def has_user_avatar_image_uploaded?
diff --git a/spec/system/user_page/user_preferences_account_spec.rb b/spec/system/user_page/user_preferences_account_spec.rb
index 7093302cc13..ca1ffbd8339 100644
--- a/spec/system/user_page/user_preferences_account_spec.rb
+++ b/spec/system/user_page/user_preferences_account_spec.rb
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-describe "User preferences for Account", type: :system do
+describe "User preferences | Avatar", type: :system do
   fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
   let(:user_account_preferences_page) { PageObjects::Pages::UserPreferencesAccount.new }
   let(:avatar_selector_modal) { PageObjects::Modals::AvatarSelector.new }
@@ -25,5 +25,12 @@ describe "User preferences for Account", type: :system do
       expect(avatar_selector_modal).to be_closed
       expect(user_account_preferences_page).to have_system_avatar_image
     end
+
+    it "does not allow for custom pictures when the user is not in uploaded_avatars_allowed_groups" do
+      SiteSetting.uploaded_avatars_allowed_groups = Group::AUTO_GROUPS[:admins]
+      user_account_preferences_page.open_avatar_selector_modal(user)
+      expect(avatar_selector_modal).to be_open
+      expect(avatar_selector_modal).to have_no_avatar_upload_button
+    end
   end
 end
diff --git a/spec/system/user_page/user_preferences_interface_spec.rb b/spec/system/user_page/user_preferences_interface_spec.rb
index 54ff29d5e7b..84ab1530934 100644
--- a/spec/system/user_page/user_preferences_interface_spec.rb
+++ b/spec/system/user_page/user_preferences_interface_spec.rb
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-describe "User preferences for Interface", type: :system do
+describe "User preferences | Interface", type: :system do
   fab!(:user)
   let(:user_preferences_page) { PageObjects::Pages::UserPreferences.new }
   let(:user_preferences_interface_page) { PageObjects::Pages::UserPreferencesInterface.new }
diff --git a/spec/system/user_page/user_preferences_navigation_spec.rb b/spec/system/user_page/user_preferences_navigation_spec.rb
index a1abdbaca8d..7d5e9a0b088 100644
--- a/spec/system/user_page/user_preferences_navigation_spec.rb
+++ b/spec/system/user_page/user_preferences_navigation_spec.rb
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-describe "User page navigation menu", type: :system do
+describe "User preferences | Navigation menu", type: :system do
   fab!(:user)
   let(:everyone_group) { Group[:everyone] }
   let(:user_preferences_page) { PageObjects::Pages::UserPreferences.new }
diff --git a/spec/system/user_page/user_preferences_security_spec.rb b/spec/system/user_page/user_preferences_security_spec.rb
index ad4c9d892dd..b4998783c87 100644
--- a/spec/system/user_page/user_preferences_security_spec.rb
+++ b/spec/system/user_page/user_preferences_security_spec.rb
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-describe "User preferences for Security", type: :system do
+describe "User preferences | Security", type: :system do
   fab!(:password) { "kungfukenny" }
   fab!(:email) { "email@user.com" }
   fab!(:user) { Fabricate(:user, email: email, password: password) }