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.
discourse · Jan 5, 2021 · afebaf4 · afebaf4
1 parent 4567127
commit afebaf4
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 28 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
@@ -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>
+    {{#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"}}
+        {{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}}
+        {{#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
@@ -1108,33 +1108,37 @@ def pick_avatar
     user = fetch_user_from_params
     guardian.ensure_can_edit!(user)
 
+    if SiteSetting.sso_overrides_avatar
+      return render json: failed_json, status: 422
+    end
+
     type = params[:type]
-    upload_id = params[:upload_id]
 
-    if SiteSetting.sso_overrides_avatar
+    invalid_type = type.present? && !AVATAR_TYPES_WITH_UPLOAD.include?(type) && type != 'system'
+    if invalid_type
       return render json: failed_json, status: 422
     end
 
-    if !SiteSetting.allow_uploaded_avatars
-      if type == "uploaded" || type == "custom"
+    if type.blank? || type == 'system'
+      upload_id = nil
+    else
+      if !SiteSetting.allow_uploaded_avatars
         return render json: failed_json, status: 422
       end
-    end
-
-    upload = Upload.find_by(id: upload_id)
 
-    # old safeguard
-    user.create_user_avatar unless user.user_avatar
+      upload_id = params[:upload_id]
+      upload = Upload.find_by(id: upload_id)
 
-    guardian.ensure_can_pick_avatar!(user.user_avatar, upload)
+      if upload.nil?
+        return render_json_error I18n.t('avatar.missing')
+      end
 
-    if AVATAR_TYPES_WITH_UPLOAD.include?(type)
+      # old safeguard
+      user.create_user_avatar unless user.user_avatar
 
-      if !upload
-        return render_json_error I18n.t("avatar.missing")
-      end
+      guardian.ensure_can_pick_avatar!(user.user_avatar, upload)
 
-      if type == "gravatar"
+      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
@@ -2309,6 +2309,29 @@ def post_user(extra_params = {})
         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"