From e2c23216344f12e2d181cdac3987a158ef9d626d Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 13 Dec 2018 16:26:07 +1100 Subject: [PATCH] SECURITY: do not delete avatars uploads when deleting accounts We rely on the clean up uploads job to do this safely --- app/models/user_avatar.rb | 5 ++--- spec/models/user_avatar_spec.rb | 8 +++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/models/user_avatar.rb b/app/models/user_avatar.rb index d414d88525..e72adb8c48 100644 --- a/app/models/user_avatar.rb +++ b/app/models/user_avatar.rb @@ -3,8 +3,8 @@ require_dependency 'upload_creator' class UserAvatar < ActiveRecord::Base belongs_to :user - belongs_to :gravatar_upload, class_name: 'Upload', dependent: :destroy - belongs_to :custom_upload, class_name: 'Upload', dependent: :destroy + belongs_to :gravatar_upload, class_name: 'Upload' + belongs_to :custom_upload, class_name: 'Upload' def contains_upload?(id) gravatar_upload_id == id || custom_upload_id == id @@ -46,7 +46,6 @@ class UserAvatar < ActiveRecord::Base user.update!(uploaded_avatar_id: upload.id) end - gravatar_upload&.destroy! self.update!(gravatar_upload: upload) end end diff --git a/spec/models/user_avatar_spec.rb b/spec/models/user_avatar_spec.rb index e5eba4498d..7571377566 100644 --- a/spec/models/user_avatar_spec.rb +++ b/spec/models/user_avatar_spec.rb @@ -30,6 +30,11 @@ describe UserAvatar do expect(avatar.gravatar_upload).to eq(Upload.last) expect(avatar.last_gravatar_download_attempt).to eq(Time.now) expect(user.reload.uploaded_avatar).to eq(nil) + + expect do + avatar.destroy + end.to_not change { Upload.count } + end describe 'when user has an existing custom upload' do @@ -57,7 +62,8 @@ describe UserAvatar do avatar.update_gravatar! - expect(Upload.find_by(id: upload.id)).to eq(nil) + # old upload to be cleaned up via clean_up_uploads + expect(Upload.find_by(id: upload.id)).not_to eq(nil) new_upload = Upload.last