From afebaf439fc4ee659cc5ac446fe243b2826bad9c Mon Sep 17 00:00:00 2001
From: Roman Rizzi <rizziromanalejandro@gmail.com>
Date: Tue, 5 Jan 2021 10:29:10 -0300
Subject: [PATCH] FIX: Validate type when picking an avatar. (#11602)

This change improves the "UsersController#pick_avatar" validations to raise an error when "allow_uploaded_avatars" is disabled.
---
 .../app/templates/modal/avatar-selector.hbs   | 28 ++++++-------
 app/controllers/users_controller.rb           | 42 ++++++++++---------
 spec/requests/users_controller_spec.rb        | 23 ++++++++++
 3 files changed, 60 insertions(+), 33 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/templates/modal/avatar-selector.hbs b/app/assets/javascripts/discourse/app/templates/modal/avatar-selector.hbs
index 67429842efb..bb86ad93f63 100644
--- a/app/assets/javascripts/discourse/app/templates/modal/avatar-selector.hbs
+++ b/app/assets/javascripts/discourse/app/templates/modal/avatar-selector.hbs
@@ -12,21 +12,21 @@
       {{radio-button id="system-avatar" name="avatar" value="system" selection=selected}}
       <label class="radio" for="system-avatar">{{bound-avatar-template user.system_avatar_template "large"}} {{html-safe (i18n "user.change_avatar.letter_based")}}</label>
     </div>
-    <div class="avatar-choice">
-      {{radio-button id="gravatar" name="avatar" value="gravatar" selection=selected}}
-      <label class="radio" for="gravatar">{{bound-avatar-template user.gravatar_avatar_template "large"}} <span>{{html-safe (i18n "user.change_avatar.gravatar" gravatarName=gravatarName gravatarBaseUrl=gravatarBaseUrl gravatarLoginUrl=gravatarLoginUrl)}} {{user.email}}</span></label>
-
-      {{d-button action=(action "refreshGravatar")
-                 translatedTitle=(i18n "user.change_avatar.refresh_gravatar_title" gravatarName=gravatarName)
-                 disabled=gravatarRefreshDisabled
-                 icon="sync"
-                 class="btn-default avatar-selector-refresh-gravatar"}}
-
-      {{#if gravatarFailed}}
-        <p class="error">{{I18n "user.change_avatar.gravatar_failed" gravatarName=gravatarName}}</p>
-      {{/if}}
-    </div>
     {{#if allowAvatarUpload}}
+      <div class="avatar-choice">
+        {{radio-button id="gravatar" name="avatar" value="gravatar" selection=selected}}
+        <label class="radio" for="gravatar">{{bound-avatar-template user.gravatar_avatar_template "large"}} <span>{{html-safe (i18n "user.change_avatar.gravatar" gravatarName=gravatarName gravatarBaseUrl=gravatarBaseUrl gravatarLoginUrl=gravatarLoginUrl)}} {{user.email}}</span></label>
+
+        {{d-button action=(action "refreshGravatar")
+                   translatedTitle=(i18n "user.change_avatar.refresh_gravatar_title" gravatarName=gravatarName)
+                   disabled=gravatarRefreshDisabled
+                   icon="sync"
+                   class="btn-default avatar-selector-refresh-gravatar"}}
+
+        {{#if gravatarFailed}}
+          <p class="error">{{I18n "user.change_avatar.gravatar_failed" gravatarName=gravatarName}}</p>
+        {{/if}}
+      </div>
       <div class="avatar-choice">
         {{radio-button id="uploaded-avatar" name="avatar" value="uploaded" selection=selected}}
         <label class="radio" for="uploaded-avatar">
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 42700f31d5a..f5301bdd872 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1108,33 +1108,37 @@ class UsersController < ApplicationController
     user = fetch_user_from_params
     guardian.ensure_can_edit!(user)
 
-    type = params[:type]
-    upload_id = params[:upload_id]
-
     if SiteSetting.sso_overrides_avatar
       return render json: failed_json, status: 422
     end
 
-    if !SiteSetting.allow_uploaded_avatars
-      if type == "uploaded" || type == "custom"
-        return render json: failed_json, status: 422
-      end
+    type = params[:type]
+
+    invalid_type = type.present? && !AVATAR_TYPES_WITH_UPLOAD.include?(type) && type != 'system'
+    if invalid_type
+      return render json: failed_json, status: 422
     end
 
-    upload = Upload.find_by(id: upload_id)
-
-    # old safeguard
-    user.create_user_avatar unless user.user_avatar
-
-    guardian.ensure_can_pick_avatar!(user.user_avatar, upload)
-
-    if AVATAR_TYPES_WITH_UPLOAD.include?(type)
-
-      if !upload
-        return render_json_error I18n.t("avatar.missing")
+    if type.blank? || type == 'system'
+      upload_id = nil
+    else
+      if !SiteSetting.allow_uploaded_avatars
+        return render json: failed_json, status: 422
       end
 
-      if type == "gravatar"
+      upload_id = params[:upload_id]
+      upload = Upload.find_by(id: upload_id)
+
+      if upload.nil?
+        return render_json_error I18n.t('avatar.missing')
+      end
+
+      # old safeguard
+      user.create_user_avatar unless user.user_avatar
+
+      guardian.ensure_can_pick_avatar!(user.user_avatar, upload)
+
+      if type == 'gravatar'
         user.user_avatar.gravatar_upload_id = upload_id
       else
         user.user_avatar.custom_upload_id = upload_id
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index b3635d1439d..a5d4af55c16 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -2309,6 +2309,29 @@ describe UsersController do
         expect(response.status).to eq(422)
       end
 
+      it 'ignores the upload if picking a system avatar' do
+        SiteSetting.allow_uploaded_avatars = false
+        another_upload = Fabricate(:upload)
+
+        put "/u/#{user.username}/preferences/avatar/pick.json", params: {
+          upload_id: another_upload.id, type: "system"
+        }
+
+        expect(response.status).to eq(200)
+        expect(user.reload.uploaded_avatar_id).to eq(nil)
+      end
+
+      it 'raises an error if the type is invalid' do
+        SiteSetting.allow_uploaded_avatars = false
+        another_upload = Fabricate(:upload)
+
+        put "/u/#{user.username}/preferences/avatar/pick.json", params: {
+          upload_id: another_upload.id, type: "x"
+        }
+
+        expect(response.status).to eq(422)
+      end
+
       it 'can successfully pick the system avatar' do
         put "/u/#{user.username}/preferences/avatar/pick.json"