From 7ee9a6a7ec1b3054bcd0272221efa0dc5a9818df 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 d414d88525e..e72adb8c485 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 e5eba4498da..7571377566c 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